Better messaging for attestation verify custom issuer mismatch error
#9616
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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
commented
Sep 12, 2024
Comment on lines
-54
to
+53
| issuerMatcher, err := verify.NewIssuerMatcher(opts.OIDCIssuer, "") | ||
| // Accept any issuer, we will verify the issuer as part of the extension verification | ||
| issuerMatcher, err := verify.NewIssuerMatcher("", ".*") |
There was a problem hiding this comment.
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
commented
Sep 12, 2024
Comment on lines
67
to
73
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
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
commented
Sep 12, 2024
| @@ -151,7 +151,6 @@ func NewVerifyCmd(f *cmdutil.Factory, runF func(*Options) error) *cobra.Command | |||
| } | |||
| config.TrustDomain = td | |||
| opts.Tenant = tenant | |||
| opts.OIDCIssuer = fmt.Sprintf(GitHubTenantOIDCIssuer, tenant) | |||
There was a problem hiding this comment.
This defaulting of the tenant-specific issuer string now happens in the VerifyCertExtensions function.
kommendorkapten
approved these changes
Sep 13, 2024
steiza
reviewed
Sep 13, 2024
6025ed4 to
090d9e1
Compare
phillmv
reviewed
Sep 13, 2024
| 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) |
There was a problem hiding this comment.
Should we give more of a hint here, too? What if we:
- detected
https://token.actions.*.ghe.comand 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 <bdehamer@github.com> Co-authored-by: Zach Steindler <steiza@github.com> Co-authored-by: Phill MV <phillmv@github.com>
1737473 to
8c8423a
Compare
tmeijn
pushed a commit
to tmeijn/dotfiles
that referenced
this pull request
Oct 16, 2024
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. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 verifycommand is unable to determine to expected issuer value by default -- instead, the user must pass in their customized issuer value using the--cert-oidc-issuerflag. If the user does NOT supply the issuer value, they receive an error message similar to the following: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-issuerflag with their expected issuer value.See: #9615