gh-Mbed-TLS/TF-PSA-Crypto
Watch
1
0
Fork
You've already forked TF-PSA-Crypto
 0
Code Issues 167 Pull requests 92 Releases 5 Activity

PSA PAKE: support persistent keys created with Mbed TLS 3.x #506

Merged
gilles-peskine-arm merged 13 commits from jpake-persistent-key-compat-1.0 into release/tf-psa-crypto1.0.0-documentation 2025-10-09 09:19:58 -05:00
Conversation 11 Commits 13 Files changed 8 +325 -87
gilles-peskine-arm commented 2025-10-08 10:50:30 -05:00 (Migrated from github.com)

Support persistent PAKE keys created with Mbed TLS 3.x. Without this patch, such keys are rejected since the encoding of PAKE algorithms has changed. There is no way to fix this from application code unless the key is exportable. Fixes https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/505, including a test.

Also document PAKE limitations (https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/502, https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/503, https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/504).

PR checklist

  • changelog not required because: fixes a bug that was never released (if this PR is merged in 1.0.0)
  • framework PR not required
  • mbedtls development PR not required because: crypto only
  • mbedtls 3.6 PR provided https://github.com/Mbed-TLS/mbedtls/pull/10444
  • tests provided
Support persistent PAKE keys created with Mbed TLS 3.x. Without this patch, such keys are rejected since the encoding of PAKE algorithms has changed. There is no way to fix this from application code unless the key is exportable. Fixes https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/505, including a test. Also document PAKE limitations (https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/502, https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/503, https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/504). ## PR checklist - [x] **changelog** not required because: fixes a bug that was never released (if this PR is merged in 1.0.0) - [x] **framework PR** not required - [x] **mbedtls development PR** not required because: crypto only - [x] **mbedtls 3.6 PR** provided https://github.com/Mbed-TLS/mbedtls/pull/10444 - **tests** provided
gilles-peskine-arm (Migrated from github.com) reviewed 2025-10-08 10:58:02 -05:00
tests/suites/test_suite_psa_crypto_storage_format.misc.data
@ -12,1 +12,4 @@
PSA storage save: PASSWORD-JPAKE(SHA_256)
depends_on:PSA_WANT_ALG_JPAKE:PSA_WANT_ALG_SHA_256:CAN_EXERCISE_JPAKE
key_storage_save:0x0001:PSA_KEY_TYPE_PASSWORD:8:PSA_KEY_USAGE_DERIVE | PSA_KEY_USAGE_EXPORT:PSA_ALG_JPAKE(PSA_ALG_SHA_256):PSA_ALG_NONE:"4c":"505341004b455900000000000100000003120800014000000901000a00000000010000004c"
gilles-peskine-arm (Migrated from github.com) commented 2025-10-08 10:58:02 -05:00

The storage format is documented in https://github.com/Mbed-TLS/mbedtls/blob/mbedtls-3.6.0/docs/architecture/mbed-crypto-storage-specification.md#key-file-format-for-mbed-tls-2250 (it hasn't change since Mbed TLS 2.25). There is Python code to construct storage content in mbedtls_framework.psa_storage.

However, you can have reasonable confidence that the test data is what it should be, without inspecting the string, in the following way:

  • key_storage_save constructs a key with the specified content, and checks that the resulting file content is identical to what is passed as the last parameter.
  • key_storage_read goes the other way round. Thus, if a key_storage_read test case has a matching key_storage_save test cases, you can have strong confidence that the representation matches what the library does.
The storage format is documented in https://github.com/Mbed-TLS/mbedtls/blob/mbedtls-3.6.0/docs/architecture/mbed-crypto-storage-specification.md#key-file-format-for-mbed-tls-2250 (it hasn't change since Mbed TLS 2.25). There is Python code to construct storage content in [`mbedtls_framework.psa_storage`](https://github.com/Mbed-TLS/mbedtls-framework/blob/d80c4f9ec3a01c001778658023f82e40fdb51d40/scripts/mbedtls_framework/psa_storage.py#L154). However, you can have reasonable confidence that the test data is what it should be, without inspecting the string, in the following way: * `key_storage_save` constructs a key with the specified content, and checks that the resulting file content is identical to what is passed as the last parameter. * `key_storage_read` goes the other way round. Thus, if a `key_storage_read` test case has a matching `key_storage_save` test cases, you can have strong confidence that the representation matches what the library does.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-10-08 10:59:40 -05:00
tests/suites/test_suite_psa_crypto_storage_format.misc.data
@ -10,6 +10,30 @@ PSA storage save: AES-GCM+CTR
depends_on:PSA_WANT_KEY_TYPE_AES
gilles-peskine-arm (Migrated from github.com) commented 2025-10-08 10:59:40 -05:00

This test case has no matching key_storage_save test case, since the library no longer officially allows such a key. However, what matters most here is that this is how a JPAKE key was represented in Mbed TLS 3.x, as evidenced by the key_storage_save test case in https://github.com/Mbed-TLS/mbedtls/pull/10444 with identical file content.

This test case has no matching `key_storage_save` test case, since the library no longer officially allows such a key. However, what matters most here is that this is how a JPAKE key was represented in Mbed TLS 3.x, as evidenced by the `key_storage_save` test case in https://github.com/Mbed-TLS/mbedtls/pull/10444 with identical file content.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-10-08 11:51:41 -05:00
tests/suites/test_suite_psa_crypto_storage_format.function
@ -191,13 +258,22 @@ static int test_read_key(const psa_key_attributes_t *expected_attributes,
exported_material, length);
gilles-peskine-arm (Migrated from github.com) commented 2025-10-08 11:51:41 -05:00 
#endif
}

I intend to amend the last commit to fix this, to facilitate future git bisect. I'm waiting for the CI to progress, in case there are other similar problems.

```suggestion #endif } ``` I intend to amend the last commit to fix this, to facilitate future `git bisect`. I'm waiting for the CI to progress, in case there are other similar problems.
mpg (Migrated from github.com) approved these changes 2025-10-09 05:20:17 -05:00
mpg (Migrated from github.com) left a comment

LGTM, thanks!

I only have minor feedback, all of which can be left for a post-release follow-up (if it needs addressing).

