proof of concept for flag-level disable auth check

[andyfeller]

Relates #8995

Building upon the existing command-level disable auth check logic, this commit adds flag-level disable auth check logic for any flag set with -b,--bundle flag of gh attestation verify being the first use case.

Subsequent commit to build out testing is needed as IsAuthCheckEnabled does not have tests.

Testing

1. Build changes and setup temporary working directory

   andyfeller@Andys-MBP:cli/cli ‹andyfeller/flag-level-disableauth›$ make
   go build -trimpath -ldflags "-X github.com/cli/cli/v2/internal/build.Date=2024-04-25 -X github.com/cli/cli/v2/internal/build.Version=v2.48.0-9-g2d910406 " -o bin/gh ./cmd/gh
   
   andyfeller@Andys-MBP:cli/cli ‹andyfeller/flag-level-disableauth›$ mkdir tmp
   andyfeller@Andys-MBP:cli/cli ‹andyfeller/flag-level-disableauth›$ cd tmp 

2. Download release artifact(s) built with attestation signatures captured

   andyfeller@Andys-MBP:cli/tmp ‹andyfeller/flag-level-disableauth›$ ../bin/gh release download v0.0.10 --repo advanced-security/gh-sbom
   andyfeller@Andys-MBP:cli/tmp ‹andyfeller/flag-level-disableauth*›$ ll
   total 152360
   -rw-r--r--  1 andyfeller  staff   5.8M Apr 25 09:41 android-amd64
   -rw-r--r--  1 andyfeller  staff   5.6M Apr 25 09:41 android-arm64
   -rw-r--r--  1 andyfeller  staff   5.6M Apr 25 09:41 darwin-amd64
   -rw-r--r--  1 andyfeller  staff   5.5M Apr 25 09:41 darwin-arm64
   -rw-r--r--  1 andyfeller  staff   5.0M Apr 25 09:41 freebsd-386
   -rw-r--r--  1 andyfeller  staff   5.3M Apr 25 09:41 freebsd-amd64
   -rw-r--r--  1 andyfeller  staff   5.1M Apr 25 09:41 freebsd-arm64
   -rw-r--r--  1 andyfeller  staff   5.1M Apr 25 09:41 linux-386
   -rw-r--r--  1 andyfeller  staff   5.3M Apr 25 09:41 linux-amd64
   -rw-r--r--  1 andyfeller  staff   5.2M Apr 25 09:41 linux-arm
   -rw-r--r--  1 andyfeller  staff   5.1M Apr 25 09:41 linux-arm64
   -rw-r--r--  1 andyfeller  staff   5.3M Apr 25 09:41 windows-386.exe
   -rw-r--r--  1 andyfeller  staff   5.5M Apr 25 09:41 windows-amd64.exe
   -rw-r--r--  1 andyfeller  staff   5.2M Apr 25 09:41 windows-arm64.exe

3. Download artifact attestation bundle for artifact and verify

   andyfeller@Andys-MBP:cli/tmp ‹andyfeller/flag-level-disableauth*›$ ../bin/gh attestation download darwin-amd64 --repo advanced-security/gh-sbom
   Wrote attestations to file sha256:ab1bae8f19e92e1dce879373194627c1b322575618d13dd635572777d6df2437.jsonl.
   Any previous content has been overwritten
   
   The trusted metadata is now available at sha256:ab1bae8f19e92e1dce879373194627c1b322575618d13dd635572777d6df2437.jsonl
   
   
   andyfeller@Andys-MBP:cli/tmp ‹andyfeller/flag-level-disableauth*›$ ../bin/gh attestation verify darwin-amd64 --bundle sha256:ab1bae8f19e92e1dce879373194627c1b322575618d13dd635572777d6df2437.jsonl --repo advanced-security/gh-sbom
   Loaded digest sha256:ab1bae8f19e92e1dce879373194627c1b322575618d13dd635572777d6df2437 for file://darwin-amd64
   Loaded 2 attestations from sha256:ab1bae8f19e92e1dce879373194627c1b322575618d13dd635572777d6df2437.jsonl
   ✓ Verification succeeded!
   
   sha256:ab1bae8f19e92e1dce879373194627c1b322575618d13dd635572777d6df2437 was attested by:
   REPO                       PREDICATE_TYPE                  WORKFLOW                                        
   advanced-security/gh-sbom  https://slsa.dev/provenance/v1  .github/workflows/release.yml@refs/tags/v0.0.10
   advanced-security/gh-sbom  https://slsa.dev/provenance/v1  .github/workflows/release.yml@refs/tags/vtesting

