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

Update PSA_WANT spec for new KEY_PAIR scheme #7774

Merged
merged 2 commits into from
Jun 16, 2023

Conversation

mpg
Copy link
Contributor

@mpg mpg commented Jun 14, 2023

Description

Resolves #7744. Part of the text shamelessly stolen from #7766.

Context: #7439 (comment) (and preceding + following discussion).

Implementation: #7641 and follow-up issues #7771, #7772 and #7773.

PR checklist

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg mpg added 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 size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Jun 14, 2023
@mpg mpg self-assigned this Jun 14, 2023
Copy link
Contributor

@valeriosetti valeriosetti left a comment

Choose a reason for hiding this comment

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

All good, only one question left.


For asymmetric cryptography, `PSA_WANT_KEY_TYPE_xxx_KEY_PAIR` determines whether private-key operations are desired, and `PSA_WANT_KEY_TYPE_xxx_PUBLIC_KEY` determines whether public-key operations are desired. `PSA_WANT_KEY_TYPE_xxx_KEY_PAIR` implicitly enables `PSA_WANT_KEY_TYPE_xxx_PUBLIC_KEY`: there is no way to only include private-key operations (which typically saves little code).
As an exception, starting in Mbed TLS 3.5.0, for `KEY_PAIR` types (that is, private keys for asymmetric cryptography), the feature selection is more fine-grained, with an additional suffix:
* `PSA_WANT_KEY_TYPE_xxx_KEY_PAIR_USE` enables support for operations with a key of that type (for enabled algorithms). This is automatically enabled if any key creation method (`IMPORT`, `GENERATE` or `DERIVE`) is enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is automatically enabled if any key creation method (IMPORT, GENERATE or DERIVE) is enabled.

Actually I think I'm enabling USE even for the EXPORT case. It makes sense in my opinion: also exporting requires some sort of support, i.e. USE. 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.

Yes, in my mind any KEY_PAIR_xxx should auto-enable KEY_PAIR_USE. I stole this text from @gilles-peskine-arm and failed to notice EXPORT was excluded.

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 that when I wrote this, my thinking was: we auto-enable USE whenever there's any way to get a key into the system. EXPORT doesn't factor into that. But “whenever there's any way to get a key into the system” not exactly “if any creation method is enabled”: a system could have a built-in key (provisioned by proprietary means) and no way to create a new one.

I think what bothers me is that USE is not exactly about using keys. It includes using keys, but it's also about knowing that at least some aspect of the key type is supported. For example, it's a promise that metadata function/macros (e.g. xxx_MAX_SIZE) take that key type into account. (Although I don't think we currently have any relevant macro that's unambiguously in the public API (either they're tied to export or they're tied to public-key support or they're tied to curve/group support or they're tied to algorithm support.)

So I'm now thinking that USE isn't a good name. @mpg originally proposed BASIC, which I think is a better name. I don't remember why we changed that name to USE. With the name BASIC, it's more intuitive that any form of support automatically enables BASIC.

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, I tend to agree that the actual meaning of what we currently call USE is not just about using the key to perform operations, it's more generally "the PSA system knows about this key type".

I have nothing against switching to name that better reflects this, such as BASIC. I'm adding a note about this in the original discussion, and if nobody objects, I'll update this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to me as well, so I'll update #7641 accordingly

gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request 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>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg mpg requested a review from valeriosetti June 16, 2023 08:38
Copy link
Contributor

@valeriosetti valeriosetti 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

@gilles-peskine-arm gilles-peskine-arm 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, and aligned with what we're doing in #7641.


For asymmetric cryptography, `PSA_WANT_KEY_TYPE_xxx_KEY_PAIR_BASIC` determines whether private-key operations are desired, and `PSA_WANT_KEY_TYPE_xxx_PUBLIC_KEY` determines whether public-key operations are desired. `PSA_WANT_KEY_TYPE_xxx_KEY_PAIR_BASIC` implicitly enables `PSA_WANT_KEY_TYPE_xxx_PUBLIC_KEY`, as well as support for `psa_export_public_key` on the private key: there is no way to only include private-key operations (which typically saves little code).

Note: the implementation is always free to include support for more than what was explicitly requested. (For example, as of Mbed TLS 3.5.0, `PSA_WANT_KEY_TYPE_xxx_KEY_PAIR_BASIC` implicitly enables import and export support for that key type, but this may not be the case in future versions.) Applications should always request support for all operations they need, rather than rely on them being implicitly enabled by the implementation. The only thing that is documented and guaranteed in the future is as follows: `PSA_WANT_KEY_TYPE_xxx_KEY_PAIR_yyy` -> `PSA_WANT_KEY_TYPE_xxx_KEY_PAIR_BASIC` -> `PSA_WANT_KEY_TYPE_xxx_PUBLIC_KEY`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: a bare > is fragile. I'd prefer to write “implies”, or at least use an actual arrow character →.

@gilles-peskine-arm gilles-peskine-arm 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, needs-reviewer This PR needs someone to pick it up for review labels Jun 16, 2023
@gilles-peskine-arm gilles-peskine-arm merged commit 44ff5a9 into Mbed-TLS:development Jun 16, 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 priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update specification for new PSA_WANT symbols
3 participants