search

sigstore / sigstore-go

Go library for Sigstore signing and verification
Apache License 2.0
45 stars 21 forks source link

Fix proof of key possession generation #283

Closed adityasaky closed 1 month ago

adityasaky commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

(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 https://github.com/sigstore/sigstore-go/pull/283#discussion_r1735246780.

adityasaky commented 1 month ago

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