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.

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

Pure C configuration of PSA crypto mechanisms #3628

Conversation

gilles-peskine-arm
Copy link
Contributor

Proposed specification for conditional inclusion of cryptographic mechanism through the PSA API in Mbed TLS.

This is work in progress, presented for a first design discussion.

@gilles-peskine-arm gilles-peskine-arm added needs-design-approval component-crypto Crypto primitives and low-level interfaces labels Aug 31, 2020
@gilles-peskine-arm gilles-peskine-arm self-assigned this Aug 31, 2020
@gilles-peskine-arm gilles-peskine-arm added this to To do in Mbed TLS PRs via automation Aug 31, 2020
@gilles-peskine-arm gilles-peskine-arm added this to To do in Mbed TLS Current Sprint via automation Aug 31, 2020
Proposed specification for conditional inclusion of cryptographic
mechanism through the PSA API in Mbed TLS.

The inclusion of a mechanism is based on a declaration of boolean
symbols by the application. There is a symbol for each key type or
parametrized key type constructor, and for each algorithm or
parametrized algorithm constructor.

This is work in progress, presented for a first design discussion.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm force-pushed the psa-conditional-inclusion-c-proposal branch from 93ed319 to b51f96a Compare August 31, 2020 12:47
@gilles-peskine-arm gilles-peskine-arm changed the title Pure C configuraiton of PSA crypto mechanisms Pure C configuration of PSA crypto mechanisms Aug 31, 2020
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Copy link
Contributor Author

My original proposal defines a new set of symbols PSA_WANT_xxx for what the application wants, and keeps the existing symbols MBEDTLS_xxx_C to mean “include the software implementation” (which, for some modules, can reroute to an ALT implementation).

But as I start to flesh out the details, I'm more and more thinking that this is going to be too disruptive, especially on the X.509 and TLS code. For example, when MBEDTLS_USE_PSA_CRYPTO is defined, the inclusion of TLS cipher suites should depend on the availability of mechanisms, not on the availability of the software implementation. It would be easier to keep e.g. MBEDTLS_AES_C to mean “AES is available”, and define a new way of excluding the AES code when an accelerator is present (for AES, maybe similar enough that we can reuse MBEDTLS_AES_ALT).


If a mechanism should only be supported in an opaque driver, what does the core need to know about it? Do we have all the information we need?

This is especially relevant to suppress a mechanism completely if there is no matching algorithm. For example, if there is no transparent implementation of RSA or ECDSA, `psa_sign_hash` and `psa_verify_hash` may still be needed if there is an opaque signature driver.
Copy link
Contributor

Choose a reason for hiding this comment

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

The more relevant question here would probably be: does the core accept key handling of key types that are only handled by an opaque driver. It sounds like it should, but should probably be left as a note to the implementer.


#### Algorithms without a key type or vice versa

Is it realistic to mandate a compile-time error if a key type is required, but no matching algorithm, or vice versa? Is it always the right thing, for example if there is an opaque driver that manipulates this key type?
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the PSA Crypto core is used to store a key in opaque storage, but the actual algorithm is handled outside of PSA Crypto? (e.g. because the core doesn't yet support the algorithm, for example ECIES)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that the same thing as the last sentence in my paragraph?

There's also an open question if the core doesn't support a mechanism, but a transparent driver does. It's not clear to me yet who is responsible for what, and how much the core can defer to drivers. That's one of the things I want to settle before declaring the driver specification as final. But it doesn't hold writing drivers that are specific to one particular implementation, where we make sure that we agree on assumptions (e.g. that at least one side verifies that it isn't using an RSA public key for HMAC).

The symbol `MBEDTLS_PSA_CRYPTO_CONFIG` in `mbedtls/config.h` determines whether `psa/crypto_config.h`. is used.

* If `MBEDTLS_PSA_CRYPTO_CONFIG` is unset, which is the default at least in Mbed TLS 2.x versions, things are as they are today: the PSA subsystem includes generic code unconditionally, and includes support for specific mechanisms conditionally based on the existing `MBEDTLS_xxx_` symbols.
* If `MBEDTLS_PSA_CRYPTO_CONFIG` is set, the necessary software implementations of cryptographic algorithms are included based on both the content of the PSA crypto configuration file and the Mbed TLS configuration file. For example, the code in `aes.c` is enabled if either `mbedtls/config.h` contains `MBEDTLS_AES_C` or `psa/crypto_config.h` contains `PSA_WANT_KEY_TYPE_AES`.
Copy link
Contributor

Choose a reason for hiding this comment

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

In practice, this means 'wants' from PSA Crypto configuration will only be able to add (include more functionality) to the mbedTLS config, right? So the Crypto config would be included as part of the mbedTLS config header, after the user config, but before check-config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

If an application uses a library that calls mbedtls_aes_xxx and another library that calls psa_xxx(PSA_KEY_TYPE_AES), it should pull in MBEDTLS_AES_C for the sake of the first library and PSA_KEY_TYPE_AES for the sake of the second library. The fact that PSA_WANT_KEY_TYPE_AES currently implies MBEDTLS_AES_C is an implementation detail that may change if later mbedtls_aes_xxx becomes a wrapper around a PSA implementation.

I can only see two viable alternatives here: either the PSA configuration can add to the ”legacy“ configuration, or it has to be compatible (so if you have PSA_WANT_KEY_TYPE_AES but no PSA AES accelerator and no MBEDTLS_AES_C, you get a compile-time error). I proposed the add model because it's a lot more transparent for users: you get what you ask for.

