Skip to content
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.

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

Use the correct verifier for RSA PSS scheme keys #625

Merged
merged 4 commits into from
Dec 17, 2024

Conversation

rdimitrov
Copy link
Contributor

@rdimitrov rdimitrov commented Mar 9, 2024

The following PR fixes an issue with go-tuf's support for RSA key types.

Details:

Apparently for RSA key types signature.LoadVerifier() is defaulting to returning a verifier that is RSAPKCS1v15Verifier which corresponds to the RSA PKCS#1 scheme.

Based on the latest TUF spec, TUF uses RSA PSS instead of PKCS#1. This manifests itself by causing go-tuf v2 TUF clients to fail metadata verification in case of RSA key types.

The fix for this is to invoke the proper verifier for the PSS scheme. This means that when we are verifying a metadata signed by an RSA key type, we should use LoadRSAPSSVerifier() instead of the generic LoadVerifier().

References:

Credits:

Thanks to @kairoaraujo for reaching out and helping track this together! 🚀 Cheers! 🍻

What's left:

  • Update the tests

@rdimitrov rdimitrov self-assigned this Mar 9, 2024
@rdimitrov rdimitrov added the bug label Mar 9, 2024
@rdimitrov rdimitrov marked this pull request as draft March 9, 2024 00:34
@kairoaraujo
Copy link

I've tested it, and it solves the issue 😃

@jku
Copy link
Member

jku commented Aug 20, 2024

Based on the latest TUF spec, TUF uses RSA PSS instead of PKCS#1

Note that these are just examples essentially, definitely not an exhaustive list of the only schemes that can be supported: I would avoid removing support for the pkcs1v15 scheme if possible as removing optional features can be a real head ache for users...

EDIT: the PR does not seem to remove the support, I just looked at comments originally and misunderstood

@jku
Copy link
Member

jku commented Aug 20, 2024

Just as an idea... I would love a keytype support test in tuf-conformance: theupdateframework/tuf-conformance#159 -- implementing that would give you keytype test coverage "for free" in go-tuf and should not be more than 20 lines of code.

EDIT: I did an initial test in tuf-conformance theupdateframework/tuf-conformance#167

jku added a commit to jku/tuf-conformance that referenced this pull request Aug 21, 2024
@kommendorkapten
Copy link
Member

FYI: I'm working on this, should have a working update within a few days.

@rdimitrov
Copy link
Contributor Author

@kommendorkapten - Awesome! 🙏 I'd consider this my Christmas gift 😄

@kommendorkapten
Copy link
Member

A lot of the tests are working, I'll hunt down the rest failures tomorrow I hope

@kommendorkapten kommendorkapten marked this pull request as ready for review December 17, 2024 12:26
@kommendorkapten kommendorkapten requested a review from a team as a code owner December 17, 2024 12:26
@kommendorkapten
Copy link
Member

This is now ready for review, the tests were failing as they were signed using PKCS#1 v1.5 but the key scheme said rsa-pss-sha256, I resigned the test data (with the helper tool added to this pr), and then made sure any signing and verification in the tests uses PSS. And when comparing to/from bytes I ignore whitespaces as its brittle to care about them.

Copy link
Contributor Author

@rdimitrov rdimitrov left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Contributor

@MDr164 MDr164 left a comment

Choose a reason for hiding this comment

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

Works locally as well for me. I'd say it's fine to go in. There are a some smaller nits in there but none of them affect the functionality so I'll refrain from blocking a merge :)

@rdimitrov
Copy link
Contributor Author

Thank you all for reviewing and contributing to this! 🙌 💯

@rdimitrov rdimitrov merged commit 830edf8 into theupdateframework:master Dec 17, 2024
23 checks passed
@rdimitrov rdimitrov deleted the fix-rsa-pss branch December 17, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants