Skip to content
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

Support offline mode for gh attestation verify #8997

Merged
merged 6 commits into from Apr 29, 2024

Conversation

steiza
Copy link
Contributor

@steiza steiza commented Apr 24, 2024

The main change is previously we always instantiated a TUF client for the public good and GitHub Sigstore instances. Now we only instantiate the TUF client we need, or no client if we are provided a custom trusted root.

Note that gh attestation verify still requires authentication, that is being addressed in #8995.

Some other changes are coming along for the ride:

  • Set TUF cache validity to 1 day, to help serial verification
  • Attempt to infer verification policy based on custom trusted root
  • Make command output more friendly if you leave off required arguments

The main change is previously we always instantiated a TUF client for
the public good and GitHub Sigstore instances. Now we only instantiate
the TUF client we need, or no client if we are provided a
custom trusted root.

Note that `gh attestation verify` still requires authentication, that is
being addressed in cli#8995.

Some other changes are coming along for the ride:
- Set TUF cache validity to 1 day, to help serial verification
- Attempt to infer verification policy based on custom trusted root
- Make command output more friendly if you leave off required arguments

Signed-off-by: Zach Steindler <steiza@github.com>
@steiza steiza requested a review from a team as a code owner April 24, 2024 14:32
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Apr 24, 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.

@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Apr 24, 2024
Signed-off-by: Zach Steindler <steiza@github.com>
Copy link
Contributor

@malancas malancas left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Glad to see we can use the TUF cache and with picking the verifier on the fly.

@williammartin
Copy link
Member

williammartin commented Apr 26, 2024

@steiza would you consider pulling in the commits in #9005 to this PR?

Commit 1

The first commit reworks the sigstore tests that you changed in this PR a little to be something I consider a little clearer. I appreciate there is some subjectivity here so I'll make my case. In configuring subtest setup in the outer scope, particularly when mutating shared resources (e.g. the SigstoreConfig) I think becomes challenging to keep the mental model and inhibits certain refactors. For example, if we move the current public good tests below the custom trusted root tests, we won't know that the code path has changed and a different verifier is being used. Similarly, if we mark the tests with t.Parallel() (which I'd love to do across the board) we run into the same issue because the outer scope is evaluated before the subtests are kicked off.

The change I've made ensures that each subtest is fully isolated, and I've leaned into helpers to remove noise from the tests. By using t.Helper and require we also reduce some error checking from within the tests themselves.

Commit 2

This is really just some additional testing that I added as I'm building my understanding of all the new pieces. It covers the first two branches in chooseVerifier:

image

I achieved this by adding two new attestation files that were copies of the "good one". In one I removed the entire verificationMaterial block and in the other I changed the x509CertificateChain to publicKey so that HasCertificate returns false.

Commit 3

This just seems like some unnecessary defensive coding. There is only one code path that leads here and it validates that the trustedRootFilePath is not empty. I got distracted trying to understand if this would ever be the case and in fact I think it would be extremely surprising behaviour to have no error and a nil *verify.SignedEntityVerifier. Very easy way to panic by accident.

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!

@williammartin williammartin merged commit 6d8709b into cli:trunk Apr 29, 2024
25 checks passed
alexcb pushed a commit to earthly/earthly that referenced this pull request May 7, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [cli/cli](https://togithub.com/cli/cli) | minor | `v2.48.0` ->
`v2.49.0` |

---

### Release Notes

<details>
<summary>cli/cli (cli/cli)</summary>

### [`v2.49.0`](https://togithub.com/cli/cli/releases/tag/v2.49.0):
GitHub CLI 2.49.0

[Compare Source](https://togithub.com/cli/cli/compare/v2.48.0...v2.49.0)

#### Support for GitHub Artifact Attestations

`v2.49.0` release introduces the `attestation` command set for
downloading and verifying attestations about artifacts built in GitHub
Actions! This is part of the larger Artifact Attestations initiative. An
artifact attestation is a piece of cryptographically signed metadata
that is generated as part of your artifact build process. These
attestations bind artifacts to the details of the workflow run that
produced them, and allow you to guarantee the integrity and provenance
of any artifact built in GitHub Actions.

```shell

### Verify a local artifact
gh attestation verify artifact.bin -o <your org>

### Verify a local artifact against a local artifact attestation
gh attestation verify artifact.bin -b ./artifact-v0.0.1-bundle.json -o <your org>

### Verify an OCI image
gh attestation verify oci://ghcr.io/foo/bar:latest -o <your org>

### Download artifact attestations
gh attestation download artifact.bin -o <your org>
```

To get started, check out gh help attestation. You can also use the `gh
at <command>` alias for short.

#### What's Changed

- Improve gh run rerun docs by
[@&#8203;sochotnicky](https://togithub.com/sochotnicky) in
[cli/cli#8969
- build(deps): bump golang.org/x/net from 0.21.0 to 0.23.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[cli/cli#8981
- Update `sigstore-go` dependency to v0.3.0 by
[@&#8203;malancas](https://togithub.com/malancas) in
[cli/cli#8977
- `gh attestation tuf-root-verify` offline test fix by
[@&#8203;malancas](https://togithub.com/malancas) in
[cli/cli#8975
- Update `gh attestation verify` output by
[@&#8203;malancas](https://togithub.com/malancas) in
[cli/cli#8991
- build(deps): bump google.golang.org/grpc from 1.62.1 to 1.62.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[cli/cli#8989
- Remove `Hidden` flag from `gh attestation` command by
[@&#8203;malancas](https://togithub.com/malancas) in
[cli/cli#8998
- Add colon for `gh secret set` by
[@&#8203;NeroBlackstone](https://togithub.com/NeroBlackstone) in
[cli/cli#9004
- Improve errors when loading bundle locally fails by
[@&#8203;williammartin](https://togithub.com/williammartin) in
[cli/cli#8996
- Support offline mode for `gh attestation verify` by
[@&#8203;steiza](https://togithub.com/steiza) in
[cli/cli#8997
- Add `projectsV2` to JSON fields of `gh repo` commands by
[@&#8203;babakks](https://togithub.com/babakks) in
[cli/cli#9007
- Support long URLs in `gh repo clone` by
[@&#8203;babakks](https://togithub.com/babakks) in
[cli/cli#9008
- Fix issue with closing pager stream by
[@&#8203;babakks](https://togithub.com/babakks) in
[cli/cli#9020
- proof of concept for flag-level disable auth check by
[@&#8203;andyfeller](https://togithub.com/andyfeller) in
[cli/cli#9000
- Be more general with attestation host checks by
[@&#8203;williammartin](https://togithub.com/williammartin) in
[cli/cli#9019
- Add beta designation on attestation command set by
[@&#8203;andyfeller](https://togithub.com/andyfeller) in
[cli/cli#9022
- Tweaked gh attestation help strings to generate nicer cli manual site.
by [@&#8203;phillmv](https://togithub.com/phillmv) in
[cli/cli#9025
- Update cli/go-gh to v2.9.0 by
[@&#8203;andyfeller](https://togithub.com/andyfeller) in
[cli/cli#9023
- Document repo clone protocol behaviour by
[@&#8203;williammartin](https://togithub.com/williammartin) in
[cli/cli#9030

#### New Contributors

- [@&#8203;sochotnicky](https://togithub.com/sochotnicky) made their
first contribution in
[cli/cli#8969
- [@&#8203;NeroBlackstone](https://togithub.com/NeroBlackstone) made
their first contribution in
[cli/cli#9004
- [@&#8203;phillmv](https://togithub.com/phillmv) made their first
contribution in
[cli/cli#9025

**Full Changelog**: cli/cli@v2.48.0...v2.49.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 4pm on monday" (UTC), Automerge
- At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/earthly/earthly).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNDAuMTAiLCJ1cGRhdGVkSW5WZXIiOiIzNy4zNDAuMTAiLCJ0YXJnZXRCcmFuY2giOiJtYWluIiwibGFiZWxzIjpbInJlbm92YXRlIl19-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external pull request originating outside of the CLI core team
Projects
No open projects
The GitHub CLI
  
Needs review 🤔
Development

Successfully merging this pull request may close these issues.

None yet

5 participants