Better messaging for `attestation verify` custom issuer mismatch error by bdehamer · Pull Request #9616 · cli/cli

bdehamer · 2024-09-12T23:03:36Z

GH Enterprise customers have the option to customize the issuer claim in their Actions-issued OIDC tokens to include their unique enterprise "slug".

For users who have customized the issuer claim, the attestation verify command is unable to determine to expected issuer value by default -- instead, the user must pass in their customized issuer value using the --cert-oidc-issuer flag. If the user does NOT supply the issuer value, they receive an error message similar to the following:

Error: verifying with issuer "sigstore.dev": failed to verify certificate identity: no matching CertificateIdentity found, last error: 
expected issuer value "https://token.actions.githubusercontent.com", got "https://token.actions.githubusercontent.com/foo-bar"

This change detects the scenario where the expected issuer differs only by the enterprise slug and nudges the user in the right direction by suggesting they use the --cert-oidc-issuer flag with their expected issuer value.

Error: expected issuer to be https://token.actions.githubusercontent.com, got https://token.actions.githubusercontent.com/foo-bar -- if you have customized the issuer for your enterprise try using the --cert-oidc-issuer flag with your expected issuer

See: #9615

cliAutomation · 2024-09-12T23:03:51Z

Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message.

bdehamer · 2024-09-12T23:07:44Z

pkg/cmd/attestation/verify/policy.go

-	issuerMatcher, err := verify.NewIssuerMatcher(opts.OIDCIssuer, "")
+	// Accept any issuer, we will verify the issuer as part of the extension verification
+	issuerMatcher, err := verify.NewIssuerMatcher("", ".*")


Currently, the issuer verification is handled by the sigstore-go verifier . . . however, the existing logic doesn't allow us to properly detect the customized issuer string and alter the error message accordingly.

Instead, we're going to let the verifier accept ANY issuer value and perform the issuer comparison as part of the extension verification done within the verify command itself.

bdehamer · 2024-09-12T23:11:30Z

pkg/cmd/attestation/verification/extensions.go

+	if !strings.EqualFold(want, certIssuer) {
+		if strings.Index(certIssuer, want) == 0 {
+			return fmt.Errorf("expected Issuer to be %s, got %s -- if you have customized the issuer for your enterprise try using the --cert-oidc-issuer flag with your expected issuer", want, certIssuer)
+		} else {
+			return fmt.Errorf("expected Issuer to be %s, got %s", want, certIssuer)
+		}
+	}


If the expected value IS NOT an exact match, but IS a substring of the actual value, the user will get the new error message suggesting they use the --cert-oidc-issuer flag.

bdehamer · 2024-09-12T23:12:58Z

pkg/cmd/attestation/verify/verify.go

 				}
 				config.TrustDomain = td
 				opts.Tenant = tenant
-				opts.OIDCIssuer = fmt.Sprintf(GitHubTenantOIDCIssuer, tenant)


This defaulting of the tenant-specific issuer string now happens in the VerifyCertExtensions function.

pkg/cmd/attestation/verification/extensions.go

steiza

LGTM!

phillmv

minor suggestions

pkg/cmd/attestation/verification/extensions.go

phillmv · 2024-09-13T19:33:16Z

pkg/cmd/attestation/verification/extensions.go

