slsa-framework / slsa-verifier

Verify provenance from SLSA compliant builders
Apache License 2.0
216 stars 45 forks source link

fix: npm publish verification #705

Closed laurentsimon closed 9 months ago

laurentsimon commented 9 months ago
laurentsimon commented 9 months ago

@trishankatdatadog PTAL

ianlewis commented 9 months ago

I like that you used the dsselib.EnvelopeVerifier interface.

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.

trishankatdatadog commented 9 months ago

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!

laurentsimon commented 9 months ago

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!

trishankatdatadog commented 9 months ago

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?

laurentsimon commented 9 months ago

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

trishankatdatadog commented 9 months ago

yes

Sorry to press you, but is there a test to this effect? I may have missed it...

laurentsimon commented 9 months ago

yes

Sorry to press you, but is there a test to this effect? I may have missed it...

the tests are not part of this PR. They are present in the rest of the code.