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

PSA drivers: specification for key derivation #5451

Merged

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Jan 24, 2022

Pass all the initial inputs in a single structure. It's impossible to pass
the inputs as soon as the application makes them available because the core
cannot know which driver to call until it receives the SECRET input.

Do support hiding the key material inside a secure element if the relevant
driver has all the requisite entry points.

Do cooked key derivation (i.e. derivation of non-raw keys) and key agreement
separately.

Fixes #3718

PR checklist

  • changelog not required - specification only, not implemented yet
  • backport not required - new feature specification
  • tests not required - documentation only

Pass all the initial inputs in a single structure. It's impossible to pass
the inputs as soon as the application makes them available because the core
cannot know which driver to call until it receives the SECRET input.

Do support hiding the key material inside a secure element if the relevant
driver has all the requisite entry points.

Do cooked key derivation (i.e. derivation of non-raw keys) and key agreement
separately.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added enhancement needs-design-approval needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review component-psa PSA keystore/dispatch layer (storage, drivers, …) size-s Estimated task size: small (~2d) labels Jan 24, 2022
* `psa_crypto_driver_key_derivation_get_input_size` and `psa_crypto_driver_key_derivation_get_input_bytes` for data inputs (steps that the application passes with `psa_key_derivation_input_bytes()` or `psa_key_derivation_input_key()`, excluding key inputs from the same secure element).
* `psa_crypto_driver_key_derivation_get_input_key` for key inputs (steps that the application passes with `psa_key_derivation_input_key()`, only for secure element drivers receiving a key from the same secure element).
* On a successful invocation of `psa_crypto_driver_key_derivation_get_input_size`, the core sets `*size` to the size of the desired input in bytes.
* On a successful invocation of `psa_crypto_driver_key_derivation_get_input_bytes`, the core fills the first *N* bytes of `output` with the desired input and sets `*output_length` to *N*, where *N* is the length of the input in bytes. The value of `output_size` must be at least *N*, otherwise this function fails with the status `PSA_ERROR_BUFFER_TOO_SMALL`.

Choose a reason for hiding this comment

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

I am probably just being dum, but why does get_input_bytes cause the core to write to 'output'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad choice of name on my part. The output parameter of psa_crypto_driver_key_derivation_get_input_bytes is where the core copies the input passed by the application. I'll change the name so it doesn't seem like it's an output of the KDF, the way “input” here is an input of the KDF.

Don't use “output” for an input of the KDF. It's correct in context (it's
the output of a function that copies the input of the KDF from core-owned
memory to driver-owned memory) but confusing.

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>
This is the case for all key creation in a secure element, but state it
explicitly where relevant.

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>
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

A few remaining typos, otherwise this looks good to me. Some open questions on "derive_key" but fine by me to address that later.

docs/proposed/psa-driver-interface.md Outdated Show resolved Hide resolved
docs/proposed/psa-driver-interface.md Outdated Show resolved Hide resolved
docs/proposed/psa-driver-interface.md Outdated Show resolved Hide resolved
docs/proposed/psa-driver-interface.md Outdated Show resolved Hide resolved
1. For a call to `psa_key_derivation_output_key()`, if the driver's capabilities indicate that its `"import_key"` entry point does not support the derived key, stop and return `PSA_ERROR_NOT_SUPPORTED`.
1. For a call to `psa_key_derivation_verify_key()`, if the driver has a `"key_derivation_verify_key"` entry point, call it and stop.
1. For a call to `psa_key_derivation_verify_key()` or `psa_key_derivation_verify_bytes()`, if the driver has a `"key_derivation_verify_bytes"` entry point, call the driver's `"export_key"` entry point on the key object that contains the expected value, call the `"key_derivation_verify_bytes"` entry point on the exported material, and stop.
1. Call the `"key_derivation_output_bytes"` entry point. The core may call this entry point multiple times to implement a single call from the application when deriving a cooked (non-raw) key as described below, or if the output size exceeds some implementation limit.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be more comfortable with two series of steps, one for the psa_key_derivation_output_bytes() and psa_key_derivation_output_key() APIs and one for the psa_key_derivation_verify_bytes() or psa_key_derivation_verify_key() APIs. But this is not a blocker.

mpg
mpg previously approved these changes Jun 2, 2023
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.

LGTM - or at least as good as we can do without actually implementing it to validate the design.

* `"key_derivation_input_step"` (optional): provide an extra input for the key derivation. This entry point is only mandatory in drivers that support algorithms that have extra inputs. See [“Key derivation driver long inputs”](#key-derivation-driver-long-inputs).
* `"key_derivation_output_bytes"` (mandatory): derive cryptographic material and output it. See [“Key derivation driver outputs”](#key-derivation-driver-outputs).
* `"key_derivation_output_key"`, `"key_derivation_verify_bytes"`, `"key_derivation_verify_key"` (optional, opaque drivers only): derive key material which remains inside the same secure element. See [“Key derivation driver outputs”](#key-derivation-driver-outputs).
* `"key_derivation_set_capacity"` (mandatory for opaque drivers that implement `"key_derivation_output_key"` for “cooked”, i.e. non-raw-data key types): update the capacity policy on the operation. See [“Key derivation driver operation capacity”](#key-derivation-driver-operation-capacity).
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, wording: perhaps it would be better to describe as "(optional): update the capacity policy on the operation. This entry point is only mandatory for ..." as with key_derivation_input_step above.

Edit, after reading the dedicated section: looks like I misunderstood and this is different: this only makes sense for opaque drivers (that implement...) but then when it makes sense it's mandatory. If that's the case, perhaps it could be made clearer: "only for ...., then mandatory". (We could try "(mandatory, only for..." that makes the disinction rely on the coma, so quite easy to miss. Or perhaps I'm overthinking this and we can just expect people to read the details in the section linked here?

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'm trying to find a reasonable compromise between imprecision and drowning the reader in a sea of words, and looking at this again to try to improve it I find myself converging back to the current wording. Other similar bullet points explain when an entry point is applicable, and are silent about the cases where the entry point is not applicable. This one follows the pattern: “mandatory ⟨if condition applies⟩”, and leaves “ignored/forbidden otherwise” (that's not clarified in the document, maybe it should be?) implicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it's difficult to find a good balance. I think we should make explicit the part "ignored/forbidden otherwise". It's currently implied by the first two paragraphs of the detailed section (at least that's when it became apparent to me), so I think that would be a good place. Perhaps at the end of the 2nd paragraph's first sentence, add "(for transparent drivers and opaque drivers that don't support "key_derivation_output_key"`, this entry point is ignored)". Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better verbose than ambiguous, so I've updated this.

docs/proposed/psa-driver-interface.md Outdated Show resolved Hide resolved
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
... if we think of a better name

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Some of the fallback mechanisms between the entry points were not described
corrrectly.

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

I'm removing the "needs-design-approval" label because at this stage of review, we've clearly converged to an agreement and are just discussing minor points.

mpg
mpg previously approved these changes Jun 5, 2023
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.

LGTM

Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

Some typos in the last commit I think, otherwise this looks good to me.

docs/proposed/psa-driver-interface.md Outdated Show resolved Hide resolved
docs/proposed/psa-driver-interface.md Outdated Show resolved Hide resolved
docs/proposed/psa-driver-interface.md Outdated Show resolved Hide resolved
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

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.

LGTM

@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 6, 2023
@gilles-peskine-arm
Copy link
Contributor Author

The CI is failing on platforms with a recent Python because this branch is older than #5840. This is a documentation-only change and the relevant jobs run on Linux with an older Python and have passed, so CI is ok for merging.

@gilles-peskine-arm gilles-peskine-arm merged commit 265ce7c into Mbed-TLS:development Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-psa PSA keystore/dispatch layer (storage, drivers, …) enhancement historical-reviewed Reviewed & agreed to keep legacy PR/issue priority-medium Medium priority - this can be reviewed as time permits size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Key derivation driver: opaque case
7 participants