diff --git a/cmd/volcano/grant_test.go b/cmd/volcano/grant_test.go index 67d93812..63e6a971 100644 --- a/cmd/volcano/grant_test.go +++ b/cmd/volcano/grant_test.go @@ -37,6 +37,12 @@ type grantRun struct { // directory's link file, telemetry off and no prompts. No config loader is // injected, so the CLI reads its configuration the way it does in the VM. func runAsWorker(t *testing.T, handler http.HandlerFunc, args ...string) grantRun { + t.Helper() + return runAsWorkerWith(t, nil, handler, args...) +} + +// runAsWorkerWith is runAsWorker with setup run in the workspace first. +func runAsWorkerWith(t *testing.T, setup func(), handler http.HandlerFunc, args ...string) grantRun { t.Helper() resetInstructions(t) api.ResetLastRefusalForTest() @@ -65,6 +71,9 @@ func runAsWorker(t *testing.T, handler http.HandlerFunc, args ...string) grantRu t.Setenv("VOLCANO_PROJECT_ID", "") t.Setenv("VOLCANO_TELEMETRY_DISABLED", "1") t.Setenv("VOLCANO_NONINTERACTIVE", "1") + if setup != nil { + setup() + } deps := cliruntime.Deps{HTTPClient: server.Client()} root := rootcmd.New(deps) @@ -191,3 +200,87 @@ func TestGrantInvokesWithTheGrantAndNoServiceKey(t *testing.T) { assert.Equal(t, []string{"Bearer " + testGrant}, run.auth) assert.JSONEq(t, `{"ok":true}`, run.stdout) } + +// Under a grant, the API's code follows the error on its own line. +func TestGrantErrorPrintsTheAPICode(t *testing.T) { + run := runAsWorker(t, func(w http.ResponseWriter, _ *http.Request) { + respondExitTestJSON(w, http.StatusTooManyRequests, + `{"error":"the delegated grant has no mutations left; ask the app for a new grant","code":"grant_budget_exhausted"}`) + }, "cloud", "functions", "list") + + assert.Equal(t, exitGrantBudgetExhausted, run.code, run.stderr) + assert.Contains(t, run.stderr, "Error: failed to list functions: HTTP 429: the delegated grant has no mutations left; "+ + "ask the app for a new grant\nError code: grant_budget_exhausted\n") +} + +// Any other credential sees the error as before, without the code line. +func TestNonGrantErrorPrintsNoCodeLine(t *testing.T) { + code, stderr := runAgainst(t, "test-token", func(w http.ResponseWriter, _ *http.Request) { + respondExitTestJSON(w, http.StatusForbidden, `{"error":"feature is not available on this plan","code":"plan_required"}`) + }, "cloud", "functions", "list") + + assert.Equal(t, exitPermissionDenied, code, stderr) + assert.NotContains(t, stderr, "Error code:") +} + +// A 426 under a grant still names the API's code: the worker branches on the +// code, not on which renderer printed the error. +func TestGrantUpgradeRequiredPrintsTheAPICode(t *testing.T) { + run := runAsWorker(t, func(w http.ResponseWriter, _ *http.Request) { + respondExitTestJSON(w, http.StatusUpgradeRequired, + `{"error":"cli version no longer supported","code":"cli_version_unsupported"}`) + }, "cloud", "functions", "list") + + assert.Equal(t, 1, run.code, run.stderr) + assert.Contains(t, run.stderr, "cli version no longer supported\nError code: cli_version_unsupported\n") +} + +// The code line follows what the command sent, not what the environment +// holds: a local-mode command sends no credential, so a grant in +// VOLCANO_TOKEN does not change its errors. +func TestGrantInTheEnvironmentAloneAddsNoCodeLine(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("VOLCANO_TOKEN", testGrant) + api.ResetLastRefusalForTest() + t.Cleanup(api.ResetLastRefusalForTest) + + var out bytes.Buffer + printError(&out, &api.Error{StatusCode: http.StatusNotFound, Message: "function not found", Code: "not_found"}, cliruntime.Deps{}) + + assert.Contains(t, out.String(), "function not found") + assert.NotContains(t, out.String(), "Error code:") +} + +// The app's worker applies migrations with `databases migration apply`, which +// starts a server-side run. Under a grant it uses only routes a grant may call +// and never reads the database, whose answer holds the connection string. +func TestGrantMigrationApplyUsesOnlyARun(t *testing.T) { + runsPath := "/projects/" + mainTestProjectAlpha + "/databases/app/migration-runs" + const runID = "44444444-4444-4444-8444-444444444444" + run := runAsWorkerWith(t, func() { + require.NoError(t, os.MkdirAll(filepath.Join("volcano", "migrations"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join("volcano", "migrations", "001_init.sql"), []byte("CREATE TABLE t();"), 0o644)) + }, func(w http.ResponseWriter, r *http.Request) { + state := `{"id":"` + runID + `","status":"succeeded","phase":"apply","source":"api","warnings":[],` + + `"database":{"id":"66666666-6666-4666-8666-666666666666","name":"app"},` + + `"files":[{"name":"001_init.sql","outcome":"applied"}],` + + `"created_at":"2026-11-11T00:00:00Z","updated_at":"2026-11-11T00:00:00Z"}` + switch { + case r.Method == http.MethodGet && r.URL.Path == "/capabilities": + respondExitTestJSON(w, http.StatusOK, `{"data":[{"id":"databases.migrations","status":"available"}]}`) + case r.Method == http.MethodPost && r.URL.Path == runsPath: + respondExitTestJSON(w, http.StatusAccepted, state) + case r.Method == http.MethodGet && r.URL.Path == runsPath+"/"+runID: + respondExitTestJSON(w, http.StatusOK, state) + default: + respondExitTestJSON(w, http.StatusForbidden, `{"error":"delegated grants cannot be used on this endpoint","code":"grant_route_not_allowed"}`) + } + }, "cloud", "databases", "migration", "apply", "-d", "app") + + assert.Equal(t, 0, run.code, run.stderr) + assert.Contains(t, run.requests, "POST "+runsPath) + for i, request := range run.requests { + assert.Contains(t, []string{"GET /capabilities", "POST " + runsPath, "GET " + runsPath + "/" + runID}, request) + assert.Equal(t, "Bearer "+testGrant, run.auth[i], request) + } +} diff --git a/cmd/volcano/grant_unix_test.go b/cmd/volcano/grant_unix_test.go new file mode 100644 index 00000000..1c9e3676 --- /dev/null +++ b/cmd/volcano/grant_unix_test.go @@ -0,0 +1,67 @@ +//go:build unix + +package main + +import ( + "net/http" + "os" + "strconv" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + cliconfig "github.com/Kong/volcano-cli/internal/config" +) + +// The worker can pass the grant on a descriptor instead of VOLCANO_TOKEN, so +// it is never in the CLI's environment. main adopts it before the command runs. +func TestGrantFromATokenDescriptor(t *testing.T) { + run := runAsWorkerWith(t, func() { adoptGrantFromADescriptor(t) }, func(w http.ResponseWriter, _ *http.Request) { + respondExitTestJSON(w, http.StatusOK, `{"data":[],"has_more":false,"page":1,"limit":100,"total":0}`) + }, "cloud", "functions", "list") + + assert.Equal(t, 0, run.code, run.stderr) + assert.Equal(t, []string{"Bearer " + testGrant}, run.auth) + assert.Empty(t, os.Getenv("VOLCANO_TOKEN")) +} + +// A refusal that says which variable to unset names VOLCANO_TOKEN_FD when the +// token came from it: unsetting VOLCANO_TOKEN would change nothing. +func TestGrantFromATokenDescriptorIsNamedInSignInHints(t *testing.T) { + for _, args := range [][]string{ + {"cloud", "lavaflow", "disable"}, + {"cloud", "durable", "approvals", "approve", "11111111-1111-4111-8111-111111111111", "--yes"}, + } { + t.Run(args[1]+" "+args[2], func(t *testing.T) { + run := runAsWorkerWith(t, func() { adoptGrantFromADescriptor(t) }, func(w http.ResponseWriter, _ *http.Request) { + respondExitTestJSON(w, http.StatusForbidden, `{"error":"delegated grants cannot be used on this endpoint","code":"grant_route_not_allowed"}`) + }, args...) + + assert.Equal(t, exitPermissionDenied, run.code, run.stderr) + assert.Contains(t, run.stderr, cliconfig.TokenFDEnv) + assert.NotContains(t, run.stderr, "VOLCANO_TOKEN ") + }) + } +} + +// adoptGrantFromADescriptor puts the grant on a pipe, names it in +// VOLCANO_TOKEN_FD and adopts it, as main does at startup. +func adoptGrantFromADescriptor(t *testing.T) { + t.Helper() + cliconfig.ResetAdoptedTokenForTest() + t.Cleanup(cliconfig.ResetAdoptedTokenForTest) + r, w, err := os.Pipe() + require.NoError(t, err) + _, err = w.WriteString(testGrant + "\n") + require.NoError(t, err) + require.NoError(t, w.Close()) + // A duplicate no *os.File owns, so only AdoptToken closes it. + fd, err := syscall.Dup(int(r.Fd())) + require.NoError(t, err) + require.NoError(t, r.Close()) + t.Setenv("VOLCANO_TOKEN", "") + t.Setenv(cliconfig.TokenFDEnv, strconv.Itoa(fd)) + require.NoError(t, cliconfig.AdoptToken()) +} diff --git a/cmd/volcano/main.go b/cmd/volcano/main.go index 8f8ef02d..fe6d8d9a 100644 --- a/cmd/volcano/main.go +++ b/cmd/volcano/main.go @@ -43,6 +43,12 @@ const ( ) func main() { + // First, before anything reads the configuration: take the token from + // VOLCANO_TOKEN_FD, or a grant out of VOLCANO_TOKEN, into memory. + if err := config.AdoptToken(); err != nil { + fmt.Fprintln(os.Stderr, theme.Error("Error:", theme.On(os.Stderr)), err) + os.Exit(exitNotAuthenticated) + } // One recorder per process: it reads the telemetry setting once, and both // the agent headers and the usage event follow that read. Agent headers // count as usage data: none before the first-run notice. @@ -212,6 +218,7 @@ func printDeprecationError(w io.Writer, err error, deps cliruntime.Deps) { if latest := api.LastInstructions().LatestVersion; latest != "" { fmt.Fprintf(w, "Upgrade to %s: %s\n", latest, cliruntime.CommandPath(deps, "upgrade")) } + printGrantErrorCode(w, err) } // printError renders a generic command error, appending a reauth hint when @@ -222,6 +229,20 @@ func printError(w io.Writer, err error, deps cliruntime.Deps) { fmt.Fprintf(w, "Run `%s` to re-authenticate.\n", cliruntime.CommandPath(deps, "login")) } printProjectTokenMismatchHint(w, err, deps) + printGrantErrorCode(w, err) +} + +// printGrantErrorCode prints the API's error code on its own line when the +// command sent a delegated grant, so the app's worker can branch on the code, +// such as grant_budget_exhausted, rather than the message. Every other +// credential sees its errors exactly as before, including a local-mode +// command, which sends no credential even with a grant in the environment. +func printGrantErrorCode(w io.Writer, err error) { + code := api.Code(err) + if code == "" || !api.GrantSent() { + return + } + fmt.Fprintf(w, "Error code: %s\n", code) } // loadConfig loads the configuration through the runtime's loader, so a diff --git a/internal/api/refusal.go b/internal/api/refusal.go index e3089de7..d0514531 100644 --- a/internal/api/refusal.go +++ b/internal/api/refusal.go @@ -4,6 +4,7 @@ import ( "net/http" "strings" "sync" + "sync/atomic" "github.com/google/uuid" @@ -45,12 +46,26 @@ func LastRefusal() Refusal { return lastRefusal } -// ResetLastRefusalForTest clears the process-global refusal state. Test-only: -// each CLI invocation is a fresh process. +// grantSent records whether any request in this process carried a delegated +// grant. +var grantSent atomic.Bool + +// GrantSent reports whether any API request in this process carried a +// delegated grant (pg-). It reads the requests that went out rather than the +// configuration: a local-mode command sends no credential even when a grant +// is in the environment. +func GrantSent() bool { + return grantSent.Load() +} + +// ResetLastRefusalForTest clears the process-global request state: the last +// refusal and whether a grant was sent. Test-only: each CLI invocation is a +// fresh process. func ResetLastRefusalForTest() { refusalMu.Lock() lastRefusal = Refusal{} refusalMu.Unlock() + grantSent.Store(false) } func recordRefusal(req *http.Request) { @@ -89,12 +104,16 @@ func bearerToken(authorization string) string { } // refusalDoer records the request behind every 403 the API returns, so a -// caller explaining one can describe the request instead of guessing at it. +// caller explaining one can describe the request instead of guessing at it, +// and whether any request carried a delegated grant (GrantSent). type refusalDoer struct { next apiclient.HttpRequestDoer } func (d refusalDoer) Do(req *http.Request) (*http.Response, error) { + if config.IsDelegatedGrant(bearerToken(req.Header.Get("Authorization"))) { + grantSent.Store(true) + } resp, err := d.next.Do(req) if resp != nil && resp.StatusCode == http.StatusForbidden { recordRefusal(req) diff --git a/internal/cmd/connect/connect.go b/internal/cmd/connect/connect.go index 3da8e3bb..052160e6 100644 --- a/internal/cmd/connect/connect.go +++ b/internal/cmd/connect/connect.go @@ -158,12 +158,11 @@ func run(ctx context.Context, opts options) error { return fmt.Errorf("--template: %w (supported: nextjs, javascript, python, ruby)", err) } - // Every command after sign-in takes VOLCANO_TOKEN over the saved login, so - // a new login would be saved and then not used. Checked before the - // dashboard's device code is spent. - if strings.TrimSpace(os.Getenv("VOLCANO_TOKEN")) != "" && - (strings.TrimSpace(opts.deviceCode) != "" || opts.signup) { - return errors.New("VOLCANO_TOKEN is set and would be used instead of the new login. " + + // Every command after sign-in takes VOLCANO_TOKEN or VOLCANO_TOKEN_FD over + // the saved login, so a new login would be saved and then not used. + // Checked before the dashboard's device code is spent. + if config.EnvironmentTokenSet() && (strings.TrimSpace(opts.deviceCode) != "" || opts.signup) { + return errors.New("VOLCANO_TOKEN or VOLCANO_TOKEN_FD is set and would be used instead of the new login. " + "Unset it, or run connect without --device-code and --signup to use it") } // VOLCANO_PROJECT_ID is deprecated, and would override the link connect @@ -398,7 +397,7 @@ func ensureSession(ctx context.Context, opts options, sessions clisession.Factor output.Success(opts.log, "Signed in") return nil } - if api.Status(err) != http.StatusUnauthorized || strings.TrimSpace(os.Getenv("VOLCANO_TOKEN")) != "" { + if api.Status(err) != http.StatusUnauthorized || cfg.TokenFromEnv() { return fmt.Errorf("failed to check the saved login: %w", err) } output.Note(opts.log, "The saved login is no longer valid. Signing in again.") diff --git a/internal/cmd/connect/connect_test.go b/internal/cmd/connect/connect_test.go index 79cbbb6b..1dc82906 100644 --- a/internal/cmd/connect/connect_test.go +++ b/internal/cmd/connect/connect_test.go @@ -1317,22 +1317,34 @@ func TestConnectSavesTheDeviceLoginWhenItsCheckFails(t *testing.T) { } func TestConnectRefusesALoginThatVolcanoTokenWouldOverride(t *testing.T) { - setConnectTestHome(t) - t.Setenv("VOLCANO_TOKEN", "pk-from-env") - fake := &fakeAPI{t: t} - server := httptest.NewServer(fake) - defer server.Close() + for name, token := range map[string]string{ + "account token in VOLCANO_TOKEN": "pk-from-env", + // AdoptToken takes a grant out of the environment into memory, as it + // does a token from VOLCANO_TOKEN_FD. + "adopted grant": cliconfig.DelegatedGrantPrefix + "from-env", + } { + t.Run(name, func(t *testing.T) { + setConnectTestHome(t) + t.Setenv("VOLCANO_TOKEN", token) + cliconfig.ResetAdoptedTokenForTest() + t.Cleanup(cliconfig.ResetAdoptedTokenForTest) + require.NoError(t, cliconfig.AdoptToken()) + fake := &fakeAPI{t: t} + server := httptest.NewServer(fake) + defer server.Close() - for _, flags := range [][]string{{"--device-code", connectDeviceCode}, {"--signup", "--email", "a@example.com"}} { - _, err := runConnect(t, server, t.TempDir(), flags...) - require.ErrorContains(t, err, "VOLCANO_TOKEN is set", flags) + for _, flags := range [][]string{{"--device-code", connectDeviceCode}, {"--signup", "--email", "a@example.com"}} { + _, err := runConnect(t, server, t.TempDir(), flags...) + require.ErrorContains(t, err, "VOLCANO_TOKEN or VOLCANO_TOKEN_FD is set", flags) + } + assert.Zero(t, fake.polls, "the device code is not spent") + assert.Zero(t, fake.creates) + assert.False(t, fake.deviceAuthorize) + cfg, err := cliconfig.Load() + require.NoError(t, err) + assert.Equal(t, "pk-connect", cfg.UserToken, "the saved login is unchanged") + }) } - assert.Zero(t, fake.polls, "the device code is not spent") - assert.Zero(t, fake.creates) - assert.False(t, fake.deviceAuthorize) - cfg, err := cliconfig.Load() - require.NoError(t, err) - assert.Equal(t, "pk-connect", cfg.UserToken, "the saved login is unchanged") } // A private relay git email is not offered. Connect stops before it signs in diff --git a/internal/config/config.go b/internal/config/config.go index cb2b6a76..2b125390 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -360,12 +360,21 @@ func (c *Config) Token() string { // Untrimmed, that reaches the Authorization header and net/http refuses to // send the request at all, reporting an invalid header field rather than // anything the user can act on. - if token := strings.TrimSpace(os.Getenv(envToken)); !c.IgnoreEnv && token != "" { + if token := environmentToken(); !c.IgnoreEnv && token != "" { return token } return c.UserToken } +// environmentToken is the token from the environment: the one AdoptToken took in, or +// else VOLCANO_TOKEN. +func environmentToken() string { + if adopted := adoptedToken.Load(); adopted != nil { + return *adopted + } + return strings.TrimSpace(os.Getenv(envToken)) +} + // TokenEnv is the environment variable whose token Token returns ahead of the // saved one. const TokenEnv = envToken @@ -374,7 +383,7 @@ const TokenEnv = envToken // rather than the saved one. Signing in saves a token, which Token then still // passes over. func (c *Config) TokenFromEnv() bool { - return !c.IgnoreEnv && strings.TrimSpace(os.Getenv(envToken)) != "" + return !c.IgnoreEnv && environmentToken() != "" } // FunctionInvokeToken returns the token used for runtime function invocation. diff --git a/internal/config/token_source.go b/internal/config/token_source.go new file mode 100644 index 00000000..6a28e638 --- /dev/null +++ b/internal/config/token_source.go @@ -0,0 +1,150 @@ +package config + +import ( + "errors" + "fmt" + "io" + "os" + "strconv" + "strings" + "sync/atomic" +) + +// TokenFDEnv names a file descriptor to read the token from, instead of +// VOLCANO_TOKEN. A process that can read the CLI's environment, such as one a +// coding agent left running next to it, can read VOLCANO_TOKEN; it can't read +// a descriptor the CLI has already read and closed. 0 reads stdin and leaves +// it open; 1 and 2 are refused. +const TokenFDEnv = "VOLCANO_TOKEN_FD" //nolint:gosec // the name of an environment variable, not a credential + +// maxTokenFDBytes bounds what is read from the token descriptor. +const maxTokenFDBytes = 16 << 10 + +// adoptedToken is the token AdoptToken took in, kept in memory only. Token +// returns it ahead of VOLCANO_TOKEN. +var adoptedToken atomic.Pointer[string] + +// tokenFromFD records that adoptedToken came from VOLCANO_TOKEN_FD. +var tokenFromFD atomic.Bool + +// AdoptToken takes the environment's token into memory, once, at startup, +// before anything reads the configuration. +// +// - With VOLCANO_TOKEN_FD set, it reads the token's line from that +// descriptor, closes it (except stdin), and unsets VOLCANO_TOKEN_FD. +// - With a delegated grant in VOLCANO_TOKEN, it unsets VOLCANO_TOKEN. +// +// Either way, processes the CLI starts (the telemetry flush, git, docker) +// don't inherit the token. Any other token stays in VOLCANO_TOKEN, as before: +// the telemetry flush reads an account token from there. +func AdoptToken() error { + fdText, fdSet := os.LookupEnv(TokenFDEnv) + if !fdSet { + if token := strings.TrimSpace(os.Getenv(envToken)); IsDelegatedGrant(token) { + adoptedToken.Store(&token) + return os.Unsetenv(envToken) + } + return nil + } + if err := os.Unsetenv(TokenFDEnv); err != nil { + return err + } + if strings.TrimSpace(os.Getenv(envToken)) != "" { + return fmt.Errorf("set %s or %s, not both", TokenFDEnv, envToken) + } + fd, err := strconv.Atoi(strings.TrimSpace(fdText)) + if err != nil || fd < 0 { + return fmt.Errorf("%s must be a file descriptor number", TokenFDEnv) + } + token, err := readTokenFD(fd) + if err != nil { + return err + } + adoptedToken.Store(&token) + tokenFromFD.Store(true) + return nil +} + +// TokenAdopted reports whether AdoptToken took a token into memory: one from +// VOLCANO_TOKEN_FD, or a delegated grant from VOLCANO_TOKEN. +func TokenAdopted() bool { + return adoptedToken.Load() != nil +} + +// TokenEnvName names the environment variable the environment's token came +// from: VOLCANO_TOKEN_FD when AdoptToken read it from a descriptor, otherwise +// VOLCANO_TOKEN. A hint that says which variable to unset uses it. +func TokenEnvName() string { + if tokenFromFD.Load() { + return TokenFDEnv + } + return envToken +} + +// EnvironmentTokenSet reports whether the environment gives a token, in +// VOLCANO_TOKEN or VOLCANO_TOKEN_FD, that Token returns ahead of a saved login. +func EnvironmentTokenSet() bool { + return environmentToken() != "" +} + +// readTokenFD reads the token from descriptor fd. +// +// Stdout and stderr are refused. Stdin is read through os.Stdin and left open: +// closing descriptor 0 would hand the number to the next file the CLI opens, +// and every later stdin read would read that file. Any other descriptor is +// closed after the read. +func readTokenFD(fd int) (string, error) { + var r io.Reader + switch fd { + case 0: + r = os.Stdin + case 1, 2: + return "", fmt.Errorf("%s can't be %d: that is stdout or stderr", TokenFDEnv, fd) + default: + file := os.NewFile(uintptr(fd), TokenFDEnv) + if file == nil { + return "", fmt.Errorf("%s=%d is not an open file descriptor", TokenFDEnv, fd) + } + defer func() { _ = file.Close() }() + r = file + } + return readTokenLine(r, fd) +} + +// readTokenLine reads up to the first newline or EOF, so a writer that keeps +// its end open after the token does not hold the CLI up. It reads one byte at +// a time so nothing after the newline is taken from stdin. +func readTokenLine(r io.Reader, fd int) (string, error) { + var line []byte + b := make([]byte, 1) + for { + n, err := r.Read(b) + if n == 1 { + if b[0] == '\n' { + break + } + if len(line) == maxTokenFDBytes { + return "", fmt.Errorf("the token on %s=%d is over %d bytes", TokenFDEnv, fd, maxTokenFDBytes) + } + line = append(line, b[0]) + } + if errors.Is(err, io.EOF) { + break + } + if err != nil { + return "", fmt.Errorf("failed to read the token from %s=%d: %w", TokenFDEnv, fd, err) + } + } + token := strings.TrimSpace(string(line)) + if token == "" { + return "", errors.New("no token on " + TokenFDEnv) + } + return token, nil +} + +// ResetAdoptedTokenForTest forgets the token AdoptToken took in. Test-only: +// each CLI invocation is a fresh process. +func ResetAdoptedTokenForTest() { + adoptedToken.Store(nil) + tokenFromFD.Store(false) +} diff --git a/internal/config/token_source_test.go b/internal/config/token_source_test.go new file mode 100644 index 00000000..bcf1c4c6 --- /dev/null +++ b/internal/config/token_source_test.go @@ -0,0 +1,81 @@ +package config + +import ( + "os" + "os/exec" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func adoptTokenTest(t *testing.T) { + t.Helper() + ResetAdoptedTokenForTest() + t.Cleanup(ResetAdoptedTokenForTest) + t.Setenv(envToken, "") + t.Setenv(TokenFDEnv, "") + require.NoError(t, os.Unsetenv(TokenFDEnv)) +} + +// envOfAChild returns the environment a process the CLI starts would get. +func envOfAChild(t *testing.T) string { + t.Helper() + if runtime.GOOS == "windows" { + return strings.Join(os.Environ(), "\n") + } + out, err := exec.CommandContext(t.Context(), "env").Output() + require.NoError(t, err) + return string(out) +} + +// A grant in VOLCANO_TOKEN is taken out of the environment, so processes the +// CLI starts don't get it. +func TestAdoptTokenTakesAGrantOutOfTheEnvironment(t *testing.T) { + adoptTokenTest(t) + t.Setenv(envToken, " "+DelegatedGrantPrefix+"from-env\n") + + require.NoError(t, AdoptToken()) + + assert.Equal(t, DelegatedGrantPrefix+"from-env", (&Config{}).Token()) + _, set := os.LookupEnv(envToken) + assert.False(t, set) + assert.NotContains(t, envOfAChild(t), "from-env") +} + +// Any other token stays where it was: the telemetry flush reads an account +// token from VOLCANO_TOKEN. +func TestAdoptTokenLeavesOtherTokensInTheEnvironment(t *testing.T) { + adoptTokenTest(t) + t.Setenv(envToken, AccountTokenPrefix+"account") + + require.NoError(t, AdoptToken()) + + assert.Equal(t, AccountTokenPrefix+"account", os.Getenv(envToken)) + assert.Equal(t, AccountTokenPrefix+"account", (&Config{}).Token()) +} + +func TestAdoptTokenRefusals(t *testing.T) { + t.Run("both set", func(t *testing.T) { + adoptTokenTest(t) + t.Setenv(envToken, DelegatedGrantPrefix+"env") + t.Setenv(TokenFDEnv, "3") + require.ErrorContains(t, AdoptToken(), "set VOLCANO_TOKEN_FD or VOLCANO_TOKEN, not both") + }) + t.Run("not a number", func(t *testing.T) { + adoptTokenTest(t) + t.Setenv(TokenFDEnv, "three") + require.ErrorContains(t, AdoptToken(), "VOLCANO_TOKEN_FD must be a file descriptor number") + }) + // Reading stdout or stderr would fail, and closing stderr would hide the + // error that says so. + for _, fd := range []string{"1", "2"} { + t.Run("fd "+fd, func(t *testing.T) { + adoptTokenTest(t) + t.Setenv(TokenFDEnv, fd) + require.ErrorContains(t, AdoptToken(), "VOLCANO_TOKEN_FD can't be "+fd+": that is stdout or stderr") + }) + } +} diff --git a/internal/config/token_source_unix_test.go b/internal/config/token_source_unix_test.go new file mode 100644 index 00000000..e804c324 --- /dev/null +++ b/internal/config/token_source_unix_test.go @@ -0,0 +1,131 @@ +//go:build unix + +package config + +import ( + "errors" + "io" + "os" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// tokenFD returns a descriptor that reads token, as a worker would pass one. +// It is a duplicate no *os.File owns, so only AdoptToken closes it: two owners +// of one descriptor would close it twice, the second time under whatever +// opened the number next. The token is written from a goroutine, since a +// token larger than the pipe's buffer blocks until the CLI reads it. +func tokenFD(t *testing.T, token string) int { + t.Helper() + r, w, err := os.Pipe() + require.NoError(t, err) + written := make(chan error, 1) + go func() { + _, err := w.WriteString(token) + written <- errors.Join(err, w.Close()) + }() + t.Cleanup(func() { + select { + case err := <-written: + // The CLI may stop reading early and close its end. + if !errors.Is(err, syscall.EPIPE) { + assert.NoError(t, err, "writing the token") + } + case <-time.After(5 * time.Second): + t.Error("the token was never read") + } + }) + fd, err := syscall.Dup(int(r.Fd())) + require.NoError(t, err) + require.NoError(t, r.Close()) + return fd +} + +func TestAdoptTokenReadsTheDescriptorAndClosesIt(t *testing.T) { + adoptTokenTest(t) + fd := tokenFD(t, DelegatedGrantPrefix+"from-fd\n") + t.Setenv(TokenFDEnv, strconv.Itoa(fd)) + + require.NoError(t, AdoptToken()) + + assert.Equal(t, DelegatedGrantPrefix+"from-fd", (&Config{}).Token()) + assert.True(t, (&Config{}).TokenFromEnv()) + assert.Empty(t, (&Config{IgnoreEnv: true}).Token(), "local mode ignores it, as it ignores VOLCANO_TOKEN") + _, set := os.LookupEnv(TokenFDEnv) + assert.False(t, set, "the descriptor's number is not passed on") + _, err := syscall.Read(fd, make([]byte, 1)) + require.Error(t, err, "the descriptor is closed") + assert.NotContains(t, envOfAChild(t), "from-fd") +} + +func TestAdoptTokenRefusesAnEmptyOrOversizedRead(t *testing.T) { + t.Run("empty", func(t *testing.T) { + adoptTokenTest(t) + t.Setenv(TokenFDEnv, strconv.Itoa(tokenFD(t, " \n"))) + require.ErrorContains(t, AdoptToken(), "no token on VOLCANO_TOKEN_FD") + }) + t.Run("too long", func(t *testing.T) { + adoptTokenTest(t) + t.Setenv(TokenFDEnv, strconv.Itoa(tokenFD(t, strings.Repeat("a", maxTokenFDBytes+1)))) + err := AdoptToken() + require.ErrorContains(t, err, "is over") + assert.NotContains(t, err.Error(), "aaaa") + }) +} + +// The token is one line. A writer that keeps its end open after the newline, +// or hands it to another process, does not hold the CLI up. +func TestAdoptTokenStopsAtTheFirstNewline(t *testing.T) { + adoptTokenTest(t) + r, w, err := os.Pipe() + require.NoError(t, err) + t.Cleanup(func() { _ = w.Close() }) + _, err = w.WriteString(DelegatedGrantPrefix + "one-line\nmore") + require.NoError(t, err) + fd, err := syscall.Dup(int(r.Fd())) + require.NoError(t, err) + require.NoError(t, r.Close()) + t.Setenv(TokenFDEnv, strconv.Itoa(fd)) + + done := make(chan error, 1) + go func() { done <- AdoptToken() }() + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("AdoptToken waited for the writer to close") + } + assert.Equal(t, DelegatedGrantPrefix+"one-line", (&Config{}).Token()) +} + +// Stdin is read up to the token's newline and left open: closing descriptor 0 +// would give the number to the next file the CLI opens, and stdin reads would +// read that file. +func TestAdoptTokenReadsStdinAndLeavesItOpen(t *testing.T) { + adoptTokenTest(t) + r, w, err := os.Pipe() + require.NoError(t, err) + _, err = w.WriteString(DelegatedGrantPrefix + "from-stdin\nrest") + require.NoError(t, err) + require.NoError(t, w.Close()) + stdin := os.Stdin + os.Stdin = r + t.Cleanup(func() { + os.Stdin = stdin + _ = r.Close() + }) + t.Setenv(TokenFDEnv, "0") + + require.NoError(t, AdoptToken()) + + assert.Equal(t, DelegatedGrantPrefix+"from-stdin", (&Config{}).Token()) + rest, err := io.ReadAll(r) + require.NoError(t, err, "stdin is still open") + assert.Equal(t, "rest", string(rest), "nothing after the token's line is read") +} diff --git a/internal/durable/durable.go b/internal/durable/durable.go index 3a2ee815..84fddd20 100644 --- a/internal/durable/durable.go +++ b/internal/durable/durable.go @@ -9,7 +9,6 @@ package durable import ( "context" - "errors" "fmt" "net/http" "strings" @@ -469,7 +468,7 @@ func (s Service) DecideApproval( switch api.Status(err) { case http.StatusForbidden: if api.LastRefusal().ProjectAccessToken || refusesProjectTokenDecision(api.Message(err)) { - return nil, errApprovalNeedsAPerson + return nil, errApprovalNeedsAPerson() } case http.StatusConflict: current, getErr := authenticated.API.GetDurableApproval(ctx, authenticated.ProjectID, approvalID) @@ -490,14 +489,16 @@ func (s Service) DecideApproval( // errApprovalNeedsAPerson is the answer to a project access token or a // delegated grant deciding. // Approvals exist so that a person signs off, which is why the API refuses -// any credential that is not a person's. VOLCANO_TOKEN overrides a login, and -// is how either usually arrives. It matches -// config.ErrAccountTokenRequired, so the exit code reports a refused -// credential. The API's 403 is left out: its generic body would add a hint -// about the token's project, which is not the problem here. -var errApprovalNeedsAPerson = api.WithCause(errors.New( - "approvals are decided by a person. Run `volcano login` with VOLCANO_TOKEN unset, then decide again"), - config.ErrAccountTokenRequired) +// any credential that is not a person's. VOLCANO_TOKEN or VOLCANO_TOKEN_FD +// overrides a login, and is how either usually arrives; the answer names the +// one in use. It matches config.ErrAccountTokenRequired, so the exit code +// reports a refused credential. The API's 403 is left out: its generic body +// would add a hint about the token's project, which is not the problem here. +func errApprovalNeedsAPerson() error { + return api.WithCause(fmt.Errorf( + "approvals are decided by a person. Run `volcano login` with %s unset, then decide again", config.TokenEnvName()), + config.ErrAccountTokenRequired) +} // RequirePerson refuses a credential the CLI knows is a project access token // or a delegated grant, so a decision is not confirmed only to be refused. DecideApproval still maps @@ -508,7 +509,7 @@ func (s Service) RequirePerson() error { return err } if config.IsProjectBound(cfg.Token()) { - return errApprovalNeedsAPerson + return errApprovalNeedsAPerson() } return nil } diff --git a/internal/lavaflow/lavaflow.go b/internal/lavaflow/lavaflow.go index b460166a..581e2a5a 100644 --- a/internal/lavaflow/lavaflow.go +++ b/internal/lavaflow/lavaflow.go @@ -537,7 +537,7 @@ func personDecides(cfg *config.Config, cause error) error { // unset first. func signInAgain(cfg *config.Config) string { if cfg.TokenFromEnv() { - return "Unset " + config.TokenEnv + " and sign in with 'volcano login', then run this again" + return "Unset " + config.TokenEnvName() + " and sign in with 'volcano login', then run this again" } return "Sign in with 'volcano login' and run this again" } diff --git a/internal/lavaflow/start.go b/internal/lavaflow/start.go index 4060ff8e..70412dc5 100644 --- a/internal/lavaflow/start.go +++ b/internal/lavaflow/start.go @@ -155,12 +155,12 @@ func runLimitReached(now time.Time, err error) error { } // liveSignIn says how the owner signs in so a start counts as theirs. A token -// in VOLCANO_TOKEN wins over a 'volcano login' sign-in, so that case says to -// unset it or replace it. +// from the environment wins over a 'volcano login' sign-in, so that case names +// the variable to unset or replace. func liveSignIn(cfg *config.Config, cause error) error { how := "Run 'volcano login', or use a platform token that expires" if cfg.TokenFromEnv() { - how = "Unset " + config.TokenEnv + " and run 'volcano login', or set it to a platform token that expires" + how = "Unset " + config.TokenEnvName() + " and run 'volcano login', or set it to a platform token that expires" } return api.WithCause(fmt.Errorf("%w. %s", ErrStartNeedsLiveSignIn, how), cause) } diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 3f7268ef..7d1c2fdb 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -384,6 +384,12 @@ func (r *Recorder) Record(inv Invocation) { } return } + // The flush process can't see a token this process took into memory (one + // from VOLCANO_TOKEN_FD, or a grant), so it would send the saved login's + // token instead. Such a run sends nothing. + if config.TokenAdopted() { + return + } _ = r.Send(payload) } diff --git a/internal/telemetry/telemetry_test.go b/internal/telemetry/telemetry_test.go index cd3060cf..4780aa68 100644 --- a/internal/telemetry/telemetry_test.go +++ b/internal/telemetry/telemetry_test.go @@ -815,6 +815,24 @@ func TestAnnounceFeedback_Off(t *testing.T) { assert.Zero(t, h.store.saves) } +// The flush process can't see a token taken into memory, and would send the +// saved login's token with the event. A run on such a token sends nothing. +func TestRecord_AdoptedTokenSendsNothing(t *testing.T) { + t.Setenv("VOLCANO_TOKEN", config.DelegatedGrantPrefix+"grant-secret") + config.ResetAdoptedTokenForTest() + t.Cleanup(config.ResetAdoptedTokenForTest) + require.NoError(t, config.AdoptToken()) + h := newHarness() + h.store.setState(State{DeviceID: "d", NoticeShown: true}) + + h.recorder().Record(Invocation{Stderr: io.Discard}) + assert.Empty(t, h.sent) + + config.ResetAdoptedTokenForTest() + h.recorder().Record(Invocation{Stderr: io.Discard}) + assert.Len(t, h.sent, 1, "the same run without an adopted token sends its event") +} + // A delegated grant never leaves the machine on a telemetry request. func TestFlush_DelegatedGrantSendsNoAuthorization(t *testing.T) { for _, token := range []string{config.DelegatedGrantPrefix + "grant-secret"} {