LGTM, thanks! I only have minor feedback, all of which can be left for a post-release follow-up (if it needs addressing).
include/psa/crypto_compat.h
@ -42,6 +42,23 @@ int psa_can_do_hash(psa_algorithm_t hash_alg);
*/
#define PSA_KEY_TYPE_DES ((psa_key_type_t) 0x2301)
/** The beta encoding of JPAKE algorithms, with no hash.
mpg (Migrated from github.com) commented 2025-10-09 04:41:41 -05:00

Question: should we even include that one in the doxygen output?

Question: should we even include that one in the doxygen output?
include/psa/crypto_extra.h
@ -537,0 +544,4 @@
* - When using the built-in implementation, the user ID and the peer ID
* must be `"client"` (6-byte string) and `"server"` (6-byte string),
* or the other way round.
* Third-party drivers may or may not have this limitation.
mpg (Migrated from github.com) commented 2025-10-09 02:54:52 -05:00

Note: also, user and peer ID can't have the same value (for example they can't both be "client"). I'm not sure if that's worth mentioning explicitly. We support exactly two cases:

  1. user="client", peer="server"
  2. user="server", peer="client"
Note: also, user and peer ID can't have the same value (for example they can't both be "client"). I'm not sure if that's worth mentioning explicitly. We support exactly two cases: 1. user="client", peer="server" 2. user="server", peer="client"
tests/suites/test_suite_psa_crypto_pake.function
mpg (Migrated from github.com) commented 2025-10-09 03:02:21 -05:00

Previously we were injecting a large input either at the PUBLIC step or at the PROOF step. Now we're only injecting it at the PUBLIC step. Is that intentional?

(Non-blocking: if there's anything to change if can be done in a follow-up.)

Previously we were injecting a large input either at the `PUBLIC` step or at the `PROOF` step. Now we're only injecting it at the `PUBLIC` step. Is that intentional? (Non-blocking: if there's anything to change if can be done in a follow-up.)
tests/suites/test_suite_psa_crypto_pake.function
mpg (Migrated from github.com) commented 2025-10-09 03:03:28 -05:00

Previously we were testing just 1 above the normal size for this step, now we're testing 1 above the maximum size over all steps. Again, was that intentional? (Again, non-blocking.)

Previously we were testing just 1 above the normal size for this step, now we're testing 1 above the maximum size over all steps. Again, was that intentional? (Again, non-blocking.)
tests/suites/test_suite_psa_crypto_storage_format.function
@ -141,0 +168,4 @@
*
* We use one particular cipher suite (picking the
* hash algorithm from the key policy), so test cases must depend on
* CAN_EXERCISE_JPAKE or else setup will fail.
mpg (Migrated from github.com) commented 2025-10-09 04:57:00 -05:00

Minor: this macro is not actually used by tests, which currently directly depend on PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_IMPORT:PSA_WANT_ECC_SECP_R1_256. This is equivalent, so everything's functionally correct, but in terms of readability we should either stop defining that macro, or actually use it.

Minor: this macro is not actually used by tests, which currently directly depend on `PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_IMPORT:PSA_WANT_ECC_SECP_R1_256`. This is equivalent, so everything's functionally correct, but in terms of readability we should either stop defining that macro, or actually use it.
mpg commented 2025-10-09 05:35:22 -05:00 (Migrated from github.com)

Interestingly the stability checks have failed to run on this PR. Seems possibly related to using a non-default target branch.

Interestingly the stability checks have failed to run on this PR. Seems possibly related to using a non-default target branch.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-10-09 05:52:37 -05:00
include/psa/crypto_compat.h
@ -42,6 +42,23 @@ int psa_can_do_hash(psa_algorithm_t hash_alg);
*/
#define PSA_KEY_TYPE_DES ((psa_key_type_t) 0x2301)
/** The beta encoding of JPAKE algorithms, with no hash.
gilles-peskine-arm (Migrated from github.com) commented 2025-10-09 05:52:36 -05:00

I wasn't sure, but it is legitimately visible to applications if they query the policy of a key in storage, so I think it should.

I wasn't sure, but it is legitimately visible to applications if they query the policy of a key in storage, so I think it should.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-10-09 06:00:44 -05:00
tests/suites/test_suite_psa_crypto_pake.function
gilles-peskine-arm (Migrated from github.com) commented 2025-10-09 06:00:44 -05:00

The call for the ZK_PROOF output would also have been out-of-sequence, not just with an invalid size. So it wasn't actually serving its stated purpose. I didn't deliberately set off to remove it, but keeping it wouldn't be right either, so I don't propose to bring it back. What should really happen would be to split ERR_INJECT_{LARGE,SMALL}_BUFFER_SIZE into ERR_INJECT_{LARGE,SMALL}_BUFFER_SIZE_AT_STEP_xxx, but that's out of scope here, I've added a note to https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/508.

The call for the `ZK_PROOF` output would also have been out-of-sequence, not just with an invalid size. So it wasn't actually serving its stated purpose. I didn't deliberately set off to remove it, but keeping it wouldn't be right either, so I don't propose to bring it back. What should really happen would be to split `ERR_INJECT_{LARGE,SMALL}_BUFFER_SIZE` into `ERR_INJECT_{LARGE,SMALL}_BUFFER_SIZE_AT_STEP_xxx`, but that's out of scope here, I've added a note to https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/508.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-10-09 06:02:40 -05:00
tests/suites/test_suite_psa_crypto_pake.function
gilles-peskine-arm (Migrated from github.com) commented 2025-10-09 06:02:40 -05:00

The intent was a wrong size, and as far as I can tell with no particular attention to how the size was wrong. (In particular I recall being a little surprised that we didn't always check one less than the expected size, which would catch the most likely mistake: attempting to read/write the expected amount of data without checking that there the buffer was large enough.) So I just treated it as “pass the wrong buffer size” when I split the output buffer sizes.

The intent was a wrong size, and as far as I can tell with no particular attention to how the size was wrong. (In particular I recall being a little surprised that we didn't always check one less than the expected size, which would catch the most likely mistake: attempting to read/write the expected amount of data without checking that there the buffer was large enough.) So I just treated it as “pass the wrong buffer size” when I split the output buffer sizes.
mpg (Migrated from github.com) approved these changes 2025-10-09 06:08:10 -05:00
mpg (Migrated from github.com) left a comment

LGTM, thanks!

LGTM, thanks!
tests/suites/test_suite_psa_crypto_storage_format.misc.data
@ -13,0 +22,4 @@
depends_on:PSA_WANT_ALG_JPAKE:PSA_WANT_ALG_SHA_256:CAN_EXERCISE_JPAKE
key_storage_save:0x0001:PSA_KEY_TYPE_PASSWORD:8:PSA_KEY_USAGE_DERIVE:PSA_ALG_JPAKE(PSA_ALG_SHA_256):PSA_ALG_NONE:"4c":"505341004b455900000000000100000003120800004000000901000a00000000010000004c"
PSA storage read: PASSWORD-JPAKE(SHA_256) (non-exportable)
mpg (Migrated from github.com) commented 2025-10-09 06:08:00 -05:00

Minor: do we really need CAN_EXERCISE_JPAKE for save tests?

Minor: do we really need `CAN_EXERCISE_JPAKE` for `save` tests?
gilles-peskine-arm (Migrated from github.com) reviewed 2025-10-09 07:32:46 -05:00
tests/suites/test_suite_psa_crypto_storage_format.misc.data
@ -13,0 +22,4 @@
depends_on:PSA_WANT_ALG_JPAKE:PSA_WANT_ALG_SHA_256:CAN_EXERCISE_JPAKE
key_storage_save:0x0001:PSA_KEY_TYPE_PASSWORD:8:PSA_KEY_USAGE_DERIVE:PSA_ALG_JPAKE(PSA_ALG_SHA_256):PSA_ALG_NONE:"4c":"505341004b455900000000000100000003120800004000000901000a00000000010000004c"
PSA storage read: PASSWORD-JPAKE(SHA_256) (non-exportable)
gilles-peskine-arm (Migrated from github.com) commented 2025-10-09 07:32:46 -05:00

That's a good point! We don't. I was following the pattern in the storage tests where read and save test cases have the same dependencies (on the key type and the algorithm), even though save tests only need the key type to be supported, not the algorithm.

In general, requiring the algorithm to be supported makes our tests more widely applicable, since other implementations of the PSA crypto API specification (and possibly we in the future) are allowed to refuse to create a key whose policy specifies an unsupported algorithm. On the other hand, in the case of JPAKE, CAN_EXERCISE_JPAKE is specifically about the primitive, which is not specified in the key policy, this reason doesn't apply.

We don't actually care about expanding the test coverage: it doesn't matter whether it's possible to create a persistent PASSWORD-JPAKE key in configurations without secp256r1. So this isn't a test gap, it's just something surprising in the code.

Given the urgency of this pull request, I'll only remove the unnecessary dependency if I need to do some other rework.

That's a good point! We don't. I was following the pattern in the storage tests where read and save test cases have the same dependencies (on the key type and the algorithm), even though save tests only need the key type to be supported, not the algorithm. In general, requiring the algorithm to be supported makes our tests more widely applicable, since other implementations of the PSA crypto API specification (and possibly we in the future) are allowed to refuse to create a key whose policy specifies an unsupported algorithm. On the other hand, in the case of JPAKE, `CAN_EXERCISE_JPAKE` is specifically about the primitive, which is not specified in the key policy, this reason doesn't apply. We don't actually care about expanding the test coverage: it doesn't matter whether it's possible to create a persistent PASSWORD-JPAKE key in configurations without secp256r1. So this isn't a test gap, it's just something surprising in the code. Given the urgency of this pull request, I'll only remove the unnecessary dependency if I need to do some other rework.
minosgalanakis (Migrated from github.com) approved these changes 2025-10-09 08:40:24 -05:00
minosgalanakis (Migrated from github.com) left a comment

LGTM

LGTM
Sign in to join this conversation.
Reviewers
No reviewers
mpg
minosgalanakis
Labels
Clear labels   
api-break
This issue/PR breaks the API and must wait for a new major version

  
approved
Design and code approved - may be waiting for CI or backports

  
bug
Something isn't working

  
component-docs
Docs / website issues filed here for tracking

  
component-platform
Portability layer and build scripts

  
component-test
Test framework and CI scripts

  
dependencies
Pull requests that update a dependency file

  
deprecation
needs deprecation announcement

  
DO-NOT-MERGE

   
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
This issue is not being actively worked on, but PRs welcome.

  
needs-adoption
stalled PR that someone should pick up and complete

  
needs-backports
Backports are missing or are pending review and approval.

  
needs-ci
Needs to pass CI tests

  
needs-design-approval
Needs design discussion / approval

  
needs-info
An issue or PR which needs further info from the reporter / author

  
needs-preceding-pr
Requires another PR to be merged first

  
needs-review
Every commit must be reviewed by at least two team members

  
needs-reviewer
This PR needs someone to pick it up for review

  
needs-work

   
priority-high
High priority - will be reviewed soon

  
priority-low
Low priority - this may not receive review soon

  
priority-medium
Medium priority - this can be reviewed as time permits

  
priority-scheduled
This PR is big - it will require time to be scheduled for review

  
priority-very-high
Highest priority - prioritise this over other review work

  
resolved-after-1.1
The issue is resolved in development, but still present in 1.1 LTS.

  
single-reviewer
This PR qualifies for having only one reviewer

  
size-l
Estimated task size: large (2w+)

  
size-m
Estimated task size: medium (~1w)

  
size-optimisation

   
size-s
Estimated task size: small (~2d)

  
size-xs
Estimated task size: extra small (a few hours at most)

No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Mbed-TLS/TF-PSA-Crypto!506
No description provided.