Driver only hashes wrap-up #6283
Conversation
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
- Some things that were indicated as in the near future are now done. - Clarify when these macros are needed and when they're not. - Prepare to make the header public. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
mpg
added
the
needs-review
mpg
added
size-s
include/mbedtls/ssl.h
Outdated
| #else | ||
| #define MBEDTLS_TLS1_3_MD_MAX_SIZE MBEDTLS_MD_MAX_SIZE | ||
| #endif /* MBEDTLS_USE_PSA_CRYPTO */ | ||
| #define MBEDTLS_TLS1_3_MD_MAX_SIZE MBEDTLS_HASH_MAX_SIZE |
There was a problem hiding this comment.
Shouldn't this be PSA_HASH_MAX_SIZE? I could see that psa_hash is used everywhere in TLS1.3, not mbedtls_md.
There was a problem hiding this comment.
This should be PSA_HASH_MAX_SIZE only if this macro is never used to size buffers for code shared with TLS 1.2 (which uses the legacy API). Considering the name of the macro this seems unlikely, but I'll double-check uses before making the changes you suggest.
There was a problem hiding this comment.
Just a remark that may help here. This macro was introduced because TLS 1.3 ciphersuites only allow SHA-256 and SHA-384 not SHA-512. While SHA-512 may be included in the library and even used for some TLS 1.3 hash calculations where the hash is not the hash of the selected ciphersuite (not completely sure if there is such a case though, maybe for certificate chain validation ...), SHA-512 is not used for hash calculations where the hash is the hash of the selected ciphersuite. Thus, this macro aims to define the maximum possible size of the hash of a selected ciphersuite. It is supposed to be equal eventually to MIN( PSA_HASH_MAX_SIZE, 384 ). Not the case yet and not documented because we never finalized that.
There was a problem hiding this comment.
Thanks for the info, @ronald-cron-arm ! I'll change to PSA_HASH_MAX_SIZE for now and let you take it from there.
|
Changes look good to me, but the CI isn't happy yet: |
As a public header, it should no longer include common.h, just use build_info.h which is what we actually need anyway. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Opportunities for using the macros were spotted using:
git grep -E -n -A2 'MBEDTLS_(MD|SHA)[0-9]+_C' | egrep 'PSA_WANT_ALG_(MD|SHA)'
then manually filtering the results.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Actually this macro is never used in parts that depend on USE_PSA, so it's always using PSA. Currently the macro seems a bit redundant, but: - since it's public we can't remove it; - and there are plans in the future to make it more precise (actually the largest hash that matters for TLS 1.3 is SHA-384 now). Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
- One module was missing the warning on psa_crypto_init(). - For modules that are affected by USE_PSA_CRYPTO, it makes more sense to mention that in the warning. - Attempt to improve the description of the TLS 1.3 situation. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
(The first entry will need editing if support for ENTROPY_C is sorted out before the next release.) Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Quite unrelated to the other commits in this branch, but I happened to spot it, so I fixed it. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
mpg
force-pushed
the
driver-only-hashes-wrap-up
branch
from
73e9317
to
138387f
Compare
|
Ah sorry for this, thanks for pointing it out. I only checked that I'm rewriting the history to fix this, as we have a guideline that we'd like each commit to at least build in the default config (makes things easier when bisecting issues later). |
Same problem as Mbed-TLS#6101, same fix (the second commit of Mbed-TLS#6111). Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Stating from the default config means a few things are implicitly excluded; starting from the full config makes it all fully explicit. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The EC J-PAKE module the ability to "fall back" to PSA when MD is not present a few PRs ago, but the dependency of this key exchange on SHA-256 wasn't updated at the time. (Note: the crypto primitive doesn't depend on SHA-256, only its use in the TLS key exchange does.) Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
include/mbedtls/check_config.h
Outdated
| !defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) ) | ||
| #error "MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED defined, but not all prerequisites" | ||
| #endif | ||
|
|
||
| /* Use of EC J-PAKE in TLS requires SHA-256. | ||
| * This will be taken from MD is present, or from PSA if MD is absent. |
There was a problem hiding this comment.
| * This will be taken from MD is present, or from PSA if MD is absent. | |
| * This will be taken from MD if it is present, or from PSA if MD is absent. |
include/mbedtls/mbedtls_config.h
Outdated
| @@ -958,7 +958,7 @@ | |||
| * might still happen. For this reason, this is disabled by default. | |||
| * | |||
| * Requires: MBEDTLS_ECJPAKE_C | |||
| * MBEDTLS_SHA256_C | |||
| * SHA-256 (via MD is present, or via PSA, see MBEDTLS_ECJPAKE_C) | |||
There was a problem hiding this comment.
| * SHA-256 (via MD is present, or via PSA, see MBEDTLS_ECJPAKE_C) | |
| * SHA-256 (via MD if it is present, or via PSA, see MBEDTLS_ECJPAKE_C) |
| /* Use of EC J-PAKE in TLS requires SHA-256. | ||
| * This will be taken from MD is present, or from PSA if MD is absent. | ||
| * Note: ECJPAKE_C depends on MD_C || PSA_CRYPTO_C. */ | ||
| #if defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED) && \ |
There was a problem hiding this comment.
You mention that its use in TLS requires SHA-256 - is it about the ad-hoc KDF function, or is there another use too? If it's about the KDF, it would be better to state the requirement specifically: https://github.com/Mbed-TLS/mbedtls/pull/6115/files#diff-1844b20cf0e3b97c23e4dc032c1fd6cfe36e69b2a6ca27788eb49d3b17bb4663R95
There was a problem hiding this comment.
Currently use in TLS can only go through the legacy API (mbedtls_ecjpake_xxx() functions), where the custom KDF is implicit in mbedtls_ecjpake_derive_secret(), so I think the dependencies are correct for now, but indeed we'll need to update them again in #5886 when we start optionally using the PSA API, including the new KDF from #6115.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
wernerlewis
added
approved
This is a wrap-up PR for the "driver-only: 1a hashes" EPIC, adding a ChangeLog entry, updating the documentation and doing misc. clean-ups that we discussed in previous PRs but left as follow-ups.
This implements part of #6146 but does not close it - we want that issue to remain open in the release EPIC so that we remember to update again for work that's going to happen between now and the next release.
Requires Backporting
NO - new features