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

PK: inlined & simplify sign functions #537

Open
bjwtaylor wants to merge 53 commits from bjwtaylor/simplify-sign_func into development
pull from: bjwtaylor/simplify-sign_func
merge into: gh-Mbed-TLS:development
Conversation 34 Commits 53 Files changed 5 +149 -180
bjwtaylor commented 2025-10-20 05:45:20 -05:00 (Migrated from github.com)

Description

PK: inlined & simplify sign functions depends https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/499 depends https://github.com/Mbed-TLS/mbedtls/pull/10439 resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/515

We may need to add a task if it doesn't already exist to change MBEDTLS_ECDSA_DETERMINISTIC to PSA_ALG_DETERMINISTIC_ECDSA as itwas preventing some tests involved in this PR from running. I have changed these ones, however there maybe more that need investigation.
From the original issue the following test cases have been added where not present.
Non-wrapping context:

PR checklist

  • changelog not required because: No changes
  • framework PR not required
  • mbedtls development PR not required because: No changes
  • mbedtls 3.6 PR not required because: No changes
  • tests not required because: No changes
## Description PK: inlined & simplify sign functions depends https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/499 depends https://github.com/Mbed-TLS/mbedtls/pull/10439 resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/515 We may need to add a task if it doesn't already exist to change MBEDTLS_ECDSA_DETERMINISTIC to PSA_ALG_DETERMINISTIC_ECDSA as itwas preventing some tests involved in this PR from running. I have changed these ones, however there maybe more that need investigation. From the original issue the following test cases have been added where not present. Non-wrapping context: - Positive: RSA (v1.5), ECC (see "common" below) - Negative: trying to sign with a public key (seems missing?) - This was added here https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f5322b7cf59730e53ee00db5c196a937bf635c80/tests/suites/test_suite_pk.data#L515 - Negative: trying to sign with a key that doesn't allow signing (ECKEY_DH) -> according to lcov this is missing! - This was added here https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f5322b7cf59730e53ee00db5c196a937bf635c80/tests/suites/test_suite_pk.data#L519 Wrapping context (created with pk_wrap_psa()): - Positive: RSA (v1.5 and v2.1 (anysalt or not)), ECC (plain ECDSA or deterministic) (see "common" below) - Negative: the key's usage bits don't allow signing - This was added here https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f5322b7cf59730e53ee00db5c196a937bf635c80/tests/suites/test_suite_pk.data#L647 - Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH) - This was added here https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f5322b7cf59730e53ee00db5c196a937bf635c80/tests/suites/test_suite_pk.data#L599 Common: - Positive: check the result with pk_verify() or verify_ext(). When we know deterministic ECDSA will be used, check the - signature output against test vectors. - This is covered as part of https://github.com/bjwtaylor/TF-PSA-Crypto/blob/589b7c46d873bdee3bd4d282a14e9792dcbd3137/tests/suites/test_suite_pk.data#L533 and other similar cases - Negative: the output buffer is too small (seems missing?) - This was added here https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f5322b7cf59730e53ee00db5c196a937bf635c80/tests/suites/test_suite_pk.data#L555 ## PR checklist - [ ] **changelog** not required because: No changes - [ ] **framework PR** not required - [ ] **mbedtls development PR** not required because: No changes - [ ] **mbedtls 3.6 PR** not required because: No changes - **tests** not required because: No changes
mpg commented 2025-11-13 03:03:06 -06:00 (Migrated from github.com)

Test vectors are provided here:

github.com/Mbed-TLS/TF-PSA-Crypto@1a4690b5a6/tests/suites/test_suite_pk.data (L538)

I realise now that the way the issue was worded is ambiguous (things were perfectly clear in my mind, but for people who can't read my mind I should have made some assumptions more explicit). I've added a section about scope that should hopefully clarify things: https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/515

> Test vectors are provided here: > > https://github.com/Mbed-TLS/TF-PSA-Crypto/blob/1a4690b5a6eeeecffc7b0ba17a021dda5ec96c8a/tests/suites/test_suite_pk.data#L538 I realise now that the way the issue was worded is ambiguous (things were perfectly clear in my mind, but for people who can't read my mind I should have made some assumptions more explicit). I've added a section about scope that should hopefully clarify things: https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/515
mpg commented 2025-11-17 05:18:27 -06:00 (Migrated from github.com)

@bjwtaylor There are some commits there that I authored, can I assume you've reviewed them? (That way, I can be a reviewer for the rest of the PR.)

@bjwtaylor There are some commits there that I authored, can I assume you've reviewed them? (That way, I can be a reviewer for the rest of the PR.)
mpg commented 2025-11-17 05:23:01 -06:00 (Migrated from github.com)

@bjwtaylor Just had a quick look at the list of commits, and I'm afraid I'm not happy with the order: I'd like the new tests to come first, and only then the refactoring. That way we can be confident the refactoring is not accidentally changing observable behaviour. (Holds for all issues in this series: the order is always (1) add tests if needed, (2) refactor.)

@bjwtaylor Just had a quick look at the list of commits, and I'm afraid I'm not happy with the order: I'd like the new tests to come first, and only then the refactoring. That way we can be confident the refactoring is not accidentally changing observable behaviour. (Holds for all issues in this series: the order is always (1) add tests if needed, (2) refactor.)
bjwtaylor commented 2025-11-17 05:48:01 -06:00 (Migrated from github.com)

@mpg, thanks for your feedback. It's safe to assume I've reviewed your commits. In terms of the commit order, this makes perfect sense. Do you want me to put the test changes right at the beginning before your commits, or after them?

@mpg, thanks for your feedback. It's safe to assume I've reviewed your commits. In terms of the commit order, this makes perfect sense. Do you want me to put the test changes right at the beginning before your commits, or after them?
bjwtaylor commented 2025-11-18 09:06:27 -06:00 (Migrated from github.com)

@mpg, the commits should hopefully now be updated as you requested. The New CI has passed, so even though the OpenCI has stalled I think this is probably ready for you to take another look. Let me know if anything is not as you expect though.

@mpg, the commits should hopefully now be updated as you requested. The New CI has passed, so even though the OpenCI has stalled I think this is probably ready for you to take another look. Let me know if anything is not as you expect though.
👍 1
valeriosetti (Migrated from github.com) requested changes 2025-11-20 11:14:34 -06:00
valeriosetti (Migrated from github.com) left a comment

Just took an initial look at the PR. I'll probably take another one tomorrow

Just took an initial look at the PR. I'll probably take another one tomorrow
drivers/builtin/src/pk_wrap.c
@ -134,4 +123,1 @@
sig, sig_size, sig_len);
}
static int rsa_check_pair_wrap(mbedtls_pk_context *pub, mbedtls_pk_context *prv)
valeriosetti (Migrated from github.com) commented 2025-11-20 10:46:06 -06:00

This comment refers to mbedtls_pk_psa_rsa_sign_ext above. Why didn't you embed it into mbedtls_pk_sign_ext() in the same way as you've done with other signature functions?
Is there perhaps another issue for the sign_ext function? If so then sorry, but I didn't find it.

This comment refers to `mbedtls_pk_psa_rsa_sign_ext` above. Why didn't you embed it into `mbedtls_pk_sign_ext()` in the same way as you've done with other signature functions? Is there perhaps another issue for the `sign_ext` function? If so then sorry, but I didn't find it.
drivers/builtin/src/rsa.c
valeriosetti (Migrated from github.com) commented 2025-11-20 10:38:57 -06:00

I'm not sure I got the point in moving this declaration earlier. Unless it's strictly necessary, I suggest to keep changes at minimum.

I'm not sure I got the point in moving this declaration earlier. Unless it's strictly necessary, I suggest to keep changes at minimum.
tests/suites/test_suite_pk.data
@ -512,37 +512,49 @@ RSA hash_len overflow (size_t vs unsigned int)
depends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY:MBEDTLS_HAVE_INT64
valeriosetti (Migrated from github.com) commented 2025-11-20 11:01:58 -06:00

I think

ECDSA restartable sign/verify: ECKEY_DH

would be more aligned with surrounding test cases

I think ```suggestion ECDSA restartable sign/verify: ECKEY_DH ``` would be more aligned with surrounding test cases
valeriosetti (Migrated from github.com) commented 2025-11-20 11:09:23 -06:00

A buffer of length 0 is definitely small, so this is OK :)
But it would be nice to check also other intermediate values. Since ECDSA signatures have (r,s) values written in ASN1 format (adding some overhead) and here you are using secp256r1 curve, I think that something like PSA_ECDSA_SIGNATURE_SIZE(256)/2 should fail as well.

If you do this change then name this test case something like:
ECDSA restartable sign/verify: Output buffer too small (0 bytes)
and a similar name for the other one

A buffer of length 0 is definitely small, so this is OK :) But it would be nice to check also other intermediate values. Since ECDSA signatures have (r,s) values written in ASN1 format (adding some overhead) and here you are using secp256r1 curve, I think that something like `PSA_ECDSA_SIGNATURE_SIZE(256)/2` should fail as well. If you do this change then name this test case something like: `ECDSA restartable sign/verify: Output buffer too small (0 bytes)` and a similar name for the other one
tests/suites/test_suite_pk.data
valeriosetti (Migrated from github.com) commented 2025-11-20 11:11:15 -06:00

Perhaps

PSA wrapped sign: RSA wrong sign alg

? But this is minor, as you prefer

Perhaps ```suggestion PSA wrapped sign: RSA wrong sign alg ``` ? But this is minor, as you prefer
tests/suites/test_suite_pk.function
@ -1204,2 +1211,2 @@
ret = mbedtls_pk_sign_restartable(&prv, md_alg, hash->x, hash->len,
sig, sizeof(sig), &slen,
if (pub_key_test) {
ret = mbedtls_pk_sign_restartable(&pub, md_alg, hash->x, hash->len,
valeriosetti (Migrated from github.com) commented 2025-11-20 10:58:19 -06:00

This is enough:

                                          sig, sig_length, &slen,
This is enough: ```suggestion sig, sig_length, &slen, ```
valeriosetti (Migrated from github.com) commented 2025-11-20 10:58:33 -06:00

ditto

                                              sig, sig_length, &slen,
ditto ```suggestion sig, sig_length, &slen, ```
bjwtaylor (Migrated from github.com) reviewed 2025-11-21 02:32:29 -06:00
drivers/builtin/src/rsa.c
bjwtaylor (Migrated from github.com) commented 2025-11-21 02:32:29 -06:00

Not sure why it was moved, however doesn't seem to be required. So I've restored it to it's original position.

Not sure why it was moved, however doesn't seem to be required. So I've restored it to it's original position.
bjwtaylor (Migrated from github.com) reviewed 2025-11-21 02:42:16 -06:00
drivers/builtin/src/pk_wrap.c
@ -134,4 +123,1 @@
sig, sig_size, sig_len);
}
static int rsa_check_pair_wrap(mbedtls_pk_context *pub, mbedtls_pk_context *prv)
bjwtaylor (Migrated from github.com) commented 2025-11-21 02:42:16 -06:00

So, I believe this is out of scope of the issue https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/515 as this is purely about inlining the functions involved in the sign_func element of the struct mbedtls_pk_info_t. I'm not sure if this is or should be involved in another issue? Or if there is another plan for this function? Probably a question for @mpg. Though if he's happy to grow the scope we can probably include it in this PR.

So, I believe this is out of scope of the issue https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/515 as this is purely about inlining the functions involved in the sign_func element of the struct mbedtls_pk_info_t. I'm not sure if this is or should be involved in another issue? Or if there is another plan for this function? Probably a question for @mpg. Though if he's happy to grow the scope we can probably include it in this PR.
bjwtaylor (Migrated from github.com) reviewed 2025-11-21 03:05:32 -06:00
tests/suites/test_suite_pk.function
@ -1204,2 +1211,2 @@
ret = mbedtls_pk_sign_restartable(&prv, md_alg, hash->x, hash->len,
sig, sizeof(sig), &slen,
if (pub_key_test) {
ret = mbedtls_pk_sign_restartable(&pub, md_alg, hash->x, hash->len,
bjwtaylor (Migrated from github.com) commented 2025-11-21 03:05:32 -06:00

Should now be removed.

Should now be removed.
bjwtaylor (Migrated from github.com) reviewed 2025-11-21 03:05:43 -06:00
tests/suites/test_suite_pk.function
@ -1204,2 +1211,2 @@
ret = mbedtls_pk_sign_restartable(&prv, md_alg, hash->x, hash->len,
sig, sizeof(sig), &slen,
if (pub_key_test) {
ret = mbedtls_pk_sign_restartable(&pub, md_alg, hash->x, hash->len,
bjwtaylor (Migrated from github.com) commented 2025-11-21 03:05:43 -06:00

Should now be removed.

Should now be removed.
bjwtaylor (Migrated from github.com) reviewed 2025-11-21 03:12:47 -06:00
tests/suites/test_suite_pk.data
@ -512,37 +512,49 @@ RSA hash_len overflow (size_t vs unsigned int)
depends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY:MBEDTLS_HAVE_INT64
bjwtaylor (Migrated from github.com) commented 2025-11-21 03:12:46 -06:00

Now updated.

Now updated.
bjwtaylor (Migrated from github.com) reviewed 2025-11-21 03:23:11 -06:00
tests/suites/test_suite_pk.data
@ -512,37 +512,49 @@ RSA hash_len overflow (size_t vs unsigned int)
depends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY:MBEDTLS_HAVE_INT64
bjwtaylor (Migrated from github.com) commented 2025-11-21 03:23:11 -06:00

I've made the changes, let me know if they are not what you're after.

I've made the changes, let me know if they are not what you're after.
bjwtaylor (Migrated from github.com) reviewed 2025-11-21 03:25:51 -06:00
tests/suites/test_suite_pk.data
bjwtaylor (Migrated from github.com) commented 2025-11-21 03:25:51 -06:00

Had to make the other updates, so did this one also.

Had to make the other updates, so did this one also.
valeriosetti (Migrated from github.com) requested changes 2025-12-01 07:24:46 -06:00
drivers/builtin/src/pk.c
valeriosetti (Migrated from github.com) commented 2025-12-01 05:12:12 -06:00

Sorry, but I don't understand the reason for this inclusion. What is the change in this PR that makes this necessary?

Sorry, but I don't understand the reason for this inclusion. What is the change in this PR that makes this necessary?
drivers/builtin/src/pk.c
@ -1201,6 +1213,16 @@ int mbedtls_pk_sign_restartable(mbedtls_pk_context *ctx,
return MBEDTLS_ERR_PK_BAD_INPUT_DATA;
}
valeriosetti (Migrated from github.com) commented 2025-12-01 05:13:37 -06:00

Isn't MBEDTLS_ERR_PK_BAD_INPUT_DATA better in this case?

Isn't `MBEDTLS_ERR_PK_BAD_INPUT_DATA` better in this case?
drivers/builtin/src/pk.c
valeriosetti (Migrated from github.com) commented 2025-12-01 05:23:55 -06:00

I know that calling psa_reset_key_attributes() nowadays is no more mandatory, but since you are calling it if PSA_KEY_TYPE_IS_ECC(type) or PSA_KEY_TYPE_IS_RSA(type), I noticed it wouldn't be called if you don't fall in one of the above mentioned conditions.
IMO for sake of uniformity, it's better to call psa_reset_key_attributes() at the end of the function anyway.
My goal is either to call it in all possible code branches, or never call it.

I know that calling `psa_reset_key_attributes()` nowadays is no more mandatory, but since you are calling it if `PSA_KEY_TYPE_IS_ECC(type)` or `PSA_KEY_TYPE_IS_RSA(type)`, I noticed it wouldn't be called if you don't fall in one of the above mentioned conditions. IMO for sake of uniformity, it's better to call `psa_reset_key_attributes()` at the end of the function anyway. My goal is either to call it in all possible code branches, or never call it.
tests/suites/test_suite_pk.data
@ -512,37 +512,49 @@ RSA hash_len overflow (size_t vs unsigned int)
depends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY:MBEDTLS_HAVE_INT64
valeriosetti (Migrated from github.com) commented 2025-12-01 06:15:07 -06:00

IMO this test duplicates what's already tested from EC_DH (no) sign-verify: SECP256R1. Isn't it?

IMO this test duplicates what's already tested from `EC_DH (no) sign-verify: SECP256R1`. Isn't it?
tests/suites/test_suite_pk.data
valeriosetti (Migrated from github.com) commented 2025-12-01 06:45:49 -06:00

IMO this is a "duplication" of PSA wrapped sign: RSA PKCS1 v1.5 (invalid RSA_PKCS1V15_CRYPT algorithm) and PSA wrapped sign: RSA PKCS1 v1.5 (invalid RSA_OAEP algorithm) which are right below this one. In both cases wrong algorithms are used for signature.

IMO this is a "duplication" of `PSA wrapped sign: RSA PKCS1 v1.5 (invalid RSA_PKCS1V15_CRYPT algorithm)` and `PSA wrapped sign: RSA PKCS1 v1.5 (invalid RSA_OAEP algorithm)` which are right below this one. In both cases wrong algorithms are used for signature.
tests/suites/test_suite_pk.function
@ -1160,25 +1160,34 @@ exit:
}
valeriosetti (Migrated from github.com) commented 2025-12-01 06:02:16 -06:00

TEST_CALLOC() can be used for this purpose in tests.

`TEST_CALLOC()` can be used for this purpose in tests.
tests/suites/test_suite_pk.function
@ -1204,2 +1211,2 @@
ret = mbedtls_pk_sign_restartable(&prv, md_alg, hash->x, hash->len,
sig, sizeof(sig), &slen,
if (pub_key_test) {
ret = mbedtls_pk_sign_restartable(&pub, md_alg, hash->x, hash->len,
valeriosetti (Migrated from github.com) commented 2025-12-01 06:07:11 -06:00

I think this assignment is unnecessary as it's overridden below when calling mbedtls_pk_sign_restartable

I think this assignment is unnecessary as it's overridden below when calling `mbedtls_pk_sign_restartable`
valeriosetti (Migrated from github.com) commented 2025-12-01 06:10:30 -06:00

mbedtls_pk_xxx() functions do not return PSA error codes. It works because PSA_SUCCESS = 0, but technically this is no right. Also ret is of int type, not psa_status_t

`mbedtls_pk_xxx()` functions do not return PSA error codes. It works because `PSA_SUCCESS = 0`, but technically this is no right. Also `ret` is of `int` type, not `psa_status_t`
tests/suites/test_suite_pk.function
@ -1245,3 +1261,4 @@
sig, sig_length, &slen,
&rs_ctx);
TEST_ASSERT(ret == MBEDTLS_ERR_ECP_IN_PROGRESS);
}
valeriosetti (Migrated from github.com) commented 2025-12-01 06:07:54 -06:00 
                                          sig, sig_length, &slen,
```suggestion sig, sig_length, &slen, ```
tests/suites/test_suite_pk.function
@ -1612,0 +1635,4 @@
if (no_sign_usage_test) {
TEST_EQUAL(mbedtls_pk_sign_ext(key_pk_type, &pk, md_alg, hash, hash_len,
sig, sizeof(sig), &sig_len), MBEDTLS_ERR_RSA_BAD_INPUT_DATA);
goto exit;
valeriosetti (Migrated from github.com) commented 2025-12-01 07:00:55 -06:00

Technically here you are destroying the PSA key before freeing the context that wraps it. Here it doesn't create any problems because after this you jump to exit label, but it's not ideal.
I suggest moving call to psa_destroy_key() after mbedtls_pk_free() at the exit label.

Technically here you are destroying the PSA key _before_ freeing the context that wraps it. Here it doesn't create any problems because after this you jump to `exit` label, but it's not ideal. I suggest moving call to `psa_destroy_key()` after `mbedtls_pk_free()` at the `exit` label.
valeriosetti commented 2025-12-01 07:29:11 -06:00 (Migrated from github.com)

I expanded the test coverage table requested by @mpg in #515 and checked against what would be available in test_suite_pk.data the the top of this PR and I got the following:

Non-wrapping context:

  • ECC

    • Positive: check the result with pk_verify() or verify_ext()
    • Positive: check deterministic ECDSA against test vectors
    • Negative: trying to sign with a public key
    • Negative: trying to sign with a key that doesn't allow signing (ECKEY_DH)
    • Negative: the output buffer is too small

  • RSA 1.5

    • Positive: check the result with pk_verify() or verify_ext().
    • Negative: trying to sign with a public key
    • Negative: the output buffer is too small

Wrapping context

  • RSA 1.5

    • Positive: check the result with pk_verify() or verify_ext().
    • Negative: the output buffer is too small
    • Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH)
    • Negative: the key's usage bits don't allow signing

  • RSA 2.1

    • Positive: check the result with pk_verify() or verify_ext().
    • Negative: the output buffer is too small
    • Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH)
    • Negative: the key's usage bits don't allow signing

  • ECC (ECDSA normal/deterministic)

    • Positive: check the result with pk_verify() or verify_ext().
    • Negative: the output buffer is too small
    • Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH)
    • Negative: the key's usage bits don't allow signing

Of course, I might have missed something, so I ask the 2nd reviewer to fix/integrate as necessary.

I expanded the test coverage table requested by @mpg in #515 and checked against what would be available in `test_suite_pk.data` the the top of this PR and I got the following: Non-wrapping context: - ECC - [x] Positive: check the result with pk_verify() or verify_ext() - [ ] Positive: check deterministic ECDSA against test vectors - [x] Negative: trying to sign with a public key - [x] Negative: trying to sign with a key that doesn't allow signing (ECKEY_DH) - [x] Negative: the output buffer is too small - RSA 1.5 - [x] Positive: check the result with pk_verify() or verify_ext(). - [ ] Negative: trying to sign with a public key - [ ] Negative: the output buffer is too small Wrapping context - RSA 1.5 - [x] Positive: check the result with pk_verify() or verify_ext(). - [ ] Negative: the output buffer is too small - [x] Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH) - [x] Negative: the key's usage bits don't allow signing - RSA 2.1 - [x] Positive: check the result with pk_verify() or verify_ext(). - [ ] Negative: the output buffer is too small - [x] Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH) - [ ] Negative: the key's usage bits don't allow signing - ECC (ECDSA normal/deterministic) - [x] Positive: check the result with pk_verify() or verify_ext(). - [ ] Negative: the output buffer is too small - [x] Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH) - [ ] Negative: the key's usage bits don't allow signing Of course, I might have missed something, so I ask the 2nd reviewer to fix/integrate as necessary.
valeriosetti (Migrated from github.com) requested changes 2025-12-01 07:36:45 -06:00
valeriosetti (Migrated from github.com) left a comment

I forgot to add one general question on the implementation before submitting the previous review. Sorry

I forgot to add one general question on the implementation before submitting the previous review. Sorry
valeriosetti (Migrated from github.com) requested changes 2025-12-01 07:37:35 -06:00
drivers/builtin/src/pk.c
@ -1239,0 +1290,4 @@
status = psa_sign_hash(ctx->priv_id, sig_alg,
hash, hash_len, sig, sig_size, sig_len);
return PSA_PK_RSA_TO_MBEDTLS_ERR(status);
valeriosetti (Migrated from github.com) commented 2025-12-01 07:37:12 -06:00

This comment is not related to this specific line, but to the entire block that follows. Why do we need to implement all this non-restartable code block in a function that ends with _restartable()?
IMO it would make more sense to have it all under mbedtls_pk_sign() and then letting mbedtls_pk_sign_restartable() to call mbedtls_pk_sign() when the operation is not restartable (for any reason), rather than the other way around as it is now.
It's not blocking, but it would be cleaner to me.

This comment is not related to this specific line, but to the entire block that follows. Why do we need to implement all this non-restartable code block in a function that ends with `_restartable()`? IMO it would make more sense to have it all under `mbedtls_pk_sign()` and then letting `mbedtls_pk_sign_restartable()` to call `mbedtls_pk_sign()` when the operation is not restartable (for any reason), rather than the other way around as it is now. It's not blocking, but it would be cleaner to me.
bjwtaylor (Migrated from github.com) reviewed 2025-12-01 08:19:11 -06:00
drivers/builtin/src/pk.c
bjwtaylor (Migrated from github.com) commented 2025-12-01 08:19:11 -06:00

This is left over from debug and has now been removed.

This is left over from debug and has now been removed.
bjwtaylor (Migrated from github.com) reviewed 2025-12-01 08:24:30 -06:00
drivers/builtin/src/pk.c
@ -1201,6 +1213,16 @@ int mbedtls_pk_sign_restartable(mbedtls_pk_context *ctx,
return MBEDTLS_ERR_PK_BAD_INPUT_DATA;
}
bjwtaylor (Migrated from github.com) commented 2025-12-01 08:24:30 -06:00

Should now updated.

Should now updated.
bjwtaylor (Migrated from github.com) reviewed 2025-12-01 08:33:29 -06:00
tests/suites/test_suite_pk.function
@ -1160,25 +1160,34 @@ exit:
}
bjwtaylor (Migrated from github.com) commented 2025-12-01 08:33:29 -06:00

Should now be updated.

Should now be updated.
bjwtaylor (Migrated from github.com) reviewed 2025-12-01 08:37:19 -06:00
tests/suites/test_suite_pk.function
@ -1204,2 +1211,2 @@
ret = mbedtls_pk_sign_restartable(&prv, md_alg, hash->x, hash->len,
sig, sizeof(sig), &slen,
if (pub_key_test) {
ret = mbedtls_pk_sign_restartable(&pub, md_alg, hash->x, hash->len,
bjwtaylor (Migrated from github.com) commented 2025-12-01 08:37:19 -06:00

It should now be removed.

It should now be removed.
bjwtaylor (Migrated from github.com) reviewed 2025-12-01 08:41:02 -06:00
tests/suites/test_suite_pk.function
@ -1245,3 +1261,4 @@
sig, sig_length, &slen,
&rs_ctx);
TEST_ASSERT(ret == MBEDTLS_ERR_ECP_IN_PROGRESS);
}
bjwtaylor (Migrated from github.com) commented 2025-12-01 08:41:01 -06:00

Should now be removed.

Should now be removed.
bjwtaylor (Migrated from github.com) reviewed 2025-12-01 09:17:57 -06:00
tests/suites/test_suite_pk.function
@ -1204,2 +1211,2 @@
ret = mbedtls_pk_sign_restartable(&prv, md_alg, hash->x, hash->len,
sig, sizeof(sig), &slen,
if (pub_key_test) {
ret = mbedtls_pk_sign_restartable(&pub, md_alg, hash->x, hash->len,
bjwtaylor (Migrated from github.com) commented 2025-12-01 09:17:57 -06:00

Yes, I agree this is not technically correct. Although functionally it is currently and probably in the future the same. If I've understood correctly what you're after ret should be of type in, as this is I believe the standard in mbedtls? However the statement on line 1228 should be if (ret != 0), I've updated this. Hopefully this is what you are after, though let me know if you wanted something else.

Yes, I agree this is not technically correct. Although functionally it is currently and probably in the future the same. If I've understood correctly what you're after ret should be of type in, as this is I believe the standard in mbedtls? However the statement on line 1228 should be if (ret != 0), I've updated this. Hopefully this is what you are after, though let me know if you wanted something else.
valeriosetti (Migrated from github.com) reviewed 2025-12-02 02:47:11 -06:00
drivers/builtin/src/pk_wrap.c
@ -134,4 +123,1 @@
sig, sig_size, sig_len);
}
static int rsa_check_pair_wrap(mbedtls_pk_context *pub, mbedtls_pk_context *prv)
valeriosetti (Migrated from github.com) commented 2025-12-02 02:47:11 -06:00

I was looking at issue https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/523 and it turned out that refactoring of _ext functions should happen there indeed, so please ignore my comment.
OTOH this sets this PR and the companion one about verify functions as "prerequisite" for #523. It's not a strong prerequisite, but there is a high chance of huge conflicts.

I was looking at issue https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/523 and it turned out that refactoring of `_ext` functions should happen there indeed, so please ignore my comment. OTOH this sets this PR and the companion one about verify functions as "prerequisite" for #523. It's not a strong prerequisite, but there is a high chance of huge conflicts.
bjwtaylor (Migrated from github.com) reviewed 2025-12-02 05:58:48 -06:00
drivers/builtin/src/pk_wrap.c
@ -134,4 +123,1 @@
sig, sig_size, sig_len);
}
static int rsa_check_pair_wrap(mbedtls_pk_context *pub, mbedtls_pk_context *prv)
bjwtaylor (Migrated from github.com) commented 2025-12-02 05:58:48 -06:00

Ok, I'll ignore it then. In terms of the conflicts, assuming none of the changes are too controversial I am hoping to get the comments sorted soon and get it merged. For #521 I've barely started it yet, however it is next on the list. I've noticed you've taken on #523, so see how you get on and how bad the conflicts are. However if there are a lot maybe we delay it till after #521? I've had a quick look at it and a lot of the tests are similar, so hopefully it shouldn't take too long. It all depends on if there's anything else in the backlog and how important it is though?

Ok, I'll ignore it then. In terms of the conflicts, assuming none of the changes are too controversial I am hoping to get the comments sorted soon and get it merged. For #521 I've barely started it yet, however it is next on the list. I've noticed you've taken on #523, so see how you get on and how bad the conflicts are. However if there are a lot maybe we delay it till after #521? I've had a quick look at it and a lot of the tests are similar, so hopefully it shouldn't take too long. It all depends on if there's anything else in the backlog and how important it is though?
bjwtaylor (Migrated from github.com) reviewed 2025-12-02 07:09:06 -06:00
drivers/builtin/src/pk.c
bjwtaylor (Migrated from github.com) commented 2025-12-02 07:09:06 -06:00

So, I think the idea is that the function will never reach the end as it will have to be one of the cases. You're right though, for the sake of uniformity it's worth adding. So I've added it, let me know if this is what you are after?

So, I think the idea is that the function will never reach the end as it will have to be one of the cases. You're right though, for the sake of uniformity it's worth adding. So I've added it, let me know if this is what you are after?
bjwtaylor (Migrated from github.com) reviewed 2025-12-02 07:31:18 -06:00
tests/suites/test_suite_pk.data
@ -512,37 +512,49 @@ RSA hash_len overflow (size_t vs unsigned int)
depends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY:MBEDTLS_HAVE_INT64
bjwtaylor (Migrated from github.com) commented 2025-12-02 07:31:18 -06:00

Now removed.

Now removed.
bjwtaylor (Migrated from github.com) reviewed 2025-12-02 07:34:41 -06:00
tests/suites/test_suite_pk.data
bjwtaylor (Migrated from github.com) commented 2025-12-02 07:34:41 -06:00

Now removed.

Now removed.
bjwtaylor (Migrated from github.com) reviewed 2025-12-02 07:40:56 -06:00
tests/suites/test_suite_pk.function
@ -1612,0 +1635,4 @@
if (no_sign_usage_test) {
TEST_EQUAL(mbedtls_pk_sign_ext(key_pk_type, &pk, md_alg, hash, hash_len,
sig, sizeof(sig), &sig_len), MBEDTLS_ERR_RSA_BAD_INPUT_DATA);
goto exit;
bjwtaylor (Migrated from github.com) commented 2025-12-02 07:40:56 -06:00

Now moved.

Now moved.
bjwtaylor commented 2025-12-02 08:45:07 -06:00 (Migrated from github.com)

@valeriosetti, I can't reply to the comment on test cases. So I've added a response here:

Positive: check deterministic ECDSA against test vectors, I've discussed this with Gilles and we decided we only need to test 1 curve and the ECDSA, 256 Bits (Prime Field) is covered by existing tests:
here github.com/bjwtaylor/TF-PSA-Crypto@5df23a947a/tests/suites/test_suite_pk.data (L521)

Negative: trying to sign with a public key: this was covered by github.com/bjwtaylor/TF-PSA-Crypto@f832f63ac8/tests/suites/test_suite_pk.data (L515)

Negative: the output buffer is too small (ECC), covered by
github.com/bjwtaylor/TF-PSA-Crypto@f832f63ac8/tests/suites/test_suite_pk.data (L551) and

github.com/bjwtaylor/TF-PSA-Crypto@f832f63ac8/tests/suites/test_suite_pk.data (L555)

Negative: the key's usage bits don't allow signing (RSA 1.5)

github.com/bjwtaylor/TF-PSA-Crypto@f832f63ac8/tests/suites/test_suite_pk.data (L643)

I'll add test cases for if we think they are necessary:

Negative: the output buffer is too small (RSA1.5/RSA2.1)
Negative: the key's usage bits don't allow signing (RSA 2.1)
Negative: the key's usage bits don't allow signing (ECC)

@valeriosetti, I can't reply to the comment on test cases. So I've added a response here: Positive: check deterministic ECDSA against test vectors, I've discussed this with Gilles and we decided we only need to test 1 curve and the ECDSA, 256 Bits (Prime Field) is covered by existing tests: here https://github.com/bjwtaylor/TF-PSA-Crypto/blob/5df23a947a4ff43d54a8a20b45a2f56bc7aa7eff/tests/suites/test_suite_pk.data#L521 Negative: trying to sign with a public key: this was covered by https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f832f63ac873c918e94347394453aae2f7b8ecdc/tests/suites/test_suite_pk.data#L515 Negative: the output buffer is too small (ECC), covered by https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f832f63ac873c918e94347394453aae2f7b8ecdc/tests/suites/test_suite_pk.data#L551 and https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f832f63ac873c918e94347394453aae2f7b8ecdc/tests/suites/test_suite_pk.data#L555 Negative: the key's usage bits don't allow signing (RSA 1.5) https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f832f63ac873c918e94347394453aae2f7b8ecdc/tests/suites/test_suite_pk.data#L643 I'll add test cases for if we think they are necessary: Negative: the output buffer is too small (RSA1.5/RSA2.1) Negative: the key's usage bits don't allow signing (RSA 2.1) Negative: the key's usage bits don't allow signing (ECC)
bjwtaylor (Migrated from github.com) reviewed 2025-12-02 08:49:21 -06:00
drivers/builtin/src/pk.c
@ -1239,0 +1290,4 @@
status = psa_sign_hash(ctx->priv_id, sig_alg,
hash, hash_len, sig, sig_size, sig_len);
return PSA_PK_RSA_TO_MBEDTLS_ERR(status);
bjwtaylor (Migrated from github.com) commented 2025-12-02 08:49:20 -06:00

So this is probably a question for @mpg, as he implemented this code. I suspect there was a reason he did it this way?

So this is probably a question for @mpg, as he implemented this code. I suspect there was a reason he did it this way?
mpg (Migrated from github.com) reviewed 2025-12-03 03:17:21 -06:00
drivers/builtin/src/pk.c
@ -1239,0 +1290,4 @@
status = psa_sign_hash(ctx->priv_id, sig_alg,
hash, hash_len, sig, sig_size, sig_len);
return PSA_PK_RSA_TO_MBEDTLS_ERR(status);
mpg (Migrated from github.com) commented 2025-12-03 03:17:21 -06:00

Yes, I considered this. The point of keeping this inside the restartable function was to avoid duplicating the first two paragraphs (parameters check and call to pk_hashlen_helper()). However that's probably a weak justification for having a function that does two entirely different things.

I can think of two approaches:

  1. Accept duplicating those 6 lines between sign() and sign_restartable(), considering that's a small price to pay for readability. Then sign_restartable() calls sign() in the non-restartable case.
  2. Introduce a new function sign_internal(). Then sign() keeps calling sign_restartable() which does the checks/adjustments, then either goes the restartable path or calls sign_internal(). (And in the future, the restartable path would be to call sign_restartable_internal() when we inline that part.)

Wdyt? I agree we should take this opportunity to make the code clean and readable, and that the current status (single function implementing both restartable and non-restartable one after the other) is not what we want.

Yes, I considered this. The point of keeping this inside the `restartable` function was to avoid duplicating the first two paragraphs (parameters check and call to `pk_hashlen_helper()`). However that's probably a weak justification for having a function that does two entirely different things. I can think of two approaches: 1. Accept duplicating those 6 lines between `sign()` and `sign_restartable()`, considering that's a small price to pay for readability. Then `sign_restartable()` calls `sign()` in the non-restartable case. 2. Introduce a new function `sign_internal()`. Then `sign()` keeps calling `sign_restartable()` which does the checks/adjustments, then either goes the restartable path or calls `sign_internal()`. (And in the future, the restartable path would be to call `sign_restartable_internal()` when we inline that part.) Wdyt? I agree we should take this opportunity to make the code clean and readable, and that the current status (single function implementing both restartable and non-restartable one after the other) is not what we want.
bjwtaylor (Migrated from github.com) reviewed 2025-12-05 07:29:39 -06:00
drivers/builtin/src/pk_wrap.c
@ -134,4 +123,1 @@
sig, sig_size, sig_len);
}
static int rsa_check_pair_wrap(mbedtls_pk_context *pub, mbedtls_pk_context *prv)
bjwtaylor (Migrated from github.com) commented 2025-12-05 07:29:38 -06:00

So, as discussed in standup I think the current plan now is to drop the verify and split this into two. Test and implement. Let me know if there is any impact on what you are doing and maybe some coordination will be required.

So, as discussed in standup I think the current plan now is to drop the verify and split this into two. Test and implement. Let me know if there is any impact on what you are doing and maybe some coordination will be required.
bjwtaylor (Migrated from github.com) reviewed 2025-12-05 07:50:59 -06:00
drivers/builtin/src/pk.c
@ -1239,0 +1290,4 @@
status = psa_sign_hash(ctx->priv_id, sig_alg,
hash, hash_len, sig, sig_size, sig_len);
return PSA_PK_RSA_TO_MBEDTLS_ERR(status);
bjwtaylor (Migrated from github.com) commented 2025-12-05 07:50:59 -06:00

So, to potentially add an option 3. We could rename the function and we could then keep restartable/non-restartable as is. I think if we start splitting it out we are going back towards the original position with the function pointers, which is not ideal either. If @mpg and @valeriosetti you have a strong preference for either, I'm happy to go with it though.

So, to potentially add an option 3. We could rename the function and we could then keep restartable/non-restartable as is. I think if we start splitting it out we are going back towards the original position with the function pointers, which is not ideal either. If @mpg and @valeriosetti you have a strong preference for either, I'm happy to go with it though.
This pull request has changes conflicting with the target branch.
  • extras/pk_wrap.c
  • tests/suites/test_suite_pk.function
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'bjwtaylor/simplify-sign_func:bjwtaylor/simplify-sign_func'
git switch 'bjwtaylor/simplify-sign_func'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'development'
git merge --no-ff 'bjwtaylor/simplify-sign_func'
git switch 'bjwtaylor/simplify-sign_func'
git rebase 'development'
git switch 'development'
git merge --ff-only 'bjwtaylor/simplify-sign_func'
git switch 'bjwtaylor/simplify-sign_func'
git rebase 'development'
git switch 'development'
git merge --no-ff 'bjwtaylor/simplify-sign_func'
git switch 'development'
git merge --squash 'bjwtaylor/simplify-sign_func'
git switch 'development'
git merge --ff-only 'bjwtaylor/simplify-sign_func'
git switch 'development'
git merge 'bjwtaylor/simplify-sign_func'
git push origin 'development'
Sign in to join this conversation.
Reviewers
No reviewers
valeriosetti
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!537
No description provided.