Fix tenant-awareness for trusted-root command#9638
Conversation
Signed-off-by: Brian DeHamer <[email protected]>
| // Target will be either the default trusted root, or the trust domain-qualified one | ||
| ghTR := defaultTR | ||
| if opts.TrustDomain != "" { | ||
| ghTR = fmt.Sprintf("%s.%s", opts.TrustDomain, defaultTR) | ||
| } | ||
|
|
There was a problem hiding this comment.
One subtle change from the existing logic: instead of retrieving both the default target (trusted_root.json) AND the tenant-prefixed target (some-trust-domain.trusted_root.json), the new logic will retrieve ONLY the tenant-prefixed version if the --hostname flag is supplied.
If you're identifying a specific tenant with the --hostname flag it seems reasonable to assume that you're only interested in the trusted root associated with that tenant.
There was a problem hiding this comment.
Seems fair. PGI is always included though. Given that a properly configured policy is configured, there would be no risk of confusion here during verification.
| targets := []string{defaultTR} | ||
| if opts.TrustDomain != "" { | ||
| targets = append(targets, fmt.Sprintf("%s.%s", | ||
| opts.TrustDomain, defaultTR)) | ||
| } |
There was a problem hiding this comment.
Note how the old logic specifies BOTH the default target AND the tenant-aware target for retrieval.
trevrosen
left a comment
There was a problem hiding this comment.
I'm approving but also it feels like maybe there should be a test here?
I take that back, there's really no reasonable way to test this without mocking out an entire TUF repository or allowing the tests to hit a remote endpoint -- I'm guessing this is why there were not tests around this functionality to begin with. |
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=-->
Addresses the issue in the
attestation trusted-rootcommand which prevents the--tuf-urland--hostnameflags from being used together.There is already logic which calculates the correct
trusted_root.jsonprefix based on the supplied--hostnameflag, but it was only being applied in the case where the--tuf-urlwas NOT supplied.This change moves the logic to generate the tenant-aware
trusted_root.jsonprefix and applies it to all cases.Fixes #9637