Skip to content

Disable auth check for attestation trusted-root command#9610

Merged
bdehamer merged 9 commits intotrunkfrom
bdehamer/disable-attestation-trusted-root-auth-check
Sep 19, 2024
Merged

Disable auth check for attestation trusted-root command#9610
bdehamer merged 9 commits intotrunkfrom
bdehamer/disable-attestation-trusted-root-auth-check

Conversation

@bdehamer
Copy link
Contributor

@bdehamer bdehamer commented Sep 12, 2024 •
edited
Loading

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

cliAutomation commented Sep 12, 2024

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

williammartin commented Sep 13, 2024

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

kommendorkapten commented Sep 13, 2024

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

@kommendorkapten
Copy link
Contributor

kommendorkapten commented Sep 13, 2024

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

@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

bdehamer commented Sep 16, 2024

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

kommendorkapten
kommendorkapten reviewed Sep 17, 2024
View reviewed changes
@BagToad BagToad mentioned this pull request Sep 18, 2024
Merged
@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.

@bdehamer
Merge pull request #9635 from cli/bagtoad/cli-9610-add-HasActiveToken…
49892e8 
…-to-AuthConfig

Add HasActiveToken method to AuthConfig to refactor auth check for `attestation trusted-root` command
@bdehamer bdehamer mentioned this pull request Sep 19, 2024
Merged
@bdehamer bdehamer requested a review from a team as a code owner September 19, 2024 14:07
williammartin
williammartin approved these changes Sep 19, 2024
View reviewed changes
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
@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
@tmeijn
build(deps): update cli/cli to v2.58.0
35fc59b 
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

Assignees

No one assigned

Labels

external pull request originating outside of the CLI core team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

gh attestation trusted-root requires auth

6 participants

@bdehamer @cliAutomation @williammartin @kommendorkapten @BagToad @malancas