+		if strings.Index(certIssuer, want+"/") == 0 {
+			return fmt.Errorf("expected Issuer to be %s, got %s -- if you have customized the issuer for your enterprise try using the --cert-oidc-issuer flag with your expected issuer", want, certIssuer)
+		} else {
+			return fmt.Errorf("expected Issuer to be %s, got %s", want, certIssuer)


Should we give more of a hint here, too? What if we:

detected https://token.actions.*.ghe.com and prompted to set the tenant correctly (is that the failure condition?)

said something like, attestations must be created inside GitHub Actions otherwise?

Signed-off-by: Brian DeHamer <[email protected]> Co-authored-by: Zach Steindler <[email protected]> Co-authored-by: Phill MV <[email protected]>

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.57.0` -> `v2.58.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.58.0`](https://github.com/cli/cli/releases/tag/v2.58.0): GitHub CLI 2.58.0 [Compare Source](cli/cli@v2.57.0...v2.58.0) #### What's Changed - Better messaging for `attestation verify` custom issuer mismatch error by [@bdehamer](https://github.com/bdehamer) in cli/cli#9616 - Enhance gh repo create docs, fix random cmd link by [@andyfeller](https://github.com/andyfeller) in cli/cli#9630 - Add HasActiveToken method to AuthConfig to refactor auth check for `attestation trusted-root` command by [@BagToad](https://github.com/BagToad) in cli/cli#9635 - Improve the suggested command for creating an issue when an extension doesn't have a binary for your platform by [@timrogers](https://github.com/timrogers) in cli/cli#9608 - Disable auth check for `attestation trusted-root` command by [@bdehamer](https://github.com/bdehamer) in cli/cli#9610 - build(deps): bump github.com/henvic/httpretty from 0.1.3 to 0.1.4 by [@dependabot](https://github.com/dependabot) in cli/cli#9645 - Fix tenant-awareness for `trusted-root` command by [@bdehamer](https://github.com/bdehamer) in cli/cli#9638 - Replace "GitHub Enterprise Server" option with "other" in gh auth login prompting by [@jtmcg](https://github.com/jtmcg) in cli/cli#9642 - build(deps): bump github.com/cpuguy83/go-md2man/v2 from 2.0.4 to 2.0.5 by [@dependabot](https://github.com/dependabot) in cli/cli#9634 - Add `dnf5` instructions to `docs/install_linux.md` by [@its-miroma](https://github.com/its-miroma) in cli/cli#9660 - build(deps): bump github.com/theupdateframework/go-tuf/v2 from 2.0.0 to 2.0.1 by [@dependabot](https://github.com/dependabot) in cli/cli#9688 #### New Contributors - [@its-miroma](https://github.com/its-miroma) made their first contribution in cli/cli#9660 **Full Changelog**: cli/cli@v2.57.0...v2.58.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

bdehamer requested a review from a team as a code owner September 12, 2024 23:03

cliAutomation added the external pull request originating outside of the CLI core team label Sep 12, 2024

bdehamer commented Sep 12, 2024

View reviewed changes

kommendorkapten approved these changes Sep 13, 2024

View reviewed changes

steiza reviewed Sep 13, 2024

View reviewed changes

pkg/cmd/attestation/verification/extensions.go Outdated Show resolved Hide resolved

bdehamer force-pushed the bdehamer/custom-issuer-error branch from 6025ed4 to 090d9e1 Compare September 13, 2024 16:26

steiza approved these changes Sep 13, 2024

View reviewed changes

phillmv reviewed Sep 13, 2024

View reviewed changes

better error for att verify custom issuer mismatch

8c8423a

Signed-off-by: Brian DeHamer <[email protected]> Co-authored-by: Zach Steindler <[email protected]> Co-authored-by: Phill MV <[email protected]>

bdehamer force-pushed the bdehamer/custom-issuer-error branch from 1737473 to 8c8423a Compare September 16, 2024 19:39

bdehamer merged commit 2e13ec5 into trunk Sep 16, 2024

bdehamer deleted the bdehamer/custom-issuer-error branch September 16, 2024 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better messaging for `attestation verify` custom issuer mismatch error#9616

Better messaging for `attestation verify` custom issuer mismatch error#9616
bdehamer merged 1 commit intotrunkfrom
bdehamer/custom-issuer-error

bdehamer commented Sep 12, 2024 •

edited

Loading

Uh oh!

cliAutomation commented Sep 12, 2024

Uh oh!

bdehamer Sep 12, 2024

Uh oh!

bdehamer Sep 12, 2024

Uh oh!

bdehamer Sep 12, 2024 •

edited

Loading

Uh oh!

Uh oh!

steiza left a comment

Uh oh!

phillmv left a comment

Uh oh!

Uh oh!

phillmv Sep 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

bdehamer commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cliAutomation commented Sep 12, 2024

Uh oh!

bdehamer Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

bdehamer Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

bdehamer Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steiza left a comment

Choose a reason for hiding this comment

Uh oh!

phillmv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phillmv Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bdehamer commented Sep 12, 2024 •

edited

Loading

bdehamer Sep 12, 2024 •

edited

Loading