Implement mbedtls_pk_import_into_psa #8807

gilles-peskine-arm · 2024-02-09T18:43:15Z

Resolves #8708. Resolves #8713.

Also fixes #7290.

Out of scope: testing with non-volatile keys (#8778).

PR checklist

changelog provided
backport of bug fixes: Backport 2.28: bugs fixed in "Implement mbedtls_pk_import_into_psa" #8811
tests provided

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Help make review efficient:

Multiple simple commits
- please structure your PR into a series of small commits, each of which does one thing
Avoid force-push
- please do not force-push to update your PR - just add new commit(s)
See our Guidelines for Contributors for more details about the review process.

This makes it possible to use the curve in test data. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

There was already code to instantiate the wildcard for sign/verify-hash. Make that work with sign/verify-message as well. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Implement mbedtls_pk_import_into_psa for all PK types except RSA_ALT. This covers importing a key pair, importing a public key and importing the public part of a key pair. Test mbedtls_pk_import_into_psa() with the output of mbedtls_pk_get_psa_attributes(). Also unit-test mbedtls_pk_import_into_psa() on its own to get extra coverage, mostly for negative cases. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Set unique configuration names in the outcome file. This was lost in the rewrite from depends-*.pl to depends.py. Fix Mbed-TLS#7290 Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

In some configurations (e.g. ECDH but no ECDSA or RSA), the PK module is useful but cannot perform any signatures. Then modern GCC complains: ``` ../source/tests/suites/test_suite_pk.function: In function ‘test_pk_sign_verify’: ../source/tests/suites/test_suite_pk.function:1136:12: error: array subscript 0 is outside array bounds of ‘unsigned char[0]’ [-Werror=array-bounds] ../source/tests/suites/test_suite_pk.function:1094:19: note: while referencing sig’ … ``` This fixes test-ref-configs.pl with a modern GCC (specifically with config-thread.h). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Fix the build in some configurations. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Parsing a key and importing it into PSA may result in a policy that specifies an algorithm that is not included in the build. This happens if the key type is supported, but not the algorithm, e.g. in a build with MBEDTLS_ECP_C but not MBEDTLS_ECDSA_C. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

This fixes the ability to exercise keys in configurations where MD5 is supported for direct use, but not inside some accelerated algorithms. This is the case in `all.sh test_psa_crypto_config_accel_ecc_ecp_light_only` and some other accelerated-ECC components of `all.sh`, where the driver is built without MD5 support but built-in MD5 remains enabled. This is only a hack, not a theoretically correct fix, but a correct fix is out of scope of my current work. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mpg

(Partial review, just releasing the notes I have so far before I leave for the day. next commit: 157679c)

tests/suites/test_suite_pk.function

tests/src/psa_exercise_key.c

library/pk.c

tests/suites/test_suite_pk.data

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The values are the same for all supported mechanisms (RSA-based), so no semantic change. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

This was only tested with opaque keys. Since the code paths are different depending on the PK type, we also need to test RSA and ECKEY. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Test that a PK key and a PSA key are consistent, i.e. that they have the same type (or are a key pair and the corresponding public key) and that they have the same public key. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

We were testing the internal consistency of the resulting key, and that the resulting key had the right metadata, but we were not testing that the PSA key had the expected key material. Comparing the public keys fixes that. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Fix the workaround for the weirdness of mbedtls_ecp_write_key(), which assumed a Weierstrass key. This fixes the Montgomery private key parse tests in test_suite_pkparse. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

valeriosetti

LGTM. I only left a couple of minor comments, which can be ignored if you think that they are not relevant.

valeriosetti · 2024-02-20T08:20:36Z

tests/src/psa_exercise_key.c

+#if defined(MBEDTLS_PK_USE_PSA_EC_DATA)
+        case MBEDTLS_PK_ECKEY:
+        case MBEDTLS_PK_ECKEY_DH:
+        case MBEDTLS_PK_ECDSA:
+            TEST_ASSERT(PSA_KEY_TYPE_IS_ECC(psa_type));
+            TEST_EQUAL(PSA_KEY_TYPE_ECC_GET_FAMILY(psa_type), pk->ec_family);
+            pk_public = pk->pub_raw;
+            pk_public_length = pk->pub_raw_len;
+            break;
+#endif /* MBEDTLS_PK_USE_PSA_EC_DATA */
+
+#if defined(MBEDTLS_PK_HAVE_ECC_KEYS) && !defined(MBEDTLS_PK_USE_PSA_EC_DATA)
+        case MBEDTLS_PK_ECKEY:
+        case MBEDTLS_PK_ECKEY_DH:
+        case MBEDTLS_PK_ECDSA:
+            TEST_ASSERT(PSA_KEY_TYPE_IS_ECC(psa_get_key_type(&psa_attributes)));
+            const mbedtls_ecp_keypair *ec = mbedtls_pk_ec_ro(*pk);
+            TEST_EQUAL(mbedtls_ecp_write_public_key(
+                           ec, MBEDTLS_ECP_PF_UNCOMPRESSED, &pk_public_length,
+                           pk_public_buffer, sizeof(pk_public_buffer)), 0);
+            pk_public = pk_public_buffer;
+            break;
+#endif /* MBEDTLS_PK_HAVE_ECC_KEYS && !MBEDTLS_PK_USE_PSA_EC_DATA */


Very minor, but why not having something like:

#if defined(MBEDTLS_PK_HAVE_ECC_KEYS) #if defined(MBEDTLS_PK_USE_PSA_EC_DATA) [...] #else /*MBEDTLS_PK_USE_PSA_EC_DATA*/ [...] #endif /*MBEDTLS_PK_USE_PSA_EC_DATA*/ #endif /*MBEDTLS_PK_HAVE_ECC_KEYS*/

?

I have that nesting in many places, but here the only thing that's shared between the USE_PSA_EC_DATA case and the legacy case is the case statements, so I found it more readable to make them separate.

valeriosetti · 2024-02-20T08:31:58Z

tests/suites/test_suite_pk.function

+    defined(MBEDTLS_ECDSA_C) ||                                         \
+    defined(MBEDTLS_USE_PSA_CRYPTO)


Shouldn't we replace these with MBEDTLS_PK_CAN_ECDSA_SIGN?

The list here matches the conditions for MBEDTLS_PK_SIGNATURE_MAX_SIZE to be nonzero, based on how it's set in include/mbedtls/pk.h. That was the motivation for 74860dd.

We can't remove MBEDTLS_RSA_C or MBEDTLS_PK_RSA_ALT_SUPPORT since these should be enabled even when PSA is disabled. We can't remove MBEDTLS_USE_PSA_CRYPTO because this covers MBEDTLS_PK_OPAQUE which can reach PSA-only signature algorithms.

Arguably we should replace defined(MBEDTLS_ECDSA_C) by MBEDTLS_PK_CAN_ECDSA_SIGN? I'm not sure it makes a difference in terms of maintainability. Intuitively, to me:

Either MBEDTLS_USE_PSA_CRYPTO is enabled, and this is always on: PK can at least try to sign through PSA (although it's possible that PSA doesn't actually support any signature algorithm, but we currently don't need to handle this refinement).

Or MBEDTLS_USE_PSA_CRYPTO is disabled, and then we know that PK is using the built-in features for anything other than MBEDTLS_PK_OPAQUE. All the built-in features that can sign are listed here.

By the way this is one of many places that need to change when MBEDTLS_PK_OPAQUE is generalized to work without MBEDTLS_USE_PSA_CRYPTO. I don't know of an easy way to find them all.

gilles-peskine-arm · 2024-02-20T20:10:22Z

There's now a merge conflict which is just changes on consecutive lines. To avoid disrupting the ongoing review, I propose to do a merge commit once the pull request has been approved.

mpg

Completed my review, LGTM, thanks!

You can now resolve conflicts - I'm fine with either merge or rebase, and for reference in case you rebase the commit I'm approving is 83b8baf

gilles-peskine-arm · 2024-02-21T11:10:17Z

I'm going to push the merge. In case there are test failures due to non-textual conflicts, this will make the history easier to understand.

…plement_import Conflicts: * tests/suites/test_suite_pk.function: consecutive changes to the depends_on line of pk_sign_verify and its argument list.

gilles-peskine-arm · 2024-02-21T11:18:51Z

Merge pushed, I hope the CI will pass (I only tested a few configs locally).

Also, if you have a bit more bandwidth, would you mind reviewing the small backport?

mpg

LGTM if the CI agrees

valeriosetti

LGTM

gilles-peskine-arm · 2024-03-04T17:35:38Z

include/mbedtls/pk.h

+ *                        psa_set_key_identifier() and optionally
+ *                        psa_set_key_lifetime().
+ *                      - To import only the public part of a key pair:
+ *                        ```


Doxygen doesn't recognize the backticks as meaning a code block here. I don't know what its rules are. Fixed in #8887.

gilles-peskine-arm added 2 commits February 9, 2024 19:31

Choose a curve for tests at compile time

48b87eb

This makes it possible to use the curve in test data. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mbedtls_pk_import_into_psa: documentation

05ee3fb

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm added enhancement component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Feb 9, 2024

This was referenced Feb 9, 2024

Test mbedtls_pk_get_psa_attributes and mbedtls_pk_import_into_psa with lifetime and key id #8778

Closed

Backport 2.28: bugs fixed in "Implement mbedtls_pk_import_into_psa" #8811

Merged

gilles-peskine-arm added the needs-backports Backports are missing or are pending review and approval. label Feb 12, 2024

gilles-peskine-arm added 10 commits February 12, 2024 17:30

exercise_key: allow SIGN_MESSAGE/VERIFY_MESSAGE with PSA_ALG_ANY_HASH

4781bd9

There was already code to instantiate the wildcard for sign/verify-hash. Make that work with sign/verify-message as well. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

exercise_key: fix asymmetric encrypt/decrypt with >2028-bit RSA

fdb809e

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mbedtls_pk_import_into_psa: negative tests for different ECC curve

10e9c41

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mbedtls_pk_import_into_psa: positive tests with pkparse output

157679c

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

depends.py: set unique configuration names in outcome file

35cb319

Set unique configuration names in the outcome file. This was lost in the rewrite from depends-*.pl to depends.py. Fix Mbed-TLS#7290 Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Fix some preprocessor guards

1d33876

Fix the build in some configurations. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm force-pushed the pk_import_into_psa-implement_import branch from fab62c4 to 465e4ed Compare February 12, 2024 18:59

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels Feb 13, 2024

gilles-peskine-arm requested review from valeriosetti and mpg February 13, 2024 08:52

mpg reviewed Feb 15, 2024

View reviewed changes

gilles-peskine-arm added 3 commits February 15, 2024 13:12

Rename identifier for consistency

cbd2cbb

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Fix encrypt/decrypt confusion

f50cd59

The values are the same for all supported mechanisms (RSA-based), so no semantic change. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Use named constants FROM_PAIR/FROM_PUBLIC for readability

68a287d

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

This was referenced Feb 15, 2024

Implement mbedtls_pk_copy_from_psa #8774

Merged

Unify consistency tests for mbedtls_pk_import_into_psa and mbedtls_pk_copy_from_psa #8836

Closed

gilles-peskine-arm added 5 commits February 15, 2024 17:15

Test mbedtls_pk_import_into_psa with different bits

d6fc350

This was only tested with opaque keys. Since the code paths are different depending on the PK type, we also need to test RSA and ECKEY. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Fix copypasta

4da1f01

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

New test helper: mbedtls_test_key_consistency_psa_pk

6fe8a06

Test that a PK key and a PSA key are consistent, i.e. that they have the same type (or are a key pair and the corresponding public key) and that they have the same public key. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm force-pushed the pk_import_into_psa-implement_import branch from c0ca571 to 83b8baf Compare February 15, 2024 16:27

valeriosetti previously approved these changes Feb 20, 2024

View reviewed changes

mpg previously approved these changes Feb 21, 2024

View reviewed changes

Merge remote-tracking branch 'development' into pk_import_into_psa-im…

dd49c73

…plement_import Conflicts: * tests/suites/test_suite_pk.function: consecutive changes to the depends_on line of pk_sign_verify and its argument list.

gilles-peskine-arm dismissed stale reviews from mpg and valeriosetti via dd49c73 February 21, 2024 11:17

gilles-peskine-arm added the needs-ci Needs to pass CI tests label Feb 21, 2024

mpg approved these changes Feb 21, 2024

View reviewed changes

valeriosetti approved these changes Feb 21, 2024

View reviewed changes

valeriosetti 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, labels Feb 21, 2024

gilles-peskine-arm added this pull request to the merge queue Feb 21, 2024

Merged via the queue into Mbed-TLS:development with commit 0aab69d Feb 21, 2024
4 of 6 checks passed

gilles-peskine-arm mentioned this pull request Mar 4, 2024

Fix intended code blocks that were not suitably indented #8887

Merged

3 tasks

gilles-peskine-arm commented Mar 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement mbedtls_pk_import_into_psa #8807

Implement mbedtls_pk_import_into_psa #8807

gilles-peskine-arm commented Feb 9, 2024 •

edited

Loading

mpg left a comment

valeriosetti left a comment

valeriosetti Feb 20, 2024

gilles-peskine-arm Feb 20, 2024

valeriosetti Feb 20, 2024

gilles-peskine-arm Feb 20, 2024

gilles-peskine-arm commented Feb 20, 2024 •

edited

Loading

mpg left a comment

gilles-peskine-arm commented Feb 21, 2024

gilles-peskine-arm commented Feb 21, 2024

mpg left a comment

valeriosetti left a comment

gilles-peskine-arm Mar 4, 2024

		defined(MBEDTLS_ECDSA_C) \|\| \
		defined(MBEDTLS_USE_PSA_CRYPTO)

Implement mbedtls_pk_import_into_psa #8807

Implement mbedtls_pk_import_into_psa #8807

Conversation

gilles-peskine-arm commented Feb 9, 2024 • edited Loading

PR checklist

Notes for the submitter

mpg left a comment

Choose a reason for hiding this comment

valeriosetti left a comment

Choose a reason for hiding this comment

valeriosetti Feb 20, 2024

Choose a reason for hiding this comment

gilles-peskine-arm Feb 20, 2024

Choose a reason for hiding this comment

valeriosetti Feb 20, 2024

Choose a reason for hiding this comment

gilles-peskine-arm Feb 20, 2024

Choose a reason for hiding this comment

gilles-peskine-arm commented Feb 20, 2024 • edited Loading

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Feb 21, 2024

gilles-peskine-arm commented Feb 21, 2024

mpg left a comment

Choose a reason for hiding this comment

valeriosetti left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Mar 4, 2024

Choose a reason for hiding this comment

gilles-peskine-arm commented Feb 9, 2024 •

edited

Loading

gilles-peskine-arm commented Feb 20, 2024 •

edited

Loading