fix(credstore): stabilize macOS Keychain reads across rebuilds

Route macOS Keychain secret reads through the system-signed security executable so access grants can persist across CLI rebuilds. Preserve native Keychain writes and management operations, and clarify the dedicated ACL-repair procedure.

Author: zzwong

credstore/osbackend_byteness.go

@@ -5,6 +5,7 @@ package credstore
 import (
 	"errors"
 	"fmt"
+	"runtime"
 
 	"github.com/byteness/keyring"
 )
@@ -15,7 +16,11 @@ func openKeyringBackend(kind Backend, cfg backendConfig) (keyringBackend, error)
 	if err != nil {
 		return nil, fmt.Errorf("keyring open %s: %w", kind, err)
 	}
-	return bytenessBackend{kr: kr}, nil
+	backend := bytenessBackend{kr: kr}
+	if kind == BackendKeychain && runtime.GOOS == "darwin" {
+		return keychainStableReadBackend{keyringBackend: backend, service: cfg.serviceName, read: readKeychainPassword}, nil
+	}
+	return backend, nil
 }
 
 func keyringConfigFromBackendConfig(cfg backendConfig) keyring.Config {

credstore/osbackend_keychain_stable_read.go

@@ -0,0 +1,58 @@
+//go:build cgo || windows
+
+package credstore
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"os/exec"
+	"time"
+)
+
+const keychainReadTimeout = 60 * time.Second
+
+// keychainStableReadBackend retains native writes, metadata, enumeration and
+// deletion. Only secret reads use the system-signed security executable, so
+// a grant can survive replacement of an ad-hoc-signed CLI binary.
+type keychainStableReadBackend struct {
+	keyringBackend
+	service string
+	read    func(string, string) ([]byte, error)
+}
+
+func (b keychainStableReadBackend) get(itemKey string) (keyringItem, error) {
+	data, err := b.read(b.service, itemKey)
+	if err != nil {
+		return keyringItem{}, err
+	}
+	return keyringItem{key: itemKey, data: data}, nil
+}
+
+func readKeychainPassword(service, account string) ([]byte, error) {
+	ctx, cancel := context.WithTimeout(context.Background(), keychainReadTimeout)
+	defer cancel()
+	return readKeychainPasswordWith(ctx, service, account, func(ctx context.Context, args ...string) ([]byte, error) {
+		// #nosec G204 -- fixed absolute executable and argv; no shell evaluates service or account.
+		return exec.CommandContext(ctx, "/usr/bin/security", args...).Output()
+	})
+}
+
+func readKeychainPasswordWith(ctx context.Context, service, account string, run func(context.Context, ...string) ([]byte, error)) ([]byte, error) {
+	output, err := run(ctx, "find-generic-password", "-s", service, "-a", account, "-w")
+	if err != nil {
+		var status interface{ ExitCode() int }
+		if errors.As(err, &status) && status.ExitCode() == 44 {
+			return nil, errKeyringItemNotFound
+		}
+		if ctx.Err() != nil {
+			return nil, fmt.Errorf("macOS Keychain read timed out or was canceled: %w", ctx.Err())
+		}
+		return nil, fmt.Errorf("macOS Keychain read failed: %w", err)
+	}
+	// security -w prints one terminal newline after the credential.
+	if len(output) > 0 && output[len(output)-1] == '\n' {
+		output = output[:len(output)-1]
+	}
+	return output, nil
+}

credstore/osbackend_keychain_stable_read_test.go

