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: remove pk_can_do() -- part 1 #615

Open
valeriosetti wants to merge 16 commits from valeriosetti/issue523 into development
pull from: valeriosetti/issue523
merge into: gh-Mbed-TLS:development
Conversation 9 Commits 16 Files changed 5 +261 -243
valeriosetti commented 2025-12-18 07:12:35 -06:00 (Migrated from github.com)

Description

TL;DR

Addresses the first part of #523: modify mbedtls_pk_[sign|verify]_ext() so that they don't use mbedtls_pk_can_do().

Details

Commits in this PR can be divided into 3 blocks:

  1. Extending tests for mbedtls_pk_[sign|verify]_ext() in order to cover all possible cases
  2. Re-shape mbedtls_pk_[sign|verify]_ext() so that they really perform as an _ext function over the simpler mbedtls_pk_[sign|verify]() counterparts: i.e. try to sign|verify using the specified sigalg and PK context.
    • This removes a lot of "special case handling" that was done previously
    • This simplifies testing: many comments added at point (1) to explain why [sign|verify]_ext() was behaving differently than expected are removed.
    • For example, after this reshaping opaque RSA keys can perform signature verification while previously this was not possible. And this didn't require any special treatment for the opaque context: just treating all the PK context in the same way allowed to improve the functionality.
  3. Adapt tests that are failing after this change
    • In most of the cases it's just a change to the returned value.
    • As mentioned above, in some other cases there was an improvement in functionality so a test that was failing for lack of support now is passing.
    • There are only changes to PK tests, meaning that "users" of [sign|verify]_ext() are not affected by this functions' reshaping! To me this is a sort of proof that these changes might be potentially transparent for the end user.

Advantage of the reshape

Once this will be merged we can make mbedtls_pk_[sign|verify]() calling into their _ext() counterpart (instead of the restartable ones) as it it normally happens for other "standard"/"extended" functions in the library.

PR checklist

  • changelog not required because: mbedtls_pk_can_do is internal, so nothing to be notified
  • framework PR not required
  • **mbedtls not required because: no change there
  • mbedtls 3.6 PR not required because: no backport
  • tests provided
