mbedtls
mbedtls copied to clipboard
Improve support of `mbedtls_psa_get_random` in client-only builds
Context: PSA has as standard RNG API that is accessible from everywhere as soon as psa_crypto_init() has been called. Legacy crypto APIs tend to accept f_rng, p_rng arguments because the legacy crypto API doesn't inlucde a globally-accessible RNG. The function mbedtls_psa_get_random() and companion macro MBEDTLS_PSA_RANDOM_STATE exist as a bridge between the two worlds, in that they allow legacy crypto APIs to use the PSA RNG.
However, as they are currently implemented, unless MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG is defined, they assume the RNG is built-in. This does work nicely with client-only (CRYPTO_CLIENT && !CRYPTO_C) builds. (And possibly neither with driver-only builds if the DRBG were accelerated.)
I suggest we:
- Change the implementation to be always the one currently use with
MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG. That it, only use the standard PSA APIs and quite making assumption about how it's implemented internally. - Move the implementation from
psa_crypto.cwhere (1) it doesn't belong (it's not a PSA API) and (2) it's guarded byPSA_CRYPTO_Cas the rest of the file is, topsa_util.cwhere (1) it belongs (it's a PSA<->legacy bridge, declared inpsa_util.h) and (2) it would be guarded only byPSA_CRYPTO_CLIENT.
@gilles-peskine-arm Can you sanity-check that this makes sense and I didn't miss anything obvious?
Also, should this go in the "legacy<->PSA bridge" EPIC, as it's about improving an existing bridge, or in the "3.6 misc." EPIC (as a nice-to-have) as it's about client-only builds?
Yes, that makes sense. I can't remember why I implemented it this way and I can't find any clue in the history of https://github.com/Mbed-TLS/mbedtls/pull/4110. I guess the issue was framed as exposing the internal PSA RNG abstraction to users. And maybe I wanted to minimize the overhead, but an extra function call is negligible compared to the crypto that has to take place.
I don't know which epic, it's not a release blocker anyway: I'd be ok with changing this in a patch update.