diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 268c651..19c1864 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -13,9 +13,9 @@ jobs: build: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Set up Go - uses: actions/setup-go@v7 + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: go.mod - name: Build diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 318cda5..556241a 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -57,8 +57,7 @@ jobs: # your codebase is analyzed, see https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/codeql-code-scanning-for-compiled-languages steps: - name: Checkout repository - uses: actions/checkout@v7 - + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 # Add any setup steps before running the `github/codeql-action/init` action. # This includes steps like installing compilers or runtimes (`actions/setup-node` # or others). This is typically only required for manual builds. @@ -67,7 +66,7 @@ jobs: # Initializes the CodeQL tools for scanning. - name: Initialize CodeQL - uses: github/codeql-action/init@v4.39.1 + uses: github/codeql-action/init@9b3254085494b55a0f7c4c2b3d2d52cc712ce370 # v4.39.1 with: languages: ${{ matrix.language }} build-mode: ${{ matrix.build-mode }} @@ -95,6 +94,6 @@ jobs: exit 1 - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@v4.39.1 + uses: github/codeql-action/analyze@9b3254085494b55a0f7c4c2b3d2d52cc712ce370 # v4.39.1 with: category: "/language:${{matrix.language}}" diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml index 21988f1..eb4ae66 100644 --- a/.github/workflows/golangci-lint.yml +++ b/.github/workflows/golangci-lint.yml @@ -10,12 +10,12 @@ jobs: name: lint runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 - - uses: actions/setup-go@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: go.mod - name: golangci-lint - uses: golangci/golangci-lint-action@v9 + uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0 with: version: v2.13.2 only-new-issues: true diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index ff52f0f..69eff89 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -20,13 +20,13 @@ jobs: # goreleaser creates the GitHub release and uploads the archives. contents: write steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: # goreleaser builds the changelog from history, so a shallow # checkout would produce an empty one. fetch-depth: 0 - - uses: actions/setup-go@v7 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: go.mod @@ -43,7 +43,7 @@ jobs: echo "HOMEBREW_TAP_GITHUB_TOKEN is present. The cask will be pushed to jferrl/homebrew-tap and the binaries to jferrl/gh-githubauth." fi - - uses: goreleaser/goreleaser-action@v7 + - uses: goreleaser/goreleaser-action@a4f02ab5355a589de60d917a659ce617ed19b3de # v7.3.0 with: version: "~> v2" args: release --clean diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 4938bd1..04d0de7 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -36,14 +36,13 @@ jobs: with: access_token: ${{ github.token }} - - uses: actions/checkout@v7 - - - uses: actions/setup-go@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version: ${{ matrix.go-version }} - name: Cache go modules - uses: actions/cache@v6 + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 with: path: | ~/.cache/go-build @@ -58,6 +57,6 @@ jobs: - name: Upload coverage to Codecov if: ${{ matrix.update-coverage }} - uses: codecov/codecov-action@v7 + uses: codecov/codecov-action@303a32d7a59b442fa8d48b6a1cc6825c09c847a5 # v7.1.1 with: token: ${{ secrets.CODECOV_TOKEN }} diff --git a/AGENTS.md b/AGENTS.md index 413db93..4a4884b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,7 +20,9 @@ githubauth token --installation 12345 --exec -- gh pr list The same works for `git` once it takes credentials from `gh` (`gh auth setup-git`): `githubauth token --installation 12345 --exec -- git push`. With `--enterprise-url` -or `--base-url`, `--exec` also sets `$GH_HOST` unless you already have. For the +or `--base-url`, `--exec` also sets `$GH_HOST`. An exported `$GH_HOST` naming +another instance than the installation token's exits 2 before minting, and the command +never sees `$GITHUB_APP_PRIVATE_KEY`. For the JWT, `gh` needs the header spelled out, because it sends `token` and GitHub takes a JWT only as `Bearer`: @@ -59,7 +61,9 @@ Every flag falls back to an environment variable — `GITHUB_APP_CLIENT_ID`, configured environment reduces the call to `githubauth token`. `--key` takes a file path, `-` for stdin, or the PEM itself, which is what lets -the key come from a secret manager without touching disk: +the key come from a secret manager without touching disk. Pass the PEM itself +through `$GITHUB_APP_PRIVATE_KEY` or stdin, never on the command line, where the +process list shows it: ```bash vault kv get -field=pem secret/github-app | githubauth token --key - --installation 12345 diff --git a/CHANGELOG.md b/CHANGELOG.md index 75bdc8b..b9543d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,15 +17,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] `githubauth token --exec` now revokes the token when the command exits, and the CLI can -find the installation and narrow the token on its own. Three changes can affect existing -code or scripts; they are listed under Changed. +find the installation and narrow the token on its own. A security review closed six +ways a credential could reach somewhere it should not, listed under Security. Four +changes can affect existing code or scripts; they are listed under Changed. + +### Security + +- `githubauth token --exec` and `jwt --exec` no longer pass `$GITHUB_APP_PRIVATE_KEY` to + the command, nor to the `git` that reads a clone's origin, in any letter case on + Windows. A child that leaked its environment leaked the key that mints tokens for every + installation, past `--repos`, `--permissions` and revocation +- A `--key` that is neither PEM nor a readable file, such as a base64-encoded key from a CI + secret, was repeated in full in the error, and so in logs and agent transcripts. The + error no longer includes a value longer than a path +- `--exec` set `$GH_HOST` without the API URL's port, so `gh` sent the credential to port + 443 of that host. The port is kept, and a clone's https origin must name the same port + as the API for the token to be scoped to it +- An exported `$GH_HOST` naming another instance than an installation token's made `gh` + send the token there. `githubauth token --exec` now exits 2 before minting (see Changed). + The JWT is not checked: it is signed locally, and only you know which instance its App + lives on +- A SIGINT sent to githubauth alone, as a supervisor does, left the command running with the + credential. On Linux and macOS, unless the command is in the terminal's foreground job, + where a Ctrl-C reaches it directly, SIGINT is now passed on like SIGTERM +- Every GitHub Action in the workflows is pinned to a commit. The release job holds the + token that publishes the Homebrew cask and the gh extension, and a moved tag could have + run code with it. A successful API response is also capped at 16 MiB ### Added - `RevokeInstallationToken` ends an installation token before its hour is up. `githubauth token --exec` calls it once the command exits; `--keep` leaves the token valid. A revocation that fails is reported on stderr, the token then stays valid until - it expires, and the exit code stays the command's (#87, #88) + it expires, and the exit code stays the command's. A cached source that handed the token + out keeps returning it, so build a new one (#87, #88) - `githubauth token --permissions contents:read,pull_requests:write` scopes the token below what the App was granted. Any permission name GitHub has is accepted; GitHub refuses one it does not know with a 422, which exits 2 (#87, #89) @@ -46,6 +71,10 @@ code or scripts; they are listed under Changed. - **`githubauth token --exec` revokes the token when the command exits.** Anything the command leaves running on the token starts getting 401s. `--keep` restores the old behaviour (#87) +- **An exported `$GH_HOST` that differs from the installation token's instance exits 2** + under `token --exec`, before anything is minted. It used to be kept, which v1.11.0 suggested for a + `--base-url` proxy in front of github.com; set it inside the command instead: + `--exec -- env GH_HOST=github.com gh …` - **`InstallationPermissions` is no longer comparable**, because `Other` is a map. Code that compares two values with `==` or uses one as a map key no longer compiles (#88) - **Comparing installation token values, `*a == *b`, now panics** when GitHub's answer diff --git a/README.md b/README.md index 700c940..7fd084c 100644 --- a/README.md +++ b/README.md @@ -99,7 +99,8 @@ curl -H "Authorization: Bearer $(githubauth token)" \ Every flag falls back to an environment variable — `GITHUB_APP_CLIENT_ID`, `GITHUB_APP_PRIVATE_KEY`, `GITHUB_APP_INSTALLATION_ID` — so a configured CI step is just `githubauth token`. `--key` takes a file path, the PEM itself, or `-` to read stdin, which -keeps the key off disk: +keeps the key off disk. Pass the PEM itself through `$GITHUB_APP_PRIVATE_KEY` or stdin +rather than on the command line, where other local users can read it in the process list: ```bash vault kv get -field=pem secret/github-app | @@ -153,8 +154,11 @@ gh auth setup-git # once githubauth token --installation 12345 --exec -- git push ``` -With `--enterprise-url` or `--base-url`, `--exec` also sets `$GH_HOST`, so `gh` talks to -that instance instead of github.com. A `$GH_HOST` you exported yourself is kept. +With `--enterprise-url` or `--base-url`, `--exec` also sets `$GH_HOST`, port included, so +`gh` talks to that instance instead of github.com. A `$GH_HOST` you exported that names +another instance is refused before an installation token is minted, since `gh` would send +it there. If a proxy really serves it, set it inside the command: +`--exec -- env GH_HOST=github.com gh …`. The command never sees `$GITHUB_APP_PRIVATE_KEY`. The JWT needs one more step. `gh` sends its credential as `token`, and GitHub accepts a JWT only as `Bearer`, so pass the header yourself: diff --git a/auth.go b/auth.go index 8a9575b..7fc229a 100644 --- a/auth.go +++ b/auth.go @@ -476,7 +476,9 @@ func NewInstallationTokenSource(id int64, src oauth2.TokenSource, opts ...Instal } // RevokeInstallationToken revokes an installation access token before its hour -// is up, so a copy that leaked stops working. WithBaseURL, WithEnterpriseURL and +// is up, so a copy that leaked stops working. A source from +// NewInstallationTokenSource that handed the token out does not know, and keeps +// returning it from its cache until it expires; build a new source instead. WithBaseURL, WithEnterpriseURL and // WithHTTPClient apply as they do to NewInstallationTokenSource. The other // options are ignored. // diff --git a/cmd/githubauth/exec.go b/cmd/githubauth/exec.go index 21f23e6..9ee5b66 100644 --- a/cmd/githubauth/exec.go +++ b/cmd/githubauth/exec.go @@ -5,11 +5,13 @@ import ( "errors" "fmt" "io" + "net" "net/http" "net/url" "os" "os/exec" "os/signal" + "runtime" "slices" "strings" "syscall" @@ -44,21 +46,25 @@ func runWithToken(argv []string, tok *oauth2.Token, host string, stdin io.Reader } cmd := exec.Command(argv[0], argv[1:]...) - cmd.Env = environWithout(os.Environ(), all...) + // The child gets an installation token for an hour, never the App's key, + // which mints tokens for every installation until it is rotated. + cmd.Env = childEnviron() for _, key := range keys { cmd.Env = append(cmd.Env, key+"="+tok.AccessToken) } // Without it gh targets github.com, where this credential is useless. A - // GH_HOST the caller exported is theirs to keep. + // GH_HOST the caller exported was checked against host before minting. if host != "" && os.Getenv("GH_HOST") == "" { cmd.Env = append(cmd.Env, "GH_HOST="+host) } cmd.Stdin, cmd.Stdout, cmd.Stderr = stdin, stdout, stderr - // Ctrl-C already reaches the child through the terminal's process group, so - // SIGINT is only held to keep githubauth alive until the child exits. A - // SIGTERM is usually sent to githubauth alone, and is passed on so the - // child does not outlive it with the credential in its environment. + // A SIGTERM, or a SIGINT from a supervisor, is sent to githubauth alone, + // and is passed on so the child does not outlive it with the credential in + // its environment. A Ctrl-C at the keyboard reaches the child itself when + // the child is in the terminal's foreground job, so then SIGINT is only + // held, to keep githubauth alive until the child exits. + inFg := foreground sigs := make(chan os.Signal, 1) signal.Notify(sigs, os.Interrupt, syscall.SIGTERM) defer signal.Stop(sigs) @@ -75,7 +81,7 @@ func runWithToken(argv []string, tok *oauth2.Token, host string, stdin io.Reader for { select { case sig := <-sigs: - if sig == syscall.SIGTERM { + if sig == syscall.SIGTERM || !inFg(cmd.Process.Pid) { _ = cmd.Process.Signal(sig) } case <-done: @@ -154,24 +160,49 @@ func ghHost(apiURL string) string { } host := strings.ToLower(u.Hostname()) if rest, ok := strings.CutPrefix(host, "api."); ok && !isEnterpriseHost(rest) { - return rest + host = rest + } + // gh honours a port in GH_HOST. Without it the credential would go to + // whatever answers on 443 of that host. gh speaks only https, so only its + // default can go; and a host with a port is never rewritten, since gh then + // treats it as Enterprise Server and would not put api. back. + if port := u.Port(); port != "" && (u.Scheme != "https" || port != "443") { + return net.JoinHostPort(strings.ToLower(u.Hostname()), port) + } + // An IPv6 address needs its brackets back to be a host gh can parse. + if strings.Contains(host, ":") { + return "[" + host + "]" } return host } +// foreground is inForeground, replaceable by the tests, whose process group a +// developer's terminal may or may not hold. +var foreground = inForeground + // isEnterpriseHost mirrors gh's own test for whether a host reads the // enterprise variables: anything but github.com and data residency (*.ghe.com). func isEnterpriseHost(host string) bool { return host != "" && host != "github.com" && !strings.HasSuffix(host, ".github.com") && !strings.HasSuffix(host, ".ghe.com") } +// childEnviron is githubauth's environment without the App's private key or +// any token already exported, for every process githubauth starts. +func childEnviron() []string { + return environWithout(os.Environ(), append(append(slices.Clone(execTokenEnv), execEnterpriseEnv...), "GITHUB_APP_PRIVATE_KEY")...) +} + +// foldEnvNames is whether environment variable names ignore case, as on +// Windows, where github_app_private_key is the same variable. +var foldEnvNames = runtime.GOOS == "windows" + // environWithout drops keys from env so the token cannot be shadowed by one // the caller already exported. func environWithout(env []string, keys ...string) []string { out := make([]string, 0, len(env)) for _, kv := range env { name, _, _ := strings.Cut(kv, "=") - if !slices.Contains(keys, name) { + if !slices.ContainsFunc(keys, func(k string) bool { return k == name || foldEnvNames && strings.EqualFold(k, name) }) { out = append(out, kv) } } diff --git a/cmd/githubauth/exec_test.go b/cmd/githubauth/exec_test.go index 1241480..49c8801 100644 --- a/cmd/githubauth/exec_test.go +++ b/cmd/githubauth/exec_test.go @@ -24,8 +24,10 @@ func TestExecHelper(t *testing.T) { t.Skip("helper process for the --exec tests") } - for _, key := range append(execTokenEnv, append(execEnterpriseEnv, "GH_HOST")...) { - fmt.Printf("%s=%s\n", key, os.Getenv(key)) + if os.Getenv("GITHUBAUTH_EXEC_QUIET") == "" { + for _, key := range append(execTokenEnv, append(execEnterpriseEnv, "GH_HOST", "GITHUB_APP_PRIVATE_KEY")...) { + fmt.Printf("%s=%s\n", key, os.Getenv(key)) + } } // Use the token the way a real command would, so a test can tell whether it // was still valid while the command ran. @@ -71,6 +73,37 @@ func TestRunWithToken(t *testing.T) { } }) + t.Run("githubauth itself writes nothing", func(t *testing.T) { + t.Setenv("GITHUBAUTH_EXEC_HELPER", "1") + t.Setenv("GITHUBAUTH_EXEC_CODE", "0") + t.Setenv("GITHUBAUTH_EXEC_QUIET", "1") + + var stdout, stderr bytes.Buffer + if err := runWithToken(helperArgv(), tok, "", strings.NewReader(""), &stdout, &stderr); err != nil { + t.Fatalf("runWithToken() error = %v", err) + } + // The test binary's own PASS line is all a silent child leaves. + if strings.Contains(stdout.String()+stderr.String(), "ghs_secret") { + t.Errorf("output = %q / %q, want the token nowhere", stdout.String(), stderr.String()) + } + }) + + // The child needs the token for an hour, not the key that mints tokens for + // every installation. + t.Run("the App's private key does not reach the child", func(t *testing.T) { + t.Setenv("GITHUBAUTH_EXEC_HELPER", "1") + t.Setenv("GITHUBAUTH_EXEC_CODE", "0") + t.Setenv("GITHUB_APP_PRIVATE_KEY", "-----BEGIN RSA PRIVATE KEY-----secret") + + var stdout, stderr bytes.Buffer + if err := runWithToken(helperArgv(), tok, "", strings.NewReader(""), &stdout, &stderr); err != nil { + t.Fatalf("runWithToken() error = %v", err) + } + if !strings.Contains(stdout.String(), "GITHUB_APP_PRIVATE_KEY=\n") { + t.Errorf("child stdout = %q, want $GITHUB_APP_PRIVATE_KEY empty", stdout.String()) + } + }) + t.Run("a token already in the environment is replaced", func(t *testing.T) { t.Setenv("GITHUBAUTH_EXEC_HELPER", "1") t.Setenv("GITHUBAUTH_EXEC_CODE", "0") @@ -197,6 +230,32 @@ func TestRunWithToken(t *testing.T) { } }) + // A supervisor stopping githubauth with SIGINT, with no terminal to have + // sent the child its own, would otherwise leave the child running. + t.Run("SIGINT is forwarded to the child outside the terminal's foreground", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("no signals on windows") + } + foreground = func(int) bool { return false } + t.Cleanup(func() { foreground = inForeground }) + t.Setenv("GITHUBAUTH_EXEC_HELPER", "1") + t.Setenv("GITHUBAUTH_EXEC_SLEEP", "1") + + go func() { + time.Sleep(500 * time.Millisecond) + p, _ := os.FindProcess(os.Getpid()) + _ = p.Signal(os.Interrupt) + }() + + var stdout, stderr bytes.Buffer + err := runWithToken(helperArgv(), tok, "", strings.NewReader(""), &stdout, &stderr) + + var status exitStatus + if !errors.As(err, &status) || int(status) != 128+int(syscall.SIGINT) { + t.Errorf("runWithToken() error = %v, want exitStatus %d", err, 128+int(syscall.SIGINT)) + } + }) + // A SIGTERM sent to githubauth alone would otherwise leave the child // running with the credential in its environment. t.Run("SIGTERM is forwarded to the child", func(t *testing.T) { @@ -239,7 +298,15 @@ func TestGhHost(t *testing.T) { {apiURL: "https://github.example.com", want: "github.example.com"}, // An Enterprise Server named api.* is that host, not a subdomain of it. {apiURL: "https://api.example.com/api/v3/", want: "api.example.com"}, - {apiURL: "https://ghe.example.com:8443/api/v3/", want: "ghe.example.com"}, + // gh honours the port, and without it the credential goes to 443. + {apiURL: "https://ghe.example.com:8443/api/v3/", want: "ghe.example.com:8443"}, + {apiURL: "https://ghe.example.com:443/api/v3/", want: "ghe.example.com"}, + {apiURL: "http://[::1]:8080/", want: "[::1]:8080"}, + {apiURL: "https://[::1]/api/v3/", want: "[::1]"}, + // gh speaks https, so an http port is kept and fails rather than moves. + {apiURL: "http://ghe.example.com:80/", want: "ghe.example.com:80"}, + // With a port gh would not restore api., so the host is left whole. + {apiURL: "https://api.octo.ghe.com:8443/", want: "api.octo.ghe.com:8443"}, } for _, tt := range tests { @@ -267,3 +334,16 @@ func TestEnvironWithout(t *testing.T) { } } } + +// TestEnvironWithoutFoldsWhereTheOSDoes checks the private key is dropped +// whatever its case on Windows, where the OS reads it case-insensitively. +func TestEnvironWithoutFoldsWhereTheOSDoes(t *testing.T) { + was := foldEnvNames + foldEnvNames = true + t.Cleanup(func() { foldEnvNames = was }) + + got := environWithout([]string{"github_app_private_key=pem", "PATH=/bin"}, "GITHUB_APP_PRIVATE_KEY") + if len(got) != 1 || got[0] != "PATH=/bin" { + t.Errorf("environWithout() = %q, want only PATH", got) + } +} diff --git a/cmd/githubauth/foreground_other.go b/cmd/githubauth/foreground_other.go new file mode 100644 index 0000000..011091e --- /dev/null +++ b/cmd/githubauth/foreground_other.go @@ -0,0 +1,8 @@ +//go:build !darwin && !linux + +package main + +// inForeground reports false where there is no process group to ask, so a +// signal is always passed on. On Windows the Go runtime cannot pass it on, and +// a console's Ctrl-C reaches every process attached to it instead. +func inForeground(int) bool { return false } diff --git a/cmd/githubauth/foreground_unix.go b/cmd/githubauth/foreground_unix.go new file mode 100644 index 0000000..1b39fc2 --- /dev/null +++ b/cmd/githubauth/foreground_unix.go @@ -0,0 +1,32 @@ +//go:build darwin || linux + +package main + +import ( + "os" + "syscall" + "unsafe" +) + +// inForeground reports whether process pid belongs to the terminal's +// foreground job, the one a Ctrl-C at the keyboard is delivered to. +func inForeground(pid int) bool { + pgid, err := syscall.Getpgid(pid) + if err != nil { + return false + } + tty, err := os.Open("/dev/tty") + if err != nil { + return false + } + defer func() { _ = tty.Close() }() + return foregroundOf(tty.Fd(), pgid) +} + +// foregroundOf reports whether the terminal open on fd has process group pgid +// in the foreground. A file that is not a terminal has none. +func foregroundOf(fd uintptr, pgid int) bool { + var pgrp int32 + _, _, errno := syscall.Syscall(syscall.SYS_IOCTL, fd, uintptr(syscall.TIOCGPGRP), uintptr(unsafe.Pointer(&pgrp))) + return errno == 0 && int(pgrp) == pgid +} diff --git a/cmd/githubauth/foreground_unix_test.go b/cmd/githubauth/foreground_unix_test.go new file mode 100644 index 0000000..872c271 --- /dev/null +++ b/cmd/githubauth/foreground_unix_test.go @@ -0,0 +1,43 @@ +//go:build darwin || linux + +package main + +import ( + "os" + "os/exec" + "strings" + "syscall" + "testing" +) + +func TestForegroundOfNonTerminal(t *testing.T) { + f, err := os.Open(os.DevNull) + if err != nil { + t.Fatal(err) + } + defer func() { _ = f.Close() }() + + // /dev/null is a character device, which is what fooled a stat-based check. + if foregroundOf(f.Fd(), syscall.Getpgrp()) { + t.Error("foregroundOf(/dev/null) = true, want false: it is no terminal") + } +} + +// TestInForegroundWithoutTerminal runs the probe in a new session, which has +// no controlling terminal, as under a supervisor: no Ctrl-C can reach the +// child there, so a SIGINT must be passed on. +func TestInForegroundWithoutTerminal(t *testing.T) { + if os.Getenv("GITHUBAUTH_FOREGROUND_PROBE") != "" { + if inForeground(os.Getpid()) { + os.Exit(3) + } + os.Exit(0) + } + + cmd := exec.Command(os.Args[0], "-test.run=^TestInForegroundWithoutTerminal$") + cmd.Env = append(os.Environ(), "GITHUBAUTH_FOREGROUND_PROBE=1") + cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} + if out, err := cmd.CombinedOutput(); err != nil { + t.Errorf("inForeground() in a session without a terminal = true or failed: %v %s", err, strings.TrimSpace(string(out))) + } +} diff --git a/cmd/githubauth/main.go b/cmd/githubauth/main.go index 1f4e97c..abe64f9 100644 --- a/cmd/githubauth/main.go +++ b/cmd/githubauth/main.go @@ -14,6 +14,7 @@ import ( "flag" "fmt" "io" + "net" "net/url" "os" "os/exec" @@ -165,7 +166,7 @@ func (a *appFlags) bind(fs *flag.FlagSet, out *output) { fs.StringVar(&a.clientID, "client-id", os.Getenv("GITHUB_APP_CLIENT_ID"), "App client ID, e.g. Iv1.1234567890abcdef ($GITHUB_APP_CLIENT_ID)") fs.Int64Var(&a.appID, "app-id", envInt64("GITHUB_APP_ID"), "legacy numeric App ID, for an App with no client ID ($GITHUB_APP_ID)") - fs.StringVar(&a.key, "key", os.Getenv("GITHUB_APP_PRIVATE_KEY"), "PEM private key: file path, \"-\" for stdin, or the PEM ($GITHUB_APP_PRIVATE_KEY)") + fs.StringVar(&a.key, "key", os.Getenv("GITHUB_APP_PRIVATE_KEY"), "PEM private key: file path, \"-\" for stdin, or the PEM, best from $GITHUB_APP_PRIVATE_KEY since argv is visible to other users") fs.BoolVar(&out.json, "json", false, "print {\"token\":...,\"expires_at\":...} instead of the bare token, with the permissions and repositories GitHub granted an installation token") fs.BoolVar(&a.exec, "exec", false, "run the command after -- with the credential in $GITHUB_TOKEN and $GH_TOKEN, printing it nowhere; $GH_HOST is set for an Enterprise or data residency host") fs.BoolVar(&out.agent, "agent", out.agent, "report failures as one JSON document on stdout (on when a coding agent is detected)") @@ -186,6 +187,20 @@ func (a *appFlags) checkExec(argv []string) error { return nil } +// checkGHHost refuses an exported GH_HOST that names another instance than +// the one an installation token is for, before anything is minted: gh would +// send the token there. The JWT is not checked: it is signed locally, and only +// the caller knows which instance its App lives on. A caller whose proxy really serves it sets GH_HOST inside +// the command instead, where nothing else in the environment can choose it. +func (a *appFlags) checkGHHost() error { + want := cmp.Or(a.host, "github.com") + // Compared exactly: gh treats github.com:443 as another host than github.com. + if h := os.Getenv("GH_HOST"); a.exec && h != "" && !strings.EqualFold(h, want) { + return usagef("$GH_HOST is %s but the credential is for %s, so gh would send it to %s: unset GH_HOST, or set it inside the command, --exec -- env GH_HOST=%s gh ..., if %s serves this credential", h, want, h, h, h) + } + return nil +} + // emit delivers the credential: into a child process under --exec, otherwise // to stdout. func (a *appFlags) emit(tok *oauth2.Token, argv []string, stdin io.Reader, stdout, stderr io.Writer) error { @@ -304,6 +319,9 @@ func tokenCmd(args []string, stdin io.Reader, stdout, stderr io.Writer, out *out return err } app.host = ghHost(*baseURL + *enterpriseURL) // at most one is set + if err := app.checkGHHost(); err != nil { + return err + } // Nothing names the installation, so the clone the command runs in does, // the way actions/create-github-app-token defaults to the current // repository. The token covers that repository alone. @@ -522,6 +540,10 @@ func printFlags(w io.Writer, fs *flag.FlagSet) { } } +// maxKeyPathLen is the longest --key value repeated in an error. A private +// key, PEM or base64, is well over it. +const maxKeyPathLen = 256 + // readKey resolves --key, which is a file path, "-" for stdin, or the PEM // itself so that $GITHUB_APP_PRIVATE_KEY can be passed straight through. func readKey(v string, stdin io.Reader) ([]byte, error) { @@ -534,7 +556,13 @@ func readKey(v string, stdin io.Reader) ([]byte, error) { pem, err := os.ReadFile(v) if err != nil { // The flag was well-formed but points nowhere, which is the caller's - // invocation to fix rather than anything GitHub was asked about. + // invocation to fix rather than anything GitHub was asked about. A key + // that is not PEM, such as base64 from a CI secret, lands here too, so + // the value is named only when it is short enough to be a path. + var pe *os.PathError + if errors.As(err, &pe) && len(v) > maxKeyPathLen { + return nil, usagef("reading private key: --key is neither PEM, %q, nor a file it can open (%v); a base64-encoded key needs decoding first", "-", pe.Err) + } return nil, usagef("reading private key: %v", err) } return pem, nil @@ -594,12 +622,28 @@ func splitOwner(names []string) ([]string, string, error) { // its origin remote, when that remote is on host, as gh names it: empty for // github.com. A clone of another host says nothing about this one. func originRepo(host string) (owner, repo string, ok bool) { - out, err := exec.Command("git", "remote", "get-url", "origin").Output() + git := exec.Command("git", "remote", "get-url", "origin") + git.Env = childEnviron() + out, err := git.Output() if err != nil { return "", "", false } remoteHost, owner, repo, ok := parseRemote(strings.TrimSpace(string(out))) - if !ok || !strings.EqualFold(remoteHost, cmp.Or(host, "github.com")) { + want := cmp.Or(host, "github.com") + // An http(s) remote is served by the instance itself, so its port has to + // agree too, git push would hand the token to it; gh's port is 443 unless + // the API names another. An ssh remote's port says nothing about the + // API's, so only the host is compared. + wantHost, wantPort, err := net.SplitHostPort(want) + if err != nil { + wantHost, wantPort = strings.Trim(want, "[]"), "443" + } + if h, p, err := net.SplitHostPort(remoteHost); err == nil { + ok = ok && strings.EqualFold(h, wantHost) && p == wantPort + } else { + ok = ok && strings.EqualFold(remoteHost, wantHost) + } + if !ok { return "", "", false } return owner, repo, true @@ -611,6 +655,11 @@ func parseRemote(remote string) (host, owner, repo string, ok bool) { var path string if u, err := url.Parse(remote); err == nil && u.Scheme != "" && u.Host != "" { host, path = u.Hostname(), u.Path + // An http(s) remote is served by the instance itself, so it carries its + // port, default included; see originRepo. + if u.Scheme == "http" || u.Scheme == "https" { + host = net.JoinHostPort(host, cmp.Or(u.Port(), map[string]string{"http": "80", "https": "443"}[u.Scheme])) + } } else if userHost, p, found := strings.Cut(remote, ":"); found && !strings.Contains(userHost, "/") { _, host, _ = strings.Cut(userHost, "@") host = cmp.Or(host, userHost) diff --git a/cmd/githubauth/main_test.go b/cmd/githubauth/main_test.go index 913ba92..d615dca 100644 --- a/cmd/githubauth/main_test.go +++ b/cmd/githubauth/main_test.go @@ -57,6 +57,7 @@ func clearEnv(t *testing.T) { _ = os.Unsetenv(key) } t.Setenv("GIT_CONFIG_GLOBAL", os.DevNull) + t.Setenv("GH_HOST", "") t.Setenv("GIT_CONFIG_NOSYSTEM", "1") } @@ -546,6 +547,47 @@ func TestRun(t *testing.T) { wantStdout: isEmpty, wantStderr: `--owner ".." is not an account`, }, + { + // A base64 key, the usual CI-secret form, is read as a path; the + // error must not repeat it into a log. + name: "a key that is not PEM is not echoed", + args: func(string) []string { + return []string{"token", "--client-id", "Iv1.abc", "--key", strings.Repeat("LS0tLS1CRUdJTi", 30), "--installation", "9"} + }, + wantCode: exitUsage, + wantStdout: isEmpty, + wantStderr: "a base64-encoded key needs decoding first", + }, + { + name: "a missing key file is still named", + args: func(string) []string { + return []string{"token", "--client-id", "Iv1.abc", "--key", "no-such-app.pem", "--installation", "9"} + }, + wantCode: exitUsage, + wantStdout: isEmpty, + wantStderr: "no-such-app.pem", + }, + { + name: "an exported GH_HOST for another instance is refused before minting", + args: func(url string) []string { + return []string{"token", "--client-id", "Iv1.abc", "--key", keyPath, "--installation", "9", "--base-url", url, "--exec", "--", "true"} + }, + env: map[string]string{"GH_HOST": "github.com"}, + setupServer: statusHandler(http.StatusInternalServerError), + wantCode: exitUsage, + wantStdout: isEmpty, + wantStderr: "$GH_HOST is github.com but the credential is for 127.0.0.1:", + }, + { + // Only the caller knows which instance a JWT's App lives on. + name: "an exported GH_HOST is left to the JWT", + args: func(string) []string { + return append([]string{"jwt", "--client-id", "Iv1.abc", "--key", keyPath, "--exec", "--"}, helperArgv()...) + }, + env: map[string]string{"GH_HOST": "ghe.example.com", "GITHUBAUTH_EXEC_HELPER": "1", "GITHUBAUTH_EXEC_CODE": "0"}, + wantCode: 0, + wantStdout: contains("GH_HOST=ghe.example.com"), + }, { name: "keep without exec is misuse", args: func(string) []string { @@ -843,6 +885,12 @@ func TestTokenRequests(t *testing.T) { wantMint: "/app/installations/123/access_tokens", wantBody: `{"repositories":["api"]}`, }, + { + // git push would send the token to the remote's port, not the API's. + name: "a clone's https origin on another port is ignored", + origin: func(u string) string { return strings.Replace(u, "127.0.0.1:", "127.0.0.1:1", 1) + "/acme/api.git" }, + wantCode: exitUsage, + }, { name: "a clone's origin on another host is ignored", origin: func(string) string { return "git@github.com:acme/api.git" }, @@ -1366,14 +1414,16 @@ func TestParseRemote(t *testing.T) { host, owner, repo string ok bool }{ - {remote: "https://github.com/acme/api.git", host: "github.com", owner: "acme", repo: "api", ok: true}, - {remote: "https://github.com/acme/api", host: "github.com", owner: "acme", repo: "api", ok: true}, - {remote: "https://x-access-token:secret@github.com/acme/api.git", host: "github.com", owner: "acme", repo: "api", ok: true}, + {remote: "https://github.com/acme/api.git", host: "github.com:443", owner: "acme", repo: "api", ok: true}, + {remote: "https://github.com/acme/api", host: "github.com:443", owner: "acme", repo: "api", ok: true}, + {remote: "https://x-access-token:secret@github.com/acme/api.git", host: "github.com:443", owner: "acme", repo: "api", ok: true}, {remote: "git@github.com:acme/api.git", host: "github.com", owner: "acme", repo: "api", ok: true}, {remote: "github.com:acme/api", host: "github.com", owner: "acme", repo: "api", ok: true}, {remote: "ssh://git@ghe.example.com:2222/acme/api.git", host: "ghe.example.com", owner: "acme", repo: "api", ok: true}, + {remote: "https://ghe.example.com:9443/acme/api.git", host: "ghe.example.com:9443", owner: "acme", repo: "api", ok: true}, + {remote: "https://ghe.example.com:443/acme/api.git", host: "ghe.example.com:443", owner: "acme", repo: "api", ok: true}, {remote: "ssh://git@ssh.github.com:443/acme/api.git", host: "github.com", owner: "acme", repo: "api", ok: true}, - {remote: "https://octo.ghe.com/acme/api/", host: "octo.ghe.com", owner: "acme", repo: "api", ok: true}, + {remote: "https://octo.ghe.com/acme/api/", host: "octo.ghe.com:443", owner: "acme", repo: "api", ok: true}, {remote: "https://github.com/acme"}, {remote: "https://github.com/acme/api/tree/main"}, {remote: "/srv/git/api.git"}, @@ -1393,3 +1443,79 @@ func TestParseRemote(t *testing.T) { }) } } + +// TestReadKeyDoesNotEchoKey checks the whole value of a key that is not PEM +// stays out of the error, whatever renders it. +func TestReadKeyDoesNotEchoKey(t *testing.T) { + t.Parallel() + + key := strings.Repeat("LS0tLS1CRUdJTi", 30) + _, err := readKey(key, strings.NewReader("")) + if err == nil || strings.Contains(err.Error(), key[:40]) { + t.Errorf("readKey() err = %v, want an error without the key", err) + } +} + +// TestOriginRepoKeepsTheKeyFromGit checks the git that reads the origin remote +// does not get the App's key either: a wrapper logging its environment would +// otherwise leak it. +func TestOriginRepoKeepsTheKeyFromGit(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the fake git is a shell script") + } + dir := t.TempDir() + seen := filepath.Join(dir, "seen") + script := "#!/bin/sh\nprintf %s \"$GITHUB_APP_PRIVATE_KEY\" > " + seen + "\necho https://github.com/acme/api.git\n" + if err := os.WriteFile(filepath.Join(dir, "git"), []byte(script), 0o700); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir) + t.Setenv("GITHUB_APP_PRIVATE_KEY", "-----BEGIN RSA PRIVATE KEY-----secret") + + if owner, repo, ok := originRepo(""); !ok || owner != "acme" || repo != "api" { + t.Fatalf("originRepo() = %q, %q, %v, want acme, api, true", owner, repo, ok) + } + if got, _ := os.ReadFile(seen); len(got) != 0 { + t.Errorf("git saw $GITHUB_APP_PRIVATE_KEY = %q, want it empty", got) + } +} + +// TestOriginRepoHost checks which clones may name the repository for a token: +// an http(s) remote is the instance itself, so its port must be the API's. +func TestOriginRepoHost(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the fake git is a shell script") + } + + tests := []struct { + remote, apiHost string + ok bool + }{ + {remote: "https://github.com/acme/api.git", apiHost: "", ok: true}, + {remote: "git@github.com:acme/api.git", apiHost: "", ok: true}, + {remote: "ssh://git@ssh.github.com:443/acme/api.git", apiHost: "", ok: true}, + {remote: "https://GHE.example.com/acme/api.git", apiHost: "ghe.example.com", ok: true}, + {remote: "ssh://git@ghe.example.com:2222/acme/api.git", apiHost: "ghe.example.com:8443", ok: true}, + {remote: "https://ghe.example.com:8443/acme/api.git", apiHost: "ghe.example.com:8443", ok: true}, + // git push would hand the token to the service on 443. + {remote: "https://ghe.example.com/acme/api.git", apiHost: "ghe.example.com:8443"}, + {remote: "https://ghe.example.com:9443/acme/api.git", apiHost: "ghe.example.com:8443"}, + {remote: "https://github.com/acme/api.git", apiHost: "ghe.example.com"}, + {remote: "https://[::1]/acme/api.git", apiHost: "[::1]", ok: true}, + } + + for _, tt := range tests { + t.Run(tt.remote+"@"+tt.apiHost, func(t *testing.T) { + dir := t.TempDir() + script := "#!/bin/sh\necho '" + tt.remote + "'\n" + if err := os.WriteFile(filepath.Join(dir, "git"), []byte(script), 0o700); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir) + + if _, _, ok := originRepo(tt.apiHost); ok != tt.ok { + t.Errorf("originRepo(%q) with origin %s: ok = %v, want %v", tt.apiHost, tt.remote, ok, tt.ok) + } + }) + } +} diff --git a/github.go b/github.go index 0f2e255..0cd4e46 100644 --- a/github.go +++ b/github.go @@ -24,6 +24,11 @@ const ( // message, so a misbehaving or hostile server cannot exhaust memory. maxErrorBodyBytes = 64 << 10 + // maxResponseBytes caps a successful response. A token scoped to the most + // repositories GitHub allows lists each in full, a few MiB; a server or + // proxy sending more than this is not answering the request. + maxResponseBytes = 16 << 20 + // maxRetrySleep is the longest wait the client sleeps through before its one // retry. A longer hint is returned to the caller instead: retrying before // GitHub said to is bound to fail, and Token() would stall for nothing. @@ -371,7 +376,7 @@ func (c *githubClient) doCreateInstallationToken(ctx context.Context, reqURL str if resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusCreated { var token InstallationToken - if err := json.NewDecoder(resp.Body).Decode(&token); err != nil { + if err := json.NewDecoder(io.LimitReader(resp.Body, maxResponseBytes)).Decode(&token); err != nil { return nil, fmt.Errorf("failed to decode response: %w", err) } return &token, nil @@ -433,7 +438,7 @@ func (c *githubClient) findInstallationID(ctx context.Context, owner string) (in var installation struct { ID int64 `json:"id"` } - if err := json.NewDecoder(resp.Body).Decode(&installation); err != nil { + if err := json.NewDecoder(io.LimitReader(resp.Body, maxResponseBytes)).Decode(&installation); err != nil { return 0, fmt.Errorf("failed to decode response: %w", err) } // Minting with 0 would fail as a missing ID and hide this answer. diff --git a/github_test.go b/github_test.go index 969db11..314d107 100644 --- a/github_test.go +++ b/github_test.go @@ -1523,6 +1523,35 @@ func TestRevokeInstallationToken(t *testing.T) { } } +// Test_createInstallationToken_BoundsResponse checks a server sending more than +// any real answer cannot make the client read it all. +func Test_createInstallationToken_BoundsResponse(t *testing.T) { + t.Parallel() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusCreated) + _, _ = io.WriteString(w, `{"token":"`) + _, _ = io.Copy(w, io.LimitReader(repeatReader('a'), maxResponseBytes+1)) + _, _ = io.WriteString(w, `"}`) + })) + defer server.Close() + + _, err := newClientForServer(t, server).createInstallationToken(context.Background(), 1, nil) + if err == nil || !strings.Contains(err.Error(), "failed to decode response") { + t.Errorf("err = %v, want the oversized response refused", err) + } +} + +// repeatReader is an endless stream of one byte. +type repeatReader byte + +func (r repeatReader) Read(p []byte) (int, error) { + for i := range p { + p[i] = byte(r) + } + return len(p), nil +} + // Test_createInstallationToken_RefusesAmbiguousPermission checks a permission // named twice is refused before anything is sent, rather than one silently // winning. diff --git a/llms.txt b/llms.txt index eab2759..f1a4968 100644 --- a/llms.txt +++ b/llms.txt @@ -65,9 +65,9 @@ The same credentials without writing Go. Install with `go install github.com/jfe - `githubauth jwt [flags]` — print the App JWT, for the endpoints that take one. Flags: `--expiry`. - Shared flags: `--client-id` (or `--app-id` for a legacy App), `--key`, `--json` (adds the expiry, and for an installation token the permissions and repositories GitHub granted), `--exec`, `--agent`. `--repos acme/api` finds the installation like `--owner acme`. With no `--installation`, `--owner`, `--repos` or `$GITHUB_APP_INSTALLATION_ID`, a clone's `origin` remote on the same host scopes the token to that repository. - Environment fallbacks for every flag: `GITHUB_APP_CLIENT_ID`, `GITHUB_APP_ID`, `GITHUB_APP_PRIVATE_KEY`, `GITHUB_APP_INSTALLATION_ID`. A configured environment reduces the call to `githubauth token`. -- `--key` accepts a file path, `-` for stdin, or the PEM itself, so the key can come from a secret manager without touching disk. +- `--key` accepts a file path, `-` for stdin, or the PEM itself, so the key can come from a secret manager without touching disk. Pass the PEM itself through `$GITHUB_APP_PRIVATE_KEY` or stdin, not argv, which the process list shows. - The token is the only thing on stdout, so it composes: `curl -H "Authorization: Bearer $(githubauth token)" https://api.github.com/installation/repositories`. -- `--exec` runs a command with the credential in its `$GITHUB_TOKEN` and `$GH_TOKEN` and prints it nowhere, which keeps it out of transcripts, logs and scrollback: `githubauth token --installation 12345 --exec -- gh pr list`. The exit code is then the command's own. An installation token is revoked when the command exits; `--keep` leaves it valid. After `gh auth setup-git`, `--exec -- git push` pushes as the App. With `--enterprise-url` or `--base-url` it also sets `$GH_HOST` unless already set. `gh` sends `token`, and GitHub takes a JWT only as `Bearer`, so pass it as `githubauth jwt --exec -- sh -c 'gh api -H "Authorization: Bearer $GITHUB_TOKEN" /app'`. +- `--exec` runs a command with the credential in its `$GITHUB_TOKEN` and `$GH_TOKEN` and prints it nowhere, which keeps it out of transcripts, logs and scrollback: `githubauth token --installation 12345 --exec -- gh pr list`. The exit code is then the command's own. An installation token is revoked when the command exits; `--keep` leaves it valid. After `gh auth setup-git`, `--exec -- git push` pushes as the App. With `--enterprise-url` or `--base-url` it also sets `$GH_HOST`, port included; an exported `$GH_HOST` naming another instance than the installation token's exits 2 before minting. The command never sees `$GITHUB_APP_PRIVATE_KEY`. `gh` sends `token`, and GitHub takes a JWT only as `Bearer`, so pass it as `githubauth jwt --exec -- sh -c 'gh api -H "Authorization: Bearer $GITHUB_TOKEN" /app'`. - Exit codes: 0 printed a credential, 1 general failure (retrying may help), 2 bad invocation, 3 GitHub refused the key or the App's permissions, 4 rate limited (wait and repeat), 5 the App is not installed where it was asked to be. Branch on these rather than on message text. - Under `--json`, or when a coding agent is detected, a failure is one JSON document on stdout and stderr stays empty: `{"type":"githubauth.error","schema_version":"1","error":{"summary":...,"exit_code":5,"status_code":404,"retry_after_seconds":...,"suggestions":[...]}}`. Agent detection reads `CLAUDECODE`, `CLAUDE_CODE`, `CURSOR_AGENT`, `GITHUB_COPILOT`, `AMAZON_Q`, `OPENCODE`, `PI_CODING_AGENT`. `GITHUBAUTH_AGENT_MODE` or `--agent`/`--agent=false` overrides it. Set `GITHUBAUTH_AGENT_MODE=0` in a test suite that shells out to the CLI. Agent mode never changes the success output — a bare token stays a bare token.