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

Evolution of pk.h in 4.0 #8452

Open
mpg opened this issue Oct 31, 2023 · 12 comments · May be fixed by #9555
Open

Evolution of pk.h in 4.0 #8452

mpg opened this issue Oct 31, 2023 · 12 comments · May be fixed by #9555
Labels
api-break This issue/PR breaks the API and must wait for a new major version component-crypto Crypto primitives and low-level interfaces needs-design-approval

Comments

@mpg
Copy link
Contributor

mpg commented Oct 31, 2023

This issue is meant as a place to discuss what we want to do with PK in 4.0.

There are two main options:

  1. Keep it as part of the public API until 5.0, but aim to make it a thin wrapper around PSA Crypto (in 4.0 or 4.x).
  2. Remove it from the public API in 4.0, and eventually remove it entirely (in 4.0 or 4.x).

Scope of PK

  • Crypto operations similar to two PSA Crypto families: asymmetric signature (RSA, ECDSA) and asymmetric encryption (RSA).
    • Note: that's not all asymmetric crypto: key agreement (ECDH) and PAKE (EC J-PAKE) are not covered.
  • Writing and parsing of public and private keys (ECC, RSA) in a variety of formats currently not supported by PSA.
  • The pk_context structure serves as a container for public/private keys, which tend to be longer-lived and more often user-provided.

Note: for low-level modules related to asymmetric crypto (rsa.h, ecdsa.h covered by PK, but also ecdh.h and ecjpake.h, and underneath them ecp.h and bignum.h) I think the plan is to remove them from the public API, regardless of what we do with PK.

The problem of X.509 vs the PSA key store

With option 1 and pk_context structures being thin wrappers around psa_key_id_t, or with option 2 and blindly replacing all uses of pk_context with psa_key_id_t and no other code change, we'd have the following problem:

  • It is not unusual with X.509 to have a more than a hundred trust roots (currently 147 on my Ubuntu laptop).
  • When parsing these (for example with mbedtls_x509_crt_parse_path()) a psa_key_id_t would be created to hold each certificate's public key, resulting in 100+ PSA key slots being used simultaneously.
  • But currently the default size of the PSA key store (MBEDTLS_PSA_KEY_SLOT_COUNT) is 32.
  • Hence this use case would break in the default config.

