-
Notifications
You must be signed in to change notification settings - Fork 46
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: npm publish verification #705
fix: npm publish verification #705
Conversation
laurentsimon
commented
Sep 26, 2023
•
edited
Loading
edited
- adding support for IEEE P1363 formatted signatures
- fix the npm publish attestation bug. The verification always return success, because it was not using PAE signature
Signed-off-by: laurentsimon <laurentsimon@google.com>
b333e6c
to
90a9a36
Compare
@trishankatdatadog PTAL |
I like that you used the Could you maybe add a bit more in the PR description as to what this PR is doing? esp. that this is adding support for IEEE P1363 formatted signatures. |
Signed-off-by: laurentsimon <laurentsimon@google.com>
Signed-off-by: laurentsimon <laurentsimon@google.com>
Can confirm the patch works: ➜ slsa-verifier git:(fix/npm-publish-sig) SLSA_VERIFIER_EXPERIMENTAL=1 go run ./cli/slsa-verifier verify-npm-package ~/github.com/trishankatdatadog/hekate/supreme-goggles.tgz \
--attestations-path ~/github.com/trishankatdatadog/hekate/attestations.json \
--builder-id "https://github.com/actions/runner/github-hosted" \
--package-name "@trishankatdatadog/supreme-goggles" \
--package-version 1.0.5 \
--source-uri github.com/trishankatdatadog/supreme-goggles
Verified build using builder https://github.com/actions/runner/github-hosted at commit 38ebf99444e033b2f1550c9aaaeacd62d02a12ba
Verifying npm package /Users/trishank.kuppusamy/github.com/trishankatdatadog/hekate/supreme-goggles.tgz: FAILED: invalid signature: accepted signatures do not match threshold, Found: 0, Expected 1
FAILED: SLSA verification failed: invalid signature: accepted signatures do not match threshold, Found: 0, Expected 1
exit status 1 Will try to review code ASAP. Thanks for fixing this! |
Co-authored-by: Ian Lewis <ianlewis@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Co-authored-by: Ian Lewis <ianlewis@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Co-authored-by: Ian Lewis <ianlewis@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Co-authored-by: Ian Lewis <ianlewis@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Co-authored-by: Ian Lewis <ianlewis@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Co-authored-by: Ian Lewis <ianlewis@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Co-authored-by: Ian Lewis <ianlewis@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Co-authored-by: Ian Lewis <ianlewis@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Co-authored-by: Ian Lewis <ianlewis@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Co-authored-by: Ian Lewis <ianlewis@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Co-authored-by: Ian Lewis <ianlewis@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Co-authored-by: Ian Lewis <ianlewis@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Signed-off-by: laurentsimon <laurentsimon@google.com>
Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
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, could use another pair of eyes to see whether I missed anything, thx!
@ianlewis provided a good review already. Waiting for his LGTM too. Thanks again! |
Just to double-check: we do verify that the observed artefact digest does match the expected subject digest in both the provenance and publish attestations, correct? |
yes |
Sorry to press you, but is there a test to this effect? I may have missed it... |
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
err: serrors.ErrorInvalidSignature, | ||
}, | ||
{ | ||
name: "invalid signature publish npm CLI", |
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.
We have tests for package name, version, and signature mismatches but @trishankatdatadog mentioned that we are missing a test for a publish attestation digest mismatch.
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.
Should we create an issue to track this?
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.
is #707 ok?
func DsseVerifierNew(content []byte, format KeyFormat, keyID string, sigEncoding *SignatureEncoding) (*dsselib.EnvelopeVerifier, error) { | ||
if format == KeyFormatPEM { | ||
block, rest := pem.Decode(content) | ||
if rest != 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.
nit
if rest != nil { | |
if len(rest) != 0 { |
the tests are not part of this PR. They are present in the rest of the code. |