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

Add extended key creation functions for non-default production methods #194

Merged
merged 5 commits into from
Sep 3, 2024

Conversation

athoelke
Copy link
Contributor

@athoelke athoelke commented Mar 28, 2024

This change has been adopted from Mbed TLS: Mbed-TLS/mbedtls#8815, with changes to make it compatible with C++ compilation.

  • Add extended key generation and derivation functions, psa_generate_key_ext() and psa_key_derivation_output_key_ext(), that accept additional parameters to control the key creation process.
  • Define a key production parameter to select a non-default exponent for RSA key generation.

Notes:

  • The alternative key generation method is described with the relevant key type (RSA), instead of where the production parameter structure type is defined (the approach in the Mbed TLS change).

Fixes #167

@athoelke athoelke added enhancement New feature or request API design Related the design of the API Crypto API Issue or PR related to the Cryptography API labels Mar 28, 2024
@athoelke athoelke added this to the Crypto API 1.3 milestone Mar 28, 2024
@athoelke athoelke self-assigned this Mar 28, 2024
@athoelke athoelke marked this pull request as draft April 9, 2024 09:41
@athoelke
Copy link
Contributor Author

athoelke commented Apr 9, 2024

For now - marked this as a draft PR. Some rework of the API is required.

It turns out that the variable-sized structure definition is not strictly legal in C++, and inclusion and use from C++ is an expected use case for the Crypto API.

typedef uint8_t psa_pake_step_t;
typedef struct psa_key_production_parameters_t {
uint32_t flags;
uint8_t data[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Yesterday I learned that flexible array members are not standard C++. It is generally expected that C++ compilers can consume C headers, so Mbed TLS and PSA should avoid making headers incompatible with C++.

In Mbed TLS, we'll stay with this API in 3.6.x LTS (too late, it's released), but we'd like to change the API to be C++-compatible in our next major release.

The obvious fix would be to separate the variable-length data out of the struct. This may require a little more RAM in client-server scenarios, but on the bright side it would save the hassle around mixing fixed-format and variable-length data.

@athoelke athoelke force-pushed the crypto-extended-key-generation branch 2 times, most recently from ed4a717 to d074a0f Compare May 10, 2024 13:05
@athoelke athoelke force-pushed the crypto-extended-key-generation branch from d074a0f to db30ef6 Compare May 10, 2024 15:18
@athoelke
Copy link
Contributor Author

Updated in line with the proposal in #167 (comment).

This is force-pushed to remove the uneccessary changes to the buffer parameter conventions. The changes between the earlier API in the PR are visible in the single commit db30ef6.

@athoelke athoelke marked this pull request as ready for review May 10, 2024 15:21
@athoelke
Copy link
Contributor Author

To support migration for applications using the beta version of this API in Mbed TLS, we need to consider if we can use a different function name for these new APIs.

@athoelke
Copy link
Contributor Author

Some ideas (including considered and discarded ones) for alternative function names:

  • psa_generate_key_extended() or psa_generate_key_extra() - unabbreviated versions of psa_generate_key_ext(), but this neither follows a recognised pattern of using _ext() in other APIs, nor inform the reader what the extended call adds to the original function.
  • psa_generate_key_custom() or psa_generate_custom_key() (the latter reads better, but a list of function names is less well ordered) - reflecting the customisation/parameterisation provided by the extra parameters.
  • psa_generate_key_parameterized() or psa_generate_parameterized_key() (the latter reads better, and also sorts immediately after the original function) - a bit longer, but better reflecting parameterisation provided by the extra parameters.
  • psa_generate_key_with_parameters() - literally does what it says, but is this a too close to writing prose as a function name?

Rejected ideas

  • psa_generate_key_new() - never a good idea to call an API 'new'. One day it won't be new anymore.
  • psa_generate_key_2() - this is a second 'generate key' function (or is it the third after psa_generate_key_ext()?), but that does not inform a reader/developer what makes it different.
  • psa_generate_key5() - prefix the overloaded versions with the number of parameters. A pattern used elsewhere. However, the function name does not help the reader understand what is different, and the extended key derivation function would have a 6 suffix, which does not create the obvious association between the two APIs.

Comment on lines 512 to 516
psa_status_t psa_generate_key_ext(const psa_key_attributes_t *attributes,
const psa_key_production_parameters_t *params,
const uint8_t *params_data,
size_t params_data_length,
mbedtls_svc_key_id_t *key);
Copy link
Contributor

Choose a reason for hiding this comment

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

In Mbed TLS 3.6.1, we want to keep the existing beta psa_generate_key_ext and psa_key_derivation_output_key_ext with a psa_key_production_parameters_t* parameter with a flexible array member, for backward compatibility, and also provide the new C++-compatible type and function with a different name. So we'd like the revised function and type names to be different from the ones in the beta. I propose to go back to “custom”:

psa_status_t psa_generate_key_custom(const psa_key_attributes_t *attributes,
                                     const psa_key_custom_production_t *custom,
                                     const uint8_t *custom_data,
                                     size_t custom_data_length,
                                     mbedtls_svc_key_id_t *key);

Copy link
Contributor

Choose a reason for hiding this comment

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

For the type, psa_key_custom_production_t or psa_custom_key_production_t? Both work. It's a custom key production (a key production that's custom), not a custom-key production (a production of a custom key). But custom_key_production is naturally understood as “custom-key production” which gives the wrong idea. So we lean towards psa_key_custom_production_t, which is less discoverable but less ambiguous.

@@ -24,6 +24,9 @@ typedef uint32_t psa_pake_primitive_t;
typedef uint8_t psa_pake_primitive_type_t;
typedef uint8_t psa_pake_role_t;
typedef uint8_t psa_pake_step_t;
typedef struct psa_key_custom_production_t {
uint32_t flags;
} psa_key_custom_production_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I'm typing it out, I feel the type name doesn't quite work: this data type doesn't exactly describe a “custom production”, but parameters for a custom production. The name makes me think of an operation context rather than configuration data. So psa_key_custom_parameters_t feels more natural, or maybe even psa_custom_key_production_parameters_t (I also feel that the key part is in a bit of a weird place). But maybe I'm overdoing it with the long descriptive name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind-of thought so too as I worked on the tweaks. My inclination would be to go for psa_custom_key_parameters_t, rather than more precise-but-long psa_custom_key_production_parameters_t. Key parameters/attributes are primarily used for key creation in the API, so I think we can get away without 'production' in the name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I've updated Mbed-TLS/mbedtls#9235 to use psa_custom_key_parameters_t.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've updated the PR with this identifier name.

@athoelke athoelke force-pushed the crypto-extended-key-generation branch from 49cc0ef to bde1d2d Compare July 30, 2024 16:04
@athoelke
Copy link
Contributor Author

Rebased to sync with main.

@athoelke athoelke merged commit ae0af92 into ARM-software:main Sep 3, 2024
@athoelke athoelke deleted the crypto-extended-key-generation branch September 3, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API design Related the design of the API Crypto API Issue or PR related to the Cryptography API enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

Custom methods for key generation and key derivation
3 participants