-
Notifications
You must be signed in to change notification settings - Fork 6.7k
Disable auth check for attestation trusted-root
command
#9610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disable auth check for attestation trusted-root
command
#9610
Conversation
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. |
Is this true for tenancy environments? Does it matter if it's not? @kommendorkapten |
Yes you are correct @williammartin, when tenancy is enabled, an authenticated call to the |
@bdehamer If you look at diff for you'll see a check I think you may want to add so it fails with a nicer error message if tenancy is on. If we are planning to do similar modifications to other attestation sub commands, we would need a similar check for that too. |
Signed-off-by: Brian DeHamer <bdehamer@github.com>
Signed-off-by: Brian DeHamer <bdehamer@github.com>
5e4852e
to
cbe8525
Compare
@williammartin / @kommendorkapten nice catch. I'd forgotten that we needed the meta endpoint when interacting with a tenant for this command. I've still left the auth check disabled at the command-level, but have added the explicit check for a token when in "tenancy-mode" |
Co-authored-by: Fredrik Skogman <kommendorkapten@github.com>
Co-authored-by: William Martin <williammartin@users.noreply.github.com>
@williammartin and I paired on reviewing this and added a suggestion in #9635 for you to review. |
…-to-AuthConfig Add HasActiveToken method to AuthConfig to refactor auth check for `attestation trusted-root` command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks!
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=-->
The
gh attestation trusted-root
command currently uses an authentication check to ensure that a GitHub token is present. When running in GH Actions, you may see an error like the following:Since this command doesn't actually interact with any GH APIs, there is no reason to force a token to be present.
This change disables the auth check for this command.
Fixes #9614