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

Fixing PK to make it compliant with the documentation for 1.0 release #471

Merged
valeriosetti merged 4 commits from fix-pk into development 2025-09-29 08:48:53 -05:00
Conversation 11 Commits 4 Files changed 3 +24 -60
valeriosetti commented 2025-09-23 09:14:42 -05:00 (Migrated from github.com)

Description

  • always set PKCS v1.5 padding in mbedtls_pk_copy[_public]_from_psa
  • mbedtls_pk_get_psa_attributes() always return PKCS v1.5 algs for RSA keys

Depends on: https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/468#discussion_r2378067577

PR checklist

  • changelog provided
  • framework PR not required
  • mbedtls development PR not required because: no change in that repo
  • mbedtls 3.6 PR not required because: no backport
  • tests provided
## Description - always set PKCS v1.5 padding in `mbedtls_pk_copy[_public]_from_psa` - `mbedtls_pk_get_psa_attributes()` always return PKCS v1.5 algs for RSA keys **Depends on:** https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/468#discussion_r2378067577 ## PR checklist - [ ] **changelog** provided - [ ] **framework PR** not required - [ ] **mbedtls development PR** not required because: no change in that repo - [ ] **mbedtls 3.6 PR** not required because: no backport - **tests** provided
gilles-peskine-arm commented 2025-09-24 03:27:00 -05:00 (Migrated from github.com)

I can't review this effectively since it's changing the behavior to not match the documentation in this branch.

Please cherry-pick Manuel's relevant commits so that the documentation and the code are consistent in this pull request.

I can't review this effectively since it's changing the behavior to _not_ match the documentation in this branch. Please cherry-pick Manuel's relevant commits so that the documentation and the code are consistent in this pull request.
🚀 1
mpg commented 2025-09-25 02:46:00 -05:00 (Migrated from github.com)

See this comment - I believe the tests that rely on mbedtls_pk_set_rsa_padding() and/or mbedtls_pk_set_rsa_hash() should ultimately be removed, because we want to remove those functions and the underlying fields. But I leave it up to you whether you're rather do this at once in this PR (for the sake of clarity) or in a follow-up PR (for the sake of speed here).

See [this comment](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/468#discussion_r2378067577) - I believe the tests that rely on `mbedtls_pk_set_rsa_padding()` and/or `mbedtls_pk_set_rsa_hash()` should ultimately be removed, because we want to remove those functions and the underlying fields. But I leave it up to you whether you're rather do this at once in this PR (for the sake of clarity) or in a follow-up PR (for the sake of speed here).
mpg (Migrated from github.com) reviewed 2025-09-25 05:53:45 -05:00
mpg (Migrated from github.com) left a comment
No description provided.
LGTM, thanks!
mpg commented 2025-09-25 05:54:50 -05:00 (Migrated from github.com)

We may want to make this one depend on#468 rather than cherry-picking two commits from it?

We may want to make this one depend on#468 rather than cherry-picking two commits from it?
👍 1
valeriosetti commented 2025-09-25 06:42:23 -05:00 (Migrated from github.com)

We may want to make this one depend on#468 rather than cherry-picking two commits from it?

I didn't do that initially because I saw that #468 had a CI with many failures and I wanted to test my changes on their own. Rebased now anyway 😉

> We may want to make this one depend on#468 rather than cherry-picking two commits from it? I didn't do that initially because I saw that #468 had a CI with many failures and I wanted to test my changes on their own. Rebased now anyway :wink:
gilles-peskine-arm (Migrated from github.com) requested changes 2025-09-25 08:07:49 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

LGTM at 67ab2d3486dcd8cb7a2e76d68c1eae65472f0826 except for the changelog

LGTM at 67ab2d3486dcd8cb7a2e76d68c1eae65472f0826 except for the changelog
ChangeLog.d/fix-pk.txt
gilles-peskine-arm (Migrated from github.com) commented 2025-09-25 07:53:38 -05:00

Manuel's PR also has the changelog entries now and I reviewed them there. Please remove them here.

[Manuel's PR](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/468) also has the changelog entries now and I reviewed them there. Please remove them here.
include/mbedtls/pk.h
gilles-peskine-arm (Migrated from github.com) commented 2025-09-25 07:58:01 -05:00

Ideally we'd get rid of the rsa_padding field and of mbedtls_pk_rsa_padding_t. But it looks like this would require significant further work in the tests, so I'm happy to defer that to a future cleanup.

Ideally we'd get rid of the `rsa_padding` field and of `mbedtls_pk_rsa_padding_t`. But it looks like this would require significant further work in the tests, so I'm happy to defer that to a future cleanup.
tests/suites/test_suite_pk.data
@ -712,3 +712,3 @@
depends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY:PSA_WANT_ALG_RSA_PSS:PSA_WANT_ALG_SHA_256
pk_rsa_v21_get_psa_attributes:MBEDTLS_MD_SHA256:FROM_PAIR:PSA_KEY_USAGE_DECRYPT:1:PSA_ALG_RSA_OAEP(PSA_ALG_SHA_256)
pk_rsa_v21_get_psa_attributes:MBEDTLS_MD_SHA256:FROM_PAIR:PSA_KEY_USAGE_DECRYPT:1:PSA_ALG_RSA_PKCS1V15_CRYPT
gilles-peskine-arm (Migrated from github.com) commented 2025-09-25 08:06:56 -05:00

I'm not sure if there's any point in pk_rsa_v21_get_psa_attributes, since there isn't a concept of “v21 PK context” any more. But that can be part of a later cleanup. I don't want to get bogged down into a major refactoring of the test code.

I'm not sure if there's any point in `pk_rsa_v21_get_psa_attributes`, since there isn't a concept of “v21 PK context” any more. But that can be part of a later cleanup. I don't want to get bogged down into a major refactoring of the test code.
tests/suites/test_suite_pk.function
@ -1623,16 +1626,15 @@ void pk_psa_wrap_sign_ext(int pk_type, int key_bits, int key_pk_type, int md_alg
/* Create legacy RSA public/private key in PK context. */
gilles-peskine-arm (Migrated from github.com) commented 2025-09-25 08:05:35 -05:00

Why keep this commented-out line?

Why keep this commented-out line?
valeriosetti (Migrated from github.com) reviewed 2025-09-25 08:11:29 -05:00
include/mbedtls/pk.h
valeriosetti (Migrated from github.com) commented 2025-09-25 08:11:29 -05:00

For these follow-up tasks I prepared https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/477. Can I add you as reviewer to that one?

For these follow-up tasks I prepared https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/477. Can I add you as reviewer to that one?
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-25 08:13:55 -05:00
include/mbedtls/pk.h
gilles-peskine-arm (Migrated from github.com) commented 2025-09-25 08:13:55 -05:00

Ah, thanks, good to know it's already there! I'm afraid I'll pass on reviewing for now, I have other urgent things for the release and for the beginning of Q4.

Ah, thanks, good to know it's already there! I'm afraid I'll pass on reviewing for now, I have other urgent things for the release and for the beginning of Q4.
mpg commented 2025-09-29 02:15:15 -05:00 (Migrated from github.com)

@valeriosetti can you rebase now that #468 has been merged? Then I'll review promptly!

@valeriosetti can you rebase now that #468 has been merged? Then I'll review promptly!
🚀 1
mpg (Migrated from github.com) approved these changes 2025-09-29 03:56:05 -05:00
mpg (Migrated from github.com) left a comment

LGTM, thanks!

LGTM, thanks!
gilles-peskine-arm (Migrated from github.com) approved these changes 2025-09-29 06:47:39 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

LGTM

LGTM
Sign in to join this conversation.
Reviewers
No reviewers
mpg
gilles-peskine-arm
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!471
No description provided.