4. Logout of github.com

   andyfeller@Andys-MBP:cli/tmp ‹andyfeller/flag-level-disableauth*›$ ../bin/gh auth status
   staffship-01.ghe.com
     ✓ Logged in to staffship-01.ghe.com account andyfeller (keyring)
     - Active account: true
     - Git operations protocol: https
     - Token: gho_************************************
     - Token scopes: 'gist', 'read:org', 'repo', 'workflow'
   
   github.ghe.com
     ✓ Logged in to github.ghe.com account andyfeller (keyring)
     - Active account: true
     - Git operations protocol: https
     - Token: gho_************************************
     - Token scopes: 'gist', 'read:org', 'repo', 'workflow'
   
   github.com
     ✓ Logged in to github.com account andyfeller (keyring)
     - Active account: true
     - Git operations protocol: ssh
     - Token: gho_************************************
     - Token scopes: 'admin:public_key', 'gist', 'read:org', 'repo'
   
   andyfeller@Andys-MBP:cli/tmp ‹andyfeller/flag-level-disableauth*›$ ../bin/gh auth logout
   ? What account do you want to log out of? andyfeller (github.com)
   ✓ Logged out of github.com account andyfeller
   
   andyfeller@Andys-MBP:cli/tmp ‹andyfeller/flag-level-disableauth*›$ ../bin/gh auth status
   staffship-01.ghe.com
     ✓ Logged in to staffship-01.ghe.com account andyfeller (keyring)
     - Active account: true
     - Git operations protocol: https
     - Token: gho_************************************
     - Token scopes: 'gist', 'read:org', 'repo', 'workflow'
   
   github.ghe.com
     ✓ Logged in to github.ghe.com account andyfeller (keyring)
     - Active account: true
     - Git operations protocol: https
     - Token: gho_************************************
     - Token scopes: 'gist', 'read:org', 'repo', 'workflow'

5. Verify that gh attestation verify -b does not require auth with flag

   andyfeller@Andys-MBP:cli/tmp ‹andyfeller/flag-level-disableauth*›$ ../bin/gh attestation verify darwin-amd64 --bundle sha256:ab1bae8f19e92e1dce879373194627c1b322575618d13dd635572777d6df2437.jsonl --repo advanced-security/gh-sbom
   Loaded digest sha256:ab1bae8f19e92e1dce879373194627c1b322575618d13dd635572777d6df2437 for file://darwin-amd64
   Loaded 2 attestations from sha256:ab1bae8f19e92e1dce879373194627c1b322575618d13dd635572777d6df2437.jsonl
   ✓ Verification succeeded!
   
   sha256:ab1bae8f19e92e1dce879373194627c1b322575618d13dd635572777d6df2437 was attested by:
   REPO                       PREDICATE_TYPE                  WORKFLOW                                        
   advanced-security/gh-sbom  https://slsa.dev/provenance/v1  .github/workflows/release.yml@refs/tags/v0.0.10
   advanced-security/gh-sbom  https://slsa.dev/provenance/v1  .github/workflows/release.yml@refs/tags/vtesting

[steiza]

This looks like a workable alternative to #8995 to me!

[andyfeller]

@steiza @phillmv : is the manual process I listed above how you would test the feature?

@williammartin : is there anything about these changes you'd like to see beyond adding tests around the various auth check annotation use cases?

[williammartin]

TL;DR

Let's move forward with this PR, thanks!

Design thoughts

I appreciate the work you've done here and I think it's pretty clever. That said, for a few reasons I prefer a less abstracted approach as in #8995. I've mentioned this elsewhere but I think that tying the authentication check logic into the cobra lifecycle takes us in a less maintainable direction in the long run. Copying and slightly modifying parts of a comment I made elsewhere so that it's in our public record:

---

