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

Define PSA_WANT symbols for key creation methods #7439

Closed
gilles-peskine-arm opened this issue Apr 13, 2023 · 19 comments
Closed

Define PSA_WANT symbols for key creation methods #7439

gilles-peskine-arm opened this issue Apr 13, 2023 · 19 comments
Labels
component-psa PSA keystore/dispatch layer (storage, drivers, …) enhancement

Comments

@gilles-peskine-arm
Copy link
Contributor

The PSA_WANT_xxx compile-time configuration mechanisms allows users to declare which cryptographic mechanisms they need. Currently, this is based on key types and algorithms. PSA_WANT_KEY_TYPE_xxx requests full support for a key type (import, export, generate, derive), and PSA_WANT_ALG_yyy requests support for an algorithm (only with supported key types). (Key type support has a refinement for DH/ECC groups/curves, but this is not relevant here.)

Some key management mechanisms involve a significant amount of code size or might not be available with some drivers, so we would like to be able to restrict them. The goal is to reduce code size, so this is only relevant for mechanisms where a particular operation requires a significant amount of dedicated code. Specifically:

  • We would like to allow requiring support for importing a private RSA key, but not random generation or deterministic derivation. This would be similar to enabling MBEDTLS_RSA_C but not MBEDTLS_RSA_GENPRIME with the legacy interface. Mbed TLS does not intend to support RSA key derivation in any case.
  • We would like to make the derivation of ECC keys optional. This feature is not available in the legacy interface.
  • There is some interest in allowing an implementation to not support key import/export for certain types, since some secure elements refuse import/export of private/secret keys by policy. However, in Mbed TLS, we do not currently have much interest for this since it reduces what we can test. In terms of code size, this is only really relevant for RSA private keys.

This is an Mbed TLS-only interface, since we do not currently intend to make PSA_WANT an official PSA standard.

Definition of done for this issue: a draft specification is merged into https://github.com/Mbed-TLS/mbedtls/blob/development/docs/proposed/psa-conditional-inclusion-c.md.

Follow-ups: issues to be created based on the specification.

@gilles-peskine-arm gilles-peskine-arm added enhancement component-psa PSA keystore/dispatch layer (storage, drivers, …) labels Apr 13, 2023
@gilles-peskine-arm
Copy link
Contributor Author

What symbols do we need?

From a basic orthogonality perspective, we could create new symbols for each key managemenr method, i.e. define symbols of the form PSA_WANT_KEY_TYPE_<type>_<method> where <method> is one of IMPORT, EXPORT, EXPORT_PUBLIC, GENERATE, DERIVE. However I don't think we want to go that far:

  • For symmetric keys (apart from DES which is deprecated), all management operations are trivial, and fine-tuning which ones are allowed would be unnecessary complexity.
  • Exporting public keys is pretty much always useful, so having a way to disable it would be unnecessary complexity.
  • Due to the way Mbed TLS is currently implemented, import is effectively always necessary: keys are stored in their export format, so there needs to be code that parses this import format.

So I think that:

  • We should only introduce new symbols for GENERATE and DERIVE.
  • We should only introduce new symbols for key pair types (not public keys, not symmetric keys).

Backward compatibility

What do we do with existing configurations that don't define the new symbols? E.g. if PSA_WANT_KEY_TYPE_RSA_KEY_PAIR is enabled but not PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_GENERATE, is RSA key generation required?

A basic principle of PSA_WANT is that it's purely additive: the application declares what it needs, and defining additional symbols always means more required functionality. I really don't want to break this simple principle, so I don't want something like PSA_WANT_KEY_TYPE_RSA_WITHOUT_GENERATE.

Thus I see two sensible designs:

  1. PSA_WANT_KEY_TYPE_xxx_KEY_PAIR requests basic support for the type: import, export, export-public (and of course destroy). Generation and derivation need to be requested separately. This is the most in keeping with the current design philosophy.
  2. PSA_WANT_KEY_TYPE_xxx_KEY_PAIR requests full support for the type. There are new symbols for generate and derive, but also for only requesting import/export support. (Export-public and destroy are supported as soon as anything is supported.)

The PSA_WANT symbols are only used when MBEDTLS_PSA_CRYPTO_CONFIG is enabled, and we document this option as experimental, so we can change their meaning. However this mechanism has been pretty stable for several versions, so I'm reluctant to make major incompatible changes at this point. This is an argument in favor of (2).

@mpg
Copy link
Contributor

mpg commented Apr 14, 2023

