Merge development into psa-api-1.0-beta #130

gilles-peskine-arm · 2019-05-27T12:38:37Z

Merge development into psa-api-1.0-beta. The conflicts fall into two buckets:

Most conflicts are due to the parallel development of the enrollment algorithm policy feature in the two branches, which was done this way because the feature in development could not be merged easily into psa-api-1.0-beta. I resolved these conflicts by taking the development done for the API branch in Implementation-specific extension: Keys may allow a second algorithm #128.
Some SSL files changed in the API branch and were removed in development. I resolved these conflicts by removing the files. The corresponding updates of PSA API usage will need to be made in mbedtls.

It was failing to set the key in the ENCRYPT direction before encrypting. This just happened to work for GCM and CCM. After re-encrypting, compare the length to the expected ciphertext length not the plaintext length. Again this just happens to work for GCM and CCM since they do not perform any kind of padding.

Closes #2003 see also #1658

Add the Wisun extended key usage oid and tests.

Add checks in `ssl_server2` that `MBEDTLS_X509_CRL_PARSE_C` is defined to fix compilation issue. Fixes #560.

Adjusted generate_query_config.pl to have a configuration option of including the crypto config. Turned on by default.

Adjusted generate_features to have a configuration option of including crypto config. Turned on by default.

* origin/pr/2106: x509.c: Fix potential memory leak in X.509 self test

* origin/pr/2189: Remove Circle CI script

* origin/pr/2192: Increase okm_hex buffer to contain null character Minor modifications to hkdf test Add explanation for okm_string size Update ChangeLog Reduce buffer size of okm Reduce Stack usage of hkdf test function

* origin/pr/2366: Change Perl to Python in test builds

When doing ABI/API checking, its useful to have a list of all the identifiers that are defined in the internal header files, as we do not promise compatibility for them. This option allows for a simple method of getting them for use with the ABI checking script.

* origin/pr/2405: Fix ChangeLog entry ordering Fix typo Add non-regression test for buffer overflow Improve documentation of mbedtls_mpi_write_string() Adapt ChangeLog Fix 1-byte buffer overflow in mbedtls_mpi_write_string()

* origin/pr/2463: Fix a rebase error Wrap lines at 80 columns Add NIST keywrap as a cipher mode Fix errors in AEAD test function

…pi-1.0-beta-merge_development_20190524 Conflicts: * library/ssl_cli.c, library/ssl_tls.c: Removed on the development branch. Keep them removed. * include/psa/crypto_extra.h, library/psa_crypto_storage.c, tests/suites/test_suite_psa_crypto.data, tests/suites/test_suite_psa_crypto.function, tests/suites/test_suite_psa_crypto_persistent_key.data, tests/suites/test_suite_psa_crypto_slot_management.data, tests/suites/test_suite_psa_crypto_slot_management.function: Modified on the development branch only to implement the enrollment algorithm, which has been reimplemented on the API branch. Keep the API branch.

generate_psa_constants.py was accidentally declared with an implicitly-Python2 shebang.

* Rename internal methods and fields to start with an underscore. * Rename global constants to uppercase. * Change methods that don't use self to be class methods or static methods as appropriate. No behavior change in this commit.

Pass Pylint by cleaning up the code where possible and silencing Pylint where I know better. No behavior change.

k-stachowiak

I have one major question regarding "always use submodule PR", and a minor remark in the Pylint handling commit.

k-stachowiak · 2019-05-30T11:40:48Z

library/CMakeLists.txt

-    x509write_csr.c
+# For files generated by the parent project (Mbed TLS) when building Mbed
+# Crypto as a submodule, ensure that the parent project instance is used.
+if(USE_CRYPTO_SUBMODULE)


Is this supposed to be here? We have already merged the "always use crypto submodule" PR, so I would expect this option to no longer exist, but maybe at this stage the "always..." PR has not yet been taken into account?

We've merged “always use crypto submodule” into Mbed TLS, but not yet dropped compatibility for previous Mbed TLS revisions in Mbed Crypto. In any case, that's out of scope here: the goal of this PR is solely to merge mbed-crypto/development into psa-api-1.0-beta, not to change anything that isn't affected by this merge.

so I would expect this option to no longer exist,

It'll continue to need to exist as long as Mbed Crypto can build both independently on its own, and as a submodule. Even if Mbed TLS always uses Mbed Crypto as a submodule, Mbed Crypto itself needs to support being built as a submodule or standalone.

k-stachowiak · 2019-05-30T13:33:28Z

tests/scripts/test_psa_constant_names.py

@@ -23,6 +23,8 @@ def __init__(self, filename, line_number):
        self.line_number = line_number

 class read_file_lines:
+    # Dear Pylint, conventionally, a context manager class name is lowercase.


Did you consider using https://docs.python.org/3/library/contextlib.html#contextlib.contextmanager here?

