Skip to content

[development] Restrict MBEDTLS_X509_RSASSA_PSS_SUPPORT #10130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
May 7, 2025
Merged

[development] Restrict MBEDTLS_X509_RSASSA_PSS_SUPPORT #10130

merged 4 commits into from
May 7, 2025

Conversation

valeriosetti
Copy link
Contributor

Description

Resolves #8154

PR checklist

@valeriosetti valeriosetti self-assigned this Apr 10, 2025
@valeriosetti valeriosetti force-pushed the issue8154-development branch 2 times, most recently from c9b0056 to cbb1267 Compare April 11, 2025 15:14
@valeriosetti valeriosetti mentioned this pull request Apr 23, 2025
Merged
5 tasks
@valeriosetti valeriosetti force-pushed the issue8154-development branch 5 times, most recently from c546b7f to 9b50527 Compare April 28, 2025 15:07
@valeriosetti valeriosetti requested a review from mpg April 29, 2025 08:18
@valeriosetti valeriosetti added 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 size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Apr 29, 2025
@valeriosetti valeriosetti force-pushed the issue8154-development branch from 9b50527 to 147e373 Compare April 29, 2025 14:13
@bjwtaylor bjwtaylor assigned bjwtaylor and unassigned bjwtaylor Apr 29, 2025
@bjwtaylor bjwtaylor self-requested a review April 29, 2025 14:59
@valeriosetti valeriosetti removed the needs-reviewer This PR needs someone to pick it up for review label Apr 30, 2025
mpg
mpg previously approved these changes Apr 30, 2025
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

In addition to reviewing commit by commit, I've also double-checked that in the end, no occurrence of mbedtls_pk_rsassa_pss_options remains in X.509/TLS. So after this PR is merged, we'll be ready to remove that type on the crypto side.

@mpg
Copy link
Contributor

mpg commented Apr 30, 2025

Unfortunately there's now a conflict in the submodule pointer, so I'm afraid you'll need to rebase once more :(

@valeriosetti valeriosetti mentioned this pull request Apr 30, 2025
Closed
@valeriosetti
Copy link
Contributor Author

In addition to reviewing commit by commit, I've also double-checked that in the end, no occurrence of mbedtls_pk_rsassa_pss_options remains in X.509/TLS. So after this PR is merged, we'll be ready to remove that type on the crypto side.

Mbed-TLS/TF-PSA-Crypto#274

@valeriosetti valeriosetti force-pushed the issue8154-development branch from 147e373 to d9c52d7 Compare April 30, 2025 10:12
@mpg mpg added the needs-preceding-pr Requires another PR to be merged first label Apr 30, 2025
mpg
mpg previously approved these changes Apr 30, 2025
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will need one more rebase when the crypto side has been merged, to point to the merge commit.

bjwtaylor
bjwtaylor previously approved these changes Apr 30, 2025
View reviewed changes
@github-project-automation github-project-automation bot moved this from In Development to Has Approval in Roadmap pull requests (new board) Apr 30, 2025
@valeriosetti valeriosetti dismissed stale reviews from bjwtaylor and mpg via d0967ac May 5, 2025 10:14
@valeriosetti valeriosetti force-pushed the issue8154-development branch from d9c52d7 to d0967ac Compare May 5, 2025 10:14
@valeriosetti valeriosetti removed the needs-preceding-pr Requires another PR to be merged first label May 5, 2025
@valeriosetti
Copy link
Contributor Author

valeriosetti commented May 5, 2025 •
edited
Loading

@mpg @bjwtaylor following the merge of Mbed-TLS/TF-PSA-Crypto#253 I've update the last commit to point to the HEAD of development of tf-psa-crypto repo. Assuming the CI is still green since I haven't change anything in the code, this PR is ready for reviews again.

@valeriosetti valeriosetti requested review from mpg and bjwtaylor May 5, 2025 10:16
@valeriosetti valeriosetti mentioned this pull request May 5, 2025
Merged
5 tasks
mpg
mpg previously approved these changes May 5, 2025
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, no change since my previous approval except updating the crypto pointer to the merge commit of c253.

bjwtaylor
bjwtaylor previously approved these changes May 6, 2025
View reviewed changes
@valeriosetti
library: x509: simplify RSA-PSS management
68878cc 
- Do not store RSA-PSS signature options in CRL/CRT/CSR structures;
- During the parsing phase, just ensure that MGF1 hash alg is the same
  as the one used for the message.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
library: x509: remove sig_opts from mbedtls_x509_sig_alg_gets()
d24dfad 
Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
library: pass NULL options parameter to mbedtls_pk_verify_ext()
7f6f4e6 
Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
tests: test_suite_x509: adapt RSA-PSS tests
b8d5649 
Parsing of CRT files with message's hash alg different from the MGF1 was
allowed in the past, but now it fails. So we need to move/adapt tests
relying on this feature, from a "verify" scope to a "parse" one.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti valeriosetti dismissed stale reviews from bjwtaylor and mpg via b8d5649 May 7, 2025 07:07
@valeriosetti valeriosetti force-pushed the issue8154-development branch from d0967ac to b8d5649 Compare May 7, 2025 07:07
@valeriosetti
Copy link
Contributor Author

valeriosetti commented May 7, 2025 •
edited
Loading

@mpg @bjwtaylor I had to rebase the PR due to a conflict on tf-psa-crypto. Since Mbed-TLS/TF-PSA-Crypto#253 was already merged and since development branch already included it, I removed the last commit from this PR because it became useless. Nothing else was changed in the PR during the rebase so hopefully the CI will be OK again.

@valeriosetti valeriosetti requested review from bjwtaylor and mpg May 7, 2025 07:09
mpg
mpg approved these changes May 7, 2025
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@valeriosetti 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 May 7, 2025
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue May 7, 2025
Merged via the queue into Mbed-TLS:development with commit 1782587 May 7, 2025
4 of 6 checks passed
@github-project-automation github-project-automation bot moved this from Has Approval to Done in Roadmap pull requests (new board) May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpg mpg mpg approved these changes

@bjwtaylor bjwtaylor bjwtaylor approved these changes

Assignees

@valeriosetti valeriosetti

Labels
approved Design and code approved - may be waiting for CI or backports priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Roadmap pull requests (new board)
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Restrict MBEDTLS_X509_RSASSA_PSS_SUPPORT
4 participants
@valeriosetti @mpg @bjwtaylor @gilles-peskine-arm