I tend to agree on both fronts:

  • GENERATE and DERIVE are probably enough;
  • option (2) seems preferable for backwards compatibility - even if we don't promise stability, we probably want to avoid overly disruptive changes if we can. Also, I think even in the long-term it's convenient, as PSA_WANT_KEY_TYPE_xxx_KEY_PAIR serves as a handy shortcut, so that people who want everything has only one symbol to define.

Though I think with option (2) we need an extra symbol like PSA_WANT_KEY_TYPE_xxx_KEY_PAIR_BASIC that means "give me just import-export(-public), thanks", right?

Then we have a diamond dependency/auto-enablement graph:

                 xxx_KEY_TYPE_GENERATE
                /                     \
xxx_KEY_TYPE --<                       >-- xxx_KEY_TYPE_BASIC
                \                     /
                 xxx_KEY_TYPE_DERIVE -

@gilles-peskine-arm
Copy link
Contributor Author

I agree. Though I am a little annoyed that PSA_WANT_KEY_TYPE_RSA_KEY_PAIR then includes a request for RSA key derivation, which we don't provide. So it seems that PSA_WANT_KEY_TYPE_xxx_KEY_PAIR is “give me what you can for xxx”, which is not really a useful description of what the application wants.

A counter-argument is that PSA_WANT_KEY_TYPE_RSA_KEY_PAIR is already supposed to mean “full support for RSA private keys” but in practice means “what you can for RSA private keys”, and this hasn't really hurt us. But maybe it is a design defect that we should be fixing?

@mpg
Copy link
Contributor

mpg commented Apr 14, 2023

But maybe it is a design defect that we should be fixing?

I'm not sure I'd consider it a design defect as opposed to a limitation of the implementation.

I think to some extend "full support" always translates to "what you can". For example, an implementation could be restricted to certain key sizes (say, multiples of 8 no larger than 4096). I agree that this is not completely analogous to the present situation, in that here is no flag to say "support all key sizes" while there is (will be) a flag for "key derivation" and then it's indeed weird for an implementation to accept that flag being set while it can't provide the corresponding feature. I'm trying to think of more similar situations but I can't find better examples.

I'm also trying to think about what I would expect as a user, and I seem to have conflicted feelings. I'm not sure how happy I would be about something like "PSA_WANT_KEY_TYPE_xxx_KEY_PAIR means _BASIC plus any of _GENERATE, _DERIVE that the implementation supports (and the implement should document that)" - so for Mbed TLS for RSA that would be BASIC + GENERATE, and for ECC all three, but if the application requests RSA_KEY_PAIR_DERIVE explicitly then this results in an #error. (Let's call that (2l) for "2 loose".)

In this respect option (1) would be cleaner, despite its compatibility issues.

I think there's also an intermediate solution which would create compatibility issues but only for RSA, let's call it (2s) for "2 strict": xxx_KEY_PAIR really means all three and results in an #error if one of them is not available. So with current Mbed TLS you can ask for ECC_KEY_PAIR and it will work, but if you ask for RSA_KEY_PAIR you get an error, you have to ask for RSA_KEY_PAIR_BASIC + RSA_KEY_PAIR_GENERATE instead.

So, to sum up:

  • Option (1) is semantically clean but has the following compat wart: if you have an application that derives or generates keys, it will stop linking (or give runtime errors?) when you upgrade, until you modify your config to ask for _DERIVE and/or _GENERATE. For users of the default config, this will be transparent as we'll upgrade the defaults.
  • Option (2s) is semantically clean but has the following compat wart: if you have an application that enables RSA_KEY_PAIR, it will stop compiling when you upgrade, until you modify your config to stop requesting it and limit yourself to BASIC (+GENERATE) for RSA. For users that only enable ECC, nothing changes.
  • Option (2l) does not seem to cause compat issues but is not semantically clean.

Note that the compat issues are only for users of MBEDTLS_PSA_CRYPTO_CONFIG. I wonder if it would be correct to assume that most people who enable that only care about ECC?

@ronald-cron-arm
Copy link
Contributor

Looking at this, my preference goes to (1): semantically clean and only three PSA_WANT_xyz symbols instead of four for (2s) if I understand correctly. I don't think the compatibility issue is a huge problem as it seems manageable from the user perspective. Option (2I) is less problematic for some users in the short term but having something not semantically clean does not seem a good longer term solution.

@mpg
Copy link
Contributor

mpg commented May 11, 2023

Following some discussion, we seem to have reached a consensus on the following proposal.