Would it help? This context manager doesn't the acquire/release pattern that is the typical use case of contextlib.contextmanager.

I presumed that could have silence the code style error, but I have just check, and it doesn't, so no, unfortunately it would not help. Please disregard my above comment.

AndrzejKurek · 2019-06-03T08:51:58Z

scripts/generate_psa_constants.py

@@ -179,6 +186,12 @@
 '''

 class MacroCollector:
+    """Collect PSA crypto macro definitions from C header files.


Strange indentation

Is it? What's strange about it?

The ending and beginning quotes are misaligned, and the comments from lines below start at the beginning of the line.

That's one of the valid indentation styles for docstrings. But I notice that other parts of the script use a different style, so I'll update this one to be consistent.

AndrzejKurek · 2019-06-03T08:59:07Z

@gilles-peskine-arm - would you be able to list the exact conflicts you've encountered, or is this data lost now? If you don't have the time for it, I can re-create it by running the merge myself in that case.

gilles-peskine-arm · 2019-06-03T09:10:40Z

@AndrzejKurek I didn't keep a list of conflicts. I only checked that apart from the deletion of ssl_*.c, they were all related to the enrollment algorithm policy feature, and given #128 the resolution for that is always to take the API branch. The “merge follow-up” commit is actually a semantic conflict where enrollment algorithm policy code from the development branch was taken in when it shouldn't have been, because there was no syntactic conflict.

Use PEP 257 indented docstring style, mostly: always with """, with the terminating """ on a separate line if the docstring is more than one line, and with all lines indented to the opening """. This commit does not change the text to keep the first paragraph single-line.

Patater · 2019-06-04T12:29:37Z

API compatibility test is failing. This is because we are changing the API on purpose and is OK.

key attributes changed
alg2 added
new enums

Patater · 2019-06-04T12:30:21Z

TLS tests with the API branch are expected to fail until TLS is updated to use the new API.

Patater · 2019-06-04T18:08:24Z

I've performed the merge myself and ran into minimal differences: I had chose to keep using local variables to hold correctly typed parameters. I see your merge decided to inconsistently remove the typed local variables. Was this deliberate?

In test_suite_psa_crypto_slot_management.function, why sometimes use a local variable for typing the function parameter and sometimes not? e.g. target_alg is used as a correctly typed variable, but target_alg2_arg is used directly, not through a correctly typed variable; expected_usage_arg turns into expected_usage, but target_usage_arg does not in copy_success.
In ``test_suite_psa_crypto.function, why remove use of local variables for typing function parameter names? e.g. target_alg` used to be present, but now `target_alg_arg` is used directly.

Patater · 2019-06-04T18:09:44Z

Just one question about typed argument locals, otherwise LGTM

gilles-peskine-arm · 2019-06-04T18:20:22Z

These were not conscious decisions: for this PR I systematically took the API branch where there was a conflict. Maybe I ended up with slightly inferior code when I ported the feature in #128. I'll look at the specific cases.

gilles-peskine-arm · 2019-06-04T18:31:20Z

As far as I can tell, alg2 is always treated consistently with alg.

_arg parameters add complexity, so if I could, I wouldn't use them. I use them when it's a choice between that and casting the parameter upon use. Parameters need an _arg and a correctly-typed variable when they're used in comparisons, or in other contexts where a promotion might not do the right thing or might trigger a compiler warning. Some parameters get the _arg treatment just because they're close to another one that needs it. Some undoubtedly get it because they're the result of copy-paste of code that needed it. We don't have a policy on their usage and I'd rather not waste time on defining a policy: that time would be better used improving the test framework to support correctly-typed parameters.

So 1. I don't see any reason why this isn't a correct merge (of two states that might each be flawed on their own); 2. I don't see any actual flaw in the result.

gilles-peskine-arm · 2019-06-05T08:45:24Z

CI is ok except for TLS (expected) and ABI checking (the script reported a pass but failed due to a known issue for which the fix isn't merged yet). @Patater Since this has two approvals and your reservations are minor, I'm merging this PR to unblock the follow-ups.

Jarno Lamsa and others added 30 commits April 1, 2019 14:58

Change docs according to review comments

f49fedc

Remove mbedtls_ from the static function name

2ee67a6

Address review comments regarding ssl_client2 and ssl tests

f7a7f9e

Address comments for x509 tests

dfd22c4

Fix name to function call

9822c0d

Add NIST keywrap as a cipher mode

ffdf288

Closes #2003 see also #1658

Wrap lines at 80 columns

5f28999

Add Wisun Fan device extended key usage

b6dc105

Add the Wisun extended key usage oid and tests.

Improve documentation of mbedtls_ssl_conf_verify()

7b58fb1

Add context-specific CRT verification callbacks

726c97a

Implement context-specific verification callbacks

8927c83

Add cmd to use context-specific CRT callback in ssl_client2

bb425db

Add test exercising context-specific CRT callback to ssl-opt.sh

efb440a

Fix doxygen documentation of mbedtls_ssl_set_verify()

f345baf

Add test for configuration specific CRT callback

ee11be6

Fix typo

924270f

Add guards for MBEDTLS_X509_CRL_PARSE_C in sample

80d0419

Add checks in `ssl_server2` that `MBEDTLS_X509_CRL_PARSE_C` is defined to fix compilation issue. Fixes #560.

Fix a rebase error

274024f

Include crypto config when generating query config

ef90760

Adjusted generate_query_config.pl to have a configuration option of including the crypto config. Turned on by default.

Add crypto includes when generating features in generate_features.pl

79369cd

Adjusted generate_features to have a configuration option of including crypto config. Turned on by default.

Add description of generate_query_config.pl argument

b4b1ae1

Add an option to use crypto source files in generated visual c project

92f91fc

Merge remote-tracking branch 'origin/pr/2106' into development

1b86e4c

* origin/pr/2106: x509.c: Fix potential memory leak in X.509 self test

Merge remote-tracking branch 'origin/pr/2189' into development

6fc099d

* origin/pr/2189: Remove Circle CI script

Merge remote-tracking branch 'origin/pr/2192' into development

3d81447

* origin/pr/2192: Increase okm_hex buffer to contain null character Minor modifications to hkdf test Add explanation for okm_string size Update ChangeLog Reduce buffer size of okm Reduce Stack usage of hkdf test function

Merge remote-tracking branch 'origin/pr/2366' into development

c7acf56

* origin/pr/2366: Change Perl to Python in test builds

Merge remote-tracking branch 'origin/pr/2463' into development

d192ba4

* origin/pr/2463: Fix a rebase error Wrap lines at 80 columns Add NIST keywrap as a cipher mode Fix errors in AEAD test function

gilles-peskine-arm added needs: review The pull request is ready for review. This generally means that it has no known issues. api-spec Issue or PR about the PSA specifications labels May 27, 2019

gilles-peskine-arm force-pushed the psa-api-1.0-beta-merge_development_20190524 branch from 017b013 to f3bde03 Compare May 27, 2019 13:01

gilles-peskine-arm added the needs: preceding PR Requires another PR to be merged first label May 27, 2019

gilles-peskine-arm mentioned this pull request May 27, 2019

Remove old key creation functions and update cipher and pk #131

Merged

AndrzejKurek self-requested a review May 29, 2019 07:33

gilles-peskine-arm added 6 commits May 29, 2019 09:57

Merge follow-up: remove unused code from the development branch

6f3c30e

Switch script to Python3

0bbad74

generate_psa_constants.py was accidentally declared with an implicitly-Python2 shebang.

Add a bit of documentation

9d4d750

Obey Python naming and method structure conventions

42a0a0a

* Rename internal methods and fields to start with an underscore. * Rename global constants to uppercase. * Change methods that don't use self to be class methods or static methods as appropriate. No behavior change in this commit.

Pacify Pylint

54f5445

Pass Pylint by cleaning up the code where possible and silencing Pylint where I know better. No behavior change.

gilles-peskine-arm force-pushed the psa-api-1.0-beta-merge_development_20190524 branch from 20f72da to 54f5445 Compare May 29, 2019 08:04

gilles-peskine-arm removed the needs: preceding PR Requires another PR to be merged first label May 29, 2019

k-stachowiak reviewed May 30, 2019

View reviewed changes

AndrzejKurek reviewed Jun 3, 2019

View reviewed changes

k-stachowiak approved these changes Jun 3, 2019

View reviewed changes

AndrzejKurek approved these changes Jun 5, 2019

View reviewed changes

gilles-peskine-arm merged commit c143b31 into ARMmbed:psa-api-1.0-beta Jun 5, 2019

Patater removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Jul 10, 2019

Merge development into psa-api-1.0-beta #130

Merge development into psa-api-1.0-beta #130

Uh oh!

Conversation

gilles-peskine-arm commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k-stachowiak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrzejKurek commented Jun 3, 2019

Uh oh!

gilles-peskine-arm commented Jun 3, 2019

Uh oh!

Patater commented Jun 4, 2019

Uh oh!

Patater commented Jun 4, 2019

Uh oh!

Patater commented Jun 4, 2019

Uh oh!

Patater commented Jun 4, 2019

Uh oh!

gilles-peskine-arm commented Jun 4, 2019

Uh oh!

gilles-peskine-arm commented Jun 4, 2019

Uh oh!

gilles-peskine-arm commented Jun 5, 2019

Uh oh!

Uh oh!

gilles-peskine-arm commented May 27, 2019 •

edited

Loading