There are a number of ways to avoid that problem; we can act at any step in the chain leading to that potential issue:

  • The easiest and most obvious would be to up the default value of MBEDTLS_PSA_KEY_SLOT_COUNT to something like 256.
  • We could also change X.509 so that it doesn't load the key into a pk_context/psa_key_id_t when parsing a certificate, but only when actually doing an operation with the key. This might be desirable for other reasons and have even been done in the baremetal branch as part of "on-demand parsing" which significantly improved RAM usage. But it requires designing new APIs for users to access the public key embedded in a certificate.
  • If we keep PK but make it a thin wrapper around PSA, we could go with a hybrid approach where it stores private keys into PSA key slots, but public keys serialized in a way that psa_import() accepts - and then only load them into a temporary PSA key slot when used. (Note: that's the approach currently used by MBEDTLS_PK_USE_PSA_EC_DATA.)

Rationale for option 1 (keep)

This gives users more time to move to PSA. People who were using low-level modules (including those not covered by PK) need to migrate immediately, but those who were only using the generic PK API can migrate at a convenient time between now and 5.0.

Work needed for option 1 (keep)

In 4.0:

  • We may still want to remove some parts:
  • We might want to remove mbedtls_pk_info_t from the API in order to make it slicker but that would require users to change their code, so it runs against the stated goal. (Alternatively, we can keep the structure but later make it trivial as it has no public field.)
  • We need some investigation (probably including a prototype) to make sure the current API can be implemented efficiently as a thin wrapper around PSA. Note: for crypto operations, this is already mostly the case with USE_PSA_CRYPTO; when using mbedtls_pk_setup_opaque() (or when MBEDTLS_PK_USE_PSA_EC_DATA is defined) that's also the case for (ECC) key storage.

In 4.0 or 4.x:

  • Actually make it a thin wrapper around PSA crypto.
  • Migrate all internal users (library and tests) to use PSA Crypto directly (note: common with option 2).

Rationale for option 2 (remove)

This leaves us with only one PK API to maintain, document and test. For new users, this also gives more clarity.

Note the current pk.h API has a few shortcomings, the most prominent being it doesn't manage key permissions like PSA does. So, pushing users to the PSA API is also pushing them to a better API.

Work needed for option 2 (remove)

In 4.0:

  • Design and implement a PSA replacement for PK parse. Note: that's probably an EPIC on its own, but also something we want to do anyway.
  • Update all TLS and X.509 APIs that handle a pk_context:
    • X.509: mbedtls_x509write_crt_set_subject_key(), mbedtls_x509write_crt_set_issuer_key() and mbedtls_x509write_csr_set_key() + public fields in mbedtls_x509_crt and mbedtls_x509_csr structure, and some private fields too.
    • TLS: mbedtls_ssl_conf_own_cert() and mbedtls_ssl_set_hs_own_cert().
  • Move pk.h to an internal location and adapt all files that #include it.
  • Change or remove all example programs that used it.
  • Provide a migration guide.

In 4.0 or 4.x:

  • Refactor psa_crypto_cipher.c so that it no longer
  • Migrate all remaining internal users (library and tests) to use PSA Crypto directly (note: common with option 2).
  • Then remove Cipher from the code base entirely.

Other options

  • We could go for a mix: keep some parts but remove others. For example, we could keep PK parse or pk_context but remove the part about doing operations?
  • Other?
@gilles-peskine-arm
Copy link
Contributor

The general wisdom for API transitions is to have one full major version cycle where the new API is stable and the old API is present but deprecated. In Mbed TLS 3.6, we won't have a new API that covers the major features of the legacy pk API. In particular, PSA is missing the equivalent of pkparse and pkwrite.

Thus I don't see any choice. We need to keep pk (without its rough edges such as mbedtls_pk_ec and mbedtls_pk_rsa) in 4.x.

@mpg
Copy link
Contributor Author

mpg commented Nov 2, 2023

The general wisdom for API transitions is to have one full major version cycle where the new API is stable and the old API is present but deprecated.

I we want to follow that rule (which I think is generally speaking a good guideline), then even if we're keeping PK in 4.0, we should still provide PSA-based replacements for the X.509 and TLS APIs that use a key. So, for example, change mbedtls_ssl_conf_own_cert() to accept a PSA key slot instead of a pk_context and add a new, and immediately deprecated, function mbedtls_ssl_conf_own_cert_pk() - and similarly for the other APIs.

(Considering TLS and X.509 both use PK internally for now, the easiest way to implement this initially is for the new, PSA-based API to just internally wrap the PSA key in a PK context, and store the result.)

@mpg
Copy link
Contributor Author

mpg commented Nov 2, 2023

I forgot to note some optional features that PK parse has, for which it's unclear whether the upcoming PSA import extension will have support:

  • parsing ECC keys where the public key is in compressed format (Weierstrass curves);
  • parsing ECC keys where the curve is encoded as SpecifiedECDomain rather than NamedCurve.

(Note: PK parse only supports these options when at least some of the built-in implementation (ECP_LIGHT) is enabled.)

@gilles-peskine-arm
Copy link
Contributor

parsing ECC keys where the public key is in compressed format (Weierstrass curves);
parsing ECC keys where the curve is encoded as SpecifiedECDomain rather than NamedCurve.

Yes, we will use the PSA formatted import API to support those. But I fear that it won't be in 4.0, only in 4.1 or so.

@mpg
Copy link
Contributor Author

mpg commented Nov 2, 2023

If we don't have a complete replacement in 4.0 but only in 4.1, I think that's a rather strong argument for keeping PK public in 4.0.

@mpg
Copy link
Contributor Author

mpg commented Nov 2, 2023

Note (just for reference, I don't think it influences the decision to keep PK or not): currently, internal users of PK are:

  • TLS (even 1.3 and even with USE_PSA);
  • X.509 and PKCS7 (even with USE_PSA);
  • the built-in implementation of RSA in PSA (psa_crypto_rsa.c) uses PK parse and PK write.

@mpg
Copy link
Contributor Author

mpg commented Nov 20, 2023

I think there's a pretty strong argument for keeping PK in 4.0 so assuming we do, I think we need to be careful about its implementation and whether TLS & X.509 depend on it, in order to not make things harder for us in the future.

I think we want to avoid a situation where using TLS & X.509 require using PK and PK relies on psa_set_key_enrollment_algorithm() (I think it's OK if only one of this conditions is true), see part of the discussion at #8449.

More generally, I think currently our story about policies when PK internally stores key in PSA key slots (that is, for EC keys in builds where MBEDTLS_PK_USE_PSA_EC_DATA is defined) is not clean and we should clarify it. Might also be related with part of the discussion that happened here: #7930 (comment)

If we keep PK in 4.0, we probably need some hard thinking about its API and semantics.

@mpg
Copy link
Contributor Author

mpg commented Nov 22, 2023

We could go for a mix: keep some parts but remove others. For example, we could keep PK parse or pk_context but remove the part about doing operations?

The more I think about it, the more I like this option: keep PK-parse/write but remove the sign/verify, encrypt/decrypt APIs (hereunder PK-ops). Instead, the only thing you can to with a pk_context is make a PSA key out of it - and then you can use PSA APIs for crypto operations.

Rationale:

  • The strongest argument for keeping PK is that we don't have a replacement yet - this only applies to PK-parse/write.
  • The design of PK-ops is flawed and inconsistent when it comes to policies, key types and operations. Improving it requires significant work, and since it will involve incompatible API changes, would reduce the backward compatibility advantage of keeping it.

@gilles-peskine-arm gilles-peskine-arm moved this to Requirements needed in Mbed TLS 4.0 planning Jun 19, 2024
@gilles-peskine-arm gilles-peskine-arm added api-break This issue/PR breaks the API and must wait for a new major version component-crypto Crypto primitives and low-level interfaces labels Jun 19, 2024
@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Aug 7, 2024

Decision: we'll keep pk.h in TF-PSA-Crypto 1.0, with some modifications. Notably:

  • A pk object containing a private key will always have it in the form of a PSA key id.
  • A pk object containing a public key will either have it in the form of PSA key id or in the form of the PSA export representation.
  • All crypto operations are dispatched via PSA functions.
  • The pk module will use the same classification of key types and algorithms as PSA.
  • No more pk debug, RSA ALT, etc.

This will allow us to keep X.509 mostly unchanged. The role of PK will therefore be to parse/write keys, and as a container for keys used by X.509.

The main gap between PK and PSA is that PSA does not provide a way to parse or write keys in formats that contain metadata. There is ongoing work on adding such an API, but we're not sure when it'll be finalized, so it would be risky to rely on it for TF-PSA-Crypto 1.0. Furthermore, it's not clear how resource management would work if X.509 directly referred to PSA key IDs instead of PK contexts; we might keep the PK abstraction layer for that purpose anyway. We'll implement the PSA formatted key import/export functions in TF-PSA-Crypto 1.x. Then later I expect that we'll deprecate the pk module in TF-PSA-Crypto, to be removed from TF-PSA-Crypto 2.0, while we create a thinner pk layer which is just a container used in X.509 objects (and TLS will not refer to pk at all at that point).

@gilles-peskine-arm
Copy link
Contributor

Since we're going to remove PKCS#1v1.5 encryption from TLS (and even altogether), there's no longer a strong reason for the PK module to support encryption. So I think we'll drop mbedtls_pk_encrypt and mbedtls_pk_decrypt.

Very conveniently, this means we no longer to work on supporting RSA private keys whose policy allows both decryption and signature.

@mpg
Copy link
Contributor Author

mpg commented Aug 7, 2024

Question: what about TLS and X.509 APIs that use PK types? Based on your last paragraph, I assume in 4.0 they're going to remain unchanged, then in 4.x we add new alternatives based on PSA types or the thin abstraction layer you're mentioning, then deprecate the old ones and remove in 5.0?

I'm thinking mbedtls_ssl_conf_own_cert(), mbedtls_x509write_crt_set_subject_key() & Co, the public pk field in struct mbedtls_x509_crt, but also MBEDTLS_X509_ID_FLAG().

@gilles-peskine-arm
Copy link
Contributor

what about TLS and X.509 APIs that use PK types?

TBD. Whatever's convenient. I suspect that this is going to end up with: keep mbedtls_pk_context when it's placed in an X.509 structure, but switch to a key ID when all that's needed is the key. Or keep mbedtls_pk_context for public keys, but switch to key IDs for private keys. And those two might well end up being the same. That's for the detailed architecture to figure out.

@gilles-peskine-arm gilles-peskine-arm changed the title [discussion] The fate of PK in 4.0 Evolution of pk in 4.0 Aug 14, 2024
@gilles-peskine-arm gilles-peskine-arm changed the title Evolution of pk in 4.0 Evolution of pk.h in 4.0 Aug 14, 2024
@yanesca yanesca moved this to 4.0 - Prepare High Level Crypto in Mbed TLS Backlog Aug 27, 2024
@github-project-automation github-project-automation bot moved this to Mbed TLS 4.0 candidates in Backlog for Mbed TLS Aug 30, 2024
@gilles-peskine-arm gilles-peskine-arm linked a pull request Sep 10, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break This issue/PR breaks the API and must wait for a new major version component-crypto Crypto primitives and low-level interfaces needs-design-approval
Projects
Status: Mbed TLS 4.0 candidates
Status: Design needed
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants