diff --git a/internal/gitclone/command.go b/internal/gitclone/command.go index 891b2314..0958fdb9 100644 --- a/internal/gitclone/command.go +++ b/internal/gitclone/command.go @@ -5,48 +5,160 @@ package gitclone import ( "bufio" "context" + "os" "os/exec" + "path/filepath" "strings" + "sync" + "time" "github.com/alecthomas/errors" + + "github.com/block/cachew/internal/logging" ) +// credentialFileRefreshInterval beats the GitHub App token's 1 h TTL with +// margin. Var (not const) so tests can shrink it. +var credentialFileRefreshInterval = 30 * time.Second //nolint:gochecknoglobals // test seam + // GitCommand returns a git subprocess configured with repository-scoped -// authentication and any per-URL git config overrides disabled. -func (r *Repository) GitCommand(ctx context.Context, args ...string) (*exec.Cmd, error) { - repoURL := r.upstreamURL - var token string - if r.credentialProvider != nil && strings.Contains(repoURL, "github.com") { - var err error - token, err = r.credentialProvider.GetTokenForURL(ctx, repoURL) - // If error getting token, fall back to original URL (system credentials) - if err != nil { - token = "" +// authentication. Callers MUST invoke cleanup (typically via defer) once +// the command has finished. cleanup is always non-nil and idempotent. +func (r *Repository) GitCommand(ctx context.Context, args ...string) (*exec.Cmd, func(), error) { + cleanup := func() {} + + configArgs, err := getInsteadOfDisableArgsForURL(ctx, r.upstreamURL) + if err != nil { + return nil, cleanup, errors.Wrap(err, "get insteadOf disable args") + } + + var allArgs []string + allArgs = append(allArgs, configArgs...) + + if r.credentialProvider != nil && strings.Contains(r.upstreamURL, "github.com") { + token, err := r.credentialProvider.GetTokenForURL(ctx, r.upstreamURL) + if err == nil && token != "" { + credFile, fileCleanup, err := r.startTokenCredentialFile(ctx, token) + if err != nil { + return nil, cleanup, errors.Wrap(err, "start token credential file") + } + cleanup = fileCleanup + // Shell-form (`!cmd`) re-reads credFile on every credential query + // so refreshes take effect mid-subprocess. The f() wrapper gates + // on the op arg git appends (`get`/`store`/`erase`), so a worktree + // file named `get` can't be cat'd as a token. + allArgs = append(allArgs, "-c", + "credential.helper=!f() { test \"$1\" = get && cat "+shellSingleQuote(credFile)+"; }; f") } } - configArgs, err := getInsteadOfDisableArgsForURL(ctx, repoURL) + allArgs = append(allArgs, args...) + + return exec.CommandContext(ctx, "git", allArgs...), cleanup, nil +} + +// startTokenCredentialFile creates a 0600 credential file and spawns a +// goroutine that rewrites it on token rotation until cleanup or ctx ends. +func (r *Repository) startTokenCredentialFile(ctx context.Context, initialToken string) (string, func(), error) { + f, err := os.CreateTemp("", "cachew-git-cred-*") if err != nil { - return nil, errors.Wrap(err, "get insteadOf disable args") + return "", func() {}, errors.Wrap(err, "create credential file") + } + path := f.Name() + if err := f.Close(); err != nil { + _ = os.Remove(path) //nolint:gosec // path is from os.CreateTemp + return "", func() {}, errors.Wrap(err, "close credential file") + } + if err := os.Chmod(path, 0o600); err != nil { //nolint:gosec // path is from os.CreateTemp + _ = os.Remove(path) //nolint:gosec // path is from os.CreateTemp + return "", func() {}, errors.Wrap(err, "chmod credential file") + } + if err := writeCredentialFile(path, initialToken); err != nil { + _ = os.Remove(path) //nolint:gosec // path is from os.CreateTemp + return "", func() {}, err } - var allArgs []string - if len(configArgs) > 0 { - allArgs = append(allArgs, configArgs...) + refreshCtx, cancel := context.WithCancel(ctx) + var wg sync.WaitGroup + wg.Go(func() { r.refreshCredentialFile(refreshCtx, path, initialToken) }) + + // Wait for any in-flight refresh tick before removing the file so it + // can't rename a new token into place after we've deleted the old one. + var once sync.Once + cleanup := func() { + once.Do(func() { + cancel() + wg.Wait() + _ = os.Remove(path) //nolint:gosec // path is from os.CreateTemp + }) } - // Add credential helper configuration if we have a token - // This ensures git uses the GitHub App token for authentication - // for all operations (clone, fetch, remote update, etc.) - if token != "" { - escapedToken := strings.ReplaceAll(token, "'", "'\\''") - credHelper := "!f() { test \"$1\" = get && echo username=x-access-token && printf 'password=%s\\n' '" + escapedToken + "'; }; f" - allArgs = append(allArgs, "-c", "credential.helper="+credHelper) + return path, cleanup, nil +} + +func (r *Repository) refreshCredentialFile(ctx context.Context, path, current string) { + logger := logging.FromContext(ctx).With("upstream", r.upstreamURL, "cred_file", path) + ticker := time.NewTicker(credentialFileRefreshInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + next, changed, err := r.refreshCredentialFileOnce(ctx, path, current) + switch { + case err != nil: + logger.WarnContext(ctx, "Failed to refresh git credential file", "error", err) + case changed: + logger.DebugContext(ctx, "Git credential file refreshed with rotated token") + current = next + } + } } +} - allArgs = append(allArgs, args...) +func (r *Repository) refreshCredentialFileOnce(ctx context.Context, path, current string) (string, bool, error) { + token, err := r.credentialProvider.GetTokenForURL(ctx, r.upstreamURL) + if err != nil { + return current, false, errors.Wrap(err, "fetch token") + } + if token == "" || token == current { + return current, false, nil + } + if err := writeCredentialFile(path, token); err != nil { + return current, false, err + } + return token, true, nil +} + +// writeCredentialFile atomically rotates the credential file at path to +// contain token. Uses os.CreateTemp (not a .new sibling) so a planted +// symlink can't redirect the write. +func writeCredentialFile(path, token string) error { + body := []byte("username=x-access-token\npassword=" + token + "\n") + tmp, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".*") + if err != nil { + return errors.Wrap(err, "create temp credential file") + } + tmpPath := tmp.Name() + if _, err := tmp.Write(body); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpPath) //nolint:gosec // tmpPath is from os.CreateTemp + return errors.Wrap(err, "write temp credential file") + } + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpPath) //nolint:gosec // tmpPath is from os.CreateTemp + return errors.Wrap(err, "close temp credential file") + } + if err := os.Rename(tmpPath, path); err != nil { //nolint:gosec // both paths are from os.CreateTemp + _ = os.Remove(tmpPath) //nolint:gosec // tmpPath is from os.CreateTemp + return errors.Wrap(err, "rename credential file") + } + return nil +} - return exec.CommandContext(ctx, "git", allArgs...), nil +func shellSingleQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } func getInsteadOfDisableArgsForURL(ctx context.Context, targetURL string) ([]string, error) { diff --git a/internal/gitclone/command_test.go b/internal/gitclone/command_test.go index 6a4f21b6..936ab642 100644 --- a/internal/gitclone/command_test.go +++ b/internal/gitclone/command_test.go @@ -2,12 +2,27 @@ package gitclone //nolint:testpackage // Internal functions need to be tested import ( "context" + "log/slog" + "os" + "os/exec" + "path/filepath" "strings" + "sync" + "sync/atomic" "testing" + "time" "github.com/alecthomas/assert/v2" + + "github.com/block/cachew/internal/logging" ) +// testContext attaches a slog logger so refreshCredentialFile doesn't panic. +func testContext(t *testing.T) context.Context { + t.Helper() + return logging.ContextWithLogger(context.Background(), slog.Default()) +} + func TestGetInsteadOfDisableArgsForURL(t *testing.T) { ctx := context.Background() @@ -51,14 +66,13 @@ func TestGitCommand(t *testing.T) { credentialProvider: nil, } - cmd, err := repo.GitCommand(ctx, "version") + cmd, cleanup, err := repo.GitCommand(ctx, "version") assert.NoError(t, err) + t.Cleanup(cleanup) assert.NotZero(t, cmd) assert.True(t, len(cmd.Args) >= 2) - // First arg should be git binary path assert.Equal(t, "git", cmd.Args[0]) - // Last arg should be "version" assert.Equal(t, "version", cmd.Args[len(cmd.Args)-1]) } @@ -70,8 +84,9 @@ func TestGitCommandWithEmptyURL(t *testing.T) { credentialProvider: nil, } - cmd, err := repo.GitCommand(ctx, "version") + cmd, cleanup, err := repo.GitCommand(ctx, "version") assert.NoError(t, err) + t.Cleanup(cleanup) assert.NotZero(t, cmd) assert.Equal(t, "git", cmd.Args[0]) @@ -79,34 +94,42 @@ func TestGitCommandWithEmptyURL(t *testing.T) { } type mockCredentialProvider struct { + mu sync.Mutex token string + calls atomic.Int64 err error } func (m *mockCredentialProvider) GetTokenForURL(_ context.Context, _ string) (string, error) { + m.calls.Add(1) + m.mu.Lock() + defer m.mu.Unlock() return m.token, m.err } +func (m *mockCredentialProvider) setToken(token string) { + m.mu.Lock() + defer m.mu.Unlock() + m.token = token +} + func TestGitCommandWithCredentialProvider(t *testing.T) { - ctx := context.Background() + ctx := testContext(t) tests := []struct { - name string - token string - expectHelper bool - expectedToken string + name string + token string + expectHelper bool }{ { - name: "WithValidToken", - token: "ghp_test123456", - expectHelper: true, - expectedToken: "ghp_test123456", + name: "WithValidToken", + token: "ghp_test123456", + expectHelper: true, }, { - name: "WithTokenContainingSingleQuote", - token: "token'with'quotes", - expectHelper: true, - expectedToken: "token'with'quotes", + name: "WithTokenContainingSingleQuote", + token: "token'with'quotes", + expectHelper: true, }, { name: "WithEmptyToken", @@ -124,23 +147,278 @@ func TestGitCommandWithCredentialProvider(t *testing.T) { }, } - cmd, err := repo.GitCommand(ctx, "version") + cmd, cleanup, err := repo.GitCommand(ctx, "version") assert.NoError(t, err) assert.NotZero(t, cmd) + t.Cleanup(cleanup) - if tt.expectHelper { - found := false - for i, arg := range cmd.Args { - if arg == "-c" && i+1 < len(cmd.Args) { - if strings.Contains(cmd.Args[i+1], "credential.helper=") { - found = true - assert.True(t, strings.Contains(cmd.Args[i+1], "username=x-access-token")) - break - } - } - } - assert.True(t, found, "expected credential.helper to be configured") + helperArg := findCredentialHelperArg(cmd.Args) + if !tt.expectHelper { + assert.Equal(t, "", helperArg, "did not expect credential.helper") + return } + assert.NotEqual(t, "", helperArg, "expected credential.helper to be configured") + + // Token must live in the file, not the helper string, so refresh + // can rotate it without restarting the subprocess. + assert.False(t, strings.Contains(helperArg, tt.token), + "credential.helper must not embed the token literal: %q", helperArg) + + path := credentialFilePathFromHelper(t, helperArg) + contents, err := os.ReadFile(path) + assert.NoError(t, err) + assert.Equal(t, + "username=x-access-token\npassword="+tt.token+"\n", + string(contents)) + + info, err := os.Stat(path) + assert.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) }) } } + +func TestGitCommand_CleanupRemovesCredentialFile(t *testing.T) { + repo := &Repository{ + upstreamURL: "https://github.com/user/repo", + credentialProvider: &mockCredentialProvider{ + token: "ghs_initial", + }, + } + + cmd, cleanup, err := repo.GitCommand(testContext(t), "version") + assert.NoError(t, err) + assert.NotZero(t, cmd) + + helperArg := findCredentialHelperArg(cmd.Args) + path := credentialFilePathFromHelper(t, helperArg) + _, err = os.Stat(path) + assert.NoError(t, err, "credential file should exist before cleanup") + + cleanup() + _, err = os.Stat(path) + assert.True(t, os.IsNotExist(err), "credential file should be removed by cleanup, got err=%v", err) + + cleanup() // cleanup must be idempotent +} + +func TestGitCommand_RefreshGoroutineUpdatesFile(t *testing.T) { + provider := &mockCredentialProvider{token: "ghs_initial"} + repo := &Repository{ + upstreamURL: "https://github.com/user/repo", + credentialProvider: provider, + } + + ctx, cancel := context.WithCancel(testContext(t)) + defer cancel() + path, cleanup, err := repo.startTokenCredentialFile(ctx, "ghs_initial") + assert.NoError(t, err) + t.Cleanup(cleanup) + + provider.setToken("ghs_rotated") + + next, changed, err := repo.refreshCredentialFileOnce(ctx, path, "ghs_initial") + assert.NoError(t, err) + assert.True(t, changed) + assert.Equal(t, "ghs_rotated", next) + + contents, err := os.ReadFile(path) + assert.NoError(t, err) + assert.Equal(t, "username=x-access-token\npassword=ghs_rotated\n", string(contents)) + + // Same token must not churn the file. + next2, changed2, err := repo.refreshCredentialFileOnce(ctx, path, "ghs_rotated") + assert.NoError(t, err) + assert.False(t, changed2) + assert.Equal(t, "ghs_rotated", next2) +} + +// Git appends the credential op (get/store/erase) as a positional arg to +// `!`-helpers, so a bare `cat ` would also cat a worktree file +// named `get`. The helper must absorb that argument. +func TestGitCommand_HelperIgnoresHostileGetFile(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not on PATH") + } + + repo := &Repository{ + upstreamURL: "https://github.com/user/repo", + credentialProvider: &mockCredentialProvider{ + token: "REAL_TOKEN", + }, + } + cmd, cleanup, err := repo.GitCommand(testContext(t), "version") + assert.NoError(t, err) + t.Cleanup(cleanup) + + helperArg := findCredentialHelperArg(cmd.Args) + assert.NotEqual(t, "", helperArg) + + workDir := t.TempDir() + for _, op := range []string{"get", "store", "erase"} { + assert.NoError(t, os.WriteFile(filepath.Join(workDir, op), + []byte("password=EVIL_TOKEN_VIA_"+op+"\n"), 0o600)) + } + + gitCmd := exec.Command("git", "-C", workDir, + "-c", "credential.helper=", // empty value resets inherited helpers + "-c", "credential.helper="+helperArg, + "credential", "fill", + ) + gitCmd.Stdin = strings.NewReader("url=https://github.com/x/y\n\n") + out, err := gitCmd.Output() + assert.NoError(t, err) + assert.True(t, strings.Contains(string(out), "password=REAL_TOKEN\n"), + "expected REAL_TOKEN in helper output, got: %s", out) + assert.False(t, strings.Contains(string(out), "EVIL_TOKEN"), + "helper output must not include any worktree file content: %s", out) +} + +func TestWriteCredentialFile_IgnoresHostileSiblingSymlink(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "cred") + assert.NoError(t, writeCredentialFile(path, "ghs_initial")) + + // Plant the symlink the old `path + ".new"` code would have followed. + sentinel := filepath.Join(dir, "attacker-readable") + assert.NoError(t, os.WriteFile(sentinel, []byte("unchanged"), 0o600)) + assert.NoError(t, os.Symlink(sentinel, path+".new")) + + assert.NoError(t, writeCredentialFile(path, "ghs_rotated")) + + credBytes, err := os.ReadFile(path) + assert.NoError(t, err) + assert.Equal(t, "username=x-access-token\npassword=ghs_rotated\n", string(credBytes)) + + sentinelBytes, err := os.ReadFile(sentinel) + assert.NoError(t, err) + assert.Equal(t, "unchanged", string(sentinelBytes)) +} + +func TestCleanup_WaitsForInFlightRefresh(t *testing.T) { + prev := credentialFileRefreshInterval + credentialFileRefreshInterval = time.Millisecond + t.Cleanup(func() { credentialFileRefreshInterval = prev }) + + release := make(chan struct{}) + var releaseOnce sync.Once + releaseProvider := func() { releaseOnce.Do(func() { close(release) }) } + provider := &blockingCredentialProvider{ + token: "ghs_rotated", + release: release, + entered: make(chan struct{}), + } + repo := &Repository{ + upstreamURL: "https://github.com/user/repo", + credentialProvider: provider, + } + + path, cleanup, err := repo.startTokenCredentialFile(testContext(t), "ghs_initial") + assert.NoError(t, err) + t.Cleanup(func() { releaseProvider(); cleanup() }) + + <-provider.entered + + cleanupReturned := make(chan struct{}) + go func() { + cleanup() + close(cleanupReturned) + }() + + select { + case <-cleanupReturned: + t.Fatal("cleanup returned while a refresh tick was still in flight") + case <-time.After(50 * time.Millisecond): + } + + releaseProvider() + <-cleanupReturned + + _, err = os.Stat(path) + assert.True(t, os.IsNotExist(err), "credential file should be removed after cleanup, got err=%v", err) +} + +// blockingCredentialProvider blocks GetTokenForURL until release is closed, +// and closes entered on the first call. +type blockingCredentialProvider struct { + token string + release chan struct{} + entered chan struct{} + enteredOnce sync.Once +} + +func (p *blockingCredentialProvider) GetTokenForURL(_ context.Context, _ string) (string, error) { + p.enteredOnce.Do(func() { close(p.entered) }) + <-p.release + return p.token, nil +} + +// Concurrent rotation must never expose a partial credential file to the +// helper running in parallel. +func TestWriteCredentialFile_Atomic(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "cred-*") + assert.NoError(t, err) + path := f.Name() + _ = f.Close() + + assert.NoError(t, writeCredentialFile(path, "ghs_one")) + + stop := make(chan struct{}) + var wg sync.WaitGroup + wg.Go(func() { + for { + select { + case <-stop: + return + default: + b, err := os.ReadFile(path) + if err != nil { + continue + } + s := string(b) + assert.True(t, + strings.HasPrefix(s, "username=x-access-token\npassword=") && strings.HasSuffix(s, "\n"), + "reader observed partial write: %q", s) + } + } + }) + + for range 200 { + assert.NoError(t, writeCredentialFile(path, "ghs_rotated")) + } + close(stop) + wg.Wait() +} + +func TestShellSingleQuote(t *testing.T) { + tests := []struct { + in, out string + }{ + {"/tmp/foo", `'/tmp/foo'`}, + {"/tmp/with space", `'/tmp/with space'`}, + {"weird'name", `'weird'\''name'`}, + } + for _, tt := range tests { + assert.Equal(t, tt.out, shellSingleQuote(tt.in)) + } +} + +func findCredentialHelperArg(args []string) string { + for i, a := range args { + if a == "-c" && i+1 < len(args) && strings.HasPrefix(args[i+1], "credential.helper=") { + return strings.TrimPrefix(args[i+1], "credential.helper=") + } + } + return "" +} + +func credentialFilePathFromHelper(t *testing.T, helper string) string { + t.Helper() + const prefix = `!f() { test "$1" = get && cat '` + const suffix = `'; }; f` + assert.True(t, strings.HasPrefix(helper, prefix), "unexpected helper format: %q", helper) + assert.True(t, strings.HasSuffix(helper, suffix), "unexpected helper format: %q", helper) + path := strings.TrimSuffix(strings.TrimPrefix(helper, prefix), suffix) + path = strings.ReplaceAll(path, `'\''`, `'`) + return path +} diff --git a/internal/gitclone/manager.go b/internal/gitclone/manager.go index 90a2bb32..5b7c9aa5 100644 --- a/internal/gitclone/manager.go +++ b/internal/gitclone/manager.go @@ -524,10 +524,11 @@ func (r *Repository) executeClone(ctx context.Context) error { r.upstreamURL, cloneDest, } - cmd, err := r.GitCommand(cloneCtx, args...) + cmd, cleanup, err := r.GitCommand(cloneCtx, args...) if err != nil { return errors.Wrap(err, "create git command") } + defer cleanup() cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} cmd.Cancel = func() error { return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) @@ -602,10 +603,11 @@ func (r *Repository) fetchInternal(ctx context.Context, timeout time.Duration, e } args = append(args, "fetch", "--prune", "--prune-tags") - cmd, err := r.GitCommand(fetchCtx, args...) + cmd, cleanup, err := r.GitCommand(fetchCtx, args...) if err != nil { return errors.Wrap(err, "create git command") } + defer cleanup() // Start the process in its own process group so we can kill the entire // tree (git spawns child processes like git-remote-https that inherit // stdout/stderr pipes and prevent CombinedOutput from returning). @@ -786,10 +788,11 @@ func (r *Repository) GetLocalRefs(ctx context.Context) (map[string]string, error func (r *Repository) GetUpstreamRefs(ctx context.Context) (map[string]string, error) { // #nosec G204 - r.upstreamURL is controlled by us - cmd, err := r.GitCommand(ctx, "ls-remote", r.upstreamURL) + cmd, cleanup, err := r.GitCommand(ctx, "ls-remote", r.upstreamURL) if err != nil { return nil, errors.Wrap(err, "create git command") } + defer cleanup() output, err := cmd.CombinedOutput() if err != nil { return nil, errors.Wrap(err, "git ls-remote") diff --git a/internal/strategy/git/snapshot.go b/internal/strategy/git/snapshot.go index 214a67fe..914b63a6 100644 --- a/internal/strategy/git/snapshot.go +++ b/internal/strategy/git/snapshot.go @@ -915,15 +915,19 @@ func (s *Strategy) generateAndUploadLFSSnapshot(ctx context.Context, repo *gitcl } // Fetch only the LFS objects referenced by HEAD (the default branch). + // Cleanup runs as soon as the subprocess returns so the credential + // file doesn't outlive the only command that needs it. fetchStart := time.Now() - fetchCmd, err := repo.GitCommand(ctx, "-C", workDir, "lfs", "fetch", "origin", "HEAD") + fetchCmd, fetchCleanup, err := repo.GitCommand(ctx, "-C", workDir, "lfs", "fetch", "origin", "HEAD") if err != nil { s.metrics.recordLFSPhase(ctx, upstream, "fetch", "error", time.Since(fetchStart)) return errors.Wrap(err, "create git lfs fetch command") } - if output, err := fetchCmd.CombinedOutput(); err != nil { + fetchOutput, fetchErr := fetchCmd.CombinedOutput() + fetchCleanup() + if fetchErr != nil { s.metrics.recordLFSPhase(ctx, upstream, "fetch", "error", time.Since(fetchStart)) - return errors.Wrapf(err, "git lfs fetch: %s", string(output)) + return errors.Wrapf(fetchErr, "git lfs fetch: %s", string(fetchOutput)) } s.metrics.recordLFSPhase(ctx, upstream, "fetch", "success", time.Since(fetchStart))