From 36d622ed48d8c68d471529d1433534bd8f2f1dd2 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 20 Aug 2024 23:42:57 -0400 Subject: [PATCH 01/26] 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 00000000000..4a9083e92f6 --- /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 00000000000..1769d5bb32b --- /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/26] 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 1769d5bb32b..e1308f6eec9 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/26] 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 e1308f6eec9..3b3b6eccd77 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/26] 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 3b3b6eccd77..c6aa1f15125 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/26] 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 c6aa1f15125..ba88e3cd36c 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/26] 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 ba88e3cd36c..68fefacf64c 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/26] 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 68fefacf64c..62bf7b8ddbe 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/26] 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 62bf7b8ddbe..479349714da 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 01f63c5cc3bb48b28b92d6cf10059e994080ee30 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 10:08:05 -0600 Subject: [PATCH 09/26] 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 c139a5af271..31afb7ce6b4 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 e22142ed565..41332dc6204 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 5b4f4a79b0c..3c05ecf858f 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 206001f9be2..75b9bce1b70 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 10/26] 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 94ba88208d6..046f245093f 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 5eb28829d65..44523465265 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 11/26] 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 046f245093f..274e833c46e 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 44523465265..a03cf79d623 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 12/26] 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 a03cf79d623..87249cb7baf 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 b7057505ef7..f1d0729a682 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 13/26] 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 3c05ecf858f..1577ff663a8 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 f1d0729a682..e14b472b0fb 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 4bd46334ffc227df43f4208a6c9e4cf73d4731e6 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 31 Oct 2024 12:38:37 -0600 Subject: [PATCH 14/26] 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 274e833c46e..4c748e3e957 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 1577ff663a8..825f9da1c00 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 15/26] 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 31afb7ce6b4..c139a5af271 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 41332dc6204..e22142ed565 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 825f9da1c00..34904988f44 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 e14b472b0fb..84dd1695a89 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 75b9bce1b70..206001f9be2 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 47d77bd51b589ccc1a6d588ea691401db92899a4 Mon Sep 17 00:00:00 2001 From: Andrew Feller Date: Sat, 2 Nov 2024 13:14:05 -0400 Subject: [PATCH 16/26] 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 d6d4951030b..89f0085f36f 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 b65c942e1f9fc7e00f0ee4b75604bc05980acb3f Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Wed, 6 Nov 2024 09:45:03 -0700 Subject: [PATCH 17/26] 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 825f9da1c00..86000405b77 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 18/26] 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 86000405b77..66005d62e69 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 6d5a26cfd194e261e11015506c357d175fa4946a Mon Sep 17 00:00:00 2001 From: Sarah Barili Date: Wed, 6 Nov 2024 14:45:41 -0700 Subject: [PATCH 19/26] 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 b9d3218027a..e00b6c3043c 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 ddf7287ab893b7d672e089e0eaffebf40f03471c Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 6 Nov 2024 22:49:02 -0500 Subject: [PATCH 20/26] 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 89f0085f36f..fcf0549c234 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 00000000000..e756bf504e7 --- /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 21/26] 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 4a9083e92f6..00000000000 --- 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 22/26] 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 479349714da..00000000000 --- 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 23/26] 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 e756bf504e7..ef94dcc715b 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 24/26] 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 e00b6c3043c..6ba8843acaa 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 b8ef951de1cff45e56c1847bd0a2ed08e9173e60 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 13 Nov 2024 13:04:01 -0500 Subject: [PATCH 25/26] 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 fcf0549c234..52250a43204 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 26/26] 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 6cd9d981d10..849beebada4 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