Added minimal TLSv1.3-only client configuration

[NadavTasher]

Description

Added a minimal configuration for a TLSv1.3-only client with certificate handshake support.
Useful for minimal clients in embedded solutions.
Can be tested using mini_client.

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.

• changelog Not required, just an optional configuration file
• development PR This is the PR.
• framework PR Not required, just an optional configuration file
• 3.6 PR Not required, just an optional configuration file
• 2.28 PR Not required, just an optional configuration file
• tests Not required, just an optional configuration file

[tom-daubney-arm]

Hi @NadavTasher , the CI is failing because the DCO check is not passing. Can you please resubmit with a signed off commit? e.g. git commit -s. Thanks

[NadavTasher]

@tom-daubney-arm Done.

[tom-daubney-arm]

Hi @NadavTasher, you have an issue with code style in this PR. You can either manually fix this, or you can run /scripts/code_style.py -f from the project root and this will automatically fix the style. You can the commit the changes arising from running the script.

[NadavTasher]

@tom-daubney-arm Fixed.

[gilles-peskine-arm]

Thank you for contributing this configuration! It is a useful purpose. I mostly agree with the selection (except for curves). A few things will need to change, and we'll need a test component.

We are moving a lot of files and renaming identifiers around at the moment, in preparation for the next major release, so this is likely to require more adaptations. I suggest waiting until things stabilize before doing rework. It should us take a couple of months to reach a more stable state.

[gilles-peskine-arm]

Since this is supposed to be a minimal configuration, shouldn't this specify just one curve? If users want to have more curve available, they can easily tweak the example to support more curves (and we can put a comment stating you can do that, or have common curves commented out, e.g. default to secp256r1 and have secp384r1 and curve25519 commented out).

[NadavTasher]

I agree with you for the most part - this does give the user a more "minimal" configuration, but this configuration might be too minimal to communicate with real servers OOTB.

As you suggested, I have removed these values and added the PSA_WANT_ECC_SECP_R1_256 define in crypto-config-aes-gcm-tls1_3.h, but I think more WANT_ECC macros should be added.

WDYT?

[gilles-peskine-arm]

I think there's more value in showing a minimal configuration where it's easy for people to add more things, than in showing a large configuration which will have people wondering what they can remove. A few things could have comments “if you need feature X, uncomment the next line”. For curves, I think it's fairly easy to figure out how to add one.

The current configuration with just AES-GCM and secp256r1 is close to what RFC 8446 §9.1 defines as the default minimal TLS 1.3 profile, except:

• No RSA here.
• There's TLS_AES_256_GCM_SHA384 as well as TLS_AES_128_GCM_SHA256.

I wonder if it would make sense to have exactly the RFC 8446 minimal profile? Or at least have comments explaining the differences?

[gilles-peskine-arm]

Please add a test component to build and test this configuration in tests/scripts/components-configuration-tls.sh, similar to component_test_config_suite_b.

[NadavTasher]

I have added a minimal compilation test. I am not quite sure what we want to test here since we are disabling most features - what is your opinion?

IMO if we could somehow have a functionality test using mini_client against openssl s_server that would be best.

[gilles-peskine-arm]

Thank you very much for updating this! Unfortunately we aren't allowed to use commits that are missing a signoff line — this signoff line is how you convey that you allow us to use your contribution. Would you mind pushing a rebased version that the DCO check accepts?

[NadavTasher]

Oh yeah I completely forgot about that - will do that ASAP.

[NadavTasher]

@tom-daubney-arm Done.

[NadavTasher]

@tom-daubney-arm Any updates?

[NadavTasher]

@gilles-peskine-arm Can you tell me what's left to do here?

[gilles-peskine-arm]

Thanks for updating the pull request and bringing this to our attention. The ball is now on our side, we need to find time to review (always a problem I'm afraid). I'll try to do at least a first pass quickly, but I can't promise a full review any time soon.

[ronald-cron-arm]

I can help as second reviewer as @tom-daubney-arm will not be able to continue on that I think.

[gilles-peskine-arm]

Why MBEDTLS_X509_CREATE_C? I don't think it's needed in TLS tests.

[NadavTasher]

Actually, when compiling without MBEDTLS_X509_CREATE_C, make test fails:

make[1]: Entering directory '/home/nadav/Local/mbedtls/tests'
  CC   test_suite_x509write.c
/usr/bin/ld: /tmp/ccEzn7RP.o: in function `test_oid_from_numeric_string':
test_suite_x509write.c:(.text+0x1ca): undefined reference to `mbedtls_oid_from_numeric_string'
collect2: error: ld returned 1 exit status

[gilles-peskine-arm]

That's a discrepancy between the library code and the test code: we shouldn't try to test a function that isn't defined. I think it makes sense that mbedtls_oid_from_numeric_string() is only defined when MBEDTLS_X509_CREATE_C is enabled. The test function has depends_on:MBEDTLS_X509_USE_C, that looks like a mistake or copypasta. Would you mind changing this to depends_on:MBEDTLS_X509_CREATE_C? Then you shouldn't need MBEDTLS_X509_CREATE_C in the configuration.

[NadavTasher]

Yeah sure I'll do that :)

[NadavTasher]

Fixed in 8edf898.

[gilles-peskine-arm]

I think there's more value in showing a minimal configuration where it's easy for people to add more things, than in showing a large configuration which will have people wondering what they can remove. A few things could have comments “if you need feature X, uncomment the next line”. For curves, I think it's fairly easy to figure out how to add one.

The current configuration with just AES-GCM and secp256r1 is close to what RFC 8446 §9.1 defines as the default minimal TLS 1.3 profile, except:

• No RSA here.
• There's TLS_AES_256_GCM_SHA384 as well as TLS_AES_128_GCM_SHA256.

I wonder if it would make sense to have exactly the RFC 8446 minimal profile? Or at least have comments explaining the differences?

[gilles-peskine-arm]

I've done a full review. The configuration almost looks good to me. We'll need some testing, and the CI is slightly unhappy.

[gilles-peskine-arm]

Please run make test. It's expected to pass in all configurations. It doesn't have a lot of TLS testing, but it has some, and that's better than nothing.

[gilles-peskine-arm]

Please run at least a subset of tests/ssl-opt.sh, in a build with debug (like component_test_config_suite_b).

(No compat.sh because that's not applicable to TLS 1.3. For 1.3, the similar tests are in ssl-opt.sh.)

In principle, we should just run tests/ssl-opt.sh with no arguments. It has declarations to skip tests that rely on features that are missing from the configuration. In practice, that may or may not work, because ssl-opt.sh is missing a lot of feature dependency statements. However, the TLS 1.3 tests are a lot cleaner than the older tests, so there's hope. Can you please try tests/ssl-opt.sh? And if it's failing due to missing dependencies that aren't trivial to fix, either exclude problematic tests with -e 'REGEX' or include only certain tests with -f 'REGEX'. At least tests/ssl-opt.sh with no option should skip all TLS 1.2 tests, since we already run it on 1.3-only configurations. What might be missing is dependencies on optional TLS 1.3 features or on cryptographic mechanisms.

[NadavTasher]

Thank you all for your review.
I will make the necessairy changes as soon as I have time 😀