Align development test helpers with 3.6

[davidhorstmann-arm]

Align the tests/src and tests/include directories in development with mbedtls-3.6 in preparation to move these to the framework.

After this PR and its backport #9547 are merged, the tests/src and tests/include directories should be identical in development and mbedtls-3.6, except for the following 2 cases:

• tests/src/psa_test_wrappers.c and tests/include/test/psa_test_wrappers.h differ because these files are generated based on the supported PSA featureset. These will not be moved to the framework, at least for the time being.
• Headers in tests/include/alt-dummy/* are related to the 3.6-only _ALT interface, so will not be moved to the framework and are only present in 3.6.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line. If the provided content is part of the present PR remove the # symbol.

• [x] changelog not required because: No user-facing changes
• [x] development PR here
• [x] framework PR not required - no framework change
• [x] 3.6 PR #9547
• [x] 2.28 PR not required because: No test helper alignment required in 2.28 (as no framework)
• tests not required because: No code / feature changes, existing tests suffice.

[ronald-cron-arm]

I've just had a quick look. I'd say that we do not really need to check the minor part of the version thus I would use #if (MBEDTLS_VERSION_MAJOR == 3) rather than#if ((MBEDTLS_VERSION_MAJOR == 3) && (MBEDTLS_VERSION_MINOR == 6). @mpg what do you think?

[mpg]

I'll note that currently in development VERSION_MAJOR is still 3 and VERSION_MINOR is still 6, so the current condition does not distinguish between development and actual 3.6 - it will evaluate to true in both cases.

So, I'd say if we want to be able to distinguish now, we need to change something in development: we could either immediately switch to 4.0 (then I'd agree we only need to check VERSION_MAJOR), or if we're not comfortable with bumping the version number to that value before it's actually done (note: normally, bumping the version is part of the release process), use some intermediate like 3.99 - and then we need to check VERSION_MINOR too as the PR is doing.

Or we could even go a different route and not use MBEDTLS_VERSION_MAJOR/MINOR but instead have a dedicated macro that's defined only in development or only in the LTS branch. (MBEDTLS_VERSION_DEVELOPMENT vs MBEDTLS_VERSION_36LTS for example. Or perhaps MBEDTLS_BRANCH_xxx instead of MBEDTLS_VERSION actually.)

Wdyt?

[gilles-peskine-arm]

we could either immediately switch to 4.0 (then I'd agree we only need to check VERSION_MAJOR)

We've already made backward-incompatible changes in development, so arguably we should already have bumped the major version.

[davidhorstmann-arm]

@mpg thanks, I missed that and indeed the CI is failing because development is detected as 3.6. For now I've bumped the version to 3.9.9 in this PR for testing purposes.

As for the final state of this PR, I tend to think keeping the version as 3.9.9 is the best approach because it keeps our release process as it is and allows us to mark the release with an execution of bump_version.sh. However, I see Gilles' point that since we are currently backwards-incompatible with 3.6 it makes no sense for us to have a 3.x version number.

As I've mentioned before, I'm somewhat sceptical about the need for us to move test helpers to the framework at all, since at the moment version control effectively does this per-branch functionality-gating for us, but that seems like a separate discussion which may be out of scope for this PR.

[mpg]

We've already made backward-incompatible changes in development, so arguably we should already have bumped the major version.

I'm not convinced the rules of semantic versioning are supposed to apply to unreleased branches. On the other hand, I agree that if we change the version number, it feels weird not to change the major component while there are already incompatible changes.

I'm uncomfortable making an exception to the release process by bumping the version to its final value in advance - processes work best when there are no exceptions.

I'm really tempted to not touch the version at all, and rely on a new dedicated macro instead. I don't think this new macro needs to be public, it could live in library/common.h. Wdyt?

[ronald-cron-arm]

I'm really tempted to not touch the version at all, and rely on a new dedicated macro instead. I don't think this new macro needs to be public, it could live in library/common.h. Wdyt?

I am not a fan of the 3.99 trick thus rather in favor of a dedicated macro solution.

[gilles-peskine-arm]

I'm uncomfortable making an exception to the release process by bumping the version to its final value in advance - processes work best when there are no exceptions.

1. I think it would actually make more sense to change our process to bump the versions (both API and ABI) the first time we make a change that departs from the previous release
2. We don't really have a process for major releases.

So I still push for changing the major version to 4 yesterday.

[mpg]

If we want to discuss changing our process about bumping version numbers, then that discussion needs a larger audience than just the people who follow this PR.

In the meantime, using a dedicated macro would unblock this PR by disentangling it from a larger process discussion.

[davidhorstmann-arm]

Re versioning, I can see the argument for bumping the version before a release.

On these specific cases though, having done a bit more thinking I now think that it would be best to try and find a more localistic solution to each of these 3.6 - 4.x conflicts that will allow us to avoid:

#if IS_VERSION_3_6
/* Do things the ye olde way */
#else
/* Do things the 4.x way */
#endif

