8000 Disable auth check for `attestation trusted-root` command by bdehamer · Pull Request #9610 · cli/cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 9 commits into from
Sep 19, 2024

Conversation

bdehamer
Copy link
Contributor
@bdehamer bdehamer commented Sep 12, 2024

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:

gh: To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable. Example:
  env:
    GH_TOKEN: ${{ github.token }}
Error: Process completed with exit code 4.

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

@bdehamer bdehamer requested a review from a team as a code owner September 12, 2024 16:22
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Sep 12, 2024
@cliAutomation
Copy link
Collaborator

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.

@williammartin
Copy link
Member

Since this command doesn't actually interact with any GH APIs, there is no reason to force a token to be present.

Is this true for tenancy environments? Does it matter if it's not? @kommendorkapten

@kommendorkapten
Copy link
Contributor

Yes you are correct @williammartin, when tenancy is enabled, an authenticated call to the /meta endpoint is made.

@kommendorkapten
Copy link
Contributor

@bdehamer If you look at diff for trusted_root here: bdehamer/disable-attestation-trusted-root-auth-check...wm-fs/error-on-not-authed

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>
@bdehamer bdehamer force-pushed the bdehamer/disable-attestation-trusted-root-auth-check branch from 5e4852e to cbe8525 Compare September 16, 2024 20:53
@bdehamer
Copy link
Contributor Author

@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"

@BagToad
Copy link
Member
BagToad commented Sep 18, 2024

👋 @bdehamer

@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
@bdehamer bdehamer requested a review from a team as a code owner September 19, 2024 14:07
Copy link
Member
@williammartin williammartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks!

@bdehamer bdehamer merged commit d451a43 into trunk Sep 19, 2024
16 checks passed
@bdehamer bdehamer deleted the bdehamer/disable-attestation-trusted-root-auth-check branch September 19, 2024 19:28
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 [@&#8203;bdehamer](https://github.com/bdehamer) in cli/cli#9616
-   Enhance gh repo create docs, fix random cmd link by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#9630
-   Add HasActiveToken method to AuthConfig to refactor auth check for `attestation trusted-root` command by [@&#8203;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 [@&#8203;timrogers](https://github.com/timrogers) in cli/cli#9608
-   Disable auth check for `attestation trusted-root` command by [@&#8203;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 [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#9645
-   Fix tenant-awareness for `trusted-root` command by [@&#8203;bdehamer](https://github.com/bdehamer) in cli/cli#9638
-   Replace "GitHub Enterprise Server" option with "other" in gh auth login prompting by [@&#8203;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 [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#9634
-   Add `dnf5` instructions to `docs/install_linux.md` by [@&#8203;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 [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#9688

#### New Contributors

-   [@&#8203;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
Reviewers

@williammartin williammartin williammartin approved these changes

@malancas malancas malancas approved these changes

@kommendorkapten kommendorkapten Awaiting requested review from kommendorkapten

Labels
external pull request originating outside of the CLI core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gh attestation trusted-root requires auth
6 participants
0