feat: VerifyNpmPackage API with supplied tuf client #768

ramonpetgrave64 · 2024-05-08T19:46:15Z

This PR

adds a new VerifyNpmPackageWithSigstoreTufClient method that allows supplying a SigstoreTufClient, and an io.Reader for the attestations json
adds a guide on how to use in ./docs/Api-Library.md

Offline rekor verification already works so long as the provenance is a valid sigstore bundle, though we could consider adding an explicit option to enforce offline rekor verifification.

https://github.com/ramonpetgrave64/slsa-verifier/blob/a2edf56f612f6ef4baef66fb07cd62988432d357/verifiers/internal/gha/provenance.go#L217-L229

Testing

manual invocation of the tool
unit added regression tests
used the library from another demo module, like in ./docs/Api-Library.md

Followups

support the 1.0 provenance spec
- Support verification for npm SLSA v1.0 #614
use a logger, rather than printing the intermediate messages, fix: #161: replace prints with logs #772
- slsa-verifier/verifiers/internal/gha/verifier.go
  
  Line 89 in e7a8f74
  
  fmt.Fprintf(os.Stderr, "Verified build using builder %q at commit %s\n",

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

ramonpetgrave64 · 2024-05-14T14:21:21Z

@slugclub

slugclub

This is looking great, thanks so much for working on this. I have a few minor nits but overall it's looking good.

slugclub · 2024-05-16T23:32:00Z

verifiers/internal/gha/npm_sigstore_tuf_test.go

@@ -87,9 +90,9 @@ type mockSigstoreTufClient struct {
 	fileContentMap map[string]string
 }

-// NewMockSigstoreTufClient returns an instance of the mock client,
+// newMockSigstoreTufClient returns an instance of the mock client,


Not for right now but in the future we might consider pulling out this mock client into its own test package so others can use it in tests (https://google.github.io/styleguide/go/best-practices.html#creating-test-helper-packages)

I'll be considering that. right now, the mock is very simple, so I guess folks can just copy-paste it.

We'll followup in #773

verifiers/internal/gha/verifier.go

docs/API-Library.md

verifiers/verifier_regression_test.go

verifiers/internal/gha/verifier.go

ramonpetgrave64 · 2024-05-17T21:18:27Z

This is looking great, thanks so much for working on this. I have a few minor nits but overall it's looking good.

Thanks for the review! I was also looking into logging in #772,

This reverts commit e855e4f. Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

slugclub

Looks good. A few minor nits but overall LGTM

docs/API-Library.md

verifiers/internal/gcb/verifier.go

verifiers/utils/sigstore_tuf.go

slugclub · 2024-05-19T21:58:43Z

verifiers/internal/gcb/verifier.go

+	provenanceOpts *options.ProvenanceOpts,
+	builderOpts *options.BuilderOpts,
+	sigstoreTufClient utils.SigstoreTufClient,
+) ([]byte, *utils.TrustedBuilderID, error) {


I know this func is following from VerifyNpmPackage but I wonder if it should return the data in a more structured manner to make it easier for callers to inspect the fields they're interested in. At the moment, you'd have to parse the attestation yourself to get information from it (except builder id which is helpfully extracted). That's quite involved because the structure of the attestation is complex.

I think this is beyond the scope of what this PR is trying to do, so don't worry about it for these changes but maybe something to think about for the future? Even just having slsa-verifier expose some of its functions for parsing an attestation could work.

With the returned []byte, I think that's what StatementsFromBytes is for, but its not yet implemented for npm.

https://pkg.go.dev/github.com/slsa-framework/slsa-verifier/v2@v2.5.1/verifiers/utils#StatementFromBytes

slsa-verifier/verifiers/internal/gha/npm.go

Lines 224 to 229 in 87b5bae

func (n *Npm) verifiedProvenanceBytes() ([]byte, error) {

// TODO(#493): prune the provenance and return only

// verified fields.

// NOTE: we currently don't verify the materials' commit sha.

return []byte{}, nil

}

@laurentsimon, @ianlewis , regarding #493, is it now safe to return the entire provenance, once verified?

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

ramonpetgrave64 · 2024-05-21T20:08:08Z

@slugclub thanks again. @ianlewis @laurentsimon , please take a look

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

haydentherapper · 2024-05-21T23:17:02Z

register/register.go

@@ -34,6 +34,12 @@ type SLSAVerifier interface {
 		provenanceOpts *options.ProvenanceOpts,
 		builderOpts *options.BuilderOpts,
 	) ([]byte, *utils.TrustedBuilderID, error)
+
+	VerifyNpmPackageWithSigstoreTUFClient(ctx context.Context,


As we chatted about offline, we may want this API to take the npm verification material directly and another API responsible for fetching this target file from TUF. Then this API could verify metadata offline, and the latter API would be responsible for network calls. I think this would also make testing easier since you don't have to mock the Sigstore TUF client, just need the verification material struct initialized.

Another change, outside of the scope of this PR, would be to update https://github.com/slsa-framework/slsa-verifier/blob/main/verifiers/internal/gha/trusted_root.go to use TrustedMaterial rather than Cosign's APIs. Like you're already doing in https://github.com/slsa-framework/slsa-verifier/blob/main/verifiers/internal/gha/bundle.go#L162, rather than passing the TUF client, you would pass in the trusted root material.

Speaking about this offline again, we have our own struct to parse the material, but the spec/layout of the material isn't defined in npmjs docs. Until we can be sure that npmjs will commit to this layout, I think it's better to hide this detail from the user, or let the user stub out their own SigstoreTUFClient.

https://docs.npmjs.com/about-registry-signatures#supporting-signatures-on-third-party-registries

verifiers/internal/gha/npm.go

verifiers/utils/sigstore_tuf.go

ramonpetgrave64 added 10 commits May 8, 2024 16:45

refactor: allow passing in an optional SigstoreTufClient

e4c034a

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

add Test_VerifyNpmPackage

a2edf56

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

lint

5441105

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

use sigstore-go@v0.3.0

5f3efc3

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

make it a new function in the interface

c78aa12

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

disbale parallel test for runVerify...

691b5ad

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

Merge branch 'main' into ramonpetgrave64-lib-api

f01388e

document the new feature

28753b2

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

cleanup

db1b43a

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

undo

2f94ff2

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

ramonpetgrave64 changed the title ~~[draft] feat: VerifyNpmPackage API~~ feat: VerifyNpmPackage API with supplied tuf client May 13, 2024

ramonpetgrave64 added 4 commits May 13, 2024 21:56

add io readfer for attestations

8f223bd

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

add specific test

30045b0

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

typo

76c83be

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

make attestations an io reader

e855e4f

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

slugclub reviewed May 17, 2024

View reviewed changes

ramonpetgrave64 mentioned this pull request May 17, 2024

fix: #161: replace prints with logs #772

Draft

ramonpetgrave64 and others added 5 commits May 17, 2024 21:22

Revert "make attestations an io reader"

e50d91b

This reverts commit e855e4f. Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

builderId to builderID

8d4ce38

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

no ioreader, better example in docs

56886da

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

copy testdata

a9e7846

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

Merge branch 'main' into ramonpetgrave64-lib-api

2535898

ramonpetgrave64 marked this pull request as ready for review May 17, 2024 22:09

ramonpetgrave64 requested review from laurentsimon and kpk47 as code owners May 17, 2024 22:09

ramonpetgrave64 requested review from ianlewis and slugclub May 17, 2024 22:10

slugclub approved these changes May 19, 2024

View reviewed changes

ramonpetgrave64 added 4 commits May 21, 2024 18:51

readme comments

1f405c8

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

fix docstring

d1e6dc4

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

tuf to TUF initialism

56b68af

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

add example about utility method for making a client

97ea5f8

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

ramonpetgrave64 mentioned this pull request May 21, 2024

feat: expose SigstoreTUFClient mocks #773

Open

typo

fb7d931

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

haydentherapper reviewed May 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: VerifyNpmPackage API with supplied tuf client #768

feat: VerifyNpmPackage API with supplied tuf client #768

ramonpetgrave64 commented May 8, 2024 •

edited

ramonpetgrave64 commented May 14, 2024

slugclub left a comment

slugclub May 16, 2024

ramonpetgrave64 May 17, 2024

ramonpetgrave64 May 21, 2024

ramonpetgrave64 commented May 17, 2024

slugclub left a comment

slugclub May 19, 2024

ramonpetgrave64 May 21, 2024

ramonpetgrave64 commented May 21, 2024

haydentherapper May 21, 2024

ramonpetgrave64 May 23, 2024

	func (n *Npm) verifiedProvenanceBytes() ([]byte, error) {
	// TODO(#493): prune the provenance and return only
	// verified fields.
	// NOTE: we currently don't verify the materials' commit sha.
	return []byte{}, nil
	}

feat: VerifyNpmPackage API with supplied tuf client #768

Are you sure you want to change the base?

feat: VerifyNpmPackage API with supplied tuf client #768

Conversation

ramonpetgrave64 commented May 8, 2024 • edited

Testing

Followups

ramonpetgrave64 commented May 14, 2024

slugclub left a comment

Choose a reason for hiding this comment

slugclub May 16, 2024

Choose a reason for hiding this comment

ramonpetgrave64 May 17, 2024

Choose a reason for hiding this comment

ramonpetgrave64 May 21, 2024

Choose a reason for hiding this comment

ramonpetgrave64 commented May 17, 2024

slugclub left a comment

Choose a reason for hiding this comment

slugclub May 19, 2024

Choose a reason for hiding this comment

ramonpetgrave64 May 21, 2024

Choose a reason for hiding this comment

ramonpetgrave64 commented May 21, 2024

haydentherapper May 21, 2024

Choose a reason for hiding this comment

ramonpetgrave64 May 23, 2024

Choose a reason for hiding this comment

ramonpetgrave64 commented May 8, 2024 •

edited