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 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 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' 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/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/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= diff --git a/internal/codespaces/rpc/invoker.go b/internal/codespaces/rpc/invoker.go index b9d321802..6ba8843ac 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_][-.a-zA-Z0-9_]*$` + re := regexp.MustCompile(validUsernamePattern) + return re.MatchString(username) +} 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/extensions.go b/pkg/cmd/attestation/verification/extensions.go index 94ba88208..e302d89c9 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 ( @@ -11,72 +13,49 @@ 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 + var lastErr error for _, attestation := range results { - if err := verifyCertExtensions(attestation, tenant, owner, repo, issuer); err != nil { - return err + err := verifyCertExtensions(*attestation.VerificationResult.Signature.Certificate, ec) + 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 } - atLeastOneVerified = true + lastErr = err } - if atLeastOneVerified { - return nil - } else { - return ErrNoAttestationsVerified - } + // if we have exited the for loop without returning early due to successful + // verification, we need to return an error + return lastErr } -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(verifiedCert certificate.Summary, criteria EnforcementCriteria) error { + sourceRepositoryOwnerURI := verifiedCert.Extensions.SourceRepositoryOwnerURI + if !strings.EqualFold(criteria.Certificate.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) { + return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", criteria.Certificate.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) } // 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) - } - - 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 criteria.Certificate.SourceRepositoryURI != "" { + sourceRepositoryURI := verifiedCert.Extensions.SourceRepositoryURI + if !strings.EqualFold(criteria.Certificate.SourceRepositoryURI, sourceRepositoryURI) { + return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", criteria.Certificate.SourceRepositoryURI, sourceRepositoryURI) } } // if 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) + 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 diff --git a/pkg/cmd/attestation/verification/extensions_test.go b/pkg/cmd/attestation/verification/extensions_test.go index 5eb28829d..b8ef2875f 100644 --- a/pkg/cmd/attestation/verification/extensions_test.go +++ b/pkg/cmd/attestation/verification/extensions_test.go @@ -8,143 +8,83 @@ 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", }, }, }, }, } +} - t.Run("VerifyCertExtensions with owner and repo", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "owner/repo", GitHubOIDCIssuer) +func TestVerifyCertExtensions(t *testing.T) { + results := []*AttestationProcessingResult{createSampleResult()} + + certSummary := certificate.Summary{} + certSummary.SourceRepositoryOwnerURI = "https://github.com/owner" + certSummary.SourceRepositoryURI = "https://github.com/owner/repo" + certSummary.Issuer = GitHubOIDCIssuer + + c := EnforcementCriteria{ + Certificate: certSummary, + } + + t.Run("passes with one result", 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) { - 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") - }) + 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" - t.Run("VerifyCertExtensions with owner", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "", GitHubOIDCIssuer) + err := VerifyCertExtensions(twoResults, c) require.NoError(t, err) }) - t.Run("VerifyCertExtensions with wrong owner", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "wrong", "", 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, c) + require.Error(t, err) + }) + + t.Run("with wrong SourceRepositoryOwnerURI", func(t *testing.T) { + expectedCriteria := c + 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("VerifyCertExtensions with wrong repo", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "wrong", GitHubOIDCIssuer) - require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/wrong, got https://github.com/owner/repo") + t.Run("with wrong SourceRepositoryURI", func(t *testing.T) { + expectedCriteria := c + 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("VerifyCertExtensions with wrong issuer", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "", "wrong") + t.Run("with wrong OIDCIssuer", func(t *testing.T) { + expectedCriteria := c + expectedCriteria.Certificate.Issuer = "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", - }, - }, - }, - }, - }, - } - - t.Run("VerifyCertExtensions with exact issuer match", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "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 OIDCIssuer match", func(t *testing.T) { + 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") }) - - t.Run("VerifyCertExtensions with wrong issuer", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "owner", "", "wrong") - 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{ - 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) - require.NoError(t, err) - }) - - t.Run("VerifyCertExtensions with owner and repo, no tenant", func(t *testing.T) { - err := VerifyCertExtensions(results, "", "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) - 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) - require.NoError(t, err) - }) - - t.Run("VerifyCertExtensions with wrong owner", func(t *testing.T) { - err := VerifyCertExtensions(results, "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) - 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") - require.NoError(t, err) - }) - - t.Run("VerifyCertExtensions with wrong issuer", func(t *testing.T) { - err := VerifyCertExtensions(results, "foo", "owner", "owner/repo", "wrong") - require.ErrorContains(t, err, "expected Issuer to be wrong, got https://token.actions.foo.ghe.com") - }) } 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/policy.go b/pkg/cmd/attestation/verification/policy.go index 91b54c75e..f5f4010aa 100644 --- a/pkg/cmd/attestation/verification/policy.go +++ b/pkg/cmd/attestation/verification/policy.go @@ -2,12 +2,17 @@ package verification import ( "encoding/hex" + "fmt" "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" + "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" "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 // from the given artifact digest and digest algorithm func BuildDigestPolicyOption(a artifact.DigestedArtifact) (verify.ArtifactPolicyOption, error) { @@ -18,3 +23,29 @@ func BuildDigestPolicyOption(a artifact.DigestedArtifact) (verify.ArtifactPolicy } return verify.WithArtifactDigest(a.Algorithm(), decoded), nil } + +type EnforcementCriteria struct { + Certificate certificate.Summary + PredicateType string + 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") + } + if c.SANRegex == "" && c.SAN == "" { + return fmt.Errorf("SANRegex or SAN must be set") + } + return nil +} diff --git a/pkg/cmd/attestation/verification/sigstore.go b/pkg/cmd/attestation/verification/sigstore.go index 5b4f4a79b..66005d62e 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,61 +167,68 @@ 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 { - // 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) { + if len(attestations) == 0 { + return nil, ErrNoAttestationsVerified + } + + results := make([]*AttestationProcessingResult, len(attestations)) + var verifyCount int + var lastError error 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 &SigstoreResults{ - Error: fmt.Errorf("failed to find recognized issuer from bundle content: %v", err), - } + lastError = err + // move onto the next attestation in the for loop 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 &SigstoreResults{ - Error: 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 + // otherwise, add the result to the results slice and increment verifyCount + results[verifyCount] = apr + verifyCount++ } - if atLeastOneVerified { - return &SigstoreResults{ - VerifyResults: apResults, - } - } else { - return &SigstoreResults{Error: ErrNoAttestationsVerified} + if verifyCount == 0 { + return nil, lastError } + + // truncate the results slice to only include verified attestations + results = results[:verifyCount] + + 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 b7057505e..e14b472b0 100644 --- a/pkg/cmd/attestation/verification/sigstore_integration_test.go +++ b/pkg/cmd/attestation/verification/sigstore_integration_test.go @@ -52,18 +52,49 @@ 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]) + require.Len(t, attestations, 3) + + results, err := verifier.Verify(attestations, publicGoodPolicy(t)) + + 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") @@ -77,9 +108,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 +121,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/policy.go b/pkg/cmd/attestation/verify/policy.go index f41b2f66b..c2e154fe2 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -12,11 +12,7 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/verification" ) -const ( - // represents the GitHub hosted runner in the certificate RunnerEnvironment extension - GitHubRunner = "github-hosted" - 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 == "" { @@ -25,26 +21,72 @@ func expandToGitHubURL(tenant, ownerOrRepo string) string { return fmt.Sprintf("(?i)^https://%s.ghe.com/%s/", tenant, ownerOrRepo) } -func buildSANMatcher(opts *Options) (verify.SubjectAlternativeNameMatcher, error) { +func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, error) { + var c verification.EnforcementCriteria + + // Set SANRegex using either the opts.SignerRepo or opts.SignerWorkflow values if opts.SignerRepo != "" { signedRepoRegex := expandToGitHubURL(opts.Tenant, opts.SignerRepo) - return verify.NewSANMatcher("", signedRepoRegex) + c.SANRegex = signedRepoRegex } else if opts.SignerWorkflow != "" { validatedWorkflowRegex, err := validateSignerWorkflow(opts) if err != nil { - return verify.SubjectAlternativeNameMatcher{}, err + return verification.EnforcementCriteria{}, err } - return verify.NewSANMatcher("", validatedWorkflowRegex) - } else if opts.SAN != "" || opts.SANRegex != "" { - return verify.NewSANMatcher(opts.SAN, opts.SANRegex) + 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 } - return verify.SubjectAlternativeNameMatcher{}, nil + // 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 { + // 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.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 { + c.Certificate.SourceRepositoryURI = fmt.Sprintf("https://github.com/%s", opts.Repo) + } + } + + // 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 { + c.Certificate.SourceRepositoryOwnerURI = fmt.Sprintf("https://github.com/%s", opts.Owner) + } + + // 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 { + // otherwise use the custom OIDC issuer provided as an option + c.Certificate.Issuer = opts.OIDCIssuer + } + + c.PredicateType = opts.PredicateType + + return c, nil } -func buildCertificateIdentityOption(opts *Options, runnerEnv string) (verify.PolicyOption, error) { - sanMatcher, err := buildSANMatcher(opts) +func buildCertificateIdentityOption(c verification.EnforcementCriteria) (verify.PolicyOption, error) { + sanMatcher, err := verify.NewSANMatcher(c.SAN, c.SANRegex) if err != nil { return nil, err } @@ -56,7 +98,7 @@ func buildCertificateIdentityOption(opts *Options, runnerEnv string) (verify.Pol } extensions := certificate.Extensions{ - RunnerEnvironment: runnerEnv, + RunnerEnvironment: c.Certificate.RunnerEnvironment, } certId, err := verify.NewCertificateIdentity(sanMatcher, issuerMatcher, extensions) @@ -67,34 +109,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) { +func buildSigstoreVerifyPolicy(c verification.EnforcementCriteria, a artifact.DigestedArtifact) (verify.PolicyBuilder, error) { artifactDigestPolicyOption, err := verification.BuildDigestPolicyOption(a) if err != nil { return verify.PolicyBuilder{}, err } - certIdOption, err := buildVerifyCertIdOption(opts) + certIdOption, err := buildCertificateIdentityOption(c) if err != nil { return verify.PolicyBuilder{}, err } @@ -103,10 +124,6 @@ func buildVerifyPolicy(opts *Options, a artifact.DigestedArtifact) (verify.Polic 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 @@ -116,12 +133,14 @@ func validateSignerWorkflow(opts *Options) (string, error) { } if match { - return addSchemeToRegex(opts.SignerWorkflow), nil + 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") } - return addSchemeToRegex(fmt.Sprintf("%s/%s", opts.Hostname, opts.SignerWorkflow)), nil + return fmt.Sprintf("^https://%s/%s", opts.Hostname, opts.SignerWorkflow), nil } diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index 39e5d3707..420c57f3a 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -3,31 +3,162 @@ package verify import ( "testing" - "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" - "github.com/cli/cli/v2/pkg/cmd/attestation/artifact/oci" + "github.com/cli/cli/v2/pkg/cmd/attestation/verification" "github.com/cli/cli/v2/pkg/cmd/factory" "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) - require.NoError(t, err) + t.Run("sets SANRegex using SignerRepo", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "foo/bar", + SignerRepo: "foo/bar", + } - opts := &Options{ - ArtifactPath: artifactPath, - Owner: "sigstore", - SANRegex: "^https://github.com/sigstore/", - } + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + require.Equal(t, "(?i)^https://github.com/foo/bar/", c.SANRegex) + require.Zero(t, c.SAN) + }) - _, err = buildVerifyPolicy(opts, *artifact) - require.NoError(t, err) + t.Run("sets SANRegex using SignerWorkflow matching host regex", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "foo/bar", + SignerWorkflow: "foo/bar/.github/workflows/attest.yml", + Hostname: "github.com", + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + 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) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "foo/bar", + SAN: "https://github/foo/bar/.github/workflows/attest.yml", + SANRegex: "(?i)^https://github/foo", + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + 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) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "foo/bar", + DenySelfHostedRunner: true, + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + require.Equal(t, verification.GitHubRunner, c.Certificate.RunnerEnvironment) + }) + + t.Run("sets Extensions.RunnerEnvironment to * value if opts.DenySelfHostedRunner is false", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "foo/bar", + DenySelfHostedRunner: false, + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + require.Zero(t, c.Certificate.RunnerEnvironment) + }) + + t.Run("sets Extensions.SourceRepositoryURI using opts.Repo and opts.Tenant", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "foo/bar", + Tenant: "baz", + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + require.Equal(t, "https://baz.ghe.com/foo/bar", c.Certificate.SourceRepositoryURI) + }) + + t.Run("sets Extensions.SourceRepositoryURI using opts.Repo", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "foo/bar", + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + 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) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "foo/bar", + Tenant: "baz", + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + require.Equal(t, "https://baz.ghe.com/foo", c.Certificate.SourceRepositoryOwnerURI) + }) + + t.Run("sets Extensions.SourceRepositoryOwnerURI using opts.Owner", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "foo/bar", + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + require.Equal(t, "https://github.com/foo", c.Certificate.SourceRepositoryOwnerURI) + }) + + t.Run("sets OIDCIssuer using opts.Tenant", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "foo/bar", + Tenant: "baz", + OIDCIssuer: verification.GitHubOIDCIssuer, + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + require.Equal(t, "https://token.actions.baz.ghe.com", c.Certificate.Issuer) + }) + + t.Run("sets OIDCIssuer using opts.OIDCIssuer", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "foo/bar", + OIDCIssuer: "https://foo.com", + Tenant: "baz", + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + require.Equal(t, "https://foo.com", c.Certificate.Issuer) + }) } func TestValidateSignerWorkflow(t *testing.T) { diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 206001f9b..47b52bb30 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) @@ -249,30 +260,30 @@ 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 } attestations = filteredAttestations - policy, err := buildVerifyPolicy(opts, *artifact) + opts.Logger.VerbosePrintf("Verifying attestations with predicate type: %s\n", ec.PredicateType) + + sp, err := buildSigstoreVerifyPolicy(ec, *artifact) if err != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build verification policy")) + opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build Sigstore verification policy")) return err } - opts.Logger.VerbosePrintf("Verifying attestations with predicate type: %s\n", opts.PredicateType) - - sigstoreRes := opts.SigstoreVerifier.Verify(attestations, policy) - if sigstoreRes.Error != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Verification failed")) - return sigstoreRes.Error + verifyResults, err := opts.SigstoreVerifier.Verify(attestations, sp) + if err != nil { + opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Sigstore verification failed")) + return err } // 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")) + if err := verification.VerifyCertExtensions(verifyResults, ec); err != nil { + opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Policy verification failed")) return err } @@ -281,7 +292,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, verifyResults); err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to write JSON output")) return err } @@ -291,7 +302,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, verifyResults) if err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("failed to parse results")) return err 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/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 diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index c6c3943e2..b79ec68c9 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,8 +693,8 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts sshOptions) (err erro return status } -func automaticPrivateKeyPath() (string, error) { - sshDir, err := config.HomeDirPath(".ssh") +func automaticPrivateKeyPath(sshContext ssh.Context) (string, error) { + sshDir, err := sshContext.SshDir() if err != nil { return "", err } diff --git a/pkg/cmd/codespace/ssh_test.go b/pkg/cmd/codespace/ssh_test.go index b76d304d7..cd04f05fa 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" @@ -68,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)) @@ -125,6 +124,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 +142,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"}, }, { @@ -202,7 +205,7 @@ 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)) @@ -226,7 +229,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 +262,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/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/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) { diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 1b82c33f5..128c51068 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, ",")) } + // 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) + } 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 diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index 2fc127aba..79c349aa4 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, . @@ -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") } @@ -652,22 +652,43 @@ 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 } } - 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,22 +757,34 @@ func hasCommits(gitClient *git.Client) (bool, error) { return false, nil } -// check if path is the top level directory of a git repo -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 *exec.ExitError + 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" { - return false, nil + + switch projectDir { + case ".": + return bare, nil + case ".git": + return working, nil + default: + return unknown, nil } - return true, 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..c33cfdad6 100644 --- a/pkg/cmd/repo/create/create_test.go +++ b/pkg/cmd/repo/create/create_test.go @@ -443,6 +443,74 @@ 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 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 true, nil + case `Would you like to mirror all refs to "origin"?`: + return true, 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 + case "What should the new remote be called?": + return defaultValue, 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") + 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✓ 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", opts: &CreateOptions{Interactive: true}, @@ -696,6 +764,71 @@ func Test_createRun(t *testing.T) { }, wantStdout: "https://github.com/OWNER/REPO\n", }, + { + name: "noninteractive create bare from source and push", + opts: &CreateOptions{ + Interactive: false, + Source: ".", + Push: true, + 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, "") + cs.Register(`git -C . push origin --mirror`, 0, "") + }, + 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{ @@ -856,11 +989,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()) }) diff --git a/pkg/cmd/root/extension.go b/pkg/cmd/root/extension.go index d6d495103..52250a432 100644 --- a/pkg/cmd/root/extension.go +++ b/pkg/cmd/root/extension.go @@ -4,6 +4,8 @@ import ( "errors" "fmt" "os/exec" + "strings" + "time" "github.com/cli/cli/v2/pkg/extensions" "github.com/cli/cli/v2/pkg/iostreams" @@ -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 { + updateMessageChan := make(chan *extensionReleaseInfo) + cs := io.ColorScheme() + 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,28 @@ 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) { + 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)) + } + case <-time.After(1 * time.Second): + // Bail on checking for new extension update as its taking too long + } + }, GroupID: "extension", Annotations: map[string]string{ "skipAuthCheck": "true", diff --git a/pkg/cmd/root/extension_test.go b/pkg/cmd/root/extension_test.go new file mode 100644 index 000000000..ef94dcc71 --- /dev/null +++ b/pkg/cmd/root/extension_test.go @@ -0,0 +1,159 @@ +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", + extURL: "https//github.com/dne/no-update", + }, + { + name: "major update", + extName: "major-update", + 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 + `), + }, + { + name: "major update, pinned", + extName: "major-update", + extUpdateAvailable: true, + 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 + `), + }, + { + name: "minor update", + extName: "minor-update", + 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 + `), + }, + { + name: "minor update, pinned", + extName: "minor-update", + 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 + `), + }, + { + name: "patch update", + extName: "patch-update", + 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 + `), + }, + { + name: "patch update, pinned", + extName: "patch-update", + 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 + `), + }, + } + + 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") + } + } +} 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", }, } diff --git a/pkg/ssh/ssh_keys.go b/pkg/ssh/ssh_keys.go index c750b608a..83d4f1b34 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 inject test-specific setup. +func NewContextForTests(configDir, keygenExe string) Context { + return Context{ + configDir, + keygenExe, + } } type KeyPair struct { @@ -26,7 +35,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 +54,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,20 +88,20 @@ func (c *Context) GenerateSSHKey(keyName string, passphrase string) (*KeyPair, e return &keyPair, nil } -func (c *Context) sshDir() (string, error) { - if c.ConfigDir != "" { - return c.ConfigDir, nil +func (c *Context) SshDir() (string, error) { + 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 }