Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve configuration coverage for asymmetric mechanisms #9197

Open
gilles-peskine-arm opened this issue May 29, 2024 · 2 comments
Open

Improve configuration coverage for asymmetric mechanisms #9197

gilles-peskine-arm opened this issue May 29, 2024 · 2 comments

Comments

@gilles-peskine-arm
Copy link
Contributor

We don't have good coverage of the TLS code when only certain asymmetric mechanisms are enabled. As of 3b40344 (currently in #9172), which is close to mbedtls-3.6.0:

[omitted: all 3 enabled, lots of coverage]
+ ./framework/scripts/search_outcomes_config.py PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY '!PSA_WANT_KEY_TYPE_DH_PUBLIC_KEY' MBEDTLS_SSL_CLI_C MBEDTLS_SSL_SRV_C
+ ./framework/scripts/search_outcomes_config.py PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY '!PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY' PSA_WANT_KEY_TYPE_DH_PUBLIC_KEY MBEDTLS_SSL_CLI_C MBEDTLS_SSL_SRV_C
component_test_depends_py_pkalgs-!MBEDTLS_RSA_C
component_test_depends_py_pkalgs_psa-!MBEDTLS_RSA_C
component_test_psa_crypto_config_accel_ecc_ffdh_no_bignum
component_test_psa_crypto_config_reference_ecc_ffdh_no_bignum
+ ./framework/scripts/search_outcomes_config.py PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY '!PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY' '!PSA_WANT_KEY_TYPE_DH_PUBLIC_KEY' MBEDTLS_SSL_CLI_C MBEDTLS_SSL_SRV_C
component_test_psa_crypto_config_accel_ecc_no_bignum
component_test_psa_crypto_config_reference_ecc_no_bignum
config-suite-b.h
config-thread.h
+ ./framework/scripts/search_outcomes_config.py '!PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY' PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY PSA_WANT_KEY_TYPE_DH_PUBLIC_KEY MBEDTLS_SSL_CLI_C MBEDTLS_SSL_SRV_C
component_test_depends_py_pkalgs-!MBEDTLS_ECP_C
component_test_depends_py_pkalgs_psa-!MBEDTLS_ECP_C
+ ./framework/scripts/search_outcomes_config.py '!PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY' PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY '!PSA_WANT_KEY_TYPE_DH_PUBLIC_KEY' MBEDTLS_SSL_CLI_C MBEDTLS_SSL_SRV_C
+ ./framework/scripts/search_outcomes_config.py '!PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY' '!PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY' PSA_WANT_KEY_TYPE_DH_PUBLIC_KEY MBEDTLS_SSL_CLI_C MBEDTLS_SSL_SRV_C
+ ./framework/scripts/search_outcomes_config.py '!PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY' '!PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY' '!PSA_WANT_KEY_TYPE_DH_PUBLIC_KEY' MBEDTLS_SSL_CLI_C MBEDTLS_SSL_SRV_C
config-ccm-psk-dtls1_2.h
config-ccm-psk-tls1_2.h

(see Mbed-TLS/mbedtls-framework#20 for tooling)

Analysis:

ECC RSA FFDH USE_PSA_CRYPTO Coverage
yes yes yes yes plenty
yes yes yes no plenty
yes yes no yes none
yes yes no no none
yes no yes yes yes
yes no yes no yes
yes no no yes yes
yes no no no yes
no yes yes yes yes
no yes yes no none
no yes no yes none
no yes no no none
no no yes yes none
no no yes no none
no no no yes TLS 1.2 only, but there is component_test_tls13_only_psk which disables ECC key exchanges but not ECC support in crypto
no no no no yes (TLS 1.3 is not relevant for not-PSA)

Note that the analysis above is about which cryptographic mechanisms are enabled, not about which key exchanges are enabled. For TLS 1.2 key exchange families (MBEDTLS_KEY_EXCHANGE_xxx), we also run tests when exactly one key exchange family is enabled, both with and without MBEDTLS_USE_PSA_CRYPTO, through depends.py. This doesn't cover all the configurations we should test because some things depend on available cryptographic mechanisms rather than key exchanges, as observed in #9188 (comment) for example.

Why this is important: some things like buffer sizes depend on the available cryptographic mechanisms. We want to have testing that covers configurations where buffer size calculations might be problematic.

Goal of this issue: to have test coverage of the TLS code in a set of asymmetric-crypto configurations that we consider to be acceptable. This may involve some code refactoring as well: maybe TLS 1.2 code shouldn't have any dependencies on asymmetric cryptographic mechanisms, only on key exchanges?

TODO: look at 2.28

@mpg
Copy link
Contributor

mpg commented Jun 4, 2024

This may involve some code refactoring as well: maybe TLS 1.2 code shouldn't have any dependencies on asymmetric cryptographic mechanisms, only on key exchanges?

Copy-pasting here what I wrote elsewhere, so that it's all in one place:

For a long time I've been unhappy about TLS 1.2 being full of guards like ECDH_C as opposed to KEY_EXCHANGE_WITH_ECDH and wanted to replace most of them at some point. As part of driver-only work we did replace quite a few (only related to ECC though), but I also learned that not everything can be expressed in terms of KEY_EXCHANGE_xxx: TLS 1.2 signature for client authentication is independent from the key exchange (the client can present an ECDSA cert even if the key exchange is ECDHE-RSA) so we ended up still using MBEDTLS_PK_CAN_ECDSA for that.

(Note that TLS 1.3 has the same for auth on both sides, and also for the key exchange. In a way, TLS 1.3 is more agile in that it treats all signature algs uniformly, and all DH algs (EC, FF) uniformly. So here we absolutely can't rely solely on KEY_EXCHANGE_MODE macros - they only tell us if a signature alg and/or a DH alg is going to be used, but nothing about which one.)

@mpg
Copy link
Contributor

mpg commented Jun 4, 2024

In terms of goal, I think for the LTS branches (and even for development as a first step), it's enough to have things building and tests passing in a large enough set of configurations, and my gut feeling is this should not require refactoring.

However for development in the long run, there's another goal which is to minimize footprint: for example if no 1.2 ECDH key exchange is enabled, but for some reason ECDH_C (or WANT_ALG_ECDH) is, then we shouldn't have any ecdh-related stuff in ssl_handshake, and all code paths related to KEY_EXCHANGE_XXX_ECDH should be compiled out. I believe that's the case for ECDH now, probably no so for FFDH though. And for ECDSA and RSA, things are more complicated due to client auth as mentioned above.

@minosgalanakis minosgalanakis moved this to Test cases not executed in Mbed TLS Epics Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Test cases not executed
Development

No branches or pull requests

2 participants