The compatible model has the advantage that it's likely easier to implement, and perhaps easier to understand for the minority of users who care about both crypto APIs, because it's the way Mbed TLS works now: if you say you want X and you don't say you want Y, but X requires Y, you get a compile-time error from check_config.h. We'd like to move away from this model in Mbed TLS (hopefully it'll be one of the 3.0 changes), and instead go for the model where you get what you ask for.

I don't think the add model will be substantially harder to implement because it's about as much effort to write the dependencies in add form (if defined(X) define Y) as in compatible form (if defined(X) and not defined(Y) error).

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 another advantage of the "add" model is that in case someone just wants to use the PSA Crypto API and doesn't care about any other part of Mbed TLS, they could use an empty mbedtls/config.h and only populate psa/crypto_config.h to get only what they want from PSA Crypto, without needing to know anything about Mbed TLS configuration. (With a "compatible" model, those people would need to know which Mbed TLS symbols correspond to the PSA symbols, even though they only care about PSA.)

@gilles-peskine-arm gilles-peskine-arm moved this from To do to In progress in Mbed TLS Current Sprint Sep 1, 2020
@gilles-peskine-arm
Copy link
Contributor Author

It would be easier to keep e.g. MBEDTLS_AES_C to mean “AES is available”, and define a new way of excluding the AES code when an accelerator is present (for AES, maybe similar enough that we can reuse MBEDTLS_AES_ALT).

Easier in some places, but also a problem in other places. And it would break backward compatibility: today MBEDTLS_AES_C means that an application can call mbedtls_aes_xxx, not that there exists some unspecified way of performing AES operations.


### Current situation

Mbed TLS offers a way to select which cryptographic mechanisms are included in a build through its configuration file (`config.h`). This mechanism is based on two main sets of symbols: `MBEDTLS_xxx_C` controls the availability of the mechanism to the application, and `MBEDTLS_xxx_ALT` controls the availability of an alternative implementation, so the software implementation is only included if ``MBEDTLS_xxx_C` is defined but not `MBEDTLS_xxx_ALT`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: double back-quote before MBEDTLS_xxx_C

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

#### Naming of symbols

The names of [elliptic curve symbols](#configuration-symbols-for-curves) are a bit weird: `SECP_R1_256` instead of `SECP256R1`. Should we make them more classical, but less systematic?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it matters a lot, but I tend to prefer the more systematic names, as they are easier to relate to the PSA Crypto API.


#### Impossible combinations

What does it mean to have `PSA_WANT_ALG_ECDSA` enabled but with only Curve25519? Is it a mandatory error?
Copy link
Contributor

Choose a reason for hiding this comment

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

Since PSA_WANT_ALG_ECDSA with no curve at all gives an #error, I don't see any reason why PSA_WANT_ALG_ECDSA with no ECDSA-capable curve shouldn't give an #error as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I think the word "curve" is a bit abused in "curve25519", and that's why some references such as RFC 7748 introduced the name "X25519": MBEDTLS_ECP_DP_CURVE25519 doesn't give you the implementation of a curve as the name unfortunately suggests, it gives you the implementation of a key exchange function, X25519, whose definition happens to be based on the union of a quotient of a curve and a quotient of its twist, which is actually quite different from a curve.

Perhaps when you see it that way, it becomes clearer that defining some key exchange function really has nothing to do with ECDSA, and shouldn't influence whether PSA_WANT_ALG_ECDSA gives an error or not.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@mpg
Copy link
Contributor

mpg commented Sep 7, 2020

It would be easier to keep e.g. MBEDTLS_AES_C to mean “AES is available”, and define a new way of excluding the AES code when an accelerator is present (for AES, maybe similar enough that we can reuse MBEDTLS_AES_ALT).

Easier in some places, but also a problem in other places. And it would break backward compatibility: today MBEDTLS_AES_C means that an application can call mbedtls_aes_xxx, not that there exists some unspecified way of performing AES operations.

Yes, I think the current proposal is good on this point:

  • if PSA_WANT_ALG_AES is defined, it means you can use AES with the psa_xxx() API
  • if MBEDTLS_AES_C is defined, it means you can use AES with the mbedtls_aes_xxx() API

This is exactly what user code (including libmbedtls and libmbedx509) needs to know.

Now, obviously over the course of moving libmbedtls to using PSA Crypto, this means we might have to write somewhat complex conditions, such as (defined(MBEDTLS_USE_PSA_CRYPTO) && defined(PSA_WANT_ALG_AES)) || (!defined(MBEDTLS_USE_PSA_CRYPTO) && defined(MBEDTLS_AES_C)), and the exact condition will vary, for example currently for a ciphersuite using SHA-256 for PRF and the HMAC, the condition would be defined(MBEDTLS_MD_C) && defined(MBEDTLS_SHA256_C) && (!defined(MBEDTLS_USE_PSA_CRYPTO) || (defined(PSA_WANT_ALG_TLS12_PRF) && defined(PSA_WANT_ALG_SHA256))) - because currently the HMAC part will use SHA-256 via the mbedtls_xxx() API regardless of whether USE_PSA_CRYPTO is defined or not, so when it is we end up using SHA-256 through both APIs.

This is not going to make our lives easy as maintainers of libmbedtls, but that complexity is inherent to what we're currently doing in that library: transitioning from one crypto API to the next in a gradual way. I think the current proposal supports that, and I don't think any simplification would support it any better (probably worse actually). APIs should be as simple as possible, but not simpler :)

Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@gilles-peskine-arm
Copy link
Contributor Author

With Manuel and Steven's approval, I consider this approved for design, and good enough to merge. The interface may still evolve, especially if we discover unforeseen problems during the implementation; that's ok: this is a proposed architecture.

I will still monitor comments to this pull request, but it would be better to submit feedback to the mailing list (https://lists.trustedfirmware.org/pipermail/mbed-tls/2020-September/000184.html) if they require discussion, or as GitHub issues if you don't think prior discussion is necessary.

@gilles-peskine-arm gilles-peskine-arm merged commit a0a210f into Mbed-TLS:development Sep 11, 2020
Mbed TLS PRs automation moved this from To do to Done Sep 11, 2020
Mbed TLS Current Sprint automation moved this from In progress to Done Sep 11, 2020
@daverodgman daverodgman removed this from Done in Mbed TLS PRs Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants