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 new key agreement function #101

Merged
merged 12 commits into from
Dec 5, 2023

Conversation

athoelke
Copy link
Contributor

@athoelke athoelke commented Sep 15, 2023

Add psa_key_agreement() as a standalone function that outputs to a new derivation key.

Fixes #85

@athoelke
Copy link
Contributor Author

After some further consideration on the open issues:

  1. Is it acceptable for the function to be required to return a key of type PSA_KEY_TYPE_DERIVE? Should we require, or even permit, the caller to specify a key type?

In alignment with the other APIs that return a key, this one should require that caller does set the key type in the attributes.

  • An implementation must permit PSA_KEY_TYPE_DERIVE and PSA_KEY_TYPE_RAW
  • All asymmetric keys (both public-keys and key-pairs) are not permitted (INVALID_ARGUMENT)
  • It is reasonably safe to use the output as an HMAC key or as a PASSWORD (for input to PAKE or password-stretching algorithms). Should we universally permit such key types?
  • Use of the output as a symmetric block-cipher or stream-cipher keys is strongly discouraged (for some ciphers this could lead to key discovery). Should we universally deny such key types?
  1. Is it acceptable for the function to be required to return the entire shared secret as the key value? Should we require, or even permit, the caller to specify a key size?

Truncating the output is unnecessary, and risks making the shared secret discoverable. This function should always return the entire shared secret. In alignment with psa_import_key() and psa_copy_key() where the key size can be determined from other inputs, the caller should be permitted to either provide attributes with a bit size of zero, or with a bit size that exactly matches the key agreement output size (in bits).

doc/crypto/api/ops/ka.rst Outdated Show resolved Hide resolved
doc/crypto/api/ops/ka.rst Outdated Show resolved Hide resolved
doc/crypto/api/ops/ka.rst Outdated Show resolved Hide resolved
doc/crypto/api/ops/ka.rst Outdated Show resolved Hide resolved
doc/crypto/api/ops/ka.rst Outdated Show resolved Hide resolved
@MarcusJGStreets
Copy link
Contributor

MarcusJGStreets commented Oct 31, 2023 via email

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.

The overall design looks good to me, but there are editorial mistakes and some places are unclear.

doc/crypto/api/ops/ka.rst Outdated Show resolved Hide resolved
doc/crypto/api/ops/ka.rst Outdated Show resolved Hide resolved
doc/crypto/api/ops/ka.rst Show resolved Hide resolved
doc/crypto/api/ops/ka.rst Show resolved Hide resolved
doc/crypto/api/ops/ka.rst Outdated Show resolved Hide resolved
doc/crypto/api/ops/ka.rst Outdated Show resolved Hide resolved
doc/crypto/api/ops/ka.rst Outdated Show resolved Hide resolved
doc/crypto/api/ops/ka.rst Outdated Show resolved Hide resolved
doc/crypto/api/ops/ka.rst Show resolved Hide resolved
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.

LGTM except for one problem with shared secret size vs key size.

doc/crypto/appendix/history.rst Outdated Show resolved Hide resolved
doc/crypto/api/ops/ka.rst Outdated Show resolved Hide resolved
@athoelke athoelke force-pushed the crypto-standalone-key-agreement branch from 7331c53 to a719ef3 Compare December 5, 2023 11:49
@athoelke athoelke dismissed gilles-peskine-arm’s stale review December 5, 2023 11:51

Reviewer is happy for the PR to now be merged

@athoelke athoelke merged commit 538b297 into ARM-software:main Dec 5, 2023
@athoelke athoelke deleted the crypto-standalone-key-agreement branch December 5, 2023 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Key agreement interface may present the shared key material out of the secured space.
4 participants