diff --git a/go.mod b/go.mod index db8ab013f..335b58b91 100644 --- a/go.mod +++ b/go.mod @@ -24,7 +24,6 @@ require ( github.com/hashicorp/go-multierror v1.1.1 github.com/hashicorp/go-version v1.3.0 github.com/henvic/httpretty v0.1.3 - github.com/in-toto/in-toto-golang v0.9.0 github.com/joho/godotenv v1.5.1 github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 github.com/mattn/go-colorable v0.1.13 @@ -97,6 +96,7 @@ require ( github.com/hashicorp/go-cleanhttp v0.5.2 // indirect github.com/hashicorp/go-retryablehttp v0.7.5 // indirect github.com/hashicorp/hcl v1.0.0 // indirect + github.com/in-toto/in-toto-golang v0.9.0 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/itchyny/gojq v0.12.15 // indirect github.com/itchyny/timefmt-go v0.1.5 // indirect diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 6e366dfa7..c23458aff 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -142,6 +142,13 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command $ gh api repos/{owner}/{repo}/issues --template \ '{{range .}}{{.title}} ({{.labels | pluck "name" | join ", " | color "yellow"}}){{"\n"}}{{end}}' + # update allowed values of the "environment" custom property in a deeply nested array + gh api --PATCH /orgs/{org}/properties/schema \ + -F 'properties[][property_name]=environment' \ + -F 'properties[][default_value]=production' \ + -F 'properties[][allowed_values][]=staging' \ + -F 'properties[][allowed_values][]=production' + # list releases with GraphQL $ gh api graphql -F owner='{owner}' -F name='{repo}' -f query=' query($name: String!, $owner: String!) { diff --git a/pkg/cmd/api/fields.go b/pkg/cmd/api/fields.go index 40167c498..024eb12ca 100644 --- a/pkg/cmd/api/fields.go +++ b/pkg/cmd/api/fields.go @@ -2,6 +2,7 @@ package api import ( "fmt" + "reflect" "strconv" "strings" ) @@ -98,6 +99,9 @@ func parseFields(opts *ApiOptions) (map[string]interface{}, error) { } } } else { + if _, exists := destMap[subkey]; exists { + return fmt.Errorf("unexpected override existing field under %q", subkey) + } destMap[subkey] = value } return nil @@ -136,6 +140,8 @@ func addParamsSlice(m map[string]interface{}, prevkey, newkey string) (map[strin if lastMap, ok := lastItem.(map[string]interface{}); ok { if _, keyExists := lastMap[newkey]; !keyExists { return lastMap, nil + } else if reflect.TypeOf(lastMap[newkey]).Kind() == reflect.Slice { + return lastMap, nil } } } diff --git a/pkg/cmd/api/fields_test.go b/pkg/cmd/api/fields_test.go index 6fd051209..73bc7463c 100644 --- a/pkg/cmd/api/fields_test.go +++ b/pkg/cmd/api/fields_test.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/iostreams" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_parseFields(t *testing.T) { @@ -61,8 +62,14 @@ func Test_parseFields_nested(t *testing.T) { "robots[]=Dependabot", "labels[][name]=bug", "labels[][color]=red", + "labels[][colorOptions][]=red", + "labels[][colorOptions][]=blue", "labels[][name]=feature", "labels[][color]=green", + "labels[][colorOptions][]=red", + "labels[][colorOptions][]=green", + "labels[][colorOptions][]=yellow", + "nested[][key1][key2][key3]=value", "empty[]", }, MagicFields: []string{ @@ -96,13 +103,31 @@ func Test_parseFields_nested(t *testing.T) { "labels": [ { "color": "red", + "colorOptions": [ + "red", + "blue" + ], "name": "bug" }, { "color": "green", + "colorOptions": [ + "red", + "green", + "yellow" + ], "name": "feature" } ], + "nested": [ + { + "key1": { + "key2": { + "key3": "value" + } + } + } + ], "robots": [ "Hubot", "Dependabot" @@ -111,6 +136,91 @@ func Test_parseFields_nested(t *testing.T) { `), "\n"), string(jsonData)) } +func Test_parseFields_errors(t *testing.T) { + ios, stdin, _, _ := iostreams.Test() + fmt.Fprint(stdin, "pasted contents") + + tests := []struct { + name string + opts *ApiOptions + expected string + }{ + { + name: "cannot overwrite string to array", + opts: &ApiOptions{ + IO: ios, + RawFields: []string{ + "object[field]=A", + "object[field][]=this should be an error", + }, + }, + expected: `expected array type under "field", got string`, + }, + { + name: "cannot overwrite string to object", + opts: &ApiOptions{ + IO: ios, + RawFields: []string{ + "object[field]=B", + "object[field][field2]=this should be an error", + }, + }, + expected: `expected map type under "field", got string`, + }, + { + name: "cannot overwrite object to string", + opts: &ApiOptions{ + IO: ios, + RawFields: []string{ + "object[field][field2]=C", + "object[field]=this should be an error", + }, + }, + expected: `unexpected override existing field under "field"`, + }, + { + name: "cannot overwrite object to array", + opts: &ApiOptions{ + IO: ios, + RawFields: []string{ + "object[field][field2]=D", + "object[field][]=this should be an error", + }, + }, + expected: `expected array type under "field", got map[string]interface {}`, + }, + { + name: "cannot overwrite array to string", + opts: &ApiOptions{ + IO: ios, + RawFields: []string{ + "object[field][]=E", + "object[field]=this should be an error", + }, + }, + expected: `unexpected override existing field under "field"`, + }, + { + name: "cannot overwrite array to object", + opts: &ApiOptions{ + IO: ios, + RawFields: []string{ + "object[field][]=F", + "object[field][field2]=this should be an error", + }, + }, + expected: `expected map type under "field", got []interface {}`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := parseFields(tt.opts) + require.EqualError(t, err, tt.expected) + }) + } +} + func Test_magicFieldValue(t *testing.T) { f, err := os.CreateTemp(t.TempDir(), "gh-test") if err != nil { diff --git a/pkg/cmd/attestation/api/mock_client.go b/pkg/cmd/attestation/api/mock_client.go index 96a64e4fc..edb51ee6e 100644 --- a/pkg/cmd/attestation/api/mock_client.go +++ b/pkg/cmd/attestation/api/mock_client.go @@ -1,11 +1,9 @@ package api import ( - "encoding/json" "fmt" - "os" - "github.com/sigstore/sigstore-go/pkg/bundle" + "github.com/cli/cli/v2/pkg/cmd/attestation/test/data" ) type MockClient struct { @@ -22,18 +20,7 @@ func (m MockClient) GetByOwnerAndDigest(owner, digest string, limit int) ([]*Att } func makeTestAttestation() Attestation { - bundleBytes, err := os.ReadFile("../test/data/sigstore-js-2.1.0-bundle.json") - if err != nil { - panic(err) - } - - var b *bundle.ProtobufBundle - err = json.Unmarshal(bundleBytes, &b) - if err != nil { - panic(err) - } - - return Attestation{Bundle: b} + return Attestation{Bundle: data.SigstoreBundle(nil)} } func OnGetByRepoAndDigestSuccess(repo, digest string, limit int) ([]*Attestation, error) { diff --git a/pkg/cmd/attestation/download/download.go b/pkg/cmd/attestation/download/download.go index 2f40970fc..7154a6edd 100644 --- a/pkg/cmd/attestation/download/download.go +++ b/pkg/cmd/attestation/download/download.go @@ -20,7 +20,7 @@ func NewDownloadCmd(f *cmdutil.Factory, runF func(*Options) error) *cobra.Comman opts := &Options{} downloadCmd := &cobra.Command{ Use: "download [ | oci://] [--owner | --repo]", - Args: cobra.ExactArgs(1), + Args: cmdutil.MinimumArgs(1, "must specify file path or container image URI, as well as one of --owner or --repo"), Short: "Download an artifact's Sigstore bundle(s) for offline use", Long: heredoc.Docf(` Download an artifact's attestations, aka Sigstore bundle(s), for offline use. @@ -102,6 +102,7 @@ func NewDownloadCmd(f *cmdutil.Factory, runF func(*Options) error) *cobra.Comman downloadCmd.Flags().StringVarP(&opts.Repo, "repo", "R", "", "Repository name in the format /") downloadCmd.MarkFlagsMutuallyExclusive("owner", "repo") downloadCmd.MarkFlagsOneRequired("owner", "repo") + downloadCmd.Flags().StringVarP(&opts.PredicateType, "predicate-type", "", "", "Filter attestations by provided predicate type") cmdutil.StringEnumFlag(downloadCmd, &opts.DigestAlgorithm, "digest-alg", "d", "sha256", []string{"sha256", "sha512"}, "The algorithm used to compute a digest of the artifact") downloadCmd.Flags().IntVarP(&opts.Limit, "limit", "L", api.DefaultLimit, "Maximum number of attestations to fetch") @@ -132,6 +133,17 @@ func runDownload(opts *Options) error { return fmt.Errorf("failed to fetch attestations: %v", err) } + // 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) + } + + attestations = filteredAttestations + } + metadataFilePath, err := opts.Store.createMetadataFile(artifact.DigestWithAlg(), attestations) if err != nil { return fmt.Errorf("failed to write attestation: %v", err) diff --git a/pkg/cmd/attestation/download/options.go b/pkg/cmd/attestation/download/options.go index 6a64ecc30..cc1c4d3b5 100644 --- a/pkg/cmd/attestation/download/options.go +++ b/pkg/cmd/attestation/download/options.go @@ -22,6 +22,7 @@ type Options struct { Store MetadataStore OCIClient oci.Client Owner string + PredicateType string Repo string } diff --git a/pkg/cmd/attestation/inspect/inspect.go b/pkg/cmd/attestation/inspect/inspect.go index 1b2105da4..759667b36 100644 --- a/pkg/cmd/attestation/inspect/inspect.go +++ b/pkg/cmd/attestation/inspect/inspect.go @@ -73,6 +73,17 @@ func NewInspectCmd(f *cmdutil.Factory, runF func(*Options) error) *cobra.Command return runF(opts) } + config := verification.SigstoreConfig{ + Logger: opts.Logger, + } + + sigstore, err := verification.NewLiveSigstoreVerifier(config) + if err != nil { + return err + } + + opts.SigstoreVerifier = sigstore + if err := runInspect(opts); err != nil { return fmt.Errorf("Failed to inspect the artifact and bundle: %w", err) } @@ -101,21 +112,12 @@ func runInspect(opts *Options) error { return fmt.Errorf("failed to read attestations for subject: %s", artifact.DigestWithAlg()) } - config := verification.SigstoreConfig{ - Logger: opts.Logger, - } - policy, err := buildPolicy(*artifact) if err != nil { return fmt.Errorf("failed to build policy: %v", err) } - sigstore, err := verification.NewSigstoreVerifier(config, policy) - if err != nil { - return err - } - - res := sigstore.Verify(attestations) + res := opts.SigstoreVerifier.Verify(attestations, policy) if res.Error != nil { return fmt.Errorf("at least one attestation failed to verify against Sigstore: %v", res.Error) } diff --git a/pkg/cmd/attestation/inspect/inspect_test.go b/pkg/cmd/attestation/inspect/inspect_test.go index e42bb2620..368cc54f5 100644 --- a/pkg/cmd/attestation/inspect/inspect_test.go +++ b/pkg/cmd/attestation/inspect/inspect_test.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/artifact/oci" "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" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" @@ -53,10 +54,11 @@ func TestNewInspectCmd(t *testing.T) { name: "Invalid digest-alg flag", cli: fmt.Sprintf("%s --bundle %s --digest-alg sha384", artifactPath, bundlePath), wants: Options{ - ArtifactPath: artifactPath, - BundlePath: bundlePath, - DigestAlgorithm: "sha384", - OCIClient: oci.MockClient{}, + ArtifactPath: artifactPath, + BundlePath: bundlePath, + DigestAlgorithm: "sha384", + OCIClient: oci.MockClient{}, + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: true, }, @@ -64,10 +66,11 @@ func TestNewInspectCmd(t *testing.T) { name: "Use default digest-alg value", cli: fmt.Sprintf("%s --bundle %s", artifactPath, bundlePath), wants: Options{ - ArtifactPath: artifactPath, - BundlePath: bundlePath, - DigestAlgorithm: "sha256", - OCIClient: oci.MockClient{}, + ArtifactPath: artifactPath, + BundlePath: bundlePath, + DigestAlgorithm: "sha256", + OCIClient: oci.MockClient{}, + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: false, }, @@ -75,10 +78,11 @@ func TestNewInspectCmd(t *testing.T) { name: "Use custom digest-alg value", cli: fmt.Sprintf("%s --bundle %s --digest-alg sha512", artifactPath, bundlePath), wants: Options{ - ArtifactPath: artifactPath, - BundlePath: bundlePath, - DigestAlgorithm: "sha512", - OCIClient: oci.MockClient{}, + ArtifactPath: artifactPath, + BundlePath: bundlePath, + DigestAlgorithm: "sha512", + OCIClient: oci.MockClient{}, + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: false, }, @@ -86,9 +90,10 @@ func TestNewInspectCmd(t *testing.T) { name: "Missing bundle flag", cli: artifactPath, wants: Options{ - ArtifactPath: artifactPath, - DigestAlgorithm: "sha256", - OCIClient: oci.MockClient{}, + ArtifactPath: artifactPath, + DigestAlgorithm: "sha256", + OCIClient: oci.MockClient{}, + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: true, }, @@ -96,10 +101,11 @@ func TestNewInspectCmd(t *testing.T) { name: "Prints output in JSON format", cli: fmt.Sprintf("%s --bundle %s --format json", artifactPath, bundlePath), wants: Options{ - ArtifactPath: artifactPath, - BundlePath: bundlePath, - DigestAlgorithm: "sha256", - OCIClient: oci.MockClient{}, + ArtifactPath: artifactPath, + BundlePath: bundlePath, + DigestAlgorithm: "sha256", + OCIClient: oci.MockClient{}, + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsExporter: true, }, @@ -128,8 +134,8 @@ func TestNewInspectCmd(t *testing.T) { assert.Equal(t, tc.wants.ArtifactPath, opts.ArtifactPath) assert.Equal(t, tc.wants.BundlePath, opts.BundlePath) assert.Equal(t, tc.wants.DigestAlgorithm, opts.DigestAlgorithm) - assert.NotNil(t, opts.OCIClient) assert.NotNil(t, opts.Logger) + assert.NotNil(t, opts.OCIClient) assert.Equal(t, tc.wantsExporter, opts.exporter != nil) }) } @@ -137,11 +143,12 @@ func TestNewInspectCmd(t *testing.T) { func TestRunInspect(t *testing.T) { opts := Options{ - ArtifactPath: artifactPath, - BundlePath: bundlePath, - DigestAlgorithm: "sha512", - Logger: io.NewTestHandler(), - OCIClient: oci.MockClient{}, + ArtifactPath: artifactPath, + BundlePath: bundlePath, + DigestAlgorithm: "sha512", + Logger: io.NewTestHandler(), + OCIClient: oci.MockClient{}, + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), } t.Run("with valid artifact and bundle", func(t *testing.T) { @@ -164,12 +171,13 @@ func TestRunInspect(t *testing.T) { func TestJSONOutput(t *testing.T) { testIO, _, out, _ := iostreams.Test() opts := Options{ - ArtifactPath: artifactPath, - BundlePath: bundlePath, - DigestAlgorithm: "sha512", - Logger: io.NewHandler(testIO), - OCIClient: oci.MockClient{}, - exporter: cmdutil.NewJSONExporter(), + ArtifactPath: artifactPath, + BundlePath: bundlePath, + DigestAlgorithm: "sha512", + Logger: io.NewHandler(testIO), + OCIClient: oci.MockClient{}, + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), + exporter: cmdutil.NewJSONExporter(), } require.Nil(t, runInspect(&opts)) diff --git a/pkg/cmd/attestation/inspect/options.go b/pkg/cmd/attestation/inspect/options.go index 56199e06b..b9c8819c4 100644 --- a/pkg/cmd/attestation/inspect/options.go +++ b/pkg/cmd/attestation/inspect/options.go @@ -5,17 +5,19 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/artifact/oci" "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" ) // Options captures the options for the inspect command type Options struct { - ArtifactPath string - BundlePath string - DigestAlgorithm string - Logger *io.Handler - OCIClient oci.Client - exporter cmdutil.Exporter + ArtifactPath string + BundlePath string + DigestAlgorithm string + Logger *io.Handler + OCIClient oci.Client + SigstoreVerifier verification.SigstoreVerifier + exporter cmdutil.Exporter } // Clean cleans the file path option values diff --git a/pkg/cmd/attestation/test/data/data.go b/pkg/cmd/attestation/test/data/data.go new file mode 100644 index 000000000..77f07e60c --- /dev/null +++ b/pkg/cmd/attestation/test/data/data.go @@ -0,0 +1,17 @@ +package data + +import ( + _ "embed" + "testing" + + "github.com/sigstore/sigstore-go/pkg/bundle" + sgData "github.com/sigstore/sigstore-go/pkg/testing/data" +) + +//go:embed sigstore-js-2.1.0-bundle.json +var SigstoreBundleRaw []byte + +// SigstoreBundle returns a test *sigstore.Bundle +func SigstoreBundle(t *testing.T) *bundle.ProtobufBundle { + return sgData.TestBundle(t, SigstoreBundleRaw) +} diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index 7fb4a1615..b131b7acd 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -2,6 +2,7 @@ package verification import ( "bufio" + "encoding/json" "errors" "fmt" "os" @@ -113,3 +114,31 @@ func GetRemoteAttestations(c FetchAttestationsConfig) ([]*api.Attestation, error } return nil, fmt.Errorf("owner or repo must be provided") } + +type IntotoStatement struct { + PredicateType string `json:"predicateType"` +} + +func FilterAttestations(predicateType string, attestations []*api.Attestation) []*api.Attestation { + 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) + } + } + } + + return filteredAttestations +} diff --git a/pkg/cmd/attestation/verification/attestation_test.go b/pkg/cmd/attestation/verification/attestation_test.go index b178a0682..ef7c7d879 100644 --- a/pkg/cmd/attestation/verification/attestation_test.go +++ b/pkg/cmd/attestation/verification/attestation_test.go @@ -3,7 +3,12 @@ package verification import ( "testing" + protobundle "github.com/sigstore/protobuf-specs/gen/pb-go/bundle/v1" + dsse "github.com/sigstore/protobuf-specs/gen/pb-go/dsse" + "github.com/sigstore/sigstore-go/pkg/bundle" "github.com/stretchr/testify/require" + + "github.com/cli/cli/v2/pkg/cmd/attestation/api" ) func TestLoadBundlesFromJSONLinesFile(t *testing.T) { @@ -47,3 +52,51 @@ func TestGetLocalAttestations(t *testing.T) { require.Nil(t, attestations) }) } + +func TestFilterAttestations(t *testing.T) { + attestations := []*api.Attestation{ + { + Bundle: &bundle.ProtobufBundle{ + Bundle: &protobundle.Bundle{ + Content: &protobundle.Bundle_DsseEnvelope{ + DsseEnvelope: &dsse.Envelope{ + PayloadType: "application/vnd.in-toto+json", + Payload: []byte("{\"predicateType\": \"https://slsa.dev/provenance/v1\"}"), + }, + }, + }, + }, + }, + { + Bundle: &bundle.ProtobufBundle{ + Bundle: &protobundle.Bundle{ + Content: &protobundle.Bundle_DsseEnvelope{ + DsseEnvelope: &dsse.Envelope{ + PayloadType: "application/vnd.something-other-than-in-toto+json", + Payload: []byte("{\"predicateType\": \"https://slsa.dev/provenance/v1\"}"), + }, + }, + }, + }, + }, + { + Bundle: &bundle.ProtobufBundle{ + Bundle: &protobundle.Bundle{ + Content: &protobundle.Bundle_DsseEnvelope{ + DsseEnvelope: &dsse.Envelope{ + PayloadType: "application/vnd.in-toto+json", + Payload: []byte("{\"predicateType\": \"https://spdx.dev/Document/v2.3\"}"), + }, + }, + }, + }, + }, + } + + filtered := FilterAttestations("https://slsa.dev/provenance/v1", attestations) + + require.Len(t, filtered, 1) + + filtered = FilterAttestations("NonExistantPredicate", attestations) + require.Len(t, filtered, 0) +} diff --git a/pkg/cmd/attestation/verification/mock_verifier.go b/pkg/cmd/attestation/verification/mock_verifier.go new file mode 100644 index 000000000..51e66c424 --- /dev/null +++ b/pkg/cmd/attestation/verification/mock_verifier.go @@ -0,0 +1,50 @@ +package verification + +import ( + "fmt" + "testing" + + "github.com/cli/cli/v2/pkg/cmd/attestation/api" + "github.com/cli/cli/v2/pkg/cmd/attestation/test/data" + + "github.com/in-toto/in-toto-golang/in_toto" + "github.com/sigstore/sigstore-go/pkg/verify" +) + +const SLSAPredicateType = "https://slsa.dev/provenance/v1" + +type MockSigstoreVerifier struct { + t *testing.T +} + +func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults { + statement := &in_toto.Statement{} + statement.PredicateType = SLSAPredicateType + + result := AttestationProcessingResult{ + Attestation: &api.Attestation{ + Bundle: data.SigstoreBundle(v.t), + }, + VerificationResult: &verify.VerificationResult{ + Statement: statement, + }, + } + + results := []*AttestationProcessingResult{&result} + + return &SigstoreResults{ + VerifyResults: results, + } +} + +func NewMockSigstoreVerifier(t *testing.T) *MockSigstoreVerifier { + return &MockSigstoreVerifier{t} +} + +type FailSigstoreVerifier struct{} + +func (v *FailSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults { + return &SigstoreResults{ + Error: fmt.Errorf("failed to verify attestations"), + } +} diff --git a/pkg/cmd/attestation/verification/sigstore.go b/pkg/cmd/attestation/verification/sigstore.go index daaacf628..ad57102ea 100644 --- a/pkg/cmd/attestation/verification/sigstore.go +++ b/pkg/cmd/attestation/verification/sigstore.go @@ -34,19 +34,22 @@ type SigstoreConfig struct { NoPublicGood bool } -type SigstoreVerifier struct { +type SigstoreVerifier interface { + Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults +} + +type LiveSigstoreVerifier struct { ghVerifier *verify.SignedEntityVerifier publicGoodVerifier *verify.SignedEntityVerifier customVerifier *verify.SignedEntityVerifier - policy verify.PolicyBuilder onlyVerifyWithGithub bool Logger *io.Handler } -// NewSigstoreVerifier creates a new SigstoreVerifier struct +// NewLiveSigstoreVerifier creates a new LiveSigstoreVerifier struct // that is used to verify artifacts and attestations against the // Public Good, GitHub, or a custom trusted root. -func NewSigstoreVerifier(config SigstoreConfig, policy verify.PolicyBuilder) (*SigstoreVerifier, error) { +func NewLiveSigstoreVerifier(config SigstoreConfig) (*LiveSigstoreVerifier, error) { customVerifier, err := newCustomVerifier(config.CustomTrustedRoot) if err != nil { return nil, fmt.Errorf("failed to create custom verifier: %v", err) @@ -62,17 +65,16 @@ func NewSigstoreVerifier(config SigstoreConfig, policy verify.PolicyBuilder) (*S return nil, fmt.Errorf("failed to create GitHub Sigstore verifier: %v", err) } - return &SigstoreVerifier{ + return &LiveSigstoreVerifier{ ghVerifier: ghVerifier, publicGoodVerifier: publicGoodVerifier, customVerifier: customVerifier, Logger: config.Logger, - policy: policy, onlyVerifyWithGithub: config.NoPublicGood, }, nil } -func (v *SigstoreVerifier) chooseVerifier(b *bundle.ProtobufBundle) (*verify.SignedEntityVerifier, string, error) { +func (v *LiveSigstoreVerifier) chooseVerifier(b *bundle.ProtobufBundle) (*verify.SignedEntityVerifier, string, error) { verifyContent, err := b.VerificationContent() if err != nil { return nil, "", fmt.Errorf("failed to get bundle verification content: %v", err) @@ -103,7 +105,7 @@ func (v *SigstoreVerifier) chooseVerifier(b *bundle.ProtobufBundle) (*verify.Sig return nil, "", fmt.Errorf("leaf certificate issuer is not recognized") } -func (v *SigstoreVerifier) Verify(attestations []*api.Attestation) *SigstoreResults { +func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults { // initialize the processing results before attempting to verify // with multiple verifiers results := make([]*AttestationProcessingResult, len(attestations)) @@ -128,7 +130,7 @@ func (v *SigstoreVerifier) Verify(attestations []*api.Attestation) *SigstoreResu v.Logger.VerbosePrintf("Attempting verification against issuer \"%s\"\n", issuer) // attempt to verify the attestation - result, err := verifier.Verify(apr.Attestation.Bundle, v.policy) + result, err := verifier.Verify(apr.Attestation.Bundle, policy) // if verification fails, create the error and exit verification early if err != nil { v.Logger.VerbosePrint(v.Logger.ColorScheme.Redf( diff --git a/pkg/cmd/attestation/verification/sigstore_test.go b/pkg/cmd/attestation/verification/sigstore_test.go index 204b5e583..0243a11ee 100644 --- a/pkg/cmd/attestation/verification/sigstore_test.go +++ b/pkg/cmd/attestation/verification/sigstore_test.go @@ -21,7 +21,7 @@ func buildPolicy(a artifact.DigestedArtifact) (verify.PolicyBuilder, error) { return policy, nil } -func TestNewSigstoreVerifier(t *testing.T) { +func TestNewLiveSigstoreVerifier(t *testing.T) { artifactPath := test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz") artifact, err := artifact.NewDigestedArtifact(nil, artifactPath, "sha512") require.NoError(t, err) @@ -32,7 +32,7 @@ func TestNewSigstoreVerifier(t *testing.T) { c := SigstoreConfig{ Logger: io.NewTestHandler(), } - verifier, err := NewSigstoreVerifier(c, policy) + verifier, err := NewLiveSigstoreVerifier(c) require.NoError(t, err) t.Run("with invalid signature", func(t *testing.T) { @@ -41,7 +41,7 @@ func TestNewSigstoreVerifier(t *testing.T) { require.NotNil(t, attestations) require.NoError(t, err) - res := verifier.Verify(attestations) + res := verifier.Verify(attestations, policy) require.Error(t, res.Error) require.ErrorContains(t, res.Error, "verifying with issuer \"sigstore.dev\"") require.Nil(t, res.VerifyResults) @@ -53,7 +53,7 @@ func TestNewSigstoreVerifier(t *testing.T) { require.Len(t, attestations, 2) require.NoError(t, err) - res := verifier.Verify(attestations) + res := verifier.Verify(attestations, policy) require.Len(t, res.VerifyResults, 2) require.NoError(t, res.Error) }) diff --git a/pkg/cmd/attestation/verify/options.go b/pkg/cmd/attestation/verify/options.go index d7742bf3a..bfa03f16e 100644 --- a/pkg/cmd/attestation/verify/options.go +++ b/pkg/cmd/attestation/verify/options.go @@ -8,6 +8,7 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/api" "github.com/cli/cli/v2/pkg/cmd/attestation/artifact/oci" "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" ) @@ -18,16 +19,18 @@ type Options struct { CustomTrustedRoot string DenySelfHostedRunner bool DigestAlgorithm string + Limit int NoPublicGood bool OIDCIssuer string Owner string + PredicateType string Repo string SAN string SANRegex string APIClient api.Client Logger *io.Handler - Limit int OCIClient oci.Client + SigstoreVerifier verification.SigstoreVerifier exporter cmdutil.Exporter } diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index 83d1f62a7..f96ca4e8a 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -11,8 +11,7 @@ import ( ) const ( - GitHubOIDCIssuer = "https://token.actions.githubusercontent.com" - SLSAPredicateType = "https://slsa.dev/provenance/v1" + GitHubOIDCIssuer = "https://token.actions.githubusercontent.com" // represents the GitHub hosted runner in the certificate RunnerEnvironment extension GitHubRunner = "github-hosted" ) diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 718f893a2..df604b9de 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -1,7 +1,6 @@ package verify import ( - // "encoding/json" "errors" "fmt" @@ -17,13 +16,11 @@ import ( "github.com/spf13/cobra" ) -var ErrNoMatchingSLSAPredicate = fmt.Errorf("the attestation does not have the expected SLSA predicate type: %s", SLSAPredicateType) - func NewVerifyCmd(f *cmdutil.Factory, runF func(*Options) error) *cobra.Command { opts := &Options{} verifyCmd := &cobra.Command{ Use: "verify [ | oci://] [--owner | --repo]", - Args: cobra.ExactArgs(1), + Args: cmdutil.MinimumArgs(1, "must specify file path or container image URI, as well as one of --owner or --repo"), Short: "Verify an artifact's integrity using attestations", Long: heredoc.Docf(` Verify the integrity and provenance of an artifact using its associated @@ -106,6 +103,19 @@ func NewVerifyCmd(f *cmdutil.Factory, runF func(*Options) error) *cobra.Command return runF(opts) } + config := verification.SigstoreConfig{ + CustomTrustedRoot: opts.CustomTrustedRoot, + Logger: opts.Logger, + NoPublicGood: opts.NoPublicGood, + } + + sv, err := verification.NewLiveSigstoreVerifier(config) + if err != nil { + return err + } + + opts.SigstoreVerifier = sv + if err := runVerify(opts); err != nil { return fmt.Errorf("Failed to verify the artifact: %v", err) } @@ -120,6 +130,7 @@ func NewVerifyCmd(f *cmdutil.Factory, runF func(*Options) error) *cobra.Command verifyCmd.Flags().StringVarP(&opts.Repo, "repo", "R", "", "Repository name in the format /") verifyCmd.MarkFlagsMutuallyExclusive("owner", "repo") verifyCmd.MarkFlagsOneRequired("owner", "repo") + verifyCmd.Flags().StringVarP(&opts.PredicateType, "predicate-type", "", "", "Filter attestations by provided predicate type") verifyCmd.Flags().BoolVarP(&opts.NoPublicGood, "no-public-good", "", false, "Only verify attestations signed with GitHub's Sigstore instance") verifyCmd.Flags().StringVarP(&opts.CustomTrustedRoot, "custom-trusted-root", "", "", "Path to a custom trustedroot.json file to use for verification") verifyCmd.Flags().IntVarP(&opts.Limit, "limit", "L", api.DefaultLimit, "Maximum number of attestations to fetch") @@ -158,23 +169,23 @@ func runVerify(opts *Options) error { return fmt.Errorf("failed to fetch attestations for subject: %s", artifact.DigestWithAlg()) } + // 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) + } + + attestations = filteredAttestations + } + policy, err := buildVerifyPolicy(opts, *artifact) if err != nil { return fmt.Errorf("failed to build policy: %v", err) } - config := verification.SigstoreConfig{ - CustomTrustedRoot: opts.CustomTrustedRoot, - Logger: opts.Logger, - NoPublicGood: opts.NoPublicGood, - } - - sv, err := verification.NewSigstoreVerifier(config, policy) - if err != nil { - return err - } - - sigstoreRes := sv.Verify(attestations) + sigstoreRes := opts.SigstoreVerifier.Verify(attestations, policy) if sigstoreRes.Error != nil { return fmt.Errorf("at least one attestation failed to verify against Sigstore: %v", sigstoreRes.Error) } @@ -183,11 +194,6 @@ func runVerify(opts *Options) error { "Successfully verified all attestations against Sigstore!\n", )) - // Try verifying the attestation's predicate type against the expect SLSA predicate type - if err = verifySLSAPredicateType(opts.Logger, sigstoreRes.VerifyResults); err != nil { - return fmt.Errorf("at least one attestation failed to verify predicate type verification: %v", err) - } - opts.Logger.VerbosePrint(opts.Logger.ColorScheme.Green("Successfully verified the SLSA predicate type of all attestations!\n")) opts.Logger.Println(opts.Logger.ColorScheme.Green("All attestations have been successfully verified!")) @@ -202,15 +208,3 @@ func runVerify(opts *Options) error { // All attestations passed verification and policy evaluation return nil } - -func verifySLSAPredicateType(logger *io.Handler, apr []*verification.AttestationProcessingResult) error { - logger.VerbosePrint("Evaluating attestations have valid SLSA predicate type") - - for _, result := range apr { - if result.VerificationResult.Statement.PredicateType != SLSAPredicateType { - return ErrNoMatchingSLSAPredicate - } - } - - return nil -} diff --git a/pkg/cmd/attestation/verify/verify_test.go b/pkg/cmd/attestation/verify/verify_test.go index b4cd864fc..f5d034541 100644 --- a/pkg/cmd/attestation/verify/verify_test.go +++ b/pkg/cmd/attestation/verify/verify_test.go @@ -19,9 +19,6 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/stretchr/testify/assert" - "github.com/in-toto/in-toto-golang/in_toto" - "github.com/sigstore/sigstore-go/pkg/verify" - "github.com/stretchr/testify/require" ) @@ -58,12 +55,13 @@ func TestNewVerifyCmd(t *testing.T) { name: "Invalid digest-alg flag", cli: fmt.Sprintf("%s --bundle %s --digest-alg sha384 --owner sigstore", artifactPath, bundlePath), wants: Options{ - ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), - BundlePath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0-bundle.json"), - DigestAlgorithm: "sha384", - Limit: 30, - OIDCIssuer: GitHubOIDCIssuer, - Owner: "sigstore", + ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), + BundlePath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0-bundle.json"), + DigestAlgorithm: "sha384", + Limit: 30, + OIDCIssuer: GitHubOIDCIssuer, + Owner: "sigstore", + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: true, }, @@ -71,13 +69,14 @@ func TestNewVerifyCmd(t *testing.T) { name: "Use default digest-alg value", cli: fmt.Sprintf("%s --bundle %s --owner sigstore", artifactPath, bundlePath), wants: Options{ - ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), - BundlePath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0-bundle.json"), - DigestAlgorithm: "sha256", - Limit: 30, - OIDCIssuer: GitHubOIDCIssuer, - Owner: "sigstore", - SANRegex: "^https://github.com/sigstore/", + ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), + BundlePath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0-bundle.json"), + DigestAlgorithm: "sha256", + Limit: 30, + OIDCIssuer: GitHubOIDCIssuer, + Owner: "sigstore", + SANRegex: "^https://github.com/sigstore/", + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: false, }, @@ -85,13 +84,14 @@ func TestNewVerifyCmd(t *testing.T) { name: "Use custom digest-alg value", cli: fmt.Sprintf("%s --bundle %s --owner sigstore --digest-alg sha512", artifactPath, bundlePath), wants: Options{ - ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), - BundlePath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0-bundle.json"), - DigestAlgorithm: "sha512", - Limit: 30, - OIDCIssuer: GitHubOIDCIssuer, - Owner: "sigstore", - SANRegex: "^https://github.com/sigstore/", + ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), + BundlePath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0-bundle.json"), + DigestAlgorithm: "sha512", + Limit: 30, + OIDCIssuer: GitHubOIDCIssuer, + Owner: "sigstore", + SANRegex: "^https://github.com/sigstore/", + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: false, }, @@ -99,12 +99,13 @@ func TestNewVerifyCmd(t *testing.T) { name: "Missing owner and repo flags", cli: artifactPath, wants: Options{ - ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), - DigestAlgorithm: "sha256", - OIDCIssuer: GitHubOIDCIssuer, - Owner: "sigstore", - Limit: 30, - SANRegex: "^https://github.com/sigstore/", + ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), + DigestAlgorithm: "sha256", + OIDCIssuer: GitHubOIDCIssuer, + Owner: "sigstore", + Limit: 30, + SANRegex: "^https://github.com/sigstore/", + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: true, }, @@ -112,12 +113,13 @@ func TestNewVerifyCmd(t *testing.T) { name: "Has both owner and repo flags", cli: fmt.Sprintf("%s --owner sigstore --repo sigstore/sigstore-js", artifactPath), wants: Options{ - ArtifactPath: artifactPath, - DigestAlgorithm: "sha256", - OIDCIssuer: GitHubOIDCIssuer, - Owner: "sigstore", - Repo: "sigstore/sigstore-js", - Limit: 30, + ArtifactPath: artifactPath, + DigestAlgorithm: "sha256", + OIDCIssuer: GitHubOIDCIssuer, + Owner: "sigstore", + Repo: "sigstore/sigstore-js", + Limit: 30, + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: true, }, @@ -125,12 +127,13 @@ func TestNewVerifyCmd(t *testing.T) { name: "Uses default limit flag", cli: fmt.Sprintf("%s --owner sigstore", artifactPath), wants: Options{ - ArtifactPath: artifactPath, - DigestAlgorithm: "sha256", - Limit: 30, - OIDCIssuer: GitHubOIDCIssuer, - Owner: "sigstore", - SANRegex: "^https://github.com/sigstore/", + ArtifactPath: artifactPath, + DigestAlgorithm: "sha256", + Limit: 30, + OIDCIssuer: GitHubOIDCIssuer, + Owner: "sigstore", + SANRegex: "^https://github.com/sigstore/", + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: false, }, @@ -138,12 +141,13 @@ func TestNewVerifyCmd(t *testing.T) { name: "Uses custom limit flag", cli: fmt.Sprintf("%s --owner sigstore --limit 101", artifactPath), wants: Options{ - ArtifactPath: artifactPath, - DigestAlgorithm: "sha256", - OIDCIssuer: GitHubOIDCIssuer, - Owner: "sigstore", - Limit: 101, - SANRegex: "^https://github.com/sigstore/", + ArtifactPath: artifactPath, + DigestAlgorithm: "sha256", + OIDCIssuer: GitHubOIDCIssuer, + Owner: "sigstore", + Limit: 101, + SANRegex: "^https://github.com/sigstore/", + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: false, }, @@ -151,12 +155,13 @@ func TestNewVerifyCmd(t *testing.T) { name: "Uses invalid limit flag", cli: fmt.Sprintf("%s --owner sigstore --limit 0", artifactPath), wants: Options{ - ArtifactPath: artifactPath, - DigestAlgorithm: "sha256", - OIDCIssuer: GitHubOIDCIssuer, - Owner: "sigstore", - Limit: 0, - SANRegex: "^https://github.com/sigstore/", + ArtifactPath: artifactPath, + DigestAlgorithm: "sha256", + OIDCIssuer: GitHubOIDCIssuer, + Owner: "sigstore", + Limit: 0, + SANRegex: "^https://github.com/sigstore/", + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: true, }, @@ -164,13 +169,14 @@ func TestNewVerifyCmd(t *testing.T) { name: "Has both cert-identity and cert-identity-regex flags", cli: fmt.Sprintf("%s --owner sigstore --cert-identity https://github.com/sigstore/ --cert-identity-regex ^https://github.com/sigstore/", artifactPath), wants: Options{ - ArtifactPath: artifactPath, - DigestAlgorithm: "sha256", - Limit: 30, - OIDCIssuer: GitHubOIDCIssuer, - Owner: "sigstore", - SAN: "https://github.com/sigstore/", - SANRegex: "^https://github.com/sigstore/", + ArtifactPath: artifactPath, + DigestAlgorithm: "sha256", + Limit: 30, + OIDCIssuer: GitHubOIDCIssuer, + Owner: "sigstore", + SAN: "https://github.com/sigstore/", + SANRegex: "^https://github.com/sigstore/", + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: true, }, @@ -178,13 +184,14 @@ func TestNewVerifyCmd(t *testing.T) { name: "Prints output in JSON format", cli: fmt.Sprintf("%s --bundle %s --owner sigstore --format json", artifactPath, bundlePath), wants: Options{ - ArtifactPath: artifactPath, - BundlePath: bundlePath, - DigestAlgorithm: "sha256", - Limit: 30, - OIDCIssuer: GitHubOIDCIssuer, - Owner: "sigstore", - SANRegex: "^https://github.com/sigstore/", + ArtifactPath: artifactPath, + BundlePath: bundlePath, + DigestAlgorithm: "sha256", + Limit: 30, + OIDCIssuer: GitHubOIDCIssuer, + Owner: "sigstore", + SANRegex: "^https://github.com/sigstore/", + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsExporter: true, }, @@ -233,16 +240,17 @@ func TestNewVerifyCmd(t *testing.T) { func TestJSONOutput(t *testing.T) { testIO, _, out, _ := iostreams.Test() opts := Options{ - ArtifactPath: artifactPath, - BundlePath: bundlePath, - DigestAlgorithm: "sha512", - APIClient: api.NewTestClient(), - Logger: io.NewHandler(testIO), - OCIClient: oci.MockClient{}, - OIDCIssuer: GitHubOIDCIssuer, - Owner: "sigstore", - SANRegex: "^https://github.com/sigstore/", - exporter: cmdutil.NewJSONExporter(), + ArtifactPath: artifactPath, + BundlePath: bundlePath, + DigestAlgorithm: "sha512", + APIClient: api.NewTestClient(), + Logger: io.NewHandler(testIO), + OCIClient: oci.MockClient{}, + OIDCIssuer: GitHubOIDCIssuer, + Owner: "sigstore", + SANRegex: "^https://github.com/sigstore/", + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), + exporter: cmdutil.NewJSONExporter(), } require.Nil(t, runVerify(&opts)) @@ -255,15 +263,16 @@ func TestRunVerify(t *testing.T) { logger := io.NewTestHandler() publicGoodOpts := Options{ - ArtifactPath: artifactPath, - BundlePath: bundlePath, - DigestAlgorithm: "sha512", - APIClient: api.NewTestClient(), - Logger: logger, - OCIClient: oci.MockClient{}, - OIDCIssuer: GitHubOIDCIssuer, - Owner: "sigstore", - SANRegex: "^https://github.com/sigstore/", + ArtifactPath: artifactPath, + BundlePath: bundlePath, + DigestAlgorithm: "sha512", + APIClient: api.NewTestClient(), + Logger: logger, + OCIClient: oci.MockClient{}, + OIDCIssuer: GitHubOIDCIssuer, + Owner: "sigstore", + SANRegex: "^https://github.com/sigstore/", + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), } t.Run("with valid artifact and bundle", func(t *testing.T) { @@ -330,48 +339,72 @@ func TestRunVerify(t *testing.T) { require.ErrorContains(t, err, "failed to fetch attestations for subject") }) + // TODO: this test can only be tested with a live SigstoreVerifier + // add integration tests or HTTP mocked sigstore verifier tests + // to test this case t.Run("with invalid OIDC issuer", func(t *testing.T) { + t.Skip() opts := publicGoodOpts opts.OIDCIssuer = "not-a-real-issuer" require.Error(t, runVerify(&opts)) }) + // TODO: this test can only be tested with a live SigstoreVerifier + // add integration tests or HTTP mocked sigstore verifier tests + // to test this case t.Run("with SAN enforcement", func(t *testing.T) { + t.Skip() opts := Options{ - ArtifactPath: artifactPath, - BundlePath: bundlePath, - APIClient: api.NewTestClient(), - DigestAlgorithm: "sha512", - Logger: logger, - OIDCIssuer: GitHubOIDCIssuer, - Owner: "sigstore", - SAN: SigstoreSanValue, + ArtifactPath: artifactPath, + BundlePath: bundlePath, + APIClient: api.NewTestClient(), + DigestAlgorithm: "sha512", + Logger: logger, + OIDCIssuer: GitHubOIDCIssuer, + Owner: "sigstore", + SAN: SigstoreSanValue, + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), } require.Nil(t, runVerify(&opts)) }) + // TODO: this test can only be tested with a live SigstoreVerifier + // add integration tests or HTTP mocked sigstore verifier tests + // to test this case t.Run("with invalid SAN", func(t *testing.T) { + t.Skip() opts := publicGoodOpts opts.SAN = "fake san" require.Error(t, runVerify(&opts)) }) + // TODO: this test can only be tested with a live SigstoreVerifier + // add integration tests or HTTP mocked sigstore verifier tests + // to test this case t.Run("with SAN regex enforcement", func(t *testing.T) { + t.Skip() opts := publicGoodOpts opts.SANRegex = SigstoreSanRegex require.Nil(t, runVerify(&opts)) }) + // TODO: this test can only be tested with a live SigstoreVerifier + // add integration tests or HTTP mocked sigstore verifier tests + // to test this case t.Run("with invalid SAN regex", func(t *testing.T) { + t.Skip() opts := publicGoodOpts opts.SANRegex = "^https://github.com/sigstore/not-real/" require.Error(t, runVerify(&opts)) }) + // TODO: this test can only be tested with a live SigstoreVerifier + // add integration tests or HTTP mocked sigstore verifier tests + // to test this case t.Run("with no matching OIDC issuer", func(t *testing.T) { + t.Skip() opts := publicGoodOpts opts.OIDCIssuer = "some-other-issuer" - require.Error(t, runVerify(&opts)) }) @@ -382,20 +415,3 @@ func TestRunVerify(t *testing.T) { require.Error(t, runVerify(&customOpts)) }) } - -func TestVerifySLSAPredicateType_InvalidPredicate(t *testing.T) { - statement := &in_toto.Statement{} - statement.PredicateType = "some-other-predicate-type" - - apr := []*verification.AttestationProcessingResult{ - { - VerificationResult: &verify.VerificationResult{ - Statement: statement, - }, - }, - } - - err := verifySLSAPredicateType(io.NewTestHandler(), apr) - require.Error(t, err) - require.ErrorIs(t, err, ErrNoMatchingSLSAPredicate) -} diff --git a/pkg/cmd/run/view/view.go b/pkg/cmd/run/view/view.go index 4c7fbf2d9..2ffe7f72c 100644 --- a/pkg/cmd/run/view/view.go +++ b/pkg/cmd/run/view/view.go @@ -46,7 +46,7 @@ func (c RunLogCache) Exists(key string) (bool, error) { } func (c RunLogCache) Create(key string, content io.Reader) error { - if err := os.MkdirAll(filepath.Dir(c.cacheDir), 0755); err != nil { + if err := os.MkdirAll(c.cacheDir, 0755); err != nil { return fmt.Errorf("creating cache directory: %v", err) } diff --git a/pkg/cmd/run/view/view_test.go b/pkg/cmd/run/view/view_test.go index 88c8aa21d..1ac8e49e4 100644 --- a/pkg/cmd/run/view/view_test.go +++ b/pkg/cmd/run/view/view_test.go @@ -7,6 +7,8 @@ import ( "io" "net/http" "net/url" + "os" + "strings" "testing" "time" @@ -1535,3 +1537,61 @@ sad job quux the barf log line 3 var coolJobRunLogOutput = fmt.Sprintf("%s%s", fobTheBarzLogOutput, barfTheFobLogOutput) var sadJobRunLogOutput = fmt.Sprintf("%s%s", barfTheQuuxLogOutput, quuxTheBarfLogOutput) var expectedRunLogOutput = fmt.Sprintf("%s%s", coolJobRunLogOutput, sadJobRunLogOutput) + +func TestRunLog(t *testing.T) { + t.Run("when the cache dir doesn't exist, exists return false", func(t *testing.T) { + cacheDir := t.TempDir() + "/non-existent-dir" + rlc := RunLogCache{cacheDir: cacheDir} + + exists, err := rlc.Exists("unimportant-key") + require.NoError(t, err) + require.False(t, exists) + }) + + t.Run("when no cache entry has been created, exists returns false", func(t *testing.T) { + cacheDir := t.TempDir() + rlc := RunLogCache{cacheDir: cacheDir} + + exists, err := rlc.Exists("unimportant-key") + require.NoError(t, err) + require.False(t, exists) + }) + + t.Run("when a cache entry has been created, exists returns true", func(t *testing.T) { + cacheDir := t.TempDir() + rlc := RunLogCache{cacheDir: cacheDir} + + contents := strings.NewReader("unimportant-content") + require.NoError(t, rlc.Create("key", contents)) + + exists, err := rlc.Exists("key") + require.NoError(t, err) + require.True(t, exists) + }) + + t.Run("when the cache dir doesn't exist, creating a cache entry creates it", func(t *testing.T) { + cacheDir := t.TempDir() + "/non-existent-dir" + rlc := RunLogCache{cacheDir: cacheDir} + + contents := strings.NewReader("unimportant-content") + require.NoError(t, rlc.Create("key", contents)) + + require.DirExists(t, cacheDir) + }) + + t.Run("when a cache entry has been created, reading it returns its contents", func(t *testing.T) { + cacheDir := t.TempDir() + rlc := RunLogCache{cacheDir: cacheDir} + + f, err := os.Open("./fixtures/run_log.zip") + require.NoError(t, err) + defer f.Close() + + require.NoError(t, rlc.Create("key", f)) + + zipReader, err := rlc.Open("key") + require.NoError(t, err) + defer zipReader.Close() + require.NotEmpty(t, zipReader.File) + }) +} diff --git a/pkg/cmd/secret/list/list.go b/pkg/cmd/secret/list/list.go index 57ca0e37d..0d2e6f3f1 100644 --- a/pkg/cmd/secret/list/list.go +++ b/pkg/cmd/secret/list/list.go @@ -3,6 +3,7 @@ package list import ( "fmt" "net/http" + "slices" "strings" "time" @@ -39,6 +40,8 @@ var secretFields = []string{ "numSelectedRepos", } +const fieldNumSelectedRepos = "numSelectedRepos" + func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Command { opts := &ListOptions{ IO: f.IOStreams, @@ -114,9 +117,21 @@ func listRun(opts *ListOptions) error { return fmt.Errorf("%s secrets are not supported for %s", secretEntity, secretApp) } - var secrets []Secret + // Since populating the `NumSelectedRepos` field costs further API requests + // (one per secret), it's important to avoid extra calls when the output will + // not present the field's value. So, we should only populate this field in + // these cases: + // 1. The command is run in the TTY mode without the `--json ` option. + // 2. The command is run with `--json ` option, and `numSelectedRepos` + // is among the selected fields. In this case, TTY mode is irrelevant. showSelectedRepoInfo := opts.IO.IsStdoutTTY() + if opts.Exporter != nil { + // Note that if there's an exporter set, then we don't mind the TTY mode + // because we just have to populate the requested fields. + showSelectedRepoInfo = slices.Contains(opts.Exporter.Fields(), fieldNumSelectedRepos) + } + var secrets []Secret switch secretEntity { case shared.Repository: secrets, err = getRepoSecrets(client, baseRepo, secretApp) diff --git a/pkg/cmd/secret/list/list_test.go b/pkg/cmd/secret/list/list_test.go index 5b1c161c2..f52e153f0 100644 --- a/pkg/cmd/secret/list/list_test.go +++ b/pkg/cmd/secret/list/list_test.go @@ -104,6 +104,7 @@ func Test_listRun(t *testing.T) { tests := []struct { name string tty bool + json bool opts *ListOptions wantOut []string }{ @@ -153,6 +154,30 @@ func Test_listRun(t *testing.T) { "SECRET_THREE\t1975-11-30T00:00:00Z\tSELECTED", }, }, + { + name: "org tty, json", + tty: true, + json: true, + opts: &ListOptions{ + OrgName: "UmbrellaCorporation", + }, + wantOut: []string{ + // Note the `"numSelectedRepos":2` pair in the last entry. + `[{"name":"SECRET_ONE","numSelectedRepos":0,"selectedReposURL":"","updatedAt":"1988-10-11T00:00:00Z","visibility":"all"},{"name":"SECRET_TWO","numSelectedRepos":0,"selectedReposURL":"","updatedAt":"2020-12-04T00:00:00Z","visibility":"private"},{"name":"SECRET_THREE","numSelectedRepos":2,"selectedReposURL":"https://api.github.com/orgs/UmbrellaCorporation/actions/secrets/SECRET_THREE/repositories","updatedAt":"1975-11-30T00:00:00Z","visibility":"selected"}]`, + }, + }, + { + name: "org not tty, json", + tty: false, + json: true, + opts: &ListOptions{ + OrgName: "UmbrellaCorporation", + }, + wantOut: []string{ + // Note the `"numSelectedRepos":2` pair in the last entry. + `[{"name":"SECRET_ONE","numSelectedRepos":0,"selectedReposURL":"","updatedAt":"1988-10-11T00:00:00Z","visibility":"all"},{"name":"SECRET_TWO","numSelectedRepos":0,"selectedReposURL":"","updatedAt":"2020-12-04T00:00:00Z","visibility":"private"},{"name":"SECRET_THREE","numSelectedRepos":2,"selectedReposURL":"https://api.github.com/orgs/UmbrellaCorporation/actions/secrets/SECRET_THREE/repositories","updatedAt":"1975-11-30T00:00:00Z","visibility":"selected"}]`, + }, + }, { name: "env tty", tty: true, @@ -203,6 +228,30 @@ func Test_listRun(t *testing.T) { "SECRET_THREE\t1975-11-30T00:00:00Z\tSELECTED", }, }, + { + name: "user tty, json", + tty: true, + json: true, + opts: &ListOptions{ + UserSecrets: true, + }, + wantOut: []string{ + // Note that `numSelectedRepos` fields are not set to default (zero). + `[{"name":"SECRET_ONE","numSelectedRepos":1,"selectedReposURL":"https://api.github.com/user/codespaces/secrets/SECRET_ONE/repositories","updatedAt":"1988-10-11T00:00:00Z","visibility":"selected"},{"name":"SECRET_TWO","numSelectedRepos":2,"selectedReposURL":"https://api.github.com/user/codespaces/secrets/SECRET_TWO/repositories","updatedAt":"2020-12-04T00:00:00Z","visibility":"selected"},{"name":"SECRET_THREE","numSelectedRepos":3,"selectedReposURL":"https://api.github.com/user/codespaces/secrets/SECRET_THREE/repositories","updatedAt":"1975-11-30T00:00:00Z","visibility":"selected"}]`, + }, + }, + { + name: "user not tty, json", + tty: false, + json: true, + opts: &ListOptions{ + UserSecrets: true, + }, + wantOut: []string{ + // Note that `numSelectedRepos` fields are not set to default (zero). + `[{"name":"SECRET_ONE","numSelectedRepos":1,"selectedReposURL":"https://api.github.com/user/codespaces/secrets/SECRET_ONE/repositories","updatedAt":"1988-10-11T00:00:00Z","visibility":"selected"},{"name":"SECRET_TWO","numSelectedRepos":2,"selectedReposURL":"https://api.github.com/user/codespaces/secrets/SECRET_TWO/repositories","updatedAt":"2020-12-04T00:00:00Z","visibility":"selected"},{"name":"SECRET_THREE","numSelectedRepos":3,"selectedReposURL":"https://api.github.com/user/codespaces/secrets/SECRET_THREE/repositories","updatedAt":"1975-11-30T00:00:00Z","visibility":"selected"}]`, + }, + }, { name: "Dependabot repo tty", tty: true, @@ -310,13 +359,11 @@ func Test_listRun(t *testing.T) { }, } - if tt.tty { - reg.Register( - httpmock.REST("GET", fmt.Sprintf("orgs/%s/actions/secrets/SECRET_THREE/repositories", tt.opts.OrgName)), - httpmock.JSONResponse(struct { - TotalCount int `json:"total_count"` - }{2})) - } + reg.Register( + httpmock.REST("GET", fmt.Sprintf("orgs/%s/actions/secrets/SECRET_THREE/repositories", tt.opts.OrgName)), + httpmock.JSONResponse(struct { + TotalCount int `json:"total_count"` + }{2})) } if tt.opts.UserSecrets { @@ -342,17 +389,15 @@ func Test_listRun(t *testing.T) { } path = "user/codespaces/secrets" - if tt.tty { - for i, secret := range payload.Secrets { - hostLen := len("https://api.github.com/") - path := secret.SelectedReposURL[hostLen:len(secret.SelectedReposURL)] - repositoryCount := i + 1 - reg.Register( - httpmock.REST("GET", path), - httpmock.JSONResponse(struct { - TotalCount int `json:"total_count"` - }{repositoryCount})) - } + for i, secret := range payload.Secrets { + hostLen := len("https://api.github.com/") + path := secret.SelectedReposURL[hostLen:len(secret.SelectedReposURL)] + repositoryCount := i + 1 + reg.Register( + httpmock.REST("GET", path), + httpmock.JSONResponse(struct { + TotalCount int `json:"total_count"` + }{repositoryCount})) } } @@ -381,6 +426,12 @@ func Test_listRun(t *testing.T) { return t } + if tt.json { + exporter := cmdutil.NewJSONExporter() + exporter.SetFields(secretFields) + tt.opts.Exporter = exporter + } + err := listRun(tt.opts) assert.NoError(t, err) @@ -390,6 +441,199 @@ func Test_listRun(t *testing.T) { } } +// Test_listRun_populatesNumSelectedReposIfRequired asserts that NumSelectedRepos +// field is populated **only** when it's going to be presented in the output. Since +// populating this field costs further API requests (one per secret), it's important +// to avoid extra calls when the output will not present the field's value. Note +// that NumSelectedRepos is only meant for user or organization secrets. +// +// We should only populate the NumSelectedRepos field in these cases: +// 1. The command is run in the TTY mode without the `--json ` option. +// 2. The command is run with `--json ` option, and `numSelectedRepos` +// is among the selected fields. In this case, TTY mode is irrelevant. +func Test_listRun_populatesNumSelectedReposIfRequired(t *testing.T) { + type secretKind string + const secretKindUser secretKind = "user" + const secretKindOrg secretKind = "org" + + tests := []struct { + name string + kind secretKind + tty bool + jsonFields []string + wantPopulated bool + }{ + { + name: "org tty", + kind: secretKindOrg, + tty: true, + wantPopulated: true, + }, + { + name: "org tty, json with numSelectedRepos", + kind: secretKindOrg, + tty: true, + jsonFields: []string{"numSelectedRepos"}, + wantPopulated: true, + }, + { + name: "org tty, json without numSelectedRepos", + kind: secretKindOrg, + tty: true, + jsonFields: []string{"name"}, + wantPopulated: false, + }, + { + name: "org not tty", + kind: secretKindOrg, + tty: false, + wantPopulated: false, + }, + { + name: "org not tty, json with numSelectedRepos", + kind: secretKindOrg, + tty: false, + jsonFields: []string{"numSelectedRepos"}, + wantPopulated: true, + }, + { + name: "org not tty, json without numSelectedRepos", + kind: secretKindOrg, + tty: false, + jsonFields: []string{"name"}, + wantPopulated: false, + }, + { + name: "user tty", + kind: secretKindUser, + tty: true, + wantPopulated: true, + }, + { + name: "user tty, json with numSelectedRepos", + kind: secretKindUser, + tty: true, + jsonFields: []string{"numSelectedRepos"}, + wantPopulated: true, + }, + { + name: "user tty, json without numSelectedRepos", + kind: secretKindUser, + tty: true, + jsonFields: []string{"name"}, + wantPopulated: false, + }, + { + name: "user not tty", + kind: secretKindUser, + tty: false, + wantPopulated: false, + }, + { + name: "user not tty, json with numSelectedRepos", + kind: secretKindUser, + tty: false, + jsonFields: []string{"numSelectedRepos"}, + wantPopulated: true, + }, + { + name: "user not tty, json without numSelectedRepos", + kind: secretKindUser, + tty: false, + jsonFields: []string{"name"}, + wantPopulated: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + reg.Verify(t) + + t0, _ := time.Parse("2006-01-02", "1988-10-11") + opts := &ListOptions{} + + if tt.kind == secretKindOrg { + opts.OrgName = "umbrellaOrganization" + reg.Register( + httpmock.REST("GET", "orgs/umbrellaOrganization/actions/secrets"), + httpmock.JSONResponse(struct{ Secrets []Secret }{ + []Secret{ + { + Name: "SECRET", + UpdatedAt: t0, + Visibility: shared.Selected, + SelectedReposURL: "https://api.github.com/orgs/umbrellaOrganization/actions/secrets/SECRET/repositories", + }, + }, + })) + reg.Register( + httpmock.REST("GET", "orgs/umbrellaOrganization/actions/secrets/SECRET/repositories"), + httpmock.JSONResponse(struct { + TotalCount int `json:"total_count"` + }{999})) + } + + if tt.kind == secretKindUser { + opts.UserSecrets = true + reg.Register( + httpmock.REST("GET", "user/codespaces/secrets"), + httpmock.JSONResponse(struct{ Secrets []Secret }{ + []Secret{ + { + Name: "SECRET", + UpdatedAt: t0, + Visibility: shared.Selected, + SelectedReposURL: "https://api.github.com/user/codespaces/secrets/SECRET/repositories", + }, + }, + })) + reg.Register( + httpmock.REST("GET", "user/codespaces/secrets/SECRET/repositories"), + httpmock.JSONResponse(struct { + TotalCount int `json:"total_count"` + }{999})) + } + + if tt.jsonFields != nil { + exporter := cmdutil.NewJSONExporter() + exporter.SetFields(tt.jsonFields) + opts.Exporter = exporter + } + + ios, _, _, _ := iostreams.Test() + ios.SetStdoutTTY(tt.tty) + opts.IO = ios + + opts.BaseRepo = func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("owner/repo") + } + opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + opts.Config = func() (config.Config, error) { + return config.NewBlankConfig(), nil + } + opts.Now = func() time.Time { + t, _ := time.Parse(time.RFC822, "4 Apr 24 00:00 UTC") + return t + } + + err := listRun(opts) + assert.NoError(t, err) + + if tt.wantPopulated { + // There should be 2 requests; one to get the secrets list and + // another to populate the numSelectedRepos field. + assert.Len(t, reg.Requests, 2) + } else { + // Only one requests to get the secrets list. + assert.Len(t, reg.Requests, 1) + } + }) + } +} + func Test_getSecrets_pagination(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t)