For each existing KEYPAIR type (in the latest release: RSA, ECC, in development also DH):

  1. We add macros with suffixes USE, IMPORT, EXPORT, GENERATE, DERIVE. (In the long term, the code and tests should only use these. In the short term, see below.)
  2. We add a convenience macro with suffix ALL, that either enables all of the above, or gives an error if one is not available for this key type.
  3. For now, we keep the existing KEYPAIR macro (except for DH as it hasn't been released yet) with the current meaning of "give me whatever you've got" (so, for ECC: everything, for RSA: everything except DERIVE), but deprecate it, with an eye towards removing it in 4.0.

The documented "auto-enable graph" would be very simple: ALL enables IMPORT, EXPORT, GENERATE, DERIVE, and any of those 4 enable USE.

In the foreseeable future though, in practice, USE would auto-enable IMPORT and EXPORT. This is because our tests heavily rely on it, and we don't want to allow configurations that we can't actually test.

Migration plan:

  1. In the short term, we might want to keep using internal macros MBEDTLS_PSA_WANT_KEY_TYPE_xxx_KEYPAIR_LEGACY (or a better name) that we can use in tests as a replacement for the existing KEYPAIR macros. Work until that point would be executed as part of the driver-only ECC EPIC (tasks to be created by Manuel once this proposal is validate).
  2. Changing test code to use correct dependencies, and making sure configurations of interest (for example, RSA with key generation) are supported and only bring in the code they actually need, would be scheduled later as part of work on code size.

@gilles-peskine-arm
Copy link
Contributor Author

We add a convenience macro with suffix ALL

I don't think this macro is needed. Few users care about derivation, for example. And it's unclear what we might add in the future that would be also covered by ALL. For example, when we add support for fancy import/export formats, would they be covered by ALL?

@ronald-cron-arm
Copy link
Contributor

One question, what would be the definition of the macro with suffix USE?

@gilles-peskine-arm
Copy link
Contributor Author

PSA_WANT_KEY_TYPE_xxx_KEY_PAIR_USE enables support for the specified type of key pairs in operations for which an algorithm that can use this key type is also enabled. For example, PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_USE combined with PSA_WANT_ALG_RSA_PSS enables RSASSA-PSS with supported hashes through psa_sign_{hash,message} (as well as the verification functions because key pair support automatically enables public key support).

Currently, PSA_WANT_KEY_TYPE_xxx_KEY_PAIR automatically enables PSA_WANT_KEY_TYPE_xxx_PUBLIC_KEY. In the revised scheme, PSA_WANT_KEY_TYPE_xxx_KEY_PAIR_USE automatically enables PSA_WANT_KEY_TYPE_xxx_PUBLIC_KEY — or equivalently, any PSA_WANT_KEY_TYPE_xxx_KEY_PAIR_yyy automatically enables PSA_WANT_KEY_TYPE_xxx_PUBLIC_KEY.

@ronald-cron-arm
Copy link
Contributor

PSA_WANT_KEY_TYPE_xxx_KEY_PAIR_USE enables support for the specified type of key pairs in operations for which an algorithm that can use this key type is also enabled. For example, PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_USE combined with PSA_WANT_ALG_RSA_PSS enables RSASSA-PSS with supported hashes through psa_sign_{hash,message} (as well as the verification functions because key pair support automatically enables public key support).

Regarding RSASSA-PSS above do you mean that for a user to express "I want only RSASSA-PSS signature verification" he/she would define PSA_WANT_ALG_RSA_PSS but not PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_USE?

@mpg
Copy link
Contributor

mpg commented May 12, 2023

That's my understanding:

  1. PSA_WANT_ALG_RSA_PSS + PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY means "I want support for RSA-PSS signature verification"
  2. PSA_WANT_ALG_RSA_PSS + PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_USE means "I want support for RSA-PSS signature generation"

We currently don't support one without the other, but in the future we want to support 1 without 2. (Though 2 will always imply 1 AFAICS.)

@ronald-cron-arm
Copy link
Contributor

That's my understanding:

1. `PSA_WANT_ALG_RSA_PSS` + `PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY` means "I want support for RSA-PSS signature verification"

2. `PSA_WANT_ALG_RSA_PSS` + `PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_USE` means "I want support for RSA-PSS signature generation"

We currently don't support one without the other, but in the future we want to support 1 without 2. (Though 2 will always imply 1 AFAICS.)

With this you cannot express I want RSA-PSS signature generation and RSA-PKCS1V15 signature verification but not generation if I understand correctly. I am not sure there is any value in such a setting but this reveals a weakness in the scheme to me. Not simpler and/or cleaner to have rather PSA_WANT_ALG_RSA_PSS_VERIFICATION/SIGNATURE to express this and no _USE macros for the key types?

@mpg
Copy link
Contributor

mpg commented May 12, 2023

Good point. However I think it's not specific for asymmetric algorithms/keys. We have the same situation for example with block ciphers and modes: there's no way to say "I want only AES-GCM and ARIA-CBC but not AES-CBC or ARIA-GCM". You can only say I want "CBC and GCM algs and AES and ARIA key types" and then you get the full matrix. I seem to remember that was by design.

Btw this has a non-trivial impact on the decision process for what built-in version to include in the presence of drivers. If you have a driver for say AES-GCM and another for ARIA-CBC and these are the only combinations you're going to use, then you'll still end up with build-in AES, ARIA, GCM and CBC in your build. So, in terms of driver-only support, this is suboptimal if these situations exist/matter - which I have no idea if they do.

I would assume that highly constrained devices will tend to support only one block cipher, which trivialises the matrix and makes the problem go away. But perhaps the situation can arise for moderately-constrained devices?

@gilles-peskine-arm
Copy link
Contributor Author

With this you cannot express I want RSA-PSS signature generation and RSA-PKCS1V15 signature verification but not generation

Indeed. This sort of limitation is acceptable because as you note, it isn't something that people particularly care about. Even if there are users who might want such an arrangement (only generate modern PSS signatures, but accept legacy v1.5 signatures when verifying), there is not much gain in code size or attack surface by not including the code for v1.5 signature generation.

My original proposal for a configuration description was a list of capabilities with a JSON concrete syntax, similar to a list of capabilities offered by a driver. But this went nowhere because there are many users who want to describe the configuration entirely with booleans expressed as preprocessor macros being on or off. So the scheme has to be simplified.

Not simpler and/or cleaner to have rather PSA_WANT_ALG_RSA_PSS_VERIFICATION/SIGNATURE to express this and no _USE macros for the key types?

In practice, an implementation needs what we're calling PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_USE to decide whether to include RSA in macros like PSA_EXPORT_KEY_PAIR_MAX_SIZE and in code like public key export. So having additional macros like PSA_WANT_ALG_RSA_PSS_VERIFICATION would make the scheme more complex, whether this is an ad hoc thing for RSA (being ad hoc makes things more complex) or we have macros for all combinations of key type + algorithm + operation (PSA_WANT_AES_CMAC_VERIFY, PSA_WANT_AES_CBC_DECRYPT, …).

@mpg
Copy link
Contributor

mpg commented May 17, 2023

I created a task for the first step, that will be executed as part of the driver-only ECC work: #7614

The only deviation from this message is I omitted the _ALL macro.

@mpg
Copy link
Contributor

mpg commented May 23, 2023

@gilles-peskine-arm @ronald-cron-arm I'm afraid we completely forgot to discuss the impact on BUILTIN and ACCEL macros, which usually mirror PSA_WANT macros.

I think for the BUILTIN macros we're quite free to do whatever we want, as these are completely internal. (And I think what we want is change them to the new scheme immediately, at least for ECC, which was the initial motivation for all this.)

But for the ACCEL macros, aren't they part of the temporary de facto driver interface for now?

If so, do we want to provide some migration help as well, like if someone defines ACCEL_KEY_TYPE_ECC_KEYPAIR we'll translate that to ACCEL_KEY_TYPE_ECC_KEYPAIR_USE, _IMPORT, _EXPORT, _GENERATE, but not _DERIVE is it can't be accelerated so far? Plus, if they have MBEDTLS_DEPRECATED_WARNING (resp. ERROR) defined, a #warning (resp. #error) message?

@gilles-peskine-arm
Copy link
Contributor Author

Indeed the ACCEL macros are part of the de facto driver interface, until we finish the JSON→C generator. They're intended to have the same semantics as PSA_WANT unless there's a good reason to deviate. So I thought we'd just use the same semantics, including transition symbols. But we might obsolete the transition symbols sooner.

@mpg
Copy link
Contributor

mpg commented Jun 15, 2023

Note: while updating the documentation, we noticed USE is not the best name, and we'll be switching to BASIC unless someone objects. See #7774 (comment)

gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Jun 15, 2023
per Mbed-TLS#7439 (comment)
and Mbed-TLS#7774 (comment)

State that EXPORT implies BASIC.

Also fix missing `WANT_` parts.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Jul 27, 2023
It's ok if people use MBEDTLS_PSA_CRYPTO_CONFIG: it's not unstable or
unpredictable. But we still reserve the right to make minor changes
(e.g. Mbed-TLS#7439).

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

Definition of done for this issue: a draft specification is merged into https://github.com/Mbed-TLS/mbedtls/blob/development/docs/proposed/psa-conditional-inclusion-c.md.

Resolved by #7774.

Follow-ups: issues to be created based on the specification.

#7614 and its follow-ups. The feature was released in Mbed TLS 3.5.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-psa PSA keystore/dispatch layer (storage, drivers, …) enhancement
Projects
None yet
Development

No branches or pull requests

3 participants