From ff9b6bb883d993e4a33cc583ccfc109694962b14 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 7 Nov 2024 14:39:11 -0700 Subject: [PATCH 1/8] refactor fetch attestations funcs Signed-off-by: Meredith Lancaster --- .../attestation/verification/attestation.go | 36 +++---------- pkg/cmd/attestation/verify/attestation.go | 51 +++++++++++++++++++ pkg/cmd/attestation/verify/verify.go | 33 ++---------- 3 files changed, 63 insertions(+), 57 deletions(-) create mode 100644 pkg/cmd/attestation/verify/attestation.go diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index 0ea91c2f7f0..dd885a1c1f5 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -9,8 +9,8 @@ import ( "path/filepath" "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/google/go-containerregistry/pkg/name" protobundle "github.com/sigstore/protobuf-specs/gen/pb-go/bundle/v1" "github.com/sigstore/sigstore-go/pkg/bundle" ) @@ -21,31 +21,11 @@ var ErrUnrecognisedBundleExtension = errors.New("bundle file extension not suppo var ErrEmptyBundleFile = errors.New("provided bundle file is empty") type FetchAttestationsConfig struct { - APIClient api.Client - BundlePath string - Digest string - Limit int - Owner string - Repo string - OCIClient oci.Client - UseBundleFromRegistry bool - NameRef name.Reference -} - -func (c *FetchAttestationsConfig) IsBundleProvided() bool { - return c.BundlePath != "" -} - -func GetAttestations(c FetchAttestationsConfig) ([]*api.Attestation, error) { - if c.IsBundleProvided() { - return GetLocalAttestations(c.BundlePath) - } - - if c.UseBundleFromRegistry { - return GetOCIAttestations(c) - } - - return GetRemoteAttestations(c) + APIClient api.Client + Digest string + Limit int + Owner string + Repo string } // GetLocalAttestations returns a slice of attestations read from a local bundle file. @@ -138,8 +118,8 @@ func GetRemoteAttestations(c FetchAttestationsConfig) ([]*api.Attestation, error return nil, fmt.Errorf("owner or repo must be provided") } -func GetOCIAttestations(c FetchAttestationsConfig) ([]*api.Attestation, error) { - attestations, err := c.OCIClient.GetAttestations(c.NameRef, c.Digest) +func GetOCIAttestations(client oci.Client, artifact artifact.DigestedArtifact) ([]*api.Attestation, error) { + attestations, err := client.GetAttestations(artifact.NameRef(), artifact.Digest()) if err != nil { return nil, fmt.Errorf("failed to fetch OCI attestations: %w", err) } diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go new file mode 100644 index 00000000000..a588d976492 --- /dev/null +++ b/pkg/cmd/attestation/verify/attestation.go @@ -0,0 +1,51 @@ +package verify + +import ( + "fmt" + + "github.com/cli/cli/v2/internal/text" + "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/verification" +) + +func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestation, string, error) { + if o.BundlePath != "" { + attestations, err := verification.GetLocalAttestations(o.BundlePath) + if err != nil { + msg := fmt.Sprintf("✗ Loading attestations from %s failed\n", a.URL) + return nil, msg, err + } + pluralAttestation := text.Pluralize(len(attestations), "attestation") + msg := fmt.Sprintf("Loaded %s from %s\n", pluralAttestation, o.BundlePath) + return attestations, msg, nil + } + + if o.UseBundleFromRegistry { + attestations, err := verification.GetOCIAttestations(o.OCIClient, a) + if err != nil { + msg := "✗ Loading attestations from OCI registry failed\n" + return nil, msg, err + } + pluralAttestation := text.Pluralize(len(attestations), "attestation") + msg := fmt.Sprintf("Loaded %s from %s\n", pluralAttestation, o.ArtifactPath) + return attestations, msg, nil + } + + c := verification.FetchAttestationsConfig{ + APIClient: o.APIClient, + Digest: a.DigestWithAlg(), + Limit: o.Limit, + Owner: o.Owner, + Repo: o.Repo, + } + + attestations, err := verification.GetRemoteAttestations(c) + if err != nil { + msg := "✗ Loading attestations from GitHub API failed\n" + return nil, msg, err + } + pluralAttestation := text.Pluralize(len(attestations), "attestation") + msg := fmt.Sprintf("Loaded %s from GitHub API\n", pluralAttestation) + return attestations, msg, nil +} diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 47b52bb30de..e5dd12f594a 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -6,7 +6,6 @@ import ( "regexp" "github.com/cli/cli/v2/internal/ghinstance" - "github.com/cli/cli/v2/internal/text" "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" @@ -222,42 +221,18 @@ func runVerify(opts *Options) error { opts.Logger.Printf("Loaded digest %s for %s\n", artifact.DigestWithAlg(), artifact.URL) - c := verification.FetchAttestationsConfig{ - APIClient: opts.APIClient, - BundlePath: opts.BundlePath, - Digest: artifact.DigestWithAlg(), - Limit: opts.Limit, - Owner: opts.Owner, - Repo: opts.Repo, - OCIClient: opts.OCIClient, - UseBundleFromRegistry: opts.UseBundleFromRegistry, - NameRef: artifact.NameRef(), - } - attestations, err := verification.GetAttestations(c) + attestations, logMsg, err := getAttestations(opts, *artifact) if err != nil { if ok := errors.Is(err, api.ErrNoAttestations{}); ok { opts.Logger.Printf(opts.Logger.ColorScheme.Red("✗ No attestations found for subject %s\n"), artifact.DigestWithAlg()) return err } - if c.IsBundleProvided() { - opts.Logger.Printf(opts.Logger.ColorScheme.Red("✗ Loading attestations from %s failed\n"), artifact.URL) - } else if c.UseBundleFromRegistry { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Loading attestations from OCI registry failed")) - } else { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Loading attestations from GitHub API failed")) - } + opts.Logger.Printf(opts.Logger.ColorScheme.Red(logMsg)) return err } - - pluralAttestation := text.Pluralize(len(attestations), "attestation") - if c.IsBundleProvided() { - opts.Logger.Printf("Loaded %s from %s\n", pluralAttestation, opts.BundlePath) - } else if c.UseBundleFromRegistry { - opts.Logger.Printf("Loaded %s from %s\n", pluralAttestation, opts.ArtifactPath) - } else { - opts.Logger.Printf("Loaded %s from GitHub API\n", pluralAttestation) - } + // Print the message signifying success fetching attestations + opts.Logger.Printf(logMsg) // Apply predicate type filter to returned attestations filteredAttestations := verification.FilterAttestations(ec.PredicateType, attestations) From 8ab5f247aff3fe65a1864d2fd9d6aba704b7769e Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 7 Nov 2024 14:47:53 -0700 Subject: [PATCH 2/8] rename type Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/download/download.go | 2 +- pkg/cmd/attestation/verification/attestation.go | 4 ++-- pkg/cmd/attestation/verify/attestation.go | 2 +- pkg/cmd/attestation/verify/verify.go | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/attestation/download/download.go b/pkg/cmd/attestation/download/download.go index 77c9280933f..ef5a11799fd 100644 --- a/pkg/cmd/attestation/download/download.go +++ b/pkg/cmd/attestation/download/download.go @@ -122,7 +122,7 @@ func runDownload(opts *Options) error { opts.Logger.VerbosePrintf("Downloading trusted metadata for artifact %s\n\n", opts.ArtifactPath) - c := verification.FetchAttestationsConfig{ + c := verification.FetchRemoteAttestations{ APIClient: opts.APIClient, Digest: artifact.DigestWithAlg(), Limit: opts.Limit, diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index dd885a1c1f5..bdee3a0146a 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -20,7 +20,7 @@ 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 FetchAttestationsConfig struct { +type FetchRemoteAttestations struct { APIClient api.Client Digest string Limit int @@ -96,7 +96,7 @@ func loadBundlesFromJSONLinesFile(path string) ([]*api.Attestation, error) { return attestations, nil } -func GetRemoteAttestations(c FetchAttestationsConfig) ([]*api.Attestation, error) { +func GetRemoteAttestations(c FetchRemoteAttestations) ([]*api.Attestation, error) { if c.APIClient == nil { return nil, fmt.Errorf("api client must be provided") } diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index a588d976492..483d4cf97d0 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -32,7 +32,7 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio return attestations, msg, nil } - c := verification.FetchAttestationsConfig{ + c := verification.FetchRemoteAttestations{ APIClient: o.APIClient, Digest: a.DigestWithAlg(), Limit: o.Limit, diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index e5dd12f594a..90149f7ca53 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -227,7 +227,7 @@ func runVerify(opts *Options) error { opts.Logger.Printf(opts.Logger.ColorScheme.Red("✗ No attestations found for subject %s\n"), artifact.DigestWithAlg()) return err } - + // Print the message signifying failure fetching attestations opts.Logger.Printf(opts.Logger.ColorScheme.Red(logMsg)) return err } From e4cd729a7b9dc6f0394d5ab9a7a0e1af401cdc7a Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 7 Nov 2024 14:59:21 -0700 Subject: [PATCH 3/8] simplify verifyCertExtensions Signed-off-by: Meredith Lancaster --- .../attestation/verification/extensions.go | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions.go b/pkg/cmd/attestation/verification/extensions.go index e302d89c9d1..540c9628422 100644 --- a/pkg/cmd/attestation/verification/extensions.go +++ b/pkg/cmd/attestation/verification/extensions.go @@ -20,7 +20,7 @@ func VerifyCertExtensions(results []*AttestationProcessingResult, ec Enforcement var lastErr error for _, attestation := range results { - err := verifyCertExtensions(*attestation.VerificationResult.Signature.Certificate, ec) + err := verifyCertExtensions(*attestation.VerificationResult.Signature.Certificate, ec.Certificate) if err == nil { // if at least one attestation is verified, we're good as verification // is defined as successful if at least one attestation is verified @@ -34,28 +34,23 @@ func VerifyCertExtensions(results []*AttestationProcessingResult, ec Enforcement return lastErr } -func verifyCertExtensions(verifiedCert certificate.Summary, criteria EnforcementCriteria) error { - sourceRepositoryOwnerURI := verifiedCert.Extensions.SourceRepositoryOwnerURI - if !strings.EqualFold(criteria.Certificate.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) { - return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", criteria.Certificate.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) +func verifyCertExtensions(verified, expected certificate.Summary) error { + if !strings.EqualFold(expected.SourceRepositoryOwnerURI, verified.SourceRepositoryOwnerURI) { + return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", expected.SourceRepositoryOwnerURI, verified.SourceRepositoryOwnerURI) } - // if repo is set, check the SourceRepositoryURI field - if criteria.Certificate.SourceRepositoryURI != "" { - sourceRepositoryURI := verifiedCert.Extensions.SourceRepositoryURI - if !strings.EqualFold(criteria.Certificate.SourceRepositoryURI, sourceRepositoryURI) { - return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", criteria.Certificate.SourceRepositoryURI, sourceRepositoryURI) - } + // if repo is set, compare the SourceRepositoryURI fields + if expected.SourceRepositoryURI != "" && !strings.EqualFold(expected.SourceRepositoryURI, verified.SourceRepositoryURI) { + return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", expected.SourceRepositoryURI, verified.SourceRepositoryURI) } - // if issuer is anything other than the default, use the user-provided value; - // otherwise, select the appropriate default based on the tenant - certIssuer := verifiedCert.Extensions.Issuer - if !strings.EqualFold(criteria.Certificate.Issuer, certIssuer) { - if strings.Index(certIssuer, criteria.Certificate.Issuer+"/") == 0 { - return fmt.Errorf("expected Issuer to be %s, got %s -- if you have a custom OIDC issuer policy for your enterprise, use the --cert-oidc-issuer flag with your expected issuer", criteria.Certificate.Issuer, certIssuer) + // compare the OIDC issuers. If not equal, return an error depending + // on if there is a partial match + if !strings.EqualFold(expected.Issuer, verified.Issuer) { + if strings.Index(verified.Issuer, expected.Issuer+"/") == 0 { + return fmt.Errorf("expected Issuer to be %s, got %s -- if you have a custom OIDC issuer policy for your enterprise, use the --cert-oidc-issuer flag with your expected issuer", expected.Issuer, verified.Issuer) } - return fmt.Errorf("expected Issuer to be %s, got %s", criteria.Certificate.Issuer, certIssuer) + return fmt.Errorf("expected Issuer to be %s, got %s", expected.Issuer, verified.Issuer) } return nil From 43e5abbcd8c7691694dc9a64f28edb2d56b6cfb7 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 7 Nov 2024 15:50:46 -0700 Subject: [PATCH 4/8] use logger println method Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/attestation.go | 12 ++++++------ pkg/cmd/attestation/verify/verify.go | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index 483d4cf97d0..c067afe9bd7 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -13,22 +13,22 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio if o.BundlePath != "" { attestations, err := verification.GetLocalAttestations(o.BundlePath) if err != nil { - msg := fmt.Sprintf("✗ Loading attestations from %s failed\n", a.URL) + msg := fmt.Sprintf("✗ Loading attestations from %s failed", a.URL) return nil, msg, err } pluralAttestation := text.Pluralize(len(attestations), "attestation") - msg := fmt.Sprintf("Loaded %s from %s\n", pluralAttestation, o.BundlePath) + msg := fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.BundlePath) return attestations, msg, nil } if o.UseBundleFromRegistry { attestations, err := verification.GetOCIAttestations(o.OCIClient, a) if err != nil { - msg := "✗ Loading attestations from OCI registry failed\n" + msg := "✗ Loading attestations from OCI registry failed" return nil, msg, err } pluralAttestation := text.Pluralize(len(attestations), "attestation") - msg := fmt.Sprintf("Loaded %s from %s\n", pluralAttestation, o.ArtifactPath) + msg := fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.ArtifactPath) return attestations, msg, nil } @@ -42,10 +42,10 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio attestations, err := verification.GetRemoteAttestations(c) if err != nil { - msg := "✗ Loading attestations from GitHub API failed\n" + 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\n", pluralAttestation) + msg := fmt.Sprintf("Loaded %s from GitHub API", pluralAttestation) return attestations, msg, nil } diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 90149f7ca53..1d34fdf99ca 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -228,11 +228,11 @@ func runVerify(opts *Options) error { return err } // Print the message signifying failure fetching attestations - opts.Logger.Printf(opts.Logger.ColorScheme.Red(logMsg)) + opts.Logger.Println(opts.Logger.ColorScheme.Red(logMsg)) return err } // Print the message signifying success fetching attestations - opts.Logger.Printf(logMsg) + opts.Logger.Println(logMsg) // Apply predicate type filter to returned attestations filteredAttestations := verification.FilterAttestations(ec.PredicateType, attestations) From c518a3b1f57d179b40eb40991741720b344e5d61 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 18 Nov 2024 08:18:04 -0700 Subject: [PATCH 5/8] Update pkg/cmd/attestation/verification/extensions.go Co-authored-by: Phill MV --- pkg/cmd/attestation/verification/extensions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/attestation/verification/extensions.go b/pkg/cmd/attestation/verification/extensions.go index 540c9628422..130a301e3eb 100644 --- a/pkg/cmd/attestation/verification/extensions.go +++ b/pkg/cmd/attestation/verification/extensions.go @@ -34,7 +34,7 @@ func VerifyCertExtensions(results []*AttestationProcessingResult, ec Enforcement return lastErr } -func verifyCertExtensions(verified, expected certificate.Summary) error { +func verifyCertExtensions(given, expected certificate.Summary) error { if !strings.EqualFold(expected.SourceRepositoryOwnerURI, verified.SourceRepositoryOwnerURI) { return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", expected.SourceRepositoryOwnerURI, verified.SourceRepositoryOwnerURI) } From 762e99d151c3c5e3927ad7c34aa7d758a59b05ce Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 18 Nov 2024 08:19:07 -0700 Subject: [PATCH 6/8] fix function param calls Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verification/extensions.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions.go b/pkg/cmd/attestation/verification/extensions.go index 130a301e3eb..a0827e9ec13 100644 --- a/pkg/cmd/attestation/verification/extensions.go +++ b/pkg/cmd/attestation/verification/extensions.go @@ -35,22 +35,22 @@ func VerifyCertExtensions(results []*AttestationProcessingResult, ec Enforcement } func verifyCertExtensions(given, expected certificate.Summary) error { - if !strings.EqualFold(expected.SourceRepositoryOwnerURI, verified.SourceRepositoryOwnerURI) { - return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", expected.SourceRepositoryOwnerURI, verified.SourceRepositoryOwnerURI) + if !strings.EqualFold(expected.SourceRepositoryOwnerURI, given.SourceRepositoryOwnerURI) { + return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", expected.SourceRepositoryOwnerURI, given.SourceRepositoryOwnerURI) } // if repo is set, compare the SourceRepositoryURI fields - if expected.SourceRepositoryURI != "" && !strings.EqualFold(expected.SourceRepositoryURI, verified.SourceRepositoryURI) { - return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", expected.SourceRepositoryURI, verified.SourceRepositoryURI) + if expected.SourceRepositoryURI != "" && !strings.EqualFold(expected.SourceRepositoryURI, given.SourceRepositoryURI) { + return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", expected.SourceRepositoryURI, given.SourceRepositoryURI) } // compare the OIDC issuers. If not equal, return an error depending // on if there is a partial match - if !strings.EqualFold(expected.Issuer, verified.Issuer) { - if strings.Index(verified.Issuer, expected.Issuer+"/") == 0 { - return fmt.Errorf("expected Issuer to be %s, got %s -- if you have a custom OIDC issuer policy for your enterprise, use the --cert-oidc-issuer flag with your expected issuer", expected.Issuer, verified.Issuer) + if !strings.EqualFold(expected.Issuer, given.Issuer) { + if strings.Index(given.Issuer, expected.Issuer+"/") == 0 { + return fmt.Errorf("expected Issuer to be %s, got %s -- if you have a custom OIDC issuer policy for your enterprise, use the --cert-oidc-issuer flag with your expected issuer", expected.Issuer, given.Issuer) } - return fmt.Errorf("expected Issuer to be %s, got %s", expected.Issuer, verified.Issuer) + return fmt.Errorf("expected Issuer to be %s, got %s", expected.Issuer, given.Issuer) } return nil From 30ae1388e4e4010f3b02620506638d813f3c884d Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 18 Nov 2024 08:19:41 -0700 Subject: [PATCH 7/8] Update pkg/cmd/attestation/download/download.go Co-authored-by: Phill MV --- pkg/cmd/attestation/download/download.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/attestation/download/download.go b/pkg/cmd/attestation/download/download.go index ef5a11799fd..a79af59358b 100644 --- a/pkg/cmd/attestation/download/download.go +++ b/pkg/cmd/attestation/download/download.go @@ -122,7 +122,7 @@ func runDownload(opts *Options) error { opts.Logger.VerbosePrintf("Downloading trusted metadata for artifact %s\n\n", opts.ArtifactPath) - c := verification.FetchRemoteAttestations{ + c := verification.FetchRemoteAttestationsParams{ APIClient: opts.APIClient, Digest: artifact.DigestWithAlg(), Limit: opts.Limit, From 63f37eb36996fbf496673052386be215b14b992d Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 18 Nov 2024 08:24:25 -0700 Subject: [PATCH 8/8] pr feedback Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/download/download.go | 13 +++++---- .../attestation/verification/attestation.go | 27 +++++++++---------- pkg/cmd/attestation/verify/attestation.go | 13 +++++---- 3 files changed, 25 insertions(+), 28 deletions(-) diff --git a/pkg/cmd/attestation/download/download.go b/pkg/cmd/attestation/download/download.go index a79af59358b..143912308d3 100644 --- a/pkg/cmd/attestation/download/download.go +++ b/pkg/cmd/attestation/download/download.go @@ -122,14 +122,13 @@ func runDownload(opts *Options) error { opts.Logger.VerbosePrintf("Downloading trusted metadata for artifact %s\n\n", opts.ArtifactPath) - c := verification.FetchRemoteAttestationsParams{ - APIClient: opts.APIClient, - Digest: artifact.DigestWithAlg(), - Limit: opts.Limit, - Owner: opts.Owner, - Repo: opts.Repo, + params := verification.FetchRemoteAttestationsParams{ + Digest: artifact.DigestWithAlg(), + Limit: opts.Limit, + Owner: opts.Owner, + Repo: opts.Repo, } - attestations, err := verification.GetRemoteAttestations(c) + attestations, err := verification.GetRemoteAttestations(opts.APIClient, params) if err != nil { if errors.Is(err, api.ErrNoAttestations{}) { fmt.Fprintf(opts.Logger.IO.Out, "No attestations found for %s\n", opts.ArtifactPath) diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index bdee3a0146a..07083a5c0a4 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -20,12 +20,11 @@ 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 FetchRemoteAttestations struct { - APIClient api.Client - Digest string - Limit int - Owner string - Repo string +type FetchRemoteAttestationsParams struct { + Digest string + Limit int + Owner string + Repo string } // GetLocalAttestations returns a slice of attestations read from a local bundle file. @@ -96,22 +95,22 @@ func loadBundlesFromJSONLinesFile(path string) ([]*api.Attestation, error) { return attestations, nil } -func GetRemoteAttestations(c FetchRemoteAttestations) ([]*api.Attestation, error) { - if c.APIClient == nil { +func GetRemoteAttestations(client api.Client, params FetchRemoteAttestationsParams) ([]*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 c.Repo != "" { - attestations, err := c.APIClient.GetByRepoAndDigest(c.Repo, c.Digest, c.Limit) + if params.Repo != "" { + attestations, err := client.GetByRepoAndDigest(params.Repo, params.Digest, params.Limit) if err != nil { - return nil, fmt.Errorf("failed to fetch attestations from %s: %w", c.Repo, err) + return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Repo, err) } return attestations, nil - } else if c.Owner != "" { - attestations, err := c.APIClient.GetByOwnerAndDigest(c.Owner, c.Digest, c.Limit) + } else if params.Owner != "" { + attestations, err := client.GetByOwnerAndDigest(params.Owner, params.Digest, params.Limit) if err != nil { - return nil, fmt.Errorf("failed to fetch attestations from %s: %w", c.Owner, err) + return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Owner, err) } return attestations, nil } diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index c067afe9bd7..f3f2792c419 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -32,15 +32,14 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio return attestations, msg, nil } - c := verification.FetchRemoteAttestations{ - APIClient: o.APIClient, - Digest: a.DigestWithAlg(), - Limit: o.Limit, - Owner: o.Owner, - Repo: o.Repo, + params := verification.FetchRemoteAttestationsParams{ + Digest: a.DigestWithAlg(), + Limit: o.Limit, + Owner: o.Owner, + Repo: o.Repo, } - attestations, err := verification.GetRemoteAttestations(c) + attestations, err := verification.GetRemoteAttestations(o.APIClient, params) if err != nil { msg := "✗ Loading attestations from GitHub API failed" return nil, msg, err