From 36d622ed48d8c68d471529d1433534bd8f2f1dd2 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 20 Aug 2024 23:42:57 -0400 Subject: [PATCH 01/76] Prototype licensing workflow --- .github/licenses.tmpl | 13 +++++++++++ .github/workflows/licenses.yml | 40 ++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 .github/licenses.tmpl create mode 100644 .github/workflows/licenses.yml diff --git a/.github/licenses.tmpl b/.github/licenses.tmpl new file mode 100644 index 000000000..4a9083e92 --- /dev/null +++ b/.github/licenses.tmpl @@ -0,0 +1,13 @@ +# GitHub CLI dependencies + +The following open source dependencies are used to build the [GitHub CLI _(`gh`)_](https://github.com/cli/cli). + +## Go Packages + +Some packages may only be included on certain architectures or operating systems. + +| **Module** | **Version** | **License** | **License File** | **Package Docs** +| ---------- | ----------- | ----------- | ---------------- | ---------------- +{{- range . }} +| {{ .Name }} | {{ .Version }} | {{ .LicenseName }} | {{ .LicenseURL }} | [pkg.go.dev](https://pkg.go.dev/{{ .Name }}@{{ .Version }}) +{{- end }} diff --git a/.github/workflows/licenses.yml b/.github/workflows/licenses.yml new file mode 100644 index 000000000..1769d5bb3 --- /dev/null +++ b/.github/workflows/licenses.yml @@ -0,0 +1,40 @@ +name: Licensing +on: + push: + tags: + - "v*" +env: + GOPACKAGE: github.com/cli/cli/v2 + LICENSE_DIR: licenses + LICENSE_ARCHIVE: licenses.tgz +jobs: + go-licenses: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version-file: 'go.mod' + + - name: Generate Go license notices + working-directory: ${{ env.LICENSE_DIR }} + run: | + go install github.com/google/go-licenses@latest + go-licenses report "$GOPACKAGE" --template=../.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > README.md + go-licenses save "$GOPACKAGE" --stderrthreshold=ERROR --logtostderr=false + + - name: Upload Go license notices + run: | + tar czf "$LICENSE_ARCHIVE" "$LICENSE_DIR" + gh release upload "$GITHUB_REF_NAME" --clobber -- "$LICENSE_ARCHIVE" + + if gh release view "$GITHUB_REF_NAME" >/dev/null; then + echo "uploading assets to an existing release..." + gh release upload "$GITHUB_REF_NAME" --clobber -- "$LICENSE_ARCHIVE" + else + echo "cannot upload as something else should create the release first..." + exit 1 + fi From e42d44994e870516b70aa48d34ffc8bbd845e1f8 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 20 Aug 2024 23:48:55 -0400 Subject: [PATCH 02/76] Fix working directory error --- .github/workflows/licenses.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/workflows/licenses.yml b/.github/workflows/licenses.yml index 1769d5bb3..e1308f6ee 100644 --- a/.github/workflows/licenses.yml +++ b/.github/workflows/licenses.yml @@ -20,11 +20,10 @@ jobs: go-version-file: 'go.mod' - name: Generate Go license notices - working-directory: ${{ env.LICENSE_DIR }} run: | go install github.com/google/go-licenses@latest - go-licenses report "$GOPACKAGE" --template=../.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > README.md - go-licenses save "$GOPACKAGE" --stderrthreshold=ERROR --logtostderr=false + go-licenses report "$GOPACKAGE" --template=../.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > "$LICENSE_DIR"/README.md + go-licenses save "$GOPACKAGE" --stderrthreshold=ERROR --logtostderr=false --save_path "$LICENSE_DIR" - name: Upload Go license notices run: | From 67d7b0752ffa1ba76b12a5d0155f7201f6089a9e Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 20 Aug 2024 23:51:27 -0400 Subject: [PATCH 03/76] Fix working directory error --- .github/workflows/licenses.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/licenses.yml b/.github/workflows/licenses.yml index e1308f6ee..3b3b6eccd 100644 --- a/.github/workflows/licenses.yml +++ b/.github/workflows/licenses.yml @@ -22,6 +22,7 @@ jobs: - name: Generate Go license notices run: | go install github.com/google/go-licenses@latest + mkdir "$LICENSE_DIR" go-licenses report "$GOPACKAGE" --template=../.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > "$LICENSE_DIR"/README.md go-licenses save "$GOPACKAGE" --stderrthreshold=ERROR --logtostderr=false --save_path "$LICENSE_DIR" From 98294a6840e186ac6785e82f53bee38c53ba84c6 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 20 Aug 2024 23:53:44 -0400 Subject: [PATCH 04/76] Fix go-licenses package targeting --- .github/workflows/licenses.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/workflows/licenses.yml b/.github/workflows/licenses.yml index 3b3b6eccd..c6aa1f151 100644 --- a/.github/workflows/licenses.yml +++ b/.github/workflows/licenses.yml @@ -4,7 +4,6 @@ on: tags: - "v*" env: - GOPACKAGE: github.com/cli/cli/v2 LICENSE_DIR: licenses LICENSE_ARCHIVE: licenses.tgz jobs: @@ -23,8 +22,8 @@ jobs: run: | go install github.com/google/go-licenses@latest mkdir "$LICENSE_DIR" - go-licenses report "$GOPACKAGE" --template=../.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > "$LICENSE_DIR"/README.md - go-licenses save "$GOPACKAGE" --stderrthreshold=ERROR --logtostderr=false --save_path "$LICENSE_DIR" + go-licenses report ./... --template=../.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > "$LICENSE_DIR"/README.md + go-licenses save ./... --stderrthreshold=ERROR --logtostderr=false --save_path "$LICENSE_DIR" - name: Upload Go license notices run: | From df7c38f8d52835f9d5c9604eb19214f48931d052 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 20 Aug 2024 23:56:47 -0400 Subject: [PATCH 05/76] Fix Go root path issue --- .github/workflows/licenses.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/licenses.yml b/.github/workflows/licenses.yml index c6aa1f151..ba88e3cd3 100644 --- a/.github/workflows/licenses.yml +++ b/.github/workflows/licenses.yml @@ -22,8 +22,8 @@ jobs: run: | go install github.com/google/go-licenses@latest mkdir "$LICENSE_DIR" - go-licenses report ./... --template=../.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > "$LICENSE_DIR"/README.md - go-licenses save ./... --stderrthreshold=ERROR --logtostderr=false --save_path "$LICENSE_DIR" + GOROOT=$(go env GOROOT) go-licenses report ./... --template=../.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > "$LICENSE_DIR"/README.md + GOROOT=$(go env GOROOT) go-licenses save ./... --stderrthreshold=ERROR --logtostderr=false --save_path "$LICENSE_DIR" - name: Upload Go license notices run: | From 341864ec25f36481f0b198ef20907bacb32825ca Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 20 Aug 2024 23:58:44 -0400 Subject: [PATCH 06/76] Fix go-licenses template path error --- .github/workflows/licenses.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/licenses.yml b/.github/workflows/licenses.yml index ba88e3cd3..68fefacf6 100644 --- a/.github/workflows/licenses.yml +++ b/.github/workflows/licenses.yml @@ -22,7 +22,7 @@ jobs: run: | go install github.com/google/go-licenses@latest mkdir "$LICENSE_DIR" - GOROOT=$(go env GOROOT) go-licenses report ./... --template=../.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > "$LICENSE_DIR"/README.md + GOROOT=$(go env GOROOT) go-licenses report ./... --template=.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > "$LICENSE_DIR"/README.md GOROOT=$(go env GOROOT) go-licenses save ./... --stderrthreshold=ERROR --logtostderr=false --save_path "$LICENSE_DIR" - name: Upload Go license notices From 0ca266bc9a0f0e60d4a9de97c5a311be8c2e08f7 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 21 Aug 2024 00:02:58 -0400 Subject: [PATCH 07/76] Fix go-licenses ordering --- .github/workflows/licenses.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/licenses.yml b/.github/workflows/licenses.yml index 68fefacf6..62bf7b8dd 100644 --- a/.github/workflows/licenses.yml +++ b/.github/workflows/licenses.yml @@ -21,9 +21,8 @@ jobs: - name: Generate Go license notices run: | go install github.com/google/go-licenses@latest - mkdir "$LICENSE_DIR" - GOROOT=$(go env GOROOT) go-licenses report ./... --template=.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > "$LICENSE_DIR"/README.md GOROOT=$(go env GOROOT) go-licenses save ./... --stderrthreshold=ERROR --logtostderr=false --save_path "$LICENSE_DIR" + GOROOT=$(go env GOROOT) go-licenses report ./... --template=.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > "$LICENSE_DIR"/README.md - name: Upload Go license notices run: | From 23acba65f9c8642115be6455ac91e64cb199871a Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 21 Aug 2024 00:07:28 -0400 Subject: [PATCH 08/76] Fix gh token usage --- .github/workflows/licenses.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/licenses.yml b/.github/workflows/licenses.yml index 62bf7b8dd..479349714 100644 --- a/.github/workflows/licenses.yml +++ b/.github/workflows/licenses.yml @@ -25,6 +25,8 @@ jobs: GOROOT=$(go env GOROOT) go-licenses report ./... --template=.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > "$LICENSE_DIR"/README.md - name: Upload Go license notices + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | tar czf "$LICENSE_ARCHIVE" "$LICENSE_DIR" gh release upload "$GITHUB_REF_NAME" --clobber -- "$LICENSE_ARCHIVE" From 704de0cf372972167f4b127f2a1a031c4c40e03e Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Tue, 29 Oct 2024 15:33:24 -0600 Subject: [PATCH 09/76] start building a separate policy struct Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index f41b2f66b..60a666ad0 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -8,6 +8,7 @@ import ( "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" "github.com/sigstore/sigstore-go/pkg/verify" + "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" ) @@ -18,6 +19,32 @@ const ( hostRegex = `^[a-zA-Z0-9-]+\.[a-zA-Z0-9-]+.*$` ) +type ExpectedExtensions struct { + RunnerEnvironment string + SAN string + buildSourceRepo string + SignerWorkflow string +} + +type Policy struct { + ExpectedExtensions ExpectedExtensions + ExpectedPredicateType string + ExpectedSigstoreInstance string +} + +func buildPolicy(opts *Options, a artifact.DigestedArtifact) Policy { + return Policy{} +} + +func (p *Policy) Verify(a []*api.Attestation) (bool, string) { + filtered := verification.FilterAttestations(p.ExpectedPredicateType, a) + if len(filtered) == 0 { + return false, fmt.Sprintf("✗ No attestations found with predicate type: %s\n", p.ExpectedPredicateType) + } + + return true, "" +} + func expandToGitHubURL(tenant, ownerOrRepo string) string { if tenant == "" { return fmt.Sprintf("(?i)^https://github.com/%s/", ownerOrRepo) From e5b2b09a6e1c1963f64efcca379963d71bd5de2a Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Tue, 29 Oct 2024 16:41:17 -0600 Subject: [PATCH 10/76] move policy functions into methods Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 100 +++++++++++++-------------- pkg/cmd/attestation/verify/verify.go | 28 ++++++++ 2 files changed, 78 insertions(+), 50 deletions(-) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index 60a666ad0..abebba9e3 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -21,19 +21,58 @@ const ( type ExpectedExtensions struct { RunnerEnvironment string + SANRegex string SAN string - buildSourceRepo string + BuildSourceRepo string SignerWorkflow string } +type SigstoreInstance string + +const ( + PublicGood SigstoreInstance = "public-good" + GitHub SigstoreInstance = "github" + Custom SigstoreInstance = "custom" +) + type Policy struct { ExpectedExtensions ExpectedExtensions ExpectedPredicateType string ExpectedSigstoreInstance string + Artifact artifact.DigestedArtifact } -func buildPolicy(opts *Options, a artifact.DigestedArtifact) Policy { - return Policy{} +func newPolicy(opts *Options, a artifact.DigestedArtifact) (Policy, error) { + p := Policy{} + + if opts.SignerRepo != "" { + signedRepoRegex := expandToGitHubURL(opts.Tenant, opts.SignerRepo) + p.ExpectedExtensions.SANRegex = signedRepoRegex + } else if opts.SignerWorkflow != "" { + validatedWorkflowRegex, err := validateSignerWorkflow(opts) + if err != nil { + return Policy{}, err + } + + p.ExpectedExtensions.SANRegex = validatedWorkflowRegex + } else { + p.ExpectedExtensions.SANRegex = opts.SANRegex + p.ExpectedExtensions.SAN = opts.SAN + } + + if opts.DenySelfHostedRunner { + p.ExpectedExtensions.RunnerEnvironment = GitHubRunner + } else { + p.ExpectedExtensions.RunnerEnvironment = "*" + } + + if opts.Repo != "" { + if opts.Tenant != "" { + p.ExpectedExtensions.BuildSourceRepo = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Repo) + } + p.ExpectedExtensions.BuildSourceRepo = fmt.Sprintf("https://github.com/%s", opts.Repo) + } + return p, nil } func (p *Policy) Verify(a []*api.Attestation) (bool, string) { @@ -52,26 +91,8 @@ func expandToGitHubURL(tenant, ownerOrRepo string) string { return fmt.Sprintf("(?i)^https://%s.ghe.com/%s/", tenant, ownerOrRepo) } -func buildSANMatcher(opts *Options) (verify.SubjectAlternativeNameMatcher, error) { - if opts.SignerRepo != "" { - signedRepoRegex := expandToGitHubURL(opts.Tenant, opts.SignerRepo) - return verify.NewSANMatcher("", signedRepoRegex) - } else if opts.SignerWorkflow != "" { - validatedWorkflowRegex, err := validateSignerWorkflow(opts) - if err != nil { - return verify.SubjectAlternativeNameMatcher{}, err - } - - return verify.NewSANMatcher("", validatedWorkflowRegex) - } else if opts.SAN != "" || opts.SANRegex != "" { - return verify.NewSANMatcher(opts.SAN, opts.SANRegex) - } - - return verify.SubjectAlternativeNameMatcher{}, nil -} - -func buildCertificateIdentityOption(opts *Options, runnerEnv string) (verify.PolicyOption, error) { - sanMatcher, err := buildSANMatcher(opts) +func (p *Policy) buildCertificateIdentityOption() (verify.PolicyOption, error) { + sanMatcher, err := verify.NewSANMatcher(p.ExpectedExtensions.SAN, p.ExpectedExtensions.SANRegex) if err != nil { return nil, err } @@ -83,7 +104,7 @@ func buildCertificateIdentityOption(opts *Options, runnerEnv string) (verify.Pol } extensions := certificate.Extensions{ - RunnerEnvironment: runnerEnv, + RunnerEnvironment: p.ExpectedExtensions.RunnerEnvironment, } certId, err := verify.NewCertificateIdentity(sanMatcher, issuerMatcher, extensions) @@ -94,34 +115,13 @@ func buildCertificateIdentityOption(opts *Options, runnerEnv string) (verify.Pol return verify.WithCertificateIdentity(certId), nil } -func buildVerifyCertIdOption(opts *Options) (verify.PolicyOption, error) { - if opts.DenySelfHostedRunner { - withGHRunner, err := buildCertificateIdentityOption(opts, GitHubRunner) - if err != nil { - return nil, err - } - - return withGHRunner, nil - } - - // if Extensions.RunnerEnvironment value is set to the empty string - // through the second function argument, - // no certificate matching will happen on the RunnerEnvironment field - withAnyRunner, err := buildCertificateIdentityOption(opts, "") - if err != nil { - return nil, err - } - - return withAnyRunner, nil -} - -func buildVerifyPolicy(opts *Options, a artifact.DigestedArtifact) (verify.PolicyBuilder, error) { - artifactDigestPolicyOption, err := verification.BuildDigestPolicyOption(a) +func (p *Policy) SigstorePolicy() (verify.PolicyBuilder, error) { + artifactDigestPolicyOption, err := verification.BuildDigestPolicyOption(p.Artifact) if err != nil { return verify.PolicyBuilder{}, err } - certIdOption, err := buildVerifyCertIdOption(opts) + certIdOption, err := p.buildCertificateIdentityOption() if err != nil { return verify.PolicyBuilder{}, err } @@ -143,12 +143,12 @@ func validateSignerWorkflow(opts *Options) (string, error) { } if match { - return addSchemeToRegex(opts.SignerWorkflow), nil + return fmt.Sprintf("^https://%s", opts.SignerWorkflow), nil } if opts.Hostname == "" { return "", errors.New("unknown host") } - return addSchemeToRegex(fmt.Sprintf("%s/%s", opts.Hostname, opts.SignerWorkflow)), nil + return fmt.Sprintf("^https://%s/%s/%s", opts.Hostname, opts.SignerWorkflow), nil } diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index d14081dd8..8b58296f3 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -364,3 +364,31 @@ func buildTableVerifyContent(tenant string, results []*verification.AttestationP return content, nil } + +func verifyAll(opts *Options, artifact artifact.DigestedArtifact, attestations []*api.Attestation) error { + policy, err := newPolicy(opts, artifact) + if err != nil { + opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build verification policy")) + return err + } + + sp, err := policy.SigstorePolicy() + if err != nil { + opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build verification policy")) + return err + } + + sigstoreRes := opts.SigstoreVerifier.Verify(attestations, sp) + if sigstoreRes.Error != nil { + opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Verification failed")) + return sigstoreRes.Error + } + + // Verify extensions + if err := verification.VerifyCertExtensions(sigstoreRes.VerifyResults, opts.Tenant, opts.Owner, opts.Repo, opts.OIDCIssuer); err != nil { + opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Verification failed")) + return err + } + + return nil +} From e16b69bd08d6c1b9e4a4c634398d11fbdf960181 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Tue, 29 Oct 2024 17:27:47 -0600 Subject: [PATCH 11/76] cert extension funcs are now policy methods Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 103 +++++++++++++++++++--- pkg/cmd/attestation/verify/policy_test.go | 3 +- pkg/cmd/attestation/verify/verify.go | 36 +++----- 3 files changed, 101 insertions(+), 41 deletions(-) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index abebba9e3..b4672213d 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "regexp" + "strings" "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" "github.com/sigstore/sigstore-go/pkg/verify" @@ -20,11 +21,13 @@ const ( ) type ExpectedExtensions struct { - RunnerEnvironment string - SANRegex string - SAN string - BuildSourceRepo string - SignerWorkflow string + RunnerEnvironment string + SANRegex string + SAN string + BuildSourceRepoURI string + SignerWorkflow string + SourceRepositoryOwnerURI string + SourceRepositoryURI string } type SigstoreInstance string @@ -40,10 +43,13 @@ type Policy struct { ExpectedPredicateType string ExpectedSigstoreInstance string Artifact artifact.DigestedArtifact + OIDCIssuer string } func newPolicy(opts *Options, a artifact.DigestedArtifact) (Policy, error) { - p := Policy{} + p := Policy{ + Artifact: a, + } if opts.SignerRepo != "" { signedRepoRegex := expandToGitHubURL(opts.Tenant, opts.SignerRepo) @@ -68,10 +74,25 @@ func newPolicy(opts *Options, a artifact.DigestedArtifact) (Policy, error) { if opts.Repo != "" { if opts.Tenant != "" { - p.ExpectedExtensions.BuildSourceRepo = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Repo) + p.ExpectedExtensions.BuildSourceRepoURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Repo) } - p.ExpectedExtensions.BuildSourceRepo = fmt.Sprintf("https://github.com/%s", opts.Repo) + p.ExpectedExtensions.BuildSourceRepoURI = fmt.Sprintf("https://github.com/%s", opts.Repo) } + + if opts.Tenant != "" { + p.ExpectedExtensions.SourceRepositoryOwnerURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Owner) + } else { + p.ExpectedExtensions.SourceRepositoryOwnerURI = fmt.Sprintf("https://github.com/%s", opts.Owner) + } + + // if issuer is anything other than the default, use the user-provided value; + // otherwise, select the appropriate default based on the tenant + if opts.Tenant != "" { + p.OIDCIssuer = fmt.Sprintf(verification.GitHubTenantOIDCIssuer, opts.Tenant) + } else { + p.OIDCIssuer = opts.OIDCIssuer + } + return p, nil } @@ -115,6 +136,16 @@ func (p *Policy) buildCertificateIdentityOption() (verify.PolicyOption, error) { return verify.WithCertificateIdentity(certId), nil } +func (p *Policy) VerifyPredicateType(a []*api.Attestation) ([]*api.Attestation, error) { + filteredAttestations := verification.FilterAttestations(p.ExpectedPredicateType, a) + + if len(filteredAttestations) == 0 { + return nil, fmt.Errorf("✗ No attestations found with predicate type: %s\n", p.ExpectedPredicateType) + } + + return filteredAttestations, nil +} + func (p *Policy) SigstorePolicy() (verify.PolicyBuilder, error) { artifactDigestPolicyOption, err := verification.BuildDigestPolicyOption(p.Artifact) if err != nil { @@ -130,10 +161,6 @@ func (p *Policy) SigstorePolicy() (verify.PolicyBuilder, error) { return policy, nil } -func addSchemeToRegex(s string) string { - return fmt.Sprintf("^https://%s", s) -} - func validateSignerWorkflow(opts *Options) (string, error) { // we expect a provided workflow argument be in the format [HOST/]///path/to/workflow.yml // if the provided workflow does not contain a host, set the host @@ -150,5 +177,55 @@ func validateSignerWorkflow(opts *Options) (string, error) { return "", errors.New("unknown host") } - return fmt.Sprintf("^https://%s/%s/%s", opts.Hostname, opts.SignerWorkflow), nil + return fmt.Sprintf("^https://%s/%s", opts.Hostname, opts.SignerWorkflow), nil +} + +func (p *Policy) VerifyCertExtensions(results []*verification.AttestationProcessingResult) error { + if len(results) == 0 { + return errors.New("no attestations proccessing results") + } + + var atLeastOneVerified bool + for _, attestation := range results { + if err := p.verifyCertExtensions(attestation); err != nil { + return err + } + atLeastOneVerified = true + } + + if atLeastOneVerified { + return nil + } else { + return verification.ErrNoAttestationsVerified + } +} + +func (p *Policy) verifyCertExtensions(attestation *verification.AttestationProcessingResult) error { + if p.ExpectedExtensions.SourceRepositoryOwnerURI != "" { + sourceRepositoryOwnerURI := attestation.VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI + if !strings.EqualFold(p.ExpectedExtensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) { + return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", p.ExpectedExtensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) + } + } + + // if repo is set, check the SourceRepositoryURI field + if p.ExpectedExtensions.SourceRepositoryURI != "" { + sourceRepositoryURI := attestation.VerificationResult.Signature.Certificate.Extensions.SourceRepositoryURI + if !strings.EqualFold(p.ExpectedExtensions.SourceRepositoryURI, sourceRepositoryURI) { + return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", p.ExpectedExtensions.SourceRepositoryURI, sourceRepositoryURI) + } + } + + if p.OIDCIssuer != "" { + certIssuer := attestation.VerificationResult.Signature.Certificate.Extensions.Issuer + if !strings.EqualFold(p.OIDCIssuer, certIssuer) { + if strings.Index(certIssuer, p.OIDCIssuer+"/") == 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", p.OIDCIssuer, certIssuer) + } else { + return fmt.Errorf("expected Issuer to be %s, got %s", p.OIDCIssuer, certIssuer) + } + } + } + + return nil } diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index ae1e52955..70c3079e1 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -26,7 +26,7 @@ func TestBuildPolicy(t *testing.T) { SANRegex: "^https://github.com/sigstore/", } - _, err = buildVerifyPolicy(opts, *artifact) + _, err = newPolicy(opts, *artifact) require.NoError(t, err) } @@ -87,7 +87,6 @@ func TestValidateSignerWorkflow(t *testing.T) { workflowRegex, err := validateSignerWorkflow(opts) require.NoError(t, err) require.Equal(t, tc.expectedWorkflowRegex, workflowRegex) - } } diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 8b58296f3..a873d6c96 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -255,21 +255,9 @@ func runVerify(opts *Options) error { attestations = filteredAttestations } - policy, err := buildVerifyPolicy(opts, *artifact) + sigstoreResults, err := verifyAll(opts, *artifact, attestations) if err != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build verification policy")) - return err - } - - sigstoreRes := opts.SigstoreVerifier.Verify(attestations, policy) - if sigstoreRes.Error != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Verification failed")) - return sigstoreRes.Error - } - - // Verify extensions - if err := verification.VerifyCertExtensions(sigstoreRes.VerifyResults, opts.Tenant, opts.Owner, opts.Repo, opts.OIDCIssuer); err != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Verification failed")) + opts.Logger.Println(opts.Logger.ColorScheme.Red(err.Error())) return err } @@ -278,7 +266,7 @@ func runVerify(opts *Options) error { // If an exporter is provided with the --json flag, write the results to the terminal in JSON format if opts.exporter != nil { // print the results to the terminal as an array of JSON objects - if err = opts.exporter.Write(opts.Logger.IO, sigstoreRes.VerifyResults); err != nil { + if err = opts.exporter.Write(opts.Logger.IO, sigstoreResults.VerifyResults); err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to write JSON output")) return err } @@ -288,7 +276,7 @@ func runVerify(opts *Options) error { opts.Logger.Printf("%s was attested by:\n", artifact.DigestWithAlg()) // Otherwise print the results to the terminal in a table - tableContent, err := buildTableVerifyContent(opts.Tenant, sigstoreRes.VerifyResults) + tableContent, err := buildTableVerifyContent(opts.Tenant, sigstoreResults.VerifyResults) if err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("failed to parse results")) return err @@ -365,30 +353,26 @@ func buildTableVerifyContent(tenant string, results []*verification.AttestationP return content, nil } -func verifyAll(opts *Options, artifact artifact.DigestedArtifact, attestations []*api.Attestation) error { +func verifyAll(opts *Options, artifact artifact.DigestedArtifact, attestations []*api.Attestation) (*verification.SigstoreResults, error) { policy, err := newPolicy(opts, artifact) if err != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build verification policy")) - return err + return nil, fmt.Errorf("✗ Failed to build verification policy") } sp, err := policy.SigstorePolicy() if err != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build verification policy")) - return err + return nil, fmt.Errorf("✗ Failed to build Sigstore verification policy") } sigstoreRes := opts.SigstoreVerifier.Verify(attestations, sp) if sigstoreRes.Error != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Verification failed")) - return sigstoreRes.Error + return nil, fmt.Errorf("✗ Sigstore verification failed") } // Verify extensions if err := verification.VerifyCertExtensions(sigstoreRes.VerifyResults, opts.Tenant, opts.Owner, opts.Repo, opts.OIDCIssuer); err != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Verification failed")) - return err + return nil, fmt.Errorf("✗ Policy verification failed") } - return nil + return sigstoreRes, nil } From 41c3ba5fa74eedbe27740cd3cc45cfffd77812d0 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Tue, 29 Oct 2024 18:19:19 -0600 Subject: [PATCH 12/76] drop sigstore instance for now Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 63 ++++++++++++---------------- 1 file changed, 27 insertions(+), 36 deletions(-) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index b4672213d..2278b1cb5 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -20,7 +20,7 @@ const ( hostRegex = `^[a-zA-Z0-9-]+\.[a-zA-Z0-9-]+.*$` ) -type ExpectedExtensions struct { +type Extensions struct { RunnerEnvironment string SANRegex string SAN string @@ -30,20 +30,11 @@ type ExpectedExtensions struct { SourceRepositoryURI string } -type SigstoreInstance string - -const ( - PublicGood SigstoreInstance = "public-good" - GitHub SigstoreInstance = "github" - Custom SigstoreInstance = "custom" -) - type Policy struct { - ExpectedExtensions ExpectedExtensions - ExpectedPredicateType string - ExpectedSigstoreInstance string - Artifact artifact.DigestedArtifact - OIDCIssuer string + Extensions Extensions + PredicateType string + Artifact artifact.DigestedArtifact + OIDCIssuer string } func newPolicy(opts *Options, a artifact.DigestedArtifact) (Policy, error) { @@ -53,36 +44,36 @@ func newPolicy(opts *Options, a artifact.DigestedArtifact) (Policy, error) { if opts.SignerRepo != "" { signedRepoRegex := expandToGitHubURL(opts.Tenant, opts.SignerRepo) - p.ExpectedExtensions.SANRegex = signedRepoRegex + p.Extensions.SANRegex = signedRepoRegex } else if opts.SignerWorkflow != "" { validatedWorkflowRegex, err := validateSignerWorkflow(opts) if err != nil { return Policy{}, err } - p.ExpectedExtensions.SANRegex = validatedWorkflowRegex + p.Extensions.SANRegex = validatedWorkflowRegex } else { - p.ExpectedExtensions.SANRegex = opts.SANRegex - p.ExpectedExtensions.SAN = opts.SAN + p.Extensions.SANRegex = opts.SANRegex + p.Extensions.SAN = opts.SAN } if opts.DenySelfHostedRunner { - p.ExpectedExtensions.RunnerEnvironment = GitHubRunner + p.Extensions.RunnerEnvironment = GitHubRunner } else { - p.ExpectedExtensions.RunnerEnvironment = "*" + p.Extensions.RunnerEnvironment = "*" } if opts.Repo != "" { if opts.Tenant != "" { - p.ExpectedExtensions.BuildSourceRepoURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Repo) + p.Extensions.BuildSourceRepoURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Repo) } - p.ExpectedExtensions.BuildSourceRepoURI = fmt.Sprintf("https://github.com/%s", opts.Repo) + p.Extensions.BuildSourceRepoURI = fmt.Sprintf("https://github.com/%s", opts.Repo) } if opts.Tenant != "" { - p.ExpectedExtensions.SourceRepositoryOwnerURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Owner) + p.Extensions.SourceRepositoryOwnerURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Owner) } else { - p.ExpectedExtensions.SourceRepositoryOwnerURI = fmt.Sprintf("https://github.com/%s", opts.Owner) + p.Extensions.SourceRepositoryOwnerURI = fmt.Sprintf("https://github.com/%s", opts.Owner) } // if issuer is anything other than the default, use the user-provided value; @@ -97,9 +88,9 @@ func newPolicy(opts *Options, a artifact.DigestedArtifact) (Policy, error) { } func (p *Policy) Verify(a []*api.Attestation) (bool, string) { - filtered := verification.FilterAttestations(p.ExpectedPredicateType, a) + filtered := verification.FilterAttestations(p.PredicateType, a) if len(filtered) == 0 { - return false, fmt.Sprintf("✗ No attestations found with predicate type: %s\n", p.ExpectedPredicateType) + return false, fmt.Sprintf("✗ No attestations found with predicate type: %s\n", p.PredicateType) } return true, "" @@ -113,7 +104,7 @@ func expandToGitHubURL(tenant, ownerOrRepo string) string { } func (p *Policy) buildCertificateIdentityOption() (verify.PolicyOption, error) { - sanMatcher, err := verify.NewSANMatcher(p.ExpectedExtensions.SAN, p.ExpectedExtensions.SANRegex) + sanMatcher, err := verify.NewSANMatcher(p.Extensions.SAN, p.Extensions.SANRegex) if err != nil { return nil, err } @@ -125,7 +116,7 @@ func (p *Policy) buildCertificateIdentityOption() (verify.PolicyOption, error) { } extensions := certificate.Extensions{ - RunnerEnvironment: p.ExpectedExtensions.RunnerEnvironment, + RunnerEnvironment: p.Extensions.RunnerEnvironment, } certId, err := verify.NewCertificateIdentity(sanMatcher, issuerMatcher, extensions) @@ -137,10 +128,10 @@ func (p *Policy) buildCertificateIdentityOption() (verify.PolicyOption, error) { } func (p *Policy) VerifyPredicateType(a []*api.Attestation) ([]*api.Attestation, error) { - filteredAttestations := verification.FilterAttestations(p.ExpectedPredicateType, a) + filteredAttestations := verification.FilterAttestations(p.PredicateType, a) if len(filteredAttestations) == 0 { - return nil, fmt.Errorf("✗ No attestations found with predicate type: %s\n", p.ExpectedPredicateType) + return nil, fmt.Errorf("✗ No attestations found with predicate type: %s\n", p.PredicateType) } return filteredAttestations, nil @@ -201,18 +192,18 @@ func (p *Policy) VerifyCertExtensions(results []*verification.AttestationProcess } func (p *Policy) verifyCertExtensions(attestation *verification.AttestationProcessingResult) error { - if p.ExpectedExtensions.SourceRepositoryOwnerURI != "" { + if p.Extensions.SourceRepositoryOwnerURI != "" { sourceRepositoryOwnerURI := attestation.VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI - if !strings.EqualFold(p.ExpectedExtensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) { - return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", p.ExpectedExtensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) + if !strings.EqualFold(p.Extensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) { + return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", p.Extensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) } } // if repo is set, check the SourceRepositoryURI field - if p.ExpectedExtensions.SourceRepositoryURI != "" { + if p.Extensions.SourceRepositoryURI != "" { sourceRepositoryURI := attestation.VerificationResult.Signature.Certificate.Extensions.SourceRepositoryURI - if !strings.EqualFold(p.ExpectedExtensions.SourceRepositoryURI, sourceRepositoryURI) { - return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", p.ExpectedExtensions.SourceRepositoryURI, sourceRepositoryURI) + if !strings.EqualFold(p.Extensions.SourceRepositoryURI, sourceRepositoryURI) { + return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", p.Extensions.SourceRepositoryURI, sourceRepositoryURI) } } From 3378b546da257a82795c02c7f8493eca7fff7693 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Wed, 30 Oct 2024 12:58:40 -0600 Subject: [PATCH 13/76] simplify if else logic Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index 2278b1cb5..d23cf32df 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -212,9 +212,8 @@ func (p *Policy) verifyCertExtensions(attestation *verification.AttestationProce if !strings.EqualFold(p.OIDCIssuer, certIssuer) { if strings.Index(certIssuer, p.OIDCIssuer+"/") == 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", p.OIDCIssuer, certIssuer) - } else { - return fmt.Errorf("expected Issuer to be %s, got %s", p.OIDCIssuer, certIssuer) } + return fmt.Errorf("expected Issuer to be %s, got %s", p.OIDCIssuer, certIssuer) } } From b44c9d3003f1e8e6869d8aedba6943a4f5ae0b6e Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Wed, 30 Oct 2024 15:23:50 -0600 Subject: [PATCH 14/76] undo policy method changes Signed-off-by: Meredith Lancaster --- .../attestation/verification/extensions.go | 56 ++--- pkg/cmd/attestation/verification/policy.go | 17 ++ pkg/cmd/attestation/verify/policy.go | 233 ++++++------------ pkg/cmd/attestation/verify/policy_test.go | 2 +- pkg/cmd/attestation/verify/verify.go | 50 ++-- 5 files changed, 131 insertions(+), 227 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions.go b/pkg/cmd/attestation/verification/extensions.go index 94ba88208..13c83651e 100644 --- a/pkg/cmd/attestation/verification/extensions.go +++ b/pkg/cmd/attestation/verification/extensions.go @@ -11,14 +11,14 @@ var ( GitHubTenantOIDCIssuer = "https://token.actions.%s.ghe.com" ) -func VerifyCertExtensions(results []*AttestationProcessingResult, tenant, owner, repo, issuer string) error { +func VerifyCertExtensions(results []*AttestationProcessingResult, ec EnforcementCriteria) error { if len(results) == 0 { return errors.New("no attestations proccessing results") } var atLeastOneVerified bool for _, attestation := range results { - if err := verifyCertExtensions(attestation, tenant, owner, repo, issuer); err != nil { + if err := verifyCertExtensions(attestation, ec); err != nil { return err } atLeastOneVerified = true @@ -31,51 +31,31 @@ func VerifyCertExtensions(results []*AttestationProcessingResult, tenant, owner, } } -func verifyCertExtensions(attestation *AttestationProcessingResult, tenant, owner, repo, issuer string) error { - var want string - - if tenant == "" { - want = fmt.Sprintf("https://github.com/%s", owner) - } else { - want = fmt.Sprintf("https://%s.ghe.com/%s", tenant, owner) - } - sourceRepositoryOwnerURI := attestation.VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI - if !strings.EqualFold(want, sourceRepositoryOwnerURI) { - return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", want, sourceRepositoryOwnerURI) +func verifyCertExtensions(attestation *AttestationProcessingResult, c EnforcementCriteria) error { + if c.Extensions.SourceRepositoryOwnerURI != "" { + sourceRepositoryOwnerURI := attestation.VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI + if !strings.EqualFold(c.Extensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) { + return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", c.Extensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) + } } // if repo is set, check the SourceRepositoryURI field - if repo != "" { - if tenant == "" { - want = fmt.Sprintf("https://github.com/%s", repo) - } else { - want = fmt.Sprintf("https://%s.ghe.com/%s", tenant, repo) - } - + if c.Extensions.SourceRepositoryURI != "" { sourceRepositoryURI := attestation.VerificationResult.Signature.Certificate.Extensions.SourceRepositoryURI - if !strings.EqualFold(want, sourceRepositoryURI) { - return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", want, sourceRepositoryURI) + if !strings.EqualFold(c.Extensions.SourceRepositoryURI, sourceRepositoryURI) { + return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", c.Extensions.SourceRepositoryURI, sourceRepositoryURI) } } // if issuer is anything other than the default, use the user-provided value; // otherwise, select the appropriate default based on the tenant - if issuer != GitHubOIDCIssuer { - want = issuer - } else { - if tenant != "" { - want = fmt.Sprintf(GitHubTenantOIDCIssuer, tenant) - } else { - want = GitHubOIDCIssuer - } - } - - certIssuer := attestation.VerificationResult.Signature.Certificate.Extensions.Issuer - if !strings.EqualFold(want, certIssuer) { - if strings.Index(certIssuer, want+"/") == 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", want, certIssuer) - } else { - return fmt.Errorf("expected Issuer to be %s, got %s", want, certIssuer) + if c.OIDCIssuer != "" { + certIssuer := attestation.VerificationResult.Signature.Certificate.Extensions.Issuer + if !strings.EqualFold(c.OIDCIssuer, certIssuer) { + if strings.Index(certIssuer, c.OIDCIssuer+"/") == 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", c.OIDCIssuer, certIssuer) + } + return fmt.Errorf("expected Issuer to be %s, got %s", c.OIDCIssuer, certIssuer) } } diff --git a/pkg/cmd/attestation/verification/policy.go b/pkg/cmd/attestation/verification/policy.go index 91b54c75e..21210e730 100644 --- a/pkg/cmd/attestation/verification/policy.go +++ b/pkg/cmd/attestation/verification/policy.go @@ -18,3 +18,20 @@ func BuildDigestPolicyOption(a artifact.DigestedArtifact) (verify.ArtifactPolicy } return verify.WithArtifactDigest(a.Algorithm(), decoded), nil } + +type Extensions struct { + RunnerEnvironment string + SANRegex string + SAN string + BuildSourceRepoURI string + SignerWorkflow string + SourceRepositoryOwnerURI string + SourceRepositoryURI string +} + +type EnforcementCriteria struct { + Extensions Extensions + PredicateType string + Artifact artifact.DigestedArtifact + OIDCIssuer string +} diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index d23cf32df..e2c357a0f 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -4,12 +4,10 @@ import ( "errors" "fmt" "regexp" - "strings" "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" "github.com/sigstore/sigstore-go/pkg/verify" - "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" ) @@ -20,82 +18,6 @@ const ( hostRegex = `^[a-zA-Z0-9-]+\.[a-zA-Z0-9-]+.*$` ) -type Extensions struct { - RunnerEnvironment string - SANRegex string - SAN string - BuildSourceRepoURI string - SignerWorkflow string - SourceRepositoryOwnerURI string - SourceRepositoryURI string -} - -type Policy struct { - Extensions Extensions - PredicateType string - Artifact artifact.DigestedArtifact - OIDCIssuer string -} - -func newPolicy(opts *Options, a artifact.DigestedArtifact) (Policy, error) { - p := Policy{ - Artifact: a, - } - - if opts.SignerRepo != "" { - signedRepoRegex := expandToGitHubURL(opts.Tenant, opts.SignerRepo) - p.Extensions.SANRegex = signedRepoRegex - } else if opts.SignerWorkflow != "" { - validatedWorkflowRegex, err := validateSignerWorkflow(opts) - if err != nil { - return Policy{}, err - } - - p.Extensions.SANRegex = validatedWorkflowRegex - } else { - p.Extensions.SANRegex = opts.SANRegex - p.Extensions.SAN = opts.SAN - } - - if opts.DenySelfHostedRunner { - p.Extensions.RunnerEnvironment = GitHubRunner - } else { - p.Extensions.RunnerEnvironment = "*" - } - - if opts.Repo != "" { - if opts.Tenant != "" { - p.Extensions.BuildSourceRepoURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Repo) - } - p.Extensions.BuildSourceRepoURI = fmt.Sprintf("https://github.com/%s", opts.Repo) - } - - if opts.Tenant != "" { - p.Extensions.SourceRepositoryOwnerURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Owner) - } else { - p.Extensions.SourceRepositoryOwnerURI = fmt.Sprintf("https://github.com/%s", opts.Owner) - } - - // if issuer is anything other than the default, use the user-provided value; - // otherwise, select the appropriate default based on the tenant - if opts.Tenant != "" { - p.OIDCIssuer = fmt.Sprintf(verification.GitHubTenantOIDCIssuer, opts.Tenant) - } else { - p.OIDCIssuer = opts.OIDCIssuer - } - - return p, nil -} - -func (p *Policy) Verify(a []*api.Attestation) (bool, string) { - filtered := verification.FilterAttestations(p.PredicateType, a) - if len(filtered) == 0 { - return false, fmt.Sprintf("✗ No attestations found with predicate type: %s\n", p.PredicateType) - } - - return true, "" -} - func expandToGitHubURL(tenant, ownerOrRepo string) string { if tenant == "" { return fmt.Sprintf("(?i)^https://github.com/%s/", ownerOrRepo) @@ -103,55 +25,6 @@ func expandToGitHubURL(tenant, ownerOrRepo string) string { return fmt.Sprintf("(?i)^https://%s.ghe.com/%s/", tenant, ownerOrRepo) } -func (p *Policy) buildCertificateIdentityOption() (verify.PolicyOption, error) { - sanMatcher, err := verify.NewSANMatcher(p.Extensions.SAN, p.Extensions.SANRegex) - if err != nil { - return nil, err - } - - // Accept any issuer, we will verify the issuer as part of the extension verification - issuerMatcher, err := verify.NewIssuerMatcher("", ".*") - if err != nil { - return nil, err - } - - extensions := certificate.Extensions{ - RunnerEnvironment: p.Extensions.RunnerEnvironment, - } - - certId, err := verify.NewCertificateIdentity(sanMatcher, issuerMatcher, extensions) - if err != nil { - return nil, err - } - - return verify.WithCertificateIdentity(certId), nil -} - -func (p *Policy) VerifyPredicateType(a []*api.Attestation) ([]*api.Attestation, error) { - filteredAttestations := verification.FilterAttestations(p.PredicateType, a) - - if len(filteredAttestations) == 0 { - return nil, fmt.Errorf("✗ No attestations found with predicate type: %s\n", p.PredicateType) - } - - return filteredAttestations, nil -} - -func (p *Policy) SigstorePolicy() (verify.PolicyBuilder, error) { - artifactDigestPolicyOption, err := verification.BuildDigestPolicyOption(p.Artifact) - if err != nil { - return verify.PolicyBuilder{}, err - } - - certIdOption, err := p.buildCertificateIdentityOption() - if err != nil { - return verify.PolicyBuilder{}, err - } - - policy := verify.NewPolicy(artifactDigestPolicyOption, certIdOption) - return policy, nil -} - func validateSignerWorkflow(opts *Options) (string, error) { // we expect a provided workflow argument be in the format [HOST/]///path/to/workflow.yml // if the provided workflow does not contain a host, set the host @@ -171,51 +44,91 @@ func validateSignerWorkflow(opts *Options) (string, error) { return fmt.Sprintf("^https://%s/%s", opts.Hostname, opts.SignerWorkflow), nil } -func (p *Policy) VerifyCertExtensions(results []*verification.AttestationProcessingResult) error { - if len(results) == 0 { - return errors.New("no attestations proccessing results") +func newEnforcementCriteria(opts *Options, a artifact.DigestedArtifact) (verification.EnforcementCriteria, error) { + c := verification.EnforcementCriteria{ + Artifact: a, } - var atLeastOneVerified bool - for _, attestation := range results { - if err := p.verifyCertExtensions(attestation); err != nil { - return err + if opts.SignerRepo != "" { + signedRepoRegex := expandToGitHubURL(opts.Tenant, opts.SignerRepo) + c.Extensions.SANRegex = signedRepoRegex + } else if opts.SignerWorkflow != "" { + validatedWorkflowRegex, err := validateSignerWorkflow(opts) + if err != nil { + return verification.EnforcementCriteria{}, err } - atLeastOneVerified = true - } - if atLeastOneVerified { - return nil + c.Extensions.SANRegex = validatedWorkflowRegex } else { - return verification.ErrNoAttestationsVerified + c.Extensions.SANRegex = opts.SANRegex + c.Extensions.SAN = opts.SAN } + + if opts.DenySelfHostedRunner { + c.Extensions.RunnerEnvironment = GitHubRunner + } else { + c.Extensions.RunnerEnvironment = "*" + } + + if opts.Repo != "" { + if opts.Tenant != "" { + c.Extensions.BuildSourceRepoURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Repo) + } + c.Extensions.BuildSourceRepoURI = fmt.Sprintf("https://github.com/%s", opts.Repo) + } + + if opts.Tenant != "" { + c.Extensions.SourceRepositoryOwnerURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Owner) + } else { + c.Extensions.SourceRepositoryOwnerURI = fmt.Sprintf("https://github.com/%s", opts.Owner) + } + + // if issuer is anything other than the default, use the user-provided value; + // otherwise, select the appropriate default based on the tenant + if opts.Tenant != "" { + c.OIDCIssuer = fmt.Sprintf(verification.GitHubTenantOIDCIssuer, opts.Tenant) + } else { + c.OIDCIssuer = opts.OIDCIssuer + } + + return c, nil } -func (p *Policy) verifyCertExtensions(attestation *verification.AttestationProcessingResult) error { - if p.Extensions.SourceRepositoryOwnerURI != "" { - sourceRepositoryOwnerURI := attestation.VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI - if !strings.EqualFold(p.Extensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) { - return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", p.Extensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) - } +func buildCertificateIdentityOption(c verification.EnforcementCriteria) (verify.PolicyOption, error) { + sanMatcher, err := verify.NewSANMatcher(c.Extensions.SAN, c.Extensions.SANRegex) + if err != nil { + return nil, err } - // if repo is set, check the SourceRepositoryURI field - if p.Extensions.SourceRepositoryURI != "" { - sourceRepositoryURI := attestation.VerificationResult.Signature.Certificate.Extensions.SourceRepositoryURI - if !strings.EqualFold(p.Extensions.SourceRepositoryURI, sourceRepositoryURI) { - return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", p.Extensions.SourceRepositoryURI, sourceRepositoryURI) - } + // Accept any issuer, we will verify the issuer as part of the extension verification + issuerMatcher, err := verify.NewIssuerMatcher("", ".*") + if err != nil { + return nil, err } - if p.OIDCIssuer != "" { - certIssuer := attestation.VerificationResult.Signature.Certificate.Extensions.Issuer - if !strings.EqualFold(p.OIDCIssuer, certIssuer) { - if strings.Index(certIssuer, p.OIDCIssuer+"/") == 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", p.OIDCIssuer, certIssuer) - } - return fmt.Errorf("expected Issuer to be %s, got %s", p.OIDCIssuer, certIssuer) - } + extensions := certificate.Extensions{ + RunnerEnvironment: c.Extensions.RunnerEnvironment, } - return nil + certId, err := verify.NewCertificateIdentity(sanMatcher, issuerMatcher, extensions) + if err != nil { + return nil, err + } + + return verify.WithCertificateIdentity(certId), nil +} + +func SigstorePolicy(c verification.EnforcementCriteria) (verify.PolicyBuilder, error) { + artifactDigestPolicyOption, err := verification.BuildDigestPolicyOption(c.Artifact) + if err != nil { + return verify.PolicyBuilder{}, err + } + + certIdOption, err := buildCertificateIdentityOption(c) + if err != nil { + return verify.PolicyBuilder{}, err + } + + policy := verify.NewPolicy(artifactDigestPolicyOption, certIdOption) + return policy, nil } diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index 70c3079e1..8d6c1dfbc 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -26,7 +26,7 @@ func TestBuildPolicy(t *testing.T) { SANRegex: "^https://github.com/sigstore/", } - _, err = newPolicy(opts, *artifact) + _, err = newEnforcementCriteria(opts, *artifact) require.NoError(t, err) } diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index a873d6c96..4c6af2b70 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -255,9 +255,27 @@ func runVerify(opts *Options) error { attestations = filteredAttestations } - sigstoreResults, err := verifyAll(opts, *artifact, attestations) + ec, err := newEnforcementCriteria(opts, *artifact) if err != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red(err.Error())) + opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build verification policy")) + return err + } + + sp, err := SigstorePolicy(ec) + if err != nil { + opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build Sigstore verification policy")) + return err + } + + sigstoreRes := opts.SigstoreVerifier.Verify(attestations, sp) + if sigstoreRes.Error != nil { + opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Sigstore verification failed")) + return err + } + + // Verify extensions + if err := verification.VerifyCertExtensions(sigstoreRes.VerifyResults, ec); err != nil { + opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Policy verification failed")) return err } @@ -266,7 +284,7 @@ func runVerify(opts *Options) error { // If an exporter is provided with the --json flag, write the results to the terminal in JSON format if opts.exporter != nil { // print the results to the terminal as an array of JSON objects - if err = opts.exporter.Write(opts.Logger.IO, sigstoreResults.VerifyResults); err != nil { + if err = opts.exporter.Write(opts.Logger.IO, sigstoreRes.VerifyResults); err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to write JSON output")) return err } @@ -276,7 +294,7 @@ func runVerify(opts *Options) error { opts.Logger.Printf("%s was attested by:\n", artifact.DigestWithAlg()) // Otherwise print the results to the terminal in a table - tableContent, err := buildTableVerifyContent(opts.Tenant, sigstoreResults.VerifyResults) + tableContent, err := buildTableVerifyContent(opts.Tenant, sigstoreRes.VerifyResults) if err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("failed to parse results")) return err @@ -352,27 +370,3 @@ func buildTableVerifyContent(tenant string, results []*verification.AttestationP return content, nil } - -func verifyAll(opts *Options, artifact artifact.DigestedArtifact, attestations []*api.Attestation) (*verification.SigstoreResults, error) { - policy, err := newPolicy(opts, artifact) - if err != nil { - return nil, fmt.Errorf("✗ Failed to build verification policy") - } - - sp, err := policy.SigstorePolicy() - if err != nil { - return nil, fmt.Errorf("✗ Failed to build Sigstore verification policy") - } - - sigstoreRes := opts.SigstoreVerifier.Verify(attestations, sp) - if sigstoreRes.Error != nil { - return nil, fmt.Errorf("✗ Sigstore verification failed") - } - - // Verify extensions - if err := verification.VerifyCertExtensions(sigstoreRes.VerifyResults, opts.Tenant, opts.Owner, opts.Repo, opts.OIDCIssuer); err != nil { - return nil, fmt.Errorf("✗ Policy verification failed") - } - - return sigstoreRes, nil -} From 93c78a21346ed579252ade47ba41f3d5bb94f337 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Wed, 30 Oct 2024 15:28:34 -0600 Subject: [PATCH 15/76] use sigstore specific err Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/verify.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 4c6af2b70..7aae31e23 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -270,7 +270,7 @@ func runVerify(opts *Options) error { sigstoreRes := opts.SigstoreVerifier.Verify(attestations, sp) if sigstoreRes.Error != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Sigstore verification failed")) - return err + return sigstoreRes.Error } // Verify extensions From 4fa5f0c5eed22e8585c74d24ac66c99985de420e Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Wed, 30 Oct 2024 15:44:53 -0600 Subject: [PATCH 16/76] update extensions test Signed-off-by: Meredith Lancaster --- .../verification/extensions_test.go | 92 +++++++++++++++---- 1 file changed, 73 insertions(+), 19 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions_test.go b/pkg/cmd/attestation/verification/extensions_test.go index 5eb28829d..e1a1555c5 100644 --- a/pkg/cmd/attestation/verification/extensions_test.go +++ b/pkg/cmd/attestation/verification/extensions_test.go @@ -25,33 +25,53 @@ func TestVerifyCertExtensions(t *testing.T) { }, } + c := EnforcementCriteria{ + Extensions: Extensions{ + SourceRepositoryOwnerURI: "https://github.com/owner", + SourceRepositoryURI: "https://github.com/owner/repo", + }, + OIDCIssuer: GitHubOIDCIssuer, + } + t.Run("VerifyCertExtensions with owner and repo", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "owner/repo", GitHubOIDCIssuer) + err := VerifyCertExtensions(results, c) require.NoError(t, err) }) t.Run("VerifyCertExtensions with owner and repo, but wrong tenant", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "owner", "owner/repo", GitHubOIDCIssuer) + expectedCriteria := c + expectedCriteria.Extensions.SourceRepositoryOwnerURI = "https://foo.ghe.com/owner" + expectedCriteria.Extensions.SourceRepositoryURI = "https://foo.ghe.com/owner/repo" + err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://foo.ghe.com/owner, got https://github.com/owner") }) t.Run("VerifyCertExtensions with owner", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "", GitHubOIDCIssuer) + expectedCriteria := c + expectedCriteria.Extensions.SourceRepositoryURI = "" + err := VerifyCertExtensions(results, expectedCriteria) require.NoError(t, err) }) t.Run("VerifyCertExtensions with wrong owner", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "wrong", "", GitHubOIDCIssuer) + expectedCriteria := c + expectedCriteria.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong" + expectedCriteria.Extensions.SourceRepositoryURI = "" + err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://github.com/wrong, got https://github.com/owner") }) t.Run("VerifyCertExtensions with wrong repo", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "wrong", GitHubOIDCIssuer) + expectedCriteria := c + expectedCriteria.Extensions.SourceRepositoryURI = "https://github.com/owner/wrong" + err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/wrong, got https://github.com/owner/repo") }) t.Run("VerifyCertExtensions with wrong issuer", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "", "wrong") + expectedCriteria := c + expectedCriteria.OIDCIssuer = "wrong" + err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected Issuer to be wrong, got https://token.actions.githubusercontent.com") }) } @@ -73,25 +93,35 @@ func TestVerifyCertExtensionsCustomizedIssuer(t *testing.T) { }, } + c := EnforcementCriteria{ + Extensions: Extensions{ + SourceRepositoryOwnerURI: "https://github.com/owner", + SourceRepositoryURI: "https://github.com/owner/repo", + }, + OIDCIssuer: "https://token.actions.githubusercontent.com/foo-bar", + } + t.Run("VerifyCertExtensions with exact issuer match", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "owner/repo", "https://token.actions.githubusercontent.com/foo-bar") + err := VerifyCertExtensions(results, c) require.NoError(t, err) }) t.Run("VerifyCertExtensions with partial issuer match", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "owner/repo", "https://token.actions.githubusercontent.com") + expectedCriteria := c + expectedCriteria.OIDCIssuer = "https://token.actions.githubusercontent.com" + err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected Issuer to be https://token.actions.githubusercontent.com, got https://token.actions.githubusercontent.com/foo-bar -- if you have a custom OIDC issuer") }) t.Run("VerifyCertExtensions with wrong issuer", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "", "wrong") + expectedCriteria := c + expectedCriteria.OIDCIssuer = "wrong" + err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected Issuer to be wrong, got https://token.actions.githubusercontent.com/foo-bar") }) } func TestVerifyTenancyCertExtensions(t *testing.T) { - defaultIssuer := GitHubOIDCIssuer - results := []*AttestationProcessingResult{ { VerificationResult: &verify.VerificationResult{ @@ -108,43 +138,67 @@ func TestVerifyTenancyCertExtensions(t *testing.T) { }, } + c := EnforcementCriteria{ + Extensions: Extensions{ + SourceRepositoryOwnerURI: "https://foo.ghe.com/owner", + SourceRepositoryURI: "https://foo.ghe.com/owner/repo", + }, + OIDCIssuer: GitHubOIDCIssuer, + } + t.Run("VerifyCertExtensions with owner and repo", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "owner", "owner/repo", defaultIssuer) + err := VerifyCertExtensions(results, c) require.NoError(t, err) }) t.Run("VerifyCertExtensions with owner and repo, no tenant", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "owner/repo", defaultIssuer) + expectedCriteria := c + expectedCriteria.Extensions.SourceRepositoryOwnerURI = "https://github.com/owner" + expectedCriteria.Extensions.SourceRepositoryURI = "https://github.com/owner/repo" + err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://github.com/owner, got https://foo.ghe.com/owner") }) t.Run("VerifyCertExtensions with owner and repo, wrong tenant", func(t *testing.T) { - err := VerifyCertExtensions(results, "bar", "owner", "owner/repo", defaultIssuer) + expectedCriteria := c + expectedCriteria.Extensions.SourceRepositoryOwnerURI = "https://bar.ghe.com/owner" + expectedCriteria.Extensions.SourceRepositoryURI = "https://bar.ghe.com/owner/repo" + err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://bar.ghe.com/owner, got https://foo.ghe.com/owner") }) t.Run("VerifyCertExtensions with owner", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "owner", "", defaultIssuer) + expectedCriteria := c + expectedCriteria.Extensions.SourceRepositoryURI = "" + err := VerifyCertExtensions(results, expectedCriteria) require.NoError(t, err) }) t.Run("VerifyCertExtensions with wrong owner", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "wrong", "", defaultIssuer) + expectedCriteria := c + expectedCriteria.Extensions.SourceRepositoryOwnerURI = "https://foo.ghe.com/wrong" + err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://foo.ghe.com/wrong, got https://foo.ghe.com/owner") }) t.Run("VerifyCertExtensions with wrong repo", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "owner", "wrong", defaultIssuer) + expectedCriteria := c + expectedCriteria.Extensions.SourceRepositoryURI = "https://foo.ghe.com/owner/wrong" + err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://foo.ghe.com/wrong, got https://foo.ghe.com/owner/repo") }) t.Run("VerifyCertExtensions with correct, non-default issuer", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "owner", "owner/repo", "https://token.actions.foo.ghe.com") + expectedCriteria := c + expectedCriteria.OIDCIssuer = "https://token.actions.foo.ghe.com" + err := VerifyCertExtensions(results, expectedCriteria) require.NoError(t, err) }) t.Run("VerifyCertExtensions with wrong issuer", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "owner", "owner/repo", "wrong") + expectedCriteria := c + expectedCriteria.OIDCIssuer = "wrong" + err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected Issuer to be wrong, got https://token.actions.foo.ghe.com") }) } From 8b02c43085f78d966e24c9f6161df2c47072a2de Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Wed, 30 Oct 2024 16:05:39 -0600 Subject: [PATCH 17/76] add tests for newEnforcementCriteria Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 4 +- pkg/cmd/attestation/verify/policy_test.go | 159 ++++++++++++++++++++-- 2 files changed, 148 insertions(+), 15 deletions(-) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index e2c357a0f..ff8a575af 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -83,8 +83,8 @@ func newEnforcementCriteria(opts *Options, a artifact.DigestedArtifact) (verific c.Extensions.SourceRepositoryOwnerURI = fmt.Sprintf("https://github.com/%s", opts.Owner) } - // if issuer is anything other than the default, use the user-provided value; - // otherwise, select the appropriate default based on the tenant + // if tenant is provided, select the appropriate default based on the tenant + // otherwise, use the provided OIDCIssuer if opts.Tenant != "" { c.OIDCIssuer = fmt.Sprintf(verification.GitHubTenantOIDCIssuer, opts.Tenant) } else { diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index d50592ba2..9ef34e440 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -10,24 +10,157 @@ import ( "github.com/stretchr/testify/require" ) -// This tests that a policy can be built from a valid artifact -// Note that policy use is tested in verify_test.go in this package -func TestBuildPolicy(t *testing.T) { - ociClient := oci.MockClient{} +func TestNewEnforcementCriteria(t *testing.T) { artifactPath := "../test/data/sigstore-js-2.1.0.tgz" - digestAlg := "sha256" - artifact, err := artifact.NewDigestedArtifact(ociClient, artifactPath, digestAlg) + artifact, err := artifact.NewDigestedArtifact(oci.MockClient{}, artifactPath, "sha256") require.NoError(t, err) - opts := &Options{ - ArtifactPath: artifactPath, - Owner: "sigstore", - SANRegex: "^https://github.com/sigstore/", - } + t.Run("sets SANRegex using SignerRepo", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "bar", + SignerRepo: "foo/bar", + } - _, err = newEnforcementCriteria(opts, *artifact) - require.NoError(t, err) + c, err := newEnforcementCriteria(opts, *artifact) + require.NoError(t, err) + require.Equal(t, "^https://github.com/foo/bar", c.Extensions.SANRegex) + require.Zero(t, c.Extensions.SAN) + }) + + t.Run("sets SANRegex using SignerWorkflow", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "bar", + SignerWorkflow: "foo/bar/.github/workflows/attest.yml", + } + + c, err := newEnforcementCriteria(opts, *artifact) + require.NoError(t, err) + require.Equal(t, "^https://github.com/foo/bar/.github/workflows/attest.yml", c.Extensions.SANRegex) + require.Zero(t, c.Extensions.SAN) + }) + + t.Run("sets SANRegex and SAN using SANRegex and SAN", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "bar", + SAN: "https://github/foo/bar/.github/workflows/attest.yml", + SANRegex: "^https://github/foo", + } + + c, err := newEnforcementCriteria(opts, *artifact) + require.NoError(t, err) + require.Equal(t, "https://github/foo/bar/.github/workflows/attest.yml", c.Extensions.SANRegex) + require.Equal(t, "^https://github/foo", c.Extensions.SAN) + }) + + t.Run("sets Extensions.RunnerEnvironment to GitHubRunner value if opts.DenySelfHostedRunner is true", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "bar", + DenySelfHostedRunner: true, + } + + c, err := newEnforcementCriteria(opts, *artifact) + require.NoError(t, err) + require.Equal(t, GitHubRunner, c.Extensions.RunnerEnvironment) + }) + + t.Run("sets Extensions.RunnerEnvironment to * value if opts.DenySelfHostedRunner is false", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "bar", + DenySelfHostedRunner: false, + } + + c, err := newEnforcementCriteria(opts, *artifact) + require.NoError(t, err) + require.Equal(t, "*", c.Extensions.RunnerEnvironment) + }) + + t.Run("sets Extensions.BuildSourceRepoURI using opts.Repo and opts.Tenant", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "bar", + Tenant: "baz", + } + + c, err := newEnforcementCriteria(opts, *artifact) + require.NoError(t, err) + require.Equal(t, "https://baz.ghe.com/foo/bar", c.Extensions.BuildSourceRepoURI) + }) + + t.Run("sets Extensions.BuildSourceRepoURI using opts.Repo", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "bar", + } + + c, err := newEnforcementCriteria(opts, *artifact) + require.NoError(t, err) + require.Equal(t, "https://github.com/foo/bar", c.Extensions.BuildSourceRepoURI) + }) + + t.Run("sets Extensions.SourceRepositoryOwnerURI using opts.Owner and opts.Tenant", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "bar", + Tenant: "baz", + } + + c, err := newEnforcementCriteria(opts, *artifact) + require.NoError(t, err) + require.Equal(t, "https://baz.ghe.com/foo", c.Extensions.SourceRepositoryOwnerURI) + }) + + t.Run("sets Extensions.SourceRepositoryOwnerURI using opts.Owner", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "bar", + } + + c, err := newEnforcementCriteria(opts, *artifact) + require.NoError(t, err) + require.Equal(t, "https://github.com/foo", c.Extensions.SourceRepositoryOwnerURI) + }) + + t.Run("sets OIDCIssuer using opts.OIDCIssuer and opts.Tenant", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "bar", + Tenant: "baz", + OIDCIssuer: "https://foo.com", + } + + c, err := newEnforcementCriteria(opts, *artifact) + require.NoError(t, err) + require.Equal(t, "https://token.actions.baz.ghe.com", c.OIDCIssuer) + }) + + t.Run("sets OIDCIssuer using opts.OIDCIssuer", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "bar", + OIDCIssuer: "https://foo.com", + } + + c, err := newEnforcementCriteria(opts, *artifact) + require.NoError(t, err) + require.Equal(t, "https://foo.com", c.OIDCIssuer) + }) } func TestValidateSignerWorkflow(t *testing.T) { From 84c823c55fec23d4d306f43cd198d880840e90e6 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Wed, 30 Oct 2024 16:12:57 -0600 Subject: [PATCH 18/76] clean up extension verification tests Signed-off-by: Meredith Lancaster --- .../verification/extensions_test.go | 152 +----------------- 1 file changed, 8 insertions(+), 144 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions_test.go b/pkg/cmd/attestation/verification/extensions_test.go index e1a1555c5..734ee1943 100644 --- a/pkg/cmd/attestation/verification/extensions_test.go +++ b/pkg/cmd/attestation/verification/extensions_test.go @@ -33,172 +33,36 @@ func TestVerifyCertExtensions(t *testing.T) { OIDCIssuer: GitHubOIDCIssuer, } - t.Run("VerifyCertExtensions with owner and repo", func(t *testing.T) { + t.Run("success", func(t *testing.T) { err := VerifyCertExtensions(results, c) require.NoError(t, err) }) - t.Run("VerifyCertExtensions with owner and repo, but wrong tenant", func(t *testing.T) { - expectedCriteria := c - expectedCriteria.Extensions.SourceRepositoryOwnerURI = "https://foo.ghe.com/owner" - expectedCriteria.Extensions.SourceRepositoryURI = "https://foo.ghe.com/owner/repo" - err := VerifyCertExtensions(results, expectedCriteria) - require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://foo.ghe.com/owner, got https://github.com/owner") - }) - - t.Run("VerifyCertExtensions with owner", func(t *testing.T) { - expectedCriteria := c - expectedCriteria.Extensions.SourceRepositoryURI = "" - err := VerifyCertExtensions(results, expectedCriteria) - require.NoError(t, err) - }) - - t.Run("VerifyCertExtensions with wrong owner", func(t *testing.T) { + t.Run("with wrong SourceRepositoryOwnerURI", func(t *testing.T) { expectedCriteria := c expectedCriteria.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong" - expectedCriteria.Extensions.SourceRepositoryURI = "" err := VerifyCertExtensions(results, expectedCriteria) - require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://github.com/wrong, got https://github.com/owner") + require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://github.com/owner, got https://github.com/wrong") }) - t.Run("VerifyCertExtensions with wrong repo", func(t *testing.T) { + t.Run("with wrong SourceRepositoryURI", func(t *testing.T) { expectedCriteria := c - expectedCriteria.Extensions.SourceRepositoryURI = "https://github.com/owner/wrong" + expectedCriteria.Extensions.SourceRepositoryURI = "https://github.com/foo/wrong" err := VerifyCertExtensions(results, expectedCriteria) - require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/wrong, got https://github.com/owner/repo") + require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/owner/wrong, got https://github.com/wrong/bar") }) - t.Run("VerifyCertExtensions with wrong issuer", func(t *testing.T) { + t.Run("with wrong OIDCIssuer", func(t *testing.T) { expectedCriteria := c expectedCriteria.OIDCIssuer = "wrong" err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected Issuer to be wrong, got https://token.actions.githubusercontent.com") }) -} -func TestVerifyCertExtensionsCustomizedIssuer(t *testing.T) { - results := []*AttestationProcessingResult{ - { - VerificationResult: &verify.VerificationResult{ - Signature: &verify.SignatureVerificationResult{ - Certificate: &certificate.Summary{ - Extensions: certificate.Extensions{ - SourceRepositoryOwnerURI: "https://github.com/owner", - SourceRepositoryURI: "https://github.com/owner/repo", - Issuer: "https://token.actions.githubusercontent.com/foo-bar", - }, - }, - }, - }, - }, - } - - c := EnforcementCriteria{ - Extensions: Extensions{ - SourceRepositoryOwnerURI: "https://github.com/owner", - SourceRepositoryURI: "https://github.com/owner/repo", - }, - OIDCIssuer: "https://token.actions.githubusercontent.com/foo-bar", - } - - t.Run("VerifyCertExtensions with exact issuer match", func(t *testing.T) { - err := VerifyCertExtensions(results, c) - require.NoError(t, err) - }) - - t.Run("VerifyCertExtensions with partial issuer match", func(t *testing.T) { + t.Run("with partial OIDCIssuer match", func(t *testing.T) { expectedCriteria := c expectedCriteria.OIDCIssuer = "https://token.actions.githubusercontent.com" err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected Issuer to be https://token.actions.githubusercontent.com, got https://token.actions.githubusercontent.com/foo-bar -- if you have a custom OIDC issuer") }) - - t.Run("VerifyCertExtensions with wrong issuer", func(t *testing.T) { - expectedCriteria := c - expectedCriteria.OIDCIssuer = "wrong" - err := VerifyCertExtensions(results, expectedCriteria) - require.ErrorContains(t, err, "expected Issuer to be wrong, got https://token.actions.githubusercontent.com/foo-bar") - }) -} - -func TestVerifyTenancyCertExtensions(t *testing.T) { - results := []*AttestationProcessingResult{ - { - VerificationResult: &verify.VerificationResult{ - Signature: &verify.SignatureVerificationResult{ - Certificate: &certificate.Summary{ - Extensions: certificate.Extensions{ - SourceRepositoryOwnerURI: "https://foo.ghe.com/owner", - SourceRepositoryURI: "https://foo.ghe.com/owner/repo", - Issuer: "https://token.actions.foo.ghe.com", - }, - }, - }, - }, - }, - } - - c := EnforcementCriteria{ - Extensions: Extensions{ - SourceRepositoryOwnerURI: "https://foo.ghe.com/owner", - SourceRepositoryURI: "https://foo.ghe.com/owner/repo", - }, - OIDCIssuer: GitHubOIDCIssuer, - } - - t.Run("VerifyCertExtensions with owner and repo", func(t *testing.T) { - err := VerifyCertExtensions(results, c) - require.NoError(t, err) - }) - - t.Run("VerifyCertExtensions with owner and repo, no tenant", func(t *testing.T) { - expectedCriteria := c - expectedCriteria.Extensions.SourceRepositoryOwnerURI = "https://github.com/owner" - expectedCriteria.Extensions.SourceRepositoryURI = "https://github.com/owner/repo" - err := VerifyCertExtensions(results, expectedCriteria) - require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://github.com/owner, got https://foo.ghe.com/owner") - }) - - t.Run("VerifyCertExtensions with owner and repo, wrong tenant", func(t *testing.T) { - expectedCriteria := c - expectedCriteria.Extensions.SourceRepositoryOwnerURI = "https://bar.ghe.com/owner" - expectedCriteria.Extensions.SourceRepositoryURI = "https://bar.ghe.com/owner/repo" - err := VerifyCertExtensions(results, expectedCriteria) - require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://bar.ghe.com/owner, got https://foo.ghe.com/owner") - }) - - t.Run("VerifyCertExtensions with owner", func(t *testing.T) { - expectedCriteria := c - expectedCriteria.Extensions.SourceRepositoryURI = "" - err := VerifyCertExtensions(results, expectedCriteria) - require.NoError(t, err) - }) - - t.Run("VerifyCertExtensions with wrong owner", func(t *testing.T) { - expectedCriteria := c - expectedCriteria.Extensions.SourceRepositoryOwnerURI = "https://foo.ghe.com/wrong" - err := VerifyCertExtensions(results, expectedCriteria) - require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://foo.ghe.com/wrong, got https://foo.ghe.com/owner") - }) - - t.Run("VerifyCertExtensions with wrong repo", func(t *testing.T) { - expectedCriteria := c - expectedCriteria.Extensions.SourceRepositoryURI = "https://foo.ghe.com/owner/wrong" - err := VerifyCertExtensions(results, expectedCriteria) - require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://foo.ghe.com/wrong, got https://foo.ghe.com/owner/repo") - }) - - t.Run("VerifyCertExtensions with correct, non-default issuer", func(t *testing.T) { - expectedCriteria := c - expectedCriteria.OIDCIssuer = "https://token.actions.foo.ghe.com" - err := VerifyCertExtensions(results, expectedCriteria) - require.NoError(t, err) - }) - - t.Run("VerifyCertExtensions with wrong issuer", func(t *testing.T) { - expectedCriteria := c - expectedCriteria.OIDCIssuer = "wrong" - err := VerifyCertExtensions(results, expectedCriteria) - require.ErrorContains(t, err, "expected Issuer to be wrong, got https://token.actions.foo.ghe.com") - }) } From 318bd9035604030f1e6941cf388231d2f1c71827 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Wed, 30 Oct 2024 16:21:15 -0600 Subject: [PATCH 19/76] update extensions tests Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verification/extensions_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions_test.go b/pkg/cmd/attestation/verification/extensions_test.go index 734ee1943..e01c14a6f 100644 --- a/pkg/cmd/attestation/verification/extensions_test.go +++ b/pkg/cmd/attestation/verification/extensions_test.go @@ -42,14 +42,14 @@ func TestVerifyCertExtensions(t *testing.T) { expectedCriteria := c expectedCriteria.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong" err := VerifyCertExtensions(results, expectedCriteria) - require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://github.com/owner, got https://github.com/wrong") + require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://github.com/wrong, got https://github.com/owner") }) t.Run("with wrong SourceRepositoryURI", func(t *testing.T) { expectedCriteria := c expectedCriteria.Extensions.SourceRepositoryURI = "https://github.com/foo/wrong" err := VerifyCertExtensions(results, expectedCriteria) - require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/owner/wrong, got https://github.com/wrong/bar") + require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/foo/wrong, got https://github.com/foo/bar") }) t.Run("with wrong OIDCIssuer", func(t *testing.T) { @@ -60,9 +60,9 @@ func TestVerifyCertExtensions(t *testing.T) { }) t.Run("with partial OIDCIssuer match", func(t *testing.T) { - expectedCriteria := c - expectedCriteria.OIDCIssuer = "https://token.actions.githubusercontent.com" - err := VerifyCertExtensions(results, expectedCriteria) + expectedResults := results + expectedResults[0].VerificationResult.Signature.Certificate.Extensions.Issuer = "https://token.actions.githubusercontent.com/foo-bar" + err := VerifyCertExtensions(expectedResults, c) require.ErrorContains(t, err, "expected Issuer to be https://token.actions.githubusercontent.com, got https://token.actions.githubusercontent.com/foo-bar -- if you have a custom OIDC issuer") }) } From bb0dcd9db4112f31cecee836af4d236f9208a58b Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Wed, 30 Oct 2024 17:19:15 -0600 Subject: [PATCH 20/76] fix wrong field settings Signed-off-by: Meredith Lancaster --- .../verification/extensions_test.go | 2 +- pkg/cmd/attestation/verification/policy.go | 1 - pkg/cmd/attestation/verify/policy.go | 5 ++- pkg/cmd/attestation/verify/policy_test.go | 41 ++++++++++--------- 4 files changed, 25 insertions(+), 24 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions_test.go b/pkg/cmd/attestation/verification/extensions_test.go index e01c14a6f..f34cc8304 100644 --- a/pkg/cmd/attestation/verification/extensions_test.go +++ b/pkg/cmd/attestation/verification/extensions_test.go @@ -49,7 +49,7 @@ func TestVerifyCertExtensions(t *testing.T) { expectedCriteria := c expectedCriteria.Extensions.SourceRepositoryURI = "https://github.com/foo/wrong" err := VerifyCertExtensions(results, expectedCriteria) - require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/foo/wrong, got https://github.com/foo/bar") + require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/foo/wrong, got https://github.com/owner/repo") }) t.Run("with wrong OIDCIssuer", func(t *testing.T) { diff --git a/pkg/cmd/attestation/verification/policy.go b/pkg/cmd/attestation/verification/policy.go index 21210e730..974eae4e2 100644 --- a/pkg/cmd/attestation/verification/policy.go +++ b/pkg/cmd/attestation/verification/policy.go @@ -23,7 +23,6 @@ type Extensions struct { RunnerEnvironment string SANRegex string SAN string - BuildSourceRepoURI string SignerWorkflow string SourceRepositoryOwnerURI string SourceRepositoryURI string diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index ff8a575af..789b7dfb8 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -72,9 +72,10 @@ func newEnforcementCriteria(opts *Options, a artifact.DigestedArtifact) (verific if opts.Repo != "" { if opts.Tenant != "" { - c.Extensions.BuildSourceRepoURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Repo) + c.Extensions.SourceRepositoryURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Repo) + } else { + c.Extensions.SourceRepositoryURI = fmt.Sprintf("https://github.com/%s", opts.Repo) } - c.Extensions.BuildSourceRepoURI = fmt.Sprintf("https://github.com/%s", opts.Repo) } if opts.Tenant != "" { diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index 9ef34e440..daadcf346 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -20,22 +20,23 @@ func TestNewEnforcementCriteria(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, Owner: "foo", - Repo: "bar", + Repo: "foo/bar", SignerRepo: "foo/bar", } c, err := newEnforcementCriteria(opts, *artifact) require.NoError(t, err) - require.Equal(t, "^https://github.com/foo/bar", c.Extensions.SANRegex) + require.Equal(t, "(?i)^https://github.com/foo/bar/", c.Extensions.SANRegex) require.Zero(t, c.Extensions.SAN) }) - t.Run("sets SANRegex using SignerWorkflow", func(t *testing.T) { + t.Run("sets SANRegex using SignerWorkflow matching host regex", func(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, Owner: "foo", - Repo: "bar", + Repo: "foo/bar", SignerWorkflow: "foo/bar/.github/workflows/attest.yml", + Hostname: "github.com", } c, err := newEnforcementCriteria(opts, *artifact) @@ -48,22 +49,22 @@ func TestNewEnforcementCriteria(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, Owner: "foo", - Repo: "bar", + Repo: "foo/bar", SAN: "https://github/foo/bar/.github/workflows/attest.yml", - SANRegex: "^https://github/foo", + SANRegex: "(?i)^https://github/foo", } c, err := newEnforcementCriteria(opts, *artifact) require.NoError(t, err) - require.Equal(t, "https://github/foo/bar/.github/workflows/attest.yml", c.Extensions.SANRegex) - require.Equal(t, "^https://github/foo", c.Extensions.SAN) + require.Equal(t, "https://github/foo/bar/.github/workflows/attest.yml", c.Extensions.SAN) + require.Equal(t, "(?i)^https://github/foo", c.Extensions.SANRegex) }) t.Run("sets Extensions.RunnerEnvironment to GitHubRunner value if opts.DenySelfHostedRunner is true", func(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, Owner: "foo", - Repo: "bar", + Repo: "foo/bar", DenySelfHostedRunner: true, } @@ -76,7 +77,7 @@ func TestNewEnforcementCriteria(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, Owner: "foo", - Repo: "bar", + Repo: "foo/bar", DenySelfHostedRunner: false, } @@ -85,36 +86,36 @@ func TestNewEnforcementCriteria(t *testing.T) { require.Equal(t, "*", c.Extensions.RunnerEnvironment) }) - t.Run("sets Extensions.BuildSourceRepoURI using opts.Repo and opts.Tenant", func(t *testing.T) { + t.Run("sets Extensions.SourceRepositoryURI using opts.Repo and opts.Tenant", func(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, Owner: "foo", - Repo: "bar", + Repo: "foo/bar", Tenant: "baz", } c, err := newEnforcementCriteria(opts, *artifact) require.NoError(t, err) - require.Equal(t, "https://baz.ghe.com/foo/bar", c.Extensions.BuildSourceRepoURI) + require.Equal(t, "https://baz.ghe.com/foo/bar", c.Extensions.SourceRepositoryURI) }) - t.Run("sets Extensions.BuildSourceRepoURI using opts.Repo", func(t *testing.T) { + t.Run("sets Extensions.SourceRepositoryURI using opts.Repo", func(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, Owner: "foo", - Repo: "bar", + Repo: "foo/bar", } c, err := newEnforcementCriteria(opts, *artifact) require.NoError(t, err) - require.Equal(t, "https://github.com/foo/bar", c.Extensions.BuildSourceRepoURI) + require.Equal(t, "https://github.com/foo/bar", c.Extensions.SourceRepositoryURI) }) t.Run("sets Extensions.SourceRepositoryOwnerURI using opts.Owner and opts.Tenant", func(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, Owner: "foo", - Repo: "bar", + Repo: "foo/bar", Tenant: "baz", } @@ -127,7 +128,7 @@ func TestNewEnforcementCriteria(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, Owner: "foo", - Repo: "bar", + Repo: "foo/bar", } c, err := newEnforcementCriteria(opts, *artifact) @@ -139,7 +140,7 @@ func TestNewEnforcementCriteria(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, Owner: "foo", - Repo: "bar", + Repo: "foo/bar", Tenant: "baz", OIDCIssuer: "https://foo.com", } @@ -153,7 +154,7 @@ func TestNewEnforcementCriteria(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, Owner: "foo", - Repo: "bar", + Repo: "foo/bar", OIDCIssuer: "https://foo.com", } From 61b60e9430e9e704e87db3793c9a68f0755fcacf Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 08:19:33 -0600 Subject: [PATCH 21/76] fix runner setting Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 2 -- pkg/cmd/attestation/verify/policy_test.go | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index 789b7dfb8..d52497664 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -66,8 +66,6 @@ func newEnforcementCriteria(opts *Options, a artifact.DigestedArtifact) (verific if opts.DenySelfHostedRunner { c.Extensions.RunnerEnvironment = GitHubRunner - } else { - c.Extensions.RunnerEnvironment = "*" } if opts.Repo != "" { diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index daadcf346..f194edb74 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -83,7 +83,7 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts, *artifact) require.NoError(t, err) - require.Equal(t, "*", c.Extensions.RunnerEnvironment) + require.Zero(t, c.Extensions.RunnerEnvironment) }) t.Run("sets Extensions.SourceRepositoryURI using opts.Repo and opts.Tenant", func(t *testing.T) { From 9cdeb31fc684dcae1caa001b5f4d8110ac365cad Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 08:32:35 -0600 Subject: [PATCH 22/76] reorganize funcs Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 38 ++++++++++++++-------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index d52497664..b79ebacf0 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -25,25 +25,6 @@ func expandToGitHubURL(tenant, ownerOrRepo string) string { return fmt.Sprintf("(?i)^https://%s.ghe.com/%s/", tenant, ownerOrRepo) } -func validateSignerWorkflow(opts *Options) (string, error) { - // we expect a provided workflow argument be in the format [HOST/]///path/to/workflow.yml - // if the provided workflow does not contain a host, set the host - match, err := regexp.MatchString(hostRegex, opts.SignerWorkflow) - if err != nil { - return "", err - } - - if match { - return fmt.Sprintf("^https://%s", opts.SignerWorkflow), nil - } - - if opts.Hostname == "" { - return "", errors.New("unknown host") - } - - return fmt.Sprintf("^https://%s/%s", opts.Hostname, opts.SignerWorkflow), nil -} - func newEnforcementCriteria(opts *Options, a artifact.DigestedArtifact) (verification.EnforcementCriteria, error) { c := verification.EnforcementCriteria{ Artifact: a, @@ -131,3 +112,22 @@ func SigstorePolicy(c verification.EnforcementCriteria) (verify.PolicyBuilder, e policy := verify.NewPolicy(artifactDigestPolicyOption, certIdOption) return policy, nil } + +func validateSignerWorkflow(opts *Options) (string, error) { + // we expect a provided workflow argument be in the format [HOST/]///path/to/workflow.yml + // if the provided workflow does not contain a host, set the host + match, err := regexp.MatchString(hostRegex, opts.SignerWorkflow) + if err != nil { + return "", err + } + + if match { + return fmt.Sprintf("^https://%s", opts.SignerWorkflow), nil + } + + if opts.Hostname == "" { + return "", errors.New("unknown host") + } + + return fmt.Sprintf("^https://%s/%s", opts.Hostname, opts.SignerWorkflow), nil +} From 01f63c5cc3bb48b28b92d6cf10059e994080ee30 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 10:08:05 -0600 Subject: [PATCH 23/76] clean up unneeded struct Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/inspect/inspect.go | 10 +++---- .../attestation/verification/mock_verifier.go | 12 +++------ pkg/cmd/attestation/verification/sigstore.go | 27 ++++++------------- pkg/cmd/attestation/verify/verify.go | 12 ++++----- 4 files changed, 23 insertions(+), 38 deletions(-) diff --git a/pkg/cmd/attestation/inspect/inspect.go b/pkg/cmd/attestation/inspect/inspect.go index c139a5af2..31afb7ce6 100644 --- a/pkg/cmd/attestation/inspect/inspect.go +++ b/pkg/cmd/attestation/inspect/inspect.go @@ -141,9 +141,9 @@ func runInspect(opts *Options) error { return fmt.Errorf("failed to build policy: %v", err) } - 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) + res, err := opts.SigstoreVerifier.Verify(attestations, policy) + if err != nil { + return fmt.Errorf("at least one attestation failed to verify against Sigstore: %v", err) } opts.Logger.VerbosePrint(opts.Logger.ColorScheme.Green( @@ -152,7 +152,7 @@ func runInspect(opts *Options) error { // If the user provides the --format=json flag, print the results in JSON format if opts.exporter != nil { - details, err := getAttestationDetails(opts.Tenant, res.VerifyResults) + details, err := getAttestationDetails(opts.Tenant, res) if err != nil { return fmt.Errorf("failed to get attestation detail: %v", err) } @@ -165,7 +165,7 @@ func runInspect(opts *Options) error { } // otherwise, print results in a table - details, err := getDetailsAsSlice(opts.Tenant, res.VerifyResults) + details, err := getDetailsAsSlice(opts.Tenant, res) if err != nil { return fmt.Errorf("failed to parse attestation details: %v", err) } diff --git a/pkg/cmd/attestation/verification/mock_verifier.go b/pkg/cmd/attestation/verification/mock_verifier.go index e22142ed5..41332dc62 100644 --- a/pkg/cmd/attestation/verification/mock_verifier.go +++ b/pkg/cmd/attestation/verification/mock_verifier.go @@ -16,7 +16,7 @@ type MockSigstoreVerifier struct { t *testing.T } -func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults { +func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { statement := &in_toto.Statement{} statement.PredicateType = SLSAPredicateV1 @@ -41,9 +41,7 @@ func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve results := []*AttestationProcessingResult{&result} - return &SigstoreResults{ - VerifyResults: results, - } + return results, nil } func NewMockSigstoreVerifier(t *testing.T) *MockSigstoreVerifier { @@ -52,8 +50,6 @@ func NewMockSigstoreVerifier(t *testing.T) *MockSigstoreVerifier { type FailSigstoreVerifier struct{} -func (v *FailSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults { - return &SigstoreResults{ - Error: fmt.Errorf("failed to verify attestations"), - } +func (v *FailSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { + return nil, fmt.Errorf("failed to verify attestations") } diff --git a/pkg/cmd/attestation/verification/sigstore.go b/pkg/cmd/attestation/verification/sigstore.go index 5b4f4a79b..3c05ecf85 100644 --- a/pkg/cmd/attestation/verification/sigstore.go +++ b/pkg/cmd/attestation/verification/sigstore.go @@ -28,11 +28,6 @@ type AttestationProcessingResult struct { VerificationResult *verify.VerificationResult `json:"verificationResult"` } -type SigstoreResults struct { - VerifyResults []*AttestationProcessingResult - Error error -} - type SigstoreConfig struct { TrustedRoot string Logger *io.Handler @@ -42,7 +37,7 @@ type SigstoreConfig struct { } type SigstoreVerifier interface { - Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults + Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) } type LiveSigstoreVerifier struct { @@ -172,7 +167,7 @@ func getLowestCertInChain(ca *root.CertificateAuthority) (*x509.Certificate, err return nil, fmt.Errorf("certificate authority had no certificates") } -func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults { +func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { // initialize the processing apResults before attempting to verify // with multiple verifiers apResults := make([]*AttestationProcessingResult, len(attestations)) @@ -192,9 +187,7 @@ func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve // determine which verifier should attempt verification against the bundle verifier, issuer, err := v.chooseVerifier(apr.Attestation.Bundle) if err != nil { - return &SigstoreResults{ - Error: fmt.Errorf("failed to find recognized issuer from bundle content: %v", err), - } + return nil, fmt.Errorf("failed to find recognized issuer from bundle content: %v", err) } v.config.Logger.VerbosePrintf("Attempting verification against issuer \"%s\"\n", issuer) @@ -206,9 +199,7 @@ func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve "Failed to verify against issuer \"%s\" \n\n", issuer, )) - return &SigstoreResults{ - Error: fmt.Errorf("verifying with issuer \"%s\"", issuer), - } + return nil, fmt.Errorf("verifying with issuer \"%s\"", issuer) } // if verification is successful, add the result @@ -220,13 +211,11 @@ func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve atLeastOneVerified = true } - if atLeastOneVerified { - return &SigstoreResults{ - VerifyResults: apResults, - } - } else { - return &SigstoreResults{Error: ErrNoAttestationsVerified} + if !atLeastOneVerified { + return nil, ErrNoAttestationsVerified } + + return apResults, nil } func newCustomVerifier(trustedRoot *root.TrustedRoot) (*verify.SignedEntityVerifier, error) { diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 206001f9b..75b9bce1b 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -264,14 +264,14 @@ func runVerify(opts *Options) error { opts.Logger.VerbosePrintf("Verifying attestations with predicate type: %s\n", opts.PredicateType) - sigstoreRes := opts.SigstoreVerifier.Verify(attestations, policy) - if sigstoreRes.Error != nil { + sgResults, err := opts.SigstoreVerifier.Verify(attestations, policy) + if err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Verification failed")) - return sigstoreRes.Error + return err } // Verify extensions - if err := verification.VerifyCertExtensions(sigstoreRes.VerifyResults, opts.Tenant, opts.Owner, opts.Repo, opts.OIDCIssuer); err != nil { + if err := verification.VerifyCertExtensions(sgResults, opts.Tenant, opts.Owner, opts.Repo, opts.OIDCIssuer); err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Verification failed")) return err } @@ -281,7 +281,7 @@ func runVerify(opts *Options) error { // If an exporter is provided with the --json flag, write the results to the terminal in JSON format if opts.exporter != nil { // print the results to the terminal as an array of JSON objects - if err = opts.exporter.Write(opts.Logger.IO, sigstoreRes.VerifyResults); err != nil { + if err = opts.exporter.Write(opts.Logger.IO, sgResults); err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to write JSON output")) return err } @@ -291,7 +291,7 @@ func runVerify(opts *Options) error { opts.Logger.Printf("%s was attested by:\n", artifact.DigestWithAlg()) // Otherwise print the results to the terminal in a table - tableContent, err := buildTableVerifyContent(opts.Tenant, sigstoreRes.VerifyResults) + tableContent, err := buildTableVerifyContent(opts.Tenant, sgResults) if err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("failed to parse results")) return err From 97262d8ce76f71e4d6b9cf5618c2c4fde45371c4 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 10:25:45 -0600 Subject: [PATCH 24/76] add test case for monotonic verification success Signed-off-by: Meredith Lancaster --- .../attestation/verification/extensions.go | 6 ++-- .../verification/extensions_test.go | 33 ++++++++++++------- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions.go b/pkg/cmd/attestation/verification/extensions.go index 94ba88208..046f24509 100644 --- a/pkg/cmd/attestation/verification/extensions.go +++ b/pkg/cmd/attestation/verification/extensions.go @@ -24,11 +24,11 @@ func VerifyCertExtensions(results []*AttestationProcessingResult, tenant, owner, atLeastOneVerified = true } - if atLeastOneVerified { - return nil - } else { + if !atLeastOneVerified { return ErrNoAttestationsVerified } + + return nil } func verifyCertExtensions(attestation *AttestationProcessingResult, tenant, owner, repo, issuer string) error { diff --git a/pkg/cmd/attestation/verification/extensions_test.go b/pkg/cmd/attestation/verification/extensions_test.go index 5eb28829d..445234652 100644 --- a/pkg/cmd/attestation/verification/extensions_test.go +++ b/pkg/cmd/attestation/verification/extensions_test.go @@ -8,28 +8,39 @@ import ( "github.com/stretchr/testify/require" ) -func TestVerifyCertExtensions(t *testing.T) { - results := []*AttestationProcessingResult{ - { - VerificationResult: &verify.VerificationResult{ - Signature: &verify.SignatureVerificationResult{ - Certificate: &certificate.Summary{ - Extensions: certificate.Extensions{ - SourceRepositoryOwnerURI: "https://github.com/owner", - SourceRepositoryURI: "https://github.com/owner/repo", - Issuer: "https://token.actions.githubusercontent.com", - }, +func createSampleResult() *AttestationProcessingResult { + return &AttestationProcessingResult{ + VerificationResult: &verify.VerificationResult{ + Signature: &verify.SignatureVerificationResult{ + Certificate: &certificate.Summary{ + Extensions: certificate.Extensions{ + SourceRepositoryOwnerURI: "https://github.com/owner", + SourceRepositoryURI: "https://github.com/owner/repo", + Issuer: "https://token.actions.githubusercontent.com", }, }, }, }, } +} + +func TestVerifyCertExtensions(t *testing.T) { + results := []*AttestationProcessingResult{createSampleResult()} t.Run("VerifyCertExtensions with owner and repo", func(t *testing.T) { err := VerifyCertExtensions(results, "", "owner", "owner/repo", GitHubOIDCIssuer) require.NoError(t, err) }) + t.Run("VerifyCertExtensions passes with at least one successful verification", func(t *testing.T) { + twoResults := []*AttestationProcessingResult{createSampleResult(), createSampleResult()} + require.Len(t, twoResults, 2) + twoResults[1].VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong" + + err := VerifyCertExtensions(twoResults, "", "owner", "owner/repo", GitHubOIDCIssuer) + require.NoError(t, err) + }) + t.Run("VerifyCertExtensions with owner and repo, but wrong tenant", func(t *testing.T) { err := VerifyCertExtensions(results, "foo", "owner", "owner/repo", GitHubOIDCIssuer) require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://foo.ghe.com/owner, got https://github.com/owner") From d29a4a751ad5e81b0eaac0e492342e055d7eef98 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 10:44:36 -0600 Subject: [PATCH 25/76] update extension verification logic Signed-off-by: Meredith Lancaster --- .../attestation/verification/extensions.go | 18 ++- .../verification/extensions_test.go | 118 ++++++++++-------- 2 files changed, 71 insertions(+), 65 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions.go b/pkg/cmd/attestation/verification/extensions.go index 046f24509..274e833c4 100644 --- a/pkg/cmd/attestation/verification/extensions.go +++ b/pkg/cmd/attestation/verification/extensions.go @@ -16,22 +16,20 @@ func VerifyCertExtensions(results []*AttestationProcessingResult, tenant, owner, return errors.New("no attestations proccessing results") } - var atLeastOneVerified bool for _, attestation := range results { - if err := verifyCertExtensions(attestation, tenant, owner, repo, issuer); err != nil { - return err + if err := verifyCertExtension(attestation, tenant, owner, repo, issuer); 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 + return nil } - atLeastOneVerified = true } - if !atLeastOneVerified { - return ErrNoAttestationsVerified - } - - return nil + // if we have exited the for loop without returning early due to successful + // verification, we need to return an error + return ErrNoAttestationsVerified } -func verifyCertExtensions(attestation *AttestationProcessingResult, tenant, owner, repo, issuer string) error { +func verifyCertExtension(attestation *AttestationProcessingResult, tenant, owner, repo, issuer string) error { var want string if tenant == "" { diff --git a/pkg/cmd/attestation/verification/extensions_test.go b/pkg/cmd/attestation/verification/extensions_test.go index 445234652..a03cf79d6 100644 --- a/pkg/cmd/attestation/verification/extensions_test.go +++ b/pkg/cmd/attestation/verification/extensions_test.go @@ -27,12 +27,12 @@ func createSampleResult() *AttestationProcessingResult { func TestVerifyCertExtensions(t *testing.T) { results := []*AttestationProcessingResult{createSampleResult()} - t.Run("VerifyCertExtensions with owner and repo", func(t *testing.T) { + t.Run("passes with one result", func(t *testing.T) { err := VerifyCertExtensions(results, "", "owner", "owner/repo", GitHubOIDCIssuer) require.NoError(t, err) }) - t.Run("VerifyCertExtensions passes with at least one successful verification", func(t *testing.T) { + t.Run("passes with 1/2 valid results", func(t *testing.T) { twoResults := []*AttestationProcessingResult{createSampleResult(), createSampleResult()} require.Len(t, twoResults, 2) twoResults[1].VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong" @@ -41,61 +41,71 @@ func TestVerifyCertExtensions(t *testing.T) { require.NoError(t, err) }) - t.Run("VerifyCertExtensions with owner and repo, but wrong tenant", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "owner", "owner/repo", GitHubOIDCIssuer) + t.Run("fails when all results fail verification", func(t *testing.T) { + twoResults := []*AttestationProcessingResult{createSampleResult(), createSampleResult()} + require.Len(t, twoResults, 2) + twoResults[0].VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong" + twoResults[1].VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong" + + err := VerifyCertExtensions(twoResults, "", "owner", "owner/repo", GitHubOIDCIssuer) + require.NoError(t, err) + }) +} + +func TestVerifyCertExtension(t *testing.T) { + t.Run("with owner and repo, but wrong tenant", func(t *testing.T) { + err := verifyCertExtension(createSampleResult(), "foo", "owner", "owner/repo", GitHubOIDCIssuer) require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://foo.ghe.com/owner, got https://github.com/owner") }) - t.Run("VerifyCertExtensions with owner", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "", GitHubOIDCIssuer) + t.Run("with owner", func(t *testing.T) { + err := verifyCertExtension(createSampleResult(), "", "owner", "", GitHubOIDCIssuer) require.NoError(t, err) }) - t.Run("VerifyCertExtensions with wrong owner", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "wrong", "", GitHubOIDCIssuer) + t.Run("with wrong owner", func(t *testing.T) { + err := verifyCertExtension(createSampleResult(), "", "wrong", "", GitHubOIDCIssuer) require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://github.com/wrong, got https://github.com/owner") }) - t.Run("VerifyCertExtensions with wrong repo", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "wrong", GitHubOIDCIssuer) + t.Run("with wrong repo", func(t *testing.T) { + err := verifyCertExtension(createSampleResult(), "", "owner", "wrong", GitHubOIDCIssuer) require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/wrong, got https://github.com/owner/repo") }) - t.Run("VerifyCertExtensions with wrong issuer", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "", "wrong") + t.Run("with wrong issuer", func(t *testing.T) { + err := verifyCertExtension(createSampleResult(), "", "owner", "", "wrong") require.ErrorContains(t, err, "expected Issuer to be wrong, got https://token.actions.githubusercontent.com") }) } -func TestVerifyCertExtensionsCustomizedIssuer(t *testing.T) { - results := []*AttestationProcessingResult{ - { - VerificationResult: &verify.VerificationResult{ - Signature: &verify.SignatureVerificationResult{ - Certificate: &certificate.Summary{ - Extensions: certificate.Extensions{ - SourceRepositoryOwnerURI: "https://github.com/owner", - SourceRepositoryURI: "https://github.com/owner/repo", - Issuer: "https://token.actions.githubusercontent.com/foo-bar", - }, +func TestVerifyCertExtensionCustomizedIssuer(t *testing.T) { + result := &AttestationProcessingResult{ + VerificationResult: &verify.VerificationResult{ + Signature: &verify.SignatureVerificationResult{ + Certificate: &certificate.Summary{ + Extensions: certificate.Extensions{ + SourceRepositoryOwnerURI: "https://github.com/owner", + SourceRepositoryURI: "https://github.com/owner/repo", + Issuer: "https://token.actions.githubusercontent.com/foo-bar", }, }, }, }, } - t.Run("VerifyCertExtensions with exact issuer match", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "owner/repo", "https://token.actions.githubusercontent.com/foo-bar") + t.Run("with exact issuer match", func(t *testing.T) { + err := verifyCertExtension(result, "", "owner", "owner/repo", "https://token.actions.githubusercontent.com/foo-bar") require.NoError(t, err) }) - t.Run("VerifyCertExtensions with partial issuer match", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "owner/repo", "https://token.actions.githubusercontent.com") + t.Run("with partial issuer match", func(t *testing.T) { + err := verifyCertExtension(result, "", "owner", "owner/repo", "https://token.actions.githubusercontent.com") require.ErrorContains(t, err, "expected Issuer to be https://token.actions.githubusercontent.com, got https://token.actions.githubusercontent.com/foo-bar -- if you have a custom OIDC issuer") }) - t.Run("VerifyCertExtensions with wrong issuer", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "", "wrong") + t.Run("with wrong issuer", func(t *testing.T) { + err := verifyCertExtension(result, "", "owner", "", "wrong") require.ErrorContains(t, err, "expected Issuer to be wrong, got https://token.actions.githubusercontent.com/foo-bar") }) } @@ -103,59 +113,57 @@ func TestVerifyCertExtensionsCustomizedIssuer(t *testing.T) { func TestVerifyTenancyCertExtensions(t *testing.T) { defaultIssuer := GitHubOIDCIssuer - results := []*AttestationProcessingResult{ - { - VerificationResult: &verify.VerificationResult{ - Signature: &verify.SignatureVerificationResult{ - Certificate: &certificate.Summary{ - Extensions: certificate.Extensions{ - SourceRepositoryOwnerURI: "https://foo.ghe.com/owner", - SourceRepositoryURI: "https://foo.ghe.com/owner/repo", - Issuer: "https://token.actions.foo.ghe.com", - }, + result := &AttestationProcessingResult{ + VerificationResult: &verify.VerificationResult{ + Signature: &verify.SignatureVerificationResult{ + Certificate: &certificate.Summary{ + Extensions: certificate.Extensions{ + SourceRepositoryOwnerURI: "https://foo.ghe.com/owner", + SourceRepositoryURI: "https://foo.ghe.com/owner/repo", + Issuer: "https://token.actions.foo.ghe.com", }, }, }, }, } - t.Run("VerifyCertExtensions with owner and repo", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "owner", "owner/repo", defaultIssuer) + t.Run("with owner and repo", func(t *testing.T) { + err := verifyCertExtension(result, "foo", "owner", "owner/repo", defaultIssuer) require.NoError(t, err) }) - t.Run("VerifyCertExtensions with owner and repo, no tenant", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "owner/repo", defaultIssuer) + t.Run("with owner and repo, no tenant", func(t *testing.T) { + err := verifyCertExtension(result, "", "owner", "owner/repo", defaultIssuer) require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://github.com/owner, got https://foo.ghe.com/owner") }) - t.Run("VerifyCertExtensions with owner and repo, wrong tenant", func(t *testing.T) { - err := VerifyCertExtensions(results, "bar", "owner", "owner/repo", defaultIssuer) + t.Run("with owner and repo, wrong tenant", func(t *testing.T) { + err := verifyCertExtension(result, "bar", "owner", "owner/repo", defaultIssuer) require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://bar.ghe.com/owner, got https://foo.ghe.com/owner") }) - t.Run("VerifyCertExtensions with owner", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "owner", "", defaultIssuer) + t.Run("with owner", func(t *testing.T) { + err := verifyCertExtension(result, "foo", "owner", "", defaultIssuer) require.NoError(t, err) }) - t.Run("VerifyCertExtensions with wrong owner", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "wrong", "", defaultIssuer) + t.Run("with wrong owner", func(t *testing.T) { + err := verifyCertExtension(result, "foo", "wrong", "", defaultIssuer) require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://foo.ghe.com/wrong, got https://foo.ghe.com/owner") }) - t.Run("VerifyCertExtensions with wrong repo", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "owner", "wrong", defaultIssuer) + t.Run("with wrong repo", func(t *testing.T) { + err := verifyCertExtension(result, "foo", "owner", "wrong", defaultIssuer) require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://foo.ghe.com/wrong, got https://foo.ghe.com/owner/repo") }) - t.Run("VerifyCertExtensions with correct, non-default issuer", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "owner", "owner/repo", "https://token.actions.foo.ghe.com") + t.Run("with correct, non-default issuer", func(t *testing.T) { + err := verifyCertExtension(result, "foo", "owner", "owner/repo", "https://token.actions.foo.ghe.com") require.NoError(t, err) }) - t.Run("VerifyCertExtensions with wrong issuer", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "owner", "owner/repo", "wrong") + t.Run("with wrong issuer", func(t *testing.T) { + err := verifyCertExtension(result, "foo", "owner", "owner/repo", "wrong") require.ErrorContains(t, err, "expected Issuer to be wrong, got https://token.actions.foo.ghe.com") }) } From 3e90628abbbb361eb53ab91286ed0b244aad4fe1 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 11:23:15 -0600 Subject: [PATCH 26/76] add test for sigstore monotonic verification Signed-off-by: Meredith Lancaster --- .../verification/extensions_test.go | 2 +- .../verification/sigstore_integration_test.go | 39 +++++++++++++------ 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions_test.go b/pkg/cmd/attestation/verification/extensions_test.go index a03cf79d6..87249cb7b 100644 --- a/pkg/cmd/attestation/verification/extensions_test.go +++ b/pkg/cmd/attestation/verification/extensions_test.go @@ -48,7 +48,7 @@ func TestVerifyCertExtensions(t *testing.T) { twoResults[1].VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong" err := VerifyCertExtensions(twoResults, "", "owner", "owner/repo", GitHubOIDCIssuer) - require.NoError(t, err) + require.Error(t, err) }) } diff --git a/pkg/cmd/attestation/verification/sigstore_integration_test.go b/pkg/cmd/attestation/verification/sigstore_integration_test.go index b7057505e..f1d0729a6 100644 --- a/pkg/cmd/attestation/verification/sigstore_integration_test.go +++ b/pkg/cmd/attestation/verification/sigstore_integration_test.go @@ -52,18 +52,33 @@ func TestLiveSigstoreVerifier(t *testing.T) { Logger: io.NewTestHandler(), }) - res := verifier.Verify(tc.attestations, publicGoodPolicy(t)) + results, err := verifier.Verify(tc.attestations, publicGoodPolicy(t)) if tc.expectErr { - require.Error(t, res.Error, "test case: %s", tc.name) - require.ErrorContains(t, res.Error, tc.errContains, "test case: %s", tc.name) - require.Nil(t, res.VerifyResults, "test case: %s", tc.name) + require.Error(t, err, "test case: %s", tc.name) + require.ErrorContains(t, err, tc.errContains, "test case: %s", tc.name) + require.Nil(t, results, "test case: %s", tc.name) } else { - require.Equal(t, len(tc.attestations), len(res.VerifyResults), "test case: %s", tc.name) - require.NoError(t, res.Error, "test case: %s", tc.name) + require.Equal(t, len(tc.attestations), len(results), "test case: %s", tc.name) + require.NoError(t, err, "test case: %s", tc.name) } } + t.Run("with 2/3 verified attestations", func(t *testing.T) { + verifier := NewLiveSigstoreVerifier(SigstoreConfig{ + Logger: io.NewTestHandler(), + }) + + 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]) + + results, err := verifier.Verify(attestations, publicGoodPolicy(t)) + + require.Equal(t, len(attestations), len(results)) + require.NoError(t, err) + }) + t.Run("with GitHub Sigstore artifact", func(t *testing.T) { githubArtifactPath := test.NormalizeRelativePath("../test/data/github_provenance_demo-0.0.12-py3-none-any.whl") githubArtifact, err := artifact.NewDigestedArtifact(nil, githubArtifactPath, "sha256") @@ -77,9 +92,9 @@ func TestLiveSigstoreVerifier(t *testing.T) { Logger: io.NewTestHandler(), }) - res := verifier.Verify(attestations, githubPolicy) - require.Len(t, res.VerifyResults, 1) - require.NoError(t, res.Error) + results, err := verifier.Verify(attestations, githubPolicy) + require.Len(t, results, 1) + require.NoError(t, err) }) t.Run("with custom trusted root", func(t *testing.T) { @@ -90,9 +105,9 @@ func TestLiveSigstoreVerifier(t *testing.T) { TrustedRoot: test.NormalizeRelativePath("../test/data/trusted_root.json"), }) - res := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Len(t, res.VerifyResults, 2) - require.NoError(t, res.Error) + results, err := verifier.Verify(attestations, publicGoodPolicy(t)) + require.Len(t, results, 2) + require.NoError(t, err) }) } From 26e04932f2d28f43962cb0791b518b634915634d Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 11:59:32 -0600 Subject: [PATCH 27/76] split out individual sigstore verification Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verification/sigstore.go | 74 ++++++++++--------- .../verification/sigstore_integration_test.go | 18 ++++- 2 files changed, 55 insertions(+), 37 deletions(-) diff --git a/pkg/cmd/attestation/verification/sigstore.go b/pkg/cmd/attestation/verification/sigstore.go index 3c05ecf85..1577ff663 100644 --- a/pkg/cmd/attestation/verification/sigstore.go +++ b/pkg/cmd/attestation/verification/sigstore.go @@ -167,55 +167,57 @@ func getLowestCertInChain(ca *root.CertificateAuthority) (*x509.Certificate, err return nil, fmt.Errorf("certificate authority had no certificates") } -func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { - // initialize the processing apResults before attempting to verify - // with multiple verifiers - apResults := make([]*AttestationProcessingResult, len(attestations)) - for i, att := range attestations { - apr := &AttestationProcessingResult{ - Attestation: att, - } - apResults[i] = apr +func (v *LiveSigstoreVerifier) verify(attestation *api.Attestation, policy verify.PolicyBuilder) (*AttestationProcessingResult, error) { + // determine which verifier should attempt verification against the bundle + verifier, issuer, err := v.chooseVerifier(attestation.Bundle) + if err != nil { + return nil, fmt.Errorf("failed to find recognized issuer from bundle content: %v", err) } - var atLeastOneVerified bool + v.config.Logger.VerbosePrintf("Attempting verification against issuer \"%s\"\n", issuer) + // attempt to verify the attestation + result, err := verifier.Verify(attestation.Bundle, policy) + // if verification fails, create the error and exit verification early + if err != nil { + v.config.Logger.VerbosePrint(v.config.Logger.ColorScheme.Redf( + "Failed to verify against issuer \"%s\" \n\n", issuer, + )) + + return nil, fmt.Errorf("verifying with issuer \"%s\"", issuer) + } + + // if verification is successful, add the result + // to the AttestationProcessingResult entry + v.config.Logger.VerbosePrint(v.config.Logger.ColorScheme.Greenf( + "SUCCESS - attestation signature verified with \"%s\"\n", issuer, + )) + + return &AttestationProcessingResult{ + Attestation: attestation, + VerificationResult: result, + }, nil +} + +func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { + results := make([]*AttestationProcessingResult, 0) totalAttestations := len(attestations) - for i, apr := range apResults { + for i, a := range attestations { v.config.Logger.VerbosePrintf("Verifying attestation %d/%d against the configured Sigstore trust roots\n", i+1, totalAttestations) - // determine which verifier should attempt verification against the bundle - verifier, issuer, err := v.chooseVerifier(apr.Attestation.Bundle) + apr, err := v.verify(a, policy) if err != nil { - return nil, fmt.Errorf("failed to find recognized issuer from bundle content: %v", err) + // move onto the next attestation if verification fails + continue } - - v.config.Logger.VerbosePrintf("Attempting verification against issuer \"%s\"\n", issuer) - // attempt to verify the attestation - result, err := verifier.Verify(apr.Attestation.Bundle, policy) - // if verification fails, create the error and exit verification early - if err != nil { - v.config.Logger.VerbosePrint(v.config.Logger.ColorScheme.Redf( - "Failed to verify against issuer \"%s\" \n\n", issuer, - )) - - return nil, fmt.Errorf("verifying with issuer \"%s\"", issuer) - } - - // if verification is successful, add the result - // to the AttestationProcessingResult entry - v.config.Logger.VerbosePrint(v.config.Logger.ColorScheme.Greenf( - "SUCCESS - attestation signature verified with \"%s\"\n", issuer, - )) - apr.VerificationResult = result - atLeastOneVerified = true + results = append(results, apr) } - if !atLeastOneVerified { + if len(results) == 0 { return nil, ErrNoAttestationsVerified } - return apResults, nil + return results, nil } func newCustomVerifier(trustedRoot *root.TrustedRoot) (*verify.SignedEntityVerifier, error) { diff --git a/pkg/cmd/attestation/verification/sigstore_integration_test.go b/pkg/cmd/attestation/verification/sigstore_integration_test.go index f1d0729a6..e14b472b0 100644 --- a/pkg/cmd/attestation/verification/sigstore_integration_test.go +++ b/pkg/cmd/attestation/verification/sigstore_integration_test.go @@ -72,13 +72,29 @@ func TestLiveSigstoreVerifier(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, err := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Equal(t, len(attestations), len(results)) + require.Len(t, results, 2) require.NoError(t, err) }) + t.Run("fail with 0/2 verified attestations", func(t *testing.T) { + verifier := NewLiveSigstoreVerifier(SigstoreConfig{ + Logger: io.NewTestHandler(), + }) + + invalidBundle := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json") + attestations := getAttestationsFor(t, "../test/data/sigstoreBundle-invalid-signature.json") + attestations = append(attestations, invalidBundle[0]) + require.Len(t, attestations, 2) + + results, err := verifier.Verify(attestations, publicGoodPolicy(t)) + require.Nil(t, results) + require.Error(t, err) + }) + t.Run("with GitHub Sigstore artifact", func(t *testing.T) { githubArtifactPath := test.NormalizeRelativePath("../test/data/github_provenance_demo-0.0.12-py3-none-any.whl") githubArtifact, err := artifact.NewDigestedArtifact(nil, githubArtifactPath, "sha256") From 56731c9b7021457acbc47c4378aed3f90b8f1930 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 12:26:06 -0600 Subject: [PATCH 28/76] remove unneeded result handling struct Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/inspect/inspect.go | 10 ++++---- .../attestation/verification/mock_verifier.go | 12 +++------ pkg/cmd/attestation/verification/sigstore.go | 25 ++++++------------- .../verification/sigstore_integration_test.go | 24 +++++++++--------- pkg/cmd/attestation/verify/verify.go | 12 ++++----- 5 files changed, 34 insertions(+), 49 deletions(-) diff --git a/pkg/cmd/attestation/inspect/inspect.go b/pkg/cmd/attestation/inspect/inspect.go index c139a5af2..a392656e1 100644 --- a/pkg/cmd/attestation/inspect/inspect.go +++ b/pkg/cmd/attestation/inspect/inspect.go @@ -141,9 +141,9 @@ func runInspect(opts *Options) error { return fmt.Errorf("failed to build policy: %v", err) } - 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) + results, err := opts.SigstoreVerifier.Verify(attestations, policy) + if err != nil { + return fmt.Errorf("at least one attestation failed to verify against Sigstore: %v", err) } opts.Logger.VerbosePrint(opts.Logger.ColorScheme.Green( @@ -152,7 +152,7 @@ func runInspect(opts *Options) error { // If the user provides the --format=json flag, print the results in JSON format if opts.exporter != nil { - details, err := getAttestationDetails(opts.Tenant, res.VerifyResults) + details, err := getAttestationDetails(opts.Tenant, results) if err != nil { return fmt.Errorf("failed to get attestation detail: %v", err) } @@ -165,7 +165,7 @@ func runInspect(opts *Options) error { } // otherwise, print results in a table - details, err := getDetailsAsSlice(opts.Tenant, res.VerifyResults) + details, err := getDetailsAsSlice(opts.Tenant, results) if err != nil { return fmt.Errorf("failed to parse attestation details: %v", err) } diff --git a/pkg/cmd/attestation/verification/mock_verifier.go b/pkg/cmd/attestation/verification/mock_verifier.go index e22142ed5..41332dc62 100644 --- a/pkg/cmd/attestation/verification/mock_verifier.go +++ b/pkg/cmd/attestation/verification/mock_verifier.go @@ -16,7 +16,7 @@ type MockSigstoreVerifier struct { t *testing.T } -func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults { +func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { statement := &in_toto.Statement{} statement.PredicateType = SLSAPredicateV1 @@ -41,9 +41,7 @@ func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve results := []*AttestationProcessingResult{&result} - return &SigstoreResults{ - VerifyResults: results, - } + return results, nil } func NewMockSigstoreVerifier(t *testing.T) *MockSigstoreVerifier { @@ -52,8 +50,6 @@ func NewMockSigstoreVerifier(t *testing.T) *MockSigstoreVerifier { type FailSigstoreVerifier struct{} -func (v *FailSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults { - return &SigstoreResults{ - Error: fmt.Errorf("failed to verify attestations"), - } +func (v *FailSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { + return nil, fmt.Errorf("failed to verify attestations") } diff --git a/pkg/cmd/attestation/verification/sigstore.go b/pkg/cmd/attestation/verification/sigstore.go index 5b4f4a79b..53200c957 100644 --- a/pkg/cmd/attestation/verification/sigstore.go +++ b/pkg/cmd/attestation/verification/sigstore.go @@ -28,11 +28,6 @@ type AttestationProcessingResult struct { VerificationResult *verify.VerificationResult `json:"verificationResult"` } -type SigstoreResults struct { - VerifyResults []*AttestationProcessingResult - Error error -} - type SigstoreConfig struct { TrustedRoot string Logger *io.Handler @@ -42,7 +37,7 @@ type SigstoreConfig struct { } type SigstoreVerifier interface { - Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults + Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) } type LiveSigstoreVerifier struct { @@ -172,7 +167,7 @@ func getLowestCertInChain(ca *root.CertificateAuthority) (*x509.Certificate, err return nil, fmt.Errorf("certificate authority had no certificates") } -func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults { +func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { // initialize the processing apResults before attempting to verify // with multiple verifiers apResults := make([]*AttestationProcessingResult, len(attestations)) @@ -192,9 +187,7 @@ func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve // determine which verifier should attempt verification against the bundle verifier, issuer, err := v.chooseVerifier(apr.Attestation.Bundle) if err != nil { - return &SigstoreResults{ - Error: fmt.Errorf("failed to find recognized issuer from bundle content: %v", err), - } + return nil, fmt.Errorf("failed to find recognized issuer from bundle content: %v", err) } v.config.Logger.VerbosePrintf("Attempting verification against issuer \"%s\"\n", issuer) @@ -206,9 +199,7 @@ func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve "Failed to verify against issuer \"%s\" \n\n", issuer, )) - return &SigstoreResults{ - Error: fmt.Errorf("verifying with issuer \"%s\"", issuer), - } + return nil, fmt.Errorf("verifying with issuer \"%s\"", issuer) } // if verification is successful, add the result @@ -221,12 +212,10 @@ func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve } if atLeastOneVerified { - return &SigstoreResults{ - VerifyResults: apResults, - } - } else { - return &SigstoreResults{Error: ErrNoAttestationsVerified} + return apResults, nil } + + return nil, ErrNoAttestationsVerified } func newCustomVerifier(trustedRoot *root.TrustedRoot) (*verify.SignedEntityVerifier, error) { diff --git a/pkg/cmd/attestation/verification/sigstore_integration_test.go b/pkg/cmd/attestation/verification/sigstore_integration_test.go index b7057505e..56b285055 100644 --- a/pkg/cmd/attestation/verification/sigstore_integration_test.go +++ b/pkg/cmd/attestation/verification/sigstore_integration_test.go @@ -52,15 +52,15 @@ func TestLiveSigstoreVerifier(t *testing.T) { Logger: io.NewTestHandler(), }) - res := verifier.Verify(tc.attestations, publicGoodPolicy(t)) + results, err := verifier.Verify(tc.attestations, publicGoodPolicy(t)) if tc.expectErr { - require.Error(t, res.Error, "test case: %s", tc.name) - require.ErrorContains(t, res.Error, tc.errContains, "test case: %s", tc.name) - require.Nil(t, res.VerifyResults, "test case: %s", tc.name) + require.Error(t, err, "test case: %s", tc.name) + require.ErrorContains(t, err, tc.errContains, "test case: %s", tc.name) + require.Nil(t, results, "test case: %s", tc.name) } else { - require.Equal(t, len(tc.attestations), len(res.VerifyResults), "test case: %s", tc.name) - require.NoError(t, res.Error, "test case: %s", tc.name) + require.Equal(t, len(tc.attestations), len(results), "test case: %s", tc.name) + require.NoError(t, err, "test case: %s", tc.name) } } @@ -77,9 +77,9 @@ func TestLiveSigstoreVerifier(t *testing.T) { Logger: io.NewTestHandler(), }) - res := verifier.Verify(attestations, githubPolicy) - require.Len(t, res.VerifyResults, 1) - require.NoError(t, res.Error) + results, err := verifier.Verify(attestations, githubPolicy) + require.Len(t, results, 1) + require.NoError(t, err) }) t.Run("with custom trusted root", func(t *testing.T) { @@ -90,9 +90,9 @@ func TestLiveSigstoreVerifier(t *testing.T) { TrustedRoot: test.NormalizeRelativePath("../test/data/trusted_root.json"), }) - res := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Len(t, res.VerifyResults, 2) - require.NoError(t, res.Error) + results, err := verifier.Verify(attestations, publicGoodPolicy(t)) + require.Len(t, results, 2) + require.NoError(t, err) }) } diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 206001f9b..4476f2ef7 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -264,14 +264,14 @@ func runVerify(opts *Options) error { opts.Logger.VerbosePrintf("Verifying attestations with predicate type: %s\n", opts.PredicateType) - sigstoreRes := opts.SigstoreVerifier.Verify(attestations, policy) - if sigstoreRes.Error != nil { + results, err := opts.SigstoreVerifier.Verify(attestations, policy) + if err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Verification failed")) - return sigstoreRes.Error + return err } // Verify extensions - if err := verification.VerifyCertExtensions(sigstoreRes.VerifyResults, opts.Tenant, opts.Owner, opts.Repo, opts.OIDCIssuer); err != nil { + if err := verification.VerifyCertExtensions(results, opts.Tenant, opts.Owner, opts.Repo, opts.OIDCIssuer); err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Verification failed")) return err } @@ -281,7 +281,7 @@ func runVerify(opts *Options) error { // If an exporter is provided with the --json flag, write the results to the terminal in JSON format if opts.exporter != nil { // print the results to the terminal as an array of JSON objects - if err = opts.exporter.Write(opts.Logger.IO, sigstoreRes.VerifyResults); err != nil { + if err = opts.exporter.Write(opts.Logger.IO, results); err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to write JSON output")) return err } @@ -291,7 +291,7 @@ func runVerify(opts *Options) error { opts.Logger.Printf("%s was attested by:\n", artifact.DigestWithAlg()) // Otherwise print the results to the terminal in a table - tableContent, err := buildTableVerifyContent(opts.Tenant, sigstoreRes.VerifyResults) + tableContent, err := buildTableVerifyContent(opts.Tenant, results) if err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("failed to parse results")) return err From 4bd46334ffc227df43f4208a6c9e4cf73d4731e6 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 12:38:37 -0600 Subject: [PATCH 29/76] return the last verification error for now Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verification/extensions.go | 7 +++++-- pkg/cmd/attestation/verification/sigstore.go | 11 ++++++++--- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions.go b/pkg/cmd/attestation/verification/extensions.go index 274e833c4..4c748e3e9 100644 --- a/pkg/cmd/attestation/verification/extensions.go +++ b/pkg/cmd/attestation/verification/extensions.go @@ -16,17 +16,20 @@ func VerifyCertExtensions(results []*AttestationProcessingResult, tenant, owner, return errors.New("no attestations proccessing results") } + var lastErr error for _, attestation := range results { - if err := verifyCertExtension(attestation, tenant, owner, repo, issuer); err == nil { + err := verifyCertExtension(attestation, tenant, owner, repo, issuer) + 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 return nil } + lastErr = err } // if we have exited the for loop without returning early due to successful // verification, we need to return an error - return ErrNoAttestationsVerified + return lastErr } func verifyCertExtension(attestation *AttestationProcessingResult, tenant, owner, repo, issuer string) error { diff --git a/pkg/cmd/attestation/verification/sigstore.go b/pkg/cmd/attestation/verification/sigstore.go index 1577ff663..825f9da1c 100644 --- a/pkg/cmd/attestation/verification/sigstore.go +++ b/pkg/cmd/attestation/verification/sigstore.go @@ -199,22 +199,27 @@ func (v *LiveSigstoreVerifier) verify(attestation *api.Attestation, policy verif } func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { - results := make([]*AttestationProcessingResult, 0) + if len(attestations) == 0 { + return nil, ErrNoAttestationsVerified + } + results := make([]*AttestationProcessingResult, 0) + var lastError error totalAttestations := len(attestations) for i, a := range attestations { v.config.Logger.VerbosePrintf("Verifying attestation %d/%d against the configured Sigstore trust roots\n", i+1, totalAttestations) apr, err := v.verify(a, policy) if err != nil { - // move onto the next attestation if verification fails + lastError = err + // move onto the next attestation in the for loop if verification fails continue } results = append(results, apr) } if len(results) == 0 { - return nil, ErrNoAttestationsVerified + return nil, lastError } return results, nil From 23374d8c623045a0f437ea49a45aa0c255de9e71 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 12:49:01 -0600 Subject: [PATCH 30/76] undo sigstore verify result handling changes for now Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/inspect/inspect.go | 10 +++--- .../attestation/verification/mock_verifier.go | 12 ++++--- pkg/cmd/attestation/verification/sigstore.go | 15 +++++--- .../verification/sigstore_integration_test.go | 36 +++++++++---------- pkg/cmd/attestation/verify/verify.go | 12 +++---- 5 files changed, 47 insertions(+), 38 deletions(-) diff --git a/pkg/cmd/attestation/inspect/inspect.go b/pkg/cmd/attestation/inspect/inspect.go index 31afb7ce6..c139a5af2 100644 --- a/pkg/cmd/attestation/inspect/inspect.go +++ b/pkg/cmd/attestation/inspect/inspect.go @@ -141,9 +141,9 @@ func runInspect(opts *Options) error { return fmt.Errorf("failed to build policy: %v", err) } - res, err := opts.SigstoreVerifier.Verify(attestations, policy) - if err != nil { - return fmt.Errorf("at least one attestation failed to verify against Sigstore: %v", err) + 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) } opts.Logger.VerbosePrint(opts.Logger.ColorScheme.Green( @@ -152,7 +152,7 @@ func runInspect(opts *Options) error { // If the user provides the --format=json flag, print the results in JSON format if opts.exporter != nil { - details, err := getAttestationDetails(opts.Tenant, res) + details, err := getAttestationDetails(opts.Tenant, res.VerifyResults) if err != nil { return fmt.Errorf("failed to get attestation detail: %v", err) } @@ -165,7 +165,7 @@ func runInspect(opts *Options) error { } // otherwise, print results in a table - details, err := getDetailsAsSlice(opts.Tenant, res) + details, err := getDetailsAsSlice(opts.Tenant, res.VerifyResults) if err != nil { return fmt.Errorf("failed to parse attestation details: %v", err) } diff --git a/pkg/cmd/attestation/verification/mock_verifier.go b/pkg/cmd/attestation/verification/mock_verifier.go index 41332dc62..e22142ed5 100644 --- a/pkg/cmd/attestation/verification/mock_verifier.go +++ b/pkg/cmd/attestation/verification/mock_verifier.go @@ -16,7 +16,7 @@ type MockSigstoreVerifier struct { t *testing.T } -func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { +func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults { statement := &in_toto.Statement{} statement.PredicateType = SLSAPredicateV1 @@ -41,7 +41,9 @@ func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve results := []*AttestationProcessingResult{&result} - return results, nil + return &SigstoreResults{ + VerifyResults: results, + } } func NewMockSigstoreVerifier(t *testing.T) *MockSigstoreVerifier { @@ -50,6 +52,8 @@ func NewMockSigstoreVerifier(t *testing.T) *MockSigstoreVerifier { type FailSigstoreVerifier struct{} -func (v *FailSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { - return nil, fmt.Errorf("failed to verify attestations") +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 825f9da1c..34904988f 100644 --- a/pkg/cmd/attestation/verification/sigstore.go +++ b/pkg/cmd/attestation/verification/sigstore.go @@ -28,6 +28,11 @@ type AttestationProcessingResult struct { VerificationResult *verify.VerificationResult `json:"verificationResult"` } +type SigstoreResults struct { + VerifyResults []*AttestationProcessingResult + Error error +} + type SigstoreConfig struct { TrustedRoot string Logger *io.Handler @@ -37,7 +42,7 @@ type SigstoreConfig struct { } type SigstoreVerifier interface { - Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) + Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults } type LiveSigstoreVerifier struct { @@ -198,9 +203,9 @@ func (v *LiveSigstoreVerifier) verify(attestation *api.Attestation, policy verif }, nil } -func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { +func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults { if len(attestations) == 0 { - return nil, ErrNoAttestationsVerified + return &SigstoreResults{Error: ErrNoAttestationsVerified} } results := make([]*AttestationProcessingResult, 0) @@ -219,10 +224,10 @@ func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve } if len(results) == 0 { - return nil, lastError + return &SigstoreResults{Error: lastError} } - return results, nil + return &SigstoreResults{VerifyResults: results} } func newCustomVerifier(trustedRoot *root.TrustedRoot) (*verify.SignedEntityVerifier, error) { diff --git a/pkg/cmd/attestation/verification/sigstore_integration_test.go b/pkg/cmd/attestation/verification/sigstore_integration_test.go index e14b472b0..84dd1695a 100644 --- a/pkg/cmd/attestation/verification/sigstore_integration_test.go +++ b/pkg/cmd/attestation/verification/sigstore_integration_test.go @@ -52,15 +52,15 @@ func TestLiveSigstoreVerifier(t *testing.T) { Logger: io.NewTestHandler(), }) - results, err := verifier.Verify(tc.attestations, publicGoodPolicy(t)) + res := verifier.Verify(tc.attestations, publicGoodPolicy(t)) if tc.expectErr { - require.Error(t, err, "test case: %s", tc.name) - require.ErrorContains(t, err, tc.errContains, "test case: %s", tc.name) - require.Nil(t, results, "test case: %s", tc.name) + require.Error(t, res.Error, "test case: %s", tc.name) + require.ErrorContains(t, res.Error, tc.errContains, "test case: %s", tc.name) + require.Nil(t, res.VerifyResults, "test case: %s", tc.name) } else { - require.Equal(t, len(tc.attestations), len(results), "test case: %s", tc.name) - require.NoError(t, err, "test case: %s", tc.name) + require.Equal(t, len(tc.attestations), len(res.VerifyResults), "test case: %s", tc.name) + require.NoError(t, res.Error, "test case: %s", tc.name) } } @@ -74,10 +74,10 @@ func TestLiveSigstoreVerifier(t *testing.T) { attestations = append(attestations, invalidBundle[0]) require.Len(t, attestations, 3) - results, err := verifier.Verify(attestations, publicGoodPolicy(t)) + res := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Len(t, results, 2) - require.NoError(t, err) + require.Len(t, res.VerifyResults, 2) + require.NoError(t, res.Error) }) t.Run("fail with 0/2 verified attestations", func(t *testing.T) { @@ -90,9 +90,9 @@ func TestLiveSigstoreVerifier(t *testing.T) { attestations = append(attestations, invalidBundle[0]) require.Len(t, attestations, 2) - results, err := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Nil(t, results) - require.Error(t, err) + res := verifier.Verify(attestations, publicGoodPolicy(t)) + require.Nil(t, res.VerifyResults) + require.Error(t, res.Error) }) t.Run("with GitHub Sigstore artifact", func(t *testing.T) { @@ -108,9 +108,9 @@ func TestLiveSigstoreVerifier(t *testing.T) { Logger: io.NewTestHandler(), }) - results, err := verifier.Verify(attestations, githubPolicy) - require.Len(t, results, 1) - require.NoError(t, err) + res := verifier.Verify(attestations, githubPolicy) + require.Len(t, res.VerifyResults, 1) + require.NoError(t, res.Error) }) t.Run("with custom trusted root", func(t *testing.T) { @@ -121,9 +121,9 @@ func TestLiveSigstoreVerifier(t *testing.T) { TrustedRoot: test.NormalizeRelativePath("../test/data/trusted_root.json"), }) - results, err := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Len(t, results, 2) - require.NoError(t, err) + res := verifier.Verify(attestations, publicGoodPolicy(t)) + require.Len(t, res.VerifyResults, 2) + require.NoError(t, res.Error) }) } diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 75b9bce1b..206001f9b 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -264,14 +264,14 @@ func runVerify(opts *Options) error { opts.Logger.VerbosePrintf("Verifying attestations with predicate type: %s\n", opts.PredicateType) - sgResults, err := opts.SigstoreVerifier.Verify(attestations, policy) - if err != nil { + sigstoreRes := opts.SigstoreVerifier.Verify(attestations, policy) + if sigstoreRes.Error != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Verification failed")) - return err + return sigstoreRes.Error } // Verify extensions - if err := verification.VerifyCertExtensions(sgResults, opts.Tenant, opts.Owner, opts.Repo, opts.OIDCIssuer); err != nil { + if err := verification.VerifyCertExtensions(sigstoreRes.VerifyResults, opts.Tenant, opts.Owner, opts.Repo, opts.OIDCIssuer); err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Verification failed")) return err } @@ -281,7 +281,7 @@ func runVerify(opts *Options) error { // If an exporter is provided with the --json flag, write the results to the terminal in JSON format if opts.exporter != nil { // print the results to the terminal as an array of JSON objects - if err = opts.exporter.Write(opts.Logger.IO, sgResults); err != nil { + if err = opts.exporter.Write(opts.Logger.IO, sigstoreRes.VerifyResults); err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to write JSON output")) return err } @@ -291,7 +291,7 @@ func runVerify(opts *Options) error { opts.Logger.Printf("%s was attested by:\n", artifact.DigestWithAlg()) // Otherwise print the results to the terminal in a table - tableContent, err := buildTableVerifyContent(opts.Tenant, sgResults) + tableContent, err := buildTableVerifyContent(opts.Tenant, sigstoreRes.VerifyResults) if err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("failed to parse results")) return err From 6f4b5ddc40e045e3869bf79c0f0f6e5d4aedb1f7 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 16:07:25 -0600 Subject: [PATCH 31/76] remove artifact from EnforcementCriteria Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verification/policy.go | 1 - pkg/cmd/attestation/verify/policy.go | 10 ++++---- pkg/cmd/attestation/verify/policy_test.go | 27 +++++++++------------- pkg/cmd/attestation/verify/verify.go | 4 ++-- 4 files changed, 17 insertions(+), 25 deletions(-) diff --git a/pkg/cmd/attestation/verification/policy.go b/pkg/cmd/attestation/verification/policy.go index 974eae4e2..fabdbc0aa 100644 --- a/pkg/cmd/attestation/verification/policy.go +++ b/pkg/cmd/attestation/verification/policy.go @@ -31,6 +31,5 @@ type Extensions struct { type EnforcementCriteria struct { Extensions Extensions PredicateType string - Artifact artifact.DigestedArtifact OIDCIssuer string } diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index b79ebacf0..38931dfc1 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -25,10 +25,8 @@ func expandToGitHubURL(tenant, ownerOrRepo string) string { return fmt.Sprintf("(?i)^https://%s.ghe.com/%s/", tenant, ownerOrRepo) } -func newEnforcementCriteria(opts *Options, a artifact.DigestedArtifact) (verification.EnforcementCriteria, error) { - c := verification.EnforcementCriteria{ - Artifact: a, - } +func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, error) { + c := verification.EnforcementCriteria{} if opts.SignerRepo != "" { signedRepoRegex := expandToGitHubURL(opts.Tenant, opts.SignerRepo) @@ -98,8 +96,8 @@ func buildCertificateIdentityOption(c verification.EnforcementCriteria) (verify. return verify.WithCertificateIdentity(certId), nil } -func SigstorePolicy(c verification.EnforcementCriteria) (verify.PolicyBuilder, error) { - artifactDigestPolicyOption, err := verification.BuildDigestPolicyOption(c.Artifact) +func SigstorePolicy(c verification.EnforcementCriteria, a artifact.DigestedArtifact) (verify.PolicyBuilder, error) { + artifactDigestPolicyOption, err := verification.BuildDigestPolicyOption(a) if err != nil { return verify.PolicyBuilder{}, err } diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index f194edb74..13a7d2822 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -3,8 +3,6 @@ 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/factory" "github.com/stretchr/testify/require" @@ -13,9 +11,6 @@ import ( func TestNewEnforcementCriteria(t *testing.T) { artifactPath := "../test/data/sigstore-js-2.1.0.tgz" - artifact, err := artifact.NewDigestedArtifact(oci.MockClient{}, artifactPath, "sha256") - require.NoError(t, err) - t.Run("sets SANRegex using SignerRepo", func(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, @@ -24,7 +19,7 @@ func TestNewEnforcementCriteria(t *testing.T) { SignerRepo: "foo/bar", } - c, err := newEnforcementCriteria(opts, *artifact) + c, err := newEnforcementCriteria(opts) require.NoError(t, err) require.Equal(t, "(?i)^https://github.com/foo/bar/", c.Extensions.SANRegex) require.Zero(t, c.Extensions.SAN) @@ -39,7 +34,7 @@ func TestNewEnforcementCriteria(t *testing.T) { Hostname: "github.com", } - c, err := newEnforcementCriteria(opts, *artifact) + c, err := newEnforcementCriteria(opts) require.NoError(t, err) require.Equal(t, "^https://github.com/foo/bar/.github/workflows/attest.yml", c.Extensions.SANRegex) require.Zero(t, c.Extensions.SAN) @@ -54,7 +49,7 @@ func TestNewEnforcementCriteria(t *testing.T) { SANRegex: "(?i)^https://github/foo", } - c, err := newEnforcementCriteria(opts, *artifact) + c, err := newEnforcementCriteria(opts) require.NoError(t, err) require.Equal(t, "https://github/foo/bar/.github/workflows/attest.yml", c.Extensions.SAN) require.Equal(t, "(?i)^https://github/foo", c.Extensions.SANRegex) @@ -68,7 +63,7 @@ func TestNewEnforcementCriteria(t *testing.T) { DenySelfHostedRunner: true, } - c, err := newEnforcementCriteria(opts, *artifact) + c, err := newEnforcementCriteria(opts) require.NoError(t, err) require.Equal(t, GitHubRunner, c.Extensions.RunnerEnvironment) }) @@ -81,7 +76,7 @@ func TestNewEnforcementCriteria(t *testing.T) { DenySelfHostedRunner: false, } - c, err := newEnforcementCriteria(opts, *artifact) + c, err := newEnforcementCriteria(opts) require.NoError(t, err) require.Zero(t, c.Extensions.RunnerEnvironment) }) @@ -94,7 +89,7 @@ func TestNewEnforcementCriteria(t *testing.T) { Tenant: "baz", } - c, err := newEnforcementCriteria(opts, *artifact) + c, err := newEnforcementCriteria(opts) require.NoError(t, err) require.Equal(t, "https://baz.ghe.com/foo/bar", c.Extensions.SourceRepositoryURI) }) @@ -106,7 +101,7 @@ func TestNewEnforcementCriteria(t *testing.T) { Repo: "foo/bar", } - c, err := newEnforcementCriteria(opts, *artifact) + c, err := newEnforcementCriteria(opts) require.NoError(t, err) require.Equal(t, "https://github.com/foo/bar", c.Extensions.SourceRepositoryURI) }) @@ -119,7 +114,7 @@ func TestNewEnforcementCriteria(t *testing.T) { Tenant: "baz", } - c, err := newEnforcementCriteria(opts, *artifact) + c, err := newEnforcementCriteria(opts) require.NoError(t, err) require.Equal(t, "https://baz.ghe.com/foo", c.Extensions.SourceRepositoryOwnerURI) }) @@ -131,7 +126,7 @@ func TestNewEnforcementCriteria(t *testing.T) { Repo: "foo/bar", } - c, err := newEnforcementCriteria(opts, *artifact) + c, err := newEnforcementCriteria(opts) require.NoError(t, err) require.Equal(t, "https://github.com/foo", c.Extensions.SourceRepositoryOwnerURI) }) @@ -145,7 +140,7 @@ func TestNewEnforcementCriteria(t *testing.T) { OIDCIssuer: "https://foo.com", } - c, err := newEnforcementCriteria(opts, *artifact) + c, err := newEnforcementCriteria(opts) require.NoError(t, err) require.Equal(t, "https://token.actions.baz.ghe.com", c.OIDCIssuer) }) @@ -158,7 +153,7 @@ func TestNewEnforcementCriteria(t *testing.T) { OIDCIssuer: "https://foo.com", } - c, err := newEnforcementCriteria(opts, *artifact) + c, err := newEnforcementCriteria(opts) require.NoError(t, err) require.Equal(t, "https://foo.com", c.OIDCIssuer) }) diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 3c95b17b7..3771cc315 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -258,13 +258,13 @@ func runVerify(opts *Options) error { opts.Logger.VerbosePrintf("Verifying attestations with predicate type: %s\n", opts.PredicateType) - ec, err := newEnforcementCriteria(opts, *artifact) + ec, err := newEnforcementCriteria(opts) if err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build verification policy")) return err } - sp, err := SigstorePolicy(ec) + sp, err := SigstorePolicy(ec, *artifact) if err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build Sigstore verification policy")) return err From 7948ce4dc9703045965ef342093efcf295811ff3 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 16:09:08 -0600 Subject: [PATCH 32/76] rename function Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 2 +- pkg/cmd/attestation/verify/verify.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index 38931dfc1..8ee34bc7a 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -96,7 +96,7 @@ func buildCertificateIdentityOption(c verification.EnforcementCriteria) (verify. return verify.WithCertificateIdentity(certId), nil } -func SigstorePolicy(c verification.EnforcementCriteria, a artifact.DigestedArtifact) (verify.PolicyBuilder, error) { +func buildSigstoreVerifyPolicy(c verification.EnforcementCriteria, a artifact.DigestedArtifact) (verify.PolicyBuilder, error) { artifactDigestPolicyOption, err := verification.BuildDigestPolicyOption(a) if err != nil { return verify.PolicyBuilder{}, err diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 3771cc315..5f1b1d6f3 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -264,7 +264,7 @@ func runVerify(opts *Options) error { return err } - sp, err := SigstorePolicy(ec, *artifact) + sp, err := buildSigstoreVerifyPolicy(ec, *artifact) if err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build Sigstore verification policy")) return err From e6d0a067e64e397637da8ca62628c5a5a0cb1ea9 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 16:09:45 -0600 Subject: [PATCH 33/76] 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 13c83651e..67cc48f18 100644 --- a/pkg/cmd/attestation/verification/extensions.go +++ b/pkg/cmd/attestation/verification/extensions.go @@ -31,7 +31,7 @@ func VerifyCertExtensions(results []*AttestationProcessingResult, ec Enforcement } } -func verifyCertExtensions(attestation *AttestationProcessingResult, c EnforcementCriteria) error { +func verifyCertExtensions(verifiedCert certificate.Summary, criteria EnforcementCriteria) error { if c.Extensions.SourceRepositoryOwnerURI != "" { sourceRepositoryOwnerURI := attestation.VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI if !strings.EqualFold(c.Extensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) { From a81cb730fcd987051dbdfe9b2665178bc051f05f Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 16:14:28 -0600 Subject: [PATCH 34/76] update VerifyCertExtensions args Signed-off-by: Meredith Lancaster --- .../attestation/verification/extensions.go | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions.go b/pkg/cmd/attestation/verification/extensions.go index 67cc48f18..b41d7f559 100644 --- a/pkg/cmd/attestation/verification/extensions.go +++ b/pkg/cmd/attestation/verification/extensions.go @@ -4,6 +4,8 @@ import ( "errors" "fmt" "strings" + + "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" ) var ( @@ -18,7 +20,7 @@ func VerifyCertExtensions(results []*AttestationProcessingResult, ec Enforcement var atLeastOneVerified bool for _, attestation := range results { - if err := verifyCertExtensions(attestation, ec); err != nil { + if err := verifyCertExtensions(*attestation.VerificationResult.Signature.Certificate, ec); err != nil { return err } atLeastOneVerified = true @@ -32,30 +34,30 @@ func VerifyCertExtensions(results []*AttestationProcessingResult, ec Enforcement } func verifyCertExtensions(verifiedCert certificate.Summary, criteria EnforcementCriteria) error { - if c.Extensions.SourceRepositoryOwnerURI != "" { - sourceRepositoryOwnerURI := attestation.VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI - if !strings.EqualFold(c.Extensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) { - return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", c.Extensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) + if criteria.Extensions.SourceRepositoryOwnerURI != "" { + sourceRepositoryOwnerURI := verifiedCert.Extensions.SourceRepositoryOwnerURI + if !strings.EqualFold(criteria.Extensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) { + return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", criteria.Extensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) } } // if repo is set, check the SourceRepositoryURI field - if c.Extensions.SourceRepositoryURI != "" { - sourceRepositoryURI := attestation.VerificationResult.Signature.Certificate.Extensions.SourceRepositoryURI - if !strings.EqualFold(c.Extensions.SourceRepositoryURI, sourceRepositoryURI) { - return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", c.Extensions.SourceRepositoryURI, sourceRepositoryURI) + if criteria.Extensions.SourceRepositoryURI != "" { + sourceRepositoryURI := verifiedCert.Extensions.SourceRepositoryURI + if !strings.EqualFold(criteria.Extensions.SourceRepositoryURI, sourceRepositoryURI) { + return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", criteria.Extensions.SourceRepositoryURI, sourceRepositoryURI) } } // if issuer is anything other than the default, use the user-provided value; // otherwise, select the appropriate default based on the tenant - if c.OIDCIssuer != "" { - certIssuer := attestation.VerificationResult.Signature.Certificate.Extensions.Issuer - if !strings.EqualFold(c.OIDCIssuer, certIssuer) { - if strings.Index(certIssuer, c.OIDCIssuer+"/") == 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", c.OIDCIssuer, certIssuer) + if criteria.OIDCIssuer != "" { + certIssuer := verifiedCert.Extensions.Issuer + if !strings.EqualFold(criteria.OIDCIssuer, certIssuer) { + if strings.Index(certIssuer, criteria.OIDCIssuer+"/") == 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.OIDCIssuer, certIssuer) } - return fmt.Errorf("expected Issuer to be %s, got %s", c.OIDCIssuer, certIssuer) + return fmt.Errorf("expected Issuer to be %s, got %s", criteria.OIDCIssuer, certIssuer) } } From 9f3d00960c43aaee2b9be3bd106a57853043bac7 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 16:16:09 -0600 Subject: [PATCH 35/76] keep comment Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index 8ee34bc7a..bd614263d 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -45,6 +45,11 @@ func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, er if opts.DenySelfHostedRunner { c.Extensions.RunnerEnvironment = GitHubRunner + } else { + // if Extensions.RunnerEnvironment value is set to the empty string + // through the second function argument, + // no certificate matching will happen on the RunnerEnvironment field + c.Extensions.RunnerEnvironment = "" } if opts.Repo != "" { From 8336f797ad35f40c6b10c886bd3b874d90f7ed8c Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 16:27:21 -0600 Subject: [PATCH 36/76] use sigstore-go certificate.Summary type for criteria Signed-off-by: Meredith Lancaster --- .../attestation/verification/extensions.go | 22 +++++++------- .../verification/extensions_test.go | 17 ++++++----- pkg/cmd/attestation/verification/policy.go | 15 +++------- pkg/cmd/attestation/verify/policy.go | 30 +++++++++---------- pkg/cmd/attestation/verify/policy_test.go | 28 ++++++++--------- 5 files changed, 53 insertions(+), 59 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions.go b/pkg/cmd/attestation/verification/extensions.go index b41d7f559..6f9945661 100644 --- a/pkg/cmd/attestation/verification/extensions.go +++ b/pkg/cmd/attestation/verification/extensions.go @@ -34,30 +34,30 @@ func VerifyCertExtensions(results []*AttestationProcessingResult, ec Enforcement } func verifyCertExtensions(verifiedCert certificate.Summary, criteria EnforcementCriteria) error { - if criteria.Extensions.SourceRepositoryOwnerURI != "" { + if criteria.Certificate.SourceRepositoryOwnerURI != "" { sourceRepositoryOwnerURI := verifiedCert.Extensions.SourceRepositoryOwnerURI - if !strings.EqualFold(criteria.Extensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) { - return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", criteria.Extensions.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) + if !strings.EqualFold(criteria.Certificate.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) { + return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", criteria.Certificate.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) } } // if repo is set, check the SourceRepositoryURI field - if criteria.Extensions.SourceRepositoryURI != "" { + if criteria.Certificate.SourceRepositoryURI != "" { sourceRepositoryURI := verifiedCert.Extensions.SourceRepositoryURI - if !strings.EqualFold(criteria.Extensions.SourceRepositoryURI, sourceRepositoryURI) { - return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", criteria.Extensions.SourceRepositoryURI, sourceRepositoryURI) + if !strings.EqualFold(criteria.Certificate.SourceRepositoryURI, sourceRepositoryURI) { + return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", criteria.Certificate.SourceRepositoryURI, sourceRepositoryURI) } } // if issuer is anything other than the default, use the user-provided value; // otherwise, select the appropriate default based on the tenant - if criteria.OIDCIssuer != "" { + if criteria.Certificate.Issuer != "" { certIssuer := verifiedCert.Extensions.Issuer - if !strings.EqualFold(criteria.OIDCIssuer, certIssuer) { - if strings.Index(certIssuer, criteria.OIDCIssuer+"/") == 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.OIDCIssuer, certIssuer) + 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) } - return fmt.Errorf("expected Issuer to be %s, got %s", criteria.OIDCIssuer, certIssuer) + return fmt.Errorf("expected Issuer to be %s, got %s", criteria.Certificate.Issuer, certIssuer) } } diff --git a/pkg/cmd/attestation/verification/extensions_test.go b/pkg/cmd/attestation/verification/extensions_test.go index f34cc8304..9548502be 100644 --- a/pkg/cmd/attestation/verification/extensions_test.go +++ b/pkg/cmd/attestation/verification/extensions_test.go @@ -25,12 +25,13 @@ func TestVerifyCertExtensions(t *testing.T) { }, } + certSummary := certificate.Summary{} + certSummary.SourceRepositoryOwnerURI = "https://github.com/owner" + certSummary.SourceRepositoryURI = "https://github.com/owner/repo" + certSummary.Issuer = GitHubOIDCIssuer + c := EnforcementCriteria{ - Extensions: Extensions{ - SourceRepositoryOwnerURI: "https://github.com/owner", - SourceRepositoryURI: "https://github.com/owner/repo", - }, - OIDCIssuer: GitHubOIDCIssuer, + Certificate: certSummary, } t.Run("success", func(t *testing.T) { @@ -40,21 +41,21 @@ func TestVerifyCertExtensions(t *testing.T) { t.Run("with wrong SourceRepositoryOwnerURI", func(t *testing.T) { expectedCriteria := c - expectedCriteria.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong" + expectedCriteria.Certificate.SourceRepositoryOwnerURI = "https://github.com/wrong" err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://github.com/wrong, got https://github.com/owner") }) t.Run("with wrong SourceRepositoryURI", func(t *testing.T) { expectedCriteria := c - expectedCriteria.Extensions.SourceRepositoryURI = "https://github.com/foo/wrong" + expectedCriteria.Certificate.SourceRepositoryURI = "https://github.com/foo/wrong" err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/foo/wrong, got https://github.com/owner/repo") }) t.Run("with wrong OIDCIssuer", func(t *testing.T) { expectedCriteria := c - expectedCriteria.OIDCIssuer = "wrong" + expectedCriteria.Certificate.Issuer = "wrong" err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected Issuer to be wrong, got https://token.actions.githubusercontent.com") }) diff --git a/pkg/cmd/attestation/verification/policy.go b/pkg/cmd/attestation/verification/policy.go index fabdbc0aa..9f8672628 100644 --- a/pkg/cmd/attestation/verification/policy.go +++ b/pkg/cmd/attestation/verification/policy.go @@ -5,6 +5,7 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" + "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" "github.com/sigstore/sigstore-go/pkg/verify" ) @@ -19,17 +20,9 @@ func BuildDigestPolicyOption(a artifact.DigestedArtifact) (verify.ArtifactPolicy return verify.WithArtifactDigest(a.Algorithm(), decoded), nil } -type Extensions struct { - RunnerEnvironment string - SANRegex string - SAN string - SignerWorkflow string - SourceRepositoryOwnerURI string - SourceRepositoryURI string -} - type EnforcementCriteria struct { - Extensions Extensions + Certificate certificate.Summary PredicateType string - OIDCIssuer string + SANRegex string + SAN string } diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index bd614263d..f69c75c52 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -30,55 +30,55 @@ func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, er if opts.SignerRepo != "" { signedRepoRegex := expandToGitHubURL(opts.Tenant, opts.SignerRepo) - c.Extensions.SANRegex = signedRepoRegex + c.SANRegex = signedRepoRegex } else if opts.SignerWorkflow != "" { validatedWorkflowRegex, err := validateSignerWorkflow(opts) if err != nil { return verification.EnforcementCriteria{}, err } - c.Extensions.SANRegex = validatedWorkflowRegex + c.SANRegex = validatedWorkflowRegex } else { - c.Extensions.SANRegex = opts.SANRegex - c.Extensions.SAN = opts.SAN + c.SANRegex = opts.SANRegex + c.SAN = opts.SAN } if opts.DenySelfHostedRunner { - c.Extensions.RunnerEnvironment = GitHubRunner + c.Certificate.RunnerEnvironment = GitHubRunner } else { - // if Extensions.RunnerEnvironment value is set to the empty string + // if Certificate.RunnerEnvironment value is set to the empty string // through the second function argument, // no certificate matching will happen on the RunnerEnvironment field - c.Extensions.RunnerEnvironment = "" + c.Certificate.RunnerEnvironment = "" } if opts.Repo != "" { if opts.Tenant != "" { - c.Extensions.SourceRepositoryURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Repo) + c.Certificate.SourceRepositoryURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Repo) } else { - c.Extensions.SourceRepositoryURI = fmt.Sprintf("https://github.com/%s", opts.Repo) + c.Certificate.SourceRepositoryURI = fmt.Sprintf("https://github.com/%s", opts.Repo) } } if opts.Tenant != "" { - c.Extensions.SourceRepositoryOwnerURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Owner) + c.Certificate.SourceRepositoryOwnerURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Owner) } else { - c.Extensions.SourceRepositoryOwnerURI = fmt.Sprintf("https://github.com/%s", opts.Owner) + c.Certificate.SourceRepositoryOwnerURI = fmt.Sprintf("https://github.com/%s", opts.Owner) } // if tenant is provided, select the appropriate default based on the tenant // otherwise, use the provided OIDCIssuer if opts.Tenant != "" { - c.OIDCIssuer = fmt.Sprintf(verification.GitHubTenantOIDCIssuer, opts.Tenant) + c.Certificate.Issuer = fmt.Sprintf(verification.GitHubTenantOIDCIssuer, opts.Tenant) } else { - c.OIDCIssuer = opts.OIDCIssuer + c.Certificate.Issuer = opts.OIDCIssuer } return c, nil } func buildCertificateIdentityOption(c verification.EnforcementCriteria) (verify.PolicyOption, error) { - sanMatcher, err := verify.NewSANMatcher(c.Extensions.SAN, c.Extensions.SANRegex) + sanMatcher, err := verify.NewSANMatcher(c.SAN, c.SANRegex) if err != nil { return nil, err } @@ -90,7 +90,7 @@ func buildCertificateIdentityOption(c verification.EnforcementCriteria) (verify. } extensions := certificate.Extensions{ - RunnerEnvironment: c.Extensions.RunnerEnvironment, + RunnerEnvironment: c.Certificate.RunnerEnvironment, } certId, err := verify.NewCertificateIdentity(sanMatcher, issuerMatcher, extensions) diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index 13a7d2822..d4516e844 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -21,8 +21,8 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Equal(t, "(?i)^https://github.com/foo/bar/", c.Extensions.SANRegex) - require.Zero(t, c.Extensions.SAN) + require.Equal(t, "(?i)^https://github.com/foo/bar/", c.SANRegex) + require.Zero(t, c.SAN) }) t.Run("sets SANRegex using SignerWorkflow matching host regex", func(t *testing.T) { @@ -36,8 +36,8 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Equal(t, "^https://github.com/foo/bar/.github/workflows/attest.yml", c.Extensions.SANRegex) - require.Zero(t, c.Extensions.SAN) + require.Equal(t, "^https://github.com/foo/bar/.github/workflows/attest.yml", c.SANRegex) + require.Zero(t, c.SAN) }) t.Run("sets SANRegex and SAN using SANRegex and SAN", func(t *testing.T) { @@ -51,8 +51,8 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Equal(t, "https://github/foo/bar/.github/workflows/attest.yml", c.Extensions.SAN) - require.Equal(t, "(?i)^https://github/foo", c.Extensions.SANRegex) + require.Equal(t, "https://github/foo/bar/.github/workflows/attest.yml", c.SAN) + require.Equal(t, "(?i)^https://github/foo", c.SANRegex) }) t.Run("sets Extensions.RunnerEnvironment to GitHubRunner value if opts.DenySelfHostedRunner is true", func(t *testing.T) { @@ -65,7 +65,7 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Equal(t, GitHubRunner, c.Extensions.RunnerEnvironment) + require.Equal(t, GitHubRunner, c.Certificate.RunnerEnvironment) }) t.Run("sets Extensions.RunnerEnvironment to * value if opts.DenySelfHostedRunner is false", func(t *testing.T) { @@ -78,7 +78,7 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Zero(t, c.Extensions.RunnerEnvironment) + require.Zero(t, c.Certificate.RunnerEnvironment) }) t.Run("sets Extensions.SourceRepositoryURI using opts.Repo and opts.Tenant", func(t *testing.T) { @@ -91,7 +91,7 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Equal(t, "https://baz.ghe.com/foo/bar", c.Extensions.SourceRepositoryURI) + require.Equal(t, "https://baz.ghe.com/foo/bar", c.Certificate.SourceRepositoryURI) }) t.Run("sets Extensions.SourceRepositoryURI using opts.Repo", func(t *testing.T) { @@ -103,7 +103,7 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Equal(t, "https://github.com/foo/bar", c.Extensions.SourceRepositoryURI) + require.Equal(t, "https://github.com/foo/bar", c.Certificate.SourceRepositoryURI) }) t.Run("sets Extensions.SourceRepositoryOwnerURI using opts.Owner and opts.Tenant", func(t *testing.T) { @@ -116,7 +116,7 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Equal(t, "https://baz.ghe.com/foo", c.Extensions.SourceRepositoryOwnerURI) + require.Equal(t, "https://baz.ghe.com/foo", c.Certificate.SourceRepositoryOwnerURI) }) t.Run("sets Extensions.SourceRepositoryOwnerURI using opts.Owner", func(t *testing.T) { @@ -128,7 +128,7 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Equal(t, "https://github.com/foo", c.Extensions.SourceRepositoryOwnerURI) + require.Equal(t, "https://github.com/foo", c.Certificate.SourceRepositoryOwnerURI) }) t.Run("sets OIDCIssuer using opts.OIDCIssuer and opts.Tenant", func(t *testing.T) { @@ -142,7 +142,7 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Equal(t, "https://token.actions.baz.ghe.com", c.OIDCIssuer) + require.Equal(t, "https://token.actions.baz.ghe.com", c.Certificate.Issuer) }) t.Run("sets OIDCIssuer using opts.OIDCIssuer", func(t *testing.T) { @@ -155,7 +155,7 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Equal(t, "https://foo.com", c.OIDCIssuer) + require.Equal(t, "https://foo.com", c.Certificate.Issuer) }) } From 50cda0df44f987c7c8803a4984eddcae7cdd4574 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 16:56:49 -0600 Subject: [PATCH 37/76] add Valid method for EnforcementCriteria Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verification/policy.go | 19 +++++++++++++++++++ pkg/cmd/attestation/verify/policy.go | 7 ++++--- pkg/cmd/attestation/verify/policy_test.go | 3 ++- pkg/cmd/attestation/verify/verify.go | 17 +++++++++++------ 4 files changed, 36 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/attestation/verification/policy.go b/pkg/cmd/attestation/verification/policy.go index 9f8672628..ae21dae48 100644 --- a/pkg/cmd/attestation/verification/policy.go +++ b/pkg/cmd/attestation/verification/policy.go @@ -2,6 +2,7 @@ package verification import ( "encoding/hex" + "fmt" "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" @@ -9,6 +10,8 @@ import ( "github.com/sigstore/sigstore-go/pkg/verify" ) +const GitHubRunner = "github-hosted" + // BuildDigestPolicyOption builds a verify.ArtifactPolicyOption // from the given artifact digest and digest algorithm func BuildDigestPolicyOption(a artifact.DigestedArtifact) (verify.ArtifactPolicyOption, error) { @@ -26,3 +29,19 @@ type EnforcementCriteria struct { SANRegex string SAN string } + +func (c EnforcementCriteria) Valid() error { + if c.Certificate.Issuer == "" { + return fmt.Errorf("Issuer must be set") + } + if c.Certificate.RunnerEnvironment != "" && c.Certificate.RunnerEnvironment != GitHubRunner { + return fmt.Errorf("RunnerEnvironment must be set to either \"\" or %s", GitHubRunner) + } + if c.Certificate.SourceRepositoryOwnerURI == "" { + return fmt.Errorf("SourceRepositoryOwnerURI must be set") + } + if c.PredicateType == "" { + return fmt.Errorf("PredicateType must be set") + } + return nil +} diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index f69c75c52..c68cdb452 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -14,8 +14,7 @@ import ( const ( // represents the GitHub hosted runner in the certificate RunnerEnvironment extension - GitHubRunner = "github-hosted" - hostRegex = `^[a-zA-Z0-9-]+\.[a-zA-Z0-9-]+.*$` + hostRegex = `^[a-zA-Z0-9-]+\.[a-zA-Z0-9-]+.*$` ) func expandToGitHubURL(tenant, ownerOrRepo string) string { @@ -44,7 +43,7 @@ func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, er } if opts.DenySelfHostedRunner { - c.Certificate.RunnerEnvironment = GitHubRunner + c.Certificate.RunnerEnvironment = verification.GitHubRunner } else { // if Certificate.RunnerEnvironment value is set to the empty string // through the second function argument, @@ -74,6 +73,8 @@ func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, er c.Certificate.Issuer = opts.OIDCIssuer } + c.PredicateType = opts.PredicateType + return c, nil } diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index d4516e844..f5755e9d5 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -3,6 +3,7 @@ package verify import ( "testing" + "github.com/cli/cli/v2/pkg/cmd/attestation/verification" "github.com/cli/cli/v2/pkg/cmd/factory" "github.com/stretchr/testify/require" @@ -65,7 +66,7 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Equal(t, GitHubRunner, c.Certificate.RunnerEnvironment) + require.Equal(t, verification.GitHubRunner, c.Certificate.RunnerEnvironment) }) t.Run("sets Extensions.RunnerEnvironment to * value if opts.DenySelfHostedRunner is false", func(t *testing.T) { diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 5f1b1d6f3..188bde7b1 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -203,6 +203,17 @@ func NewVerifyCmd(f *cmdutil.Factory, runF func(*Options) error) *cobra.Command } func runVerify(opts *Options) error { + ec, err := newEnforcementCriteria(opts) + if err != nil { + opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build verification policy")) + return err + } + + if err := ec.Valid(); err != nil { + opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Invalid verification policy")) + return err + } + artifact, err := artifact.NewDigestedArtifact(opts.OCIClient, opts.ArtifactPath, opts.DigestAlgorithm) if err != nil { opts.Logger.Printf(opts.Logger.ColorScheme.Red("✗ Loading digest for %s failed\n"), opts.ArtifactPath) @@ -258,12 +269,6 @@ func runVerify(opts *Options) error { opts.Logger.VerbosePrintf("Verifying attestations with predicate type: %s\n", opts.PredicateType) - ec, err := newEnforcementCriteria(opts) - if err != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build verification policy")) - return err - } - sp, err := buildSigstoreVerifyPolicy(ec, *artifact) if err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build Sigstore verification policy")) From a7a70fc91c70ab9ff6f95bb4fabaabb45e56e0c0 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 16:59:25 -0600 Subject: [PATCH 38/76] check for SAN and SANRegex Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verification/policy.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/cmd/attestation/verification/policy.go b/pkg/cmd/attestation/verification/policy.go index ae21dae48..6e8ce7ae4 100644 --- a/pkg/cmd/attestation/verification/policy.go +++ b/pkg/cmd/attestation/verification/policy.go @@ -43,5 +43,8 @@ func (c EnforcementCriteria) Valid() error { if c.PredicateType == "" { return fmt.Errorf("PredicateType must be set") } + if c.SANRegex == "" && c.SAN == "" { + return fmt.Errorf("SANRegex or SAN must be set") + } return nil } From 0fb82a6e7c94f66165876575db44152a9470f2b2 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 17:11:02 -0600 Subject: [PATCH 39/76] comments Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index c68cdb452..c6be71ae3 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -27,6 +27,7 @@ func expandToGitHubURL(tenant, ownerOrRepo string) string { func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, error) { c := verification.EnforcementCriteria{} + // Set SANRegex using either the opts.SignerRepo or opts.SignerWorkflow values if opts.SignerRepo != "" { signedRepoRegex := expandToGitHubURL(opts.Tenant, opts.SignerRepo) c.SANRegex = signedRepoRegex @@ -38,10 +39,13 @@ func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, er c.SANRegex = validatedWorkflowRegex } else { + // If neither of those values were set, default to the provided SANRegex and SAN values c.SANRegex = opts.SANRegex c.SAN = opts.SAN } + // if the DenySelfHostedRunner option is set to true, set the + // RunnerEnvironment extension to the GitHub hosted runner value if opts.DenySelfHostedRunner { c.Certificate.RunnerEnvironment = verification.GitHubRunner } else { @@ -51,7 +55,10 @@ func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, er c.Certificate.RunnerEnvironment = "" } + // If the Repo option is provided, set the SourceRepositoryURI extension if opts.Repo != "" { + // If the Tenant options is also provided, set the SourceRepositoryURI extension + // using the specific URI format if opts.Tenant != "" { c.Certificate.SourceRepositoryURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Repo) } else { @@ -59,6 +66,8 @@ func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, er } } + // If the Tenant option is provided, set the SourceRepositoryOwnerURI extension + // using the specific URI format if opts.Tenant != "" { c.Certificate.SourceRepositoryOwnerURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Owner) } else { @@ -66,10 +75,10 @@ func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, er } // if tenant is provided, select the appropriate default based on the tenant - // otherwise, use the provided OIDCIssuer if opts.Tenant != "" { c.Certificate.Issuer = fmt.Sprintf(verification.GitHubTenantOIDCIssuer, opts.Tenant) } else { + // otherwise, use the provided OIDCIssuer c.Certificate.Issuer = opts.OIDCIssuer } From 815fcb72b57f3d5386b58f0b124c41942bc1198f Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 1 Nov 2024 14:10:24 +0000 Subject: [PATCH 40/76] Bump github.com/creack/pty from 1.1.23 to 1.1.24 Bumps [github.com/creack/pty](https://github.com/creack/pty) from 1.1.23 to 1.1.24. - [Release notes](https://github.com/creack/pty/releases) - [Commits](https://github.com/creack/pty/compare/v1.1.23...v1.1.24) --- updated-dependencies: - dependency-name: github.com/creack/pty dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 77be0c19d..40193c4f8 100644 --- a/go.mod +++ b/go.mod @@ -16,7 +16,7 @@ require ( github.com/cli/oauth v1.1.1 github.com/cli/safeexec v1.0.1 github.com/cpuguy83/go-md2man/v2 v2.0.5 - github.com/creack/pty v1.1.23 + github.com/creack/pty v1.1.24 github.com/distribution/reference v0.5.0 github.com/gabriel-vasile/mimetype v1.4.6 github.com/gdamore/tcell/v2 v2.5.4 diff --git a/go.sum b/go.sum index fe7adc3e7..d068d9d11 100644 --- a/go.sum +++ b/go.sum @@ -115,8 +115,8 @@ github.com/cpuguy83/go-md2man/v2 v2.0.4/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46t github.com/cpuguy83/go-md2man/v2 v2.0.5 h1:ZtcqGrnekaHpVLArFSe4HK5DoKx1T0rq2DwVB0alcyc= github.com/cpuguy83/go-md2man/v2 v2.0.5/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= github.com/creack/pty v1.1.17/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4= -github.com/creack/pty v1.1.23 h1:4M6+isWdcStXEf15G/RbrMPOQj1dZ7HPZCGwE4kOeP0= -github.com/creack/pty v1.1.23/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= +github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= +github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/cyberphone/json-canonicalization v0.0.0-20220623050100-57a0ce2678a7 h1:vU+EP9ZuFUCYE0NYLwTSob+3LNEJATzNfP/DC7SWGWI= github.com/cyberphone/json-canonicalization v0.0.0-20220623050100-57a0ce2678a7/go.mod h1:uzvlm1mxhHkdfqitSA92i7Se+S9ksOn3a3qmv/kyOCw= github.com/danieljoos/wincred v1.2.1 h1:dl9cBrupW8+r5250DYkYxocLeZ1Y4vB1kxgtjxw8GQs= From a5eca00d0d2e175959f55033c92623a57817a334 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Fri, 1 Nov 2024 08:20:32 -0600 Subject: [PATCH 41/76] remove emtpy string checks Signed-off-by: Meredith Lancaster --- .../attestation/verification/extensions.go | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/attestation/verification/extensions.go b/pkg/cmd/attestation/verification/extensions.go index 6f9945661..5434974a2 100644 --- a/pkg/cmd/attestation/verification/extensions.go +++ b/pkg/cmd/attestation/verification/extensions.go @@ -34,11 +34,9 @@ func VerifyCertExtensions(results []*AttestationProcessingResult, ec Enforcement } func verifyCertExtensions(verifiedCert certificate.Summary, criteria EnforcementCriteria) error { - if criteria.Certificate.SourceRepositoryOwnerURI != "" { - 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) - } + 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) } // if repo is set, check the SourceRepositoryURI field @@ -51,14 +49,12 @@ func verifyCertExtensions(verifiedCert certificate.Summary, criteria Enforcement // if issuer is anything other than the default, use the user-provided value; // otherwise, select the appropriate default based on the tenant - if criteria.Certificate.Issuer != "" { - 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) - } - return fmt.Errorf("expected Issuer to be %s, got %s", criteria.Certificate.Issuer, certIssuer) + 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) } + return fmt.Errorf("expected Issuer to be %s, got %s", criteria.Certificate.Issuer, certIssuer) } return nil From a6d15b4f60b4db8bd26689c3c74cb0f912a2c858 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Fri, 1 Nov 2024 09:02:23 -0600 Subject: [PATCH 42/76] update OIDC issuer logic Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 14 +++++++++----- pkg/cmd/attestation/verify/policy_test.go | 5 +++-- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index c6be71ae3..d6b55abc0 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -74,12 +74,16 @@ func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, er c.Certificate.SourceRepositoryOwnerURI = fmt.Sprintf("https://github.com/%s", opts.Owner) } - // if tenant is provided, select the appropriate default based on the tenant - if opts.Tenant != "" { - c.Certificate.Issuer = fmt.Sprintf(verification.GitHubTenantOIDCIssuer, opts.Tenant) - } else { - // otherwise, use the provided OIDCIssuer + // If the OIDCIssuer option has been set, use that custom value + // Otherwise check if tenant is provided, select the appropriate default based on that + if opts.OIDCIssuer != verification.GitHubOIDCIssuer { c.Certificate.Issuer = opts.OIDCIssuer + } else { + if opts.Tenant != "" { + c.Certificate.Issuer = fmt.Sprintf(verification.GitHubTenantOIDCIssuer, opts.Tenant) + } else { + c.Certificate.Issuer = verification.GitHubOIDCIssuer + } } c.PredicateType = opts.PredicateType diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index f5755e9d5..420c57f3a 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -132,13 +132,13 @@ func TestNewEnforcementCriteria(t *testing.T) { require.Equal(t, "https://github.com/foo", c.Certificate.SourceRepositoryOwnerURI) }) - t.Run("sets OIDCIssuer using opts.OIDCIssuer and opts.Tenant", func(t *testing.T) { + t.Run("sets OIDCIssuer using opts.Tenant", func(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, Owner: "foo", Repo: "foo/bar", Tenant: "baz", - OIDCIssuer: "https://foo.com", + OIDCIssuer: verification.GitHubOIDCIssuer, } c, err := newEnforcementCriteria(opts) @@ -152,6 +152,7 @@ func TestNewEnforcementCriteria(t *testing.T) { Owner: "foo", Repo: "foo/bar", OIDCIssuer: "https://foo.com", + Tenant: "baz", } c, err := newEnforcementCriteria(opts) From bb1584b52afc45d3952c942046051c28d0f22e37 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Fri, 1 Nov 2024 09:02:56 -0600 Subject: [PATCH 43/76] comment Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index d6b55abc0..99e3ea94e 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -74,8 +74,8 @@ func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, er c.Certificate.SourceRepositoryOwnerURI = fmt.Sprintf("https://github.com/%s", opts.Owner) } - // If the OIDCIssuer option has been set, use that custom value - // Otherwise check if tenant is provided, select the appropriate default based on that + // if issuer is anything other than the default, use the user-provided value; + // otherwise, select the appropriate default based on the tenant if opts.OIDCIssuer != verification.GitHubOIDCIssuer { c.Certificate.Issuer = opts.OIDCIssuer } else { From 43810a5fc3f03c460707340e4a05d0715137f7a3 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Fri, 1 Nov 2024 09:17:47 -0600 Subject: [PATCH 44/76] use predicate type stored in enforcementCriteria Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/verify.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 188bde7b1..0d129a03e 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -260,7 +260,7 @@ func runVerify(opts *Options) error { } // Apply predicate type filter to returned attestations - filteredAttestations := verification.FilterAttestations(opts.PredicateType, 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 err From 91967cced8833e2b1b3ec9eeb0ebbd44d95d76ce Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Fri, 1 Nov 2024 09:51:05 -0600 Subject: [PATCH 45/76] Update pkg/cmd/attestation/verify/verify.go Co-authored-by: Phill MV --- pkg/cmd/attestation/verify/verify.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 0d129a03e..cbfc91f1c 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -267,7 +267,7 @@ func runVerify(opts *Options) error { } attestations = filteredAttestations - opts.Logger.VerbosePrintf("Verifying attestations with predicate type: %s\n", opts.PredicateType) + opts.Logger.VerbosePrintf("Verifying attestations with predicate type: %s\n", ec.PredicateType) sp, err := buildSigstoreVerifyPolicy(ec, *artifact) if err != nil { From 47d77bd51b589ccc1a6d588ea691401db92899a4 Mon Sep 17 00:00:00 2001 From: Andrew Feller Date: Sat, 2 Nov 2024 13:14:05 -0400 Subject: [PATCH 46/76] Add version checking when executing extensions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Building on logic from the `gh ext list` for retrieving and assessing extension release information, this commit enhances the logic around invoking extensions to check for new releases. Using the same user experience from checking `gh` version, this should only output information when the extension is used and gives the user information on how to upgrade depending on the type of extension and whether it is pinned or not. ```shell andrewfeller@Andrews-MacBook-Pro cli % gh ext install dlvhdr/gh-dash --pin v4.6.0 ✓ Installed extension dlvhdr/gh-dash ✓ Pinned extension at v4.6.0 andrewfeller@Andrews-MacBook-Pro cli % ./bin/gh dash A new release of dash is available: 4.6.0 → 4.7.0 To upgrade, run: gh extension upgrade dash --force https://github.com/dlvhdr/gh-dash ``` --- pkg/cmd/root/extension.go | 43 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/pkg/cmd/root/extension.go b/pkg/cmd/root/extension.go index d6d495103..89f0085f3 100644 --- a/pkg/cmd/root/extension.go +++ b/pkg/cmd/root/extension.go @@ -4,9 +4,11 @@ import ( "errors" "fmt" "os/exec" + "strings" "github.com/cli/cli/v2/pkg/extensions" "github.com/cli/cli/v2/pkg/iostreams" + "github.com/mgutz/ansi" "github.com/spf13/cobra" ) @@ -14,10 +16,33 @@ type ExternalCommandExitError struct { *exec.ExitError } +type extensionReleaseInfo struct { + CurrentVersion string + LatestVersion string + Pinned bool + URL string +} + func NewCmdExtension(io *iostreams.IOStreams, em extensions.ExtensionManager, ext extensions.Extension) *cobra.Command { + // Setup channel containing information about potential latest release info + updateMessageChan := make(chan *extensionReleaseInfo) + return &cobra.Command{ Use: ext.Name(), Short: fmt.Sprintf("Extension %s", ext.Name()), + // PreRun handles looking up whether extension has a latest version only when the command is ran. + PreRun: func(c *cobra.Command, args []string) { + go func() { + if ext.UpdateAvailable() { + updateMessageChan <- &extensionReleaseInfo{ + CurrentVersion: ext.CurrentVersion(), + LatestVersion: ext.LatestVersion(), + Pinned: ext.IsPinned(), + URL: ext.URL(), + } + } + }() + }, RunE: func(c *cobra.Command, args []string) error { args = append([]string{ext.Name()}, args...) if _, err := em.Dispatch(args, io.In, io.Out, io.ErrOut); err != nil { @@ -29,6 +54,24 @@ func NewCmdExtension(io *iostreams.IOStreams, em extensions.ExtensionManager, ex } return nil }, + // PostRun handles communicating extension release information if found + PostRun: func(c *cobra.Command, args []string) { + releaseInfo := <-updateMessageChan + if releaseInfo != nil { + stderr := io.ErrOut + fmt.Fprintf(stderr, "\n\n%s %s → %s\n", + ansi.Color(fmt.Sprintf("A new release of %s is available:", ext.Name()), "yellow"), + ansi.Color(strings.TrimPrefix(releaseInfo.CurrentVersion, "v"), "cyan"), + ansi.Color(strings.TrimPrefix(releaseInfo.LatestVersion, "v"), "cyan")) + if releaseInfo.Pinned { + fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s --force\n", ext.Name()) + } else { + fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s\n", ext.Name()) + } + fmt.Fprintf(stderr, "%s\n\n", + ansi.Color(releaseInfo.URL, "yellow")) + } + }, GroupID: "extension", Annotations: map[string]string{ "skipAuthCheck": "true", From 3281bd457cc5c38c4ca62ce9481f8691ea567bc0 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 4 Nov 2024 07:32:10 -0700 Subject: [PATCH 47/76] simplify logic, add comments Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index 99e3ea94e..d158cf375 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -25,7 +25,7 @@ func expandToGitHubURL(tenant, ownerOrRepo string) string { } func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, error) { - c := verification.EnforcementCriteria{} + var c verification.EnforcementCriteria // Set SANRegex using either the opts.SignerRepo or opts.SignerWorkflow values if opts.SignerRepo != "" { @@ -66,7 +66,7 @@ func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, er } } - // If the Tenant option is provided, set the SourceRepositoryOwnerURI extension + // If the tenant option is provided, set the SourceRepositoryOwnerURI extension // using the specific URI format if opts.Tenant != "" { c.Certificate.SourceRepositoryOwnerURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Owner) @@ -74,16 +74,13 @@ func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, er c.Certificate.SourceRepositoryOwnerURI = fmt.Sprintf("https://github.com/%s", opts.Owner) } - // if issuer is anything other than the default, use the user-provided value; - // otherwise, select the appropriate default based on the tenant - if opts.OIDCIssuer != verification.GitHubOIDCIssuer { - c.Certificate.Issuer = opts.OIDCIssuer + // if the tenant is provided and OIDC issuer provided matches the default + // use the tenant-specific issuer + if opts.Tenant != "" && opts.OIDCIssuer == verification.GitHubOIDCIssuer { + c.Certificate.Issuer = fmt.Sprintf(verification.GitHubTenantOIDCIssuer, opts.Tenant) } else { - if opts.Tenant != "" { - c.Certificate.Issuer = fmt.Sprintf(verification.GitHubTenantOIDCIssuer, opts.Tenant) - } else { - c.Certificate.Issuer = verification.GitHubOIDCIssuer - } + // otherwise use the custom OIDC issuer provided as an option + c.Certificate.Issuer = opts.OIDCIssuer } c.PredicateType = opts.PredicateType @@ -142,6 +139,8 @@ func validateSignerWorkflow(opts *Options) (string, error) { return fmt.Sprintf("^https://%s", opts.SignerWorkflow), nil } + // if the provided workflow did not match the expect format + // we move onto creating a signer workflow using the provided host name if opts.Hostname == "" { return "", errors.New("unknown host") } From b9c9f0acc27b79008649fefc4cabc9c0b5623aa1 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 4 Nov 2024 07:35:42 -0700 Subject: [PATCH 48/76] move comment Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verification/policy.go | 1 + pkg/cmd/attestation/verify/policy.go | 5 +---- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/attestation/verification/policy.go b/pkg/cmd/attestation/verification/policy.go index 6e8ce7ae4..f5f4010aa 100644 --- a/pkg/cmd/attestation/verification/policy.go +++ b/pkg/cmd/attestation/verification/policy.go @@ -10,6 +10,7 @@ import ( "github.com/sigstore/sigstore-go/pkg/verify" ) +// represents the GitHub hosted runner in the certificate RunnerEnvironment extension const GitHubRunner = "github-hosted" // BuildDigestPolicyOption builds a verify.ArtifactPolicyOption diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index d158cf375..c2e154fe2 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -12,10 +12,7 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/verification" ) -const ( - // represents the GitHub hosted runner in the certificate RunnerEnvironment extension - hostRegex = `^[a-zA-Z0-9-]+\.[a-zA-Z0-9-]+.*$` -) +const hostRegex = `^[a-zA-Z0-9-]+\.[a-zA-Z0-9-]+.*$` func expandToGitHubURL(tenant, ownerOrRepo string) string { if tenant == "" { From 1c4c8e51453088949fd445c3b45d07fa6962edf3 Mon Sep 17 00:00:00 2001 From: Aarni Koskela Date: Mon, 4 Nov 2024 17:55:35 +0200 Subject: [PATCH 49/76] Fix verbiage for deleting workflow runs It's not deleting _workflows_ (which are specified in YAML)... --- acceptance/testdata/workflow/run-delete.txtar | 2 +- pkg/cmd/run/delete/delete.go | 2 +- pkg/cmd/run/delete/delete_test.go | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/acceptance/testdata/workflow/run-delete.txtar b/acceptance/testdata/workflow/run-delete.txtar index 72d098740..b78135330 100644 --- a/acceptance/testdata/workflow/run-delete.txtar +++ b/acceptance/testdata/workflow/run-delete.txtar @@ -40,7 +40,7 @@ exec gh run watch $RUN_ID --exit-status # Delete the workflow run exec gh run delete $RUN_ID -stdout '✓ Request to delete workflow submitted.' +stdout '✓ Request to delete workflow run submitted.' # It takes some time for a workflow run to be deleted sleep 5 diff --git a/pkg/cmd/run/delete/delete.go b/pkg/cmd/run/delete/delete.go index b785b5f0f..711e98c02 100644 --- a/pkg/cmd/run/delete/delete.go +++ b/pkg/cmd/run/delete/delete.go @@ -133,7 +133,7 @@ func runDelete(opts *DeleteOptions) error { return err } - fmt.Fprintf(opts.IO.Out, "%s Request to delete workflow submitted.\n", cs.SuccessIcon()) + fmt.Fprintf(opts.IO.Out, "%s Request to delete workflow run submitted.\n", cs.SuccessIcon()) return nil } diff --git a/pkg/cmd/run/delete/delete_test.go b/pkg/cmd/run/delete/delete_test.go index 3ded01182..5df69aa60 100644 --- a/pkg/cmd/run/delete/delete_test.go +++ b/pkg/cmd/run/delete/delete_test.go @@ -110,7 +110,7 @@ func TestRunDelete(t *testing.T) { httpmock.REST("DELETE", fmt.Sprintf("repos/OWNER/REPO/actions/runs/%d", shared.SuccessfulRun.ID)), httpmock.StatusStringResponse(204, "")) }, - wantOut: "✓ Request to delete workflow submitted.\n", + wantOut: "✓ Request to delete workflow run submitted.\n", }, { name: "not found", @@ -153,7 +153,7 @@ func TestRunDelete(t *testing.T) { httpmock.REST("DELETE", fmt.Sprintf("repos/OWNER/REPO/actions/runs/%d", shared.SuccessfulRun.ID)), httpmock.StatusStringResponse(204, "")) }, - wantOut: "✓ Request to delete workflow submitted.\n", + wantOut: "✓ Request to delete workflow run submitted.\n", }, } From b4c221dfb7dd12b369b7524a829f6cc52956080a Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Tue, 5 Nov 2024 22:30:15 +0000 Subject: [PATCH 50/76] Create the automatic key when specified with -i --- pkg/cmd/codespace/ssh.go | 27 ++++++++++++++++----------- pkg/cmd/codespace/ssh_test.go | 35 +++++++++++++++++++++++++++++------ pkg/ssh/ssh_keys.go | 6 +++--- 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index c6c3943e2..44ed30eb0 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -20,7 +20,6 @@ import ( "github.com/cli/cli/v2/internal/codespaces/api" "github.com/cli/cli/v2/internal/codespaces/portforwarder" "github.com/cli/cli/v2/internal/codespaces/rpc" - "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/ssh" "github.com/cli/safeexec" @@ -336,10 +335,20 @@ func selectSSHKeys( return nil, false, errors.New("missing value to -i argument") } + privateKeyPath := args[i+1] + + // The --config setup will set the automatic key with -i, but it might not actually be created, so we need to ensure that here + if automaticPrivateKeyPath, _ := automaticPrivateKeyPath(sshContext); automaticPrivateKeyPath == privateKeyPath { + _, err := generateAutomaticSSHKeys(sshContext) + if err != nil { + return nil, false, fmt.Errorf("generating automatic keypair: %w", err) + } + } + // User manually specified an identity file so just trust it is correct return &ssh.KeyPair{ - PrivateKeyPath: args[i+1], - PublicKeyPath: args[i+1] + ".pub", + PrivateKeyPath: privateKeyPath, + PublicKeyPath: privateKeyPath + ".pub", }, false, nil } @@ -636,7 +645,8 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts sshOptions) (err erro return fmt.Errorf("error formatting template: %w", err) } - automaticIdentityFilePath, err := automaticPrivateKeyPath() + sshContext := ssh.Context{} + automaticIdentityFilePath, err := automaticPrivateKeyPath(sshContext) if err != nil { return fmt.Errorf("error finding .ssh directory: %w", err) } @@ -683,13 +693,8 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts sshOptions) (err erro return status } -func automaticPrivateKeyPath() (string, error) { - sshDir, err := config.HomeDirPath(".ssh") - if err != nil { - return "", err - } - - return path.Join(sshDir, automaticPrivateKeyName), nil +func automaticPrivateKeyPath(sshContext ssh.Context) (string, error) { + return path.Join(sshContext.ConfigDir, automaticPrivateKeyName), nil } type cpOptions struct { diff --git a/pkg/cmd/codespace/ssh_test.go b/pkg/cmd/codespace/ssh_test.go index b76d304d7..d06bcda8e 100644 --- a/pkg/cmd/codespace/ssh_test.go +++ b/pkg/cmd/codespace/ssh_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os" + "path" "path/filepath" "strings" "testing" @@ -125,6 +126,10 @@ func TestGenerateAutomaticSSHKeys(t *testing.T) { } func TestSelectSSHKeys(t *testing.T) { + // This string will be subsituted in sshArgs for test cases + // This is to work around the temp test ssh dir not being known until the test is executing + substituteSSHDir := "SUB_SSH_DIR" + tests := []struct { sshDirFiles []string sshConfigKeys []string @@ -139,7 +144,7 @@ func TestSelectSSHKeys(t *testing.T) { wantKeyPair: &ssh.KeyPair{PrivateKeyPath: "custom-private-key", PublicKeyPath: "custom-private-key.pub"}, }, { - sshArgs: []string{"-i", automaticPrivateKeyName}, + sshArgs: []string{"-i", path.Join(substituteSSHDir, automaticPrivateKeyName)}, wantKeyPair: &ssh.KeyPair{PrivateKeyPath: automaticPrivateKeyName, PublicKeyPath: automaticPrivateKeyName + ".pub"}, }, { @@ -226,7 +231,12 @@ func TestSelectSSHKeys(t *testing.T) { t.Fatalf("could not write test config %v", err) } - tt.sshArgs = append([]string{"-F", configPath}, tt.sshArgs...) + var subbedSSHArgs []string + for _, arg := range tt.sshArgs { + subbedSSHArgs = append(subbedSSHArgs, strings.Replace(arg, substituteSSHDir, sshDir, -1)) + } + + tt.sshArgs = append([]string{"-F", configPath}, subbedSSHArgs...) gotKeyPair, gotShouldAddArg, err := selectSSHKeys(context.Background(), sshContext, tt.sshArgs, sshOptions{profile: tt.profileOpt}) @@ -254,11 +264,24 @@ func TestSelectSSHKeys(t *testing.T) { } // Strip the dir (sshDir) from the gotKeyPair paths so that they match wantKeyPair (which doesn't know the directory) - gotKeyPair.PrivateKeyPath = filepath.Base(gotKeyPair.PrivateKeyPath) - gotKeyPair.PublicKeyPath = filepath.Base(gotKeyPair.PublicKeyPath) + gotKeyPairJustFileNames := &ssh.KeyPair{ + PrivateKeyPath: filepath.Base(gotKeyPair.PrivateKeyPath), + PublicKeyPath: filepath.Base(gotKeyPair.PublicKeyPath), + } - if fmt.Sprintf("%v", gotKeyPair) != fmt.Sprintf("%v", tt.wantKeyPair) { - t.Errorf("Want selectSSHKeys result to be %v, got %v", tt.wantKeyPair, gotKeyPair) + if fmt.Sprintf("%v", gotKeyPairJustFileNames) != fmt.Sprintf("%v", tt.wantKeyPair) { + t.Errorf("Want selectSSHKeys result to be %v, got %v", tt.wantKeyPair, gotKeyPairJustFileNames) + } + + // If the automatic key pair is selected, it needs to exist no matter what + if strings.Contains(tt.wantKeyPair.PrivateKeyPath, automaticPrivateKeyName) { + if _, err := os.Stat(gotKeyPair.PrivateKeyPath); err != nil { + t.Errorf("Expected automatic key pair private key to exist, but it did not") + } + + if _, err := os.Stat(gotKeyPair.PublicKeyPath); err != nil { + t.Errorf("Expected automatic key pair public key to exist, but it did not") + } } } } diff --git a/pkg/ssh/ssh_keys.go b/pkg/ssh/ssh_keys.go index c750b608a..0b9e12f1f 100644 --- a/pkg/ssh/ssh_keys.go +++ b/pkg/ssh/ssh_keys.go @@ -26,7 +26,7 @@ type KeyPair struct { var ErrKeyAlreadyExists = errors.New("SSH key already exists") func (c *Context) LocalPublicKeys() ([]string, error) { - sshDir, err := c.sshDir() + sshDir, err := c.SshDir() if err != nil { return nil, err } @@ -45,7 +45,7 @@ func (c *Context) GenerateSSHKey(keyName string, passphrase string) (*KeyPair, e return nil, err } - sshDir, err := c.sshDir() + sshDir, err := c.SshDir() if err != nil { return nil, err } @@ -79,7 +79,7 @@ func (c *Context) GenerateSSHKey(keyName string, passphrase string) (*KeyPair, e return &keyPair, nil } -func (c *Context) sshDir() (string, error) { +func (c *Context) SshDir() (string, error) { if c.ConfigDir != "" { return c.ConfigDir, nil } From a569d1030d35bbf2fe8d79d451a1c12b38f89bf5 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 6 Nov 2024 12:56:13 +0100 Subject: [PATCH 51/76] Export empty results for cache list --- .../testdata/workflow/cache-list-empty.txtar | 36 +++++++++++ pkg/cmd/cache/list/list.go | 2 +- pkg/cmd/cache/list/list_test.go | 62 ++++++++++++++++++- 3 files changed, 98 insertions(+), 2 deletions(-) create mode 100644 acceptance/testdata/workflow/cache-list-empty.txtar diff --git a/acceptance/testdata/workflow/cache-list-empty.txtar b/acceptance/testdata/workflow/cache-list-empty.txtar new file mode 100644 index 000000000..0e6d32cb7 --- /dev/null +++ b/acceptance/testdata/workflow/cache-list-empty.txtar @@ -0,0 +1,36 @@ +# It's unclear what we want to do with these acceptance tests beyond our GHEC discovery, so skip new ones by default +skip + +# Set up env vars +env REPO=${ORG}/${SCRIPT_NAME}-${RANDOM_STRING} + +# Create a repository with a file so it has a default branch +exec gh repo create ${REPO} --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes ${REPO} + +# Set the repo to be targeted by all following commands +env GH_REPO=${REPO} + +# Listing the cache non-interactively shows nothing +exec gh cache list +! stdout '.' + +# Listing the cache non-interactively with --json shows an empty array +exec gh cache list --json id +stdout '\[\]' + +# Now set an env var so the commands run interactively and without colour for stdout matching +# Unfortunately testscript provides no way to turn them off again, and since this +# script is for discovery, we're not adding a new command. +env GH_FORCE_TTY=true +env CLICOLOR=0 + +# Listing the cache interactively shows an informative message on stderr +exec gh cache list +stderr 'No caches found in' + +# Listing the cache interactively with --json shows an empty array +exec gh cache list --json id +stdout '\[\]' diff --git a/pkg/cmd/cache/list/list.go b/pkg/cmd/cache/list/list.go index f5aa8fd5a..902285df6 100644 --- a/pkg/cmd/cache/list/list.go +++ b/pkg/cmd/cache/list/list.go @@ -106,7 +106,7 @@ func listRun(opts *ListOptions) error { return fmt.Errorf("%s Failed to get caches: %w", opts.IO.ColorScheme().FailureIcon(), err) } - if len(result.ActionsCaches) == 0 { + if len(result.ActionsCaches) == 0 && opts.Exporter == nil { return cmdutil.NewNoResultsError(fmt.Sprintf("No caches found in %s", ghrepo.FullName(repo))) } diff --git a/pkg/cmd/cache/list/list_test.go b/pkg/cmd/cache/list/list_test.go index 24d835bca..d4810cbcc 100644 --- a/pkg/cmd/cache/list/list_test.go +++ b/pkg/cmd/cache/list/list_test.go @@ -2,6 +2,7 @@ package list import ( "bytes" + "fmt" "net/http" "testing" "time" @@ -243,7 +244,8 @@ ID KEY SIZE CREATED ACCESSED wantErrMsg: "No caches found in OWNER/REPO", }, { - name: "displays no results", + name: "displays no results when there is a tty", + tty: true, stubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"), @@ -267,6 +269,48 @@ ID KEY SIZE CREATED ACCESSED wantErr: true, wantErrMsg: "X Failed to get caches: HTTP 404 (https://api.github.com/repos/OWNER/REPO/actions/caches?per_page=100)", }, + { + name: "calls the exporter when requested", + opts: ListOptions{ + Exporter: &verboseExporter{}, + }, + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"), + httpmock.JSONResponse(shared.CachePayload{ + ActionsCaches: []shared.Cache{ + { + Id: 1, + Key: "foo", + CreatedAt: time.Date(2021, 1, 1, 1, 1, 1, 1, time.UTC), + LastAccessedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC), + SizeInBytes: 100, + }, + }, + TotalCount: 1, + }), + ) + }, + wantErr: false, + wantStdout: "[{CreatedAt:2021-01-01 01:01:01.000000001 +0000 UTC Id:1 Key:foo LastAccessedAt:2022-01-01 01:01:01.000000001 +0000 UTC Ref: SizeInBytes:100 Version:}]", + }, + { + name: "calls the exporter even when there are no results", + opts: ListOptions{ + Exporter: &verboseExporter{}, + }, + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"), + httpmock.JSONResponse(shared.CachePayload{ + ActionsCaches: []shared.Cache{}, + TotalCount: 0, + }), + ) + }, + wantErr: false, + wantStdout: "[]", + }, } for _, tt := range tests { @@ -305,6 +349,22 @@ ID KEY SIZE CREATED ACCESSED } } +// The verboseExporter just writes data formatted as %+v to stdout. +// This allows for easy assertion on the data provided to the exporter. +type verboseExporter struct{} + +func (e *verboseExporter) Fields() []string { + return nil +} + +func (e *verboseExporter) Write(io *iostreams.IOStreams, data interface{}) error { + _, err := io.Out.Write([]byte(fmt.Sprintf("%+v", data))) + if err != nil { + return err + } + return nil +} + func Test_humanFileSize(t *testing.T) { tests := []struct { name string From 2318fde15f9fe5ef6853de5a41a03884c2a59779 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 6 Nov 2024 14:14:48 +0000 Subject: [PATCH 52/76] Bump actions/attest-build-provenance from 1.4.3 to 1.4.4 Bumps [actions/attest-build-provenance](https://github.com/actions/attest-build-provenance) from 1.4.3 to 1.4.4. - [Release notes](https://github.com/actions/attest-build-provenance/releases) - [Changelog](https://github.com/actions/attest-build-provenance/blob/main/RELEASE.md) - [Commits](https://github.com/actions/attest-build-provenance/compare/1c608d11d69870c2092266b3f9a6f3abbf17002c...ef244123eb79f2f7a7e75d99086184180e6d0018) --- updated-dependencies: - dependency-name: actions/attest-build-provenance dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- .github/workflows/deployment.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deployment.yml b/.github/workflows/deployment.yml index 82966ced4..57e926b3f 100644 --- a/.github/workflows/deployment.yml +++ b/.github/workflows/deployment.yml @@ -299,7 +299,7 @@ jobs: rpmsign --addsign dist/*.rpm - name: Attest release artifacts if: inputs.environment == 'production' - uses: actions/attest-build-provenance@1c608d11d69870c2092266b3f9a6f3abbf17002c # v1.4.3 + uses: actions/attest-build-provenance@ef244123eb79f2f7a7e75d99086184180e6d0018 # v1.4.4 with: subject-path: "dist/gh_*" - name: Run createrepo From 940560acf2721a0f09b6795d813d84a1192986d5 Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Wed, 6 Nov 2024 16:13:29 +0000 Subject: [PATCH 53/76] Fix ssh directory --- pkg/cmd/auth/shared/login_flow_test.go | 24 ++-- pkg/cmd/codespace/ssh.go | 7 +- pkg/cmd/codespace/ssh_test.go | 158 ++++++++++++++++++++++++- pkg/ssh/ssh_keys.go | 25 ++-- 4 files changed, 191 insertions(+), 23 deletions(-) diff --git a/pkg/cmd/auth/shared/login_flow_test.go b/pkg/cmd/auth/shared/login_flow_test.go index 8c8ba5d72..31cf2c107 100644 --- a/pkg/cmd/auth/shared/login_flow_test.go +++ b/pkg/cmd/auth/shared/login_flow_test.go @@ -101,10 +101,7 @@ func TestLogin(t *testing.T) { // simulate that the public key file has been generated _ = os.WriteFile(keyFile+".pub", []byte("PUBKEY asdf"), 0600) }) - opts.sshContext = ssh.Context{ - ConfigDir: dir, - KeygenExe: "ssh-keygen", - } + opts.sshContext = ssh.NewContextForTests(dir, "ssh-keygen") }, wantsConfig: map[string]string{ "example.com:user": "monalisa", @@ -112,6 +109,11 @@ func TestLogin(t *testing.T) { "example.com:git_protocol": "ssh", }, stderrAssert: func(t *testing.T, opts *LoginOptions, stderr string) { + sshDir, err := opts.sshContext.SshDir() + if err != nil { + t.Errorf("Could not load ssh config dir: %v", err) + } + assert.Equal(t, heredoc.Docf(` Tip: you can generate a Personal Access Token here https://example.com/settings/tokens The minimum required scopes are 'repo', 'read:org', 'admin:public_key'. @@ -119,7 +121,7 @@ func TestLogin(t *testing.T) { ✓ Configured git protocol ✓ Uploaded the SSH key to your GitHub account: %s ✓ Logged in as monalisa - `, filepath.Join(opts.sshContext.ConfigDir, "id_ed25519.pub")), stderr) + `, filepath.Join(sshDir, "id_ed25519.pub")), stderr) }, }, { @@ -179,10 +181,7 @@ func TestLogin(t *testing.T) { // simulate that the public key file has been generated _ = os.WriteFile(keyFile+".pub", []byte("PUBKEY asdf"), 0600) }) - opts.sshContext = ssh.Context{ - ConfigDir: dir, - KeygenExe: "ssh-keygen", - } + opts.sshContext = ssh.NewContextForTests(dir, "ssh-keygen") }, wantsConfig: map[string]string{ "example.com:user": "monalisa", @@ -190,6 +189,11 @@ func TestLogin(t *testing.T) { "example.com:git_protocol": "ssh", }, stderrAssert: func(t *testing.T, opts *LoginOptions, stderr string) { + sshDir, err := opts.sshContext.SshDir() + if err != nil { + t.Errorf("Could not load ssh config dir: %v", err) + } + assert.Equal(t, heredoc.Docf(` Tip: you can generate a Personal Access Token here https://example.com/settings/tokens The minimum required scopes are 'repo', 'read:org', 'admin:public_key'. @@ -197,7 +201,7 @@ func TestLogin(t *testing.T) { ✓ Configured git protocol ✓ Uploaded the SSH key to your GitHub account: %s ✓ Logged in as monalisa - `, filepath.Join(opts.sshContext.ConfigDir, "id_ed25519.pub")), stderr) + `, filepath.Join(sshDir, "id_ed25519.pub")), stderr) }, }, { diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 44ed30eb0..b79ec68c9 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -694,7 +694,12 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts sshOptions) (err erro } func automaticPrivateKeyPath(sshContext ssh.Context) (string, error) { - return path.Join(sshContext.ConfigDir, automaticPrivateKeyName), nil + sshDir, err := sshContext.SshDir() + if err != nil { + return "", err + } + + return path.Join(sshDir, automaticPrivateKeyName), nil } type cpOptions struct { diff --git a/pkg/cmd/codespace/ssh_test.go b/pkg/cmd/codespace/ssh_test.go index d06bcda8e..e656e0ecd 100644 --- a/pkg/cmd/codespace/ssh_test.go +++ b/pkg/cmd/codespace/ssh_test.go @@ -69,9 +69,7 @@ func TestGenerateAutomaticSSHKeys(t *testing.T) { for _, tt := range tests { dir := t.TempDir() - sshContext := ssh.Context{ - ConfigDir: dir, - } + sshContext := ssh.NewContextForTests(dir, "") for _, file := range tt.existingFiles { f, err := os.Create(filepath.Join(dir, file)) @@ -207,7 +205,159 @@ func TestSelectSSHKeys(t *testing.T) { for _, tt := range tests { sshDir := t.TempDir() - sshContext := ssh.Context{ConfigDir: sshDir} + sshContext := ssh.NewContextForTests(sshDir, "") + + for _, file := range tt.sshDirFiles { + f, err := os.Create(filepath.Join(sshDir, file)) + if err != nil { + t.Errorf("Failed to create test ssh dir file %q: %v", file, err) + } + f.Close() + } + + configPath := filepath.Join(sshDir, "test-config") + + // Seed the config with a non-existent key so that the default config won't apply + configContent := "IdentityFile dummy\n" + + for _, key := range tt.sshConfigKeys { + configContent += fmt.Sprintf("IdentityFile %s\n", filepath.Join(sshDir, key)) + } + + err := os.WriteFile(configPath, []byte(configContent), 0666) + if err != nil { + t.Fatalf("could not write test config %v", err) + } + + var subbedSSHArgs []string + for _, arg := range tt.sshArgs { + subbedSSHArgs = append(subbedSSHArgs, strings.Replace(arg, substituteSSHDir, sshDir, -1)) + } + + tt.sshArgs = append([]string{"-F", configPath}, subbedSSHArgs...) + + gotKeyPair, gotShouldAddArg, err := selectSSHKeys(context.Background(), sshContext, tt.sshArgs, sshOptions{profile: tt.profileOpt}) + + if tt.wantKeyPair == nil { + if err == nil { + t.Errorf("Expected error from selectSSHKeys but got nil") + } + + continue + } + + if err != nil { + t.Errorf("Unexpected error from selectSSHKeys: %v", err) + continue + } + + if gotKeyPair == nil { + t.Errorf("Expected non-nil result from selectSSHKeys but got nil") + continue + } + + if gotShouldAddArg != tt.wantShouldAddArg { + t.Errorf("Got wrong shouldAddArg value from selectSSHKeys, wanted %v got %v", tt.wantShouldAddArg, gotShouldAddArg) + continue + } + + // Strip the dir (sshDir) from the gotKeyPair paths so that they match wantKeyPair (which doesn't know the directory) + gotKeyPairJustFileNames := &ssh.KeyPair{ + PrivateKeyPath: filepath.Base(gotKeyPair.PrivateKeyPath), + PublicKeyPath: filepath.Base(gotKeyPair.PublicKeyPath), + } + + if fmt.Sprintf("%v", gotKeyPairJustFileNames) != fmt.Sprintf("%v", tt.wantKeyPair) { + t.Errorf("Want selectSSHKeys result to be %v, got %v", tt.wantKeyPair, gotKeyPairJustFileNames) + } + + // If the automatic key pair is selected, it needs to exist no matter what + if strings.Contains(tt.wantKeyPair.PrivateKeyPath, automaticPrivateKeyName) { + if _, err := os.Stat(gotKeyPair.PrivateKeyPath); err != nil { + t.Errorf("Expected automatic key pair private key to exist, but it did not") + } + + if _, err := os.Stat(gotKeyPair.PublicKeyPath); err != nil { + t.Errorf("Expected automatic key pair public key to exist, but it did not") + } + } + } +} + +func TestConfigGeneration(t *testing.T) { + tests := []struct { + selector *CodespaceSelector + }{ + // -i tests + { + sshArgs: []string{"-i", "custom-private-key"}, + wantKeyPair: &ssh.KeyPair{PrivateKeyPath: "custom-private-key", PublicKeyPath: "custom-private-key.pub"}, + }, + { + sshArgs: []string{"-i", path.Join(substituteSSHDir, automaticPrivateKeyName)}, + wantKeyPair: &ssh.KeyPair{PrivateKeyPath: automaticPrivateKeyName, PublicKeyPath: automaticPrivateKeyName + ".pub"}, + }, + { + // Edge case check for missing arg value + sshArgs: []string{"-i"}, + }, + + // Auto key exists tests + { + sshDirFiles: []string{automaticPrivateKeyName, automaticPrivateKeyName + ".pub"}, + wantKeyPair: &ssh.KeyPair{PrivateKeyPath: automaticPrivateKeyName, PublicKeyPath: automaticPrivateKeyName + ".pub"}, + wantShouldAddArg: true, + }, + { + sshDirFiles: []string{automaticPrivateKeyName, automaticPrivateKeyName + ".pub", "custom-private-key", "custom-private-key.pub"}, + wantKeyPair: &ssh.KeyPair{PrivateKeyPath: automaticPrivateKeyName, PublicKeyPath: automaticPrivateKeyName + ".pub"}, + wantShouldAddArg: true, + }, + + // SSH config tests + { + sshDirFiles: []string{"custom-private-key", "custom-private-key.pub"}, + sshConfigKeys: []string{"custom-private-key"}, + wantKeyPair: &ssh.KeyPair{PrivateKeyPath: "custom-private-key", PublicKeyPath: "custom-private-key.pub"}, + wantShouldAddArg: true, + }, + { + // 2 pairs, but only 1 is configured + sshDirFiles: []string{"custom-private-key", "custom-private-key.pub", "custom-private-key-2", "custom-private-key-2.pub"}, + sshConfigKeys: []string{"custom-private-key-2"}, + wantKeyPair: &ssh.KeyPair{PrivateKeyPath: "custom-private-key-2", PublicKeyPath: "custom-private-key-2.pub"}, + wantShouldAddArg: true, + }, + { + // 2 pairs, but only 1 has both public and private + sshDirFiles: []string{"custom-private-key", "custom-private-key-2", "custom-private-key-2.pub"}, + sshConfigKeys: []string{"custom-private-key", "custom-private-key-2"}, + wantKeyPair: &ssh.KeyPair{PrivateKeyPath: "custom-private-key-2", PublicKeyPath: "custom-private-key-2.pub"}, + wantShouldAddArg: true, + }, + + // Automatic key tests + { + wantKeyPair: &ssh.KeyPair{PrivateKeyPath: automaticPrivateKeyName, PublicKeyPath: automaticPrivateKeyName + ".pub"}, + wantShouldAddArg: true, + }, + { + // Renames old key pair to new + sshDirFiles: []string{automaticPrivateKeyNameOld, automaticPrivateKeyNameOld + ".pub"}, + wantKeyPair: &ssh.KeyPair{PrivateKeyPath: automaticPrivateKeyName, PublicKeyPath: automaticPrivateKeyName + ".pub"}, + wantShouldAddArg: true, + }, + { + // Other key is configured, but doesn't exist + sshConfigKeys: []string{"custom-private-key"}, + wantKeyPair: &ssh.KeyPair{PrivateKeyPath: automaticPrivateKeyName, PublicKeyPath: automaticPrivateKeyName + ".pub"}, + wantShouldAddArg: true, + }, + } + + for _, tt := range tests { + sshDir := t.TempDir() + sshContext := ssh.NewContextForTests(sshDir, "") for _, file := range tt.sshDirFiles { f, err := os.Create(filepath.Join(sshDir, file)) diff --git a/pkg/ssh/ssh_keys.go b/pkg/ssh/ssh_keys.go index 0b9e12f1f..309b498c5 100644 --- a/pkg/ssh/ssh_keys.go +++ b/pkg/ssh/ssh_keys.go @@ -14,8 +14,17 @@ import ( ) type Context struct { - ConfigDir string - KeygenExe string + configDir string + keygenExe string +} + +// NewContextForTests creates a new `ssh.Context` with internal properties set to the +// specified values. It should only be used to testing to inject test-specific setup. +func NewContextForTests(configDir, keygenExe string) Context { + return Context{ + configDir, + keygenExe, + } } type KeyPair struct { @@ -80,19 +89,19 @@ func (c *Context) GenerateSSHKey(keyName string, passphrase string) (*KeyPair, e } func (c *Context) SshDir() (string, error) { - if c.ConfigDir != "" { - return c.ConfigDir, nil + if c.configDir != "" { + return c.configDir, nil } dir, err := config.HomeDirPath(".ssh") if err == nil { - c.ConfigDir = dir + c.configDir = dir } return dir, err } func (c *Context) findKeygen() (string, error) { - if c.KeygenExe != "" { - return c.KeygenExe, nil + if c.keygenExe != "" { + return c.keygenExe, nil } keygenExe, err := safeexec.LookPath("ssh-keygen") @@ -107,7 +116,7 @@ func (c *Context) findKeygen() (string, error) { } if err == nil { - c.KeygenExe = keygenExe + c.keygenExe = keygenExe } return keygenExe, err } From b65c942e1f9fc7e00f0ee4b75604bc05980acb3f Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Wed, 6 Nov 2024 09:45:03 -0700 Subject: [PATCH 54/76] update verification slice building Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verification/sigstore.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/attestation/verification/sigstore.go b/pkg/cmd/attestation/verification/sigstore.go index 825f9da1c..86000405b 100644 --- a/pkg/cmd/attestation/verification/sigstore.go +++ b/pkg/cmd/attestation/verification/sigstore.go @@ -203,7 +203,8 @@ func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve return nil, ErrNoAttestationsVerified } - results := make([]*AttestationProcessingResult, 0) + results := make([]*AttestationProcessingResult, len(attestations)) + var verifyCount int var lastError error totalAttestations := len(attestations) for i, a := range attestations { @@ -215,13 +216,17 @@ func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve // move onto the next attestation in the for loop if verification fails continue } - results = append(results, apr) + results[verifyCount] = apr + verifyCount++ } - if len(results) == 0 { + if verifyCount == 0 { return nil, lastError } + // truncate the results slice to only include verified attestations + results = results[:verifyCount] + return results, nil } From 0665fb4916d971c2f426427fa1a1470e559966ed Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Wed, 6 Nov 2024 09:45:42 -0700 Subject: [PATCH 55/76] comments Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verification/sigstore.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/cmd/attestation/verification/sigstore.go b/pkg/cmd/attestation/verification/sigstore.go index 86000405b..66005d62e 100644 --- a/pkg/cmd/attestation/verification/sigstore.go +++ b/pkg/cmd/attestation/verification/sigstore.go @@ -216,6 +216,7 @@ func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve // move onto the next attestation in the for loop if verification fails continue } + // otherwise, add the result to the results slice and increment verifyCount results[verifyCount] = apr verifyCount++ } From 509a181d798fece7768e503db32d9eb0fe050587 Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Wed, 6 Nov 2024 19:10:28 +0000 Subject: [PATCH 56/76] Remove unimplemented tests --- pkg/cmd/codespace/ssh_test.go | 152 ---------------------------------- 1 file changed, 152 deletions(-) diff --git a/pkg/cmd/codespace/ssh_test.go b/pkg/cmd/codespace/ssh_test.go index e656e0ecd..cd04f05fa 100644 --- a/pkg/cmd/codespace/ssh_test.go +++ b/pkg/cmd/codespace/ssh_test.go @@ -284,158 +284,6 @@ func TestSelectSSHKeys(t *testing.T) { } } -func TestConfigGeneration(t *testing.T) { - tests := []struct { - selector *CodespaceSelector - }{ - // -i tests - { - sshArgs: []string{"-i", "custom-private-key"}, - wantKeyPair: &ssh.KeyPair{PrivateKeyPath: "custom-private-key", PublicKeyPath: "custom-private-key.pub"}, - }, - { - sshArgs: []string{"-i", path.Join(substituteSSHDir, automaticPrivateKeyName)}, - wantKeyPair: &ssh.KeyPair{PrivateKeyPath: automaticPrivateKeyName, PublicKeyPath: automaticPrivateKeyName + ".pub"}, - }, - { - // Edge case check for missing arg value - sshArgs: []string{"-i"}, - }, - - // Auto key exists tests - { - sshDirFiles: []string{automaticPrivateKeyName, automaticPrivateKeyName + ".pub"}, - wantKeyPair: &ssh.KeyPair{PrivateKeyPath: automaticPrivateKeyName, PublicKeyPath: automaticPrivateKeyName + ".pub"}, - wantShouldAddArg: true, - }, - { - sshDirFiles: []string{automaticPrivateKeyName, automaticPrivateKeyName + ".pub", "custom-private-key", "custom-private-key.pub"}, - wantKeyPair: &ssh.KeyPair{PrivateKeyPath: automaticPrivateKeyName, PublicKeyPath: automaticPrivateKeyName + ".pub"}, - wantShouldAddArg: true, - }, - - // SSH config tests - { - sshDirFiles: []string{"custom-private-key", "custom-private-key.pub"}, - sshConfigKeys: []string{"custom-private-key"}, - wantKeyPair: &ssh.KeyPair{PrivateKeyPath: "custom-private-key", PublicKeyPath: "custom-private-key.pub"}, - wantShouldAddArg: true, - }, - { - // 2 pairs, but only 1 is configured - sshDirFiles: []string{"custom-private-key", "custom-private-key.pub", "custom-private-key-2", "custom-private-key-2.pub"}, - sshConfigKeys: []string{"custom-private-key-2"}, - wantKeyPair: &ssh.KeyPair{PrivateKeyPath: "custom-private-key-2", PublicKeyPath: "custom-private-key-2.pub"}, - wantShouldAddArg: true, - }, - { - // 2 pairs, but only 1 has both public and private - sshDirFiles: []string{"custom-private-key", "custom-private-key-2", "custom-private-key-2.pub"}, - sshConfigKeys: []string{"custom-private-key", "custom-private-key-2"}, - wantKeyPair: &ssh.KeyPair{PrivateKeyPath: "custom-private-key-2", PublicKeyPath: "custom-private-key-2.pub"}, - wantShouldAddArg: true, - }, - - // Automatic key tests - { - wantKeyPair: &ssh.KeyPair{PrivateKeyPath: automaticPrivateKeyName, PublicKeyPath: automaticPrivateKeyName + ".pub"}, - wantShouldAddArg: true, - }, - { - // Renames old key pair to new - sshDirFiles: []string{automaticPrivateKeyNameOld, automaticPrivateKeyNameOld + ".pub"}, - wantKeyPair: &ssh.KeyPair{PrivateKeyPath: automaticPrivateKeyName, PublicKeyPath: automaticPrivateKeyName + ".pub"}, - wantShouldAddArg: true, - }, - { - // Other key is configured, but doesn't exist - sshConfigKeys: []string{"custom-private-key"}, - wantKeyPair: &ssh.KeyPair{PrivateKeyPath: automaticPrivateKeyName, PublicKeyPath: automaticPrivateKeyName + ".pub"}, - wantShouldAddArg: true, - }, - } - - for _, tt := range tests { - sshDir := t.TempDir() - sshContext := ssh.NewContextForTests(sshDir, "") - - for _, file := range tt.sshDirFiles { - f, err := os.Create(filepath.Join(sshDir, file)) - if err != nil { - t.Errorf("Failed to create test ssh dir file %q: %v", file, err) - } - f.Close() - } - - configPath := filepath.Join(sshDir, "test-config") - - // Seed the config with a non-existent key so that the default config won't apply - configContent := "IdentityFile dummy\n" - - for _, key := range tt.sshConfigKeys { - configContent += fmt.Sprintf("IdentityFile %s\n", filepath.Join(sshDir, key)) - } - - err := os.WriteFile(configPath, []byte(configContent), 0666) - if err != nil { - t.Fatalf("could not write test config %v", err) - } - - var subbedSSHArgs []string - for _, arg := range tt.sshArgs { - subbedSSHArgs = append(subbedSSHArgs, strings.Replace(arg, substituteSSHDir, sshDir, -1)) - } - - tt.sshArgs = append([]string{"-F", configPath}, subbedSSHArgs...) - - gotKeyPair, gotShouldAddArg, err := selectSSHKeys(context.Background(), sshContext, tt.sshArgs, sshOptions{profile: tt.profileOpt}) - - if tt.wantKeyPair == nil { - if err == nil { - t.Errorf("Expected error from selectSSHKeys but got nil") - } - - continue - } - - if err != nil { - t.Errorf("Unexpected error from selectSSHKeys: %v", err) - continue - } - - if gotKeyPair == nil { - t.Errorf("Expected non-nil result from selectSSHKeys but got nil") - continue - } - - if gotShouldAddArg != tt.wantShouldAddArg { - t.Errorf("Got wrong shouldAddArg value from selectSSHKeys, wanted %v got %v", tt.wantShouldAddArg, gotShouldAddArg) - continue - } - - // Strip the dir (sshDir) from the gotKeyPair paths so that they match wantKeyPair (which doesn't know the directory) - gotKeyPairJustFileNames := &ssh.KeyPair{ - PrivateKeyPath: filepath.Base(gotKeyPair.PrivateKeyPath), - PublicKeyPath: filepath.Base(gotKeyPair.PublicKeyPath), - } - - if fmt.Sprintf("%v", gotKeyPairJustFileNames) != fmt.Sprintf("%v", tt.wantKeyPair) { - t.Errorf("Want selectSSHKeys result to be %v, got %v", tt.wantKeyPair, gotKeyPairJustFileNames) - } - - // If the automatic key pair is selected, it needs to exist no matter what - if strings.Contains(tt.wantKeyPair.PrivateKeyPath, automaticPrivateKeyName) { - if _, err := os.Stat(gotKeyPair.PrivateKeyPath); err != nil { - t.Errorf("Expected automatic key pair private key to exist, but it did not") - } - - if _, err := os.Stat(gotKeyPair.PublicKeyPath); err != nil { - t.Errorf("Expected automatic key pair public key to exist, but it did not") - } - } - } -} - func testingSSHApp() *App { disabledCodespace := &api.Codespace{ Name: "disabledCodespace", From 2435f6915b3abf4134aae74d00ffa923bebc443e Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 6 Nov 2024 16:12:55 -0500 Subject: [PATCH 57/76] Minor nit suggestion --- pkg/ssh/ssh_keys.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/ssh/ssh_keys.go b/pkg/ssh/ssh_keys.go index 309b498c5..83d4f1b34 100644 --- a/pkg/ssh/ssh_keys.go +++ b/pkg/ssh/ssh_keys.go @@ -19,7 +19,7 @@ type Context struct { } // NewContextForTests creates a new `ssh.Context` with internal properties set to the -// specified values. It should only be used to testing to inject test-specific setup. +// specified values. It should only be used to inject test-specific setup. func NewContextForTests(configDir, keygenExe string) Context { return Context{ configDir, From 6d5a26cfd194e261e11015506c357d175fa4946a Mon Sep 17 00:00:00 2001 From: Sarah Barili Date: Wed, 6 Nov 2024 14:45:41 -0700 Subject: [PATCH 58/76] adding username validation to the invoker ssh server --- internal/codespaces/rpc/invoker.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/internal/codespaces/rpc/invoker.go b/internal/codespaces/rpc/invoker.go index b9d321802..e00b6c304 100644 --- a/internal/codespaces/rpc/invoker.go +++ b/internal/codespaces/rpc/invoker.go @@ -8,6 +8,7 @@ import ( "fmt" "net" "os" + "regexp" "strconv" "strings" "time" @@ -241,6 +242,9 @@ func (i *invoker) StartSSHServerWithOptions(ctx context.Context, options StartSS return 0, "", fmt.Errorf("failed to parse SSH server port: %w", err) } + if !isUsernameValid(response.User) { + return 0, "", fmt.Errorf("invalid username: %s", response.User) + } return port, response.User, nil } @@ -300,3 +304,10 @@ func (i *invoker) notifyCodespaceOfClientActivity(ctx context.Context, activity return nil } + +func isUsernameValid(username string) bool { + // assuming valid usernames are alphanumeric, with these special characters allowed: . _ - + var validUsernamePattern = `^[a-zA-Z0-9._-]+$` + re := regexp.MustCompile(validUsernamePattern) + return re.MatchString(username) +} From a780b488a3dbc0607081e80ea6111fc2113813d5 Mon Sep 17 00:00:00 2001 From: nilvng Date: Sat, 2 Nov 2024 16:31:52 +1100 Subject: [PATCH 59/76] fix: ignore template flag --- pkg/cmd/pr/create/create.go | 3 +++ pkg/cmd/pr/shared/params.go | 5 +++++ pkg/cmd/pr/shared/state.go | 2 ++ 3 files changed, 10 insertions(+) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 1242af2c5..06f4e1e89 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -278,6 +278,9 @@ func createRun(opts *CreateOptions) (err error) { state.Title = opts.Title state.Body = opts.Body } + if opts.Template != "" { + state.Template = opts.Template + } err = handlePush(*opts, *ctx) if err != nil { return diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 1b82c33f5..dc53d0059 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -27,6 +27,10 @@ func WithPrAndIssueQueryParams(client *api.Client, baseRepo ghrepo.Interface, ba if len(state.Assignees) > 0 { q.Set("assignees", strings.Join(state.Assignees, ",")) } + // Embded a template only if the body is empty, which is useful for Web Mode, and avoid duplication in Editor Mode + if len(state.Template) > 0 && len(state.Body) == 0 { + q.Set("template", state.Template) + } if len(state.Labels) > 0 { q.Set("labels", strings.Join(state.Labels, ",")) } @@ -40,6 +44,7 @@ func WithPrAndIssueQueryParams(client *api.Client, baseRepo ghrepo.Interface, ba if len(state.Milestones) > 0 { q.Set("milestone", state.Milestones[0]) } + u.RawQuery = q.Encode() return u.String(), nil } diff --git a/pkg/cmd/pr/shared/state.go b/pkg/cmd/pr/shared/state.go index 6467777f7..143021cb6 100644 --- a/pkg/cmd/pr/shared/state.go +++ b/pkg/cmd/pr/shared/state.go @@ -23,6 +23,8 @@ type IssueMetadataState struct { Body string Title string + Template string + Metadata []string Reviewers []string Assignees []string From 874fa7ad4d21e4ada100213ae8fd287f7dfd4748 Mon Sep 17 00:00:00 2001 From: nilvng Date: Sat, 2 Nov 2024 17:01:19 +1100 Subject: [PATCH 60/76] feat: add test --- pkg/cmd/pr/create/create_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 81684ff00..d31174999 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1714,6 +1714,19 @@ func Test_generateCompareURL(t *testing.T) { want: "https://github.com/OWNER/REPO/compare/main%2Ftrunk...owner:%21$&%27%28%29+%2C%3B=@?body=&expand=1", wantErr: false, }, + { + name: "with template", + ctx: CreateContext{ + BaseRepo: api.InitRepoHostname(&api.Repository{Name: "REPO", Owner: api.RepositoryOwner{Login: "OWNER"}}, "github.com"), + BaseBranch: "main", + HeadBranchLabel: "feature", + }, + state: shared.IssueMetadataState{ + Template: "story.md", + }, + want: "https://github.com/OWNER/REPO/compare/main...feature?body=&expand=1&template=story.md", + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 2eaab56912183bba4527c54e09388f3c57aec36e Mon Sep 17 00:00:00 2001 From: nilvng Date: Thu, 7 Nov 2024 00:43:08 +1100 Subject: [PATCH 61/76] chore: tidy up --- pkg/cmd/pr/shared/params.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index dc53d0059..128c51068 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -27,7 +27,7 @@ func WithPrAndIssueQueryParams(client *api.Client, baseRepo ghrepo.Interface, ba if len(state.Assignees) > 0 { q.Set("assignees", strings.Join(state.Assignees, ",")) } - // Embded a template only if the body is empty, which is useful for Web Mode, and avoid duplication in Editor Mode + // Set a template parameter if no body parameter is provided e.g. Web Mode if len(state.Template) > 0 && len(state.Body) == 0 { q.Set("template", state.Template) } From ddf7287ab893b7d672e089e0eaffebf40f03471c Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 6 Nov 2024 22:49:02 -0500 Subject: [PATCH 62/76] Test extension command update behaviors This commit expands upon the previous work by creating tests around extension command execution and how various extension update scenarios are handled. Along the way, the logic handling formatting update messaging has been switched to use `ColorScheme` in order to honor color behavior flags. --- pkg/cmd/root/extension.go | 34 ++++---- pkg/cmd/root/extension_test.go | 146 +++++++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 15 deletions(-) create mode 100644 pkg/cmd/root/extension_test.go diff --git a/pkg/cmd/root/extension.go b/pkg/cmd/root/extension.go index 89f0085f3..fcf0549c2 100644 --- a/pkg/cmd/root/extension.go +++ b/pkg/cmd/root/extension.go @@ -5,10 +5,10 @@ import ( "fmt" "os/exec" "strings" + "time" "github.com/cli/cli/v2/pkg/extensions" "github.com/cli/cli/v2/pkg/iostreams" - "github.com/mgutz/ansi" "github.com/spf13/cobra" ) @@ -24,8 +24,8 @@ type extensionReleaseInfo struct { } func NewCmdExtension(io *iostreams.IOStreams, em extensions.ExtensionManager, ext extensions.Extension) *cobra.Command { - // Setup channel containing information about potential latest release info updateMessageChan := make(chan *extensionReleaseInfo) + cs := io.ColorScheme() return &cobra.Command{ Use: ext.Name(), @@ -56,20 +56,24 @@ func NewCmdExtension(io *iostreams.IOStreams, em extensions.ExtensionManager, ex }, // PostRun handles communicating extension release information if found PostRun: func(c *cobra.Command, args []string) { - releaseInfo := <-updateMessageChan - if releaseInfo != nil { - stderr := io.ErrOut - fmt.Fprintf(stderr, "\n\n%s %s → %s\n", - ansi.Color(fmt.Sprintf("A new release of %s is available:", ext.Name()), "yellow"), - ansi.Color(strings.TrimPrefix(releaseInfo.CurrentVersion, "v"), "cyan"), - ansi.Color(strings.TrimPrefix(releaseInfo.LatestVersion, "v"), "cyan")) - if releaseInfo.Pinned { - fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s --force\n", ext.Name()) - } else { - fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s\n", ext.Name()) + select { + case releaseInfo := <-updateMessageChan: + if releaseInfo != nil { + stderr := io.ErrOut + fmt.Fprintf(stderr, "\n\n%s %s → %s\n", + cs.Yellowf("A new release of %s is available:", ext.Name()), + cs.Cyan(strings.TrimPrefix(releaseInfo.CurrentVersion, "v")), + cs.Cyan(strings.TrimPrefix(releaseInfo.LatestVersion, "v"))) + if releaseInfo.Pinned { + fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s --force\n", ext.Name()) + } else { + fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s\n", ext.Name()) + } + fmt.Fprintf(stderr, "%s\n\n", + cs.Yellow(releaseInfo.URL)) } - fmt.Fprintf(stderr, "%s\n\n", - ansi.Color(releaseInfo.URL, "yellow")) + case <-time.After(3 * time.Second): + // Bail on checking for new extension update as its taking too long } }, GroupID: "extension", diff --git a/pkg/cmd/root/extension_test.go b/pkg/cmd/root/extension_test.go new file mode 100644 index 000000000..e756bf504 --- /dev/null +++ b/pkg/cmd/root/extension_test.go @@ -0,0 +1,146 @@ +package root_test + +import ( + "io" + "testing" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/pkg/cmd/root" + "github.com/cli/cli/v2/pkg/extensions" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewCmdExtension_Updates(t *testing.T) { + tests := []struct { + name string + extCurrentVersion string + extIsPinned bool + extLatestVersion string + extName string + extUpdateAvailable bool + extURL string + wantStderr string + }{ + { + name: "no update available", + extName: "no-update", + extUpdateAvailable: false, + extCurrentVersion: "1.0.0", + extLatestVersion: "1.0.0", + }, + { + name: "major update", + extName: "major-update", + extUpdateAvailable: true, + extCurrentVersion: "1.0.0", + extLatestVersion: "2.0.0", + wantStderr: heredoc.Doc(` + A new release of major-update is available: 1.0.0 → 2.0.0 + To upgrade, run: gh extension upgrade major-update + `), + }, + { + name: "major update, pinned", + extName: "major-update", + extUpdateAvailable: true, + extCurrentVersion: "1.0.0", + extLatestVersion: "2.0.0", + extIsPinned: true, + wantStderr: heredoc.Doc(` + A new release of major-update is available: 1.0.0 → 2.0.0 + To upgrade, run: gh extension upgrade major-update --force + `), + }, + { + name: "minor update", + extName: "minor-update", + extUpdateAvailable: true, + extCurrentVersion: "1.0.0", + extLatestVersion: "1.1.0", + wantStderr: heredoc.Doc(` + A new release of minor-update is available: 1.0.0 → 1.1.0 + To upgrade, run: gh extension upgrade minor-update + `), + }, + { + name: "minor update, pinned", + extName: "minor-update", + extUpdateAvailable: true, + extCurrentVersion: "1.0.0", + extLatestVersion: "1.1.0", + extIsPinned: true, + wantStderr: heredoc.Doc(` + A new release of minor-update is available: 1.0.0 → 1.1.0 + To upgrade, run: gh extension upgrade minor-update --force + `), + }, + { + name: "patch update", + extName: "patch-update", + extUpdateAvailable: true, + extCurrentVersion: "1.0.0", + extLatestVersion: "1.0.1", + wantStderr: heredoc.Doc(` + A new release of patch-update is available: 1.0.0 → 1.0.1 + To upgrade, run: gh extension upgrade patch-update + `), + }, + { + name: "patch update, pinned", + extName: "patch-update", + extUpdateAvailable: true, + extCurrentVersion: "1.0.0", + extLatestVersion: "1.0.1", + extIsPinned: true, + wantStderr: heredoc.Doc(` + A new release of patch-update is available: 1.0.0 → 1.0.1 + To upgrade, run: gh extension upgrade patch-update --force + `), + }, + } + + for _, tt := range tests { + ios, _, _, stderr := iostreams.Test() + + em := &extensions.ExtensionManagerMock{ + DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) { + // Assume extension executed / dispatched without problems as test is focused on upgrade checking. + return true, nil + }, + } + + ext := &extensions.ExtensionMock{ + CurrentVersionFunc: func() string { + return tt.extCurrentVersion + }, + IsPinnedFunc: func() bool { + return tt.extIsPinned + }, + LatestVersionFunc: func() string { + return tt.extLatestVersion + }, + NameFunc: func() string { + return tt.extName + }, + UpdateAvailableFunc: func() bool { + return tt.extUpdateAvailable + }, + URLFunc: func() string { + return tt.extURL + }, + } + + cmd := root.NewCmdExtension(ios, em, ext) + + _, err := cmd.ExecuteC() + require.NoError(t, err) + + if tt.wantStderr == "" { + assert.Emptyf(t, stderr.String(), "executing extension command should output nothing to stderr") + } else { + assert.Containsf(t, stderr.String(), tt.wantStderr, "executing extension command should output message about upgrade to stderr") + } + } +} From e629443a2c1451a6dbe6d99897a310f76b1af59e Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 6 Nov 2024 23:18:32 -0500 Subject: [PATCH 63/76] Delete .github/licenses.tmpl --- .github/licenses.tmpl | 13 ------------- 1 file changed, 13 deletions(-) delete mode 100644 .github/licenses.tmpl diff --git a/.github/licenses.tmpl b/.github/licenses.tmpl deleted file mode 100644 index 4a9083e92..000000000 --- a/.github/licenses.tmpl +++ /dev/null @@ -1,13 +0,0 @@ -# GitHub CLI dependencies - -The following open source dependencies are used to build the [GitHub CLI _(`gh`)_](https://github.com/cli/cli). - -## Go Packages - -Some packages may only be included on certain architectures or operating systems. - -| **Module** | **Version** | **License** | **License File** | **Package Docs** -| ---------- | ----------- | ----------- | ---------------- | ---------------- -{{- range . }} -| {{ .Name }} | {{ .Version }} | {{ .LicenseName }} | {{ .LicenseURL }} | [pkg.go.dev](https://pkg.go.dev/{{ .Name }}@{{ .Version }}) -{{- end }} From 54c073965934a9bd285b913ec25ab96445850c8a Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 6 Nov 2024 23:18:43 -0500 Subject: [PATCH 64/76] Delete .github/workflows/licenses.yml --- .github/workflows/licenses.yml | 40 ---------------------------------- 1 file changed, 40 deletions(-) delete mode 100644 .github/workflows/licenses.yml diff --git a/.github/workflows/licenses.yml b/.github/workflows/licenses.yml deleted file mode 100644 index 479349714..000000000 --- a/.github/workflows/licenses.yml +++ /dev/null @@ -1,40 +0,0 @@ -name: Licensing -on: - push: - tags: - - "v*" -env: - LICENSE_DIR: licenses - LICENSE_ARCHIVE: licenses.tgz -jobs: - go-licenses: - runs-on: ubuntu-latest - steps: - - name: Checkout - uses: actions/checkout@v4 - - - name: Set up Go - uses: actions/setup-go@v5 - with: - go-version-file: 'go.mod' - - - name: Generate Go license notices - run: | - go install github.com/google/go-licenses@latest - GOROOT=$(go env GOROOT) go-licenses save ./... --stderrthreshold=ERROR --logtostderr=false --save_path "$LICENSE_DIR" - GOROOT=$(go env GOROOT) go-licenses report ./... --template=.github/licenses.tmpl --stderrthreshold=ERROR --logtostderr=false > "$LICENSE_DIR"/README.md - - - name: Upload Go license notices - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - tar czf "$LICENSE_ARCHIVE" "$LICENSE_DIR" - gh release upload "$GITHUB_REF_NAME" --clobber -- "$LICENSE_ARCHIVE" - - if gh release view "$GITHUB_REF_NAME" >/dev/null; then - echo "uploading assets to an existing release..." - gh release upload "$GITHUB_REF_NAME" --clobber -- "$LICENSE_ARCHIVE" - else - echo "cannot upload as something else should create the release first..." - exit 1 - fi From 3ec657d087a963b11987b5950ecbe9eae9504cb4 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Thu, 7 Nov 2024 17:34:34 -0500 Subject: [PATCH 65/76] Enhance extension upgrade tests for URL --- pkg/cmd/root/extension_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/pkg/cmd/root/extension_test.go b/pkg/cmd/root/extension_test.go index e756bf504..ef94dcc71 100644 --- a/pkg/cmd/root/extension_test.go +++ b/pkg/cmd/root/extension_test.go @@ -29,6 +29,7 @@ func TestNewCmdExtension_Updates(t *testing.T) { extUpdateAvailable: false, extCurrentVersion: "1.0.0", extLatestVersion: "1.0.0", + extURL: "https//github.com/dne/no-update", }, { name: "major update", @@ -36,9 +37,11 @@ func TestNewCmdExtension_Updates(t *testing.T) { extUpdateAvailable: true, extCurrentVersion: "1.0.0", extLatestVersion: "2.0.0", + extURL: "https//github.com/dne/major-update", wantStderr: heredoc.Doc(` A new release of major-update is available: 1.0.0 → 2.0.0 To upgrade, run: gh extension upgrade major-update + https//github.com/dne/major-update `), }, { @@ -48,9 +51,11 @@ func TestNewCmdExtension_Updates(t *testing.T) { extCurrentVersion: "1.0.0", extLatestVersion: "2.0.0", extIsPinned: true, + extURL: "https//github.com/dne/major-update", wantStderr: heredoc.Doc(` A new release of major-update is available: 1.0.0 → 2.0.0 To upgrade, run: gh extension upgrade major-update --force + https//github.com/dne/major-update `), }, { @@ -59,9 +64,11 @@ func TestNewCmdExtension_Updates(t *testing.T) { extUpdateAvailable: true, extCurrentVersion: "1.0.0", extLatestVersion: "1.1.0", + extURL: "https//github.com/dne/minor-update", wantStderr: heredoc.Doc(` A new release of minor-update is available: 1.0.0 → 1.1.0 To upgrade, run: gh extension upgrade minor-update + https//github.com/dne/minor-update `), }, { @@ -70,10 +77,12 @@ func TestNewCmdExtension_Updates(t *testing.T) { extUpdateAvailable: true, extCurrentVersion: "1.0.0", extLatestVersion: "1.1.0", + extURL: "https//github.com/dne/minor-update", extIsPinned: true, wantStderr: heredoc.Doc(` A new release of minor-update is available: 1.0.0 → 1.1.0 To upgrade, run: gh extension upgrade minor-update --force + https//github.com/dne/minor-update `), }, { @@ -82,9 +91,11 @@ func TestNewCmdExtension_Updates(t *testing.T) { extUpdateAvailable: true, extCurrentVersion: "1.0.0", extLatestVersion: "1.0.1", + extURL: "https//github.com/dne/patch-update", wantStderr: heredoc.Doc(` A new release of patch-update is available: 1.0.0 → 1.0.1 To upgrade, run: gh extension upgrade patch-update + https//github.com/dne/patch-update `), }, { @@ -93,10 +104,12 @@ func TestNewCmdExtension_Updates(t *testing.T) { extUpdateAvailable: true, extCurrentVersion: "1.0.0", extLatestVersion: "1.0.1", + extURL: "https//github.com/dne/patch-update", extIsPinned: true, wantStderr: heredoc.Doc(` A new release of patch-update is available: 1.0.0 → 1.0.1 To upgrade, run: gh extension upgrade patch-update --force + https//github.com/dne/patch-update `), }, } From a02f84528a43d7cb5e68bf7060e7b7abeecb00ee Mon Sep 17 00:00:00 2001 From: Sarah Barili <103978419+sarahbarili@users.noreply.github.com> Date: Fri, 8 Nov 2024 09:11:44 -0700 Subject: [PATCH 66/76] Update internal/codespaces/rpc/invoker.go Co-authored-by: Caleb Brose <5447118+cmbrose@users.noreply.github.com> --- internal/codespaces/rpc/invoker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/codespaces/rpc/invoker.go b/internal/codespaces/rpc/invoker.go index e00b6c304..6ba8843ac 100644 --- a/internal/codespaces/rpc/invoker.go +++ b/internal/codespaces/rpc/invoker.go @@ -307,7 +307,7 @@ func (i *invoker) notifyCodespaceOfClientActivity(ctx context.Context, activity func isUsernameValid(username string) bool { // assuming valid usernames are alphanumeric, with these special characters allowed: . _ - - var validUsernamePattern = `^[a-zA-Z0-9._-]+$` + var validUsernamePattern = `^[a-zA-Z0-9_][-.a-zA-Z0-9_]*$` re := regexp.MustCompile(validUsernamePattern) return re.MatchString(username) } From 4a7f2e57b0c58fd2c0af5ad101d54189eb3ce805 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 11 Nov 2024 13:57:05 +0100 Subject: [PATCH 67/76] Support bare repo creation --- pkg/cmd/repo/create/create.go | 8 ++- pkg/cmd/repo/create/create_test.go | 95 ++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index 2fc127aba..684ba78ba 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -748,10 +748,12 @@ func isLocalRepo(gitClient *git.Client) (bool, error) { return false, projectDirErr } } - if projectDir != ".git" { - return false, nil + + if projectDir == ".git" || projectDir == "." { + return true, nil } - return true, nil + + return false, nil } // clone the checkout branch to specified path diff --git a/pkg/cmd/repo/create/create_test.go b/pkg/cmd/repo/create/create_test.go index cc0ec602a..cef86db9c 100644 --- a/pkg/cmd/repo/create/create_test.go +++ b/pkg/cmd/repo/create/create_test.go @@ -443,6 +443,68 @@ func Test_createRun(t *testing.T) { }, wantStdout: "✓ Created repository OWNER/REPO on GitHub\n https://github.com/OWNER/REPO\n", }, + { + name: "interactive with existing bare repository public", + opts: &CreateOptions{Interactive: true}, + tty: true, + promptStubs: func(p *prompter.PrompterMock) { + p.ConfirmFunc = func(message string, defaultValue bool) (bool, error) { + switch message { + case "Add a remote?": + return false, nil + default: + return false, fmt.Errorf("unexpected confirm prompt: %s", message) + } + } + p.InputFunc = func(message, defaultValue string) (string, error) { + switch message { + case "Path to local repository": + return defaultValue, nil + case "Repository name": + return "REPO", nil + case "Description": + return "my new repo", nil + default: + return "", fmt.Errorf("unexpected input prompt: %s", message) + } + } + p.SelectFunc = func(message, defaultValue string, options []string) (int, error) { + switch message { + case "What would you like to do?": + return prompter.IndexFor(options, "Push an existing local repository to GitHub") + case "Visibility": + return prompter.IndexFor(options, "Private") + default: + return 0, fmt.Errorf("unexpected select prompt: %s", message) + } + } + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"someuser","organizations":{"nodes": []}}}}`)) + reg.Register( + httpmock.GraphQL(`mutation RepositoryCreate\b`), + httpmock.StringResponse(` + { + "data": { + "createRepository": { + "repository": { + "id": "REPOID", + "name": "REPO", + "owner": {"login":"OWNER"}, + "url": "https://github.com/OWNER/REPO" + } + } + } + }`)) + }, + execStubs: func(cs *run.CommandStubber) { + cs.Register(`git -C . rev-parse --git-dir`, 0, ".") + cs.Register(`git -C . rev-parse HEAD`, 0, "commithash") + }, + wantStdout: "✓ Created repository OWNER/REPO on GitHub\n https://github.com/OWNER/REPO\n", + }, { name: "interactive with existing repository public add remote and push", opts: &CreateOptions{Interactive: true}, @@ -696,6 +758,39 @@ func Test_createRun(t *testing.T) { }, wantStdout: "https://github.com/OWNER/REPO\n", }, + { + name: "noninteractive create bare from source", + opts: &CreateOptions{ + Interactive: false, + Source: ".", + Name: "REPO", + Visibility: "PRIVATE", + }, + tty: false, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`mutation RepositoryCreate\b`), + httpmock.StringResponse(` + { + "data": { + "createRepository": { + "repository": { + "id": "REPOID", + "name": "REPO", + "owner": {"login":"OWNER"}, + "url": "https://github.com/OWNER/REPO" + } + } + } + }`)) + }, + execStubs: func(cs *run.CommandStubber) { + cs.Register(`git -C . rev-parse --git-dir`, 0, ".") + cs.Register(`git -C . rev-parse HEAD`, 0, "commithash") + cs.Register(`git -C . remote add origin https://github.com/OWNER/REPO`, 0, "") + }, + wantStdout: "https://github.com/OWNER/REPO\n", + }, { name: "noninteractive clone from scratch", opts: &CreateOptions{ From bc85e11d05ec2af5e322d2e324405e6d412d1e6a Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 11 Nov 2024 14:11:16 +0100 Subject: [PATCH 68/76] Backfill repo creation failure tests --- pkg/cmd/repo/create/create_test.go | 36 +++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/repo/create/create_test.go b/pkg/cmd/repo/create/create_test.go index cef86db9c..490a976ba 100644 --- a/pkg/cmd/repo/create/create_test.go +++ b/pkg/cmd/repo/create/create_test.go @@ -791,6 +791,36 @@ func Test_createRun(t *testing.T) { }, wantStdout: "https://github.com/OWNER/REPO\n", }, + { + name: "noninteractive create from cwd that isn't a git repo", + opts: &CreateOptions{ + Interactive: false, + Source: ".", + Name: "REPO", + Visibility: "PRIVATE", + }, + tty: false, + execStubs: func(cs *run.CommandStubber) { + cs.Register(`git -C . rev-parse --git-dir`, 128, "") + }, + wantErr: true, + errMsg: "current directory is not a git repository. Run `git init` to initialize it", + }, + { + name: "noninteractive create from cwd that isn't a git repo", + opts: &CreateOptions{ + Interactive: false, + Source: "some-dir", + Name: "REPO", + Visibility: "PRIVATE", + }, + tty: false, + execStubs: func(cs *run.CommandStubber) { + cs.Register(`git -C some-dir rev-parse --git-dir`, 128, "") + }, + wantErr: true, + errMsg: "some-dir is not a git repository. Run `git -C \"some-dir\" init` to initialize it", + }, { name: "noninteractive clone from scratch", opts: &CreateOptions{ @@ -951,11 +981,11 @@ func Test_createRun(t *testing.T) { defer reg.Verify(t) err := createRun(tt.opts) if tt.wantErr { - assert.Error(t, err) - assert.Equal(t, tt.errMsg, err.Error()) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errMsg) return } - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, tt.wantStdout, stdout.String()) assert.Equal(t, "", stderr.String()) }) From f515e9c1e7cc0472e3c15e73c55e6e110a204e71 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 11 Nov 2024 14:12:25 +0100 Subject: [PATCH 69/76] Use errWithExitCode interface in repo create isLocalRepo --- pkg/cmd/repo/create/create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index 684ba78ba..c3a16956d 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -740,7 +740,7 @@ func hasCommits(gitClient *git.Client) (bool, error) { func isLocalRepo(gitClient *git.Client) (bool, error) { projectDir, projectDirErr := gitClient.GitDir(context.Background()) if projectDirErr != nil { - var execError *exec.ExitError + var execError errWithExitCode if errors.As(projectDirErr, &execError) { if exitCode := int(execError.ExitCode()); exitCode == 128 { return false, nil From 2efb9935db2ba3429559c21a60d19c9504a1751b Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 11 Nov 2024 14:18:26 +0100 Subject: [PATCH 70/76] Doc isLocalRepo and git.Client IsLocalRepo differences --- pkg/cmd/repo/create/create.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index c3a16956d..77961c987 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -736,7 +736,11 @@ func hasCommits(gitClient *git.Client) (bool, error) { return false, nil } -// check if path is the top level directory of a git repo +// Check if path is the top level directory of a git repo +// This is subtly different from the git.Client IsLocalRepo method, +// which returns true if we are _anywhere_ inside of a git repository. +// I'm not sure whether this distinction really matters for repo create, +// but I'm not confident enough right now to make that change. func isLocalRepo(gitClient *git.Client) (bool, error) { projectDir, projectDirErr := gitClient.GitDir(context.Background()) if projectDirErr != nil { From 6a97dbfadf0322cd024091efc8fc96a2a71ce23f Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 11 Nov 2024 15:34:43 +0100 Subject: [PATCH 71/76] Add acceptance test for bare repo create --- .../testdata/repo/repo-create-bare.txtar | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 acceptance/testdata/repo/repo-create-bare.txtar diff --git a/acceptance/testdata/repo/repo-create-bare.txtar b/acceptance/testdata/repo/repo-create-bare.txtar new file mode 100644 index 000000000..b835c420b --- /dev/null +++ b/acceptance/testdata/repo/repo-create-bare.txtar @@ -0,0 +1,35 @@ +# It's unclear what we want to do with these acceptance tests beyond our GHEC discovery, so skip new ones by default +skip + +# Set up env var +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Initialise a local repository with two branches +# We expect a bare repo to have all refs pushed with --mirror +mkdir ${REPO} +cd ${REPO} +exec git init +exec git checkout -b feature-1 +exec git commit --allow-empty -m 'Empty Commit 1' + +exec git checkout -b feature-2 +exec git commit --allow-empty -m 'Empty Commit 2' + +# Clone a bare repo from that local repo +cd .. +exec git clone --bare ${REPO} ${REPO}-bare +cd ${REPO}-bare + +# Create a GitHub repository from that bare repo +exec gh repo create ${ORG}/${REPO} --private --source . --push --remote bare + +# Defer repo cleanup +defer gh repo delete --yes ${ORG}/${REPO} + +# Check the remote repo has both branches +exec gh api /repos/${ORG}/${REPO}/branches +stdout 'feature-1' +stdout 'feature-2' From e3665955a5143325f497cdf10c2076d5e76b69c0 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 11 Nov 2024 16:08:50 +0100 Subject: [PATCH 72/76] Push --mirror on bare repo create --- pkg/cmd/repo/create/create.go | 52 +++++++++++++++++++++--------- pkg/cmd/repo/create/create_test.go | 36 +++++++++++++-------- 2 files changed, 59 insertions(+), 29 deletions(-) diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index 77961c987..ac4ef39c9 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -556,11 +556,11 @@ func createFromLocal(opts *CreateOptions) error { return err } - isRepo, err := isLocalRepo(opts.GitClient) + repoType, err := localRepoType(opts.GitClient) if err != nil { return err } - if !isRepo { + if repoType == unknown { if repoPath == "." { return fmt.Errorf("current directory is not a git repository. Run `git init` to initialize it") } @@ -659,15 +659,31 @@ func createFromLocal(opts *CreateOptions) error { } } - if opts.Push { + if opts.Push && repoType == working { err := opts.GitClient.Push(context.Background(), baseRemote, "HEAD") if err != nil { return err } + if isTTY { fmt.Fprintf(stdout, "%s Pushed commits to %s\n", cs.SuccessIcon(), remoteURL) } } + + if opts.Push && repoType == bare { + cmd, err := opts.GitClient.AuthenticatedCommand(context.Background(), "push", baseRemote, "--mirror") + if err != nil { + return err + } + if err = cmd.Run(); err != nil { + return err + } + + if isTTY { + fmt.Fprintf(stdout, "%s Mirrored all refs to %s\n", cs.SuccessIcon(), remoteURL) + } + } + return nil } @@ -736,28 +752,34 @@ func hasCommits(gitClient *git.Client) (bool, error) { return false, nil } -// Check if path is the top level directory of a git repo -// This is subtly different from the git.Client IsLocalRepo method, -// which returns true if we are _anywhere_ inside of a git repository. -// I'm not sure whether this distinction really matters for repo create, -// but I'm not confident enough right now to make that change. -func isLocalRepo(gitClient *git.Client) (bool, error) { +type repoType int + +const ( + unknown repoType = iota + working + bare +) + +func localRepoType(gitClient *git.Client) (repoType, error) { projectDir, projectDirErr := gitClient.GitDir(context.Background()) if projectDirErr != nil { var execError errWithExitCode if errors.As(projectDirErr, &execError) { if exitCode := int(execError.ExitCode()); exitCode == 128 { - return false, nil + return unknown, nil } - return false, projectDirErr + return unknown, projectDirErr } } - if projectDir == ".git" || projectDir == "." { - return true, nil + switch projectDir { + case ".": + return bare, nil + case ".git": + return working, nil + default: + return unknown, nil } - - return false, nil } // clone the checkout branch to specified path diff --git a/pkg/cmd/repo/create/create_test.go b/pkg/cmd/repo/create/create_test.go index 490a976ba..a0f9ac207 100644 --- a/pkg/cmd/repo/create/create_test.go +++ b/pkg/cmd/repo/create/create_test.go @@ -444,14 +444,16 @@ func Test_createRun(t *testing.T) { wantStdout: "✓ Created repository OWNER/REPO on GitHub\n https://github.com/OWNER/REPO\n", }, { - name: "interactive with existing bare repository public", + name: "interactive with existing bare repository public and push", opts: &CreateOptions{Interactive: true}, tty: true, promptStubs: func(p *prompter.PrompterMock) { p.ConfirmFunc = func(message string, defaultValue bool) (bool, error) { switch message { case "Add a remote?": - return false, nil + return true, nil + case `Would you like to push commits from the current branch to "origin"?`: + return true, nil default: return false, fmt.Errorf("unexpected confirm prompt: %s", message) } @@ -464,6 +466,8 @@ func Test_createRun(t *testing.T) { return "REPO", nil case "Description": return "my new repo", nil + case "What should the new remote be called?": + return defaultValue, nil default: return "", fmt.Errorf("unexpected input prompt: %s", message) } @@ -502,8 +506,10 @@ func Test_createRun(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git -C . rev-parse --git-dir`, 0, ".") cs.Register(`git -C . rev-parse HEAD`, 0, "commithash") + cs.Register(`git -C . remote add origin https://github.com/OWNER/REPO`, 0, "") + cs.Register(`git -C . push origin --mirror`, 0, "") }, - wantStdout: "✓ Created repository OWNER/REPO on GitHub\n https://github.com/OWNER/REPO\n", + wantStdout: "✓ Created repository OWNER/REPO on GitHub\n https://github.com/OWNER/REPO\n✓ Added remote https://github.com/OWNER/REPO.git\n✓ Mirrored all refs to https://github.com/OWNER/REPO.git\n", }, { name: "interactive with existing repository public add remote and push", @@ -759,10 +765,11 @@ func Test_createRun(t *testing.T) { wantStdout: "https://github.com/OWNER/REPO\n", }, { - name: "noninteractive create bare from source", + name: "noninteractive create bare from source and push", opts: &CreateOptions{ Interactive: false, Source: ".", + Push: true, Name: "REPO", Visibility: "PRIVATE", }, @@ -771,23 +778,24 @@ func Test_createRun(t *testing.T) { reg.Register( httpmock.GraphQL(`mutation RepositoryCreate\b`), httpmock.StringResponse(` - { - "data": { - "createRepository": { - "repository": { - "id": "REPOID", - "name": "REPO", - "owner": {"login":"OWNER"}, - "url": "https://github.com/OWNER/REPO" + { + "data": { + "createRepository": { + "repository": { + "id": "REPOID", + "name": "REPO", + "owner": {"login":"OWNER"}, + "url": "https://github.com/OWNER/REPO" + } } } - } - }`)) + }`)) }, execStubs: func(cs *run.CommandStubber) { cs.Register(`git -C . rev-parse --git-dir`, 0, ".") cs.Register(`git -C . rev-parse HEAD`, 0, "commithash") cs.Register(`git -C . remote add origin https://github.com/OWNER/REPO`, 0, "") + cs.Register(`git -C . push origin --mirror`, 0, "") }, wantStdout: "https://github.com/OWNER/REPO\n", }, From 8e63268aba2c643771c6427c31abce21ad4fe95a Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 11 Nov 2024 16:10:32 +0100 Subject: [PATCH 73/76] Doc push behaviour for bare repo create --- pkg/cmd/repo/create/create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index ac4ef39c9..de99c4bc3 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -94,7 +94,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co To create a remote repository from an existing local repository, specify the source directory with %[1]s--source%[1]s. By default, the remote repository name will be the name of the source directory. - Pass %[1]s--push%[1]s to push any local commits to the new repository. + Pass %[1]s--push%[1]s to push any local commits to the new repository. If the repo is bare, this will mirror all refs. For language or platform .gitignore templates to use with %[1]s--gitignore%[1]s, . From 7bcb0633914e635692fe7a1e31d96d713bf48392 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 11 Nov 2024 16:17:06 +0100 Subject: [PATCH 74/76] Modify push prompt on repo create when bare --- pkg/cmd/repo/create/create.go | 7 ++++++- pkg/cmd/repo/create/create_test.go | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index de99c4bc3..79c349aa4 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -652,8 +652,13 @@ func createFromLocal(opts *CreateOptions) error { // don't prompt for push if there are no commits if opts.Interactive && committed { + msg := fmt.Sprintf("Would you like to push commits from the current branch to %q?", baseRemote) + if repoType == bare { + msg = fmt.Sprintf("Would you like to mirror all refs to %q?", baseRemote) + } + var err error - opts.Push, err = opts.Prompter.Confirm(fmt.Sprintf("Would you like to push commits from the current branch to %q?", baseRemote), true) + opts.Push, err = opts.Prompter.Confirm(msg, true) if err != nil { return err } diff --git a/pkg/cmd/repo/create/create_test.go b/pkg/cmd/repo/create/create_test.go index a0f9ac207..c33cfdad6 100644 --- a/pkg/cmd/repo/create/create_test.go +++ b/pkg/cmd/repo/create/create_test.go @@ -452,7 +452,7 @@ func Test_createRun(t *testing.T) { switch message { case "Add a remote?": return true, nil - case `Would you like to push commits from the current branch to "origin"?`: + case `Would you like to mirror all refs to "origin"?`: return true, nil default: return false, fmt.Errorf("unexpected confirm prompt: %s", message) From b8ef951de1cff45e56c1847bd0a2ed08e9173e60 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 13 Nov 2024 13:04:01 -0500 Subject: [PATCH 75/76] Shorten extension release checking from 3s to 1s Addressing feedback from extension author demonstration about a noticable pause waiting for extension execution to complete due to amount of time waiting on channel. --- pkg/cmd/root/extension.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/root/extension.go b/pkg/cmd/root/extension.go index fcf0549c2..52250a432 100644 --- a/pkg/cmd/root/extension.go +++ b/pkg/cmd/root/extension.go @@ -72,7 +72,7 @@ func NewCmdExtension(io *iostreams.IOStreams, em extensions.ExtensionManager, ex fmt.Fprintf(stderr, "%s\n\n", cs.Yellow(releaseInfo.URL)) } - case <-time.After(3 * time.Second): + case <-time.After(1 * time.Second): // Bail on checking for new extension update as its taking too long } }, From d4262f818386ba4eea60ebdbb1951bf95c284a9f Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Thu, 14 Nov 2024 10:31:36 -0500 Subject: [PATCH 76/76] Mention GitHub CLI team on discussion issues --- .github/workflows/triage.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/triage.yml b/.github/workflows/triage.yml index 6cd9d981d..849beebad 100644 --- a/.github/workflows/triage.yml +++ b/.github/workflows/triage.yml @@ -35,6 +35,8 @@ jobs: --- + cc: @github/cli + > $BODY EOF @@ -63,5 +65,7 @@ jobs: --- + cc: @github/cli + > $BODY EOF