-
Notifications
You must be signed in to change notification settings - Fork 98
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
Merge psa api branch into development #212
Merge psa api branch into development #212
Conversation
psa_set_key_lifetime and psa_set_key_id aren't pure setters: they also set the other attribute in some conditions. Add dedicated tests for this behavior.
In psa_generate_derived_key, change the order of parameters to pass the pointer where the newly created handle will be stored last. This is consistent with most other library functions that put inputs before outputs.
psa_import_key now takes an attribute structure, not a type.
In psa_import_key, change the order of parameters to pass the pointer where the newly created handle will be stored last. This is consistent with most other library functions that put inputs before outputs.
When importing a private elliptic curve key, require the input to have exactly the right size. RFC 5915 requires the right size (you aren't allow to omit leading zeros). A different buffer size likely means that something is wrong, e.g. a mismatch between the declared key type and the actual data.
…am-order Pass handle parameter last on key creation
…_macro PSA: EC curve size macro
…olicy Add policy usage flag to copy a key
…part-delay Multipart AEAD buffer output sizes
Make key ids global and define their range
…utes-set_persistent Individual setters for persistent key attributes
Generators are mostly about key derivation (currently: only about key derivation). "Generator" is not a commonly used term in cryptography. So favor "derivation" as terminology. Call a generator a key derivation operation structure, since it behaves like other multipart operation structures. Furthermore, the function names are not fully consistent. In this commit, I rename the functions to consistently have the prefix "psa_key_derivation_". I used the following command: perl -i -pe '%t = ( psa_crypto_generator_t => "psa_key_derivation_operation_t", psa_crypto_generator_init => "psa_key_derivation_init", psa_key_derivation_setup => "psa_key_derivation_setup", psa_key_derivation_input_key => "psa_key_derivation_input_key", psa_key_derivation_input_bytes => "psa_key_derivation_input_bytes", psa_key_agreement => "psa_key_derivation_key_agreement", psa_set_generator_capacity => "psa_key_derivation_set_capacity", psa_get_generator_capacity => "psa_key_derivation_get_capacity", psa_generator_read => "psa_key_derivation_output_bytes", psa_generate_derived_key => "psa_key_derivation_output_key", psa_generator_abort => "psa_key_derivation_abort", PSA_CRYPTO_GENERATOR_INIT => "PSA_KEY_DERIVATION_OPERATION_INIT", PSA_GENERATOR_UNBRIDLED_CAPACITY => "PSA_KEY_DERIVATION_UNLIMITED_CAPACITY", ); s/\b(@{[join("|", keys %t)]})\b/$t{$1}/ge' $(git ls-files)
More consistent with the new function names.
perl -pe 's/crypto_generator/key_derivation/gi' $(git ls-files) perl -pe 's/_generator/_key_derivation/gi' $(git ls-files)
Generators are mostly about key derivation (currently: only about key derivation). "Generator" is not a commonly used term in cryptography. So favor "derivation" as terminology. This commit updates the function descriptions.
Generators are now key derivation operations. Keep "random generator" intact.
After renaming several identifiers, re-wrap and re-indent some lines to make the code prettier.
Present key derivation functions in a more logical order, corresponding roughly to the order in which an application would call them.
There is less of a risk of confusion with the KA+KDF function now.
There was some copypasta from the KA+KDF function's description.
…to_derivation Replace "generator" with "key derivation"
Move psa_get_key_domain_parameters() and psa_set_key_domain_parameters() out of the official API and declare them to be implementation-specific extensions. Expand the documentation of psa_set_key_domain_parameters() a bit to explain how domain parameters are used. Remove all mentions of domain parameters from the documentation of API functions. This leaves DH and DSA effectively unusable.
Parametrize finite-field Diffie-Hellman key types with a DH group identifier, in the same way elliptic curve keys are parametrized with an EC curve identifier. Define the DH groups from the TLS registry (these are the groups from RFC 7919). Replicate the macro definitions and the metadata tests from elliptic curve identifiers to DH group identifiers. Define PSA_DH_GROUP_CUSTOM as an implementation-specific extension for which domain parameters are used to specify the group.
Move DSA-related key types and algorithms to the implementation-specific header file. Not that we actually implement DSA, but with domain parameters, we should be able to.
When psa_generate_random fails, psa_generate_key_internal frees the key buffer but a the pointer to the now-freed buffer in the slot. Then psa_generate_key calls psa_fail_key_creation which sees the pointer and calls free() again. This bug was introduced by ff5f0e7 "Implement atomic-creation psa_{generate,generator_import}_key" which changed how psa_generate_key() cleans up on errors. I went through the code and could not find a similar bug in cleanup on an error during key creation. Fix ARMmbed#207
Add tests that call psa_generate_random() (possibly via psa_generate_key()) with a size that's larger than MBEDTLS_CTR_DRBG_MAX_REQUEST. This causes psa_generate_random() to fail because it calls mbedtls_ctr_drbg_random() without taking the maximum request size of CTR_DRBG into account. Non-regression test for ARMmbed#206
mbedtls_ctr_drbg_random can only return up to MBEDTLS_CTR_DRBG_MAX_REQUEST (normally 1024) bytes at a time. So if more than that is requested, call mbedtls_ctr_drbg_random in a loop.
…ttributes-and-slots Tidy up attribute management inside psa_crypto
…ta-merge_development_20190801 Merge mbed-crypto/development into psa-api-1.0-beta
The test framework has changed, but it did not cause any merge conflicts. Still it affected new code in the tests.
The crypto CI only fails on the API check, which is correct, because we are changing the API and the TLS tests are not expected to pass without Mbed-TLS/mbedtls#2767. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the changes looking for signs of mis-merge and found none. I only noticed one thing I'd like to confirm we want in the develepment branch (see review comments).
library/psa_crypto.c
Outdated
} | ||
|
||
return( PSA_SUCCESS ); | ||
} | ||
|
||
static void psa_abort_operations_using_key( psa_key_slot_t *slot ) | ||
{ | ||
/*FIXME how to implement this?*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to merge this into development?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are happy with some code path not being fully complete in Mbed Crypto and PSA support is still marked as experimental in Mbed TLS.
Regarding the comment itself, @gilles-peskine-arm prefers to keep them and I can't see any benefit of removing them either.
Still being consistent and call all of the comments of this kind TODO
every time would look better. I'll add a commit with fixing this and all the TOnogrepDO
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this into development and follow up with #221 soon after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing this. However, I have seen a todo issue checker in one version of the crypto library. If that is placed in the development branch, the todos will get pointed out as issues. Is that planned to happen, or has it been removed from check-files.py
?
I don't mind comments like this, I just wanted to raise this as a potential unwanted change; maybe some task was not finished, or the necessary changes have got lost by the merge algorithm.
If this is supposed not to be implemented yet, I don't mind the way it is commented, especially that "FIXME" is one of the canonical terms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find the word TODO
in check-files.py
and @gilles-peskine-arm said that he remembers that we removed that check. So I think it shouldn't be a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check-files.py
used to proscribe “TODO” but we removed this (#2587) and I used “TOnogrepDO” while waiting for that change to propagate to the API branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my question. Looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're accepting the TODO's for now, this PR is fine.
@k-stachowiak , @AndrzejKurek Thank you very much for the reviews! @gilles-peskine-arm Can we merge this? |
@yanesca I haven't looked at the code, but it's a straightforward merge and CI is happy. I don't want to merge this until Mbed-TLS/mbedtls#2767 is ready though. I'd rather not merge Mbed-TLS/mbedtls#2767 until we fully understand the implications on our users. This is an API change, even if it's in an API ( |
Tested merge at https://jenkins-internal.mbed.com/blue/organizations/jenkins/mbedtls-psa-release-new/detail/mbedtls-psa-release-new/497/ Mbed OS testing didn't work due to the CI system having a bug, but otherwise all tests passing. |
There were no merge conflicts. (Thanks to the recent merge of development into the API branch.)
This PR has to be merged in tandem with Mbed-TLS/mbedtls#2767.