diff --git a/README.md b/README.md index dc08bdc..f1bf586 100644 --- a/README.md +++ b/README.md @@ -235,6 +235,40 @@ jtk issues list --project PROJ cfl page list --space DEV ``` +### Headless and Keyring-Free Environments + +The API token lives in the OS keyring (macOS Keychain, Windows Credential +Manager, Linux Secret Service). On a headless host, in CI, or under an +automation agent there may be no keyring — or one that is locked and +cannot be unlocked interactively (the unlock prompt never reaches you). +Both tools support keyring-free operation without putting the token in a +plaintext config file: + +- **Supply the token via an environment variable.** `ATLASSIAN_API_TOKEN` + (or the tool-specific `JIRA_API_TOKEN` / `CFL_API_TOKEN`) is resolved + *before* the keyring is ever opened, so the keyring is never touched. This + is the canonical answer for `op run`-style setups and agent automation. + + ```bash + export ATLASSIAN_API_TOKEN="your-api-token" + jtk issues list --project PROJ # never touches the OS keyring + ``` + +- **Use the encrypted-file backend.** `--backend file` (or + `ATLASSIAN_CLI_KEYRING_BACKEND=file`, or `keyring.backend: file` in the + config file) stores the token in an encrypted file instead of the OS + keyring. Supply its passphrase non-interactively with + `ATLASSIAN_CLI_KEYRING_PASSPHRASE` for unattended runs. + +- **Use the `pass` backend.** `--backend pass` (or + `ATLASSIAN_CLI_KEYRING_BACKEND=pass`) integrates with an existing + [`pass`](https://www.passwordstore.org/) password-store — the canonical + headless-Linux option when you already run one. + +If a command fails because the keyring is unavailable or locked, the error +names these same options. Run `jtk config show` / `cfl config show` to see +which backend is active on the current machine. + ### Output Representations Both tools support three output representations: diff --git a/shared/keyring/resolve.go b/shared/keyring/resolve.go index 4cb96b8..720fe68 100644 --- a/shared/keyring/resolve.go +++ b/shared/keyring/resolve.go @@ -7,6 +7,8 @@ import ( "strings" "sync" + cccredstore "github.com/open-cli-collective/cli-common/credstore" + "github.com/open-cli-collective/atlassian-go/credstore" ) @@ -86,7 +88,12 @@ func envToken(tool string) (string, bool) { // the one-time §1.8 migration (Open) and the effective key is read. Env // winning does not force a keyring open — the migration then runs on the // next invocation that does open it (opportunistic, template-consistent). -// Keyring errors propagate (never folded into "absent"). +// Keyring errors propagate (never folded into "absent"), but they are +// wrapped with the headless/no-keyring escape hatches (see +// keyringUnavailableError) so a user on a machine without a usable OS +// keyring — or running the CLI headless under an agent, where the unlock +// prompt never surfaces (issue #384) — gets an actionable, self-service +// message instead of an opaque backend error. func ResolveToken(tool string) (string, TokenSource, error) { if v, ok := envToken(tool); ok { return v, SourceEnv, nil @@ -96,20 +103,91 @@ func ResolveToken(tool string) (string, TokenSource, error) { // A corrupt shared CONFIG file only blocks the migration source; // it must not kill the command. Defer migration, warn once, and // still resolve the token from the keyring (non-migrating). - // Genuine keyring-backend errors still propagate. + // Genuine keyring-backend errors still propagate (wrapped with + // the escape hatches); other failures propagate unwrapped. if errors.Is(err, credstore.ErrCorruptStore) { warnCorruptOnce(err) ns, nerr := OpenNoMigrate() if nerr != nil { - return "", SourceNone, nerr + return "", SourceNone, keyringUnavailableError(tool, nerr) } defer func() { _ = ns.Close() }() - return resolveFromStore(ns) + tok, src, rerr := resolveFromStore(ns) + if rerr != nil { + return "", SourceNone, keyringUnavailableError(tool, rerr) + } + return tok, src, nil } - return "", SourceNone, err + return "", SourceNone, keyringUnavailableError(tool, err) } defer func() { _ = s.Close() }() - return resolveFromStore(s) + tok, src, rerr := resolveFromStore(s) + if rerr != nil { + return "", SourceNone, keyringUnavailableError(tool, rerr) + } + return tok, src, nil +} + +// isBackendError reports whether err is a genuine keyring-BACKEND +// availability failure — the only class for which the headless / +// no-keyring escape-hatch advice (env vars, --backend file/pass) is +// actually accurate. It matches the cli-common credstore sentinels that +// mean "the backend itself could not be opened or used": +// - ErrSecretServiceFailClosed: Secret Service present but locked/denied +// (the issue #384 headless case); +// - ErrBackendNotImplemented: the selected/default backend is unavailable +// on this platform/build; +// - ErrFilePassphraseRequired: the file backend cannot open without its +// passphrase (the advice names that env var). +// +// A credential-decode failure, a format mismatch, ErrStoreClosed, or any +// other non-backend error returns false so it propagates UNWRAPPED rather +// than being mislabeled with "no usable keyring" remedies. +func isBackendError(err error) bool { + return errors.Is(err, cccredstore.ErrSecretServiceFailClosed) || + errors.Is(err, cccredstore.ErrBackendNotImplemented) || + errors.Is(err, cccredstore.ErrFilePassphraseRequired) +} + +// keyringUnavailableError wraps a genuine keyring backend-availability +// failure with the documented escape hatches so the headless / no-keyring +// case (issue #384) is self-service instead of an opaque backend error. +// It wraps ONLY real backend errors (see isBackendError): the §1.8 +// corrupt-shared-config graceful path never reaches it, a benign "no token +// present" result is a nil error, and a non-backend failure (decode, +// format mismatch, store sentinel) is returned UNWRAPPED so it is not +// mislabeled with "no usable keyring" advice that does not apply. +// +// The wrapped error keeps the original via %w, so callers and tests that +// classify with errors.Is (e.g. credstore.ErrSecretServiceFailClosed) +// still match. The message names, in resolution order, the runtime escape +// hatches the Secret-Handling Standard already supports: +// - the per-tool / shared API-token env vars (resolved before the +// keyring is ever opened — the canonical headless answer); +// - the encrypted-file backend (--backend file) and its passphrase env +// var, plus the pass backend (--backend pass) for password-store users. +// +// No new plaintext path is introduced: the env var and file backend are +// the standard's own §1.4 fallbacks, surfaced here so users discover them +// instead of hitting a silent keyring-unlock prompt that never appears. +func keyringUnavailableError(tool string, err error) error { + if err == nil { + return nil + } + // Only backend-availability failures get the headless/no-keyring + // remedies; anything else propagates unwrapped (the advice would be + // misleading for, e.g., a decode failure or ErrStoreClosed). + if !isBackendError(err) { + return err + } + envVars := strings.Join(envVarsFor(tool), " or ") + return fmt.Errorf( + "could not read the API token from the OS keyring (%w). "+ + "If you are running headless or have no usable keyring, supply the token via %s, "+ + "or select the encrypted-file backend with --backend file "+ + "(passphrase via %s) or the pass backend with --backend pass. "+ + "See `%s config show` for the active backend", + err, envVars, passphraseEnvVar(Service), tool) } // ResolveTokenNoMigrate is the DIAGNOSTIC resolver (`config show` source @@ -129,7 +207,12 @@ func ResolveTokenNoMigrate(tool string) (string, TokenSource, error) { // resolveFromStore reads the single shared api_token. One key per logical // credential (§1.11.10): jtk and cfl resolve the same key. -func resolveFromStore(s *Store) (string, TokenSource, error) { +// +// It is a package var (not a plain func) purely as a test seam: a test +// must be able to exercise the "Open succeeded but the read failed" branch +// of ResolveToken without a backend that can fail mid-read on every OS. +// Production code never reassigns it. +var resolveFromStore = func(s *Store) (string, TokenSource, error) { if v, ok, err := s.get(KeyAPIToken); err != nil { return "", SourceNone, err } else if ok { diff --git a/shared/keyring/resolve_test.go b/shared/keyring/resolve_test.go new file mode 100644 index 0000000..9f0fe92 --- /dev/null +++ b/shared/keyring/resolve_test.go @@ -0,0 +1,249 @@ +package keyring + +import ( + "errors" + "fmt" + "os" + "strings" + "testing" + + cccredstore "github.com/open-cli-collective/cli-common/credstore" +) + +// keyringUnavailableError must wrap a genuine backend-availability error +// (so errors.Is still classifies it) and name every documented runtime +// escape hatch so the headless / no-keyring case (issue #384) is +// self-service. The backend hint is parameterized off the tool. It must +// be a no-op on a nil error. +func TestKeyringUnavailableError_WrapsAndNamesEscapeHatches(t *testing.T) { + t.Parallel() + + if got := keyringUnavailableError(ToolCFL, nil); got != nil { + t.Fatalf("nil error must not be wrapped; got %v", got) + } + + // A genuine backend-availability error (the only class that gets the + // headless escape-hatch advice). It carries an underlying message we + // can assert is preserved, not replaced. + base := fmt.Errorf("secret-service is locked: %w", cccredstore.ErrSecretServiceFailClosed) + for _, tc := range []struct { + tool string + wantVars []string + }{ + {ToolCFL, []string{"CFL_API_TOKEN", "ATLASSIAN_API_TOKEN"}}, + {ToolJTK, []string{"JIRA_API_TOKEN", "ATLASSIAN_API_TOKEN"}}, + } { + err := keyringUnavailableError(tc.tool, base) + if err == nil { + t.Fatalf("%s: expected a wrapped error", tc.tool) + } + // Underlying error preserved for errors.Is classification. + if !errors.Is(err, base) { + t.Fatalf("%s: wrapped error must keep the underlying error; got %v", tc.tool, err) + } + msg := err.Error() + // Names the per-tool and shared env vars (the headless answer: + // resolved before the keyring is ever opened). + for _, v := range tc.wantVars { + if !strings.Contains(msg, v) { + t.Fatalf("%s: error must name env var %s; got %v", tc.tool, v, err) + } + } + // Names the file + pass backend escape hatches and the passphrase + // env var (the standard's §1.4 fallbacks). + for _, want := range []string{"--backend file", "--backend pass", passphraseEnvVar(Service)} { + if !strings.Contains(msg, want) { + t.Fatalf("%s: error must name %q; got %v", tc.tool, want, err) + } + } + // The "active backend" hint is parameterized off the tool, not + // hardcoded — a third tool would otherwise get a wrong `cfl`/`jtk` + // suggestion (review comment 2). + if !strings.Contains(msg, "`"+tc.tool+" config show`") { + t.Fatalf("%s: backend hint must be derived from the tool; got %v", tc.tool, err) + } + // Wrapper prose present (we wrap, not replace, the backend message). + if !strings.Contains(msg, "could not read the API token") { + t.Fatalf("%s: wrapper prose missing; got %v", tc.tool, err) + } + } +} + +// keyringUnavailableError must NOT apply the headless / no-keyring escape +// hatches to a NON-backend error (a decode failure, format mismatch, +// ErrStoreClosed, etc.): that advice would be misleading. Such errors are +// returned UNWRAPPED (review comment 1). +func TestKeyringUnavailableError_NonBackendErrorNotWrapped(t *testing.T) { + t.Parallel() + + for _, base := range []error{ + errors.New("decode api_token: invalid base64 payload"), + fmt.Errorf("read api_token: %w", cccredstore.ErrStoreClosed), + } { + got := keyringUnavailableError(ToolCFL, base) + // Identity is preserved (returned verbatim, not re-wrapped): the + // message is byte-for-byte the original and errors.Is still matches. + if got.Error() != base.Error() { + t.Fatalf("non-backend error must be returned unwrapped; got %q (want %q)", got, base) + } + if !errors.Is(got, base) { + t.Fatalf("non-backend error must still classify via errors.Is; got %v", got) + } + if strings.Contains(got.Error(), "--backend file") || + strings.Contains(got.Error(), "no usable keyring") { + t.Fatalf("non-backend error must not gain keyring escape-hatch advice; got %v", got) + } + } +} + +// ResolveToken must surface the actionable escape-hatch message — not an +// opaque backend error — when the keyring backend cannot be opened. We +// force a genuine open failure by selecting an unknown backend, which +// cli-common rejects with ErrBackendNotImplemented (OS-independent). +func TestResolveToken_KeyringOpenFailure_WrappedActionable(t *testing.T) { + hermetic(t) + // Override the file backend the hermetic harness selects with a bogus + // one so credstore.Open fails closed before any read/migration. + t.Setenv(cccredstore.BackendEnvVar(Service), "no-such-backend") + + _, _, err := ResolveToken(ToolJTK) + if err == nil { + t.Fatal("expected a keyring-open error") + } + // Underlying classification still works. + if !errors.Is(err, cccredstore.ErrBackendNotImplemented) { + t.Fatalf("must preserve the underlying backend error for errors.Is; got %v", err) + } + msg := err.Error() + for _, want := range []string{ + "JIRA_API_TOKEN", "ATLASSIAN_API_TOKEN", + "--backend file", "--backend pass", + passphraseEnvVar(Service), + } { + if !strings.Contains(msg, want) { + t.Fatalf("error must name %q so the failure is self-service; got %v", want, err) + } + } +} + +// The escape hatch the wrapped message advertises must actually work: with +// the API-token env var set, ResolveToken returns it and NEVER opens the +// keyring — so a broken/locked backend (or no keyring at all, the issue +// #384 headless case) is fully bypassed. This is the config-file-present +// user's fix: export the token, don't touch the keystore. +func TestResolveToken_EnvBypassesBrokenKeyring(t *testing.T) { + hermetic(t) + // A backend that would fail if opened — proving env short-circuits + // before any keyring access. + t.Setenv(cccredstore.BackendEnvVar(Service), "no-such-backend") + const envTok = "ENV-TOKEN-pqrSTU" //nolint:gosec // G101: test fixture, not a real credential + t.Setenv("ATLASSIAN_API_TOKEN", envTok) + + tok, src, err := ResolveToken(ToolCFL) + if err != nil { + t.Fatalf("env token must bypass the keyring entirely; got error %v", err) + } + if tok != envTok { + t.Fatalf("token = %q, want %q", tok, envTok) + } + if src != SourceEnv { + t.Fatalf("source = %q, want %q", src, SourceEnv) + } +} + +// The companion to the open-failure case: when Open SUCCEEDS but the +// subsequent read fails with a genuine backend error (e.g. the secret +// service goes locked/denied between open and read), ResolveToken must +// still surface the actionable escape-hatch message — not the opaque +// backend error — and still classify via errors.Is. We inject a +// resolveFromStore that returns ErrSecretServiceFailClosed because no +// backend can be made to open cleanly yet fail mid-read on every OS. +func TestResolveToken_StoreReadFailure_WrappedActionable(t *testing.T) { + hermetic(t) + // A real, working backend so Open() succeeds and we exercise the + // post-open read branch (not the open-failure branch). + if err := SetCredential(strings.NewReader(secret), ""); err != nil { + t.Fatalf("seed: %v", err) + } + orig := resolveFromStore + t.Cleanup(func() { resolveFromStore = orig }) + resolveFromStore = func(*Store) (string, TokenSource, error) { + // A backend-availability failure (the locked/denied secret-service + // case) — exactly the class isBackendError matches, so it is wrapped. + return "", SourceNone, fmt.Errorf("read api_token: %w", cccredstore.ErrSecretServiceFailClosed) + } + + _, _, err := ResolveToken(ToolJTK) + if err == nil { + t.Fatal("expected a keyring-read error") + } + // Underlying classification still works through the wrapper. + if !errors.Is(err, cccredstore.ErrSecretServiceFailClosed) { + t.Fatalf("must preserve the underlying backend error for errors.Is; got %v", err) + } + msg := err.Error() + for _, want := range []string{ + "JIRA_API_TOKEN", "ATLASSIAN_API_TOKEN", + "--backend file", "--backend pass", + passphraseEnvVar(Service), + } { + if !strings.Contains(msg, want) { + t.Fatalf("error must name %q so the failure is self-service; got %v", want, err) + } + } +} + +// The flip side of the predicate (comment 1): a NON-backend read failure +// (e.g. a credential-decode / format error) must propagate UNWRAPPED — it +// must NOT be mislabeled with the headless / no-keyring escape-hatch +// advice, which would be misleading. The underlying error is preserved +// verbatim for errors.Is. +func TestResolveToken_NonBackendReadFailure_NotWrapped(t *testing.T) { + hermetic(t) + if err := SetCredential(strings.NewReader(secret), ""); err != nil { + t.Fatalf("seed: %v", err) + } + orig := resolveFromStore + t.Cleanup(func() { resolveFromStore = orig }) + decodeErr := errors.New("decode api_token: invalid base64 payload") + resolveFromStore = func(*Store) (string, TokenSource, error) { + return "", SourceNone, decodeErr + } + + _, _, err := ResolveToken(ToolCFL) + if err == nil { + t.Fatal("expected a read error") + } + if !errors.Is(err, decodeErr) { + t.Fatalf("non-backend error must propagate; got %v", err) + } + // The escape-hatch prose must NOT appear: this is not a no-keyring case. + if strings.Contains(err.Error(), "no usable keyring") || + strings.Contains(err.Error(), "--backend file") { + t.Fatalf("non-backend read failure must not get keyring escape-hatch advice; got %v", err) + } +} + +// Regression guard for the graceful-degradation contract: a corrupt +// shared config must still resolve the token from a WORKING keyring with +// NO error — the escape-hatch wrapper must not leak into that path (it is +// reserved for genuine backend failures). +func TestResolveToken_CorruptSharedConfig_NotWrappedWhenKeyringWorks(t *testing.T) { + hermetic(t) + if err := SetCredential(strings.NewReader(secret), ""); err != nil { + t.Fatalf("seed: %v", err) + } + sharedPath := sharedConfigPath(t) + // Neither valid YAML nor JSON → credstore.Load wraps ErrCorruptStore. + if err := os.WriteFile(sharedPath, []byte(":::not yaml: ["), 0o600); err != nil { + t.Fatal(err) + } + + tok, src, err := ResolveToken(ToolCFL) + if err != nil { + t.Fatalf("corrupt shared config with a working keyring must NOT error; got %v", err) + } + if tok != secret || src != SourceKeyAPI { + t.Fatalf("token=%q src=%q, want %q / %q", tok, src, secret, SourceKeyAPI) + } +}