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

Fix proof of key possession generation #283

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

adityasaky
Copy link
Member

Summary

This commit updates the proof of key possession signature to prioritize email over subject when the claim is present in the token. This matches the current behaviour of Fulcio, which verifies the proof signature using the token's email claim.

Closes #282

Release Note

Updated proof of key possession signature to use email when it's present in the token.

Documentation

NONE

@adityasaky adityasaky requested a review from a team as a code owner August 28, 2024 19:24
@adityasaky
Copy link
Member Author

I've copied subjectFromClaims from sigstore/sigstore for the time being. We can't currently use the exported SubjectFromToken function because it accepts an oidc.IDToken object which is only created after the token is verified by the client. This library doesn't currently verify the token, leaving that to Fulcio. FWIW, SubjectFromToken operates on the token's bytes (tok.Claims() is a wrapper around json.Unmarshal of the token bytes). However, changing it instead to just use the token's bytes would be a breaking change, and I also haven't checked whether the token bytes are still available when the function is invoked. I suppose another question is whether cosign, etc. should continue to verify the token at all or just leave it to Fulcio as this library does.

@adityasaky adityasaky marked this pull request as draft August 28, 2024 19:32
Copy link
Member

@steiza steiza left a comment

Choose a reason for hiding this comment

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

I think this approach makes sense. Could we add some unit tests?

pkg/sign/certificate.go Outdated Show resolved Hide resolved
@adityasaky
Copy link
Member Author

Could we add some unit tests?

Happy to! I held off until there was consensus on whether subjectFromClaims should in fact be in this repo or sigstore/sigstore. Also, I think we can't test the proof of key possession signature itself, since fulcio is mocked?

@adityasaky
Copy link
Member Author

adityasaky commented Sep 3, 2024

(apologies for all the force pushes)

I think this is now ready for review, I've updated sigstore/sigstore to v1.8.9 that @haydentherapper just cut, which includes SubjectFromUnverifiedToken discussed in #283 (comment).

haydentherapper
haydentherapper previously approved these changes Sep 3, 2024
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

thanks!

This commit updates the proof of key possession signature to prioritize
email over subject when the claim is present in the token. This matches
the current behaviour of Fulcio, which verifies the proof signature
using the token's email claim.

Signed-off-by: Aditya Sirish <[email protected]>
@adityasaky
Copy link
Member Author

Didn't realize there was another go.mod, I've updated it as well now.

Copy link
Member

@kommendorkapten kommendorkapten left a comment

Choose a reason for hiding this comment

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

👍

@steiza steiza merged commit 725e508 into sigstore:main Sep 4, 2024
10 checks passed
@adityasaky adityasaky deleted the pop-subject-email branch September 4, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proof of key possession is inconsistent with Fulcio
4 participants