-
Notifications
You must be signed in to change notification settings - Fork 53
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
Allow the use of PSA Crypto in TLS client example #232
Conversation
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.
I left one comment.
Currently not approving or requesting changes, until this is settled
* hashing. | ||
*/ | ||
psa_status_t status; | ||
status = psa_crypto_init(); |
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.
I think this should come after mbedtls_platform_setup()
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.
@RonEld Why?
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.
because mbedtls_platform_setup()
initializes the hw acceleration engine, and it psa_init()
is dependent on the driver, then it should come after.
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.
In addition, if psa_init()
is also intended to initialize the hw acceleration, then there could be conflicts, and undefined behaviour
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.
Perhaps the best location of calling psa_init()
is within mbedtls_platform_setup()
in mbed_platform_setup()
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's a standalone library.
However, mbedtls_platform_setup()
was first and foremost intended to initialize the HW acceleration engine. psa_crypto_init()
may need that engine.
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.
@RonEld Sorry, I could only repeat what I just said. There must not be any dependency of psa_crypto_init()
on Mbed TLS, in particular on mbedtls_platform_setup()
. If that's the case at the moment, it's a bug. In that case, I'm ok swapping the calls and opening a separate issue for it.
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.
There must not be any dependency of
psa_crypto_init()
on Mbed TLS, in particular onmbedtls_platform_setup()
.
Correct, there shouldn't be a dependency between PSA crypto and Mbed TLS. however, there is a dependency between PSA crypto and the platform HW accelerators. At the moment, the HW accelerators are initialized by mbedtls_platform_setup()
(currently only in NRF52840_Dk target on Mbed OS), so if the hw accelerator will be initialized also in psa_crypto_init()
(as it probably should), there could be some resource leak or undefined behavior if initializing twice the cryptographic engine.
To be honest, platforms that initialize the hw crypto engine in mbedtls_platform_setup()
, don't support psa crypto, and once they migrate to psa crypto, they should remove the initialization in the mbedtls_platform_xxx()
API. mbedtls_platform_setup()
was intended to initialize the hw acceleration engine, so actually it can't really co exist with `psa_crypto_init(). This is why I think my suggestion in #232 (comment) should probably be used
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.
@RonEld Omitting mbedtls_platform_setup()
if MBEDTLS_USE_PSA_CRYPTO
is defined doesn't work because the function might perform other platform initialization tasks not related to PSA crypto. Are you ok letting this rest until ARMmbed/mbed-crypto#24 is settled?
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.
At the moment, there are no other platform initialization tasks done in Mbed OS. The first intent for the platform initialization functions were to initialize hw acceleration. In the future it may be considered for other initialization (assuming it won't get deprecated), after the psa crypto will be fully integrated.
I will approve for the time being
As PSA Crypto is independent of Mbed TLS, `psa_crypto_init()` must eventually work standalone, and in particular not require a prior call to `mbedtls_platform_init()`. At the moment, however, it might still rely on HW drivers being initialized by `mbedtls_platform_init()`, and so the latter should be invoked before `psa_crypto_init()`. This commit changes the order of the calls accordingly. In general, the relation between of `psa_crypto_init()` and `mbedtls_platform_init()` still needs to be discussed and documented, and once that's done, the code might need revision. As it stands, however, it should work with current implementations of `psa_crypto_init()` and `mbedtls_platform_init()`.
@gilles-peskine-arm @RonEld Please see the latest commit. |
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.
Given the discussed scope, I can approve this PR.
Reviewed, approved, and passing CI, so can be merged. |
Summary: This PR allows the use of PSA Crypto in the TLS client example by adding the necessary
psa_crypto_init()
call to the initialization code.To use, define
MBEDTLS_USE_PSA_CRYPTO
inmbedtls_entropy_config.h
before invokingmbed compile
. No further changes are necessary.