Skip to content

Fix proof of key possession generation#283

Merged
steiza merged 1 commit intosigstore:mainfrom
adityasaky:pop-subject-email
Sep 4, 2024
Merged

Fix proof of key possession generation#283
steiza merged 1 commit intosigstore:mainfrom
adityasaky:pop-subject-email

Conversation

@adityasaky
Copy link
Copy Markdown
Member

@adityasaky adityasaky commented Aug 28, 2024

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
Copy Markdown
Member Author

adityasaky commented Aug 28, 2024

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
Hayden-IO
Hayden-IO reviewed Aug 28, 2024
View reviewed changes
Comment thread pkg/sign/certificate.go Outdated
steiza
steiza reviewed Aug 28, 2024
View reviewed changes
Copy link
Copy Markdown
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?

Comment thread pkg/sign/certificate.go Outdated
@adityasaky
Copy link
Copy Markdown
Member Author

adityasaky commented Aug 28, 2024

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?

This was referenced Aug 29, 2024
Merged
Merged
@adityasaky adityasaky force-pushed the pop-subject-email branch 2 times, most recently from c2495cc to 06647de Compare September 3, 2024 19:37
@adityasaky adityasaky marked this pull request as ready for review September 3, 2024 19:38
@adityasaky
Copy link
Copy Markdown
Member Author

adityasaky commented Sep 3, 2024 •
edited
Loading

(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).

Hayden-IO
Hayden-IO previously approved these changes Sep 3, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@Hayden-IO Hayden-IO left a comment

Choose a reason for hiding this comment

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

thanks!

@adityasaky
Fix proof of key possession generation
46ff857 
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 <aditya@saky.in>
@adityasaky
Copy link
Copy Markdown
Member Author

adityasaky commented Sep 3, 2024

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

kommendorkapten
kommendorkapten approved these changes Sep 4, 2024
View reviewed changes
Copy link
Copy Markdown
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
@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

Reviewers

@steiza steiza steiza approved these changes

@kommendorkapten kommendorkapten kommendorkapten approved these changes

@Hayden-IO Hayden-IO Hayden-IO left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Proof of key possession is inconsistent with Fulcio

4 participants

@adityasaky @steiza @Hayden-IO @kommendorkapten