-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat: fixes #547: add npm sigstore-tuf suport #731
feat: fixes #547: add npm sigstore-tuf suport #731
Conversation
c3c9feb
to
17e2f5c
Compare
@laurentsimon please take a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nits, but LGTM. Thank you!
go.mod
Outdated
) | ||
|
||
// use the pending PR #41 branch tuf-client-2 | ||
replace github.com/sigstore/sigstore-go => github.com/sigstore/sigstore-go v0.0.0-20231222133331-d489b534902f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to you: don't forget to replace before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's now using an official release of sigstore/[email protected], so I think we're ready to merge @laurentsimon @ianlewis
I've updated the PR to dynamically use the keyid specified in the sigstore bundle, rather than the hardcoded keyid. |
dfc1274
to
7d6ef52
Compare
Signed-off-by: Ramon Petgrave <[email protected]>
7d6ef52
to
230ea37
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@haydentherapper if you have comments let us know
func getKeyDataWithNpmjsKeysTarget(keys *npmjsKeysTarget, keyID, keyUsage string) (string, error) { | ||
for _, key := range keys.Keys { | ||
if key.KeyID == keyID && key.KeyUsage == keyUsage { | ||
return key.PublicKey.RawBytes, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that's where we should eventually get the TUF validity period and validate against rekor entry. Let's create a tracking issue to do that both here and in Fulcio cert verification (leaf cert period verification and rekor entry timestamp)
/cc @haydentherapper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, for example it's done for Rekor like so - https://github.com/sigstore/sigstore-go/blob/main/pkg/tlog/entry.go#L274-L277
you should be able to check the attestation against the npm key validity period too - https://github.com/sigstore/root-signing/blob/main/repository/repository/targets/registry.npmjs.org/7a8ec9678ad824cdccaa7a6dc0961caf8f8df61bc7274189122c123446248426.keys.json#L9-L11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I created the issue to track #757
@ramonpetgrave64 feel free to merge after addressing the few nits. Please create a tracking issue for the TUF verification discussed in #731 (comment). Thanks for the PR! |
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Addresses: #547
Uses the new [email protected]
Currently slsa-verifier has npmjs' attestation key hardcoded. But sigstore now stores the same key within their own TUF root.
This PR