In many of these cases we can just accept either the old or new config option, without worrying about which branch it comes from. For the libtestdriver differences, I think there may be an ecumenical way to address this with some careful macro work - I'll have a go and report back as to how I get on.

[mpg]

@davidhorstmann-arm We've been discussing this with Ronald, and came to the conclusion that we'd like to reduce the (initial) scope here to only include what's needed for the repo split, it only files used by the crypto part. Practically, that would exclude certs.c and ssl_helpers.*.

The reasoning is that there are things that depend on the crypto part being done, but nothing that depends on the X.509/TLS part, which also happens to be where most of the difficult problems live. So, we'd like to avoid delaying the urgent-and-relatively-easy part due to the not-urgent-and-harder part.

Could you update this and #9547 with that new reduced scope in mind? Thanks!

[davidhorstmann-arm]

@mpg I have de-scoped both PRs and I expect CI to pass (it was passing with the larger scope). This and #9547 should be ready for review.

[mpg]

I'm very tempted to suggest we split the file in two, and only try to share the second part between 3.6 and 1.0.

If we go this route and want to minimize disruption, I'd suggest doing something like this:

• we keep psa_crypto_helpers.h and make it include a new file to which we move the problematic parts - that way, files that included psa_crypto_helpers.h keep working;
• we give the new file different names for 3.6 and 4.0/1.0, just to avoid a situation where in 3.6 we'd have two headers with the same name in different places (one in the main repo, one from the submodule), which would be a recipe for headaches.

[mpg]

PS: can you update the description to reflect the new reduced scope?

[davidhorstmann-arm]

Sharing the file between 3.6 and 4.0/1.0 would force use to carry all the complexity of 3.6 in 4.0/1.0 which I don't think is a desirable outcome.

Wdyt?

Hmm. I'm not too keen on splitting the header file to straddle 3.6 and 4.0 - that seems like we might be just adding the extra complexity by the backdoor. Couldn't we just wait until USE_PSA is removed and then put all of the *PSA_INIT() and *PSA_DONE() macros behind a big #if MBEDTLS_VERSION_MAJOR < 4 guard?

[gilles-peskine-arm]

I'm not too keen on splitting the header file to straddle 3.6 and 4.0 - that seems like we might be just adding the extra complexity by the backdoor.

Historical note (I haven't looked at any details): there was never any particular reason for grouping the content of psa_crypto_helpers.h, it was just stuff that was somehow related to PSA. We originally had everything in helpers.function and gradually moved it out. Splitting cleanup-related declarations, the decision of calling init, support for exotic options and whatever else would be rather cleaning up a bit.

wait until USE_PSA is removed

I'm not sure it will help that much here, but it's coming very soon. (And you're welcome to review it if you want to hurry it along...)

[mpg]

Couldn't we just wait until USE_PSA is removed and then put all of the *PSA_INIT() and *PSA_DONE() macros behind a big #if MBEDTLS_VERSION_MAJOR < 4 guard?

It's not just about USE_PSA, see the various other macros, it's also whether MD, CTR-DRBG, block_cipher use PSA (and for that last one, whether GCC and CCM use block_cipher).

However, thinking more about it: we don't need to wait until all of the above work is done, or any of it really. The point of the complex definitions for 3.6 is that we need to ensure that the code works without calling psa_crypto_init(), except in the circumstances where the documentation says we have to (when USE_PSA is on for PK, X.509 and TLS, in certain driver-only builds for MD, CTR-DRBG and GCM/CCM - the details are complicated). That's not something we need in development: we don't need any crypto operation to work without calling psa_crypto_init().

Merging this observation with your proposal to put all the *PSA_INIT/DONE behind a single 4.0 guard, we could do immediately do something like:

#if MBEDTLS_VERSION_MAJOR >= 4
/* Legacy names from 3.6 */
#define USE_PSA_INIT() PSA_INIT()
#define USE_PSA_DONE() PSA_DONE()
#define MD_PSA_INIT()   PSA_INIT()
#define MD_PSA_DONE()   PSA_DONE()
// and so one
#else
// The 3.6 things with all their complexity - but crucially no additional "4.0 vs 3.6" complexity

Then at some point in development we can do a general s/[A-Z_]*PSA_\(INIT\|DONE\)/PSA_\1/ and remove the legacy names - but that would be for another PR.

Wdyt?

[mpg]

Splitting cleanup-related declarations, the decision of calling init, support for exotic options and whatever else would be rather cleaning up a bit.

Indeed, but I think the part of my proposal that adds complexity is when I suggest one of the files continues to live in the main repo for 3.6 and moves to the framework for 4.0 and 1.0 - on second thought, I'm really not happy about that part.

(Without that part, splitting up the file to clean it up becomes quite orthogonal to this PR - would probably still be a good thing, but probably for another PR.)

[davidhorstmann-arm]

Wdyt?

That sounds good to me (and much simpler). I'll give that a try.

[davidhorstmann-arm]

ABI-check failing as a result of the recent motion of error.c I believe.