diff --git a/README.md b/README.md index f5f938d..18cf219 100644 --- a/README.md +++ b/README.md @@ -213,6 +213,50 @@ as raw JSON so unknown members survive configuration reads. For example: {"notion":{"api_key":"your-integration-token"}} ``` +### Resolving `api_key` from the macOS Keychain + +`api_key` can name a macOS Keychain item instead of carrying the credential +itself, so `orcli.json` never has to hold the raw key (issue #67). Add +`api_key_keychain` naming an entry in a `keychain.items` block, where each +entry is the `account`/`service` pair `security find-generic-password`'s own +`-a`/`-s` flags match against: + +```json +{"api_key_keychain":"openrouter","keychain":{"items":{"openrouter":{"account":"john@smith.com","service":"private-openrouter-api"}}}} +``` + +**Precedence is deliberate: an inline `api_key`, when present, always wins +over `api_key_keychain`.** Migrating means removing `api_key` from the file, +not just adding `api_key_keychain` beside it - a stale `api_key` sitting +next to a working Keychain reference would otherwise keep being used with no +error anywhere, which is the opposite of what this feature is for. + +Manage named items without ever typing a secret into the interactive session +(which would echo it into the transcript) or display one on screen: + +```bash +orcli keychain set openrouter john@smith.com private-openrouter-api # prompts for the secret, no echo +orcli keychain list # names, accounts, services - never secret values +orcli keychain delete openrouter # removes the config reference AND the Keychain item +``` + +`orcli keychain set` reads the secret from the terminal with echo +suppressed when stdin is a real terminal (`golang.org/x/term.ReadPassword`); +piped input is read as a single line instead, which has no terminal echo to +suppress in the first place. The secret reaches the underlying `security +add-generic-password` command as one argument - the one exposure this +approach cannot avoid while driving that CLI rather than the Security +framework's own API directly, visible only to another local process +inspecting the process list at that moment, and never written to this +program's own logs, stdout, or any error it returns. + +Separate named entries support distinct per-provider credentials and a +dedicated test key (`keychain.items.test` in the example above), so running +the test suite never has to revoke or share the credential a real session +uses. Only macOS is supported; `orcli keychain` and `api_key_keychain` both +report a clear, named error on any other platform rather than attempting to +run a `security` binary that is not there. + The optional `github` block carries an `api_key` string and preserves unknown members in the same way: diff --git a/cmd/orcli/keychain_cmd.go b/cmd/orcli/keychain_cmd.go new file mode 100644 index 0000000..22f498e --- /dev/null +++ b/cmd/orcli/keychain_cmd.go @@ -0,0 +1,160 @@ +package main + +import ( + "bufio" + "context" + "fmt" + "io" + "os" + "sort" + "strings" + + "github.com/glenjbarber/orcli/internal/config" + "github.com/glenjbarber/orcli/internal/keychain" + "golang.org/x/term" +) + +// keychainSet and keychainDelete are keychain.Set/keychain.Delete, held as +// package variables so a test can stand in for the real Keychain the same +// way draw and gate already do elsewhere in this package - this repository's +// own CI runs on Linux, which has no Keychain and no `security` binary at +// all, and a test over the real functions would only ever exercise +// ErrUnsupported there. +var ( + keychainSet = keychain.Set + keychainDelete = keychain.Delete +) + +// runKeychainCommand implements `orcli keychain ` (issue +// #67's own "secure way to add/update named API-key items and list their +// names/provider association without displaying secret values" and +// "deletion should remove only the specifically selected item"). +// +// It is a command rather than a TUI slash command: every other credential +// surface here (`/attribute`, the cloudflare block) is edited by hand in +// the file or, for a value that is not a secret, through the interactive +// session - but a Keychain secret has to be typed once, with no terminal +// echo, and the interactive session's own input line is not that: it is +// rendered into the transcript and would put the secret on screen the +// moment it was typed. A separate, one-shot command that exits before any +// chat session opens is the only place this can be entered without ever +// being drawn. +func runKeychainCommand(ctx context.Context, args []string, stdin io.Reader, stdout io.Writer, cfg config.Config, path string) error { + if len(args) == 0 { + return fmt.Errorf("usage: orcli keychain ...\n%s", keychainUsage) + } + switch args[0] { + case "set": + return runKeychainSet(ctx, args[1:], stdin, stdout, path) + case "list": + return runKeychainList(stdout, cfg) + case "delete": + return runKeychainDelete(ctx, args[1:], stdout, cfg, path) + default: + return fmt.Errorf("orcli keychain: unknown subcommand %q\n%s", args[0], keychainUsage) + } +} + +const keychainUsage = `usage: + orcli keychain set NAME ACCOUNT SERVICE add or update a named Keychain reference, then prompt for the secret + orcli keychain list list configured names, accounts and services - never secret values + orcli keychain delete NAME remove a named reference from both the config file and the Keychain` + +func runKeychainSet(ctx context.Context, args []string, stdin io.Reader, stdout io.Writer, path string) error { + if len(args) != 3 { + return fmt.Errorf("orcli keychain set: want NAME ACCOUNT SERVICE, got %d argument(s)", len(args)) + } + name, account, service := args[0], args[1], args[2] + ref := keychain.Ref{Account: account, Service: service} + + fmt.Fprintf(stdout, "Secret for %s (%s): ", name, ref) + secret, err := readSecret(stdin) + if err != nil { + return fmt.Errorf("orcli keychain set: reading the secret: %w", err) + } + if secret == "" { + return fmt.Errorf("orcli keychain set: no secret was given") + } + + if err := keychainSet(ctx, ref, secret); err != nil { + return err + } + if err := config.WriteKeychainItem(path, name, config.KeychainRef{Account: account, Service: service}); err != nil { + return err + } + fmt.Fprintf(stdout, "\nStored %q (%s). Set \"api_key_keychain\": %q in your configuration file to use it as the credential.\n", name, ref, name) + return nil +} + +// readSecret reads one line from stdin without echoing it, when stdin is an +// interactive terminal - golang.org/x/term.ReadPassword is what actually +// suppresses the echo, and it needs a real terminal file descriptor to do +// that. When stdin is not a terminal (piped input, or a test), it falls +// back to reading one line in the plain way: a pipe has no terminal to +// suppress echo on in the first place, so there is nothing this function +// could additionally protect there, and refusing to work at all with +// piped input would make this command untestable. +func readSecret(stdin io.Reader) (string, error) { + if f, ok := stdin.(*os.File); ok && term.IsTerminal(int(f.Fd())) { + b, err := term.ReadPassword(int(f.Fd())) + if err != nil { + return "", err + } + return string(b), nil + } + line, err := bufio.NewReader(stdin).ReadString('\n') + if err != nil && err != io.EOF { + return "", err + } + return strings.TrimRight(line, "\r\n"), nil +} + +func runKeychainList(stdout io.Writer, cfg config.Config) error { + if cfg.Keychain == nil || len(cfg.Keychain.Items) == 0 { + fmt.Fprintln(stdout, "no Keychain items are configured.") + return nil + } + names := make([]string, 0, len(cfg.Keychain.Items)) + for name := range cfg.Keychain.Items { + names = append(names, name) + } + sort.Strings(names) + + for _, name := range names { + ref := cfg.Keychain.Items[name] + marker := "" + if name == cfg.APIKeyKeychain { + marker = " (api_key_keychain)" + } + fmt.Fprintf(stdout, "%s: account=%q service=%q%s\n", name, ref.Account, ref.Service, marker) + } + return nil +} + +func runKeychainDelete(ctx context.Context, args []string, stdout io.Writer, cfg config.Config, path string) error { + if len(args) != 1 { + return fmt.Errorf("orcli keychain delete: want exactly NAME, got %d argument(s)", len(args)) + } + name := args[0] + if cfg.Keychain == nil { + return fmt.Errorf("orcli keychain delete: %q is not configured", name) + } + ref, ok := cfg.Keychain.Items[name] + if !ok { + return fmt.Errorf("orcli keychain delete: %q is not configured", name) + } + + if err := config.DeleteKeychainItem(path, name); err != nil { + return err + } + if err := keychainDelete(ctx, keychain.Ref{Account: ref.Account, Service: ref.Service}); err != nil { + // The config reference is already gone at this point. Reported + // rather than silently ignored, since a Keychain item left + // behind (locked, or the Delete call itself failed) is the kind + // of orphaned secret a reader should be told about rather than + // have an apparently-clean command hide. + return fmt.Errorf("removed the configuration reference for %q, but removing the Keychain item itself failed: %w", name, err) + } + fmt.Fprintf(stdout, "Removed %q.\n", name) + return nil +} diff --git a/cmd/orcli/keychain_cmd_test.go b/cmd/orcli/keychain_cmd_test.go new file mode 100644 index 0000000..a1f4a26 --- /dev/null +++ b/cmd/orcli/keychain_cmd_test.go @@ -0,0 +1,214 @@ +package main + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/glenjbarber/orcli/internal/config" + "github.com/glenjbarber/orcli/internal/keychain" +) + +// stubKeychain stands in for internal/keychain's own Set/Delete for the +// duration of one test, restoring the real functions afterward - the same +// pattern runIn's own doc comment describes for the trust gate. +func stubKeychain(t *testing.T, set func(context.Context, keychain.Ref, string) error, del func(context.Context, keychain.Ref) error) { + t.Helper() + oldSet, oldDelete := keychainSet, keychainDelete + if set != nil { + keychainSet = set + } + if del != nil { + keychainDelete = del + } + t.Cleanup(func() { + keychainSet, keychainDelete = oldSet, oldDelete + }) +} + +func TestKeychainSet_WritesTheConfigReferenceAndTheSecret(t *testing.T) { + path := writeConfigPath(t, `{"api_key":"k"}`) + var gotRef keychain.Ref + var gotSecret string + stubKeychain(t, func(ctx context.Context, ref keychain.Ref, secret string) error { + gotRef, gotSecret = ref, secret + return nil + }, nil) + + var out bytes.Buffer + err := runKeychainCommand(context.Background(), []string{"set", "openrouter", "john@smith.com", "private-openrouter-api"}, + strings.NewReader("sk-or-v1-secret\n"), &out, config.Config{}, path) + if err != nil { + t.Fatalf("runKeychainCommand: %v", err) + } + if gotSecret != "sk-or-v1-secret" { + t.Errorf("Set was called with secret %q", gotSecret) + } + if gotRef != (keychain.Ref{Account: "john@smith.com", Service: "private-openrouter-api"}) { + t.Errorf("Set was called with %+v", gotRef) + } + if strings.Contains(out.String(), "sk-or-v1-secret") { + t.Errorf("the secret was printed to stdout: %q", out.String()) + } + + cfg, _, err := config.LoadFromPath(path) + if err != nil { + t.Fatalf("LoadFromPath: %v", err) + } + if cfg.Keychain == nil || cfg.Keychain.Items["openrouter"] != (config.KeychainRef{Account: "john@smith.com", Service: "private-openrouter-api"}) { + t.Errorf("the config reference was not written: %+v", cfg.Keychain) + } +} + +func TestKeychainSet_WrongArgCount(t *testing.T) { + path := writeConfigPath(t, `{"api_key":"k"}`) + var out bytes.Buffer + err := runKeychainCommand(context.Background(), []string{"set", "openrouter", "only-account"}, + strings.NewReader(""), &out, config.Config{}, path) + if err == nil { + t.Fatal("expected an error for the wrong argument count") + } +} + +func TestKeychainSet_EmptySecretIsRefused(t *testing.T) { + path := writeConfigPath(t, `{"api_key":"k"}`) + stubKeychain(t, func(context.Context, keychain.Ref, string) error { + t.Fatal("Set should not be called when no secret was given") + return nil + }, nil) + + var out bytes.Buffer + err := runKeychainCommand(context.Background(), []string{"set", "openrouter", "a", "s"}, + strings.NewReader("\n"), &out, config.Config{}, path) + if err == nil { + t.Fatal("expected a refusal for an empty secret") + } +} + +func TestKeychainSet_KeychainFailureLeavesNoConfigReference(t *testing.T) { + path := writeConfigPath(t, `{"api_key":"k"}`) + sentinel := errors.New("keychain: locked") + stubKeychain(t, func(context.Context, keychain.Ref, string) error { + return sentinel + }, nil) + + var out bytes.Buffer + err := runKeychainCommand(context.Background(), []string{"set", "openrouter", "a", "s"}, + strings.NewReader("secret\n"), &out, config.Config{}, path) + if !errors.Is(err, sentinel) { + t.Fatalf("runKeychainCommand() = %v, want it to propagate the keychain failure", err) + } + cfg, _, err := config.LoadFromPath(path) + if err != nil { + t.Fatalf("LoadFromPath: %v", err) + } + if cfg.Keychain != nil && len(cfg.Keychain.Items) != 0 { + t.Errorf("a config reference was written despite the Keychain write failing: %+v", cfg.Keychain) + } +} + +func TestKeychainList_NeverPrintsASecretAndMarksTheActiveOne(t *testing.T) { + cfg := config.Config{ + APIKeyKeychain: "openrouter", + Keychain: &config.Keychain{Items: map[string]config.KeychainRef{ + "openrouter": {Account: "john@smith.com", Service: "private-openrouter-api"}, + "test": {Account: "john@smith.com", Service: "orcli-test-key"}, + }}, + } + var out bytes.Buffer + if err := runKeychainCommand(context.Background(), []string{"list"}, strings.NewReader(""), &out, cfg, "/unused"); err != nil { + t.Fatalf("runKeychainCommand: %v", err) + } + s := out.String() + if !strings.Contains(s, "openrouter") || !strings.Contains(s, "test") { + t.Errorf("list did not mention both items:\n%s", s) + } + if !strings.Contains(s, "(api_key_keychain)") { + t.Errorf("list did not mark which item is active:\n%s", s) + } + // The account value itself ("john@smith.com") is not a secret - it is + // the reference the file already carries in the open - so only the + // service names stand in here for what must never appear: a secret + // value never passes through this path at all, by construction, since + // runKeychainList never touches internal/keychain. +} + +func TestKeychainList_NoneConfigured(t *testing.T) { + var out bytes.Buffer + if err := runKeychainCommand(context.Background(), []string{"list"}, strings.NewReader(""), &out, config.Config{}, "/unused"); err != nil { + t.Fatalf("runKeychainCommand: %v", err) + } + if !strings.Contains(out.String(), "no Keychain items") { + t.Errorf("list output = %q", out.String()) + } +} + +func TestKeychainDelete_RemovesTheConfigReferenceAndTheKeychainItem(t *testing.T) { + path := writeConfigPath(t, `{"api_key":"k","keychain":{"items":{"openrouter":{"account":"a","service":"s"}}}}`) + var gotRef keychain.Ref + stubKeychain(t, nil, func(ctx context.Context, ref keychain.Ref) error { + gotRef = ref + return nil + }) + cfg, _, err := config.LoadFromPath(path) + if err != nil { + t.Fatalf("LoadFromPath: %v", err) + } + + var out bytes.Buffer + if err := runKeychainCommand(context.Background(), []string{"delete", "openrouter"}, strings.NewReader(""), &out, cfg, path); err != nil { + t.Fatalf("runKeychainCommand: %v", err) + } + if gotRef != (keychain.Ref{Account: "a", Service: "s"}) { + t.Errorf("Delete was called with %+v", gotRef) + } + after, _, err := config.LoadFromPath(path) + if err != nil { + t.Fatalf("LoadFromPath (after): %v", err) + } + if after.Keychain != nil { + if _, ok := after.Keychain.Items["openrouter"]; ok { + t.Error("the config reference was not removed") + } + } +} + +func TestKeychainDelete_UnknownNameIsRefused(t *testing.T) { + path := writeConfigPath(t, `{"api_key":"k"}`) + stubKeychain(t, nil, func(context.Context, keychain.Ref) error { + t.Fatal("Delete should not be called for a name that was never configured") + return nil + }) + + var out bytes.Buffer + err := runKeychainCommand(context.Background(), []string{"delete", "openrouter"}, strings.NewReader(""), &out, config.Config{}, path) + if err == nil { + t.Fatal("expected a refusal for an unconfigured name") + } +} + +func TestKeychainCommand_UnknownSubcommand(t *testing.T) { + var out bytes.Buffer + err := runKeychainCommand(context.Background(), []string{"frobnicate"}, strings.NewReader(""), &out, config.Config{}, "/unused") + if err == nil { + t.Fatal("expected a refusal for an unknown subcommand") + } +} + +// writeConfigPath writes a configuration file at a fresh temporary path +// (not the shared default-search-order path withHome/writeConfig use), so +// tests here can run without withHome's own HOME/SearchOrder stand-in, +// mirroring how the rest of this package's tests pass an explicit path to +// config.LoadFromPath directly. +func writeConfigPath(t *testing.T, body string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "orcli.json") + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + return path +} diff --git a/cmd/orcli/main.go b/cmd/orcli/main.go index c1e5af7..62aed5f 100644 --- a/cmd/orcli/main.go +++ b/cmd/orcli/main.go @@ -14,10 +14,21 @@ import ( "github.com/glenjbarber/orcli/internal/config" "github.com/glenjbarber/orcli/internal/groq" + "github.com/glenjbarber/orcli/internal/keychain" "github.com/glenjbarber/orcli/internal/openrouter" "github.com/glenjbarber/orcli/internal/tui" ) +// keychainLookup adapts internal/keychain.Lookup to config.KeychainLookupFunc +// - the one place this package's own dependency on internal/keychain is +// named, so internal/config itself stays free of it (see +// config.KeychainLookupFunc's own doc comment for why that boundary +// matters). A package variable, like keychainSet/keychainDelete in +// keychain_cmd.go, so a test can stand in for the real Keychain. +var keychainLookup config.KeychainLookupFunc = func(ctx context.Context, ref config.KeychainRef) (string, error) { + return keychain.Lookup(ctx, keychain.Ref{Account: ref.Account, Service: ref.Service}) +} + // version is the build identity. // // It is a variable rather than a constant so a build can set it with -ldflags, and @@ -147,7 +158,12 @@ func run(ctx context.Context, args []string, stdin io.Reader, stdout, stderr io. return nil } - if rest := fs.Args(); len(rest) > 0 { + // keychain is dispatched after the configuration is loaded, below, + // because every one of its own subcommands needs cfg and + // activeConfigPath - version and help need neither, so they are + // still answered immediately rather than waiting on a config load + // and write they have no use for. + if rest := fs.Args(); len(rest) > 0 && rest[0] != "keychain" { return subcommand(rest[0], rest[1:], stdout) } @@ -190,6 +206,24 @@ func run(ctx context.Context, args []string, stdin io.Reader, stdout, stderr io. switch { case err == nil: case errors.Is(err, config.ErrNoAPIKey): + // api_key_keychain (issue #67) is tried before falling back to the + // stub: a reader who migrated to a Keychain reference and removed + // api_key from the file should start normally, not be told the + // credential is missing when it is one Lookup away. + if cfg.APIKeyKeychain != "" { + key, kerr := cfg.ResolveAPIKey(ctx, keychainLookup) + if kerr == nil { + cfg.APIKey = key + break + } + // A configured reference that fails to resolve (locked + // Keychain, denied access, the item renamed or removed) is + // reported here rather than silently falling through to the + // stub, which would read as "no credential configured" when + // one plainly is - a reader debugging this needs to see which + // of the two states they are actually in. + return fmt.Errorf("resolving api_key_keychain %q: %w", cfg.APIKeyKeychain, kerr) + } // A missing key does not stop startup. Keep an explicit empty member in the // file the reader can edit, and let the interface report the missing value. if err := config.EnsureAPIKeyStub(activeConfigPath); err != nil { @@ -207,6 +241,10 @@ func run(ctx context.Context, args []string, stdin io.Reader, stdout, stderr io. return err } + if rest := fs.Args(); len(rest) > 0 && rest[0] == "keychain" { + return runKeychainCommand(ctx, rest[1:], stdin, stdout, cfg, activeConfigPath) + } + approval, err := cfg.ApprovalMode() if err != nil { // The file named a mode this client does not know. Substituting a default @@ -625,8 +663,11 @@ func readable(path string) error { // subcommand runs a non-interactive command. // -// There are two, and both print and stop. They exist so a script can ask what this -// build is without starting a session that will not finish. +// version and help print and stop; they exist so a script can ask what this +// build is without starting a session that will not finish. keychain is +// dispatched separately, in run itself, because it needs the loaded +// configuration and this function does not have it (see run's own comment +// at that call site). func subcommand(name string, _ []string, stdout io.Writer) error { switch name { case "version": @@ -639,7 +680,7 @@ func subcommand(name string, _ []string, stdout io.Writer) error { // An unknown name is refused by name rather than treated as a prompt, since // a script that passed the wrong word has a bug in it that running an // interactive session would hide. - return fmt.Errorf("unknown command %q: the commands are version and help", name) + return fmt.Errorf("unknown command %q: the commands are version, help and keychain", name) } } @@ -655,6 +696,9 @@ usage: orcli open the interface orcli version print the version orcli help print this message + orcli keychain set NAME ACCOUNT SERVICE add/update a named macOS Keychain reference + orcli keychain list list configured names (never secret values) + orcli keychain delete NAME remove a named reference and its Keychain item flags: --version print the version and stop diff --git a/cmd/orcli/main_test.go b/cmd/orcli/main_test.go index 292554d..7a01130 100644 --- a/cmd/orcli/main_test.go +++ b/cmd/orcli/main_test.go @@ -715,6 +715,79 @@ func writeConfig(t *testing.T, body string) string { return path } +// TestStartupResolvesAPIKeyFromKeychainWhenConfigured is issue #67's own +// runtime path: a file with api_key_keychain set and no inline api_key +// should start normally, using the Keychain-resolved credential, rather +// than being treated as having no credential at all. +func TestStartupResolvesAPIKeyFromKeychainWhenConfigured(t *testing.T) { + withHome(t, func() { + writeConfig(t, `{"api_key_keychain":"openrouter","keychain":{"items":{"openrouter":{"account":"john@smith.com","service":"private-openrouter-api"}}}}`) + old := keychainLookup + keychainLookup = func(ctx context.Context, ref config.KeychainRef) (string, error) { + if ref.Account != "john@smith.com" || ref.Service != "private-openrouter-api" { + t.Errorf("lookup called with %+v", ref) + } + return "resolved-from-keychain", nil + } + t.Cleanup(func() { keychainLookup = old }) + + if _, _, err := runIn(t, false); err != nil { + t.Fatalf("run: %v", err) + } + data, readErr := os.ReadFile(config.SearchOrder[0]) + if readErr != nil { + t.Fatalf("read config: %v", readErr) + } + if strings.Contains(string(data), `"api_key": ""`) { + t.Error("the empty api_key stub was written despite a resolvable api_key_keychain") + } + }) +} + +// TestStartupReportsAFailedKeychainResolutionRatherThanFallingBackSilently +// proves the deliberate choice in run's own ErrNoAPIKey case: a configured +// api_key_keychain that fails to resolve is a startup error naming the +// reference, not a silent fall-through to "no credential configured". +func TestStartupReportsAFailedKeychainResolutionRatherThanFallingBackSilently(t *testing.T) { + withHome(t, func() { + writeConfig(t, `{"api_key_keychain":"openrouter","keychain":{"items":{"openrouter":{"account":"a","service":"s"}}}}`) + old := keychainLookup + sentinel := errors.New("keychain: locked") + keychainLookup = func(ctx context.Context, ref config.KeychainRef) (string, error) { + return "", sentinel + } + t.Cleanup(func() { keychainLookup = old }) + + _, _, err := runIn(t, false) + if !errors.Is(err, sentinel) || !strings.Contains(err.Error(), "openrouter") { + t.Fatalf("run() error = %v, want it to wrap the lookup failure and name %q", err, "openrouter") + } + data, readErr := os.ReadFile(config.SearchOrder[0]) + if readErr != nil { + t.Fatalf("read config: %v", readErr) + } + if strings.Contains(string(data), `"api_key": ""`) { + t.Error("the empty api_key stub was written over a configured (if currently failing) api_key_keychain") + } + }) +} + +// TestKeychainSubcommandDispatchesThroughRun proves `orcli keychain list` +// reaches runKeychainCommand end to end, with the configuration run itself +// loaded rather than a hand-built one. +func TestKeychainSubcommandDispatchesThroughRun(t *testing.T) { + withHome(t, func() { + writeConfig(t, `{"api_key":"k","keychain":{"items":{"openrouter":{"account":"a","service":"s"}}}}`) + out, _, err := runIn(t, false, "keychain", "list") + if err != nil { + t.Fatalf("run: %v", err) + } + if !strings.Contains(out, "openrouter") { + t.Errorf("output = %q, want it to mention the configured item", out) + } + }) +} + func TestMissingAPIKeyLeavesAnEmptyConfigStub(t *testing.T) { withHome(t, func() { _, _, err := runIn(t, false) diff --git a/go.mod b/go.mod index 1c3ffed..e1c8e9f 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ require ( github.com/gdamore/tcell/v2 v2.13.10 github.com/rivo/tview v0.42.0 github.com/yuin/goldmark v1.8.6 + golang.org/x/term v0.46.0 modernc.org/sqlite v1.60.1 ) @@ -61,7 +62,6 @@ require ( golang.org/x/sync v0.23.0 // indirect golang.org/x/sys v0.48.0 // indirect golang.org/x/telemetry v0.0.0-20260924152758-ed294f943157 // indirect - golang.org/x/term v0.46.0 // indirect golang.org/x/text v0.42.0 // indirect golang.org/x/tools v0.51.0 // indirect golang.org/x/vuln v1.8.0 // indirect diff --git a/internal/config/keychain.go b/internal/config/keychain.go new file mode 100644 index 0000000..b6f1873 --- /dev/null +++ b/internal/config/keychain.go @@ -0,0 +1,190 @@ +package config + +import ( + "context" + "encoding/json" + "fmt" +) + +// keychainKey is the member the Keychain block lives under, named once so a +// reader and a writer cannot disagree about the spelling. +const keychainKey = "keychain" + +// KeychainRef is one named Keychain item's account/service pair - the +// reference a configuration file carries instead of a credential (issue +// #67). It mirrors internal/keychain's own Ref, as a plain struct rather +// than a shared type: this package has no dependency on internal/keychain +// today (ResolveAPIKey takes a lookup function instead, see its own doc +// comment for why), and a reference that lives in a file is a different +// thing from a reference a lookup takes, even though the two are shaped the +// same. +type KeychainRef struct { + Account string `json:"account"` + Service string `json:"service"` +} + +// Keychain is the block naming every Keychain item this file knows about. +// +// Unlike Cloudflare/Notion/GitHub/Apiary above, this block is NOT kept as +// raw bytes. Those three are read-only from this package's own side - "the +// reader edits the file by hand" - and keeping their bytes verbatim is what +// lets a future member survive a round trip with no writer here to lose it. +// This block needs a writer (WriteKeychainItem, for the add/update flow +// issue #67 asks for), and unlike api_key or a model identifier, nothing +// under this key is ever a secret: an account and a service name are a +// reference, not a credential, so decoding and re-rendering this one block +// carries none of the risk a full-file re-encode would for the rest of the +// file (see WriteModel's own doc comment for that risk). +type Keychain struct { + // Items maps a name (what api_key_keychain, or a provider block's own + // future equivalent, points at) to the account/service pair that + // resolves it. + Items map[string]KeychainRef `json:"items,omitempty"` +} + +// readKeychain decodes the keychain block, used by Config.decode. +func readKeychain(raw json.RawMessage) (*Keychain, error) { + if isNull(raw) { + return nil, nil + } + var k Keychain + if err := json.Unmarshal(raw, &k); err != nil { + return nil, fmt.Errorf("%s: %w", keychainKey, err) + } + return &k, nil +} + +// WriteKeychainItem adds or updates one named Keychain reference. +// +// It never touches a secret value - only the account/service reference - so +// unlike WriteModel it has no key-material byte-copying concern of its own. +// It still edits only the "keychain" top-level member and leaves the rest +// of the file untouched, on the same grounds every writer here does: a +// reader's own formatting elsewhere in the file is not this package's to +// reflow. +func WriteKeychainItem(path, name string, ref KeychainRef) error { + if name == "" { + return fmt.Errorf("config: no keychain item name was given") + } + if ref.Account == "" || ref.Service == "" { + return fmt.Errorf("config: keychain item %q needs both an account and a service", name) + } + + data, err := readForEdit(path) + if err != nil { + return err + } + + k, err := currentKeychain(data) + if err != nil { + return err + } + if k.Items == nil { + k.Items = map[string]KeychainRef{} + } + k.Items[name] = ref + + return writeKeychainBlock(path, data, k) +} + +// DeleteKeychainItem removes one named reference from the file. It is the +// config-side half of issue #67's "deletion should remove only the +// specifically selected item" requirement - the Keychain-side half is +// internal/keychain.Delete, a separate call a caller makes alongside this +// one, since removing the reference and removing the secret are two +// different systems and a caller may want either without the other (for +// example, retiring a reference to an item kept for another tool). +func DeleteKeychainItem(path, name string) error { + if name == "" { + return fmt.Errorf("config: no keychain item name was given") + } + + data, err := readForEdit(path) + if err != nil { + return err + } + k, err := currentKeychain(data) + if err != nil { + return err + } + if _, ok := k.Items[name]; !ok { + return fmt.Errorf("config: keychain item %q is not configured", name) + } + delete(k.Items, name) + + return writeKeychainBlock(path, data, k) +} + +// currentKeychain reads the keychain block already in data, or an empty one +// if the file has none yet. +func currentKeychain(data []byte) (Keychain, error) { + span, found := findMember(data, keychainKey) + if !found { + return Keychain{}, nil + } + var k Keychain + if err := json.Unmarshal(data[span.start:span.end], &k); err != nil { + return Keychain{}, fmt.Errorf("config: the existing %s member does not parse: %w", keychainKey, err) + } + return k, nil +} + +// writeKeychainBlock renders k and sets it as data's own "keychain" member. +func writeKeychainBlock(path string, data []byte, k Keychain) error { + rendered, err := json.Marshal(k) + if err != nil { + return fmt.Errorf("config: rendering the keychain block: %w", err) + } + edited, err := setMember(data, keychainKey, string(rendered)) + if err != nil { + return err + } + if err := checkMember(edited, keychainKey, string(rendered)); err != nil { + return fmt.Errorf("config: the edit did not verify: %w", err) + } + return osWriteFile(path, edited) +} + +// KeychainLookupFunc resolves one Keychain reference to a secret. +// +// ResolveAPIKey takes this as a parameter rather than importing +// internal/keychain directly, because this package is read by every command +// (including ones that never touch a credential, like /level) and has never +// depended on anything that shells out to an external program. A caller in +// cmd/orcli passes keychain.Lookup, adapted to this signature, at the one +// call site that actually needs a live Keychain - everywhere else in this +// package keeps testing configuration parsing without a subprocess. +type KeychainLookupFunc func(ctx context.Context, ref KeychainRef) (string, error) + +// ResolveAPIKey returns the credential a session should start with. +// +// Precedence is deliberate and is issue #67's own open question, settled +// here: an inline api_key, when present, always wins over api_key_keychain. +// This is the least surprising choice for a reader migrating one field at a +// time - adding api_key_keychain to a file that still carries a stale +// api_key must not silently stop using the value the reader can still see +// in the file, since a security-relevant field going quiet with no error is +// exactly the failure mode this whole feature exists to avoid elsewhere. +// Migrating means removing api_key from the file, not just adding +// api_key_keychain beside it. +// +// An empty api_key with no api_key_keychain returns ErrNoAPIKey unchanged, +// which is the same sentinel Load already returns for that case - this +// function is meant to be called after Load's own ErrNoAPIKey, as a second +// attempt, not as a replacement for the check Load already makes. +func (c Config) ResolveAPIKey(ctx context.Context, lookup KeychainLookupFunc) (string, error) { + if c.APIKey != "" { + return c.APIKey, nil + } + if c.APIKeyKeychain == "" { + return "", ErrNoAPIKey + } + if c.Keychain == nil { + return "", fmt.Errorf("config: api_key_keychain names %q, but this file has no keychain.items block", c.APIKeyKeychain) + } + ref, ok := c.Keychain.Items[c.APIKeyKeychain] + if !ok { + return "", fmt.Errorf("config: api_key_keychain names %q, which is not in keychain.items", c.APIKeyKeychain) + } + return lookup(ctx, ref) +} diff --git a/internal/config/keychain_test.go b/internal/config/keychain_test.go new file mode 100644 index 0000000..abe1374 --- /dev/null +++ b/internal/config/keychain_test.go @@ -0,0 +1,218 @@ +package config + +import ( + "context" + "errors" + "path/filepath" + "strings" + "testing" +) + +func TestKeychainBlockIsReadAndEnumerable(t *testing.T) { + h := home(t) + writeConfigAt(t, h, `{"api_key":"inline-key","keychain":{"items":{"openrouter":{"account":"john@smith.com","service":"private-openrouter-api"},"test":{"account":"john@smith.com","service":"orcli-test-key"}}}}`) + + cfg, err := Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if cfg.Keychain == nil { + t.Fatal("Keychain is nil") + } + if len(cfg.Keychain.Items) != 2 { + t.Fatalf("Items = %v, want 2 entries", cfg.Keychain.Items) + } + got := cfg.Keychain.Items["openrouter"] + want := KeychainRef{Account: "john@smith.com", Service: "private-openrouter-api"} + if got != want { + t.Errorf("Items[openrouter] = %+v, want %+v", got, want) + } +} + +func TestResolveAPIKey_InlineAlwaysWinsOverKeychain(t *testing.T) { + cfg := Config{ + APIKey: "inline-key", + APIKeyKeychain: "openrouter", + Keychain: &Keychain{Items: map[string]KeychainRef{"openrouter": {Account: "a", Service: "s"}}}, + } + called := false + lookup := func(ctx context.Context, ref KeychainRef) (string, error) { + called = true + return "from-keychain", nil + } + + got, err := cfg.ResolveAPIKey(context.Background(), lookup) + if err != nil { + t.Fatalf("ResolveAPIKey: %v", err) + } + if got != "inline-key" { + t.Errorf("ResolveAPIKey() = %q, want the inline key", got) + } + if called { + t.Error("the keychain lookup ran despite an inline api_key being present") + } +} + +func TestResolveAPIKey_FallsBackToKeychainWhenNoInlineKey(t *testing.T) { + ref := KeychainRef{Account: "john@smith.com", Service: "private-openrouter-api"} + cfg := Config{ + APIKeyKeychain: "openrouter", + Keychain: &Keychain{Items: map[string]KeychainRef{"openrouter": ref}}, + } + var gotRef KeychainRef + lookup := func(ctx context.Context, r KeychainRef) (string, error) { + gotRef = r + return "resolved-secret", nil + } + + got, err := cfg.ResolveAPIKey(context.Background(), lookup) + if err != nil { + t.Fatalf("ResolveAPIKey: %v", err) + } + if got != "resolved-secret" { + t.Errorf("ResolveAPIKey() = %q, want the keychain-resolved secret", got) + } + if gotRef != ref { + t.Errorf("lookup was called with %+v, want %+v", gotRef, ref) + } +} + +func TestResolveAPIKey_NoInlineNoKeychainReturnsErrNoAPIKey(t *testing.T) { + cfg := Config{} + _, err := cfg.ResolveAPIKey(context.Background(), func(context.Context, KeychainRef) (string, error) { + t.Fatal("lookup should not be called with no api_key_keychain configured") + return "", nil + }) + if !errors.Is(err, ErrNoAPIKey) { + t.Errorf("ResolveAPIKey() = %v, want ErrNoAPIKey", err) + } +} + +func TestResolveAPIKey_NamedItemMissingFromKeychainBlock(t *testing.T) { + cfg := Config{ + APIKeyKeychain: "openrouter", + Keychain: &Keychain{Items: map[string]KeychainRef{"groq": {Account: "a", Service: "s"}}}, + } + _, err := cfg.ResolveAPIKey(context.Background(), func(context.Context, KeychainRef) (string, error) { + t.Fatal("lookup should not be called for an unconfigured name") + return "", nil + }) + if err == nil || !strings.Contains(err.Error(), "openrouter") { + t.Errorf("ResolveAPIKey() = %v, want an error naming the missing item", err) + } +} + +func TestResolveAPIKey_NoKeychainBlockAtAll(t *testing.T) { + cfg := Config{APIKeyKeychain: "openrouter"} + _, err := cfg.ResolveAPIKey(context.Background(), func(context.Context, KeychainRef) (string, error) { + t.Fatal("lookup should not be called with no keychain block") + return "", nil + }) + if err == nil || !strings.Contains(err.Error(), "openrouter") { + t.Errorf("ResolveAPIKey() = %v, want an error naming what was asked for", err) + } +} + +func TestResolveAPIKey_PropagatesALookupFailureUnwrapped(t *testing.T) { + sentinel := errors.New("keychain: no matching item") + cfg := Config{ + APIKeyKeychain: "openrouter", + Keychain: &Keychain{Items: map[string]KeychainRef{"openrouter": {Account: "a", Service: "s"}}}, + } + _, err := cfg.ResolveAPIKey(context.Background(), func(context.Context, KeychainRef) (string, error) { + return "", sentinel + }) + if !errors.Is(err, sentinel) { + t.Errorf("ResolveAPIKey() = %v, want it to wrap/return the lookup's own error", err) + } +} + +func TestWriteKeychainItem_AddsAndUpdates(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "orcli.json") + writeConfig(t, path, `{"api_key":"k"}`, FileMode) + + if err := WriteKeychainItem(path, "openrouter", KeychainRef{Account: "john@smith.com", Service: "private-openrouter-api"}); err != nil { + t.Fatalf("WriteKeychainItem: %v", err) + } + cfg, _, err := LoadFromPath(path) + if err != nil { + t.Fatalf("LoadFromPath: %v", err) + } + if cfg.Keychain == nil || cfg.Keychain.Items["openrouter"] != (KeychainRef{Account: "john@smith.com", Service: "private-openrouter-api"}) { + t.Fatalf("item was not written: %+v", cfg.Keychain) + } + // api_key must survive untouched - this writer only edits the + // keychain member. + if cfg.APIKey != "k" { + t.Errorf("api_key was disturbed: %q", cfg.APIKey) + } + + // Update in place: adding a second named item must not disturb the + // first. + if err := WriteKeychainItem(path, "groq", KeychainRef{Account: "john@smith.com", Service: "private-groq-api"}); err != nil { + t.Fatalf("WriteKeychainItem (second): %v", err) + } + cfg, _, err = LoadFromPath(path) + if err != nil { + t.Fatalf("LoadFromPath (second): %v", err) + } + if len(cfg.Keychain.Items) != 2 { + t.Fatalf("Items = %v, want both entries present", cfg.Keychain.Items) + } +} + +func TestWriteKeychainItem_RefusesAnIncompleteReference(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "orcli.json") + writeConfig(t, path, `{"api_key":"k"}`, FileMode) + + if err := WriteKeychainItem(path, "openrouter", KeychainRef{Account: "john@smith.com"}); err == nil { + t.Error("expected a refusal for a reference with no service") + } +} + +func TestDeleteKeychainItem_RemovesOnlyTheNamedItem(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "orcli.json") + writeConfig(t, path, `{"api_key":"k"}`, FileMode) + + if err := WriteKeychainItem(path, "openrouter", KeychainRef{Account: "a", Service: "s1"}); err != nil { + t.Fatalf("WriteKeychainItem: %v", err) + } + if err := WriteKeychainItem(path, "groq", KeychainRef{Account: "a", Service: "s2"}); err != nil { + t.Fatalf("WriteKeychainItem: %v", err) + } + + if err := DeleteKeychainItem(path, "openrouter"); err != nil { + t.Fatalf("DeleteKeychainItem: %v", err) + } + cfg, _, err := LoadFromPath(path) + if err != nil { + t.Fatalf("LoadFromPath: %v", err) + } + if _, ok := cfg.Keychain.Items["openrouter"]; ok { + t.Error("deleted item is still present") + } + if _, ok := cfg.Keychain.Items["groq"]; !ok { + t.Error("the other item was removed too") + } +} + +func TestDeleteKeychainItem_RefusesAnUnknownName(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "orcli.json") + writeConfig(t, path, `{"api_key":"k","keychain":{"items":{"openrouter":{"account":"a","service":"s"}}}}`, FileMode) + + if err := DeleteKeychainItem(path, "groq"); err == nil { + t.Error("expected a refusal for a name that was never configured") + } + // And the existing item must survive the refused call untouched. + cfg, _, err := LoadFromPath(path) + if err != nil { + t.Fatalf("LoadFromPath: %v", err) + } + if _, ok := cfg.Keychain.Items["openrouter"]; !ok { + t.Error("the existing item was lost by a refused delete") + } +} diff --git a/internal/config/load.go b/internal/config/load.go index 28c8f08..98fb9d2 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -40,6 +40,18 @@ type Config struct { // APIKey is the credential. It is never read from the environment. APIKey string `json:"api_key,omitempty"` + // APIKeyKeychain names an entry in Keychain's own Items map that + // resolves to the credential when APIKey itself is empty (issue + // #67). See ResolveAPIKey for the precedence between the two. + APIKeyKeychain string `json:"api_key_keychain,omitempty"` + + // Keychain holds the named macOS Keychain account/service + // references APIKeyKeychain (and, in future, a per-provider + // block's own equivalent) resolves against. See keychain.go's own + // doc comment for why this block is decoded rather than kept as + // raw bytes like Cloudflare/Notion/GitHub/Apiary below. + Keychain *Keychain `json:"keychain,omitempty"` + // Model is the model identifier a session starts with. Model string `json:"model,omitempty"` @@ -330,6 +342,8 @@ func (c *Config) decode(raw map[string]json.RawMessage) error { switch key { case "api_key": err = readString(value, &c.APIKey) + case "api_key_keychain": + err = readString(value, &c.APIKeyKeychain) case "model": err = readString(value, &c.Model) case "provider": @@ -374,6 +388,8 @@ func (c *Config) decode(raw map[string]json.RawMessage) error { c.Apiary, err = readApiary(value) case mcpKey: c.MCP, err = readMCP(value) + case keychainKey: + c.Keychain, err = readKeychain(value) } if err != nil { return fmt.Errorf("%s: %w", key, err) diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go new file mode 100644 index 0000000..e9c5f37 --- /dev/null +++ b/internal/keychain/keychain.go @@ -0,0 +1,198 @@ +// Package keychain resolves a secret from the macOS Keychain by shelling out +// to the `security` command line tool, as issue #67's own design question +// settles it: the Security framework's native API has no stable, unprivileged +// cgo-free binding this project can reach without adding a cgo dependency to +// a client that otherwise has none, and `security find-generic-password` +// already does exactly the lookup this package needs. +// +// Every call here goes through exec.CommandContext with an argument array, +// never a shell - a configured account or service name reaches argv as a +// single element, never as text interpolated into a command line, so a name +// containing a quote or a semicolon is passed as-is rather than parsed as +// shell syntax. +// +// The one invariant every function here is built to keep: a secret value +// never appears in a returned error. Lookup separates the command's stdout +// (the secret) from its stderr (the diagnostic), and only stderr - plus the +// account/service the caller supplied, which is a reference, not a credential +// - is allowed into an error. Set and Delete never see the secret's bytes +// again once it reaches argv, so they have nothing to leak by construction. +package keychain + +import ( + "bytes" + "context" + "errors" + "fmt" + "os/exec" + "runtime" + "strings" +) + +// Ref names one Keychain item: the account and service (item name) pair +// `security find-generic-password`'s own -a and -s flags match against. +// Neither is a secret - both are the reference a configuration file is +// meant to carry instead of the credential itself (issue #67). +type Ref struct { + Account string + Service string +} + +// Empty reports whether r names no item at all, as opposed to one this +// Keychain happens not to have - a caller checks this before Lookup to tell +// "not configured" from "configured and missing" apart, which need different +// messages. +func (r Ref) Empty() bool { + return r.Account == "" && r.Service == "" +} + +func (r Ref) String() string { + return fmt.Sprintf("account=%q service=%q", r.Account, r.Service) +} + +// ErrUnsupported is returned on any OS but macOS. `security` is a macOS-only +// binary, and a clear refusal here is more useful than the "executable file +// not found in $PATH" a bare exec would produce. +var ErrUnsupported = errors.New("keychain: only supported on macOS") + +// ErrNotFound is returned when the Keychain has no item matching a Ref. +var ErrNotFound = errors.New("keychain: no matching item") + +// ErrIncompleteRef is returned when a Ref is missing its account or its +// service - find-generic-password needs both to be a narrow, correct match +// rather than the first same-service item it happens to find. +var ErrIncompleteRef = errors.New("keychain: a reference needs both an account and a service") + +// goos is runtime.GOOS, held as a variable so a test can exercise the +// unsupported-platform path on whatever OS actually runs the test suite +// (this repository's own CI runs on Linux, never macOS). +var goos = runtime.GOOS + +// runner is the subprocess seam every exported function goes through, and +// the one a test replaces. It is a function type rather than an interface +// because every call here has the same shape: a program name, its +// arguments, and stdout/stderr returned separately so a caller can decide +// which of the two is safe to put in an error. +type runner func(ctx context.Context, name string, args ...string) (stdout, stderr []byte, err error) + +// run is the real subprocess runner. Replaced in tests with one that never +// touches an actual Keychain. +var run runner = execRun + +func execRun(ctx context.Context, name string, args ...string) (stdout, stderr []byte, err error) { + cmd := exec.CommandContext(ctx, name, args...) + var outBuf, errBuf bytes.Buffer + cmd.Stdout = &outBuf + cmd.Stderr = &errBuf + err = cmd.Run() + return outBuf.Bytes(), errBuf.Bytes(), err +} + +// securityNotFoundExitCode is `security`'s own exit status for +// errSecItemNotFound (-25300), documented in Apple's Security framework +// headers and the status find-generic-password/delete-generic-password both +// return for no match. +const securityNotFoundExitCode = 44 + +// isNotFound reports whether err is security's own "no such item" outcome, +// checked by exit code first (the stable signal across macOS releases) and +// by the one stderr phrase security has used for this outcome as a fallback, +// in case a future release changes the exit code but not the wording. +func isNotFound(err error, stderr []byte) bool { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == securityNotFoundExitCode { + return true + } + return bytes.Contains(stderr, []byte("could not be found in the keychain")) +} + +// commandErr builds the error Lookup/Set/Delete return for a failed +// command. It is handed the reference and stderr, and explicitly NOT the +// command's stdout - the one place a secret could otherwise leak through is +// a caller passing stdout in here by mistake, and this signature makes that +// impossible rather than merely documented against. +func commandErr(verb string, ref Ref, stderr []byte, err error) error { + detail := strings.TrimSpace(string(stderr)) + if detail == "" { + detail = err.Error() + } + return fmt.Errorf("keychain: %s %s: %s", verb, ref, detail) +} + +// Lookup reads the secret Ref names. +// +// The secret is returned as the function's own value, never embedded in an +// error - a caller that logs an error from this function cannot log a +// secret by doing so, because the secret is never in one. +func Lookup(ctx context.Context, ref Ref) (string, error) { + if goos != "darwin" { + return "", ErrUnsupported + } + if ref.Account == "" || ref.Service == "" { + return "", fmt.Errorf("%w (got %s)", ErrIncompleteRef, ref) + } + stdout, stderr, err := run(ctx, "security", "find-generic-password", + "-a", ref.Account, "-s", ref.Service, "-w") + if err != nil { + if isNotFound(err, stderr) { + return "", fmt.Errorf("%w: %s", ErrNotFound, ref) + } + return "", commandErr("looking up", ref, stderr, err) + } + // `-w` prints the secret followed by exactly one trailing newline; + // TrimRight rather than TrimSpace, since a secret itself may legitimately + // carry leading or trailing spaces that are not this package's to guess + // are insignificant. + return strings.TrimRight(string(stdout), "\n"), nil +} + +// Set writes secret under ref, creating the item if none exists and +// updating it in place (`-U`) if one already does - the same +// create-or-update shape this project's own config writers use, so "set a +// credential" is one operation rather than two the caller has to choose +// between. +// +// secret reaches the `security` process as one argv element, which is the +// documented limitation of driving this particular CLI rather than the +// Security framework's own API directly (see this package's own doc +// comment): it is visible to another local process briefly through that +// process's own command line, the same exposure every other tool built on +// this CLI (including Apple's own documentation for it) carries. It is +// never written to this process's own logs, stdout, or any error this +// package returns. +func Set(ctx context.Context, ref Ref, secret string) error { + if goos != "darwin" { + return ErrUnsupported + } + if ref.Account == "" || ref.Service == "" { + return fmt.Errorf("%w (got %s)", ErrIncompleteRef, ref) + } + _, stderr, err := run(ctx, "security", "add-generic-password", + "-a", ref.Account, "-s", ref.Service, "-w", secret, "-U") + if err != nil { + return commandErr("writing", ref, stderr, err) + } + return nil +} + +// Delete removes the item Ref names. Only that one item: `security +// delete-generic-password` matches on the same -a/-s pair Lookup and Set +// use, so a Delete for one named credential cannot reach a different one +// that happens to share only an account or only a service with it. +func Delete(ctx context.Context, ref Ref) error { + if goos != "darwin" { + return ErrUnsupported + } + if ref.Account == "" || ref.Service == "" { + return fmt.Errorf("%w (got %s)", ErrIncompleteRef, ref) + } + _, stderr, err := run(ctx, "security", "delete-generic-password", + "-a", ref.Account, "-s", ref.Service) + if err != nil { + if isNotFound(err, stderr) { + return fmt.Errorf("%w: %s", ErrNotFound, ref) + } + return commandErr("deleting", ref, stderr, err) + } + return nil +} diff --git a/internal/keychain/keychain_test.go b/internal/keychain/keychain_test.go new file mode 100644 index 0000000..0fe82fe --- /dev/null +++ b/internal/keychain/keychain_test.go @@ -0,0 +1,254 @@ +package keychain + +import ( + "context" + "errors" + "os/exec" + "strings" + "testing" +) + +// withFakeRunner replaces run for the duration of one test and restores it +// afterward, so tests never touch a real Keychain (and pass identically on +// Linux CI, which has no `security` binary at all). +func withFakeRunner(t *testing.T, fn func(ctx context.Context, name string, args ...string) (stdout, stderr []byte, err error)) { + t.Helper() + old := run + run = fn + t.Cleanup(func() { run = old }) +} + +// withDarwin forces goos to "darwin" for the duration of one test, so the +// platform-gated paths are reachable from any host running this suite. +func withDarwin(t *testing.T) { + t.Helper() + old := goos + goos = "darwin" + t.Cleanup(func() { goos = old }) +} + +// fakeExitError builds an *exec.ExitError carrying the given exit code, +// the only shape isNotFound actually inspects. +func fakeExitError(t *testing.T, code int) error { + t.Helper() + // The simplest portable way to obtain a real *exec.ExitError with a + // chosen exit code is to actually run a shell that exits with it - + // this still never touches the Keychain or any fake of it. + cmd := exec.Command("sh", "-c", "exit "+itoa(code)) + err := cmd.Run() + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) { + t.Fatalf("expected an *exec.ExitError from a deliberately failing command, got %v (%T)", err, err) + } + return exitErr +} + +func itoa(n int) string { + if n == 0 { + return "0" + } + digits := "" + for n > 0 { + digits = string(rune('0'+n%10)) + digits + n /= 10 + } + return digits +} + +func TestLookup_UnsupportedOffDarwin(t *testing.T) { + old := goos + goos = "linux" + t.Cleanup(func() { goos = old }) + + if _, err := Lookup(context.Background(), Ref{Account: "a", Service: "s"}); !errors.Is(err, ErrUnsupported) { + t.Errorf("Lookup() on a non-darwin goos = %v, want ErrUnsupported", err) + } +} + +func TestLookup_IncompleteRefIsRefusedWithoutRunningAnything(t *testing.T) { + withDarwin(t) + withFakeRunner(t, func(ctx context.Context, name string, args ...string) ([]byte, []byte, error) { + t.Fatal("run was called for an incomplete reference; it should have been refused first") + return nil, nil, nil + }) + + for _, ref := range []Ref{{}, {Account: "only-account"}, {Service: "only-service"}} { + if _, err := Lookup(context.Background(), ref); !errors.Is(err, ErrIncompleteRef) { + t.Errorf("Lookup(%+v) = %v, want ErrIncompleteRef", ref, err) + } + } +} + +func TestLookup_SuccessReturnsTheSecretTrimmedOfItsTrailingNewline(t *testing.T) { + withDarwin(t) + ref := Ref{Account: "john@smith.com", Service: "private-cloudflare-api"} + withFakeRunner(t, func(ctx context.Context, name string, args ...string) ([]byte, []byte, error) { + if name != "security" { + t.Errorf("program = %q, want security", name) + } + want := []string{"find-generic-password", "-a", ref.Account, "-s", ref.Service, "-w"} + if len(args) != len(want) { + t.Fatalf("args = %v, want %v", args, want) + } + for i, a := range want { + if args[i] != a { + t.Errorf("args[%d] = %q, want %q", i, args[i], a) + } + } + return []byte("super-secret-token\n"), nil, nil + }) + + got, err := Lookup(context.Background(), ref) + if err != nil { + t.Fatalf("Lookup() error = %v", err) + } + if got != "super-secret-token" { + t.Errorf("Lookup() = %q, want %q", got, "super-secret-token") + } +} + +func TestLookup_NotFoundByExitCode(t *testing.T) { + withDarwin(t) + withFakeRunner(t, func(ctx context.Context, name string, args ...string) ([]byte, []byte, error) { + return nil, []byte("SecKeychainSearchCopyNext: The specified item could not be found in the keychain."), + fakeExitError(t, 44) + }) + + _, err := Lookup(context.Background(), Ref{Account: "a", Service: "s"}) + if !errors.Is(err, ErrNotFound) { + t.Errorf("Lookup() error = %v, want ErrNotFound", err) + } +} + +// TestLookup_FailureNeverLeaksStdout is the load-bearing test in this +// package (issue #67's own acceptance criteria): a command that fails after +// having already written something to stdout - a shape real `security` +// should never produce, but this proves the package does not trust that - +// must not let that stdout reach the returned error. +func TestLookup_FailureNeverLeaksStdout(t *testing.T) { + withDarwin(t) + const leakCandidate = "THIS-MUST-NEVER-APPEAR-IN-AN-ERROR" + withFakeRunner(t, func(ctx context.Context, name string, args ...string) ([]byte, []byte, error) { + return []byte(leakCandidate), []byte("some other diagnostic"), fakeExitError(t, 1) + }) + + _, err := Lookup(context.Background(), Ref{Account: "a", Service: "s"}) + if err == nil { + t.Fatal("expected an error") + } + if strings.Contains(err.Error(), leakCandidate) { + t.Errorf("the returned error leaked stdout: %v", err) + } +} + +func TestLookup_GenericFailureNamesTheReferenceNotTheSecret(t *testing.T) { + withDarwin(t) + ref := Ref{Account: "john@smith.com", Service: "private-cloudflare-api"} + withFakeRunner(t, func(ctx context.Context, name string, args ...string) ([]byte, []byte, error) { + return nil, []byte("Keychain could not be unlocked."), fakeExitError(t, 1) + }) + + _, err := Lookup(context.Background(), ref) + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), "john@smith.com") || !strings.Contains(err.Error(), "private-cloudflare-api") { + t.Errorf("error does not identify the lookup reference: %v", err) + } + if !strings.Contains(err.Error(), "could not be unlocked") { + t.Errorf("error does not carry the diagnostic: %v", err) + } +} + +func TestSet_SendsTheSecretAsOneArgument(t *testing.T) { + withDarwin(t) + ref := Ref{Account: "a", Service: "s"} + var gotArgs []string + withFakeRunner(t, func(ctx context.Context, name string, args ...string) ([]byte, []byte, error) { + gotArgs = args + return nil, nil, nil + }) + + if err := Set(context.Background(), ref, "the-secret"); err != nil { + t.Fatalf("Set() error = %v", err) + } + want := []string{"add-generic-password", "-a", "a", "-s", "s", "-w", "the-secret", "-U"} + if len(gotArgs) != len(want) { + t.Fatalf("args = %v, want %v", gotArgs, want) + } + for i, a := range want { + if gotArgs[i] != a { + t.Errorf("args[%d] = %q, want %q", i, gotArgs[i], a) + } + } +} + +func TestSet_FailureNeverLeaksTheSecret(t *testing.T) { + withDarwin(t) + const secret = "THIS-MUST-NEVER-APPEAR-IN-AN-ERROR" + withFakeRunner(t, func(ctx context.Context, name string, args ...string) ([]byte, []byte, error) { + return nil, []byte("some diagnostic"), fakeExitError(t, 1) + }) + + err := Set(context.Background(), Ref{Account: "a", Service: "s"}, secret) + if err == nil { + t.Fatal("expected an error") + } + if strings.Contains(err.Error(), secret) { + t.Errorf("the returned error leaked the secret: %v", err) + } +} + +func TestSet_IncompleteRefIsRefusedWithoutRunningAnything(t *testing.T) { + withDarwin(t) + withFakeRunner(t, func(ctx context.Context, name string, args ...string) ([]byte, []byte, error) { + t.Fatal("run was called for an incomplete reference; it should have been refused first") + return nil, nil, nil + }) + + if err := Set(context.Background(), Ref{Account: "only-account"}, "x"); !errors.Is(err, ErrIncompleteRef) { + t.Errorf("Set() = %v, want ErrIncompleteRef", err) + } +} + +func TestDelete_RemovesExactlyTheNamedItem(t *testing.T) { + withDarwin(t) + ref := Ref{Account: "a", Service: "s"} + var gotArgs []string + withFakeRunner(t, func(ctx context.Context, name string, args ...string) ([]byte, []byte, error) { + gotArgs = args + return nil, nil, nil + }) + + if err := Delete(context.Background(), ref); err != nil { + t.Fatalf("Delete() error = %v", err) + } + want := []string{"delete-generic-password", "-a", "a", "-s", "s"} + if len(gotArgs) != len(want) { + t.Fatalf("args = %v, want %v", gotArgs, want) + } + for i, a := range want { + if gotArgs[i] != a { + t.Errorf("args[%d] = %q, want %q", i, gotArgs[i], a) + } + } +} + +func TestDelete_NotFound(t *testing.T) { + withDarwin(t) + withFakeRunner(t, func(ctx context.Context, name string, args ...string) ([]byte, []byte, error) { + return nil, []byte("could not be found in the keychain"), fakeExitError(t, 44) + }) + + if err := Delete(context.Background(), Ref{Account: "a", Service: "s"}); !errors.Is(err, ErrNotFound) { + t.Errorf("Delete() = %v, want ErrNotFound", err) + } +} + +func TestRef_String_NeverPanicsOnUnusualNames(t *testing.T) { + ref := Ref{Account: `weird" account`, Service: "svc; rm -rf /"} + s := ref.String() + if !strings.Contains(s, "account") || !strings.Contains(s, "svc") { + t.Errorf("String() = %q, want it to contain the unusual names verbatim", s) + } +} diff --git a/orcli.json.example b/orcli.json.example index 5b4d2c1..7a8f1d5 100644 --- a/orcli.json.example +++ b/orcli.json.example @@ -46,5 +46,17 @@ } } ] + }, + "keychain": { + "items": { + "openrouter": { + "account": "john@smith.com", + "service": "private-openrouter-api" + }, + "test": { + "account": "john@smith.com", + "service": "orcli-test-key" + } + } } }