-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Cleanup: Introduce MBEDTLS_PSA_CRYPTO_RNG_HASH (4/4) #10353
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
Cleanup: Introduce MBEDTLS_PSA_CRYPTO_RNG_HASH (4/4) #10353
Conversation
Only need to review the last two commit's range The scope is to undo the intermediate changes that needed to bring in the TF-PSA-Commit in and remove MBEDTLS_ENTROPY_FORCE_SHA256 calls from the mbedtls side. |
Parameterised test job -> 569 (Running) |
894317a
to
70d3108
Compare
d96e224
to
14b7bfd
Compare
fc5f6d0
to
4c66f05
Compare
Removed setters for `MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` and `MBEDTLS_ENTROPY_FORCE_SHA256` Signed-off-by: Minos Galanakis <[email protected]>
Signed-off-by: Minos Galanakis <[email protected]>
Signed-off-by: Minos Galanakis <[email protected]>
4c66f05
to
6cc9c66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and verified that the crypto update points to the head of Mbed-TLS/TF-PSA-Crypto#396. Obviously this is waiting for the merge of that PR.
*"programs/ssl/ssl_client1 "*) | ||
requires_config_enabled MBEDTLS_CTR_DRBG_C | ||
requires_config_enabled MBEDTLS_ENTROPY_C | ||
requires_config_enabled MBEDTLS_PSA_CRYPTO_C |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requires_config_enabled MBEDTLS_PSA_CRYPTO_C | |
requires_config_enabled MBEDTLS_PSA_CRYPTO_C | |
requires_config_disabled MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG |
Signed-off-by: Minos Galanakis <[email protected]> Signed-off-by: Ronald Cron <[email protected]>
Signed-off-by: Ronald Cron <[email protected]>
Ensure that when we switch from SHA-512 to SHA-256 as the default CTR_DRBG hash, we still properly test CTR_DRBG with SHA-512. Signed-off-by: Ronald Cron <[email protected]>
6cc9c66
to
aad5f1b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me but #10353 (comment).
I've updated the "Updated tf-psa-crypto pointer" commit with the merge of Mbed-TLS/TF-PSA-Crypto#396. Otherwise I've added two commits, the first one to address #10353 (comment) and the second to prepare for the switch to SHA-256 as the CTR_DRBG default hash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look correct to me, but they seem incomplete. (Although I'm not sure about the exact intended scope of this PR in the chain — I haven't fully absorbed what each step is supposed to do.)
not grep aesce_decrypt_block ${BUILTIN_SRC_PATH}/aesce.o | ||
} | ||
|
||
component_test_ctr_drbg_aes_256_sha_512 () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this component using exactly the full
config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently yes but not when we will switch to SHA-256 as the default hash for CTR_DRBG when both SHA-256 and SHA-512 are enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then component_test_ctr_drbg_aes_256_sha_256
will become identical to full
. I don't understand why both components are present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is just to ensure that we always test both SHA-256 and SHA-512 as CTR_DRBG hashes while we are doing changes in tf-psa-crypto. I will remove component_test_ctr_drbg_aes_256_sha_256 in a later PR when Mbed-TLS/TF-PSA-Crypto#419 is merged. We will need another mbedtls PR anyway for some further clean-up related to the removal of MBEDTLS_ENTROPY_C in Mbed-TLS/TF-PSA-Crypto#419.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for the explanation.
Signed-off-by: Ronald Cron <[email protected]>
f971548
to
a0b1c8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
not grep aesce_decrypt_block ${BUILTIN_SRC_PATH}/aesce.o | ||
} | ||
|
||
component_test_ctr_drbg_aes_256_sha_512 () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for the explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Corrections made by the very person who requested them.
Description
Last of the pr's required by Mbed-TLS/TF-PSA-Crypto#328
It is meant to be merged after Mbed-TLS/TF-PSA-Crypto#396.
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.