## Description ### TL;DR Addresses the first part of #523: modify `mbedtls_pk_[sign|verify]_ext()` so that they don't use `mbedtls_pk_can_do()`. ### Details Commits in this PR can be divided into 3 blocks: 1. Extending tests for `mbedtls_pk_[sign|verify]_ext()` in order to cover all possible cases 2. Re-shape `mbedtls_pk_[sign|verify]_ext()` so that they _really_ perform as an `_ext` function over the simpler `mbedtls_pk_[sign|verify]()` counterparts: i.e. try to sign|verify using the specified sigalg and PK context. * This removes a lot of "special case handling" that was done previously * This simplifies testing: many comments added at point (1) to explain why `[sign|verify]_ext()` was behaving differently than expected are removed. * For example, after this reshaping opaque RSA keys can perform signature verification while previously this was not possible. And this didn't require any special treatment for the opaque context: just treating all the PK context in the same way allowed to improve the functionality. 3. Adapt tests that are failing after this change * In most of the cases it's just a change to the returned value. * As mentioned above, in some other cases there was an improvement in functionality so a test that was failing for lack of support now is passing. * There are only changes to PK tests, meaning that "users" of `[sign|verify]_ext()` are not affected by this functions' reshaping! To me this is a sort of proof that these changes might be potentially transparent for the end user. ### Advantage of the reshape Once this will be merged we can make `mbedtls_pk_[sign|verify]()` calling into their `_ext()` counterpart (instead of the restartable ones) as it it normally happens for other "standard"/"extended" functions in the library. ## PR checklist - [ ] **changelog** not required because: `mbedtls_pk_can_do` is internal, so nothing to be notified - [ ] **framework PR** not required - [ ] **mbedtls not required because: no change there - [ ] **mbedtls 3.6 PR** not required because: no backport - **tests** provided
bjwtaylor (Migrated from github.com) reviewed 2026-02-03 07:57:59 -06:00
drivers/builtin/src/pk.c
@ -1208,0 +1201,4 @@
#if defined(PSA_HAVE_ALG_SOME_ECDSA)
/* In case of ECDSA the signature must be converted from DER to RAW format.
* For RSA ones no change is needed. */
if (sigalg == MBEDTLS_PK_SIGALG_ECDSA) {
bjwtaylor (Migrated from github.com) commented 2026-02-03 07:31:10 -06:00

So if you build the branch with PSA_WANT_ALG_RSA_PKCS1V15_SIGN and MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED unset a number of the tests will fail. Which I think is related to this line. Is it expected that it will no longer work with this configuration.

So if you build the branch with PSA_WANT_ALG_RSA_PKCS1V15_SIGN and MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED unset a number of the tests will fail. Which I think is related to this line. Is it expected that it will no longer work with this configuration.
valeriosetti (Migrated from github.com) reviewed 2026-02-03 08:05:34 -06:00
drivers/builtin/src/pk.c
@ -1208,0 +1201,4 @@
#if defined(PSA_HAVE_ALG_SOME_ECDSA)
/* In case of ECDSA the signature must be converted from DER to RAW format.
* For RSA ones no change is needed. */
if (sigalg == MBEDTLS_PK_SIGALG_ECDSA) {
valeriosetti (Migrated from github.com) commented 2026-02-03 08:05:33 -06:00

Ideally if MBEDTLS_PSA_CRYPTO_C is defined then RSA-PSS is still used as enrollment algorithm, so verification should still work if the signature to be verified is really RSA-PSS and not PKCS v1.5.
But where did you get that configuration? The CI is green, but now I suspect that I missed something :/

Ideally if `MBEDTLS_PSA_CRYPTO_C` is defined then RSA-PSS is still used as enrollment algorithm, so verification should still work if the signature to be verified is really RSA-PSS and not PKCS v1.5. But where did you get that configuration? The CI is green, but now I suspect that I missed something :/
bjwtaylor (Migrated from github.com) reviewed 2026-02-03 08:11:43 -06:00
drivers/builtin/src/pk.c
@ -1208,0 +1201,4 @@
#if defined(PSA_HAVE_ALG_SOME_ECDSA)
/* In case of ECDSA the signature must be converted from DER to RAW format.
* For RSA ones no change is needed. */
if (sigalg == MBEDTLS_PK_SIGALG_ECDSA) {
bjwtaylor (Migrated from github.com) commented 2026-02-03 08:11:43 -06:00

So, this is potentially the question that config is completely made up to check if this change causes any issues. I guess it should still build with those configs disabled though? I also built development with the same configs disabled and you don't get the same failures. So I guess it's possible a regression, though I'm not sure if this is part of a wider plan?

So, this is potentially the question that config is completely made up to check if this change causes any issues. I guess it should still build with those configs disabled though? I also built development with the same configs disabled and you don't get the same failures. So I guess it's possible a regression, though I'm not sure if this is part of a wider plan?
valeriosetti (Migrated from github.com) reviewed 2026-02-05 02:29:26 -06:00
drivers/builtin/src/pk.c
@ -1208,0 +1201,4 @@
#if defined(PSA_HAVE_ALG_SOME_ECDSA)
/* In case of ECDSA the signature must be converted from DER to RAW format.
* For RSA ones no change is needed. */
if (sigalg == MBEDTLS_PK_SIGALG_ECDSA) {
valeriosetti (Migrated from github.com) commented 2026-02-05 02:29:26 -06:00

I'm currently investigating this and it's a bit weird. I just built mbedtls using

scripts/config.py full
scripts/config.py unset PSA_WANT_ALG_RSA_PKCS1V15_SIGN
scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED

as you suggested and there are indeed tests which are failing. For example
X509 CRT verification #1 (Revoked Cert, Expired CRL, no CN)
from test_suite_x509parse.data. The problem is that this test has a depends_on on MBEDTLS_PKCS1_V15 which should be disabled based on my configuration, but it's executed anyway (and it fails, of course).
I need to investigate on this a bit further

I'm currently investigating this and it's a bit weird. I just built `mbedtls` using ``` scripts/config.py full scripts/config.py unset PSA_WANT_ALG_RSA_PKCS1V15_SIGN scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED ``` as you suggested and there are indeed tests which are failing. For example `X509 CRT verification #1 (Revoked Cert, Expired CRL, no CN)` from `test_suite_x509parse.data`. The problem is that this test has a `depends_on` on `MBEDTLS_PKCS1_V15` which should be disabled based on my configuration, but it's executed anyway (and it fails, of course). I need to investigate on this a bit further
valeriosetti (Migrated from github.com) reviewed 2026-02-05 02:32:07 -06:00
drivers/builtin/src/pk.c
@ -1208,0 +1201,4 @@
#if defined(PSA_HAVE_ALG_SOME_ECDSA)
/* In case of ECDSA the signature must be converted from DER to RAW format.
* For RSA ones no change is needed. */
if (sigalg == MBEDTLS_PK_SIGALG_ECDSA) {
valeriosetti (Migrated from github.com) commented 2026-02-05 02:32:07 -06:00

Oh wait, that's because you must disable also PSA_WANT_ALG_RSA_PKCS1V15_CRYPT. Doing that the test passes. Apparently legacy crypto had a single configuration for PKCS v1.5 (sign and crypt) whereas PSA API differentiate between the two.

Oh wait, that's because you must disable _also_ `PSA_WANT_ALG_RSA_PKCS1V15_CRYPT`. Doing that the test passes. Apparently legacy crypto had a single configuration for PKCS v1.5 (sign and crypt) whereas PSA API differentiate between the two.
valeriosetti (Migrated from github.com) reviewed 2026-02-05 04:51:37 -06:00
drivers/builtin/src/pk.c
@ -1208,0 +1201,4 @@
#if defined(PSA_HAVE_ALG_SOME_ECDSA)
/* In case of ECDSA the signature must be converted from DER to RAW format.
* For RSA ones no change is needed. */
if (sigalg == MBEDTLS_PK_SIGALG_ECDSA) {
valeriosetti (Migrated from github.com) commented 2026-02-05 04:51:37 -06:00

@bjwtaylor I just tried this component on mbedtls's development (+ git submodule update --recursive):

component_tmp() {
    msg "Configure"

    scripts/config.py full
    scripts/config.py unset PSA_WANT_ALG_RSA_PKCS1V15_SIGN
    scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED

    rm -rf build
    mkdir build

    msg "Build"
    (
        cd build
        cmake -D CMAKE_BUILD_TYPE=Debug ..
        make
    )

    msg "Test"
    (
        cd build/tests
        ./test_suite_x509parse
    )
}

and it fails the same as with my PR checked out in tf-psa-crypto. So it seems my PR is not introducing regressions.
Can you please double check?

@bjwtaylor I just tried this component on `mbedtls`'s `development` (+ `git submodule update --recursive`): ``` component_tmp() { msg "Configure" scripts/config.py full scripts/config.py unset PSA_WANT_ALG_RSA_PKCS1V15_SIGN scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED rm -rf build mkdir build msg "Build" ( cd build cmake -D CMAKE_BUILD_TYPE=Debug .. make ) msg "Test" ( cd build/tests ./test_suite_x509parse ) } ``` and it fails the same as with my PR checked out in tf-psa-crypto. So it seems my PR is not introducing regressions. Can you please double check?
bjwtaylor (Migrated from github.com) reviewed 2026-02-05 05:02:30 -06:00
drivers/builtin/src/pk.c
@ -1208,0 +1201,4 @@
#if defined(PSA_HAVE_ALG_SOME_ECDSA)
/* In case of ECDSA the signature must be converted from DER to RAW format.
* For RSA ones no change is needed. */
if (sigalg == MBEDTLS_PK_SIGALG_ECDSA) {
bjwtaylor (Migrated from github.com) commented 2026-02-05 05:02:30 -06:00

As discussed, I think this requires further investigation as it worked prior to the addition of these commits. If we need to also disable PSA_WANT_ALG_RSA_PKCS1V15_CRYPT this should probably be reflected in the config dependencies. What do you think? EDIT: hadn't seen this above, let me confirm and come back to you.

As discussed, I think this requires further investigation as it worked prior to the addition of these commits. If we need to also disable PSA_WANT_ALG_RSA_PKCS1V15_CRYPT this should probably be reflected in the config dependencies. What do you think? EDIT: hadn't seen this above, let me confirm and come back to you.
bjwtaylor (Migrated from github.com) reviewed 2026-02-05 05:03:37 -06:00
bjwtaylor (Migrated from github.com) left a comment

as per https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/615#discussion_r2768434259

as per https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/615#discussion_r2768434259
bjwtaylor (Migrated from github.com) reviewed 2026-02-05 05:24:12 -06:00
drivers/builtin/src/pk.c
@ -1208,0 +1201,4 @@
#if defined(PSA_HAVE_ALG_SOME_ECDSA)
/* In case of ECDSA the signature must be converted from DER to RAW format.
* For RSA ones no change is needed. */
if (sigalg == MBEDTLS_PK_SIGALG_ECDSA) {
bjwtaylor (Migrated from github.com) commented 2026-02-05 05:24:12 -06:00

So, It's also failing for me on development now. So it could be something we've introduced recently, though doesn't look like a regression on this PR. So I'll leave it up to you and potentially the second reviewer, as to if this is worth fixing.

So, It's also failing for me on development now. So it could be something we've introduced recently, though doesn't look like a regression on this PR. So I'll leave it up to you and potentially the second reviewer, as to if this is worth fixing.
This pull request is broken due to missing fork information.
View command line instructions

Checkout

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

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