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/README.md b/README.md index 1ddeb1dfe..064008e3b 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ ![screenshot of gh pr status](https://user-images.githubusercontent.com/98482/84171218-327e7a80-aa40-11ea-8cd1-5177fc2d0e72.png) -GitHub CLI is supported for users on GitHub.com and GitHub Enterprise Server 2.20+ with support for macOS, Windows, and Linux. +GitHub CLI is supported for users on GitHub.com, GitHub Enterprise Cloud, and GitHub Enterprise Server 2.20+ with support for macOS, Windows, and Linux. ## Documentation diff --git a/acceptance/README.md b/acceptance/README.md index 750cb75d1..8e8d7838e 100644 --- a/acceptance/README.md +++ b/acceptance/README.md @@ -159,7 +159,9 @@ When tests fail they fail like this: This is generally enough information to understand why a test has failed. However, we can get more information by providing the `-v` flag to `go test`, which turns on verbose mode and shows each command and any associated `stdio`. > [!WARNING] -> Verbose mode dumps the `testscript` environment variables, including the `GH_TOKEN`, so be careful. +> Verbose mode dumps the `testscript` environment variables, so make sure there is nothing sensitive in there. +> We have taken steps to [redact tokens](https://github.com/cli/cli/pull/9804) in log output but there's no +> guarantee it's comprehensive. By default `testscript` removes the directory in which it was running the script, and if you've been a conscientious engineer, you should be cleaning up resources using the `defer` statement. However, this can be an impediment to debugging. As such you can set `GH_ACCEPTANCE_PRESERVE_WORK_DIR=true` and `GH_ACCEPTANCE_SKIP_DEFER=true` to skip these cleanup steps. diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index bdd631703..b22929ed5 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -14,7 +14,7 @@ import ( "math/rand" "github.com/cli/cli/v2/internal/ghcmd" - "github.com/rogpeppe/go-internal/testscript" + "github.com/cli/go-internal/testscript" ) func ghMain() int { @@ -27,13 +27,40 @@ func TestMain(m *testing.M) { })) } -func TestPullRequests(t *testing.T) { +func TestAPI(t *testing.T) { var tsEnv testScriptEnv if err := tsEnv.fromEnv(); err != nil { t.Fatal(err) } - testscript.Run(t, testScriptParamsFor(tsEnv, "pr")) + testscript.Run(t, testScriptParamsFor(tsEnv, "api")) +} + +func TestAuth(t *testing.T) { + var tsEnv testScriptEnv + if err := tsEnv.fromEnv(); err != nil { + t.Fatal(err) + } + + testscript.Run(t, testScriptParamsFor(tsEnv, "auth")) +} + +func TestGPGKeys(t *testing.T) { + var tsEnv testScriptEnv + if err := tsEnv.fromEnv(); err != nil { + t.Fatal(err) + } + + testscript.Run(t, testScriptParamsFor(tsEnv, "gpg-key")) +} + +func TestExtensions(t *testing.T) { + var tsEnv testScriptEnv + if err := tsEnv.fromEnv(); err != nil { + t.Fatal(err) + } + + testscript.Run(t, testScriptParamsFor(tsEnv, "extension")) } func TestIssues(t *testing.T) { @@ -45,22 +72,40 @@ func TestIssues(t *testing.T) { testscript.Run(t, testScriptParamsFor(tsEnv, "pr")) } -func TestWorkflows(t *testing.T) { +func TestLabels(t *testing.T) { var tsEnv testScriptEnv if err := tsEnv.fromEnv(); err != nil { t.Fatal(err) } - testscript.Run(t, testScriptParamsFor(tsEnv, "workflow")) + testscript.Run(t, testScriptParamsFor(tsEnv, "label")) } -func TestAPI(t *testing.T) { +func TestOrg(t *testing.T) { var tsEnv testScriptEnv if err := tsEnv.fromEnv(); err != nil { t.Fatal(err) } - testscript.Run(t, testScriptParamsFor(tsEnv, "api")) + testscript.Run(t, testScriptParamsFor(tsEnv, "org")) +} + +func TestProject(t *testing.T) { + var tsEnv testScriptEnv + if err := tsEnv.fromEnv(); err != nil { + t.Fatal(err) + } + + testscript.Run(t, testScriptParamsFor(tsEnv, "project")) +} + +func TestPullRequests(t *testing.T) { + var tsEnv testScriptEnv + if err := tsEnv.fromEnv(); err != nil { + t.Fatal(err) + } + + testscript.Run(t, testScriptParamsFor(tsEnv, "pr")) } func TestReleases(t *testing.T) { @@ -72,6 +117,24 @@ func TestReleases(t *testing.T) { testscript.Run(t, testScriptParamsFor(tsEnv, "release")) } +func TestRepo(t *testing.T) { + var tsEnv testScriptEnv + if err := tsEnv.fromEnv(); err != nil { + t.Fatal(err) + } + + testscript.Run(t, testScriptParamsFor(tsEnv, "repo")) +} + +func TestRulesets(t *testing.T) { + var tsEnv testScriptEnv + if err := tsEnv.fromEnv(); err != nil { + t.Fatal(err) + } + + testscript.Run(t, testScriptParamsFor(tsEnv, "ruleset")) +} + func TestSearches(t *testing.T) { var tsEnv testScriptEnv if err := tsEnv.fromEnv(); err != nil { @@ -81,15 +144,6 @@ func TestSearches(t *testing.T) { testscript.Run(t, testScriptParamsFor(tsEnv, "search")) } -func TestRepo(t *testing.T) { - var tsEnv testScriptEnv - if err := tsEnv.fromEnv(); err != nil { - t.Fatal(err) - } - - testscript.Run(t, testScriptParamsFor(tsEnv, "repo")) -} - func TestSecrets(t *testing.T) { var tsEnv testScriptEnv if err := tsEnv.fromEnv(); err != nil { @@ -99,6 +153,15 @@ func TestSecrets(t *testing.T) { testscript.Run(t, testScriptParamsFor(tsEnv, "secret")) } +func TestSSHKeys(t *testing.T) { + var tsEnv testScriptEnv + if err := tsEnv.fromEnv(); err != nil { + t.Fatal(err) + } + + testscript.Run(t, testScriptParamsFor(tsEnv, "ssh-key")) +} + func TestVariables(t *testing.T) { var tsEnv testScriptEnv if err := tsEnv.fromEnv(); err != nil { @@ -108,6 +171,15 @@ func TestVariables(t *testing.T) { testscript.Run(t, testScriptParamsFor(tsEnv, "variable")) } +func TestWorkflows(t *testing.T) { + var tsEnv testScriptEnv + if err := tsEnv.fromEnv(); err != nil { + t.Fatal(err) + } + + testscript.Run(t, testScriptParamsFor(tsEnv, "workflow")) +} + func testScriptParamsFor(tsEnv testScriptEnv, command string) testscript.Params { var files []string if tsEnv.script != "" { diff --git a/acceptance/testdata/auth/auth-login-logout.txtar b/acceptance/testdata/auth/auth-login-logout.txtar new file mode 100644 index 000000000..fb25f9eb5 --- /dev/null +++ b/acceptance/testdata/auth/auth-login-logout.txtar @@ -0,0 +1,25 @@ +# We aren't logged in at the moment, but GH_TOKEN will override the +# need to login. We are going to clear GH_TOKEN first to ensure no +# overrides are happening + +# Copy $GH_TOKEN to a new env var +env LOGIN_TOKEN=$GH_TOKEN + +# Remove GH_TOKEN env var so we don't fall back to it +env GH_TOKEN='' + +# Login to the host by feeding the token to stdin +exec echo $LOGIN_TOKEN +stdin stdout +exec gh auth login --hostname=$GH_HOST --with-token --insecure-storage + +# Check that we are logged in +exec gh auth status --hostname $GH_HOST +stdout $GH_HOST + +# Logout of the host +exec gh auth logout --hostname $GH_HOST +stderr 'Logged out of' + +# Check that we are logged out +! exec gh auth status --hostname $GH_HOST diff --git a/acceptance/testdata/auth/auth-setup-git.txtar b/acceptance/testdata/auth/auth-setup-git.txtar new file mode 100644 index 000000000..e3be28cd5 --- /dev/null +++ b/acceptance/testdata/auth/auth-setup-git.txtar @@ -0,0 +1,10 @@ +# Check that the credential helper is unset for the host. This command is +# expected to fail before gh auth setup-git is run. +! exec git config --get credential.https://${GH_HOST}.helper + +# Run the setup-git command +exec gh auth setup-git + +# Check that the credential helper is set to gh +exec git config --get credential.https://${GH_HOST}.helper +stdout '^.*gh auth git-credential$' diff --git a/acceptance/testdata/auth/auth-status.txtar b/acceptance/testdata/auth/auth-status.txtar new file mode 100644 index 000000000..2afee1eb6 --- /dev/null +++ b/acceptance/testdata/auth/auth-status.txtar @@ -0,0 +1,3 @@ +# Check the authentication status +exec gh auth status --hostname $GH_HOST +stdout '✓ Logged in to ' \ No newline at end of file diff --git a/acceptance/testdata/auth/auth-token.txtar b/acceptance/testdata/auth/auth-token.txtar new file mode 100644 index 000000000..614d11817 --- /dev/null +++ b/acceptance/testdata/auth/auth-token.txtar @@ -0,0 +1,3 @@ +# Check authentication token +exec gh auth token --hostname $GH_HOST +stdout $GH_TOKEN \ No newline at end of file diff --git a/acceptance/testdata/extension/extension.txtar b/acceptance/testdata/extension/extension.txtar new file mode 100644 index 000000000..a4d194757 --- /dev/null +++ b/acceptance/testdata/extension/extension.txtar @@ -0,0 +1,69 @@ +# Skip if Bash is not available given script extension +[!exec:bash] skip + +# Setup environment variables used for testscript +env EXT_NAME=${SCRIPT_NAME}-${RANDOM_STRING} +env EXT_SCRIPT=gh-${EXT_NAME} +env REPO=gh-${EXT_NAME} + +# Use gh as a credential helper +exec gh auth setup-git + +# Create local repository for extension +exec gh extension create $EXT_NAME +cd $REPO + +# Setup v1 executable baseline for extension +mv ../v1.sh $EXT_SCRIPT +chmod 777 $EXT_SCRIPT +exec git add $EXT_SCRIPT +exec git commit -m 'Setup extension as v1' + +# Upload local extension repository +exec gh repo create $ORG/$REPO --private --source . --push + +# Defer repo cleanup +defer gh repo delete --yes $ORG/$REPO + +# Verify extension shows up in search, sleep for indexing +exec gh repo edit --add-topic gh-extension +sleep 10 +exec gh extension search --owner $ORG $EXT_NAME +stdout ${ORG}/${REPO} + +# Verify repository can be installed as extension +exec gh extension install $ORG/$REPO +exec gh extension list +stdout ${ORG}/${REPO} + +# Verify v1 extension behavior before upgrade +exec gh extension exec $EXT_NAME +stdout 'gh ext create v1' + +# Setup v2 executable upgrade for extension +mv ../v2.sh $EXT_SCRIPT +chmod 777 $EXT_SCRIPT +exec git add $EXT_SCRIPT +exec git commit -m 'Upgrade extension to v2' +exec git push -u origin + +# Verify v2 extension upgrade +exec gh extension upgrade $EXT_NAME +exec gh extension exec $EXT_NAME +stdout 'gh ext upgrade v2' + +# Verify extension can be removed +exec gh extension remove $EXT_NAME +! stdout ${ORG}/${REPO} + +-- v1.sh -- +#!/usr/bin/env bash +set -e + +echo "gh ext create v1" + +-- v2.sh -- +#!/usr/bin/env bash +set -e + +echo "gh ext upgrade v2" diff --git a/acceptance/testdata/gpg-key/gpg-key.txtar b/acceptance/testdata/gpg-key/gpg-key.txtar new file mode 100644 index 000000000..8f0d71545 --- /dev/null +++ b/acceptance/testdata/gpg-key/gpg-key.txtar @@ -0,0 +1,36 @@ +skip 'it modifies the user''s personal GitHub account GPG keys' + +# This test requires the admin:gpg_key scope to add and delete GPG keys to and +# from the user's personal GitHub account. +# This test uses a GPG key that generated for this test only. The private key +# has been deleted + +# Add the gpg key to GH account +exec gh gpg-key add gpg-key.pub + +# Verify the gpg key was added to GH account +exec gh gpg-key list +stdout '24C30F9C9115E747' + +# Delete the gpg key from GH account +exec gh gpg-key delete --yes '24C30F9C9115E747' + +# Check the key is deleted +exec gh gpg-key list +! stdout '24C30F9C9115E747' + +-- gpg-key.pub -- +-----BEGIN PGP PUBLIC KEY BLOCK----- + +mDMEZxpWhhYJKwYBBAHaRw8BAQdAmYiobR2ai/lVWOBtlAPRG1ZEMG5Effavpt5w +n+wQ//W0R0dIIENMSSBhY2NlcHRhbmNlIHRlc3QgKGZvciBHSCBDTEkgYWNjZXB0 +YW5jZSB0ZXN0aW5nKSA8Y2xpQGdpdGh1Yi5jb20+iJkEExYKAEEWIQTEAQLLUl1x +MDSmbL0kww+ckRXnRwUCZxpWhgIbAwUJAAFRgAULCQgHAgIiAgYVCgkICwIEFgID +AQIeBwIXgAAKCRAkww+ckRXnRxkuAP9GiFi/etWxRjnkomdTaOU8Ccd6oHspuEzB +PFxOJdYslQD+MXgY5UhM/q2iEVj0tiVsfRzDqB+g2weaF5EpqIwWcQ+4OARnGlaG +EgorBgEEAZdVAQUBAQdA3D1vnVTc9URDQw/oAd1mG/zRX7vF4QrjFqFIt7uMf2gD +AQgHiH4EGBYKACYWIQTEAQLLUl1xMDSmbL0kww+ckRXnRwUCZxpWhgIbDAUJAAFR +gAAKCRAkww+ckRXnRxVuAQCngnR11jh2mob0FN0rPWce2juoJsh5gPB2d7LS4r5P +VwEA6F2FeetcP51EyKyQGTp3GpmZgk0uCGJa1G5uqT+9mgc= +=RLWi +-----END PGP PUBLIC KEY BLOCK----- \ No newline at end of file diff --git a/acceptance/testdata/label/label.txtar b/acceptance/testdata/label/label.txtar new file mode 100644 index 000000000..dd72133da --- /dev/null +++ b/acceptance/testdata/label/label.txtar @@ -0,0 +1,25 @@ +# Setup useful env vars +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Create a repository +exec gh repo create ${ORG}/${REPO} --private + +# Defer repo cleanup +defer gh repo delete --yes ${ORG}/${REPO} + +# Set the GH_REPO env var to reduce redunant flags +env GH_REPO=${ORG}/${REPO} + +# Create a custom label +exec gh label create 'acceptance-test' --description 'First Description' + +# List the labels and check our custom label is there +exec gh label list +stdout 'acceptance-test\tFirst Description' + +# Edit the label +exec gh label edit 'acceptance-test' --description 'Edited Description' + +# List the labels and check our custom label has been updated +exec gh label list +stdout 'acceptance-test\tEdited Description' diff --git a/acceptance/testdata/org/org-list.txtar b/acceptance/testdata/org/org-list.txtar new file mode 100644 index 000000000..ca114babe --- /dev/null +++ b/acceptance/testdata/org/org-list.txtar @@ -0,0 +1,6 @@ +# This test could fail if the user is a member of more than 30 organizations because +# the `gh org list` command only returns the first 30 organizations the user is a member of + +# List organizations the user is a member of +exec gh org list +stdout ${GH_ACCEPTANCE_ORG} \ No newline at end of file diff --git a/acceptance/testdata/project/project-create-delete.txtar b/acceptance/testdata/project/project-create-delete.txtar new file mode 100644 index 000000000..115253669 --- /dev/null +++ b/acceptance/testdata/project/project-create-delete.txtar @@ -0,0 +1,13 @@ +# Create a project and get the project number +env PROJECT_TITLE=$SCRIPT_NAME-$RANDOM_STRING +exec gh project create --owner=$ORG --title=$PROJECT_TITLE --format='json' --jq='.number' +stdout2env PROJECT_NUMBER + +# Confirm the project has been created +exec gh project view --owner=$ORG $PROJECT_NUMBER + +# Delete the project +exec gh project delete --owner=$ORG $PROJECT_NUMBER + +# Confirm the project has been deleted +! exec gh project view --owner=$ORG $PROJECT_NUMBER \ No newline at end of file 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/ruleset/ruleset.txtar b/acceptance/testdata/ruleset/ruleset.txtar new file mode 100644 index 000000000..99be3683c --- /dev/null +++ b/acceptance/testdata/ruleset/ruleset.txtar @@ -0,0 +1,62 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository with a file so it has a default branch +exec gh repo create $ORG/$REPO --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes $ORG/$REPO + +# Clone the repo +exec gh repo clone $ORG/$REPO +cd $REPO + +# Verify repository ruleset does not exist +env LIST_MATCH=testscript\s+$ORG/$REPO (repo) +exec gh ruleset list +! stdout $LIST_MATCH + +# Verify no repository ruleset applies to default branch +exec gh ruleset check +stdout '0 rules apply' + +# Create a repository ruleset +exec gh api /repos/{owner}/{repo}/rulesets -X POST --input ../create-repo-ruleset.json + +# Verify repository ruleset does exist +exec gh ruleset list +stdout $LIST_MATCH + +# Verify repository ruleset associated with branch +exec gh ruleset check +stdout '- pull_request:.+dismiss_stale_reviews_on_push: false.+require_code_owner_review: true.+require_last_push_approval: false.+required_approving_review_count: 1.+required_review_thread_resolution: false' + +-- create-repo-ruleset.json -- +{ + "name": "testscript", + "target": "branch", + "enforcement": "active", + "conditions": { + "ref_name": { + "include": [ + "~DEFAULT_BRANCH" + ], + "exclude": [] + } + }, + "rules": [ + { + "type": "pull_request", + "parameters": { + "dismiss_stale_reviews_on_push": false, + "require_code_owner_review": true, + "require_last_push_approval": false, + "required_approving_review_count": 1, + "required_review_thread_resolution": false + } + } + ] +} diff --git a/acceptance/testdata/ssh-key/ssh-key.txtar b/acceptance/testdata/ssh-key/ssh-key.txtar new file mode 100644 index 000000000..4ba8643bb --- /dev/null +++ b/acceptance/testdata/ssh-key/ssh-key.txtar @@ -0,0 +1,24 @@ +skip 'it modifies the user''s personal GitHub account SSH keys' + +# scopes admin:ssh_signing_key,admin:public_key + +# Add an SSH key to the account +exec gh ssh-key add sshKey.pub --title 'acceptance-test-key' + +# List the SSH keys +exec gh ssh-key list +stdout 'acceptance-test-key' + +# Get the ID of the key we created +exec gh api /user/keys --jq '.[] | select(.title == "acceptance-test-key") | .id' +stdout2env SSH_KEY_ID + +# Delete the SSH key +exec gh ssh-key delete --yes ${SSH_KEY_ID} + +# Check the key is deleted +exec gh ssh-key list +! stdout 'acceptance-test-key' + +-- sshKey.pub -- +ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAZmdeRNskfpvYL5YHB/YJaW8hTEXpnvPMkx5Ri+YwUr acceptance 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/docs/install_linux.md b/docs/install_linux.md index 9be6aed9b..5943bba24 100644 --- a/docs/install_linux.md +++ b/docs/install_linux.md @@ -33,39 +33,37 @@ sudo apt install gh > [!NOTE] > If errors regarding GPG signatures occur, see [cli/cli#9569](https://github.com/cli/cli/issues/9569) for steps to fix this. -### Fedora, CentOS, Red Hat Enterprise Linux (dnf) +### Fedora, CentOS, Red Hat Enterprise Linux (dnf5) Install from our package repository for immediate access to latest releases: ```bash -sudo dnf install 'dnf-command(config-manager)' -sudo dnf config-manager --add-repo https://cli.github.com/packages/rpm/gh-cli.repo +sudo dnf install dnf5-plugins +sudo dnf config-manager addrepo --from-repofile=https://cli.github.com/packages/rpm/gh-cli.repo sudo dnf install gh --repo gh-cli ``` -
-Show dnf5 commands +These commands apply for `dnf5`. If you're using `dnf4`, commands will vary slightly. -If you're using `dnf5`, commands will vary slightly: +
+Show dnf4 commands ```bash -sudo dnf5 install dnf5-plugins -sudo dnf5 config-manager addrepo --from-repofile=https://cli.github.com/packages/rpm/gh-cli.repo -sudo dnf5 install gh --repo gh-cli +sudo dnf4 install 'dnf-command(config-manager)' +sudo dnf4 config-manager --add-repo https://cli.github.com/packages/rpm/gh-cli.repo +sudo dnf4 install gh --repo gh-cli ``` - -For more details, check out the [`dnf5 config-manager` documentation](https://dnf5.readthedocs.io/en/latest/dnf5_plugins/config-manager.8.html).
+> [!NOTE] +> If errors regarding GPG signatures occur, see [cli/cli#9569](https://github.com/cli/cli/issues/9569) for steps to fix this. + Alternatively, install from the [community repository](https://packages.fedoraproject.org/pkgs/gh/gh/): ```bash sudo dnf install gh ``` -> [!NOTE] -> If errors regarding GPG signatures occur, see [cli/cli#9569](https://github.com/cli/cli/issues/9569) for steps to fix this. - Upgrade: ```bash diff --git a/go.mod b/go.mod index 76a7818f8..40193c4f8 100644 --- a/go.mod +++ b/go.mod @@ -12,10 +12,11 @@ require ( github.com/charmbracelet/glamour v0.7.0 github.com/charmbracelet/lipgloss v0.10.1-0.20240413172830-d0be07ea6b9c github.com/cli/go-gh/v2 v2.11.0 + github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24 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 @@ -125,7 +126,6 @@ require ( github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/rodaine/table v1.0.1 // indirect - github.com/rogpeppe/go-internal v1.13.1 github.com/russross/blackfriday/v2 v2.1.0 // indirect github.com/sagikazarmark/locafero v0.4.0 // indirect github.com/sagikazarmark/slog-shim v0.1.0 // indirect @@ -158,10 +158,10 @@ require ( go.uber.org/multierr v1.11.0 // indirect go.uber.org/zap v1.27.0 // indirect golang.org/x/exp v0.0.0-20240112132812-db7319d0e0e3 // indirect - golang.org/x/mod v0.20.0 // indirect + golang.org/x/mod v0.21.0 // indirect golang.org/x/net v0.30.0 // indirect golang.org/x/sys v0.26.0 // indirect - golang.org/x/tools v0.22.0 // indirect + golang.org/x/tools v0.26.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20240520151616-dc85e6b867a5 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect gopkg.in/ini.v1 v1.67.0 // indirect diff --git a/go.sum b/go.sum index 848747498..d068d9d11 100644 --- a/go.sum +++ b/go.sum @@ -97,6 +97,8 @@ github.com/cli/browser v1.3.0 h1:LejqCrpWr+1pRqmEPDGnTZOjsMe7sehifLynZJuqJpo= github.com/cli/browser v1.3.0/go.mod h1:HH8s+fOAxjhQoBUAsKuPCbqUuxZDhQ2/aD+SzsEfBTk= github.com/cli/go-gh/v2 v2.11.0 h1:TERLYMMWderKBO3lBff/JIu2+eSly2oFRgN2WvO+3eA= github.com/cli/go-gh/v2 v2.11.0/go.mod h1:MeRoKzXff3ygHu7zP+NVTT+imcHW6p3tpuxHAzRM2xE= +github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24 h1:QDrhR4JA2n3ij9YQN0u5ZeuvRIIvsUGmf5yPlTS0w8E= +github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24/go.mod h1:rr9GNING0onuVw8MnracQHn7PcchnFlP882Y0II2KZk= github.com/cli/oauth v1.1.1 h1:459gD3hSjlKX9B1uXBuiAMdpXBUQ9QGf/NDcCpoQxPs= github.com/cli/oauth v1.1.1/go.mod h1:qd/FX8ZBD6n1sVNQO3aIdRxeu5LGw9WhKnYhIIoC2A4= github.com/cli/safeexec v1.0.0/go.mod h1:Z/D4tTN8Vs5gXYHDCbaM1S/anmEDnJb1iW0+EJ5zx3Q= @@ -113,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= @@ -366,8 +368,8 @@ github.com/rivo/uniseg v0.4.7 h1:WUdvkW8uEhrYfLC4ZzdpI2ztxP1I582+49Oc5Mq64VQ= github.com/rivo/uniseg v0.4.7/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88= github.com/rodaine/table v1.0.1 h1:U/VwCnUxlVYxw8+NJiLIuCxA/xa6jL38MY3FYysVWWQ= github.com/rodaine/table v1.0.1/go.mod h1:UVEtfBsflpeEcD56nF4F5AocNFta0ZuolpSVdPtlmP4= -github.com/rogpeppe/go-internal v1.13.1 h1:KvO1DLK/DRN07sQ1LQKScxyZJuNnedQ5/wKSR38lUII= -github.com/rogpeppe/go-internal v1.13.1/go.mod h1:uMEvuHeurkdAXX61udpOXGD/AzZDWNMNyH2VO9fmH0o= +github.com/rogpeppe/go-internal v1.11.0 h1:cWPaGQEPrBb5/AsnsZesgZZ9yb1OQ+GOISoDNXVBh4M= +github.com/rogpeppe/go-internal v1.11.0/go.mod h1:ddIwULY96R17DhadqLgMfk9H9tvdUzkipdSkR5nkCZA= github.com/russross/blackfriday/v2 v2.1.0 h1:JIOH55/0cWyOuilr9/qlrm0BSXldqnqwMsf35Ld67mk= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/ryanuber/go-glob v1.0.0 h1:iQh3xXAumdQ+4Ufa5b25cRpC5TYKlno6hsv6Cb3pkBk= @@ -491,8 +493,8 @@ golang.org/x/crypto v0.28.0/go.mod h1:rmgy+3RHxRZMyY0jjAJShp2zgEdOqj2AO7U0pYmeQ7 golang.org/x/exp v0.0.0-20240112132812-db7319d0e0e3 h1:hNQpMuAJe5CtcUqCXaWga3FHu+kQvCqcsoVaQgSV60o= golang.org/x/exp v0.0.0-20240112132812-db7319d0e0e3/go.mod h1:idGWGoKP1toJGkd5/ig9ZLuPcZBC3ewk7SzmH0uou08= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= -golang.org/x/mod v0.20.0 h1:utOm6MM3R3dnawAiJgn0y+xvuYRsm1RKM/4giyfDgV0= -golang.org/x/mod v0.20.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= +golang.org/x/mod v0.21.0 h1:vvrHzRwRfVKSiLrG+d4FMl/Qi4ukBCE6kZlTUkDYRT0= +golang.org/x/mod v0.21.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c= @@ -533,8 +535,8 @@ golang.org/x/time v0.5.0/go.mod h1:3BpzKBy/shNhVucY/MWOyx10tF3SFh9QdLuxbVysPQM= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc= -golang.org/x/tools v0.22.0 h1:gqSGLZqv+AI9lIQzniJ0nZDRG5GBPsSi+DRNHWNz6yA= -golang.org/x/tools v0.22.0/go.mod h1:aCwcsjqvq7Yqt6TNyX7QMU2enbQ/Gt0bo6krSeEri+c= +golang.org/x/tools v0.26.0 h1:v/60pFQmzmT9ExmjDv2gGIfi3OqfKoEP6I5+umXlbnQ= +golang.org/x/tools v0.26.0/go.mod h1:TPVVj70c7JJ3WCazhD8OdXcZg/og+b9+tH/KxylGwH0= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= google.golang.org/api v0.172.0 h1:/1OcMZGPmW1rX2LCu2CmGUD1KXK1+pfzxotxyRUCCdk= google.golang.org/api v0.172.0/go.mod h1:+fJZq6QXWfa9pXhnIzsjx4yI22d4aI9ZpLb58gvXjis= 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/api/client.go b/pkg/cmd/attestation/api/client.go index 460ae3aad..f47b5f759 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -1,11 +1,14 @@ package api import ( + "errors" "fmt" "io" "net/http" "strings" + "time" + "github.com/cenkalti/backoff/v4" "github.com/cli/cli/v2/api" ioconfig "github.com/cli/cli/v2/pkg/cmd/attestation/io" ) @@ -69,6 +72,9 @@ func (c *LiveClient) GetTrustDomain() (string, error) { return c.getTrustDomain(MetaPath) } +// Allow injecting backoff interval in tests. +var getAttestationRetryInterval = time.Millisecond * 200 + func (c *LiveClient) getAttestations(url, name, digest string, limit int) ([]*Attestation, error) { c.logger.VerbosePrintf("Fetching attestations for artifact digest %s\n\n", digest) @@ -86,15 +92,31 @@ func (c *LiveClient) getAttestations(url, name, digest string, limit int) ([]*At var attestations []*Attestation var resp AttestationsResponse - var err error + bo := backoff.NewConstantBackOff(getAttestationRetryInterval) + // if no attestation or less than limit, then keep fetching for url != "" && len(attestations) < limit { - url, err = c.api.RESTWithNext(c.host, http.MethodGet, url, nil, &resp) + err := backoff.Retry(func() error { + newURL, restErr := c.api.RESTWithNext(c.host, http.MethodGet, url, nil, &resp) + + if restErr != nil { + if shouldRetry(restErr) { + return restErr + } else { + return backoff.Permanent(restErr) + } + } + + url = newURL + attestations = append(attestations, resp.Attestations...) + + return nil + }, backoff.WithMaxRetries(bo, 3)) + + // bail if RESTWithNext errored out if err != nil { return nil, err } - - attestations = append(attestations, resp.Attestations...) } if len(attestations) == 0 { @@ -108,10 +130,34 @@ func (c *LiveClient) getAttestations(url, name, digest string, limit int) ([]*At return attestations, nil } +func shouldRetry(err error) bool { + var httpError api.HTTPError + if errors.As(err, &httpError) { + if httpError.StatusCode >= 500 && httpError.StatusCode <= 599 { + return true + } + } + + return false +} + func (c *LiveClient) getTrustDomain(url string) (string, error) { var resp MetaResponse - err := c.api.REST(c.host, http.MethodGet, url, nil, &resp) + bo := backoff.NewConstantBackOff(getAttestationRetryInterval) + err := backoff.Retry(func() error { + restErr := c.api.REST(c.host, http.MethodGet, url, nil, &resp) + if restErr != nil { + if shouldRetry(restErr) { + return restErr + } else { + return backoff.Permanent(restErr) + } + } + + return nil + }, backoff.WithMaxRetries(bo, 3)) + if err != nil { return "", err } diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index bfcb40f5a..adac00598 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -204,3 +204,62 @@ func TestGetTrustDomain(t *testing.T) { }) } + +func TestGetAttestationsRetries(t *testing.T) { + getAttestationRetryInterval = 0 + + fetcher := mockDataGenerator{ + NumAttestations: 5, + } + + c := &LiveClient{ + api: mockAPIClient{ + OnRESTWithNext: fetcher.FlakyOnRESTSuccessWithNextPageHandler(), + }, + logger: io.NewTestHandler(), + } + + attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, DefaultLimit) + require.NoError(t, err) + + // assert the error path was executed; because this is a paged + // request, it should have errored twice + fetcher.AssertNumberOfCalls(t, "FlakyOnRESTSuccessWithNextPage:error", 2) + + // but we still successfully got the right data + require.Equal(t, len(attestations), 10) + bundle := (attestations)[0].Bundle + require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") + + // same test as above, but for GetByOwnerAndDigest: + attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, DefaultLimit) + require.NoError(t, err) + + // because we haven't reset the mock, we have added 2 more failed requests + fetcher.AssertNumberOfCalls(t, "FlakyOnRESTSuccessWithNextPage:error", 4) + + require.Equal(t, len(attestations), 10) + bundle = (attestations)[0].Bundle + require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") +} + +// test total retries +func TestGetAttestationsMaxRetries(t *testing.T) { + getAttestationRetryInterval = 0 + + fetcher := mockDataGenerator{ + NumAttestations: 5, + } + + c := &LiveClient{ + api: mockAPIClient{ + OnRESTWithNext: fetcher.OnREST500ErrorHandler(), + }, + logger: io.NewTestHandler(), + } + + _, err := c.GetByRepoAndDigest(testRepo, testDigest, DefaultLimit) + require.Error(t, err) + + fetcher.AssertNumberOfCalls(t, "OnREST500Error", 4) +} diff --git a/pkg/cmd/attestation/api/mock_apiClient_test.go b/pkg/cmd/attestation/api/mock_apiClient_test.go index d58cdbc79..e1654bd3f 100644 --- a/pkg/cmd/attestation/api/mock_apiClient_test.go +++ b/pkg/cmd/attestation/api/mock_apiClient_test.go @@ -6,6 +6,10 @@ import ( "fmt" "io" "strings" + + cliAPI "github.com/cli/cli/v2/api" + ghAPI "github.com/cli/go-gh/v2/pkg/api" + "github.com/stretchr/testify/mock" ) type mockAPIClient struct { @@ -22,14 +26,15 @@ func (m mockAPIClient) REST(hostname, method, p string, body io.Reader, data int } type mockDataGenerator struct { + mock.Mock NumAttestations int } -func (m mockDataGenerator) OnRESTSuccess(hostname, method, p string, body io.Reader, data interface{}) (string, error) { +func (m *mockDataGenerator) OnRESTSuccess(hostname, method, p string, body io.Reader, data interface{}) (string, error) { return m.OnRESTWithNextSuccessHelper(hostname, method, p, body, data, false) } -func (m mockDataGenerator) OnRESTSuccessWithNextPage(hostname, method, p string, body io.Reader, data interface{}) (string, error) { +func (m *mockDataGenerator) OnRESTSuccessWithNextPage(hostname, method, p string, body io.Reader, data interface{}) (string, error) { // if path doesn't contain after, it means first time hitting the mock server // so return the first page and return the link header in the response if !strings.Contains(p, "after") { @@ -40,7 +45,37 @@ func (m mockDataGenerator) OnRESTSuccessWithNextPage(hostname, method, p string, return m.OnRESTWithNextSuccessHelper(hostname, method, p, body, data, false) } -func (m mockDataGenerator) OnRESTWithNextSuccessHelper(hostname, method, p string, body io.Reader, data interface{}, hasNext bool) (string, error) { +// Returns a func that just calls OnRESTSuccessWithNextPage but half the time +// it returns a 500 error. +func (m *mockDataGenerator) FlakyOnRESTSuccessWithNextPageHandler() func(hostname, method, p string, body io.Reader, data interface{}) (string, error) { + // set up the flake counter + m.On("FlakyOnRESTSuccessWithNextPage:error").Return() + + count := 0 + return func(hostname, method, p string, body io.Reader, data interface{}) (string, error) { + if count%2 == 0 { + m.MethodCalled("FlakyOnRESTSuccessWithNextPage:error") + + count = count + 1 + return "", cliAPI.HTTPError{HTTPError: &ghAPI.HTTPError{StatusCode: 500}} + } else { + count = count + 1 + return m.OnRESTSuccessWithNextPage(hostname, method, p, body, data) + } + } +} + +// always returns a 500 +func (m *mockDataGenerator) OnREST500ErrorHandler() func(hostname, method, p string, body io.Reader, data interface{}) (string, error) { + m.On("OnREST500Error").Return() + return func(hostname, method, p string, body io.Reader, data interface{}) (string, error) { + m.MethodCalled("OnREST500Error") + + return "", cliAPI.HTTPError{HTTPError: &ghAPI.HTTPError{StatusCode: 500}} + } +} + +func (m *mockDataGenerator) OnRESTWithNextSuccessHelper(hostname, method, p string, body io.Reader, data interface{}, hasNext bool) (string, error) { atts := make([]*Attestation, m.NumAttestations) for j := 0; j < m.NumAttestations; j++ { att := makeTestAttestation() @@ -70,7 +105,7 @@ func (m mockDataGenerator) OnRESTWithNextSuccessHelper(hostname, method, p strin return "", nil } -func (m mockDataGenerator) OnRESTWithNextNoAttestations(hostname, method, p string, body io.Reader, data interface{}) (string, error) { +func (m *mockDataGenerator) OnRESTWithNextNoAttestations(hostname, method, p string, body io.Reader, data interface{}) (string, error) { resp := AttestationsResponse{ Attestations: make([]*Attestation, 0), } @@ -89,7 +124,7 @@ func (m mockDataGenerator) OnRESTWithNextNoAttestations(hostname, method, p stri return "", nil } -func (m mockDataGenerator) OnRESTWithNextError(hostname, method, p string, body io.Reader, data interface{}) (string, error) { +func (m *mockDataGenerator) OnRESTWithNextError(hostname, method, p string, body io.Reader, data interface{}) (string, error) { return "", errors.New("failed to get attestations") } diff --git a/pkg/cmd/attestation/download/download.go b/pkg/cmd/attestation/download/download.go index 77c928093..143912308 100644 --- a/pkg/cmd/attestation/download/download.go +++ b/pkg/cmd/attestation/download/download.go @@ -122,14 +122,13 @@ func runDownload(opts *Options) error { opts.Logger.VerbosePrintf("Downloading trusted metadata for artifact %s\n\n", opts.ArtifactPath) - c := verification.FetchAttestationsConfig{ - APIClient: opts.APIClient, - Digest: artifact.DigestWithAlg(), - Limit: opts.Limit, - Owner: opts.Owner, - Repo: opts.Repo, + params := verification.FetchRemoteAttestationsParams{ + Digest: artifact.DigestWithAlg(), + Limit: opts.Limit, + Owner: opts.Owner, + Repo: opts.Repo, } - attestations, err := verification.GetRemoteAttestations(c) + attestations, err := verification.GetRemoteAttestations(opts.APIClient, params) if err != nil { if errors.Is(err, api.ErrNoAttestations{}) { fmt.Fprintf(opts.Logger.IO.Out, "No attestations found for %s\n", opts.ArtifactPath) diff --git a/pkg/cmd/attestation/inspect/inspect.go b/pkg/cmd/attestation/inspect/inspect.go index f0fae8961..6eb571d66 100644 --- a/pkg/cmd/attestation/inspect/inspect.go +++ b/pkg/cmd/attestation/inspect/inspect.go @@ -167,8 +167,8 @@ func runInspect(opts *Options) error { for _, a := range attestations { inspectedBundle := BundleInspection{} - sigstoreRes := opts.SigstoreVerifier.Verify([]*api.Attestation{a}, sigstorePolicy) - if sigstoreRes.Error == nil { + _, err := opts.SigstoreVerifier.Verify([]*api.Attestation{a}, sigstorePolicy) + if err == nil { inspectedBundle.Authentic = true } diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index 50542a6b3..07083a5c0 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -9,41 +9,22 @@ import ( "path/filepath" "github.com/cli/cli/v2/pkg/cmd/attestation/api" + "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" "github.com/cli/cli/v2/pkg/cmd/attestation/artifact/oci" - "github.com/google/go-containerregistry/pkg/name" protobundle "github.com/sigstore/protobuf-specs/gen/pb-go/bundle/v1" "github.com/sigstore/sigstore-go/pkg/bundle" ) +const SLSAPredicateV1 = "https://slsa.dev/provenance/v1" + var ErrUnrecognisedBundleExtension = errors.New("bundle file extension not supported, must be json or jsonl") var ErrEmptyBundleFile = errors.New("provided bundle file is empty") -type FetchAttestationsConfig struct { - APIClient api.Client - BundlePath string - Digest string - Limit int - Owner string - Repo string - OCIClient oci.Client - UseBundleFromRegistry bool - NameRef name.Reference -} - -func (c *FetchAttestationsConfig) IsBundleProvided() bool { - return c.BundlePath != "" -} - -func GetAttestations(c FetchAttestationsConfig) ([]*api.Attestation, error) { - if c.IsBundleProvided() { - return GetLocalAttestations(c.BundlePath) - } - - if c.UseBundleFromRegistry { - return GetOCIAttestations(c) - } - - return GetRemoteAttestations(c) +type FetchRemoteAttestationsParams struct { + Digest string + Limit int + Owner string + Repo string } // GetLocalAttestations returns a slice of attestations read from a local bundle file. @@ -114,30 +95,30 @@ func loadBundlesFromJSONLinesFile(path string) ([]*api.Attestation, error) { return attestations, nil } -func GetRemoteAttestations(c FetchAttestationsConfig) ([]*api.Attestation, error) { - if c.APIClient == nil { +func GetRemoteAttestations(client api.Client, params FetchRemoteAttestationsParams) ([]*api.Attestation, error) { + if client == nil { return nil, fmt.Errorf("api client must be provided") } // check if Repo is set first because if Repo has been set, Owner will be set using the value of Repo. // If Repo is not set, the field will remain empty. It will not be populated using the value of Owner. - if c.Repo != "" { - attestations, err := c.APIClient.GetByRepoAndDigest(c.Repo, c.Digest, c.Limit) + if params.Repo != "" { + attestations, err := client.GetByRepoAndDigest(params.Repo, params.Digest, params.Limit) if err != nil { - return nil, fmt.Errorf("failed to fetch attestations from %s: %w", c.Repo, err) + return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Repo, err) } return attestations, nil - } else if c.Owner != "" { - attestations, err := c.APIClient.GetByOwnerAndDigest(c.Owner, c.Digest, c.Limit) + } else if params.Owner != "" { + attestations, err := client.GetByOwnerAndDigest(params.Owner, params.Digest, params.Limit) if err != nil { - return nil, fmt.Errorf("failed to fetch attestations from %s: %w", c.Owner, err) + return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Owner, err) } return attestations, nil } return nil, fmt.Errorf("owner or repo must be provided") } -func GetOCIAttestations(c FetchAttestationsConfig) ([]*api.Attestation, error) { - attestations, err := c.OCIClient.GetAttestations(c.NameRef, c.Digest) +func GetOCIAttestations(client oci.Client, artifact artifact.DigestedArtifact) ([]*api.Attestation, error) { + attestations, err := client.GetAttestations(artifact.NameRef(), artifact.Digest()) if err != nil { return nil, fmt.Errorf("failed to fetch OCI attestations: %w", err) } diff --git a/pkg/cmd/attestation/verification/extensions.go b/pkg/cmd/attestation/verification/extensions.go index 94ba88208..a0827e9ec 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,44 @@ 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.Certificate) + if err == nil { + // if at least one attestation is verified, we're good as verification + // is defined as successful if at least one attestation is verified + 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(given, expected certificate.Summary) error { + if !strings.EqualFold(expected.SourceRepositoryOwnerURI, given.SourceRepositoryOwnerURI) { + return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", expected.SourceRepositoryOwnerURI, given.SourceRepositoryOwnerURI) } - // if repo is set, 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 repo is set, compare the SourceRepositoryURI fields + if expected.SourceRepositoryURI != "" && !strings.EqualFold(expected.SourceRepositoryURI, given.SourceRepositoryURI) { + return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", expected.SourceRepositoryURI, given.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) + // compare the OIDC issuers. If not equal, return an error depending + // on if there is a partial match + if !strings.EqualFold(expected.Issuer, given.Issuer) { + if strings.Index(given.Issuer, expected.Issuer+"/") == 0 { + return fmt.Errorf("expected Issuer to be %s, got %s -- if you have a custom OIDC issuer policy for your enterprise, use the --cert-oidc-issuer flag with your expected issuer", expected.Issuer, given.Issuer) } + return fmt.Errorf("expected Issuer to be %s, got %s", expected.Issuer, given.Issuer) } 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 cb3a4c061..41332dc62 100644 --- a/pkg/cmd/attestation/verification/mock_verifier.go +++ b/pkg/cmd/attestation/verification/mock_verifier.go @@ -12,15 +12,13 @@ import ( "github.com/sigstore/sigstore-go/pkg/verify" ) -const SLSAPredicateType = "https://slsa.dev/provenance/v1" - type MockSigstoreVerifier struct { t *testing.T } -func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults { +func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { statement := &in_toto.Statement{} - statement.PredicateType = SLSAPredicateType + statement.PredicateType = SLSAPredicateV1 result := AttestationProcessingResult{ Attestation: &api.Attestation{ @@ -43,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 { @@ -54,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 e237a3eb9..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 { @@ -113,7 +108,7 @@ func (v *LiveSigstoreVerifier) chooseVerifier(b *bundle.Bundle) (*verify.SignedE // issuer. We *must* use the trusted root provided. if issuer == PublicGoodIssuerOrg { if v.config.NoPublicGood { - return nil, "", fmt.Errorf("Detected public good instance but requested verification without public good instance") + return nil, "", fmt.Errorf("detected public good instance but requested verification without public good instance") } verifier, err := newPublicGoodVerifierWithTrustedRoot(trustedRoot) if err != nil { @@ -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 1d3ec2d75..e14b472b0 100644 --- a/pkg/cmd/attestation/verification/sigstore_integration_test.go +++ b/pkg/cmd/attestation/verification/sigstore_integration_test.go @@ -15,31 +15,84 @@ import ( ) func TestLiveSigstoreVerifier(t *testing.T) { - t.Run("with invalid signature", func(t *testing.T) { - attestations := getAttestationsFor(t, "../test/data/sigstoreBundle-invalid-signature.json") - require.NotNil(t, attestations) + type testcase struct { + name string + attestations []*api.Attestation + expectErr bool + errContains string + } + testcases := []testcase{ + { + name: "with invalid signature", + attestations: getAttestationsFor(t, "../test/data/sigstoreBundle-invalid-signature.json"), + expectErr: true, + errContains: "verifying with issuer \"sigstore.dev\"", + }, + { + name: "with valid artifact and JSON lines file containing multiple Sigstore bundles", + attestations: getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl"), + }, + { + name: "with invalid bundle version", + attestations: getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json"), + expectErr: true, + errContains: "unsupported bundle version", + }, + { + name: "with no attestations", + attestations: []*api.Attestation{}, + expectErr: true, + errContains: "no attestations were verified", + }, + } + + for _, tc := range testcases { verifier := NewLiveSigstoreVerifier(SigstoreConfig{ Logger: io.NewTestHandler(), }) - res := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Error(t, res.Error) - require.ErrorContains(t, res.Error, "verifying with issuer \"sigstore.dev\"") - require.Nil(t, res.VerifyResults) + results, err := 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) + } else { + 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("with valid artifact and JSON lines file containing multiple Sigstore bundles", func(t *testing.T) { - attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") - require.Len(t, attestations, 2) - + t.Run("fail with 0/2 verified attestations", func(t *testing.T) { verifier := NewLiveSigstoreVerifier(SigstoreConfig{ Logger: io.NewTestHandler(), }) - res := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Len(t, res.VerifyResults, 2) - require.NoError(t, res.Error) + 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) { @@ -55,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) { @@ -68,38 +121,10 @@ 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) }) - - t.Run("with invalid bundle version", func(t *testing.T) { - attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json") - require.Len(t, attestations, 1) - - verifier := NewLiveSigstoreVerifier(SigstoreConfig{ - Logger: io.NewTestHandler(), - }) - - res := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Len(t, res.VerifyResults, 0) - require.ErrorContains(t, res.Error, "unsupported bundle version") - }) - - t.Run("with no attestations", func(t *testing.T) { - attestations := []*api.Attestation{} - require.Len(t, attestations, 0) - - verifier := NewLiveSigstoreVerifier(SigstoreConfig{ - Logger: io.NewTestHandler(), - TrustedRoot: test.NormalizeRelativePath("../test/data/trusted_root.json"), - }) - - res := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Len(t, res.VerifyResults, 0) - require.NotNil(t, res.Error) - }) - } func publicGoodPolicy(t *testing.T) verify.PolicyBuilder { diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go new file mode 100644 index 000000000..f3f2792c4 --- /dev/null +++ b/pkg/cmd/attestation/verify/attestation.go @@ -0,0 +1,50 @@ +package verify + +import ( + "fmt" + + "github.com/cli/cli/v2/internal/text" + "github.com/cli/cli/v2/pkg/cmd/attestation/api" + "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" + "github.com/cli/cli/v2/pkg/cmd/attestation/verification" +) + +func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestation, string, error) { + if o.BundlePath != "" { + attestations, err := verification.GetLocalAttestations(o.BundlePath) + if err != nil { + msg := fmt.Sprintf("✗ Loading attestations from %s failed", a.URL) + return nil, msg, err + } + pluralAttestation := text.Pluralize(len(attestations), "attestation") + msg := fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.BundlePath) + return attestations, msg, nil + } + + if o.UseBundleFromRegistry { + attestations, err := verification.GetOCIAttestations(o.OCIClient, a) + if err != nil { + msg := "✗ Loading attestations from OCI registry failed" + return nil, msg, err + } + pluralAttestation := text.Pluralize(len(attestations), "attestation") + msg := fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.ArtifactPath) + return attestations, msg, nil + } + + params := verification.FetchRemoteAttestationsParams{ + Digest: a.DigestWithAlg(), + Limit: o.Limit, + Owner: o.Owner, + Repo: o.Repo, + } + + attestations, err := verification.GetRemoteAttestations(o.APIClient, params) + if err != nil { + msg := "✗ Loading attestations from GitHub API failed" + return nil, msg, err + } + pluralAttestation := text.Pluralize(len(attestations), "attestation") + msg := fmt.Sprintf("Loaded %s from GitHub API", pluralAttestation) + return attestations, msg, nil +} diff --git a/pkg/cmd/attestation/verify/options_test.go b/pkg/cmd/attestation/verify/options_test.go index b9be054a5..77c0e3b23 100644 --- a/pkg/cmd/attestation/verify/options_test.go +++ b/pkg/cmd/attestation/verify/options_test.go @@ -13,29 +13,28 @@ var ( publicGoodBundlePath = test.NormalizeRelativePath("../test/data/psigstore-js-2.1.0-bundle.json") ) +var baseOptions = Options{ + ArtifactPath: publicGoodArtifactPath, + BundlePath: publicGoodBundlePath, + DigestAlgorithm: "sha512", + Limit: 1, + Owner: "sigstore", + OIDCIssuer: "some issuer", +} + func TestAreFlagsValid(t *testing.T) { t.Run("has invalid Repo value", func(t *testing.T) { - opts := Options{ - ArtifactPath: publicGoodArtifactPath, - DigestAlgorithm: "sha512", - OIDCIssuer: "some issuer", - Repo: "sigstoresigstore-js", - } + opts := baseOptions + opts.Repo = "sigstoresigstore-js" err := opts.AreFlagsValid() require.Error(t, err) require.ErrorContains(t, err, "invalid value provided for repo") }) - t.Run("invalid limit < 0", func(t *testing.T) { - opts := Options{ - ArtifactPath: publicGoodArtifactPath, - BundlePath: publicGoodBundlePath, - DigestAlgorithm: "sha512", - Owner: "sigstore", - OIDCIssuer: "some issuer", - Limit: 0, - } + t.Run("invalid limit == 0", func(t *testing.T) { + opts := baseOptions + opts.Limit = 0 err := opts.AreFlagsValid() require.Error(t, err) @@ -43,19 +42,43 @@ func TestAreFlagsValid(t *testing.T) { }) t.Run("invalid limit > 1000", func(t *testing.T) { - opts := Options{ - ArtifactPath: publicGoodArtifactPath, - BundlePath: publicGoodBundlePath, - DigestAlgorithm: "sha512", - Owner: "sigstore", - OIDCIssuer: "some issuer", - Limit: 1001, - } + opts := baseOptions + opts.Limit = 1001 err := opts.AreFlagsValid() require.Error(t, err) require.ErrorContains(t, err, "limit 1001 not allowed, must be between 1 and 1000") }) + + t.Run("returns error when UseBundleFromRegistry is true and ArtifactPath is not an OCI path", func(t *testing.T) { + opts := baseOptions + opts.BundlePath = "" + opts.UseBundleFromRegistry = true + + err := opts.AreFlagsValid() + require.Error(t, err) + require.ErrorContains(t, err, "bundle-from-oci flag can only be used with OCI artifact paths") + }) + + t.Run("does not return error when UseBundleFromRegistry is true and ArtifactPath is an OCI path", func(t *testing.T) { + opts := baseOptions + opts.ArtifactPath = "oci://sigstore/sigstore-js:2.1.0" + opts.BundlePath = "" + opts.UseBundleFromRegistry = true + + err := opts.AreFlagsValid() + require.NoError(t, err) + }) + + t.Run("returns error when UseBundleFromRegistry is true and BundlePath is provided", func(t *testing.T) { + opts := baseOptions + opts.ArtifactPath = "oci://sigstore/sigstore-js:2.1.0" + opts.UseBundleFromRegistry = true + + err := opts.AreFlagsValid() + require.Error(t, err) + require.ErrorContains(t, err, "bundle-from-oci flag cannot be used with bundle-path flag") + }) } func TestSetPolicyFlags(t *testing.T) { @@ -116,47 +139,4 @@ func TestSetPolicyFlags(t *testing.T) { require.Equal(t, "sigstore", opts.Owner) require.Equal(t, "^https://github/foo", opts.SANRegex) }) - - t.Run("returns error when UseBundleFromRegistry is true and ArtifactPath is not an OCI path", func(t *testing.T) { - opts := Options{ - ArtifactPath: publicGoodArtifactPath, - DigestAlgorithm: "sha512", - Owner: "sigstore", - UseBundleFromRegistry: true, - Limit: 1, - } - - err := opts.AreFlagsValid() - require.Error(t, err) - require.ErrorContains(t, err, "bundle-from-oci flag can only be used with OCI artifact paths") - }) - - t.Run("does not return error when UseBundleFromRegistry is true and ArtifactPath is an OCI path", func(t *testing.T) { - opts := Options{ - ArtifactPath: "oci://sigstore/sigstore-js:2.1.0", - DigestAlgorithm: "sha512", - OIDCIssuer: "some issuer", - Owner: "sigstore", - UseBundleFromRegistry: true, - Limit: 1, - } - - err := opts.AreFlagsValid() - require.NoError(t, err) - }) - - t.Run("returns error when UseBundleFromRegistry is true and BundlePath is provided", func(t *testing.T) { - opts := Options{ - ArtifactPath: "oci://sigstore/sigstore-js:2.1.0", - BundlePath: publicGoodBundlePath, - DigestAlgorithm: "sha512", - Owner: "sigstore", - UseBundleFromRegistry: true, - Limit: 1, - } - - err := opts.AreFlagsValid() - require.Error(t, err) - require.ErrorContains(t, err, "bundle-from-oci flag cannot be used with bundle-path flag") - }) } 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 ae1e52955..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) { @@ -36,18 +167,28 @@ func TestValidateSignerWorkflow(t *testing.T) { providedSignerWorkflow string expectedWorkflowRegex string host string + expectErr bool + errContains string } testcases := []testcase{ { name: "workflow with no host specified", providedSignerWorkflow: "github/artifact-attestations-workflows/.github/workflows/attest.yml", - expectedWorkflowRegex: "^https://github.com/github/artifact-attestations-workflows/.github/workflows/attest.yml", + expectErr: true, + errContains: "unknown host", }, { - name: "workflow with host specified", + name: "workflow with default host", + providedSignerWorkflow: "github/artifact-attestations-workflows/.github/workflows/attest.yml", + expectedWorkflowRegex: "^https://github.com/github/artifact-attestations-workflows/.github/workflows/attest.yml", + host: "github.com", + }, + { + name: "workflow with workflow URL included", providedSignerWorkflow: "github.com/github/artifact-attestations-workflows/.github/workflows/attest.yml", expectedWorkflowRegex: "^https://github.com/github/artifact-attestations-workflows/.github/workflows/attest.yml", + host: "github.com", }, { name: "workflow with GH_HOST set", @@ -61,45 +202,25 @@ func TestValidateSignerWorkflow(t *testing.T) { expectedWorkflowRegex: "^https://authedhost.github.com/github/artifact-attestations-workflows/.github/workflows/attest.yml", host: "authedhost.github.com", }, - { - name: "workflow with authenticated host", - providedSignerWorkflow: "github/artifact-attestations-workflows/.github/workflows/attest.yml", - expectedWorkflowRegex: "^https://authedhost.github.com/github/artifact-attestations-workflows/.github/workflows/attest.yml", - host: "authedhost.github.com", - }, } for _, tc := range testcases { - cmdFactory := factory.New("test") - opts := &Options{ - Config: cmdFactory.Config, + Config: factory.New("test").Config, SignerWorkflow: tc.providedSignerWorkflow, } // All host resolution is done verify.go:RunE - if tc.host == "" { - // Set to default host - tc.host = "github.com" - } opts.Hostname = tc.host - workflowRegex, err := validateSignerWorkflow(opts) - require.NoError(t, err) require.Equal(t, tc.expectedWorkflowRegex, workflowRegex) + if tc.expectErr { + require.Error(t, err) + require.ErrorContains(t, err, tc.errContains) + } else { + require.NoError(t, err) + require.Equal(t, tc.expectedWorkflowRegex, workflowRegex) + } } } - -func TestValidateSignerWorkflowNoHost(t *testing.T) { - cmdFactory := factory.New("test") - opts := &Options{ - Config: cmdFactory.Config, - SignerWorkflow: "github/artifact-attestations-workflows/.github/workflows/attest.yml", - } - - workflowRegex, err := validateSignerWorkflow(opts) - require.Error(t, err) - require.ErrorContains(t, err, "unknown host") - require.Equal(t, "", workflowRegex) -} diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index d14081dd8..1d34fdf99 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -6,7 +6,6 @@ import ( "regexp" "github.com/cli/cli/v2/internal/ghinstance" - "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmd/attestation/api" "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" "github.com/cli/cli/v2/pkg/cmd/attestation/artifact/oci" @@ -52,10 +51,15 @@ func NewVerifyCmd(f *cmdutil.Factory, runF func(*Options) error) *cobra.Command The %[1]s--owner%[1]s flag value must match the name of the GitHub organization that the artifact's linked repository belongs to. - By default, the verify command will attempt to fetch attestations associated - with the provided artifact from the GitHub API. If you would prefer to verify - the artifact using attestations stored on disk (c.f. the %[1]sdownload%[1]s command), - provide a path to the %[1]s--bundle%[1]s flag. + By default, the verify command will: + - only verify provenance attestations + - attempt to fetch relevant attestations via the GitHub API. + + To verify other types of attestations, use the %[1]s--predicate-type%[1]s flag. + + To use your artifact's OCI registry instead of GitHub's API, use the + %[1]s--bundle-from-oci%[1]s flag. For offline verification, using attestations + stored on desk (c.f. the download command), provide a path to the %[1]s--bundle%[1]s flag. To see the full results that are generated upon successful verification, i.e. for use with a policy engine, provide the %[1]s--format=json%[1]s flag. @@ -179,7 +183,7 @@ func NewVerifyCmd(f *cmdutil.Factory, runF func(*Options) error) *cobra.Command verifyCmd.Flags().StringVarP(&opts.Repo, "repo", "R", "", "Repository name in the format /") verifyCmd.MarkFlagsMutuallyExclusive("owner", "repo") verifyCmd.MarkFlagsOneRequired("owner", "repo") - verifyCmd.Flags().StringVarP(&opts.PredicateType, "predicate-type", "", "", "Filter attestations by provided predicate type") + verifyCmd.Flags().StringVarP(&opts.PredicateType, "predicate-type", "", verification.SLSAPredicateV1, "Filter attestations by provided predicate type") verifyCmd.Flags().BoolVarP(&opts.NoPublicGood, "no-public-good", "", false, "Do not verify attestations signed with Sigstore public good instance") verifyCmd.Flags().StringVarP(&opts.TrustedRoot, "custom-trusted-root", "", "", "Path to a trusted_root.jsonl file; likely for offline verification") verifyCmd.Flags().IntVarP(&opts.Limit, "limit", "L", api.DefaultLimit, "Maximum number of attestations to fetch") @@ -198,6 +202,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) @@ -206,70 +221,44 @@ func runVerify(opts *Options) error { opts.Logger.Printf("Loaded digest %s for %s\n", artifact.DigestWithAlg(), artifact.URL) - c := verification.FetchAttestationsConfig{ - APIClient: opts.APIClient, - BundlePath: opts.BundlePath, - Digest: artifact.DigestWithAlg(), - Limit: opts.Limit, - Owner: opts.Owner, - Repo: opts.Repo, - OCIClient: opts.OCIClient, - UseBundleFromRegistry: opts.UseBundleFromRegistry, - NameRef: artifact.NameRef(), - } - attestations, err := verification.GetAttestations(c) + attestations, logMsg, err := getAttestations(opts, *artifact) if err != nil { if ok := errors.Is(err, api.ErrNoAttestations{}); ok { opts.Logger.Printf(opts.Logger.ColorScheme.Red("✗ No attestations found for subject %s\n"), artifact.DigestWithAlg()) return err } - - if c.IsBundleProvided() { - opts.Logger.Printf(opts.Logger.ColorScheme.Red("✗ Loading attestations from %s failed\n"), artifact.URL) - } else if c.UseBundleFromRegistry { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Loading attestations from OCI registry failed")) - } else { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Loading attestations from GitHub API failed")) - } + // Print the message signifying failure fetching attestations + opts.Logger.Println(opts.Logger.ColorScheme.Red(logMsg)) return err } - - pluralAttestation := text.Pluralize(len(attestations), "attestation") - if c.IsBundleProvided() { - opts.Logger.Printf("Loaded %s from %s\n", pluralAttestation, opts.BundlePath) - } else if c.UseBundleFromRegistry { - opts.Logger.Printf("Loaded %s from %s\n", pluralAttestation, opts.ArtifactPath) - } else { - opts.Logger.Printf("Loaded %s from GitHub API\n", pluralAttestation) - } + // Print the message signifying success fetching attestations + opts.Logger.Println(logMsg) // Apply predicate type filter to returned attestations - if opts.PredicateType != "" { - filteredAttestations := verification.FilterAttestations(opts.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 + 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 } - 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 } @@ -278,7 +267,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 } @@ -288,7 +277,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/attestation/verify/verify_integration_test.go b/pkg/cmd/attestation/verify/verify_integration_test.go index 4b0f0adfb..781cb4df1 100644 --- a/pkg/cmd/attestation/verify/verify_integration_test.go +++ b/pkg/cmd/attestation/verify/verify_integration_test.go @@ -40,6 +40,7 @@ func TestVerifyIntegration(t *testing.T) { OCIClient: oci.NewLiveClient(), OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", + PredicateType: verification.SLSAPredicateV1, SANRegex: "^https://github.com/sigstore/", SigstoreVerifier: verification.NewLiveSigstoreVerifier(sigstoreConfig), } @@ -83,6 +84,33 @@ func TestVerifyIntegration(t *testing.T) { require.Error(t, err) require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/fakeowner/fakerepo, got https://github.com/sigstore/sigstore-js") }) + + t.Run("with no matching OIDC issuer", func(t *testing.T) { + opts := publicGoodOpts + opts.OIDCIssuer = "some-other-issuer" + + err := runVerify(&opts) + require.Error(t, err) + require.ErrorContains(t, err, "expected Issuer to be some-other-issuer, got https://token.actions.githubusercontent.com") + }) + + t.Run("with invalid SAN", func(t *testing.T) { + opts := publicGoodOpts + opts.SAN = "fake san" + + err := runVerify(&opts) + require.Error(t, err) + require.ErrorContains(t, err, "verifying with issuer \"sigstore.dev\"") + }) + + t.Run("with invalid SAN regex", func(t *testing.T) { + opts := publicGoodOpts + opts.SANRegex = "^https://github.com/sigstore/not-real/" + + err := runVerify(&opts) + require.Error(t, err) + require.ErrorContains(t, err, "verifying with issuer \"sigstore.dev\"") + }) } func TestVerifyIntegrationCustomIssuer(t *testing.T) { @@ -112,6 +140,7 @@ func TestVerifyIntegrationCustomIssuer(t *testing.T) { Logger: logger, OCIClient: oci.NewLiveClient(), OIDCIssuer: "https://token.actions.githubusercontent.com/hammer-time", + PredicateType: verification.SLSAPredicateV1, SigstoreVerifier: verification.NewLiveSigstoreVerifier(sigstoreConfig), } @@ -181,6 +210,7 @@ func TestVerifyIntegrationReusableWorkflow(t *testing.T) { Logger: logger, OCIClient: oci.NewLiveClient(), OIDCIssuer: verification.GitHubOIDCIssuer, + PredicateType: verification.SLSAPredicateV1, SigstoreVerifier: verification.NewLiveSigstoreVerifier(sigstoreConfig), } @@ -271,6 +301,7 @@ func TestVerifyIntegrationReusableWorkflowSignerWorkflow(t *testing.T) { OCIClient: oci.NewLiveClient(), OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "malancas", + PredicateType: verification.SLSAPredicateV1, Repo: "malancas/attest-demo", SigstoreVerifier: verification.NewLiveSigstoreVerifier(sigstoreConfig), } diff --git a/pkg/cmd/attestation/verify/verify_test.go b/pkg/cmd/attestation/verify/verify_test.go index 306ff8b35..9a2e9f18c 100644 --- a/pkg/cmd/attestation/verify/verify_test.go +++ b/pkg/cmd/attestation/verify/verify_test.go @@ -70,11 +70,12 @@ func TestNewVerifyCmd(t *testing.T) { ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), BundlePath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0-bundle.json"), DigestAlgorithm: "sha384", + Hostname: "github.com", Limit: 30, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", + PredicateType: verification.SLSAPredicateV1, SigstoreVerifier: verification.NewMockSigstoreVerifier(t), - Hostname: "github.com", }, wantsErr: true, }, @@ -85,12 +86,13 @@ func TestNewVerifyCmd(t *testing.T) { ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), BundlePath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0-bundle.json"), DigestAlgorithm: "sha256", + Hostname: "github.com", Limit: 30, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", + PredicateType: verification.SLSAPredicateV1, SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), - Hostname: "github.com", }, wantsErr: false, }, @@ -101,12 +103,13 @@ func TestNewVerifyCmd(t *testing.T) { ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), BundlePath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0-bundle.json"), DigestAlgorithm: "sha256", + Hostname: "foo.ghe.com", Limit: 30, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", + PredicateType: verification.SLSAPredicateV1, SANRegex: "(?i)^https://foo.ghe.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), - Hostname: "foo.ghe.com", }, wantsErr: false, }, @@ -117,12 +120,13 @@ func TestNewVerifyCmd(t *testing.T) { ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), BundlePath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0-bundle.json"), DigestAlgorithm: "sha256", + Hostname: "foo.ghe.com", Limit: 30, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", + PredicateType: verification.SLSAPredicateV1, SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), - Hostname: "foo.ghe.com", }, wantsErr: true, }, @@ -133,12 +137,13 @@ func TestNewVerifyCmd(t *testing.T) { ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), BundlePath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0-bundle.json"), DigestAlgorithm: "sha512", + Hostname: "github.com", Limit: 30, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", + PredicateType: verification.SLSAPredicateV1, SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), - Hostname: "github.com", }, wantsErr: false, }, @@ -148,12 +153,13 @@ func TestNewVerifyCmd(t *testing.T) { wants: Options{ ArtifactPath: test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz"), DigestAlgorithm: "sha256", + Hostname: "github.com", + Limit: 30, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", - Limit: 30, + PredicateType: verification.SLSAPredicateV1, SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), - Hostname: "github.com", }, wantsErr: true, }, @@ -163,12 +169,13 @@ func TestNewVerifyCmd(t *testing.T) { wants: Options{ ArtifactPath: artifactPath, DigestAlgorithm: "sha256", + Hostname: "github.com", + Limit: 30, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", + PredicateType: verification.SLSAPredicateV1, Repo: "sigstore/sigstore-js", - Limit: 30, SigstoreVerifier: verification.NewMockSigstoreVerifier(t), - Hostname: "github.com", }, wantsErr: true, }, @@ -178,12 +185,13 @@ func TestNewVerifyCmd(t *testing.T) { wants: Options{ ArtifactPath: artifactPath, DigestAlgorithm: "sha256", + Hostname: "github.com", Limit: 30, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", + PredicateType: verification.SLSAPredicateV1, SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), - Hostname: "github.com", }, wantsErr: false, }, @@ -193,12 +201,13 @@ func TestNewVerifyCmd(t *testing.T) { wants: Options{ ArtifactPath: artifactPath, DigestAlgorithm: "sha256", + Hostname: "github.com", + Limit: 101, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", - Limit: 101, + PredicateType: verification.SLSAPredicateV1, SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), - Hostname: "github.com", }, wantsErr: false, }, @@ -208,12 +217,13 @@ func TestNewVerifyCmd(t *testing.T) { wants: Options{ ArtifactPath: artifactPath, DigestAlgorithm: "sha256", + Hostname: "github.com", + Limit: 0, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", - Limit: 0, + PredicateType: verification.SLSAPredicateV1, SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), - Hostname: "github.com", }, wantsErr: true, }, @@ -223,13 +233,14 @@ func TestNewVerifyCmd(t *testing.T) { wants: Options{ ArtifactPath: artifactPath, DigestAlgorithm: "sha256", + Hostname: "github.com", Limit: 30, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", + PredicateType: verification.SLSAPredicateV1, SAN: "https://github.com/sigstore/", SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), - Hostname: "github.com", }, wantsErr: true, }, @@ -240,12 +251,30 @@ func TestNewVerifyCmd(t *testing.T) { ArtifactPath: artifactPath, BundlePath: bundlePath, DigestAlgorithm: "sha256", + Hostname: "github.com", Limit: 30, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", + PredicateType: verification.SLSAPredicateV1, SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), + }, + wantsExporter: true, + }, + { + name: "Use specified predicate type", + cli: fmt.Sprintf("%s --bundle %s --owner sigstore --predicate-type https://spdx.dev/Document/v2.3 --format json", artifactPath, bundlePath), + wants: Options{ + ArtifactPath: artifactPath, + BundlePath: bundlePath, + DigestAlgorithm: "sha256", Hostname: "github.com", + Limit: 30, + OIDCIssuer: verification.GitHubOIDCIssuer, + Owner: "sigstore", + PredicateType: "https://spdx.dev/Document/v2.3", + SANRegex: "(?i)^https://github.com/sigstore/", + SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsExporter: true, }, @@ -273,17 +302,18 @@ func TestNewVerifyCmd(t *testing.T) { assert.Equal(t, tc.wants.ArtifactPath, opts.ArtifactPath) assert.Equal(t, tc.wants.BundlePath, opts.BundlePath) - assert.Equal(t, tc.wants.TrustedRoot, opts.TrustedRoot) assert.Equal(t, tc.wants.DenySelfHostedRunner, opts.DenySelfHostedRunner) assert.Equal(t, tc.wants.DigestAlgorithm, opts.DigestAlgorithm) + assert.Equal(t, tc.wants.Hostname, opts.Hostname) assert.Equal(t, tc.wants.Limit, opts.Limit) assert.Equal(t, tc.wants.NoPublicGood, opts.NoPublicGood) assert.Equal(t, tc.wants.OIDCIssuer, opts.OIDCIssuer) assert.Equal(t, tc.wants.Owner, opts.Owner) + assert.Equal(t, tc.wants.PredicateType, opts.PredicateType) assert.Equal(t, tc.wants.Repo, opts.Repo) assert.Equal(t, tc.wants.SAN, opts.SAN) assert.Equal(t, tc.wants.SANRegex, opts.SANRegex) - assert.Equal(t, tc.wants.Hostname, opts.Hostname) + assert.Equal(t, tc.wants.TrustedRoot, opts.TrustedRoot) assert.NotNil(t, opts.APIClient) assert.NotNil(t, opts.Logger) assert.NotNil(t, opts.OCIClient) @@ -333,11 +363,12 @@ func TestJSONOutput(t *testing.T) { OCIClient: oci.MockClient{}, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", + PredicateType: verification.SLSAPredicateV1, SANRegex: "^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), exporter: cmdutil.NewJSONExporter(), } - require.Nil(t, runVerify(&opts)) + require.NoError(t, runVerify(&opts)) var target []*verification.AttestationProcessingResult err := json.Unmarshal(out.Bytes(), &target) @@ -356,12 +387,13 @@ func TestRunVerify(t *testing.T) { OCIClient: oci.MockClient{}, OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", + PredicateType: verification.SLSAPredicateV1, SANRegex: "^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), } t.Run("with valid artifact and bundle", func(t *testing.T) { - require.Nil(t, runVerify(&publicGoodOpts)) + require.NoError(t, runVerify(&publicGoodOpts)) }) t.Run("with failing OCI artifact fetch", func(t *testing.T) { @@ -453,75 +485,6 @@ func TestRunVerify(t *testing.T) { require.ErrorContains(t, err, "failed to fetch attestations from wrong-owner") }) - // TODO: this test can only be tested with a live SigstoreVerifier - // add integration tests or HTTP mocked sigstore verifier tests - // to test this case - t.Run("with invalid OIDC issuer", func(t *testing.T) { - t.Skip() - opts := publicGoodOpts - opts.OIDCIssuer = "not-a-real-issuer" - require.Error(t, runVerify(&opts)) - }) - - // TODO: this test can only be tested with a live SigstoreVerifier - // add integration tests or HTTP mocked sigstore verifier tests - // to test this case - t.Run("with SAN enforcement", func(t *testing.T) { - t.Skip() - opts := Options{ - ArtifactPath: artifactPath, - BundlePath: bundlePath, - APIClient: api.NewTestClient(), - DigestAlgorithm: "sha512", - Logger: logger, - OIDCIssuer: verification.GitHubOIDCIssuer, - Owner: "sigstore", - SAN: SigstoreSanValue, - SigstoreVerifier: verification.NewMockSigstoreVerifier(t), - } - require.Nil(t, runVerify(&opts)) - }) - - // TODO: this test can only be tested with a live SigstoreVerifier - // add integration tests or HTTP mocked sigstore verifier tests - // to test this case - t.Run("with invalid SAN", func(t *testing.T) { - t.Skip() - opts := publicGoodOpts - opts.SAN = "fake san" - require.Error(t, runVerify(&opts)) - }) - - // TODO: this test can only be tested with a live SigstoreVerifier - // add integration tests or HTTP mocked sigstore verifier tests - // to test this case - t.Run("with SAN regex enforcement", func(t *testing.T) { - t.Skip() - opts := publicGoodOpts - opts.SANRegex = SigstoreSanRegex - require.Nil(t, runVerify(&opts)) - }) - - // TODO: this test can only be tested with a live SigstoreVerifier - // add integration tests or HTTP mocked sigstore verifier tests - // to test this case - t.Run("with invalid SAN regex", func(t *testing.T) { - t.Skip() - opts := publicGoodOpts - opts.SANRegex = "^https://github.com/sigstore/not-real/" - require.Error(t, runVerify(&opts)) - }) - - // TODO: this test can only be tested with a live SigstoreVerifier - // add integration tests or HTTP mocked sigstore verifier tests - // to test this case - t.Run("with no matching OIDC issuer", func(t *testing.T) { - t.Skip() - opts := publicGoodOpts - opts.OIDCIssuer = "some-other-issuer" - require.Error(t, runVerify(&opts)) - }) - t.Run("with missing API client", func(t *testing.T) { customOpts := publicGoodOpts customOpts.APIClient = nil 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/repo/edit/edit.go b/pkg/cmd/repo/edit/edit.go index 508dea06c..29a149cf9 100644 --- a/pkg/cmd/repo/edit/edit.go +++ b/pkg/cmd/repo/edit/edit.go @@ -15,6 +15,7 @@ import ( fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/set" @@ -49,15 +50,16 @@ const ( ) type EditOptions struct { - HTTPClient *http.Client - Repository ghrepo.Interface - IO *iostreams.IOStreams - Edits EditRepositoryInput - AddTopics []string - RemoveTopics []string - InteractiveMode bool - Detector fd.Detector - Prompter iprompter + HTTPClient *http.Client + Repository ghrepo.Interface + IO *iostreams.IOStreams + Edits EditRepositoryInput + AddTopics []string + RemoveTopics []string + AcceptVisibilityChangeConsequences bool + InteractiveMode bool + Detector fd.Detector + Prompter iprompter // Cache of current repo topics to avoid retrieving them // in multiple flows. topicsCache []string @@ -103,7 +105,16 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr To toggle a setting off, use the %[1]s--=false%[1]s syntax. - Note that changing repository visibility to private will cause loss of stars and watchers. + Changing repository visibility can have unexpected consequences including but not limited to: + + - Losing stars and watchers, affecting repository ranking + - Detaching public forks from the network + - Disabling push rulesets + - Allowing access to GitHub Actions history and logs + + When the %[1]s--visibility%[1]s flag is used, %[1]s--accept-visibility-change-consequences%[1]s flag is required. + + For information on all the potential consequences, see `, "`"), Args: cobra.MaximumNArgs(1), Example: heredoc.Doc(` @@ -142,6 +153,10 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr return cmdutil.FlagErrorf("specify properties to edit when not running interactively") } + if opts.Edits.Visibility != nil && !opts.AcceptVisibilityChangeConsequences { + return cmdutil.FlagErrorf("use of --visibility flag requires --accept-visibility-change-consequences flag") + } + if runF != nil { return runF(opts) } @@ -167,6 +182,7 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr cmdutil.NilBoolFlag(cmd, &opts.Edits.AllowUpdateBranch, "allow-update-branch", "", "Allow a pull request head branch that is behind its base branch to be updated") cmd.Flags().StringSliceVar(&opts.AddTopics, "add-topic", nil, "Add repository topic") cmd.Flags().StringSliceVar(&opts.RemoveTopics, "remove-topic", nil, "Remove repository topic") + cmd.Flags().BoolVar(&opts.AcceptVisibilityChangeConsequences, "accept-visibility-change-consequences", false, "Accept the consequences of changing the repository visibility") return cmd } @@ -379,23 +395,26 @@ func interactiveRepoEdit(opts *EditOptions, r *api.Repository) error { } opts.Edits.EnableProjects = &a case optionVisibility: + cs := opts.IO.ColorScheme() + fmt.Fprintf(opts.IO.ErrOut, "%s Danger zone: changing repository visibility can have unexpected consequences; consult https://gh.io/setting-repository-visibility before continuing.\n", cs.WarningIcon()) + visibilityOptions := []string{"public", "private", "internal"} selected, err := p.Select("Visibility", strings.ToLower(r.Visibility), visibilityOptions) if err != nil { return err } - confirmed := true - if visibilityOptions[selected] == "private" && - (r.StargazerCount > 0 || r.Watchers.TotalCount > 0) { - cs := opts.IO.ColorScheme() - fmt.Fprintf(opts.IO.ErrOut, "%s Changing the repository visibility to private will cause permanent loss of stars and watchers.\n", cs.WarningIcon()) - confirmed, err = p.Confirm("Do you want to change visibility to private?", false) - if err != nil { - return err - } + selectedVisibility := visibilityOptions[selected] + + if selectedVisibility != r.Visibility && (r.StargazerCount > 0 || r.Watchers.TotalCount > 0) { + fmt.Fprintf(opts.IO.ErrOut, "%s Changing the repository visibility to %s will cause permanent loss of %s and %s.\n", cs.WarningIcon(), selectedVisibility, text.Pluralize(r.StargazerCount, "star"), text.Pluralize(r.Watchers.TotalCount, "watcher")) + } + + confirmed, err := p.Confirm(fmt.Sprintf("Do you want to change visibility to %s?", selectedVisibility), false) + if err != nil { + return err } if confirmed { - opts.Edits.Visibility = &visibilityOptions[selected] + opts.Edits.Visibility = &selectedVisibility } case optionMergeOptions: var defaultMergeOptions []string diff --git a/pkg/cmd/repo/edit/edit_test.go b/pkg/cmd/repo/edit/edit_test.go index 07f7d4d55..217c1dce4 100644 --- a/pkg/cmd/repo/edit/edit_test.go +++ b/pkg/cmd/repo/edit/edit_test.go @@ -34,6 +34,63 @@ func TestNewCmdEdit(t *testing.T) { }, }, }, + { + name: "deny public visibility change without accepting consequences", + args: "--visibility public", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{}, + }, + wantErr: "use of --visibility flag requires --accept-visibility-change-consequences flag", + }, + { + name: "allow public visibility change with accepting consequences", + args: "--visibility public --accept-visibility-change-consequences", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + Visibility: sp("public"), + }, + }, + }, + { + name: "deny private visibility change without accepting consequences", + args: "--visibility private", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{}, + }, + wantErr: "use of --visibility flag requires --accept-visibility-change-consequences flag", + }, + { + name: "allow private visibility change with accepting consequences", + args: "--visibility private --accept-visibility-change-consequences", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + Visibility: sp("private"), + }, + }, + }, + { + name: "deny internal visibility change without accepting consequences", + args: "--visibility internal", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{}, + }, + wantErr: "use of --visibility flag requires --accept-visibility-change-consequences flag", + }, + { + name: "allow internal visibility change with accepting consequences", + args: "--visibility internal --accept-visibility-change-consequences", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + Visibility: sp("internal"), + }, + }, + }, } for _, tt := range tests { @@ -241,6 +298,109 @@ func Test_editRun_interactive(t *testing.T) { })) }, }, + { + name: "skipping visibility without confirmation", + opts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + InteractiveMode: true, + }, + promptStubs: func(pm *prompter.MockPrompter) { + pm.RegisterMultiSelect("What do you want to edit?", nil, editList, + func(_ string, _, opts []string) ([]int, error) { + return []int{8}, nil + }) + pm.RegisterSelect("Visibility", []string{"public", "private", "internal"}, + func(_, _ string, opts []string) (int, error) { + return prompter.IndexFor(opts, "private") + }) + pm.RegisterConfirm("Do you want to change visibility to private?", func(_ string, _ bool) (bool, error) { + return false, nil + }) + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { + "data": { + "repository": { + "visibility": "public", + "description": "description", + "homePageUrl": "https://url.com", + "defaultBranchRef": { + "name": "main" + }, + "stargazerCount": 10, + "isInOrganization": false, + "repositoryTopics": { + "nodes": [{ + "topic": { + "name": "x" + } + }] + } + } + } + }`)) + reg.Exclude(t, httpmock.REST("PATCH", "repos/OWNER/REPO")) + }, + wantsStderr: "Changing the repository visibility to private will cause permanent loss of 10 stars and 0 watchers.", + }, + { + name: "changing visibility with confirmation", + opts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + InteractiveMode: true, + }, + promptStubs: func(pm *prompter.MockPrompter) { + pm.RegisterMultiSelect("What do you want to edit?", nil, editList, + func(_ string, _, opts []string) ([]int, error) { + return []int{8}, nil + }) + pm.RegisterSelect("Visibility", []string{"public", "private", "internal"}, + func(_, _ string, opts []string) (int, error) { + return prompter.IndexFor(opts, "private") + }) + pm.RegisterConfirm("Do you want to change visibility to private?", func(_ string, _ bool) (bool, error) { + return true, nil + }) + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { + "data": { + "repository": { + "visibility": "public", + "description": "description", + "homePageUrl": "https://url.com", + "defaultBranchRef": { + "name": "main" + }, + "stargazerCount": 10, + "watchers": { + "totalCount": 15 + }, + "isInOrganization": false, + "repositoryTopics": { + "nodes": [{ + "topic": { + "name": "x" + } + }] + } + } + } + }`)) + reg.Register( + httpmock.REST("PATCH", "repos/OWNER/REPO"), + httpmock.RESTPayload(200, `{}`, func(payload map[string]interface{}) { + assert.Equal(t, "private", payload["visibility"]) + })) + }, + wantsStderr: "Changing the repository visibility to private will cause permanent loss of 10 stars and 15 watchers", + }, { name: "the rest", opts: EditOptions{ @@ -250,7 +410,7 @@ func Test_editRun_interactive(t *testing.T) { promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterMultiSelect("What do you want to edit?", nil, editList, func(_ string, _, opts []string) ([]int, error) { - return []int{0, 2, 3, 5, 6, 8, 9}, nil + return []int{0, 2, 3, 5, 6, 9}, nil }) pm.RegisterInput("Default branch name", func(_, _ string) (string, error) { return "trunk", nil @@ -267,13 +427,6 @@ func Test_editRun_interactive(t *testing.T) { pm.RegisterConfirm("Convert into a template repository?", func(_ string, _ bool) (bool, error) { return true, nil }) - pm.RegisterSelect("Visibility", []string{"public", "private", "internal"}, - func(_, _ string, opts []string) (int, error) { - return prompter.IndexFor(opts, "private") - }) - pm.RegisterConfirm("Do you want to change visibility to private?", func(_ string, _ bool) (bool, error) { - return true, nil - }) pm.RegisterConfirm("Enable Wikis?", func(_ string, _ bool) (bool, error) { return true, nil }) @@ -310,7 +463,6 @@ func Test_editRun_interactive(t *testing.T) { assert.Equal(t, "https://zombo.com", payload["homepage"]) assert.Equal(t, true, payload["has_issues"]) assert.Equal(t, true, payload["has_projects"]) - assert.Equal(t, "private", payload["visibility"]) assert.Equal(t, true, payload["is_template"]) assert.Equal(t, true, payload["has_wiki"]) })) @@ -484,7 +636,7 @@ func Test_editRun_interactive(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ios, _, _, _ := iostreams.Test() + ios, _, _, stderr := iostreams.Test() ios.SetStdoutTTY(true) ios.SetStdinTTY(true) ios.SetStderrTTY(true) @@ -509,9 +661,11 @@ func Test_editRun_interactive(t *testing.T) { if tt.wantsErr == "" { require.NoError(t, err) } else { - assert.EqualError(t, err, tt.wantsErr) + require.EqualError(t, err, tt.wantsErr) return } + + assert.Contains(t, stderr.String(), tt.wantsStderr) }) } } 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/root/help_topic.go b/pkg/cmd/root/help_topic.go index e76dd2271..6d22b6ae7 100644 --- a/pkg/cmd/root/help_topic.go +++ b/pkg/cmd/root/help_topic.go @@ -41,17 +41,18 @@ var HelpTopics = []helpTopic{ { name: "environment", short: "Environment variables that can be used with gh", - long: heredoc.Docf(` - %[1]sGH_TOKEN%[1]s, %[1]sGITHUB_TOKEN%[1]s (in order of precedence): an authentication token for github.com - API requests. Setting this avoids being prompted to authenticate and takes precedence over - previously stored credentials. + long: heredoc.Docf(` + %[1]sGH_TOKEN%[1]s, %[1]sGITHUB_TOKEN%[1]s (in order of precedence): an authentication token that will be used when + a command targets either github.com or a subdomain of ghe.com. Setting this avoids being prompted to + authenticate and takes precedence over previously stored credentials. - %[1]sGH_ENTERPRISE_TOKEN%[1]s, %[1]sGITHUB_ENTERPRISE_TOKEN%[1]s (in order of precedence): an authentication - token for API requests to GitHub Enterprise. When setting this, also set %[1]sGH_HOST%[1]s. + %[1]sGH_ENTERPRISE_TOKEN%[1]s, %[1]sGITHUB_ENTERPRISE_TOKEN%[1]s (in order of precedence): an authentication + token that will be used when a command targets a GitHub Enterprise Server host. - %[1]sGH_HOST%[1]s: specify the GitHub hostname for commands that would otherwise assume the - "github.com" host when not in a context of an existing repository. When setting this, - also set %[1]sGH_ENTERPRISE_TOKEN%[1]s. + %[1]sGH_HOST%[1]s: specify the GitHub hostname for commands where a hostname has not been provided, or + cannot be inferred from the context of a local Git repository. If this host was previously + authenticated with, the stored credentials will be used. Otherwise, setting %[1]sGH_TOKEN%[1]s or + %[1]sGH_ENTERPRISE_TOKEN%[1]s is required, depending on the targeted host. %[1]sGH_REPO%[1]s: specify the GitHub repository in the %[1]s[HOST/]OWNER/REPO%[1]s format for commands that otherwise operate on a local repository. 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 }