From a78c06970a6b351da65f449d61278b0838e774a5 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 24 Mar 2025 17:28:00 -0600 Subject: [PATCH 01/18] pass predicate type to get attestation api methods Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 21 ++++++++++-------- pkg/cmd/attestation/api/client_test.go | 30 +++++++++++++------------- pkg/cmd/attestation/api/mock_client.go | 20 ++++++++--------- 3 files changed, 37 insertions(+), 34 deletions(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index 1e99a2a06..8c1af495f 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -39,8 +39,8 @@ type httpClient interface { } type Client interface { - GetByRepoAndDigest(repo, digest string, limit int) ([]*Attestation, error) - GetByOwnerAndDigest(owner, digest string, limit int) ([]*Attestation, error) + GetByRepoAndDigest(repo, digest, predicateType string, limit int) ([]*Attestation, error) + GetByOwnerAndDigest(owner, digest, predicateType string, limit int) ([]*Attestation, error) GetTrustDomain() (string, error) } @@ -61,21 +61,21 @@ func NewLiveClient(hc *http.Client, host string, l *ioconfig.Handler) *LiveClien } // GetByRepoAndDigest fetches the attestation by repo and digest -func (c *LiveClient) GetByRepoAndDigest(repo, digest string, limit int) ([]*Attestation, error) { +func (c *LiveClient) GetByRepoAndDigest(repo, digest, predicateType string, limit int) ([]*Attestation, error) { c.logger.VerbosePrintf("Fetching attestations for artifact digest %s\n\n", digest) url := fmt.Sprintf(GetAttestationByRepoAndSubjectDigestPath, repo, digest) - return c.getByURL(url, limit) + return c.getByURL(url, predicateType, limit) } // GetByOwnerAndDigest fetches attestation by owner and digest -func (c *LiveClient) GetByOwnerAndDigest(owner, digest string, limit int) ([]*Attestation, error) { +func (c *LiveClient) GetByOwnerAndDigest(owner, digest, predicateType string, limit int) ([]*Attestation, error) { c.logger.VerbosePrintf("Fetching attestations for artifact digest %s\n\n", digest) url := fmt.Sprintf(GetAttestationByOwnerAndSubjectDigestPath, owner, digest) - return c.getByURL(url, limit) + return c.getByURL(url, predicateType, limit) } -func (c *LiveClient) getByURL(url string, limit int) ([]*Attestation, error) { - attestations, err := c.getAttestations(url, limit) +func (c *LiveClient) getByURL(url, predicateType string, limit int) ([]*Attestation, error) { + attestations, err := c.getAttestations(url, predicateType, limit) if err != nil { return nil, err } @@ -94,7 +94,7 @@ func (c *LiveClient) GetTrustDomain() (string, error) { return c.getTrustDomain(MetaPath) } -func (c *LiveClient) getAttestations(url string, limit int) ([]*Attestation, error) { +func (c *LiveClient) getAttestations(url, predicateType string, limit int) ([]*Attestation, error) { perPage := limit if perPage <= 0 || perPage > maxLimitForFlag { return nil, fmt.Errorf("limit must be greater than 0 and less than or equal to %d", maxLimitForFlag) @@ -106,6 +106,9 @@ func (c *LiveClient) getAttestations(url string, limit int) ([]*Attestation, err // ref: https://github.com/cli/go-gh/blob/d32c104a9a25c9de3d7c7b07a43ae0091441c858/example_gh_test.go#L96 url = fmt.Sprintf("%s?per_page=%d", url, perPage) + if predicateType != "" { + url = fmt.Sprintf("%s&predicate_type=%s", url, predicateType) + } var attestations []*Attestation var resp AttestationsResponse diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index 787408a4e..2a62d5662 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -44,14 +44,14 @@ func NewClientWithMockGHClient(hasNextPage bool) Client { func TestGetByDigest(t *testing.T) { c := NewClientWithMockGHClient(false) - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, DefaultLimit) + attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) require.NoError(t, err) require.Equal(t, 5, len(attestations)) bundle := (attestations)[0].Bundle require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, DefaultLimit) + attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) require.NoError(t, err) require.Equal(t, 5, len(attestations)) @@ -64,14 +64,14 @@ func TestGetByDigestGreaterThanLimit(t *testing.T) { limit := 3 // The method should return five results when the limit is not set - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, limit) + attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", limit) require.NoError(t, err) require.Equal(t, 3, len(attestations)) bundle := (attestations)[0].Bundle require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, limit) + attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", limit) require.NoError(t, err) require.Equal(t, len(attestations), limit) @@ -81,14 +81,14 @@ func TestGetByDigestGreaterThanLimit(t *testing.T) { func TestGetByDigestWithNextPage(t *testing.T) { c := NewClientWithMockGHClient(true) - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, DefaultLimit) + attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) require.NoError(t, err) require.Equal(t, len(attestations), 10) bundle := (attestations)[0].Bundle require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, DefaultLimit) + attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) require.NoError(t, err) require.Equal(t, len(attestations), 10) @@ -101,14 +101,14 @@ func TestGetByDigestGreaterThanLimitWithNextPage(t *testing.T) { limit := 7 // The method should return five results when the limit is not set - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, limit) + attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", limit) require.NoError(t, err) require.Equal(t, len(attestations), limit) bundle := (attestations)[0].Bundle require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, limit) + attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", limit) require.NoError(t, err) require.Equal(t, len(attestations), limit) @@ -130,12 +130,12 @@ func TestGetByDigest_NoAttestationsFound(t *testing.T) { logger: io.NewTestHandler(), } - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, DefaultLimit) + attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) require.Error(t, err) require.IsType(t, ErrNoAttestationsFound, err) require.Nil(t, attestations) - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, DefaultLimit) + attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) require.Error(t, err) require.IsType(t, ErrNoAttestationsFound, err) require.Nil(t, attestations) @@ -153,11 +153,11 @@ func TestGetByDigest_Error(t *testing.T) { logger: io.NewTestHandler(), } - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, DefaultLimit) + attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) require.Error(t, err) require.Nil(t, attestations) - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, DefaultLimit) + attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) require.Error(t, err) require.Nil(t, attestations) } @@ -362,7 +362,7 @@ func TestGetAttestationsRetries(t *testing.T) { logger: io.NewTestHandler(), } - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, DefaultLimit) + attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) require.NoError(t, err) // assert the error path was executed; because this is a paged @@ -375,7 +375,7 @@ func TestGetAttestationsRetries(t *testing.T) { require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") // same test as above, but for GetByOwnerAndDigest: - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, DefaultLimit) + attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) require.NoError(t, err) // because we haven't reset the mock, we have added 2 more failed requests @@ -401,7 +401,7 @@ func TestGetAttestationsMaxRetries(t *testing.T) { logger: io.NewTestHandler(), } - _, err := c.GetByRepoAndDigest(testRepo, testDigest, DefaultLimit) + _, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) require.Error(t, err) fetcher.AssertNumberOfCalls(t, "OnREST500Error", 4) diff --git a/pkg/cmd/attestation/api/mock_client.go b/pkg/cmd/attestation/api/mock_client.go index b2fd334c0..8e6dcdd6f 100644 --- a/pkg/cmd/attestation/api/mock_client.go +++ b/pkg/cmd/attestation/api/mock_client.go @@ -7,17 +7,17 @@ import ( ) type MockClient struct { - OnGetByRepoAndDigest func(repo, digest string, limit int) ([]*Attestation, error) - OnGetByOwnerAndDigest func(owner, digest string, limit int) ([]*Attestation, error) + OnGetByRepoAndDigest func(repo, digest, predicateType string, limit int) ([]*Attestation, error) + OnGetByOwnerAndDigest func(owner, digest, predicateType string, limit int) ([]*Attestation, error) OnGetTrustDomain func() (string, error) } -func (m MockClient) GetByRepoAndDigest(repo, digest string, limit int) ([]*Attestation, error) { - return m.OnGetByRepoAndDigest(repo, digest, limit) +func (m MockClient) GetByRepoAndDigest(repo, digest, predicateType string, limit int) ([]*Attestation, error) { + return m.OnGetByRepoAndDigest(repo, digest, predicateType, limit) } -func (m MockClient) GetByOwnerAndDigest(owner, digest string, limit int) ([]*Attestation, error) { - return m.OnGetByOwnerAndDigest(owner, digest, limit) +func (m MockClient) GetByOwnerAndDigest(owner, digest, predicateType string, limit int) ([]*Attestation, error) { + return m.OnGetByOwnerAndDigest(owner, digest, predicateType, limit) } func (m MockClient) GetTrustDomain() (string, error) { @@ -28,23 +28,23 @@ func makeTestAttestation() Attestation { return Attestation{Bundle: data.SigstoreBundle(nil), BundleURL: "https://example.com"} } -func OnGetByRepoAndDigestSuccess(repo, digest string, limit int) ([]*Attestation, error) { +func OnGetByRepoAndDigestSuccess(repo, digest, predicateType string, limit int) ([]*Attestation, error) { att1 := makeTestAttestation() att2 := makeTestAttestation() return []*Attestation{&att1, &att2}, nil } -func OnGetByRepoAndDigestFailure(repo, digest string, limit int) ([]*Attestation, error) { +func OnGetByRepoAndDigestFailure(repo, digest, predicateType string, limit int) ([]*Attestation, error) { return nil, fmt.Errorf("failed to fetch by repo and digest") } -func OnGetByOwnerAndDigestSuccess(owner, digest string, limit int) ([]*Attestation, error) { +func OnGetByOwnerAndDigestSuccess(owner, digest, predicateType string, limit int) ([]*Attestation, error) { att1 := makeTestAttestation() att2 := makeTestAttestation() return []*Attestation{&att1, &att2}, nil } -func OnGetByOwnerAndDigestFailure(owner, digest string, limit int) ([]*Attestation, error) { +func OnGetByOwnerAndDigestFailure(owner, digest, predicateType string, limit int) ([]*Attestation, error) { return nil, fmt.Errorf("failed to fetch by owner and digest") } From faef81f4bc7bea7fdee721a700dd563d6669f31f Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 24 Mar 2025 17:28:50 -0600 Subject: [PATCH 02/18] reorganize getAttestations func to check for remote gh api fetching first Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/download/download_test.go | 4 +- .../attestation/verification/attestation.go | 13 ++++--- pkg/cmd/attestation/verify/attestation.go | 39 ++++++++++--------- pkg/cmd/attestation/verify/options.go | 6 +++ 4 files changed, 35 insertions(+), 27 deletions(-) diff --git a/pkg/cmd/attestation/download/download_test.go b/pkg/cmd/attestation/download/download_test.go index ddcd08c92..899d15339 100644 --- a/pkg/cmd/attestation/download/download_test.go +++ b/pkg/cmd/attestation/download/download_test.go @@ -275,7 +275,7 @@ func TestRunDownload(t *testing.T) { t.Run("no attestations found", func(t *testing.T) { opts := baseOpts opts.APIClient = api.MockClient{ - OnGetByOwnerAndDigest: func(repo, digest string, limit int) ([]*api.Attestation, error) { + OnGetByOwnerAndDigest: func(repo, digest, predicateType string, limit int) ([]*api.Attestation, error) { return nil, api.ErrNoAttestationsFound }, } @@ -291,7 +291,7 @@ func TestRunDownload(t *testing.T) { t.Run("failed to fetch attestations", func(t *testing.T) { opts := baseOpts opts.APIClient = api.MockClient{ - OnGetByOwnerAndDigest: func(repo, digest string, limit int) ([]*api.Attestation, error) { + OnGetByOwnerAndDigest: func(repo, digest, predicateType string, limit int) ([]*api.Attestation, error) { return nil, fmt.Errorf("failed to fetch attestations") }, } diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index db419ebac..53a53722b 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -21,10 +21,11 @@ var ErrUnrecognisedBundleExtension = errors.New("bundle file extension not suppo var ErrEmptyBundleFile = errors.New("provided bundle file is empty") type FetchRemoteAttestationsParams struct { - Digest string - Limit int - Owner string - Repo string + Digest string + Limit int + Owner string + PredicateType string + Repo string } // GetLocalAttestations returns a slice of attestations read from a local bundle file. @@ -96,13 +97,13 @@ func GetRemoteAttestations(client api.Client, params FetchRemoteAttestationsPara // check if Repo is set first because if Repo has been set, Owner will be set using the value of Repo. // If Repo is not set, the field will remain empty. It will not be populated using the value of Owner. if params.Repo != "" { - attestations, err := client.GetByRepoAndDigest(params.Repo, params.Digest, params.Limit) + attestations, err := client.GetByRepoAndDigest(params.Repo, params.Digest, params.PredicateType, params.Limit) if err != nil { return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Repo, err) } return attestations, nil } else if params.Owner != "" { - attestations, err := client.GetByOwnerAndDigest(params.Owner, params.Digest, params.Limit) + attestations, err := client.GetByOwnerAndDigest(params.Owner, params.Digest, params.PredicateType, params.Limit) if err != nil { return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Owner, err) } diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index bb96c9526..55fcf1f7e 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -10,7 +10,24 @@ import ( ) func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestation, string, error) { - if o.BundlePath != "" { + if o.FetchAttestationsFromGitHubAPI() { + params := verification.FetchRemoteAttestationsParams{ + Digest: a.DigestWithAlg(), + Limit: o.Limit, + Owner: o.Owner, + PredicateType: o.PredicateType, + Repo: o.Repo, + } + + attestations, err := verification.GetRemoteAttestations(o.APIClient, params) + if err != nil { + msg := "✗ Loading attestations from GitHub API failed" + return nil, msg, err + } + pluralAttestation := text.Pluralize(len(attestations), "attestation") + msg := fmt.Sprintf("Loaded %s from GitHub API", pluralAttestation) + return attestations, msg, nil + } else if o.BundlePath != "" { attestations, err := verification.GetLocalAttestations(o.BundlePath) if err != nil { msg := fmt.Sprintf("✗ Loading attestations from %s failed", a.URL) @@ -19,9 +36,7 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio pluralAttestation := text.Pluralize(len(attestations), "attestation") msg := fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.BundlePath) return attestations, msg, nil - } - - if o.UseBundleFromRegistry { + } else if o.UseBundleFromRegistry { attestations, err := verification.GetOCIAttestations(o.OCIClient, a) if err != nil { msg := "✗ Loading attestations from OCI registry failed" @@ -32,21 +47,7 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio return attestations, msg, nil } - params := verification.FetchRemoteAttestationsParams{ - Digest: a.DigestWithAlg(), - Limit: o.Limit, - Owner: o.Owner, - Repo: o.Repo, - } - - attestations, err := verification.GetRemoteAttestations(o.APIClient, params) - if err != nil { - msg := "✗ Loading attestations from GitHub API failed" - return nil, msg, err - } - pluralAttestation := text.Pluralize(len(attestations), "attestation") - msg := fmt.Sprintf("Loaded %s from GitHub API", pluralAttestation) - return attestations, msg, nil + return nil, "", fmt.Errorf("no valid attestation source provided") } func verifyAttestations(art artifact.DigestedArtifact, att []*api.Attestation, sgVerifier verification.SigstoreVerifier, ec verification.EnforcementCriteria) ([]*verification.AttestationProcessingResult, string, error) { diff --git a/pkg/cmd/attestation/verify/options.go b/pkg/cmd/attestation/verify/options.go index 0fbbec55a..e47c4f4a8 100644 --- a/pkg/cmd/attestation/verify/options.go +++ b/pkg/cmd/attestation/verify/options.go @@ -53,6 +53,12 @@ func (opts *Options) Clean() { } } +// FetchAttestationsFromGitHubAPI returns true if the command should fetch attestations from the GitHub API +// It checks that a bundle path is not provided and that the "use bundle from registry" flag is not set +func (opts *Options) FetchAttestationsFromGitHubAPI() bool { + return opts.BundlePath == "" && !opts.UseBundleFromRegistry +} + // AreFlagsValid checks that the provided flag combination is valid // and returns an error otherwise func (opts *Options) AreFlagsValid() error { From ad20ef35d9f97f30c352cf613ba167a08918a4ad Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 24 Mar 2025 17:35:52 -0600 Subject: [PATCH 03/18] move local and oci registry attestation filtering Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/attestation.go | 30 ++++++++++++++++++++--- pkg/cmd/attestation/verify/verify.go | 8 ------ 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index 55fcf1f7e..5774170d9 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -9,6 +9,16 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/verification" ) +func filterByPredicateType(predicateType string, attestations []*api.Attestation) ([]*api.Attestation, string, error) { + // Apply predicate type filter to returned attestations + filteredAttestations := verification.FilterAttestations(predicateType, attestations) + if len(filteredAttestations) == 0 { + msg := fmt.Sprintf("✗ No attestations found with predicate type: %s\n", predicateType) + return nil, msg, fmt.Errorf("no matching predicate found") + } + return filteredAttestations, "", nil +} + func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestation, string, error) { if o.FetchAttestationsFromGitHubAPI() { params := verification.FetchRemoteAttestationsParams{ @@ -33,18 +43,30 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio msg := fmt.Sprintf("✗ Loading attestations from %s failed", a.URL) return nil, msg, err } - pluralAttestation := text.Pluralize(len(attestations), "attestation") + + filtered, errMsg, err := filterByPredicateType(o.PredicateType, attestations) + if err != nil { + return nil, errMsg, err + } + + pluralAttestation := text.Pluralize(len(filtered), "attestation") msg := fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.BundlePath) - return attestations, msg, nil + return filtered, msg, nil } else if o.UseBundleFromRegistry { attestations, err := verification.GetOCIAttestations(o.OCIClient, a) if err != nil { msg := "✗ Loading attestations from OCI registry failed" return nil, msg, err } - pluralAttestation := text.Pluralize(len(attestations), "attestation") + + filtered, errMsg, err := filterByPredicateType(o.PredicateType, attestations) + if err != nil { + return nil, errMsg, err + } + + pluralAttestation := text.Pluralize(len(filtered), "attestation") msg := fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.ArtifactPath) - return attestations, msg, nil + return filtered, msg, nil } return nil, "", fmt.Errorf("no valid attestation source provided") diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 65ae8ca3e..1de4172b4 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -235,14 +235,6 @@ func runVerify(opts *Options) error { // Print the message signifying success fetching attestations opts.Logger.Println(logMsg) - // Apply predicate type filter to returned attestations - filteredAttestations := verification.FilterAttestations(ec.PredicateType, attestations) - if len(filteredAttestations) == 0 { - opts.Logger.Printf(opts.Logger.ColorScheme.Red("✗ No attestations found with predicate type: %s\n"), opts.PredicateType) - return fmt.Errorf("no matching predicate found") - } - attestations = filteredAttestations - // print information about the policy that will be enforced against attestations opts.Logger.Println("\nThe following policy criteria will be enforced:") opts.Logger.Println(ec.BuildPolicyInformation()) From 95a61974bf4bff0c916fd0e5434b78c614780372 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 24 Mar 2025 18:01:57 -0600 Subject: [PATCH 04/18] pass params object to api client methods Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 46 +++++++++------- pkg/cmd/attestation/api/client_test.go | 55 ++++++++++++++----- pkg/cmd/attestation/api/mock_client.go | 20 +++---- pkg/cmd/attestation/download/download.go | 2 +- pkg/cmd/attestation/download/download_test.go | 4 +- .../attestation/verification/attestation.go | 14 +---- pkg/cmd/attestation/verify/attestation.go | 2 +- 7 files changed, 84 insertions(+), 59 deletions(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index 8c1af495f..ae33d9ce3 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -27,6 +27,15 @@ const ( // Allow injecting backoff interval in tests. var getAttestationRetryInterval = time.Millisecond * 200 +// FetchParams are the parameters for fetching attestations from the GitHub API +type FetchParams struct { + Digest string + Limit int + Owner string + PredicateType string + Repo string +} + // githubApiClient makes REST calls to the GitHub API type githubApiClient interface { REST(hostname, method, p string, body io.Reader, data interface{}) error @@ -39,8 +48,8 @@ type httpClient interface { } type Client interface { - GetByRepoAndDigest(repo, digest, predicateType string, limit int) ([]*Attestation, error) - GetByOwnerAndDigest(owner, digest, predicateType string, limit int) ([]*Attestation, error) + GetByRepoAndDigest(params FetchParams) ([]*Attestation, error) + GetByOwnerAndDigest(params FetchParams) ([]*Attestation, error) GetTrustDomain() (string, error) } @@ -61,21 +70,20 @@ func NewLiveClient(hc *http.Client, host string, l *ioconfig.Handler) *LiveClien } // GetByRepoAndDigest fetches the attestation by repo and digest -func (c *LiveClient) GetByRepoAndDigest(repo, digest, predicateType string, limit int) ([]*Attestation, error) { - c.logger.VerbosePrintf("Fetching attestations for artifact digest %s\n\n", digest) - url := fmt.Sprintf(GetAttestationByRepoAndSubjectDigestPath, repo, digest) - return c.getByURL(url, predicateType, limit) +func (c *LiveClient) GetByRepoAndDigest(params FetchParams) ([]*Attestation, error) { + url := fmt.Sprintf(GetAttestationByRepoAndSubjectDigestPath, params.Repo, params.Digest) + return c.getByURL(url, params) } // GetByOwnerAndDigest fetches attestation by owner and digest -func (c *LiveClient) GetByOwnerAndDigest(owner, digest, predicateType string, limit int) ([]*Attestation, error) { - c.logger.VerbosePrintf("Fetching attestations for artifact digest %s\n\n", digest) - url := fmt.Sprintf(GetAttestationByOwnerAndSubjectDigestPath, owner, digest) - return c.getByURL(url, predicateType, limit) +func (c *LiveClient) GetByOwnerAndDigest(params FetchParams) ([]*Attestation, error) { + url := fmt.Sprintf(GetAttestationByOwnerAndSubjectDigestPath, params.Owner, params.Digest) + return c.getByURL(url, params) } -func (c *LiveClient) getByURL(url, predicateType string, limit int) ([]*Attestation, error) { - attestations, err := c.getAttestations(url, predicateType, limit) +func (c *LiveClient) getByURL(url string, params FetchParams) ([]*Attestation, error) { + c.logger.VerbosePrintf("Fetching attestations for artifact digest %s\n\n", params.Digest) + attestations, err := c.getAttestations(url, params) if err != nil { return nil, err } @@ -94,8 +102,8 @@ func (c *LiveClient) GetTrustDomain() (string, error) { return c.getTrustDomain(MetaPath) } -func (c *LiveClient) getAttestations(url, predicateType string, limit int) ([]*Attestation, error) { - perPage := limit +func (c *LiveClient) getAttestations(url string, params FetchParams) ([]*Attestation, error) { + perPage := params.Limit if perPage <= 0 || perPage > maxLimitForFlag { return nil, fmt.Errorf("limit must be greater than 0 and less than or equal to %d", maxLimitForFlag) } @@ -106,8 +114,8 @@ func (c *LiveClient) getAttestations(url, predicateType string, limit int) ([]*A // ref: https://github.com/cli/go-gh/blob/d32c104a9a25c9de3d7c7b07a43ae0091441c858/example_gh_test.go#L96 url = fmt.Sprintf("%s?per_page=%d", url, perPage) - if predicateType != "" { - url = fmt.Sprintf("%s&predicate_type=%s", url, predicateType) + if params.PredicateType != "" { + url = fmt.Sprintf("%s&predicate_type=%s", url, params.PredicateType) } var attestations []*Attestation @@ -115,7 +123,7 @@ func (c *LiveClient) getAttestations(url, predicateType string, limit int) ([]*A bo := backoff.NewConstantBackOff(getAttestationRetryInterval) // if no attestation or less than limit, then keep fetching - for url != "" && len(attestations) < limit { + for url != "" && len(attestations) < params.Limit { err := backoff.Retry(func() error { newURL, restErr := c.githubAPI.RESTWithNext(c.host, http.MethodGet, url, nil, &resp) @@ -143,8 +151,8 @@ func (c *LiveClient) getAttestations(url, predicateType string, limit int) ([]*A return nil, ErrNoAttestationsFound } - if len(attestations) > limit { - return attestations[:limit], nil + if len(attestations) > params.Limit { + return attestations[:params.Limit], nil } return attestations, nil diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index 2a62d5662..f77b39d08 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -42,16 +42,24 @@ func NewClientWithMockGHClient(hasNextPage bool) Client { } } +var testFetchParams = FetchParams{ + Digest: testDigest, + Limit: DefaultLimit, + PredicateType: "https://slsa.dev/provenance/v1", +} + func TestGetByDigest(t *testing.T) { c := NewClientWithMockGHClient(false) - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) + testFetchParams.Repo = testRepo + attestations, err := c.GetByRepoAndDigest(testFetchParams) require.NoError(t, err) require.Equal(t, 5, len(attestations)) bundle := (attestations)[0].Bundle require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) + testFetchParams.Owner = testOwner + attestations, err = c.GetByOwnerAndDigest(testFetchParams) require.NoError(t, err) require.Equal(t, 5, len(attestations)) @@ -64,14 +72,17 @@ func TestGetByDigestGreaterThanLimit(t *testing.T) { limit := 3 // The method should return five results when the limit is not set - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", limit) + testFetchParams.Limit = limit + testFetchParams.Repo = testRepo + attestations, err := c.GetByRepoAndDigest(testFetchParams) require.NoError(t, err) require.Equal(t, 3, len(attestations)) bundle := (attestations)[0].Bundle require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", limit) + testFetchParams.Owner = testOwner + attestations, err = c.GetByOwnerAndDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), limit) @@ -81,14 +92,17 @@ func TestGetByDigestGreaterThanLimit(t *testing.T) { func TestGetByDigestWithNextPage(t *testing.T) { c := NewClientWithMockGHClient(true) - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) + testFetchParams.Repo = testRepo + testFetchParams.Limit = 30 + attestations, err := c.GetByRepoAndDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), 10) bundle := (attestations)[0].Bundle require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) + testFetchParams.Owner = testOwner + attestations, err = c.GetByOwnerAndDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), 10) @@ -101,14 +115,17 @@ func TestGetByDigestGreaterThanLimitWithNextPage(t *testing.T) { limit := 7 // The method should return five results when the limit is not set - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", limit) + testFetchParams.Limit = limit + testFetchParams.Repo = testRepo + attestations, err := c.GetByRepoAndDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), limit) bundle := (attestations)[0].Bundle require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", limit) + testFetchParams.Owner = testOwner + attestations, err = c.GetByOwnerAndDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), limit) @@ -130,12 +147,14 @@ func TestGetByDigest_NoAttestationsFound(t *testing.T) { logger: io.NewTestHandler(), } - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) + testFetchParams.Repo = testRepo + attestations, err := c.GetByRepoAndDigest(testFetchParams) require.Error(t, err) require.IsType(t, ErrNoAttestationsFound, err) require.Nil(t, attestations) - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) + testFetchParams.Owner = testOwner + attestations, err = c.GetByOwnerAndDigest(testFetchParams) require.Error(t, err) require.IsType(t, ErrNoAttestationsFound, err) require.Nil(t, attestations) @@ -153,11 +172,13 @@ func TestGetByDigest_Error(t *testing.T) { logger: io.NewTestHandler(), } - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) + testFetchParams.Repo = testRepo + attestations, err := c.GetByRepoAndDigest(testFetchParams) require.Error(t, err) require.Nil(t, attestations) - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) + testFetchParams.Owner = testOwner + attestations, err = c.GetByOwnerAndDigest(testFetchParams) require.Error(t, err) require.Nil(t, attestations) } @@ -362,7 +383,9 @@ func TestGetAttestationsRetries(t *testing.T) { logger: io.NewTestHandler(), } - attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) + testFetchParams.Repo = testRepo + testFetchParams.Limit = 30 + attestations, err := c.GetByRepoAndDigest(testFetchParams) require.NoError(t, err) // assert the error path was executed; because this is a paged @@ -375,7 +398,8 @@ func TestGetAttestationsRetries(t *testing.T) { require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") // same test as above, but for GetByOwnerAndDigest: - attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) + testFetchParams.Owner = testOwner + attestations, err = c.GetByOwnerAndDigest(testFetchParams) require.NoError(t, err) // because we haven't reset the mock, we have added 2 more failed requests @@ -401,7 +425,8 @@ func TestGetAttestationsMaxRetries(t *testing.T) { logger: io.NewTestHandler(), } - _, err := c.GetByRepoAndDigest(testRepo, testDigest, "https://slsa.dev/provenance/v1", DefaultLimit) + testFetchParams.Repo = testRepo + _, err := c.GetByRepoAndDigest(testFetchParams) require.Error(t, err) fetcher.AssertNumberOfCalls(t, "OnREST500Error", 4) diff --git a/pkg/cmd/attestation/api/mock_client.go b/pkg/cmd/attestation/api/mock_client.go index 8e6dcdd6f..be2b9b76d 100644 --- a/pkg/cmd/attestation/api/mock_client.go +++ b/pkg/cmd/attestation/api/mock_client.go @@ -7,17 +7,17 @@ import ( ) type MockClient struct { - OnGetByRepoAndDigest func(repo, digest, predicateType string, limit int) ([]*Attestation, error) - OnGetByOwnerAndDigest func(owner, digest, predicateType string, limit int) ([]*Attestation, error) + OnGetByRepoAndDigest func(params FetchParams) ([]*Attestation, error) + OnGetByOwnerAndDigest func(params FetchParams) ([]*Attestation, error) OnGetTrustDomain func() (string, error) } -func (m MockClient) GetByRepoAndDigest(repo, digest, predicateType string, limit int) ([]*Attestation, error) { - return m.OnGetByRepoAndDigest(repo, digest, predicateType, limit) +func (m MockClient) GetByRepoAndDigest(params FetchParams) ([]*Attestation, error) { + return m.OnGetByRepoAndDigest(params) } -func (m MockClient) GetByOwnerAndDigest(owner, digest, predicateType string, limit int) ([]*Attestation, error) { - return m.OnGetByOwnerAndDigest(owner, digest, predicateType, limit) +func (m MockClient) GetByOwnerAndDigest(params FetchParams) ([]*Attestation, error) { + return m.OnGetByOwnerAndDigest(params) } func (m MockClient) GetTrustDomain() (string, error) { @@ -28,23 +28,23 @@ func makeTestAttestation() Attestation { return Attestation{Bundle: data.SigstoreBundle(nil), BundleURL: "https://example.com"} } -func OnGetByRepoAndDigestSuccess(repo, digest, predicateType string, limit int) ([]*Attestation, error) { +func OnGetByRepoAndDigestSuccess(params FetchParams) ([]*Attestation, error) { att1 := makeTestAttestation() att2 := makeTestAttestation() return []*Attestation{&att1, &att2}, nil } -func OnGetByRepoAndDigestFailure(repo, digest, predicateType string, limit int) ([]*Attestation, error) { +func OnGetByRepoAndDigestFailure(params FetchParams) ([]*Attestation, error) { return nil, fmt.Errorf("failed to fetch by repo and digest") } -func OnGetByOwnerAndDigestSuccess(owner, digest, predicateType string, limit int) ([]*Attestation, error) { +func OnGetByOwnerAndDigestSuccess(params FetchParams) ([]*Attestation, error) { att1 := makeTestAttestation() att2 := makeTestAttestation() return []*Attestation{&att1, &att2}, nil } -func OnGetByOwnerAndDigestFailure(owner, digest, predicateType string, limit int) ([]*Attestation, error) { +func OnGetByOwnerAndDigestFailure(params FetchParams) ([]*Attestation, error) { return nil, fmt.Errorf("failed to fetch by owner and digest") } diff --git a/pkg/cmd/attestation/download/download.go b/pkg/cmd/attestation/download/download.go index 6913c0787..65b6f83df 100644 --- a/pkg/cmd/attestation/download/download.go +++ b/pkg/cmd/attestation/download/download.go @@ -127,7 +127,7 @@ func runDownload(opts *Options) error { opts.Logger.VerbosePrintf("Downloading trusted metadata for artifact %s\n\n", opts.ArtifactPath) - params := verification.FetchRemoteAttestationsParams{ + params := api.FetchParams{ Digest: artifact.DigestWithAlg(), Limit: opts.Limit, Owner: opts.Owner, diff --git a/pkg/cmd/attestation/download/download_test.go b/pkg/cmd/attestation/download/download_test.go index 899d15339..629de4a66 100644 --- a/pkg/cmd/attestation/download/download_test.go +++ b/pkg/cmd/attestation/download/download_test.go @@ -275,7 +275,7 @@ func TestRunDownload(t *testing.T) { t.Run("no attestations found", func(t *testing.T) { opts := baseOpts opts.APIClient = api.MockClient{ - OnGetByOwnerAndDigest: func(repo, digest, predicateType string, limit int) ([]*api.Attestation, error) { + OnGetByOwnerAndDigest: func(params api.FetchParams) ([]*api.Attestation, error) { return nil, api.ErrNoAttestationsFound }, } @@ -291,7 +291,7 @@ func TestRunDownload(t *testing.T) { t.Run("failed to fetch attestations", func(t *testing.T) { opts := baseOpts opts.APIClient = api.MockClient{ - OnGetByOwnerAndDigest: func(repo, digest, predicateType string, limit int) ([]*api.Attestation, error) { + OnGetByOwnerAndDigest: func(params api.FetchParams) ([]*api.Attestation, error) { return nil, fmt.Errorf("failed to fetch attestations") }, } diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index 53a53722b..c4d5330f6 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -20,14 +20,6 @@ const SLSAPredicateV1 = "https://slsa.dev/provenance/v1" var ErrUnrecognisedBundleExtension = errors.New("bundle file extension not supported, must be json or jsonl") var ErrEmptyBundleFile = errors.New("provided bundle file is empty") -type FetchRemoteAttestationsParams struct { - Digest string - Limit int - Owner string - PredicateType string - Repo string -} - // GetLocalAttestations returns a slice of attestations read from a local bundle file. func GetLocalAttestations(path string) ([]*api.Attestation, error) { var attestations []*api.Attestation @@ -90,20 +82,20 @@ func loadBundlesFromJSONLinesFile(path string) ([]*api.Attestation, error) { return attestations, nil } -func GetRemoteAttestations(client api.Client, params FetchRemoteAttestationsParams) ([]*api.Attestation, error) { +func GetRemoteAttestations(client api.Client, params api.FetchParams) ([]*api.Attestation, error) { if client == nil { return nil, fmt.Errorf("api client must be provided") } // check if Repo is set first because if Repo has been set, Owner will be set using the value of Repo. // If Repo is not set, the field will remain empty. It will not be populated using the value of Owner. if params.Repo != "" { - attestations, err := client.GetByRepoAndDigest(params.Repo, params.Digest, params.PredicateType, params.Limit) + attestations, err := client.GetByRepoAndDigest(params) if err != nil { return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Repo, err) } return attestations, nil } else if params.Owner != "" { - attestations, err := client.GetByOwnerAndDigest(params.Owner, params.Digest, params.PredicateType, params.Limit) + attestations, err := client.GetByOwnerAndDigest(params) if err != nil { return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Owner, err) } diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index 5774170d9..e8211003c 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -21,7 +21,7 @@ func filterByPredicateType(predicateType string, attestations []*api.Attestation func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestation, string, error) { if o.FetchAttestationsFromGitHubAPI() { - params := verification.FetchRemoteAttestationsParams{ + params := api.FetchParams{ Digest: a.DigestWithAlg(), Limit: o.Limit, Owner: o.Owner, From 5a895b9d72377b7793a1b0084c3ecb6261b15c6e Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 24 Mar 2025 18:12:41 -0600 Subject: [PATCH 05/18] dedpulicate if else logic Signed-off-by: Meredith Lancaster --- .../attestation/verification/attestation.go | 25 ++++---- pkg/cmd/attestation/verify/attestation.go | 58 +++++++++---------- 2 files changed, 41 insertions(+), 42 deletions(-) diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index c4d5330f6..33d8b18b8 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -88,20 +88,23 @@ func GetRemoteAttestations(client api.Client, params api.FetchParams) ([]*api.At } // check if Repo is set first because if Repo has been set, Owner will be set using the value of Repo. // If Repo is not set, the field will remain empty. It will not be populated using the value of Owner. + var attestations []*api.Attestation + var err error + var owner string if params.Repo != "" { - attestations, err := client.GetByRepoAndDigest(params) - if err != nil { - return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Repo, err) - } - return attestations, nil + attestations, err = client.GetByRepoAndDigest(params) + owner = params.Repo } else if params.Owner != "" { - attestations, err := client.GetByOwnerAndDigest(params) - if err != nil { - return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Owner, err) - } - return attestations, nil + attestations, err = client.GetByOwnerAndDigest(params) + owner = params.Owner + } else { + return nil, fmt.Errorf("owner or repo must be provided") } - return nil, fmt.Errorf("owner or repo must be provided") + + if err != nil { + return nil, fmt.Errorf("failed to fetch attestations from %s: %w", owner, err) + } + return attestations, err } func GetOCIAttestations(client oci.Client, artifact artifact.DigestedArtifact) ([]*api.Attestation, error) { diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index e8211003c..f956e82b8 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -37,39 +37,35 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio pluralAttestation := text.Pluralize(len(attestations), "attestation") msg := fmt.Sprintf("Loaded %s from GitHub API", pluralAttestation) return attestations, msg, nil - } else if o.BundlePath != "" { - attestations, err := verification.GetLocalAttestations(o.BundlePath) - if err != nil { - msg := fmt.Sprintf("✗ Loading attestations from %s failed", a.URL) - return nil, msg, err - } - - filtered, errMsg, err := filterByPredicateType(o.PredicateType, attestations) - if err != nil { - return nil, errMsg, err - } - - pluralAttestation := text.Pluralize(len(filtered), "attestation") - msg := fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.BundlePath) - return filtered, msg, nil - } else if o.UseBundleFromRegistry { - attestations, err := verification.GetOCIAttestations(o.OCIClient, a) - if err != nil { - msg := "✗ Loading attestations from OCI registry failed" - return nil, msg, err - } - - filtered, errMsg, err := filterByPredicateType(o.PredicateType, attestations) - if err != nil { - return nil, errMsg, err - } - - pluralAttestation := text.Pluralize(len(filtered), "attestation") - msg := fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.ArtifactPath) - return filtered, msg, nil } - return nil, "", fmt.Errorf("no valid attestation source provided") + var attestations []*api.Attestation + var err error + var errMsg string + if o.BundlePath != "" { + attestations, err = verification.GetLocalAttestations(o.BundlePath) + if err != nil { + errMsg = fmt.Sprintf("✗ Loading attestations from %s failed", a.URL) + } + } else if o.UseBundleFromRegistry { + attestations, err = verification.GetOCIAttestations(o.OCIClient, a) + if err != nil { + errMsg = "✗ Loading attestations from OCI registry failed" + } + } + + if err != nil { + return nil, errMsg, err + } + + filtered, errMsg, err := filterByPredicateType(o.PredicateType, attestations) + if err != nil { + return nil, errMsg, err + } + + pluralAttestation := text.Pluralize(len(filtered), "attestation") + msg := fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.BundlePath) + return filtered, msg, nil } func verifyAttestations(art artifact.DigestedArtifact, att []*api.Attestation, sgVerifier verification.SigstoreVerifier, ec verification.EnforcementCriteria) ([]*verification.AttestationProcessingResult, string, error) { From a9cc7b481e2ab718163702cfe57565fe83370e19 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 24 Mar 2025 18:28:27 -0600 Subject: [PATCH 06/18] create single fetch by digest client method Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 35 ++++++++++----- pkg/cmd/attestation/api/client_test.go | 32 +++++++------- pkg/cmd/attestation/api/mock_client.go | 44 +++++++------------ pkg/cmd/attestation/download/download.go | 2 +- pkg/cmd/attestation/download/download_test.go | 4 +- .../attestation/verification/attestation.go | 25 ----------- pkg/cmd/attestation/verify/attestation.go | 2 +- 7 files changed, 58 insertions(+), 86 deletions(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index ae33d9ce3..a3e627852 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -48,8 +48,7 @@ type httpClient interface { } type Client interface { - GetByRepoAndDigest(params FetchParams) ([]*Attestation, error) - GetByOwnerAndDigest(params FetchParams) ([]*Attestation, error) + GetByDigest(params FetchParams) ([]*Attestation, error) GetTrustDomain() (string, error) } @@ -69,16 +68,28 @@ func NewLiveClient(hc *http.Client, host string, l *ioconfig.Handler) *LiveClien } } -// GetByRepoAndDigest fetches the attestation by repo and digest -func (c *LiveClient) GetByRepoAndDigest(params FetchParams) ([]*Attestation, error) { - url := fmt.Sprintf(GetAttestationByRepoAndSubjectDigestPath, params.Repo, params.Digest) - return c.getByURL(url, params) -} - -// GetByOwnerAndDigest fetches attestation by owner and digest -func (c *LiveClient) GetByOwnerAndDigest(params FetchParams) ([]*Attestation, error) { - url := fmt.Sprintf(GetAttestationByOwnerAndSubjectDigestPath, params.Owner, params.Digest) - return c.getByURL(url, params) +// GetByDigest fetches the attestation by digest and either owner or repo +// depending on which is provided +func (c *LiveClient) GetByDigest(params FetchParams) ([]*Attestation, error) { + if params.Repo == "" && params.Owner == "" { + return nil, fmt.Errorf("owner or repo must be provided") + } else if params.Repo != "" { + // check if Repo is set first because if Repo has been set, Owner will be set using the value of Repo. + // If Repo is not set, the field will remain empty. It will not be populated using the value of Owner. + url := fmt.Sprintf(GetAttestationByRepoAndSubjectDigestPath, params.Repo, params.Digest) + attestations, err := c.getByURL(url, params) + if err != nil { + return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Repo, err) + } + return attestations, nil + } else { + url := fmt.Sprintf(GetAttestationByOwnerAndSubjectDigestPath, params.Owner, params.Digest) + attestations, err := c.getByURL(url, params) + if err != nil { + return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Owner, err) + } + return attestations, nil + } } func (c *LiveClient) getByURL(url string, params FetchParams) ([]*Attestation, error) { diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index f77b39d08..fb2e36b4d 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -51,7 +51,7 @@ var testFetchParams = FetchParams{ func TestGetByDigest(t *testing.T) { c := NewClientWithMockGHClient(false) testFetchParams.Repo = testRepo - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, 5, len(attestations)) @@ -59,7 +59,7 @@ func TestGetByDigest(t *testing.T) { require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, 5, len(attestations)) @@ -74,7 +74,7 @@ func TestGetByDigestGreaterThanLimit(t *testing.T) { // The method should return five results when the limit is not set testFetchParams.Limit = limit testFetchParams.Repo = testRepo - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, 3, len(attestations)) @@ -82,7 +82,7 @@ func TestGetByDigestGreaterThanLimit(t *testing.T) { require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), limit) @@ -94,7 +94,7 @@ func TestGetByDigestWithNextPage(t *testing.T) { c := NewClientWithMockGHClient(true) testFetchParams.Repo = testRepo testFetchParams.Limit = 30 - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), 10) @@ -102,7 +102,7 @@ func TestGetByDigestWithNextPage(t *testing.T) { require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), 10) @@ -117,7 +117,7 @@ func TestGetByDigestGreaterThanLimitWithNextPage(t *testing.T) { // The method should return five results when the limit is not set testFetchParams.Limit = limit testFetchParams.Repo = testRepo - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), limit) @@ -125,7 +125,7 @@ func TestGetByDigestGreaterThanLimitWithNextPage(t *testing.T) { require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), limit) @@ -148,13 +148,13 @@ func TestGetByDigest_NoAttestationsFound(t *testing.T) { } testFetchParams.Repo = testRepo - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.Error(t, err) require.IsType(t, ErrNoAttestationsFound, err) require.Nil(t, attestations) testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.Error(t, err) require.IsType(t, ErrNoAttestationsFound, err) require.Nil(t, attestations) @@ -173,12 +173,12 @@ func TestGetByDigest_Error(t *testing.T) { } testFetchParams.Repo = testRepo - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.Error(t, err) require.Nil(t, attestations) testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.Error(t, err) require.Nil(t, attestations) } @@ -385,7 +385,7 @@ func TestGetAttestationsRetries(t *testing.T) { testFetchParams.Repo = testRepo testFetchParams.Limit = 30 - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.NoError(t, err) // assert the error path was executed; because this is a paged @@ -397,9 +397,9 @@ func TestGetAttestationsRetries(t *testing.T) { bundle := (attestations)[0].Bundle require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - // same test as above, but for GetByOwnerAndDigest: + // same test as above, but for GetByDigest: testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.NoError(t, err) // because we haven't reset the mock, we have added 2 more failed requests @@ -426,7 +426,7 @@ func TestGetAttestationsMaxRetries(t *testing.T) { } testFetchParams.Repo = testRepo - _, err := c.GetByRepoAndDigest(testFetchParams) + _, err := c.GetByDigest(testFetchParams) require.Error(t, err) fetcher.AssertNumberOfCalls(t, "OnREST500Error", 4) diff --git a/pkg/cmd/attestation/api/mock_client.go b/pkg/cmd/attestation/api/mock_client.go index be2b9b76d..efcedc8b5 100644 --- a/pkg/cmd/attestation/api/mock_client.go +++ b/pkg/cmd/attestation/api/mock_client.go @@ -6,58 +6,44 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/test/data" ) +func makeTestAttestation() Attestation { + return Attestation{Bundle: data.SigstoreBundle(nil), BundleURL: "https://example.com"} +} + type MockClient struct { - OnGetByRepoAndDigest func(params FetchParams) ([]*Attestation, error) - OnGetByOwnerAndDigest func(params FetchParams) ([]*Attestation, error) - OnGetTrustDomain func() (string, error) + OnGetByDigest func(params FetchParams) ([]*Attestation, error) + OnGetTrustDomain func() (string, error) } -func (m MockClient) GetByRepoAndDigest(params FetchParams) ([]*Attestation, error) { - return m.OnGetByRepoAndDigest(params) -} - -func (m MockClient) GetByOwnerAndDigest(params FetchParams) ([]*Attestation, error) { - return m.OnGetByOwnerAndDigest(params) +func (m MockClient) GetByDigest(params FetchParams) ([]*Attestation, error) { + return m.OnGetByDigest(params) } func (m MockClient) GetTrustDomain() (string, error) { return m.OnGetTrustDomain() } -func makeTestAttestation() Attestation { - return Attestation{Bundle: data.SigstoreBundle(nil), BundleURL: "https://example.com"} -} - -func OnGetByRepoAndDigestSuccess(params FetchParams) ([]*Attestation, error) { +func OnGetByDigestSuccess(params FetchParams) ([]*Attestation, error) { att1 := makeTestAttestation() att2 := makeTestAttestation() return []*Attestation{&att1, &att2}, nil } -func OnGetByRepoAndDigestFailure(params FetchParams) ([]*Attestation, error) { - return nil, fmt.Errorf("failed to fetch by repo and digest") -} - -func OnGetByOwnerAndDigestSuccess(params FetchParams) ([]*Attestation, error) { - att1 := makeTestAttestation() - att2 := makeTestAttestation() - return []*Attestation{&att1, &att2}, nil -} - -func OnGetByOwnerAndDigestFailure(params FetchParams) ([]*Attestation, error) { +func OnGetByDigestFailure(params FetchParams) ([]*Attestation, error) { + if params.Repo != "" { + return nil, fmt.Errorf("failed to fetch by repo and digest") + } return nil, fmt.Errorf("failed to fetch by owner and digest") } func NewTestClient() *MockClient { return &MockClient{ - OnGetByRepoAndDigest: OnGetByRepoAndDigestSuccess, - OnGetByOwnerAndDigest: OnGetByOwnerAndDigestSuccess, + OnGetByDigest: OnGetByDigestSuccess, } } func NewFailTestClient() *MockClient { return &MockClient{ - OnGetByRepoAndDigest: OnGetByRepoAndDigestFailure, - OnGetByOwnerAndDigest: OnGetByOwnerAndDigestFailure, + OnGetByDigest: OnGetByDigestFailure, } } diff --git a/pkg/cmd/attestation/download/download.go b/pkg/cmd/attestation/download/download.go index 65b6f83df..2a297bc9a 100644 --- a/pkg/cmd/attestation/download/download.go +++ b/pkg/cmd/attestation/download/download.go @@ -133,7 +133,7 @@ func runDownload(opts *Options) error { Owner: opts.Owner, Repo: opts.Repo, } - attestations, err := verification.GetRemoteAttestations(opts.APIClient, params) + attestations, err := opts.APIClient.GetByDigest(params) if err != nil { if errors.Is(err, api.ErrNoAttestationsFound) { fmt.Fprintf(opts.Logger.IO.Out, "No attestations found for %s\n", opts.ArtifactPath) diff --git a/pkg/cmd/attestation/download/download_test.go b/pkg/cmd/attestation/download/download_test.go index 629de4a66..11872daf9 100644 --- a/pkg/cmd/attestation/download/download_test.go +++ b/pkg/cmd/attestation/download/download_test.go @@ -275,7 +275,7 @@ func TestRunDownload(t *testing.T) { t.Run("no attestations found", func(t *testing.T) { opts := baseOpts opts.APIClient = api.MockClient{ - OnGetByOwnerAndDigest: func(params api.FetchParams) ([]*api.Attestation, error) { + OnGetByDigest: func(params api.FetchParams) ([]*api.Attestation, error) { return nil, api.ErrNoAttestationsFound }, } @@ -291,7 +291,7 @@ func TestRunDownload(t *testing.T) { t.Run("failed to fetch attestations", func(t *testing.T) { opts := baseOpts opts.APIClient = api.MockClient{ - OnGetByOwnerAndDigest: func(params api.FetchParams) ([]*api.Attestation, error) { + OnGetByDigest: func(params api.FetchParams) ([]*api.Attestation, error) { return nil, fmt.Errorf("failed to fetch attestations") }, } diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index 33d8b18b8..9757b72c5 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -82,31 +82,6 @@ func loadBundlesFromJSONLinesFile(path string) ([]*api.Attestation, error) { return attestations, nil } -func GetRemoteAttestations(client api.Client, params api.FetchParams) ([]*api.Attestation, error) { - if client == nil { - return nil, fmt.Errorf("api client must be provided") - } - // check if Repo is set first because if Repo has been set, Owner will be set using the value of Repo. - // If Repo is not set, the field will remain empty. It will not be populated using the value of Owner. - var attestations []*api.Attestation - var err error - var owner string - if params.Repo != "" { - attestations, err = client.GetByRepoAndDigest(params) - owner = params.Repo - } else if params.Owner != "" { - attestations, err = client.GetByOwnerAndDigest(params) - owner = params.Owner - } else { - return nil, fmt.Errorf("owner or repo must be provided") - } - - if err != nil { - return nil, fmt.Errorf("failed to fetch attestations from %s: %w", owner, err) - } - return attestations, err -} - func GetOCIAttestations(client oci.Client, artifact artifact.DigestedArtifact) ([]*api.Attestation, error) { attestations, err := client.GetAttestations(artifact.NameRef(), artifact.DigestWithAlg()) if err != nil { diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index f956e82b8..f91526bbd 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -29,7 +29,7 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio Repo: o.Repo, } - attestations, err := verification.GetRemoteAttestations(o.APIClient, params) + attestations, err := o.APIClient.GetByDigest(params) if err != nil { msg := "✗ Loading attestations from GitHub API failed" return nil, msg, err From a856a796f0e0ed801155933f4ff7daf07cef0604 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 24 Mar 2025 18:34:54 -0600 Subject: [PATCH 07/18] remove duplicate predicate filtering code Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/download/download.go | 7 +++---- pkg/cmd/attestation/verification/attestation.go | 8 ++++++-- .../attestation/verification/attestation_test.go | 7 ++++--- pkg/cmd/attestation/verify/attestation.go | 14 ++------------ 4 files changed, 15 insertions(+), 21 deletions(-) diff --git a/pkg/cmd/attestation/download/download.go b/pkg/cmd/attestation/download/download.go index 2a297bc9a..86cf08d72 100644 --- a/pkg/cmd/attestation/download/download.go +++ b/pkg/cmd/attestation/download/download.go @@ -144,10 +144,9 @@ func runDownload(opts *Options) error { // Apply predicate type filter to returned attestations if opts.PredicateType != "" { - filteredAttestations := verification.FilterAttestations(opts.PredicateType, attestations) - - if len(filteredAttestations) == 0 { - return fmt.Errorf("no attestations found with predicate type: %s", opts.PredicateType) + filteredAttestations, err := verification.FilterAttestations(opts.PredicateType, attestations) + if err != nil { + return fmt.Errorf("failed to filter attestations: %v", err) } attestations = filteredAttestations diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index 9757b72c5..ba357a5cc 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -97,7 +97,7 @@ type IntotoStatement struct { PredicateType string `json:"predicateType"` } -func FilterAttestations(predicateType string, attestations []*api.Attestation) []*api.Attestation { +func FilterAttestations(predicateType string, attestations []*api.Attestation) ([]*api.Attestation, error) { filteredAttestations := []*api.Attestation{} for _, each := range attestations { @@ -118,5 +118,9 @@ func FilterAttestations(predicateType string, attestations []*api.Attestation) [ } } - return filteredAttestations + if len(filteredAttestations) == 0 { + return nil, fmt.Errorf("no attestations found with predicate type: %s", predicateType) + } + + return filteredAttestations, nil } diff --git a/pkg/cmd/attestation/verification/attestation_test.go b/pkg/cmd/attestation/verification/attestation_test.go index 8acff0c37..55a447cf4 100644 --- a/pkg/cmd/attestation/verification/attestation_test.go +++ b/pkg/cmd/attestation/verification/attestation_test.go @@ -157,10 +157,11 @@ func TestFilterAttestations(t *testing.T) { }, } - filtered := FilterAttestations("https://slsa.dev/provenance/v1", attestations) - + filtered, err := FilterAttestations("https://slsa.dev/provenance/v1", attestations) require.Len(t, filtered, 1) + require.NoError(t, err) - filtered = FilterAttestations("NonExistentPredicate", attestations) + filtered, err = FilterAttestations("NonExistentPredicate", attestations) require.Len(t, filtered, 0) + require.NoError(t, err) } diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index f91526bbd..c09b433b0 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -9,16 +9,6 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/verification" ) -func filterByPredicateType(predicateType string, attestations []*api.Attestation) ([]*api.Attestation, string, error) { - // Apply predicate type filter to returned attestations - filteredAttestations := verification.FilterAttestations(predicateType, attestations) - if len(filteredAttestations) == 0 { - msg := fmt.Sprintf("✗ No attestations found with predicate type: %s\n", predicateType) - return nil, msg, fmt.Errorf("no matching predicate found") - } - return filteredAttestations, "", nil -} - func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestation, string, error) { if o.FetchAttestationsFromGitHubAPI() { params := api.FetchParams{ @@ -58,9 +48,9 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio return nil, errMsg, err } - filtered, errMsg, err := filterByPredicateType(o.PredicateType, attestations) + filtered, err := verification.FilterAttestations(o.PredicateType, attestations) if err != nil { - return nil, errMsg, err + return nil, err.Error(), err } pluralAttestation := text.Pluralize(len(filtered), "attestation") From 0d0654738b7bf1cc1f200dd086c284d0a25f5be0 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 24 Mar 2025 18:58:35 -0600 Subject: [PATCH 08/18] simplify client methods Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 66 +++++++++++++++++-------------- 1 file changed, 37 insertions(+), 29 deletions(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index a3e627852..d0cffcb27 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -36,6 +36,19 @@ type FetchParams struct { Repo string } +func (p *FetchParams) Validate() error { + if p.Digest == "" { + return fmt.Errorf("digest must be provided") + } + if p.Limit <= 0 || p.Limit > maxLimitForFlag { + return fmt.Errorf("limit must be greater than 0 and less than or equal to %d", maxLimitForFlag) + } + if p.Repo == "" && p.Owner == "" { + return fmt.Errorf("owner or repo must be provided") + } + return nil +} + // githubApiClient makes REST calls to the GitHub API type githubApiClient interface { REST(hostname, method, p string, body io.Reader, data interface{}) error @@ -71,30 +84,8 @@ func NewLiveClient(hc *http.Client, host string, l *ioconfig.Handler) *LiveClien // GetByDigest fetches the attestation by digest and either owner or repo // depending on which is provided func (c *LiveClient) GetByDigest(params FetchParams) ([]*Attestation, error) { - if params.Repo == "" && params.Owner == "" { - return nil, fmt.Errorf("owner or repo must be provided") - } else if params.Repo != "" { - // check if Repo is set first because if Repo has been set, Owner will be set using the value of Repo. - // If Repo is not set, the field will remain empty. It will not be populated using the value of Owner. - url := fmt.Sprintf(GetAttestationByRepoAndSubjectDigestPath, params.Repo, params.Digest) - attestations, err := c.getByURL(url, params) - if err != nil { - return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Repo, err) - } - return attestations, nil - } else { - url := fmt.Sprintf(GetAttestationByOwnerAndSubjectDigestPath, params.Owner, params.Digest) - attestations, err := c.getByURL(url, params) - if err != nil { - return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Owner, err) - } - return attestations, nil - } -} - -func (c *LiveClient) getByURL(url string, params FetchParams) ([]*Attestation, error) { c.logger.VerbosePrintf("Fetching attestations for artifact digest %s\n\n", params.Digest) - attestations, err := c.getAttestations(url, params) + attestations, err := c.getAttestations(params) if err != nil { return nil, err } @@ -107,13 +98,24 @@ func (c *LiveClient) getByURL(url string, params FetchParams) ([]*Attestation, e return bundles, nil } -// GetTrustDomain returns the current trust domain. If the default is used -// the empty string is returned -func (c *LiveClient) GetTrustDomain() (string, error) { - return c.getTrustDomain(MetaPath) -} +func (c *LiveClient) getAttestations(params FetchParams) ([]*Attestation, error) { + if err := params.Validate(); err != nil { + return nil, err + } + + var urlTemplate string + var resourceOwner string + if params.Repo != "" { + // check if Repo is set first because if Repo has been set, Owner will be set using the value of Repo. + // If Repo is not set, the field will remain empty. It will not be populated using the value of Owner. + urlTemplate = GetAttestationByRepoAndSubjectDigestPath + resourceOwner = params.Repo + } else { + urlTemplate = GetAttestationByOwnerAndSubjectDigestPath + resourceOwner = params.Owner + } + url := fmt.Sprintf(urlTemplate, resourceOwner, params.Digest) -func (c *LiveClient) getAttestations(url string, params FetchParams) ([]*Attestation, error) { perPage := params.Limit if perPage <= 0 || perPage > maxLimitForFlag { return nil, fmt.Errorf("limit must be greater than 0 and less than or equal to %d", maxLimitForFlag) @@ -263,6 +265,12 @@ func shouldRetry(err error) bool { return false } +// GetTrustDomain returns the current trust domain. If the default is used +// the empty string is returned +func (c *LiveClient) GetTrustDomain() (string, error) { + return c.getTrustDomain(MetaPath) +} + func (c *LiveClient) getTrustDomain(url string) (string, error) { var resp MetaResponse From baeaf66011464ea49954dcf0ec25574b8fa7070c Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 24 Mar 2025 19:13:27 -0600 Subject: [PATCH 09/18] restructure api client methods Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index d0cffcb27..61d0bee52 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -98,29 +98,21 @@ func (c *LiveClient) GetByDigest(params FetchParams) ([]*Attestation, error) { return bundles, nil } -func (c *LiveClient) getAttestations(params FetchParams) ([]*Attestation, error) { +func (c *LiveClient) buildRequestURL(params FetchParams) (string, error) { if err := params.Validate(); err != nil { - return nil, err + return "", err } - var urlTemplate string - var resourceOwner string + var url string if params.Repo != "" { // check if Repo is set first because if Repo has been set, Owner will be set using the value of Repo. // If Repo is not set, the field will remain empty. It will not be populated using the value of Owner. - urlTemplate = GetAttestationByRepoAndSubjectDigestPath - resourceOwner = params.Repo + url = fmt.Sprintf(GetAttestationByRepoAndSubjectDigestPath, params.Repo, params.Digest) } else { - urlTemplate = GetAttestationByOwnerAndSubjectDigestPath - resourceOwner = params.Owner + url = fmt.Sprintf(GetAttestationByOwnerAndSubjectDigestPath, params.Owner, params.Digest) } - url := fmt.Sprintf(urlTemplate, resourceOwner, params.Digest) perPage := params.Limit - if perPage <= 0 || perPage > maxLimitForFlag { - return nil, fmt.Errorf("limit must be greater than 0 and less than or equal to %d", maxLimitForFlag) - } - if perPage > maxLimitForFetch { perPage = maxLimitForFetch } @@ -130,6 +122,14 @@ func (c *LiveClient) getAttestations(params FetchParams) ([]*Attestation, error) if params.PredicateType != "" { url = fmt.Sprintf("%s&predicate_type=%s", url, params.PredicateType) } + return url, nil +} + +func (c *LiveClient) getAttestations(params FetchParams) ([]*Attestation, error) { + url, err := c.buildRequestURL(params) + if err != nil { + return nil, err + } var attestations []*Attestation var resp AttestationsResponse @@ -139,13 +139,11 @@ func (c *LiveClient) getAttestations(params FetchParams) ([]*Attestation, error) for url != "" && len(attestations) < params.Limit { err := backoff.Retry(func() error { newURL, restErr := c.githubAPI.RESTWithNext(c.host, http.MethodGet, url, nil, &resp) - if restErr != nil { if shouldRetry(restErr) { return restErr - } else { - return backoff.Permanent(restErr) } + return backoff.Permanent(restErr) } url = newURL From d1c4bf7dd9f02a50c851c74df7dd9ae8cb759609 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Tue, 25 Mar 2025 08:24:52 -0600 Subject: [PATCH 10/18] comment Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/attestation.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index c09b433b0..6dd855bbc 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -10,6 +10,8 @@ import ( ) func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestation, string, error) { + // Fetch attestations from GitHub API within this if block since predicate type + // filter is done when the API is called if o.FetchAttestationsFromGitHubAPI() { params := api.FetchParams{ Digest: a.DigestWithAlg(), @@ -29,6 +31,8 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio return attestations, msg, nil } + // Fetch attestations from local bundle or OCI registry + // Predicate type filtering is done after the attestations are fetched var attestations []*api.Attestation var err error var errMsg string From e3fbe9008f8c0caf91b88ecbb3581bc96f7484ba Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Tue, 25 Mar 2025 08:25:00 -0600 Subject: [PATCH 11/18] reduce test duplication Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client_test.go | 186 ++++++++++--------------- 1 file changed, 73 insertions(+), 113 deletions(-) diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index fb2e36b4d..0b7adfcf4 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -42,95 +42,82 @@ func NewClientWithMockGHClient(hasNextPage bool) Client { } } -var testFetchParams = FetchParams{ +var testFetchParamsWithOwner = FetchParams{ Digest: testDigest, Limit: DefaultLimit, + Owner: testOwner, + PredicateType: "https://slsa.dev/provenance/v1", +} +var testFetchParamsWithRepo = FetchParams{ + Digest: testDigest, + Limit: DefaultLimit, + Repo: testRepo, PredicateType: "https://slsa.dev/provenance/v1", } +type getByTestCase struct { + name string + params FetchParams + limit int + expectedAttestations int + expectedError error + hasNextPage bool +} + +var getByTestCases = []getByTestCase{ + { + name: "get by digest with owner", + params: testFetchParamsWithOwner, + expectedAttestations: 5, + expectedError: nil, + }, + { + name: "get by digest with repo", + params: testFetchParamsWithRepo, + expectedAttestations: 5, + expectedError: nil, + }, + { + name: "get by digest with attestations greater than limit", + params: testFetchParamsWithRepo, + limit: 3, + expectedAttestations: 3, + expectedError: nil, + }, + { + name: "get by digest with next page", + params: testFetchParamsWithRepo, + limit: 30, + expectedAttestations: 10, + expectedError: nil, + hasNextPage: true, + }, + { + name: "greater than limit with next page", + params: testFetchParamsWithRepo, + limit: 7, + expectedAttestations: 7, + expectedError: nil, + hasNextPage: true, + }, +} + func TestGetByDigest(t *testing.T) { - c := NewClientWithMockGHClient(false) - testFetchParams.Repo = testRepo - attestations, err := c.GetByDigest(testFetchParams) - require.NoError(t, err) + for _, tc := range getByTestCases { + t.Run(tc.name, func(t *testing.T) { + c := NewClientWithMockGHClient(tc.hasNextPage) - require.Equal(t, 5, len(attestations)) - bundle := (attestations)[0].Bundle - require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") + if tc.limit > 0 { + tc.params.Limit = tc.limit + } + attestations, err := c.GetByDigest(tc.params) + require.NoError(t, err) - testFetchParams.Owner = testOwner - attestations, err = c.GetByDigest(testFetchParams) - require.NoError(t, err) - - require.Equal(t, 5, len(attestations)) - bundle = (attestations)[0].Bundle - require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") -} - -func TestGetByDigestGreaterThanLimit(t *testing.T) { - c := NewClientWithMockGHClient(false) - - limit := 3 - // The method should return five results when the limit is not set - testFetchParams.Limit = limit - testFetchParams.Repo = testRepo - attestations, err := c.GetByDigest(testFetchParams) - require.NoError(t, err) - - require.Equal(t, 3, len(attestations)) - bundle := (attestations)[0].Bundle - require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - - testFetchParams.Owner = testOwner - attestations, err = c.GetByDigest(testFetchParams) - require.NoError(t, err) - - require.Equal(t, len(attestations), limit) - bundle = (attestations)[0].Bundle - require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") -} - -func TestGetByDigestWithNextPage(t *testing.T) { - c := NewClientWithMockGHClient(true) - testFetchParams.Repo = testRepo - testFetchParams.Limit = 30 - attestations, err := c.GetByDigest(testFetchParams) - require.NoError(t, err) - - require.Equal(t, len(attestations), 10) - bundle := (attestations)[0].Bundle - require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - - testFetchParams.Owner = testOwner - attestations, err = c.GetByDigest(testFetchParams) - require.NoError(t, err) - - require.Equal(t, len(attestations), 10) - bundle = (attestations)[0].Bundle - require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") -} - -func TestGetByDigestGreaterThanLimitWithNextPage(t *testing.T) { - c := NewClientWithMockGHClient(true) - - limit := 7 - // The method should return five results when the limit is not set - testFetchParams.Limit = limit - testFetchParams.Repo = testRepo - attestations, err := c.GetByDigest(testFetchParams) - require.NoError(t, err) - - require.Equal(t, len(attestations), limit) - bundle := (attestations)[0].Bundle - require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - - testFetchParams.Owner = testOwner - attestations, err = c.GetByDigest(testFetchParams) - require.NoError(t, err) - - require.Equal(t, len(attestations), limit) - bundle = (attestations)[0].Bundle - require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") + require.Equal(t, tc.expectedAttestations, len(attestations)) + bundle := (attestations)[0].Bundle + require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") + }) + } } func TestGetByDigest_NoAttestationsFound(t *testing.T) { @@ -147,14 +134,7 @@ func TestGetByDigest_NoAttestationsFound(t *testing.T) { logger: io.NewTestHandler(), } - testFetchParams.Repo = testRepo - attestations, err := c.GetByDigest(testFetchParams) - require.Error(t, err) - require.IsType(t, ErrNoAttestationsFound, err) - require.Nil(t, attestations) - - testFetchParams.Owner = testOwner - attestations, err = c.GetByDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParamsWithRepo) require.Error(t, err) require.IsType(t, ErrNoAttestationsFound, err) require.Nil(t, attestations) @@ -172,13 +152,7 @@ func TestGetByDigest_Error(t *testing.T) { logger: io.NewTestHandler(), } - testFetchParams.Repo = testRepo - attestations, err := c.GetByDigest(testFetchParams) - require.Error(t, err) - require.Nil(t, attestations) - - testFetchParams.Owner = testOwner - attestations, err = c.GetByDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParamsWithRepo) require.Error(t, err) require.Nil(t, attestations) } @@ -383,9 +357,8 @@ func TestGetAttestationsRetries(t *testing.T) { logger: io.NewTestHandler(), } - testFetchParams.Repo = testRepo - testFetchParams.Limit = 30 - attestations, err := c.GetByDigest(testFetchParams) + testFetchParamsWithRepo.Limit = 30 + attestations, err := c.GetByDigest(testFetchParamsWithRepo) require.NoError(t, err) // assert the error path was executed; because this is a paged @@ -396,18 +369,6 @@ func TestGetAttestationsRetries(t *testing.T) { require.Equal(t, len(attestations), 10) bundle := (attestations)[0].Bundle require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - - // same test as above, but for GetByDigest: - testFetchParams.Owner = testOwner - attestations, err = c.GetByDigest(testFetchParams) - require.NoError(t, err) - - // because we haven't reset the mock, we have added 2 more failed requests - fetcher.AssertNumberOfCalls(t, "FlakyOnRESTSuccessWithNextPage:error", 4) - - require.Equal(t, len(attestations), 10) - bundle = (attestations)[0].Bundle - require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") } // test total retries @@ -425,8 +386,7 @@ func TestGetAttestationsMaxRetries(t *testing.T) { logger: io.NewTestHandler(), } - testFetchParams.Repo = testRepo - _, err := c.GetByDigest(testFetchParams) + _, err := c.GetByDigest(testFetchParamsWithRepo) require.Error(t, err) fetcher.AssertNumberOfCalls(t, "OnREST500Error", 4) From 166e211e2bab796bd72594350f35956760ce14f3 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Tue, 25 Mar 2025 08:28:33 -0600 Subject: [PATCH 12/18] clean up test fixtures Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client_test.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index 0b7adfcf4..384c7c9c8 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -60,7 +60,6 @@ type getByTestCase struct { params FetchParams limit int expectedAttestations int - expectedError error hasNextPage bool } @@ -69,27 +68,22 @@ var getByTestCases = []getByTestCase{ name: "get by digest with owner", params: testFetchParamsWithOwner, expectedAttestations: 5, - expectedError: nil, }, { name: "get by digest with repo", params: testFetchParamsWithRepo, expectedAttestations: 5, - expectedError: nil, }, { name: "get by digest with attestations greater than limit", params: testFetchParamsWithRepo, limit: 3, expectedAttestations: 3, - expectedError: nil, }, { name: "get by digest with next page", params: testFetchParamsWithRepo, - limit: 30, expectedAttestations: 10, - expectedError: nil, hasNextPage: true, }, { @@ -97,7 +91,6 @@ var getByTestCases = []getByTestCase{ params: testFetchParamsWithRepo, limit: 7, expectedAttestations: 7, - expectedError: nil, hasNextPage: true, }, } From 05d9156a992b7df3b5d7fd85020f8da7d2bea863 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Tue, 1 Apr 2025 11:16:00 -0600 Subject: [PATCH 13/18] add check for nil api client Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/download/download.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/cmd/attestation/download/download.go b/pkg/cmd/attestation/download/download.go index 86cf08d72..2cb648414 100644 --- a/pkg/cmd/attestation/download/download.go +++ b/pkg/cmd/attestation/download/download.go @@ -127,6 +127,9 @@ func runDownload(opts *Options) error { opts.Logger.VerbosePrintf("Downloading trusted metadata for artifact %s\n\n", opts.ArtifactPath) + if opts.APIClient == nil { + return fmt.Errorf("no APIClient provided") + } params := api.FetchParams{ Digest: artifact.DigestWithAlg(), Limit: opts.Limit, From 13dafefcb5ddb5eaa1e634b51338256f7ac588ee Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Tue, 1 Apr 2025 11:23:25 -0600 Subject: [PATCH 14/18] add missing nil struct checks and udpate error messages Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/mock_client.go | 4 ++-- pkg/cmd/attestation/verification/attestation_test.go | 4 ++-- pkg/cmd/attestation/verify/attestation.go | 6 ++++++ pkg/cmd/attestation/verify/verify_test.go | 2 +- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/attestation/api/mock_client.go b/pkg/cmd/attestation/api/mock_client.go index efcedc8b5..fbf44bbb9 100644 --- a/pkg/cmd/attestation/api/mock_client.go +++ b/pkg/cmd/attestation/api/mock_client.go @@ -31,9 +31,9 @@ func OnGetByDigestSuccess(params FetchParams) ([]*Attestation, error) { func OnGetByDigestFailure(params FetchParams) ([]*Attestation, error) { if params.Repo != "" { - return nil, fmt.Errorf("failed to fetch by repo and digest") + return nil, fmt.Errorf("failed to fetch attestations from %s", params.Repo) } - return nil, fmt.Errorf("failed to fetch by owner and digest") + return nil, fmt.Errorf("failed to fetch attestations from %s", params.Owner) } func NewTestClient() *MockClient { diff --git a/pkg/cmd/attestation/verification/attestation_test.go b/pkg/cmd/attestation/verification/attestation_test.go index 55a447cf4..18e2c6cca 100644 --- a/pkg/cmd/attestation/verification/attestation_test.go +++ b/pkg/cmd/attestation/verification/attestation_test.go @@ -162,6 +162,6 @@ func TestFilterAttestations(t *testing.T) { require.NoError(t, err) filtered, err = FilterAttestations("NonExistentPredicate", attestations) - require.Len(t, filtered, 0) - require.NoError(t, err) + require.Nil(t, filtered) + require.Error(t, err) } diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index 6dd855bbc..2a935a56c 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -1,6 +1,7 @@ package verify import ( + "errors" "fmt" "github.com/cli/cli/v2/internal/text" @@ -13,6 +14,11 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio // Fetch attestations from GitHub API within this if block since predicate type // filter is done when the API is called if o.FetchAttestationsFromGitHubAPI() { + if o.APIClient == nil { + errMsg := "✗ No APIClient provided" + return nil, errMsg, errors.New(errMsg) + } + params := api.FetchParams{ Digest: a.DigestWithAlg(), Limit: o.Limit, diff --git a/pkg/cmd/attestation/verify/verify_test.go b/pkg/cmd/attestation/verify/verify_test.go index 092a009d8..2b821a435 100644 --- a/pkg/cmd/attestation/verify/verify_test.go +++ b/pkg/cmd/attestation/verify/verify_test.go @@ -510,7 +510,7 @@ func TestRunVerify(t *testing.T) { err := runVerify(&customOpts) require.Error(t, err) - require.ErrorContains(t, err, "no matching predicate found") + require.ErrorContains(t, err, "no attestations found with predicate type") }) t.Run("with valid OCI artifact with UseBundleFromRegistry flag but no bundle return from registry", func(t *testing.T) { From f43ec0079bab00cbc2065da7468c6867511818c9 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Tue, 1 Apr 2025 11:52:13 -0600 Subject: [PATCH 15/18] add test for predicate type filtering Signed-off-by: Meredith Lancaster --- .../verify/verify-with-internal-github-sigstore.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/integration/attestation-cmd/verify/verify-with-internal-github-sigstore.sh b/test/integration/attestation-cmd/verify/verify-with-internal-github-sigstore.sh index 647a13a4c..cea3c7228 100644 --- a/test/integration/attestation-cmd/verify/verify-with-internal-github-sigstore.sh +++ b/test/integration/attestation-cmd/verify/verify-with-internal-github-sigstore.sh @@ -14,3 +14,9 @@ if ! $ghBuildPath attestation verify "$ghCLIArtifact" --digest-alg=sha256 --owne echo "Failed to verify" exit 1 fi + +# Try to verify when specifying a predicate type that does not match the attestation +if $ghBuildPath attestation verify "$ghCLIArtifact" --digest-alg=sha256 --owner=cli --predicate-type=my-custom-predicate-type; then + echo "Verification should have failed" + exit 1 +fi From 56d924d25b39bb30497ba26c268d71428d794880 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Tue, 1 Apr 2025 12:58:37 -0600 Subject: [PATCH 16/18] getAttestations unit tests Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/attestation.go | 18 +-- .../verify/attestation_integration_test.go | 119 ------------------ .../attestation/verify/attestation_test.go | 53 ++++++++ 3 files changed, 63 insertions(+), 127 deletions(-) delete mode 100644 pkg/cmd/attestation/verify/attestation_integration_test.go create mode 100644 pkg/cmd/attestation/verify/attestation_test.go diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index 2a935a56c..68d38f985 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -41,30 +41,32 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio // Predicate type filtering is done after the attestations are fetched var attestations []*api.Attestation var err error - var errMsg string + var msg string if o.BundlePath != "" { attestations, err = verification.GetLocalAttestations(o.BundlePath) if err != nil { - errMsg = fmt.Sprintf("✗ Loading attestations from %s failed", a.URL) + pluralAttestation := text.Pluralize(len(attestations), "attestation") + msg = fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.BundlePath) + } else { + msg = fmt.Sprintf("Loaded %d attestations from %s", len(attestations), o.BundlePath) } } else if o.UseBundleFromRegistry { attestations, err = verification.GetOCIAttestations(o.OCIClient, a) if err != nil { - errMsg = "✗ Loading attestations from OCI registry failed" + msg = "✗ Loading attestations from OCI registry failed" + } else { + pluralAttestation := text.Pluralize(len(attestations), "attestation") + msg = fmt.Sprintf("Loaded %s from OCI registry", pluralAttestation) } } - if err != nil { - return nil, errMsg, err + return nil, msg, err } filtered, err := verification.FilterAttestations(o.PredicateType, attestations) if err != nil { return nil, err.Error(), err } - - pluralAttestation := text.Pluralize(len(filtered), "attestation") - msg := fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.BundlePath) return filtered, msg, nil } diff --git a/pkg/cmd/attestation/verify/attestation_integration_test.go b/pkg/cmd/attestation/verify/attestation_integration_test.go deleted file mode 100644 index 9ff174141..000000000 --- a/pkg/cmd/attestation/verify/attestation_integration_test.go +++ /dev/null @@ -1,119 +0,0 @@ -//go:build integration - -package verify - -import ( - "testing" - - "github.com/cli/cli/v2/pkg/cmd/attestation/api" - "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" - "github.com/cli/cli/v2/pkg/cmd/attestation/io" - "github.com/cli/cli/v2/pkg/cmd/attestation/test" - "github.com/cli/cli/v2/pkg/cmd/attestation/verification" - o "github.com/cli/cli/v2/pkg/option" - "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" - "github.com/stretchr/testify/require" -) - -func getAttestationsFor(t *testing.T, bundlePath string) []*api.Attestation { - t.Helper() - - attestations, err := verification.GetLocalAttestations(bundlePath) - require.NoError(t, err) - - return attestations -} - -func TestVerifyAttestations(t *testing.T) { - sgVerifier := verification.NewLiveSigstoreVerifier(verification.SigstoreConfig{ - Logger: io.NewTestHandler(), - TUFMetadataDir: o.Some(t.TempDir()), - }) - - certSummary := certificate.Summary{} - certSummary.SourceRepositoryOwnerURI = "https://github.com/sigstore" - certSummary.SourceRepositoryURI = "https://github.com/sigstore/sigstore-js" - certSummary.Issuer = verification.GitHubOIDCIssuer - - ec := verification.EnforcementCriteria{ - Certificate: certSummary, - PredicateType: verification.SLSAPredicateV1, - SANRegex: "^https://github.com/sigstore/", - } - require.NoError(t, ec.Valid()) - - artifactPath := test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz") - a, err := artifact.NewDigestedArtifact(nil, artifactPath, "sha512") - require.NoError(t, err) - - t.Run("all attestations pass verification", func(t *testing.T) { - attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") - require.Len(t, attestations, 2) - results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, ec) - require.NoError(t, err) - require.Zero(t, errMsg) - require.Len(t, results, 2) - }) - - t.Run("passes verification with 2/3 attestations passing Sigstore verification", func(t *testing.T) { - invalidBundle := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json") - attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") - attestations = append(attestations, invalidBundle[0]) - require.Len(t, attestations, 3) - - results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, ec) - require.NoError(t, err) - require.Zero(t, errMsg) - require.Len(t, results, 2) - }) - - t.Run("fails verification when Sigstore verification fails", func(t *testing.T) { - invalidBundle := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json") - invalidBundle2 := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json") - attestations := append(invalidBundle, invalidBundle2...) - require.Len(t, attestations, 2) - - results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, ec) - require.Error(t, err) - require.Contains(t, errMsg, "✗ Sigstore verification failed") - require.Nil(t, results) - }) - - t.Run("attestations fail to verify when cert extensions don't match enforcement criteria", func(t *testing.T) { - sgjAttestation := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") - reusableWorkflowAttestations := getAttestationsFor(t, "../test/data/reusable-workflow-attestation.sigstore.json") - attestations := []*api.Attestation{sgjAttestation[0], reusableWorkflowAttestations[0], sgjAttestation[1]} - require.Len(t, attestations, 3) - - rwfResult := verification.BuildMockResult(reusableWorkflowAttestations[0].Bundle, "", "", "https://github.com/malancas", "", verification.GitHubOIDCIssuer) - sgjResult := verification.BuildSigstoreJsMockResult(t) - mockResults := []*verification.AttestationProcessingResult{&sgjResult, &rwfResult, &sgjResult} - mockSgVerifier := verification.NewMockSigstoreVerifierWithMockResults(t, mockResults) - - // we want to test that attestations that pass Sigstore verification but fail - // cert extension verification are filtered out properly in the second step - // in verifyAttestations. By using a mock Sigstore verifier, we can ensure - // that the call to verification.VerifyCertExtensions in verifyAttestations - // is filtering out attestations as expected - results, errMsg, err := verifyAttestations(*a, attestations, mockSgVerifier, ec) - require.NoError(t, err) - require.Zero(t, errMsg) - require.Len(t, results, 2) - for _, result := range results { - require.NotEqual(t, result.Attestation.Bundle, reusableWorkflowAttestations[0].Bundle) - } - }) - - t.Run("fails verification when cert extension verification fails", func(t *testing.T) { - attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") - require.Len(t, attestations, 2) - - expectedCriteria := ec - expectedCriteria.Certificate.SourceRepositoryOwnerURI = "https://github.com/wrong" - - results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, expectedCriteria) - require.Error(t, err) - require.Contains(t, errMsg, "✗ Policy verification failed") - require.Nil(t, results) - }) -} diff --git a/pkg/cmd/attestation/verify/attestation_test.go b/pkg/cmd/attestation/verify/attestation_test.go new file mode 100644 index 000000000..93ac7d327 --- /dev/null +++ b/pkg/cmd/attestation/verify/attestation_test.go @@ -0,0 +1,53 @@ +package verify + +import ( + "testing" + + "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" + "github.com/cli/cli/v2/pkg/cmd/attestation/artifact/oci" + "github.com/cli/cli/v2/pkg/cmd/attestation/verification" + "github.com/stretchr/testify/require" +) + +func TestGetAttestations_OCIRegistry_PredicateTypeFiltering(t *testing.T) { + artifact, err := artifact.NewDigestedArtifact(nil, "../test/data/gh_2.60.1_windows_arm64.zip", "sha256") + require.NoError(t, err) + + o := &Options{ + OCIClient: oci.MockClient{}, + PredicateType: verification.SLSAPredicateV1, + Repo: "cli/cli", + UseBundleFromRegistry: true, + } + attestations, msg, err := getAttestations(o, *artifact) + require.NoError(t, err) + require.Contains(t, msg, "Loaded 2 attestations from OCI registry") + require.Len(t, attestations, 2) + + o.PredicateType = "custom predicate type" + attestations, msg, err = getAttestations(o, *artifact) + require.Error(t, err) + require.Contains(t, msg, "no attestations found with predicate type") + require.Nil(t, attestations) +} + +func TestGetAttestations_LocalBundle_PredicateTypeFiltering(t *testing.T) { + artifact, err := artifact.NewDigestedArtifact(nil, "../test/data/gh_2.60.1_windows_arm64.zip", "sha256") + require.NoError(t, err) + + o := &Options{ + BundlePath: "../test/data/sigstore-js-2.1.0-bundle.json", + PredicateType: verification.SLSAPredicateV1, + Repo: "sigstore/sigstore-js", + } + attestations, msg, err := getAttestations(o, *artifact) + require.NoError(t, err) + require.Contains(t, msg, "Loaded 1 attestation from ../test/data/gh_2.60.1_windows_arm64.zip") + require.Len(t, attestations, 1) + + o.PredicateType = "custom predicate type" + attestations, msg, err = getAttestations(o, *artifact) + require.Error(t, err) + require.Contains(t, msg, "no attestations found with predicate type") + require.Nil(t, attestations) +} From 164a56cb663702233a4bf4e884acf8d0578d6632 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 3 Apr 2025 11:02:45 -0600 Subject: [PATCH 17/18] move filterAttestations function Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/attestation.go | 35 +++++++++++++++++++ pkg/cmd/attestation/api/mock_client.go | 7 +++- pkg/cmd/attestation/download/download.go | 3 +- .../attestation/verification/attestation.go | 32 ----------------- .../verification/attestation_test.go | 4 +-- pkg/cmd/attestation/verify/attestation.go | 2 +- .../attestation/verify/attestation_test.go | 26 +++++++++++--- 7 files changed, 67 insertions(+), 42 deletions(-) diff --git a/pkg/cmd/attestation/api/attestation.go b/pkg/cmd/attestation/api/attestation.go index fd6b484a7..daec12b50 100644 --- a/pkg/cmd/attestation/api/attestation.go +++ b/pkg/cmd/attestation/api/attestation.go @@ -1,7 +1,10 @@ package api import ( + "encoding/json" "errors" + "fmt" + "github.com/sigstore/sigstore-go/pkg/bundle" ) @@ -20,3 +23,35 @@ type Attestation struct { type AttestationsResponse struct { Attestations []*Attestation `json:"attestations"` } + +type IntotoStatement struct { + PredicateType string `json:"predicateType"` +} + +func FilterAttestations(predicateType string, attestations []*Attestation) ([]*Attestation, error) { + filteredAttestations := []*Attestation{} + + for _, each := range attestations { + dsseEnvelope := each.Bundle.GetDsseEnvelope() + if dsseEnvelope != nil { + if dsseEnvelope.PayloadType != "application/vnd.in-toto+json" { + // Don't fail just because an entry isn't intoto + continue + } + var intotoStatement IntotoStatement + if err := json.Unmarshal([]byte(dsseEnvelope.Payload), &intotoStatement); err != nil { + // Don't fail just because a single entry can't be unmarshalled + continue + } + if intotoStatement.PredicateType == predicateType { + filteredAttestations = append(filteredAttestations, each) + } + } + } + + if len(filteredAttestations) == 0 { + return nil, fmt.Errorf("no attestations found with predicate type: %s", predicateType) + } + + return filteredAttestations, nil +} diff --git a/pkg/cmd/attestation/api/mock_client.go b/pkg/cmd/attestation/api/mock_client.go index fbf44bbb9..b6062b39f 100644 --- a/pkg/cmd/attestation/api/mock_client.go +++ b/pkg/cmd/attestation/api/mock_client.go @@ -26,7 +26,12 @@ func (m MockClient) GetTrustDomain() (string, error) { func OnGetByDigestSuccess(params FetchParams) ([]*Attestation, error) { att1 := makeTestAttestation() att2 := makeTestAttestation() - return []*Attestation{&att1, &att2}, nil + attestations := []*Attestation{&att1, &att2} + if params.PredicateType != "" { + return FilterAttestations(params.PredicateType, attestations) + } + + return attestations, nil } func OnGetByDigestFailure(params FetchParams) ([]*Attestation, error) { diff --git a/pkg/cmd/attestation/download/download.go b/pkg/cmd/attestation/download/download.go index 2cb648414..8d1d1dc05 100644 --- a/pkg/cmd/attestation/download/download.go +++ b/pkg/cmd/attestation/download/download.go @@ -9,7 +9,6 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/artifact/oci" "github.com/cli/cli/v2/pkg/cmd/attestation/auth" "github.com/cli/cli/v2/pkg/cmd/attestation/io" - "github.com/cli/cli/v2/pkg/cmd/attestation/verification" "github.com/cli/cli/v2/pkg/cmdutil" ghauth "github.com/cli/go-gh/v2/pkg/auth" @@ -147,7 +146,7 @@ func runDownload(opts *Options) error { // Apply predicate type filter to returned attestations if opts.PredicateType != "" { - filteredAttestations, err := verification.FilterAttestations(opts.PredicateType, attestations) + filteredAttestations, err := api.FilterAttestations(opts.PredicateType, attestations) if err != nil { return fmt.Errorf("failed to filter attestations: %v", err) } diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index ba357a5cc..10eb02ac4 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -92,35 +92,3 @@ func GetOCIAttestations(client oci.Client, artifact artifact.DigestedArtifact) ( } return attestations, nil } - -type IntotoStatement struct { - PredicateType string `json:"predicateType"` -} - -func FilterAttestations(predicateType string, attestations []*api.Attestation) ([]*api.Attestation, error) { - filteredAttestations := []*api.Attestation{} - - for _, each := range attestations { - dsseEnvelope := each.Bundle.GetDsseEnvelope() - if dsseEnvelope != nil { - if dsseEnvelope.PayloadType != "application/vnd.in-toto+json" { - // Don't fail just because an entry isn't intoto - continue - } - var intotoStatement IntotoStatement - if err := json.Unmarshal([]byte(dsseEnvelope.Payload), &intotoStatement); err != nil { - // Don't fail just because a single entry can't be unmarshalled - continue - } - if intotoStatement.PredicateType == predicateType { - filteredAttestations = append(filteredAttestations, each) - } - } - } - - if len(filteredAttestations) == 0 { - return nil, fmt.Errorf("no attestations found with predicate type: %s", predicateType) - } - - return filteredAttestations, nil -} diff --git a/pkg/cmd/attestation/verification/attestation_test.go b/pkg/cmd/attestation/verification/attestation_test.go index 18e2c6cca..6826e2e40 100644 --- a/pkg/cmd/attestation/verification/attestation_test.go +++ b/pkg/cmd/attestation/verification/attestation_test.go @@ -157,11 +157,11 @@ func TestFilterAttestations(t *testing.T) { }, } - filtered, err := FilterAttestations("https://slsa.dev/provenance/v1", attestations) + filtered, err := api.FilterAttestations("https://slsa.dev/provenance/v1", attestations) require.Len(t, filtered, 1) require.NoError(t, err) - filtered, err = FilterAttestations("NonExistentPredicate", attestations) + filtered, err = api.FilterAttestations("NonExistentPredicate", attestations) require.Nil(t, filtered) require.Error(t, err) } diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index 68d38f985..1b98fabf3 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -63,7 +63,7 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio return nil, msg, err } - filtered, err := verification.FilterAttestations(o.PredicateType, attestations) + filtered, err := api.FilterAttestations(o.PredicateType, attestations) if err != nil { return nil, err.Error(), err } diff --git a/pkg/cmd/attestation/verify/attestation_test.go b/pkg/cmd/attestation/verify/attestation_test.go index 93ac7d327..f015805ae 100644 --- a/pkg/cmd/attestation/verify/attestation_test.go +++ b/pkg/cmd/attestation/verify/attestation_test.go @@ -3,6 +3,7 @@ package verify import ( "testing" + "github.com/cli/cli/v2/pkg/cmd/attestation/api" "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" "github.com/cli/cli/v2/pkg/cmd/attestation/artifact/oci" "github.com/cli/cli/v2/pkg/cmd/attestation/verification" @@ -40,14 +41,31 @@ func TestGetAttestations_LocalBundle_PredicateTypeFiltering(t *testing.T) { PredicateType: verification.SLSAPredicateV1, Repo: "sigstore/sigstore-js", } - attestations, msg, err := getAttestations(o, *artifact) + attestations, _, err := getAttestations(o, *artifact) require.NoError(t, err) - require.Contains(t, msg, "Loaded 1 attestation from ../test/data/gh_2.60.1_windows_arm64.zip") require.Len(t, attestations, 1) o.PredicateType = "custom predicate type" - attestations, msg, err = getAttestations(o, *artifact) + attestations, _, err = getAttestations(o, *artifact) + require.Error(t, err) + require.Nil(t, attestations) +} + +func TestGetAttestations_GhAPI_NoAttestationsFound(t *testing.T) { + artifact, err := artifact.NewDigestedArtifact(nil, "../test/data/gh_2.60.1_windows_arm64.zip", "sha256") + require.NoError(t, err) + + o := &Options{ + APIClient: api.NewTestClient(), + PredicateType: verification.SLSAPredicateV1, + Repo: "sigstore/sigstore-js", + } + attestations, _, err := getAttestations(o, *artifact) + require.NoError(t, err) + require.Len(t, attestations, 2) + + o.PredicateType = "custom predicate type" + attestations, _, err = getAttestations(o, *artifact) require.Error(t, err) - require.Contains(t, msg, "no attestations found with predicate type") require.Nil(t, attestations) } From 69507282d251ffde053baa28eb26e644ed4a3be5 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 3 Apr 2025 11:07:06 -0600 Subject: [PATCH 18/18] restore deleted file Signed-off-by: Meredith Lancaster --- .../verify/attestation_integration_test.go | 119 ++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 pkg/cmd/attestation/verify/attestation_integration_test.go diff --git a/pkg/cmd/attestation/verify/attestation_integration_test.go b/pkg/cmd/attestation/verify/attestation_integration_test.go new file mode 100644 index 000000000..9ff174141 --- /dev/null +++ b/pkg/cmd/attestation/verify/attestation_integration_test.go @@ -0,0 +1,119 @@ +//go:build integration + +package verify + +import ( + "testing" + + "github.com/cli/cli/v2/pkg/cmd/attestation/api" + "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" + "github.com/cli/cli/v2/pkg/cmd/attestation/io" + "github.com/cli/cli/v2/pkg/cmd/attestation/test" + "github.com/cli/cli/v2/pkg/cmd/attestation/verification" + o "github.com/cli/cli/v2/pkg/option" + "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" + "github.com/stretchr/testify/require" +) + +func getAttestationsFor(t *testing.T, bundlePath string) []*api.Attestation { + t.Helper() + + attestations, err := verification.GetLocalAttestations(bundlePath) + require.NoError(t, err) + + return attestations +} + +func TestVerifyAttestations(t *testing.T) { + sgVerifier := verification.NewLiveSigstoreVerifier(verification.SigstoreConfig{ + Logger: io.NewTestHandler(), + TUFMetadataDir: o.Some(t.TempDir()), + }) + + certSummary := certificate.Summary{} + certSummary.SourceRepositoryOwnerURI = "https://github.com/sigstore" + certSummary.SourceRepositoryURI = "https://github.com/sigstore/sigstore-js" + certSummary.Issuer = verification.GitHubOIDCIssuer + + ec := verification.EnforcementCriteria{ + Certificate: certSummary, + PredicateType: verification.SLSAPredicateV1, + SANRegex: "^https://github.com/sigstore/", + } + require.NoError(t, ec.Valid()) + + artifactPath := test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz") + a, err := artifact.NewDigestedArtifact(nil, artifactPath, "sha512") + require.NoError(t, err) + + t.Run("all attestations pass verification", func(t *testing.T) { + attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") + require.Len(t, attestations, 2) + results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, ec) + require.NoError(t, err) + require.Zero(t, errMsg) + require.Len(t, results, 2) + }) + + t.Run("passes verification with 2/3 attestations passing Sigstore verification", func(t *testing.T) { + invalidBundle := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json") + attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") + attestations = append(attestations, invalidBundle[0]) + require.Len(t, attestations, 3) + + results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, ec) + require.NoError(t, err) + require.Zero(t, errMsg) + require.Len(t, results, 2) + }) + + t.Run("fails verification when Sigstore verification fails", func(t *testing.T) { + invalidBundle := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json") + invalidBundle2 := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json") + attestations := append(invalidBundle, invalidBundle2...) + require.Len(t, attestations, 2) + + results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, ec) + require.Error(t, err) + require.Contains(t, errMsg, "✗ Sigstore verification failed") + require.Nil(t, results) + }) + + t.Run("attestations fail to verify when cert extensions don't match enforcement criteria", func(t *testing.T) { + sgjAttestation := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") + reusableWorkflowAttestations := getAttestationsFor(t, "../test/data/reusable-workflow-attestation.sigstore.json") + attestations := []*api.Attestation{sgjAttestation[0], reusableWorkflowAttestations[0], sgjAttestation[1]} + require.Len(t, attestations, 3) + + rwfResult := verification.BuildMockResult(reusableWorkflowAttestations[0].Bundle, "", "", "https://github.com/malancas", "", verification.GitHubOIDCIssuer) + sgjResult := verification.BuildSigstoreJsMockResult(t) + mockResults := []*verification.AttestationProcessingResult{&sgjResult, &rwfResult, &sgjResult} + mockSgVerifier := verification.NewMockSigstoreVerifierWithMockResults(t, mockResults) + + // we want to test that attestations that pass Sigstore verification but fail + // cert extension verification are filtered out properly in the second step + // in verifyAttestations. By using a mock Sigstore verifier, we can ensure + // that the call to verification.VerifyCertExtensions in verifyAttestations + // is filtering out attestations as expected + results, errMsg, err := verifyAttestations(*a, attestations, mockSgVerifier, ec) + require.NoError(t, err) + require.Zero(t, errMsg) + require.Len(t, results, 2) + for _, result := range results { + require.NotEqual(t, result.Attestation.Bundle, reusableWorkflowAttestations[0].Bundle) + } + }) + + t.Run("fails verification when cert extension verification fails", func(t *testing.T) { + attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") + require.Len(t, attestations, 2) + + expectedCriteria := ec + expectedCriteria.Certificate.SourceRepositoryOwnerURI = "https://github.com/wrong" + + results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, expectedCriteria) + require.Error(t, err) + require.Contains(t, errMsg, "✗ Policy verification failed") + require.Nil(t, results) + }) +}