From db53127ebdaf1d7b40a16aadf41460a7311a9cc6 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Tue, 7 Jul 2026 02:58:00 -0400 Subject: [PATCH 1/4] fix(keyring): pass generic password via stdin on macOS Pass the generic password secret to security add-generic-password via stdin instead of passing it as a command-line argument. This prevents the secret from leaking to the local process list (visible via ps) and aligns the macOS keyring implementation with the Linux secret-tool implementation. --- internal/keyring/keyring.go | 3 ++- internal/keyring/keyring_test.go | 31 ++++++++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/internal/keyring/keyring.go b/internal/keyring/keyring.go index 7f116b50f..38aae3c45 100644 --- a/internal/keyring/keyring.go +++ b/internal/keyring/keyring.go @@ -64,7 +64,8 @@ func (k *Keyring) Set(service, account, secret string) error { switch k.goos { case "darwin": // -U updates the item if it already exists rather than failing. - _, err := k.exec(nil, "security", "add-generic-password", "-U", "-s", service, "-a", account, "-w", secret) + // Pass the secret via stdin to prevent leaking it in the process list. + _, err := k.exec([]byte(secret), "security", "add-generic-password", "-U", "-s", service, "-a", account, "-w") return wrap("set", err) case "linux": // secret-tool reads the secret from stdin, keeping it out of the argv. diff --git a/internal/keyring/keyring_test.go b/internal/keyring/keyring_test.go index 01a09ebfc..a0b3173e0 100644 --- a/internal/keyring/keyring_test.go +++ b/internal/keyring/keyring_test.go @@ -61,7 +61,11 @@ func (f *fakeKeyring) run(_ context.Context, name string, stdin []byte, args ... svc, acct := flagValue(args, "-s"), flagValue(args, "-a") switch args[0] { case "add-generic-password": - f.data[key(svc, acct)] = flagValue(args, "-w") + password := flagValue(args, "-w") + if password == "" && len(args) > 0 && args[len(args)-1] == "-w" { + password = string(stdin) + } + f.data[key(svc, acct)] = password return nil, nil case "find-generic-password": if v, ok := f.data[key(svc, acct)]; ok { @@ -129,6 +133,31 @@ func TestKeyringRoundTripDarwin(t *testing.T) { } } +func TestKeyringRoundTripDarwinUsesStdin(t *testing.T) { + f := newFake("darwin") + k := f.keyring() + if err := k.Set("zero", "tokens", "blob-CCC"); err != nil { + t.Fatalf("Set: %v", err) + } + // The secret must travel via stdin, never the argument vector. + if f.lastStdin != "blob-CCC" { + t.Fatalf("secret not sent via stdin: stdin=%q", f.lastStdin) + } + for _, a := range f.lastArgs { + if strings.Contains(a, "blob-CCC") { + t.Fatalf("secret leaked into argv: %v", f.lastArgs) + } + } + got, ok, err := k.Get("zero", "tokens") + if err != nil || !ok || got != "blob-CCC" { + t.Fatalf("Get = %q ok=%v err=%v", got, ok, err) + } + existed, err := k.Delete("zero", "tokens") + if err != nil || !existed { + t.Fatalf("Delete: existed=%v err=%v", existed, err) + } +} + func TestKeyringRoundTripLinuxUsesStdin(t *testing.T) { f := newFake("linux") k := f.keyring() From 1a4a3737e557e0c57a93d7525b0bb484258c8c12 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Tue, 7 Jul 2026 03:35:53 -0400 Subject: [PATCH 2/4] fix(keyring): pipe secret twice for macOS security password+retype prompt The macOS security add-generic-password command prompts for both a password and a retype confirmation when -w has no trailing value. The previous stdin approach only piped the secret once, causing a passwords don't match failure. Write the secret twice separated by newlines to satisfy both prompts. --- internal/keyring/keyring.go | 5 ++++- internal/keyring/keyring_test.go | 12 +++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/internal/keyring/keyring.go b/internal/keyring/keyring.go index 38aae3c45..72471eb5b 100644 --- a/internal/keyring/keyring.go +++ b/internal/keyring/keyring.go @@ -65,7 +65,10 @@ func (k *Keyring) Set(service, account, secret string) error { case "darwin": // -U updates the item if it already exists rather than failing. // Pass the secret via stdin to prevent leaking it in the process list. - _, err := k.exec([]byte(secret), "security", "add-generic-password", "-U", "-s", service, "-a", account, "-w") + // A trailing -w without a value makes security prompt for password + retype, + // reading both from stdin, so we write the secret twice separated by a newline. + stdinPayload := []byte(secret + "\n" + secret + "\n") + _, err := k.exec(stdinPayload, "security", "add-generic-password", "-U", "-s", service, "-a", account, "-w") return wrap("set", err) case "linux": // secret-tool reads the secret from stdin, keeping it out of the argv. diff --git a/internal/keyring/keyring_test.go b/internal/keyring/keyring_test.go index a0b3173e0..68964d41b 100644 --- a/internal/keyring/keyring_test.go +++ b/internal/keyring/keyring_test.go @@ -63,7 +63,10 @@ func (f *fakeKeyring) run(_ context.Context, name string, stdin []byte, args ... case "add-generic-password": password := flagValue(args, "-w") if password == "" && len(args) > 0 && args[len(args)-1] == "-w" { - password = string(stdin) + // Mirror real security behavior: read the first line as + // the password (the second line is the retype confirmation). + lines := strings.SplitN(string(stdin), "\n", 2) + password = lines[0] } f.data[key(svc, acct)] = password return nil, nil @@ -140,8 +143,11 @@ func TestKeyringRoundTripDarwinUsesStdin(t *testing.T) { t.Fatalf("Set: %v", err) } // The secret must travel via stdin, never the argument vector. - if f.lastStdin != "blob-CCC" { - t.Fatalf("secret not sent via stdin: stdin=%q", f.lastStdin) + // The payload contains the secret twice (password + retype confirmation) + // separated by newlines, matching the real security prompt behavior. + wantStdin := "blob-CCC\nblob-CCC\n" + if f.lastStdin != wantStdin { + t.Fatalf("secret not sent via stdin correctly: stdin=%q, want=%q", f.lastStdin, wantStdin) } for _, a := range f.lastArgs { if strings.Contains(a, "blob-CCC") { From b893a724356ff7ac9849966b7254a64e81f1da18 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Tue, 7 Jul 2026 15:58:10 -0400 Subject: [PATCH 3/4] fix(keyring): reject multiline secrets on macOS instead of corrupting them Addresses jatmn's review: - security's -w password+retype prompt reads stdin line by line, so a secret containing its own newline can't be distinguished from the password/retype separator: the first line read back would silently truncate the stored value. Set now rejects a secret containing \r or \n on darwin before invoking security, rather than storing a corrupted value. Linux's secret-tool has no such restriction (reads the whole stdin payload), so it is unaffected. - Updated the package doc comment, which still described the old argv-based (process-list-exposed) macOS path this PR replaced. Added TestKeyringSetRejectsMultilineSecretOnDarwin and TestKeyringSetAllowsMultilineSecretOnLinux. --- internal/keyring/keyring.go | 18 ++++++++++++----- internal/keyring/keyring_test.go | 34 ++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/internal/keyring/keyring.go b/internal/keyring/keyring.go index 72471eb5b..cd3003a89 100644 --- a/internal/keyring/keyring.go +++ b/internal/keyring/keyring.go @@ -3,11 +3,12 @@ // macOS and `secret-tool` (libsecret) on Linux. It stores a single secret string // per (service, account). Windows and other platforms report unsupported. // -// It shells out to the OS tools rather than taking a third-party dependency. On -// macOS the secret is passed to `security` as an argument, so it is briefly -// visible to other processes via the process list; on Linux the secret is passed -// over stdin and is not exposed in the argument vector. Callers that need to keep -// a secret out of the process list on macOS should prefer the file backend. +// It shells out to the OS tools rather than taking a third-party dependency. +// On both macOS and Linux the secret is passed over stdin, never the argument +// vector, so it is not exposed via the process list. On macOS this relies on +// `security`'s interactive password+retype prompt, which is line-based, so a +// secret containing a newline is rejected by Set rather than silently +// truncated; Linux's secret-tool has no such restriction. package keyring import ( @@ -67,6 +68,13 @@ func (k *Keyring) Set(service, account, secret string) error { // Pass the secret via stdin to prevent leaking it in the process list. // A trailing -w without a value makes security prompt for password + retype, // reading both from stdin, so we write the secret twice separated by a newline. + // That prompt is line-based (getpass-style), so a secret containing its own + // newline cannot be told apart from the password/retype separator: the first + // line read back would silently truncate the stored value. Reject it instead + // of storing a corrupted secret. + if strings.ContainsAny(secret, "\r\n") { + return wrap("set", errors.New("secret must not contain newlines on macOS (security's password+retype prompt is line-based)")) + } stdinPayload := []byte(secret + "\n" + secret + "\n") _, err := k.exec(stdinPayload, "security", "add-generic-password", "-U", "-s", service, "-a", account, "-w") return wrap("set", err) diff --git a/internal/keyring/keyring_test.go b/internal/keyring/keyring_test.go index 68964d41b..f7863e56b 100644 --- a/internal/keyring/keyring_test.go +++ b/internal/keyring/keyring_test.go @@ -164,6 +164,40 @@ func TestKeyringRoundTripDarwinUsesStdin(t *testing.T) { } } +// TestKeyringSetRejectsMultilineSecretOnDarwin guards against silently +// truncating a secret containing a newline: security's -w password+retype +// prompt is line-based, so writing "secret\nsecret\n" to stdin reads only the +// text up to the first newline as the password. Set must reject this before +// ever invoking security, not store a corrupted value. +func TestKeyringSetRejectsMultilineSecretOnDarwin(t *testing.T) { + for _, secret := range []string{"line1\nline2", "line1\r\nline2", "trailing\n"} { + f := newFake("darwin") + k := f.keyring() + if err := k.Set("zero", "tokens", secret); err == nil { + t.Fatalf("Set(%q) = nil error, want rejection of the embedded newline", secret) + } + if f.lastArgs != nil { + t.Fatalf("Set(%q) should be rejected before invoking security, got args=%v", secret, f.lastArgs) + } + } +} + +// TestKeyringSetAllowsMultilineSecretOnLinux confirms the newline restriction +// is macOS-specific: secret-tool reads the whole stdin payload as the secret, +// with no line-based prompt to corrupt an embedded newline. +func TestKeyringSetAllowsMultilineSecretOnLinux(t *testing.T) { + f := newFake("linux") + k := f.keyring() + secret := "line1\nline2" + if err := k.Set("zero", "tokens", secret); err != nil { + t.Fatalf("Set: %v", err) + } + got, ok, err := k.Get("zero", "tokens") + if err != nil || !ok || got != secret { + t.Fatalf("Get = %q ok=%v err=%v, want %q", got, ok, err, secret) + } +} + func TestKeyringRoundTripLinuxUsesStdin(t *testing.T) { f := newFake("linux") k := f.keyring() From ef3e91d9931d577d803133c1a101240dbac35c55 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Thu, 9 Jul 2026 11:18:10 -0400 Subject: [PATCH 4/4] fix(keyring): drive security -i for macOS Set instead of the -w prompt The trailing -w prompt uses getpass(3), which reads from /dev/tty whenever the process has a controlling terminal, so the piped secret was ignored in any interactive session, and getpass's small fixed buffer would truncate long secrets (like the base64 OAuth blob) when the fallback did engage. Interactive mode reads whole commands from stdin, keeping argv at just -i. Arguments are quoted for SecurityTool's split_line parser (backslash and double quote escaped inside double quotes). The parser is line-based with a 4096-byte buffer, so Set rejects newlines and oversized payloads up front rather than corrupting the stored value. --- internal/keyring/keyring.go | 53 ++++++++---- internal/keyring/keyring_test.go | 136 ++++++++++++++++++++++++++----- 2 files changed, 152 insertions(+), 37 deletions(-) diff --git a/internal/keyring/keyring.go b/internal/keyring/keyring.go index cd3003a89..3c9f41166 100644 --- a/internal/keyring/keyring.go +++ b/internal/keyring/keyring.go @@ -5,10 +5,11 @@ // // It shells out to the OS tools rather than taking a third-party dependency. // On both macOS and Linux the secret is passed over stdin, never the argument -// vector, so it is not exposed via the process list. On macOS this relies on -// `security`'s interactive password+retype prompt, which is line-based, so a -// secret containing a newline is rejected by Set rather than silently -// truncated; Linux's secret-tool has no such restriction. +// vector, so it is not exposed via the process list. On macOS the write goes +// through `security -i` (interactive mode), whose command parser is line-based +// with a fixed 4096-byte line buffer, so Set rejects secrets containing +// newlines or exceeding that budget rather than silently corrupting them; +// Linux's secret-tool has no such restriction. package keyring import ( @@ -64,19 +65,28 @@ func (k *Keyring) Set(service, account, secret string) error { } switch k.goos { case "darwin": + // The secret must stay out of the argument vector (visible to every + // process via `ps`), but a trailing -w prompt is not a substitute: + // security prompts with getpass(3), which reads from /dev/tty whenever + // the process has a controlling terminal (ignoring a piped stdin) and + // truncates at getpass's small fixed buffer otherwise. Instead drive + // `security -i`: interactive mode reads whole commands from stdin, so + // argv is just ["-i"] and the secret rides inside the command line. + // That parser is line-based with a fixed 4096-byte buffer, so reject + // newlines and oversized payloads up front; an overlong line would be + // split and executed as two garbage commands, silently corrupting the + // stored value. + for _, s := range []string{service, account, secret} { + if strings.ContainsAny(s, "\r\n") { + return wrap("set", errors.New("service, account, and secret must not contain newlines on macOS (security -i is line-based)")) + } + } // -U updates the item if it already exists rather than failing. - // Pass the secret via stdin to prevent leaking it in the process list. - // A trailing -w without a value makes security prompt for password + retype, - // reading both from stdin, so we write the secret twice separated by a newline. - // That prompt is line-based (getpass-style), so a secret containing its own - // newline cannot be told apart from the password/retype separator: the first - // line read back would silently truncate the stored value. Reject it instead - // of storing a corrupted secret. - if strings.ContainsAny(secret, "\r\n") { - return wrap("set", errors.New("secret must not contain newlines on macOS (security's password+retype prompt is line-based)")) + line := "add-generic-password -U -s " + securityQuote(service) + " -a " + securityQuote(account) + " -w " + securityQuote(secret) + "\n" + if len(line) > securityMaxLine { + return wrap("set", fmt.Errorf("secret too large for the macOS security tool's %d-byte command line; use the file backend instead", securityMaxLine)) } - stdinPayload := []byte(secret + "\n" + secret + "\n") - _, err := k.exec(stdinPayload, "security", "add-generic-password", "-U", "-s", service, "-a", account, "-w") + _, err := k.exec([]byte(line), "security", "-i") return wrap("set", err) case "linux": // secret-tool reads the secret from stdin, keeping it out of the argv. @@ -152,6 +162,19 @@ func (k *Keyring) Delete(service, account string) (bool, error) { } } +// securityMaxLine is the most `security -i` can read as one command: its line +// buffer is MAX_LINE_LEN (4096) bytes in Apple's SecurityTool, one of which the +// terminating NUL consumes. +const securityMaxLine = 4095 + +// securityQuote wraps s for one argument of a `security -i` command line. The +// tool's parser (split_line in Apple's SecurityTool) treats a backslash inside +// double quotes as escaping the next character and the matching quote as the +// argument terminator, so those two characters are all that needs escaping. +func securityQuote(s string) string { + return `"` + strings.NewReplacer(`\`, `\\`, `"`, `\"`).Replace(s) + `"` +} + func (k *Keyring) exec(stdin []byte, name string, args ...string) ([]byte, error) { ctx, cancel := context.WithTimeout(context.Background(), commandTimeout) defer cancel() diff --git a/internal/keyring/keyring_test.go b/internal/keyring/keyring_test.go index f7863e56b..430d402ed 100644 --- a/internal/keyring/keyring_test.go +++ b/internal/keyring/keyring_test.go @@ -50,6 +50,52 @@ func attrValue(args []string, attr string) string { func key(service, account string) string { return service + "\x00" + account } +// splitSecurityLine mirrors split_line in Apple's SecurityTool so the fake +// parses `security -i` command lines the way the real tool does: whitespace +// separates arguments, double or single quotes group one, and a backslash +// escapes the next character both inside and outside quotes. +func splitSecurityLine(line string) []string { + var args []string + var cur strings.Builder + inArg, escaped := false, false + quote := byte(0) + for i := 0; i < len(line); i++ { + c := line[i] + switch { + case escaped: + cur.WriteByte(c) + escaped = false + case c == '\\': + escaped = true + inArg = true + case quote != 0: + if c != quote { + cur.WriteByte(c) + continue + } + args = append(args, cur.String()) + cur.Reset() + inArg, quote = false, 0 + case c == '"' || c == '\'': + quote = c + inArg = true + case c == ' ' || c == '\t': + if inArg { + args = append(args, cur.String()) + cur.Reset() + inArg = false + } + default: + cur.WriteByte(c) + inArg = true + } + } + if inArg { + args = append(args, cur.String()) + } + return args +} + func (f *fakeKeyring) run(_ context.Context, name string, stdin []byte, args ...string) ([]byte, error) { f.lastStdin = string(stdin) f.lastArgs = append([]string{name}, args...) @@ -58,17 +104,19 @@ func (f *fakeKeyring) run(_ context.Context, name string, stdin []byte, args ... } switch f.goos { case "darwin": - svc, acct := flagValue(args, "-s"), flagValue(args, "-a") - switch args[0] { - case "add-generic-password": - password := flagValue(args, "-w") - if password == "" && len(args) > 0 && args[len(args)-1] == "-w" { - // Mirror real security behavior: read the first line as - // the password (the second line is the retype confirmation). - lines := strings.SplitN(string(stdin), "\n", 2) - password = lines[0] + cmdArgs := args + if args[0] == "-i" { + // Interactive mode: the command arrives as one line on stdin. + line, _, _ := strings.Cut(string(stdin), "\n") + cmdArgs = splitSecurityLine(line) + if len(cmdArgs) == 0 { + return nil, fakeExit{1} } - f.data[key(svc, acct)] = password + } + svc, acct := flagValue(cmdArgs, "-s"), flagValue(cmdArgs, "-a") + switch cmdArgs[0] { + case "add-generic-password": + f.data[key(svc, acct)] = flagValue(cmdArgs, "-w") return nil, nil case "find-generic-password": if v, ok := f.data[key(svc, acct)]; ok { @@ -142,17 +190,16 @@ func TestKeyringRoundTripDarwinUsesStdin(t *testing.T) { if err := k.Set("zero", "tokens", "blob-CCC"); err != nil { t.Fatalf("Set: %v", err) } - // The secret must travel via stdin, never the argument vector. - // The payload contains the secret twice (password + retype confirmation) - // separated by newlines, matching the real security prompt behavior. - wantStdin := "blob-CCC\nblob-CCC\n" + // The secret must travel via stdin, never the argument vector: the write + // goes through `security -i`, whose whole command line (secret included) + // arrives on stdin while argv carries only the -i flag. + wantStdin := "add-generic-password -U -s \"zero\" -a \"tokens\" -w \"blob-CCC\"\n" if f.lastStdin != wantStdin { t.Fatalf("secret not sent via stdin correctly: stdin=%q, want=%q", f.lastStdin, wantStdin) } - for _, a := range f.lastArgs { - if strings.Contains(a, "blob-CCC") { - t.Fatalf("secret leaked into argv: %v", f.lastArgs) - } + wantArgs := []string{"security", "-i"} + if len(f.lastArgs) != len(wantArgs) || f.lastArgs[0] != wantArgs[0] || f.lastArgs[1] != wantArgs[1] { + t.Fatalf("argv = %v, want %v", f.lastArgs, wantArgs) } got, ok, err := k.Get("zero", "tokens") if err != nil || !ok || got != "blob-CCC" { @@ -165,10 +212,10 @@ func TestKeyringRoundTripDarwinUsesStdin(t *testing.T) { } // TestKeyringSetRejectsMultilineSecretOnDarwin guards against silently -// truncating a secret containing a newline: security's -w password+retype -// prompt is line-based, so writing "secret\nsecret\n" to stdin reads only the -// text up to the first newline as the password. Set must reject this before -// ever invoking security, not store a corrupted value. +// truncating a secret containing a newline: `security -i` reads one command +// per line, so an embedded newline would split the write into two garbage +// commands. Set must reject this before ever invoking security, not store a +// corrupted value. func TestKeyringSetRejectsMultilineSecretOnDarwin(t *testing.T) { for _, secret := range []string{"line1\nline2", "line1\r\nline2", "trailing\n"} { f := newFake("darwin") @@ -182,6 +229,51 @@ func TestKeyringSetRejectsMultilineSecretOnDarwin(t *testing.T) { } } +// TestKeyringDarwinQuotesSpecialCharacters proves the quoting survives the +// real tool's parser: the fake tokenizes stdin with a faithful mirror of +// SecurityTool's split_line, so a secret full of quotes, backslashes, and +// spaces must round-trip unchanged. +func TestKeyringDarwinQuotesSpecialCharacters(t *testing.T) { + for _, secret := range []string{ + `spa ced`, + `quo"te`, + `back\slash`, + `sin'gle`, + `mi"x'ed \" \\ end\`, + ` leading and trailing `, + } { + f := newFake("darwin") + k := f.keyring() + if err := k.Set("zero", "tokens", secret); err != nil { + t.Fatalf("Set(%q): %v", secret, err) + } + got, ok, err := k.Get("zero", "tokens") + if err != nil || !ok || got != secret { + t.Fatalf("Get after Set(%q) = %q ok=%v err=%v", secret, got, ok, err) + } + } +} + +// TestKeyringSetRejectsOversizedSecretOnDarwin guards the `security -i` line +// budget: MAX_LINE_LEN is 4096 bytes, and an overlong line would be split and +// executed as two garbage commands. A payload comfortably under the budget +// must succeed; one over it must be rejected before invoking security. +func TestKeyringSetRejectsOversizedSecretOnDarwin(t *testing.T) { + f := newFake("darwin") + k := f.keyring() + if err := k.Set("zero", "tokens", strings.Repeat("a", 4000)); err != nil { + t.Fatalf("Set(4000 bytes): %v", err) + } + f = newFake("darwin") + k = f.keyring() + if err := k.Set("zero", "tokens", strings.Repeat("a", 5000)); err == nil { + t.Fatal("Set(5000 bytes) = nil error, want rejection of the oversized line") + } + if f.lastArgs != nil { + t.Fatalf("oversized Set should be rejected before invoking security, got args=%v", f.lastArgs) + } +} + // TestKeyringSetAllowsMultilineSecretOnLinux confirms the newline restriction // is macOS-specific: secret-tool reads the whole stdin payload as the secret, // with no line-based prompt to corrupt an embedded newline.