Merge psa api branch into development #212

yanesca · 2019-08-09T07:56:29Z

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.

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

…pment

The test framework has changed, but it did not cause any merge conflicts. Still it affected new code in the tests.

yanesca · 2019-08-09T09:48:34Z

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.

k-stachowiak

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).

k-stachowiak · 2019-08-12T12:22:52Z

library/psa_crypto.c

    }

    return( PSA_SUCCESS );
 }

+static void psa_abort_operations_using_key( psa_key_slot_t *slot )
+{
+    /*FIXME how to implement this?*/


Do we want to merge this into development?

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 TOnogrepDOs.

Let's merge this into development and follow up with #221 soon after.

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.

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.

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.

k-stachowiak

Thanks for addressing my question. Looks good.

AndrzejKurek

If we're accepting the TODO's for now, this PR is fine.

yanesca · 2019-08-14T08:18:42Z

@k-stachowiak , @AndrzejKurek Thank you very much for the reviews!

@gilles-peskine-arm Can we merge this?

gilles-peskine-arm · 2019-08-14T10:35:04Z

@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 (psa_xxx) that few of our users use yet. What's the impact on Mbed OS? On TF-M? We should make sure we understand the impact or are prepared to deal with anything that breaks. Since I won't be around for the next two weeks, I'd rather hand that hot potato to @Patater.

Patater · 2019-08-16T09:59:18Z

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.

gilles-peskine-arm and others added 30 commits May 15, 2019 19:14

Add a few tests for persistent attributes

dd835cb

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.

Put handle parameter last: psa_generate_derived_key

98dd779

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.

Update an obsolete use of psa_import_key in documentation

806051f

psa_import_key now takes an attribute structure, not a type.

Put handle parameter last: psa_import_key

73676cb

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.

Split long lines after psa_import_key refactoring

049c753

New macro to get the bit size of an elliptic curve

6c95144

Fix unused variable in builds without storage

e2f62ba

Merge pull request ARMmbed#111 from gilles-peskine-arm/psa-handle-par…

beb0cc2

…am-order Pass handle parameter last on key creation

Merge pull request ARMmbed#107 from gilles-peskine-arm/psa-curve_size…

826e326

…_macro PSA: EC curve size macro

Merge pull request ARMmbed#108 from gilles-peskine-arm/psa-copy_key-p…

76be7f9

…olicy Add policy usage flag to copy a key

Merge pull request ARMmbed#102 from gilles-peskine-arm/psa-aead_multi…

16ab391

…part-delay Multipart AEAD buffer output sizes

Fix copypasta in the documentation of PSA_KEY_ID_xxx_{MIN,MAX}

280948a

Minor documentation improvements

f1b7694

Merge pull request ARMmbed#104 from gilles-peskine-arm/psa-global_key_id

99e8d26

Make key ids global and define their range

Merge pull request ARMmbed#109 from gilles-peskine-arm/psa-key_attrib…

d35249e

…utes-set_persistent Individual setters for persistent key attributes

Rename PSA_KDF_STEP_xxx -> PSA_KEY_DERIVATION_INPUT_xxx

03410b5

More consistent with the new function names.

Rename generator-related internal identifiers

cbe6650

perl -pe 's/crypto_generator/key_derivation/gi' $(git ls-files) perl -pe 's/_generator/_key_derivation/gi' $(git ls-files)

Terminology: say "key derivation operation", not "generator"

35675b6

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.

Rename "generator" to "operation"

51ae0e4

Generators are now key derivation operations. Keep "random generator" intact.

Wrap and reindent some lines

cf7292e

After renaming several identifiers, re-wrap and re-indent some lines to make the code prettier.

Reorder key derivation functions in the header file

1cb9a08

Present key derivation functions in a more logical order, corresponding roughly to the order in which an application would call them.

Shorten the name of psa_key_agreement_raw_shared_secret

be697d8

There is less of a risk of confusion with the KA+KDF function now.

Correct the description of psa_raw_key_agreement

58fe9e8

There was some copypasta from the KA+KDF function's description.

Merge pull request ARMmbed#113 from gilles-peskine-arm/psa-generator_…

eef988f

…to_derivation Replace "generator" with "key derivation"

Move DSA definitions out of the specification

e38ab1a

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.

Move remaining text about DSA out of the specification

a130219

gilles-peskine-arm and others added 6 commits August 7, 2019 13:43

Fix psa_generate_random for >1024 bytes

f181eca

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.

Merge pull request ARMmbed#197 from gilles-peskine-arm/psa-refactor-a…

82a5711

…ttributes-and-slots Tidy up attribute management inside psa_crypto

Merge pull request ARMmbed#198 from gilles-peskine-arm/psa-api-1.0-be…

0c77b0e

…ta-merge_development_20190801 Merge mbed-crypto/development into psa-api-1.0-beta

Merge branch 'psa-api-1.0-beta' into merge-psa-api-branch-into-develo…

8aa7e9b

…pment

yanesca added needs: preceding PR Requires another PR to be merged first needs: review The pull request is ready for review. This generally means that it has no known issues. needs: ci Needs a passing full CI run labels Aug 9, 2019

yanesca mentioned this pull request Aug 9, 2019

Update to PSA 1.0 beta 3 Mbed-TLS/mbedtls#2767

Merged

2 tasks

yanesca added needs: work The pull request needs rework before it can be merged. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Aug 9, 2019

Fix PSA tests

849b05a

The test framework has changed, but it did not cause any merge conflicts. Still it affected new code in the tests.

yanesca added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: ci Needs a passing full CI run needs: work The pull request needs rework before it can be merged. labels Aug 9, 2019

k-stachowiak reviewed Aug 13, 2019

View reviewed changes

Make TODO comments consistent

1d57a20

gilles-peskine-arm mentioned this pull request Aug 13, 2019

SE keys: fix psa_destroy_key; add Github issue numbers for missing code #221

Merged

yanesca requested review from AndrzejKurek and k-stachowiak August 13, 2019 14:49

k-stachowiak approved these changes Aug 13, 2019

View reviewed changes

AndrzejKurek approved these changes Aug 14, 2019

View reviewed changes

yanesca removed needs: preceding PR Requires another PR to be merged first needs: review The pull request is ready for review. This generally means that it has no known issues. labels Aug 14, 2019

Patater merged commit 1d57a20 into ARMmbed:development Aug 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge psa api branch into development #212

Merge psa api branch into development #212

yanesca commented Aug 9, 2019

yanesca commented Aug 9, 2019

k-stachowiak left a comment

k-stachowiak Aug 12, 2019

yanesca Aug 13, 2019

gilles-peskine-arm Aug 13, 2019

k-stachowiak Aug 13, 2019

yanesca Aug 13, 2019

gilles-peskine-arm Aug 13, 2019

k-stachowiak left a comment

AndrzejKurek left a comment

yanesca commented Aug 14, 2019 •

edited

Loading

gilles-peskine-arm commented Aug 14, 2019

Patater commented Aug 16, 2019

Merge psa api branch into development #212

Merge psa api branch into development #212

Conversation

yanesca commented Aug 9, 2019

yanesca commented Aug 9, 2019

k-stachowiak left a comment

Choose a reason for hiding this comment

k-stachowiak Aug 12, 2019

Choose a reason for hiding this comment

yanesca Aug 13, 2019

Choose a reason for hiding this comment

gilles-peskine-arm Aug 13, 2019

Choose a reason for hiding this comment

k-stachowiak Aug 13, 2019

Choose a reason for hiding this comment

yanesca Aug 13, 2019

Choose a reason for hiding this comment

gilles-peskine-arm Aug 13, 2019

Choose a reason for hiding this comment

k-stachowiak left a comment

Choose a reason for hiding this comment

AndrzejKurek left a comment

Choose a reason for hiding this comment

yanesca commented Aug 14, 2019 • edited Loading

gilles-peskine-arm commented Aug 14, 2019

Patater commented Aug 16, 2019

yanesca commented Aug 14, 2019 •

edited

Loading