I think the auth checks are too tightly coupled to Cobra configuration and that the responsibility to make this decision should be owned closer to the business logic. From an architecture astronauting point of view, it's a leaky abstraction and though I don't expect we'll ever swap out cobra, that all the auth checks that the business logic depends upon would no longer be validated preconditions if we did, should give us a hint that it's not where it belongs. Furthermore we see that the root command has become knowledgable about gh codespaces with some hard to understand logic which should be suspicious.

From a less theoretical point of view, this knowledge living in Cobra configuration makes it hard for the business logic to be specific about when it matters, as we've discovered with this offline verification issue. Additionally, the current situation causes spooky action at a distance and I would argue the situation that attestation is facing right now is a direct result of that. Had we looked at another command and seen something like:

func (c MyCommand) Run() error {
    if err := c.SharedActor.EnsureAuthenticated(); err != nil {
       return err
    }
}

We probably would have realised pretty quickly that authed / not authed was something to look at more closely. I'm not suggesting ^ is the solution, just an indication of code that would have acted as a beacon.

---

To call out a specific example, imagine a command that doesn't know whether authentication is required until some state has been evaluated.

All that said, I'm happy to move forward with this PR (plus tests) since you are keen on this approach and the risk is low. Really I just wanted a place to capture thoughts should we run into a situation where we wanted to revisit this decision, and to serve as a mental trigger when we come to thinking about other places where command level auth checks are not sufficient (e.g. extension install of public extensions).

[andyfeller]

Firstly, I want to say I completely agree with you:

I think the auth checks are too tightly coupled to Cobra configuration and that the responsibility to make this decision should be owned closer to the business logic.

Cobra is / should be merely an invocation layer handling flags and arguments to application code while whether authentication is necessary is an application concern. From a long term goal, I think refactoring the code to ensure application code is responsible for whether authentication is required makes sense.

Secondly, I don't think that work is trivial in the short term, necessitating a now solution until we get to the later solution

This draft was only intended as a now solution because authentication is tightly coupled to Cobra event hooks and I couldn't think of a simpler way to add support without significantly refactoring commands and the logic around Cobra event hooks:

cli/pkg/cmd/root/root.go

Lines 73 to 86 in 7d432bc

	PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
	// require that the user is authenticated before running most commands
	if cmdutil.IsAuthCheckEnabled(cmd) && !cmdutil.CheckAuth(cfg) {
	parent := cmd.Parent()
	if parent != nil && parent.Use == "codespace" {
	fmt.Fprintln(io.ErrOut, "To get started with GitHub CLI, please run: gh auth login -s codespace")
	} else {
	fmt.Fprint(io.ErrOut, authHelp())
	}
	return &AuthError{}
	}
	return nil
	},
	}

[williammartin]

One test addition I think would be useful

[williammartin]

Lol not suspicious of coupling at all.

[williammartin]

Before (without --bundle)

GH_CONFIG_DIR=/tmp/test/gh gh attestation verify oci://ghcr.io/phillmv/arquivo -o phillmv                     
To get started with GitHub CLI, please run:  gh auth login
Alternatively, populate the GH_TOKEN environment variable with a GitHub API authentication token.

After (without --bundle)

GH_CONFIG_DIR=/tmp/test/gh ~/workspace/cli/bin/gh attestation verify oci://ghcr.io/phillmv/arquivo -o phillmv 
To get started with GitHub CLI, please run:  gh auth login
Alternatively, populate the GH_TOKEN environment variable with a GitHub API authentication token.

Before (with --bundle)

GH_CONFIG_DIR=/tmp/test/gh gh attestation verify oci://ghcr.io/phillmv/arquivo -o phillmv --bundle bundle.json 
To get started with GitHub CLI, please run:  gh auth login
Alternatively, populate the GH_TOKEN environment variable with a GitHub API authentication token.

After (with --bundle)

GH_CONFIG_DIR=/tmp/test/gh ~/workspace/cli/bin/gh attestation verify oci://ghcr.io/phillmv/arquivo -o phillmv --bundle bundle.json
Loaded digest sha256:5936c74c8999e9fd50ca0658f378a5d3f8492fb04bd77b952942fdbba4d038bb for oci://ghcr.io/phillmv/arquivo
✗ Loading attestations from oci://ghcr.io/phillmv/arquivo failed

Error: bundle could not be loaded from JSON file: open bundle.json: no such file or directory

We can see in this last case that although the bundle didn't exist (expected error) we did not have the auth check.

LGTM!