-
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: support npm cli provenance v1 attestations #776
feat: support npm cli provenance v1 attestations #776
Conversation
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]>
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]>
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]>
My implementation turns out to be very similar to another earlier draft in #706 |
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 think this looks good. It'd be nice to see an actual npm provenance included in here for documentation (instead of having to go parse the dsse envelope)
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
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.
Thanks so much for looking at this, v1 support will be great!
verifiers/internal/gha/slsaprovenance/v1.0/npmcli_github_actions.go
Outdated
Show resolved
Hide resolved
@@ -9,13 +9,21 @@ import ( | |||
"github.com/slsa-framework/slsa-verifier/v2/verifiers/internal/gha/slsaprovenance/common" | |||
"github.com/slsa-framework/slsa-verifier/v2/verifiers/internal/gha/slsaprovenance/iface" | |||
slsav02 "github.com/slsa-framework/slsa-verifier/v2/verifiers/internal/gha/slsaprovenance/v0.2" | |||
slsav1 "github.com/slsa-framework/slsa-verifier/v2/verifiers/internal/gha/slsaprovenance/v1.0" | |||
) | |||
|
|||
func verifyProvenanceMatchesCertificate(prov iface.Provenance, workflow *WorkflowIdentity) error { |
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.
This isn't new with this change, but it seems this function assumes that the provenance is going to be generated by GitHub but its possible that it comes from elsewhere (e.g. GitLab https://registry.npmjs.org/-/npm/v1/attestations/@ps-testing%[email protected]). Is my interpretation correct? Are there any plans to support verifying provenance that doesn't come from GitHub?
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.
As they're still on provenance v0.2, I think I agree with laurent that we should wait until they start generating slsa v1 provenance
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.
OK that makes sense. My only other concern would be making sure the errors the library returns if you try to verify an unsupported type of provenance (e.g. gitlab v0.2) are clear. This switch statement defaults to calling verifySystemParameters
for any type that isn't slsav1.NpmCLIGithubActionsProvenance
(with verifySystemParameters
assuming its a GitHub attestation). Another way to do it would be to check for all expected types and then default to returning some "unsupported type" error, to make it clear to external clients what's going on. But maybe this is handled elsewhere in the code before it gets to this function?
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.
Yes, earlier in the code, the codepath is routed based on BuilderID,
slsa-verifier/verifiers/verifier.go
Lines 26 to 32 in 96619e4
for _, v := range register.SLSAVerifiers { if v.IsAuthoritativeFor(name) { return v, nil } } // No builder found. return nil, fmt.Errorf("%w: %s", serrors.ErrorVerifierNotSupported, *builderOpts.ExpectedID) slsa-verifier/verifiers/internal/gha/verifier.go
Lines 42 to 45 in 96619e4
func (v *GHAVerifier) IsAuthoritativeFor(builderID string) bool { // This verifier only supports builders defined on GitHub. return strings.HasPrefix(builderID, httpsGithubCom) }
and will error for unsupported builders, like those from GitLab.
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.
OK that's great, thanks
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
I added some docs in a new commit |
@@ -9,13 +9,21 @@ import ( | |||
"github.com/slsa-framework/slsa-verifier/v2/verifiers/internal/gha/slsaprovenance/common" | |||
"github.com/slsa-framework/slsa-verifier/v2/verifiers/internal/gha/slsaprovenance/iface" | |||
slsav02 "github.com/slsa-framework/slsa-verifier/v2/verifiers/internal/gha/slsaprovenance/v0.2" | |||
slsav1 "github.com/slsa-framework/slsa-verifier/v2/verifiers/internal/gha/slsaprovenance/v1.0" | |||
) | |||
|
|||
func verifyProvenanceMatchesCertificate(prov iface.Provenance, workflow *WorkflowIdentity) error { |
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.
OK that makes sense. My only other concern would be making sure the errors the library returns if you try to verify an unsupported type of provenance (e.g. gitlab v0.2) are clear. This switch statement defaults to calling verifySystemParameters
for any type that isn't slsav1.NpmCLIGithubActionsProvenance
(with verifySystemParameters
assuming its a GitHub attestation). Another way to do it would be to check for all expected types and then default to returning some "unsupported type" error, to make it clear to external clients what's going on. But maybe this is handled elsewhere in the code before it gets to this function?
Signed-off-by: Ramon Petgrave <[email protected]>
Fixes #614, #450, #449, #515
Adds support for NPM CLIs build provenances, generated when running
npm publish --provenance --access public
from a GitHub Actions workflow.Testing
Future work
--print-provenance