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.

Sign up for GitHub

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

Jump to bottom

Allow the use of PSA Crypto in TLS client example #232

Merged
merged 2 commits into from
Jan 28, 2019
Merged

Allow the use of PSA Crypto in TLS client example #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b1f3c3
Allow the use of PSA Crypto in TLS client example
Jan 22, 2019
4dc9aeb
Swap initializations of PSA Crypto and Mbed TLS platform
Jan 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions tls-client/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@
#include "mbed.h"

#include "mbedtls/platform.h"
#if defined(MBEDTLS_USE_PSA_CRYPTO)
#include "psa/crypto.h"
#endif /* MBEDTLS_USE_PSA_CRYPTO */

#include "HelloHttpsClient.h"

Expand All @@ -56,6 +59,25 @@ int main()
printf("Platform initialization failed with error %d\r\n", exit_code);
return MBEDTLS_EXIT_FAILURE;
}

#if defined(MBEDTLS_USE_PSA_CRYPTO)
/*
* Initialize underlying PSA Crypto implementation.
* Even if the HTTPS client doesn't make use of
* PSA-specific API, for example for setting opaque PSKs
* or opaque private keys, Mbed TLS will use PSA
* for public and symmetric key operations as well as
* hashing.
*/
psa_status_t status;
status = psa_crypto_init();
Copy link
Contributor

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()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RonEld Why?

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

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()

Copy link
Contributor

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.

Copy link
Author

@hanno-becker hanno-becker Jan 25, 2019 •
edited
Loading

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.

Copy link
Contributor

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 on mbedtls_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

Copy link
Author

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?

Copy link
Contributor

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

if( status != PSA_SUCCESS )
{
printf("psa_crypto_init() failed with %d\r\n", status );
return MBEDTLS_EXIT_FAILURE;
}
#endif /* MBEDTLS_USE_PSA_CRYPTO */

/*
* The default 9600 bps is too slow to print full TLS debug info and could
* cause the other party to time out.
Expand Down
10 changes: 10 additions & 0 deletions tls-client/mbedtls_entropy_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,14 @@
#undef MBEDTLS_MPI_MAX_SIZE
#define MBEDTLS_MPI_MAX_SIZE 256

/* This macro determines whether Mbed TLS uses its own legacy crypto library
* or an implementation of the PSA Crypto API such as Mbed Crypto.
*
* To confirm the use of PSA Crypto, you may enable debugging by setting
* HELLO_HTTPS_CLIENT_DEBUG_LEVEL in HelloHttpsClient.h and look for
* PSA-related debugging output on the serial line.
*
* Uncomment this to use the PSA Crypto API. */
//#define MBEDTLS_USE_PSA_CRYPTO

#define MBEDTLS_MPI_WINDOW_SIZE 1