@@ -0,0 +1,74 @@
+//go:build cgo || windows
+
+package credstore
+
+import (
+	"context"
+	"errors"
+	"reflect"
+	"testing"
+)
+
+type keychainExitCode int
+
+func (e keychainExitCode) Error() string { return "security failed" }
+func (e keychainExitCode) ExitCode() int { return int(e) }
+
+func TestStableKeychainReadUsesServiceAndAccount(t *testing.T) {
+	var args []string
+	want := []byte("secret\nwith trailing newline\n")
+	got, err := readKeychainPasswordWith(context.Background(), "example-cli", "default/bundle", func(_ context.Context, command ...string) ([]byte, error) {
+		args = command
+		return append(append([]byte(nil), want...), '\n'), nil
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !reflect.DeepEqual(args, []string{"find-generic-password", "-s", "example-cli", "-a", "default/bundle", "-w"}) {
+		t.Fatalf("security args = %q", args)
+	}
+	if !reflect.DeepEqual(got, want) {
+		t.Fatal("credential changed during read")
+	}
+}
+
+func TestStableKeychainReadErrorMapping(t *testing.T) {
+	for _, tc := range []struct {
+		name string
+		err  error
+		want error
+	}{
+		{"missing", keychainExitCode(44), errKeyringItemNotFound},
+		{"denied", keychainExitCode(36), keychainExitCode(36)},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := readKeychainPasswordWith(context.Background(), "svc", "acct", func(context.Context, ...string) ([]byte, error) {
+				return nil, tc.err
+			})
+			if !errors.Is(err, tc.want) {
+				t.Fatalf("read error = %v, want %v", err, tc.want)
+			}
+		})
+	}
+}
+
+func TestStableKeychainReadLeavesOtherOperationsNative(t *testing.T) {
+	kr := &captureBytenessKeyring{}
+	b := keychainStableReadBackend{
+		keyringBackend: bytenessBackend{kr: kr},
+		service:        "svc",
+		read: func(service, account string) ([]byte, error) {
+			if service != "svc" || account != "default/token" {
+				t.Fatalf("read %q/%q", service, account)
+			}
+			return []byte("secret"), nil
+		},
+	}
+	item, err := b.get("default/token")
+	if err != nil || string(item.data) != "secret" || kr.getCalls != 0 {
+		t.Fatalf("get = %q, %v; native get calls = %d", item.data, err, kr.getCalls)
+	}
+	if err := b.set(keyringItem{key: "default/token", data: []byte("new")}); err != nil || kr.setCalls != 1 {
+		t.Fatalf("native set error = %v, calls = %d", err, kr.setCalls)
+	}
+}

docs/distribution.md

@@ -84,6 +84,8 @@ literal content hash — so every `brew upgrade` produces a new hash, the DR no
 matches, and the user is re-prompted. Any CLI whose `keychain_probe` (§8) selects the
 macOS Keychain backend has this problem.
 
+`credstore` reads Keychain secrets through the fixed, system-signed `/usr/bin/security` executable while retaining native writes and management. That reduces rebuild prompts for consumers that adopt this version of the library; release signing remains required for direct Keychain users and for other native accesses by a CLI. A newly trusted helper may require one grant per existing item. Normal CLI operation must not rewrite credentials with `security -i`; the dedicated [`repair-macos-keychain-credentials.sh`](../scripts/repair-macos-keychain-credentials.sh) script uses it only when repairing an existing item's ACL.
+
 **Standard: sign every darwin binary at release time with the family's single
 self-signed code-signing cert and a constant per-tool identifier.** That makes the DR
 `identifier "org.open-cli-collective.<binary>" and certificate leaf = H"<cert hash>"`

docs/working-with-secrets.md

@@ -51,7 +51,7 @@ The standard is a *better default*, not a security boundary. It removes the most
 
 All Collective CLIs use **`github.com/byteness/keyring`** as the credential-store abstraction. (Migrated from `github.com/99designs/keyring` in #23 — ByteNess is an active fork that picks up CVE fixes and ongoing maintenance.)
 
-- macOS → Keychain (Security framework, no shelling out).
+- macOS → Keychain (native writes, metadata, enumeration, and deletion; secret reads use the fixed system `/usr/bin/security` executable so Keychain grants can survive CLI rebuilds). Normal CLI credential writes use the native backend; never pass a secret on a command line or use `security -i` for those writes. The dedicated [`repair-macos-keychain-credentials.sh`](../scripts/repair-macos-keychain-credentials.sh) script uses `security -i` only to recreate existing items with repaired ACLs.
 - Windows → Credential Manager (`wincred`).
 - Linux → Secret Service (D-Bus), then file fallback (see §1.4).
 - A shared internal package, `cli-common/credstore`, wraps the library so every CLI uses the same backend priority, error messages, and config layout. CLIs do not depend on `byteness/keyring` directly.
@@ -138,6 +138,8 @@ deterministic user-facing label and description derived from the same mapping:
 application by default (`KeychainTrustApplication` / implied self). Existing
 items are not migrated in place: if an older item has blank metadata or older
 ACL state, it keeps that state until the CLI rewrites or overwrites it.
+The stable read path retains the same service/account mapping and strips only
+the terminal newline printed by `security -w`; it does not change stored items.
 
 Because `/` is structural in this mapping, **`/` is forbidden inside any segment.** Allowed characters within `service`, `profile`, and `key` are `[A-Za-z0-9_-]`. The shared package rejects anything else at write time with a clear error. CLIs that need a richer identifier (e.g. an email address as a profile) must escape it; the shared package provides helpers.