From eb0e0b7e50977301f2a3cb1ea7b08e11ec5b8a9b Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 16 Jun 2026 00:36:23 -0400 Subject: [PATCH 1/5] feat(credstore): support 1password backends --- credstore/flag_test.go | 12 +++++ credstore/osbackend_1password_build_test.go | 26 +++++++++++ credstore/osbackend_1password_disabled.go | 14 ++++++ credstore/osbackend_1password_enabled.go | 7 +++ credstore/osbackend_byteness.go | 24 ++++++++-- credstore/osbackend_byteness_test.go | 37 +++++++++++++++ credstore/osbackend_core.go | 27 ++++++++++- credstore/osbackend_test.go | 52 +++++++++++++++++++++ credstore/select.go | 3 ++ credstore/select_test.go | 19 ++++++++ credstore/store.go | 29 ++++++++++++ 11 files changed, 244 insertions(+), 6 deletions(-) create mode 100644 credstore/osbackend_1password_build_test.go create mode 100644 credstore/osbackend_1password_disabled.go create mode 100644 credstore/osbackend_1password_enabled.go diff --git a/credstore/flag_test.go b/credstore/flag_test.go index 48168d2..1cb5b65 100644 --- a/credstore/flag_test.go +++ b/credstore/flag_test.go @@ -50,6 +50,9 @@ func TestAllBackends_MatchesConstants(t *testing.T) { BackendSecretService, BackendFile, BackendPass, + BackendOP, + BackendOPConnect, + BackendOPDesktop, BackendMemory, } if len(allBackends) != len(expected) { @@ -222,3 +225,12 @@ func TestBackendFlagUsage_IncludesPass(t *testing.T) { t.Errorf("BackendFlagUsage() should mention pass; got %q", usage) } } + +func TestBackendFlagUsage_IncludesOnePasswordBackends(t *testing.T) { + usage := BackendFlagUsage() + for _, want := range []string{"op", "op-connect", "op-desktop"} { + if !strings.Contains(usage, want) { + t.Errorf("BackendFlagUsage() should mention %q; got %q", want, usage) + } + } +} diff --git a/credstore/osbackend_1password_build_test.go b/credstore/osbackend_1password_build_test.go new file mode 100644 index 0000000..c2c20ed --- /dev/null +++ b/credstore/osbackend_1password_build_test.go @@ -0,0 +1,26 @@ +package credstore + +import ( + "errors" + "testing" +) + +func TestUnsupportedOnePasswordBackendInBuild_NonOnePasswordBackendsPass(t *testing.T) { + for _, kind := range []Backend{BackendKeychain, BackendWinCred, BackendSecretService, BackendFile, BackendPass, BackendMemory} { + if err := unsupportedOnePasswordBackendInBuild(kind); err != nil { + t.Fatalf("unsupportedOnePasswordBackendInBuild(%q) = %v, want nil", kind, err) + } + } +} + +func TestUnsupportedOnePasswordBackendInBuild_OnePasswordBackendsAreDeterministic(t *testing.T) { + for _, kind := range []Backend{BackendOP, BackendOPConnect, BackendOPDesktop} { + err := unsupportedOnePasswordBackendInBuild(kind) + if err == nil { + continue + } + if !errors.Is(err, ErrBackendNotImplemented) { + t.Fatalf("unsupportedOnePasswordBackendInBuild(%q) = %v, want ErrBackendNotImplemented", kind, err) + } + } +} diff --git a/credstore/osbackend_1password_disabled.go b/credstore/osbackend_1password_disabled.go new file mode 100644 index 0000000..bf672c2 --- /dev/null +++ b/credstore/osbackend_1password_disabled.go @@ -0,0 +1,14 @@ +//go:build keyring_no1password + +package credstore + +import "fmt" + +func unsupportedOnePasswordBackendInBuild(kind Backend) error { + switch kind { + case BackendOP, BackendOPConnect, BackendOPDesktop: + return fmt.Errorf("%w: backend %q was compiled out of this build via keyring_no1password", ErrBackendNotImplemented, kind) + default: + return nil + } +} diff --git a/credstore/osbackend_1password_enabled.go b/credstore/osbackend_1password_enabled.go new file mode 100644 index 0000000..83d6864 --- /dev/null +++ b/credstore/osbackend_1password_enabled.go @@ -0,0 +1,7 @@ +//go:build !keyring_no1password + +package credstore + +func unsupportedOnePasswordBackendInBuild(Backend) error { + return nil +} diff --git a/credstore/osbackend_byteness.go b/credstore/osbackend_byteness.go index 7f9e137..fa32fbf 100644 --- a/credstore/osbackend_byteness.go +++ b/credstore/osbackend_byteness.go @@ -10,6 +10,15 @@ import ( ) func openKeyringBackend(kind Backend, cfg backendConfig) (keyringBackend, error) { + kcfg := keyringConfigFromBackendConfig(cfg) + kr, err := keyring.Open(kcfg) + if err != nil { + return nil, fmt.Errorf("keyring open %s: %w", kind, err) + } + return bytenessBackend{kr: kr}, nil +} + +func keyringConfigFromBackendConfig(cfg backendConfig) keyring.Config { kcfg := keyring.Config{ ServiceName: cfg.serviceName, AllowedBackends: []keyring.BackendType{keyring.BackendType(cfg.allowedBackend)}, @@ -18,15 +27,20 @@ func openKeyringBackend(kind Backend, cfg backendConfig) (keyringBackend, error) PassDir: cfg.passDir, PassCmd: cfg.passCmd, PassPrefix: cfg.passPrefix, + OPTimeout: cfg.opTimeout, + OPVaultID: cfg.opVaultID, + OPItemTitlePrefix: cfg.opItemTitlePrefix, + OPItemTag: cfg.opItemTag, + OPItemFieldTitle: cfg.opItemFieldTitle, + OPConnectHost: cfg.opConnectHost, + OPConnectTokenEnv: cfg.opConnectTokenEnv, + OPTokenEnv: cfg.opTokenEnv, + OPDesktopAccountID: cfg.opDesktopAccountID, } if cfg.filePasswordFunc != nil { kcfg.FilePasswordFunc = keyring.PromptFunc(cfg.filePasswordFunc) } - kr, err := keyring.Open(kcfg) - if err != nil { - return nil, fmt.Errorf("keyring open %s: %w", kind, err) - } - return bytenessBackend{kr: kr}, nil + return kcfg } type bytenessBackend struct { diff --git a/credstore/osbackend_byteness_test.go b/credstore/osbackend_byteness_test.go index 8b98d5d..5b36fbc 100644 --- a/credstore/osbackend_byteness_test.go +++ b/credstore/osbackend_byteness_test.go @@ -4,6 +4,7 @@ package credstore import ( "testing" + "time" "github.com/byteness/keyring" ) @@ -61,3 +62,39 @@ func TestBytenessBackendSetPassesThroughMetadata(t *testing.T) { t.Fatal("KeychainNotSynchronizable = false, want true") } } + +func TestKeyringConfigFromBackendConfigForwardsOnePasswordOptions(t *testing.T) { + // #nosec G101 -- test fixture values are non-secret placeholders + cfg := keyringConfigFromBackendConfig(backendConfig{ + serviceName: "codereview", + allowedBackend: BackendOPDesktop, + opTimeout: 5 * time.Second, + opVaultID: "vault-123", + opItemTitlePrefix: "cr", + opItemTag: "codereview", + opItemFieldTitle: "credential", + opConnectHost: "https://connect.example", + opConnectTokenEnv: "CUSTOM_OP_CONNECT_TOKEN", + opTokenEnv: "CUSTOM_OP_TOKEN", + opDesktopAccountID: "desktop-account", + }) + + if cfg.ServiceName != "codereview" { + t.Fatalf("ServiceName = %q, want %q", cfg.ServiceName, "codereview") + } + if len(cfg.AllowedBackends) != 1 || cfg.AllowedBackends[0] != keyring.OPDesktopBackend { + t.Fatalf("AllowedBackends = %v, want [%v]", cfg.AllowedBackends, keyring.OPDesktopBackend) + } + if cfg.OPTimeout != 5*time.Second || cfg.OPVaultID != "vault-123" { + t.Fatalf("unexpected timeout/vault forwarding: %+v", cfg) + } + if cfg.OPItemTitlePrefix != "cr" || cfg.OPItemTag != "codereview" || cfg.OPItemFieldTitle != "credential" { + t.Fatalf("unexpected item metadata forwarding: %+v", cfg) + } + if cfg.OPConnectHost != "https://connect.example" || cfg.OPConnectTokenEnv != "CUSTOM_OP_CONNECT_TOKEN" { + t.Fatalf("unexpected connect forwarding: %+v", cfg) + } + if cfg.OPTokenEnv != "CUSTOM_OP_TOKEN" || cfg.OPDesktopAccountID != "desktop-account" { + t.Fatalf("unexpected token/account forwarding: %+v", cfg) + } +} diff --git a/credstore/osbackend_core.go b/credstore/osbackend_core.go index 2d8ef4a..8e5fb51 100644 --- a/credstore/osbackend_core.go +++ b/credstore/osbackend_core.go @@ -13,6 +13,7 @@ import ( "os/exec" "path/filepath" "runtime" + "time" ) var errKeyringItemNotFound = errors.New("credstore: keyring item not found") @@ -44,6 +45,15 @@ type backendConfig struct { passDir string passCmd string passPrefix string + opTimeout time.Duration + opVaultID string + opItemTitlePrefix string + opItemTag string + opItemFieldTitle string + opConnectHost string + opConnectTokenEnv string + opTokenEnv string + opDesktopAccountID string } // osKeyringBackend adapts a platform keyring implementation to the @@ -59,6 +69,9 @@ type osKeyringBackend struct { // build-tagged so static Unix builds do not import ByteNess keyring and // therefore do not compile its unused 1Password backends. func openOSBackend(kind Backend, service string, opts *Options, getenv func(string) string) (backend, error) { + if err := unsupportedOnePasswordBackendInBuild(kind); err != nil { + return nil, fmt.Errorf("credstore: opening %s backend for service %q: %w", kind, service, err) + } if err := preflightOSBackend(kind); err != nil { return nil, fmt.Errorf("credstore: opening %s backend for service %q: %w", kind, service, err) } @@ -88,7 +101,7 @@ func preflightOSBackend(kind Backend) error { if _, err := exec.LookPath("pass"); err != nil { return fmt.Errorf("the pass(1) CLI is not on $PATH; install it (e.g. `apt install pass` / `brew install pass`) and run `pass init ` before selecting --backend pass") } - case BackendKeychain, BackendWinCred, BackendSecretService, BackendFile, BackendMemory: + case BackendKeychain, BackendWinCred, BackendSecretService, BackendFile, BackendOP, BackendOPConnect, BackendOPDesktop, BackendMemory: // No preflight check — the openers (or store.Open's memory // short-circuit) already return actionable errors. } @@ -130,6 +143,18 @@ func buildKeyringConfig(kind Backend, service string, opts *Options, getenv func if kind == BackendKeychain { cfg.keychainTrustApplication = true } + case BackendOP, BackendOPConnect, BackendOPDesktop: + if opts != nil && opts.OnePassword != nil { + cfg.opTimeout = opts.OnePassword.Timeout + cfg.opVaultID = opts.OnePassword.VaultID + cfg.opItemTitlePrefix = opts.OnePassword.ItemTitlePrefix + cfg.opItemTag = opts.OnePassword.ItemTag + cfg.opItemFieldTitle = opts.OnePassword.ItemFieldTitle + cfg.opConnectHost = opts.OnePassword.ConnectHost + cfg.opConnectTokenEnv = opts.OnePassword.ConnectTokenEnv + cfg.opTokenEnv = opts.OnePassword.ServiceTokenEnv + cfg.opDesktopAccountID = opts.OnePassword.DesktopAccountID + } case BackendMemory: // BackendMemory short-circuits in store.Open before reaching // openOSBackend; this arm exists only to keep the exhaustive diff --git a/credstore/osbackend_test.go b/credstore/osbackend_test.go index f4cad39..b854b95 100644 --- a/credstore/osbackend_test.go +++ b/credstore/osbackend_test.go @@ -389,6 +389,58 @@ func TestBuildKeyringConfig_PassSetsPrefixToService(t *testing.T) { } }) + t.Run("1password service account: forwards non-secret options", func(t *testing.T) { + cfg, err := buildKeyringConfig(BackendOP, "codereview", &Options{ + OnePassword: &OnePasswordOptions{ + Timeout: 5, + VaultID: "vault-123", + ItemTitlePrefix: "cr", + ItemTag: "codereview", + ItemFieldTitle: "credential", + ServiceTokenEnv: "CUSTOM_OP_TOKEN", + DesktopAccountID: "desktop-account", + }, + }, emptyEnv) + if err != nil { + t.Fatalf("buildKeyringConfig op: %v", err) + } + if cfg.opTimeout != 5 { + t.Fatalf("opTimeout = %v, want 5", cfg.opTimeout) + } + if cfg.opVaultID != "vault-123" { + t.Fatalf("opVaultID = %q, want %q", cfg.opVaultID, "vault-123") + } + if cfg.opItemTitlePrefix != "cr" || cfg.opItemTag != "codereview" || cfg.opItemFieldTitle != "credential" { + t.Fatalf("unexpected item metadata forwarding: %+v", cfg) + } + if cfg.opTokenEnv != "CUSTOM_OP_TOKEN" { + t.Fatalf("opTokenEnv = %q, want %q", cfg.opTokenEnv, "CUSTOM_OP_TOKEN") + } + if cfg.opDesktopAccountID != "desktop-account" { + t.Fatalf("opDesktopAccountID = %q, want %q", cfg.opDesktopAccountID, "desktop-account") + } + }) + + t.Run("1password connect: forwards host and token env", func(t *testing.T) { + // #nosec G101 -- test fixture values are non-secret placeholders + cfg, err := buildKeyringConfig(BackendOPConnect, "codereview", &Options{ + OnePassword: &OnePasswordOptions{ + VaultID: "vault-123", + ConnectHost: "https://connect.example", + ConnectTokenEnv: "CUSTOM_OP_CONNECT_TOKEN", + }, + }, emptyEnv) + if err != nil { + t.Fatalf("buildKeyringConfig op-connect: %v", err) + } + if cfg.opConnectHost != "https://connect.example" { + t.Fatalf("opConnectHost = %q, want %q", cfg.opConnectHost, "https://connect.example") + } + if cfg.opConnectTokenEnv != "CUSTOM_OP_CONNECT_TOKEN" { + t.Fatalf("opConnectTokenEnv = %q, want %q", cfg.opConnectTokenEnv, "CUSTOM_OP_CONNECT_TOKEN") + } + }) + t.Run("keychain: trust current application by default", func(t *testing.T) { cfg, err := buildKeyringConfig(BackendKeychain, "codereview", &Options{}, emptyEnv) if err != nil { diff --git a/credstore/select.go b/credstore/select.go index ccf85ec..602e2e3 100644 --- a/credstore/select.go +++ b/credstore/select.go @@ -74,6 +74,9 @@ var allBackends = []Backend{ BackendSecretService, BackendFile, BackendPass, + BackendOP, + BackendOPConnect, + BackendOPDesktop, BackendMemory, } diff --git a/credstore/select_test.go b/credstore/select_test.go index 5616a56..a66d708 100644 --- a/credstore/select_test.go +++ b/credstore/select_test.go @@ -118,6 +118,25 @@ func TestParseBackend_RecognizesPass(t *testing.T) { } } +func TestParseBackend_RecognizesOnePasswordBackends(t *testing.T) { + tests := map[string]Backend{ + "op": BackendOP, + "op-connect": BackendOPConnect, + "op-desktop": BackendOPDesktop, + } + for input, want := range tests { + t.Run(input, func(t *testing.T) { + got, ok := parseBackend(input) + if !ok { + t.Fatalf("parseBackend(%q) = (_, false); want true", input) + } + if got != want { + t.Fatalf("parseBackend(%q) = %q, want %q", input, got, want) + } + }) + } +} + // TestSelectBackendPassNeverAuto mirrors the BackendMemory contract: // pass is selectable explicitly / via env / via config, but never via // auto. A future selectBackend refactor that started auto-picking pass diff --git a/credstore/store.go b/credstore/store.go index d618b17..8dc5773 100644 --- a/credstore/store.go +++ b/credstore/store.go @@ -26,6 +26,7 @@ import ( "sort" "strings" "sync" + "time" ) // Backend identifies which credential backend a Store is using. @@ -37,9 +38,18 @@ const ( BackendSecretService Backend = "secret-service" // Linux — later unit BackendFile Backend = "file" // encrypted file — later unit BackendPass Backend = "pass" // pass(1) CLI shell-out; !windows; requires `pass` binary + initialized password-store + BackendOP Backend = "op" // 1Password service account backend + BackendOPConnect Backend = "op-connect" // 1Password Connect backend + BackendOPDesktop Backend = "op-desktop" // 1Password desktop integration backend BackendMemory Backend = "memory" // tests/CI, no disk ) +const ( + DefaultOnePasswordServiceTokenEnv = "OP_SERVICE_ACCOUNT_TOKEN" // #nosec G101 -- environment variable name, not a secret + DefaultOnePasswordConnectTokenEnv = "OP_CONNECT_TOKEN" // #nosec G101 -- environment variable name, not a secret + DefaultOnePasswordDesktopAccountEnv = "OP_DESKTOP_ACCOUNT_ID" // #nosec G101 -- environment variable name, not a secret +) + // Source describes how the backend was selected. type Source string @@ -73,6 +83,25 @@ type Options struct { // for BackendFile. Nil is fine for headless/CI/tests that set the // env var instead. FilePassphrase func() (string, error) + // OnePassword carries non-secret backend-specific wiring for the + // 1Password backends. Secrets themselves still come from the backend's + // runtime environment or native integration, never from config. + OnePassword *OnePasswordOptions +} + +// OnePasswordOptions configures the opt-in 1Password backend families. +// All fields are non-secret. Blank Env names fall back to the upstream +// ByteNess defaults during backend construction. +type OnePasswordOptions struct { + Timeout time.Duration + VaultID string + ItemTitlePrefix string + ItemTag string + ItemFieldTitle string + ConnectHost string + ConnectTokenEnv string + ServiceTokenEnv string + DesktopAccountID string } // Sentinels and typed errors. All are matchable via errors.Is. From 421ac605b793b25f27835c570560d0f34d8ef267 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Wed, 17 Jun 2026 17:04:23 -0400 Subject: [PATCH 2/5] fix(credstore): harden 1password backend defaults Closes #29 --- .github/workflows/ci.yml | 13 +++++++++---- credstore/osbackend_core.go | 17 ++++++++++++++--- credstore/osbackend_test.go | 30 +++++++++++++++++++++++++++++- credstore/select_test.go | 34 ++++++++++++++++++++++++++++++++++ credstore/store.go | 14 ++++++++++---- docs/development.md | 9 +++++---- docs/working-with-secrets.md | 25 +++++++++++++------------ 7 files changed, 114 insertions(+), 28 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 264d8d0..82559f3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,10 +40,15 @@ jobs: - name: Test run: go test -v -race -coverprofile=coverage.out ./... # Standard consumer build configuration (working-with-secrets.md - # §1.10): credstore must stay green with the keyring opt-out tags - # the CLIs ship with. keyring_nofile / keyring_nopass are NOT in - # the set — credstore exposes those backends in cgo builds. - - name: Build and test with keyring opt-out tags + # §1.10): 1Password support is enabled, Passage is disabled. + - name: Build and test with standard keyring tags + run: | + go build -tags keyring_nopassage ./... + go test -race -tags keyring_nopassage ./... + # Optional compliance/minimal-dependency build configuration: + # 1Password support is compiled out but exposed file/pass backends + # remain enabled. + - name: Build and test with no-1Password opt-out tags run: | go build -tags keyring_no1password,keyring_nopassage ./... go test -race -tags keyring_no1password,keyring_nopassage ./... diff --git a/credstore/osbackend_core.go b/credstore/osbackend_core.go index 8e5fb51..7eae1ed 100644 --- a/credstore/osbackend_core.go +++ b/credstore/osbackend_core.go @@ -144,11 +144,22 @@ func buildKeyringConfig(kind Backend, service string, opts *Options, getenv func cfg.keychainTrustApplication = true } case BackendOP, BackendOPConnect, BackendOPDesktop: + cfg.opItemTitlePrefix = service + cfg.opItemTag = service + if kind == BackendOP || kind == BackendOPDesktop { + cfg.opTimeout = DefaultOnePasswordTimeout + } if opts != nil && opts.OnePassword != nil { - cfg.opTimeout = opts.OnePassword.Timeout + if opts.OnePassword.Timeout != 0 { + cfg.opTimeout = opts.OnePassword.Timeout + } cfg.opVaultID = opts.OnePassword.VaultID - cfg.opItemTitlePrefix = opts.OnePassword.ItemTitlePrefix - cfg.opItemTag = opts.OnePassword.ItemTag + if opts.OnePassword.ItemTitlePrefix != "" { + cfg.opItemTitlePrefix = opts.OnePassword.ItemTitlePrefix + } + if opts.OnePassword.ItemTag != "" { + cfg.opItemTag = opts.OnePassword.ItemTag + } cfg.opItemFieldTitle = opts.OnePassword.ItemFieldTitle cfg.opConnectHost = opts.OnePassword.ConnectHost cfg.opConnectTokenEnv = opts.OnePassword.ConnectTokenEnv diff --git a/credstore/osbackend_test.go b/credstore/osbackend_test.go index b854b95..e8951d7 100644 --- a/credstore/osbackend_test.go +++ b/credstore/osbackend_test.go @@ -6,6 +6,7 @@ import ( "runtime" "strings" "testing" + "time" ) // TestFileBackendRoundTrip exercises the real encrypted-file backend @@ -441,6 +442,33 @@ func TestBuildKeyringConfig_PassSetsPrefixToService(t *testing.T) { } }) + t.Run("1password defaults: service-scoped item metadata and required timeouts", func(t *testing.T) { + for _, tc := range []struct { + kind Backend + wantTimeout time.Duration + }{ + {BackendOP, DefaultOnePasswordTimeout}, + {BackendOPConnect, 0}, + {BackendOPDesktop, DefaultOnePasswordTimeout}, + } { + t.Run(string(tc.kind), func(t *testing.T) { + cfg, err := buildKeyringConfig(tc.kind, "codereview", &Options{}, emptyEnv) + if err != nil { + t.Fatalf("buildKeyringConfig %s: %v", tc.kind, err) + } + if cfg.opItemTitlePrefix != "codereview" { + t.Fatalf("opItemTitlePrefix = %q, want service-scoped prefix %q", cfg.opItemTitlePrefix, "codereview") + } + if cfg.opItemTag != "codereview" { + t.Fatalf("opItemTag = %q, want service-scoped tag %q", cfg.opItemTag, "codereview") + } + if cfg.opTimeout != tc.wantTimeout { + t.Fatalf("opTimeout = %v, want %v", cfg.opTimeout, tc.wantTimeout) + } + }) + } + }) + t.Run("keychain: trust current application by default", func(t *testing.T) { cfg, err := buildKeyringConfig(BackendKeychain, "codereview", &Options{}, emptyEnv) if err != nil { @@ -557,7 +585,7 @@ func TestPreflightOSBackend_PassNotOnPath(t *testing.T) { func TestPreflightOSBackend_NonPassBackendsSkipPreflight(t *testing.T) { t.Setenv("PATH", "") for _, kind := range []Backend{ - BackendKeychain, BackendWinCred, BackendSecretService, BackendFile, BackendMemory, + BackendKeychain, BackendWinCred, BackendSecretService, BackendFile, BackendOP, BackendOPConnect, BackendOPDesktop, BackendMemory, } { if err := preflightOSBackend(kind); err != nil { t.Errorf("preflightOSBackend(%q) = %v, want nil — non-pass backends must not preflight", kind, err) diff --git a/credstore/select_test.go b/credstore/select_test.go index a66d708..1f4bbc3 100644 --- a/credstore/select_test.go +++ b/credstore/select_test.go @@ -137,6 +137,40 @@ func TestParseBackend_RecognizesOnePasswordBackends(t *testing.T) { } } +// TestSelectBackendOnePasswordNeverAuto mirrors the pass/memory contract: +// 1Password backends are selectable explicitly / via env / via config, but +// never by auto. Selecting one has external availability and account/vault +// coupling, so auto-picking it would be a stealth runtime behavior change. +func TestSelectBackendOnePasswordNeverAuto(t *testing.T) { + const svc = "svc" + for _, kind := range []Backend{BackendOP, BackendOPConnect, BackendOPDesktop} { + t.Run(string(kind), func(t *testing.T) { + if b, src, err := selectBackend(svc, &Options{Backend: kind}, envFrom(nil), "linux"); err != nil || b != kind || src != SourceExplicit { + t.Fatalf("explicit %s = (%q,%q,%v), want (%q,explicit,nil)", kind, b, src, err, kind) + } + if b, src, err := selectBackend(svc, &Options{}, envFrom(map[string]string{"SVC_KEYRING_BACKEND": string(kind)}), "linux"); err != nil || b != kind || src != SourceEnv { + t.Fatalf("env %s = (%q,%q,%v), want (%q,env,nil)", kind, b, src, err, kind) + } + if b, src, err := selectBackend(svc, &Options{ConfigBackend: kind}, envFrom(nil), "linux"); err != nil || b != kind || src != SourceConfig { + t.Fatalf("config %s = (%q,%q,%v), want (%q,config,nil)", kind, b, src, err, kind) + } + }) + } + + for _, goos := range []string{"darwin", "windows", "linux"} { + b, src, err := selectBackend(svc, &Options{}, envFrom(nil), goos) + if err != nil { + continue + } + if b == BackendOP || b == BackendOPConnect || b == BackendOPDesktop { + t.Fatalf("auto on %s selected %s; 1Password backends must be opt-in only", goos, b) + } + if src != SourceAuto { + t.Fatalf("auto %s src = %q, want auto", goos, src) + } + } +} + // TestSelectBackendPassNeverAuto mirrors the BackendMemory contract: // pass is selectable explicitly / via env / via config, but never via // auto. A future selectBackend refactor that started auto-picking pass diff --git a/credstore/store.go b/credstore/store.go index 8dc5773..fd0384f 100644 --- a/credstore/store.go +++ b/credstore/store.go @@ -6,8 +6,9 @@ package credstore // // -> ServiceName + / mapping), // and §1.5/§1.5.2 (overwrite semantics, allowed-key enforcement). // -// Backends: the in-memory backend (memory.go) and the real OS keyrings -// (osbackend.go: keychain/wincred/secret-service/file) selected per +// Backends: the in-memory backend (memory.go) and the real credential +// backends (osbackend*.go: keychain/wincred/secret-service/file/pass/op) +// selected per // §1.4 (select.go). Selection is fail-closed — an unrecognized backend // is an error and memory is never auto-selected, never a silent // degradation. @@ -48,6 +49,7 @@ const ( DefaultOnePasswordServiceTokenEnv = "OP_SERVICE_ACCOUNT_TOKEN" // #nosec G101 -- environment variable name, not a secret DefaultOnePasswordConnectTokenEnv = "OP_CONNECT_TOKEN" // #nosec G101 -- environment variable name, not a secret DefaultOnePasswordDesktopAccountEnv = "OP_DESKTOP_ACCOUNT_ID" // #nosec G101 -- environment variable name, not a secret + DefaultOnePasswordTimeout = 30 * time.Second ) // Source describes how the backend was selected. @@ -90,8 +92,12 @@ type Options struct { } // OnePasswordOptions configures the opt-in 1Password backend families. -// All fields are non-secret. Blank Env names fall back to the upstream -// ByteNess defaults during backend construction. +// All fields are non-secret. Blank token env names and backend-specific +// connection selectors fall back to upstream ByteNess defaults during +// backend construction. Blank Timeout uses DefaultOnePasswordTimeout for +// backends that require one. Blank ItemTitlePrefix and ItemTag are +// scoped to the credstore service name so separate CLI consumers do not +// collide in the same 1Password vault. type OnePasswordOptions struct { Timeout time.Duration VaultID string diff --git a/docs/development.md b/docs/development.md index 7e12afa..0df11bc 100644 --- a/docs/development.md +++ b/docs/development.md @@ -47,7 +47,8 @@ ported consumer is green against the candidate SHA. ## Keyring opt-out tags `byteness/keyring` (≥ v1.11.0) supports per-backend opt-out build tags; -consumer CLIs build with `-tags keyring_no1password,keyring_nopassage` as -standard configuration, and CI here tests credstore under the same set — -see [`working-with-secrets.md`](working-with-secrets.md) §1.10 for the -contract and why `keyring_nofile` / `keyring_nopass` are excluded. +consumer CLIs that expose 1Password build with `-tags keyring_nopassage` as +standard configuration. CI here tests that set plus the optional +`keyring_no1password,keyring_nopassage` opt-out path — see +[`working-with-secrets.md`](working-with-secrets.md) §1.10 for the contract and +why `keyring_nofile` / `keyring_nopass` are excluded. diff --git a/docs/working-with-secrets.md b/docs/working-with-secrets.md index 268a84e..6ff65ac 100644 --- a/docs/working-with-secrets.md +++ b/docs/working-with-secrets.md @@ -15,8 +15,8 @@ The "Deriving work from this standard" bridge between the two parts spells out h ## Goals -- **Runtime secrets live in the OS credential store.** Once a CLI is operating — reading config and making API calls — the only place it looks for a secret is the platform keyring. Never plaintext on disk, never a config-file field, never an env var the CLI reads as a primary source. -- **Deployment-time secrets can live anywhere.** Getting a secret *into* the keyring is an install-time concern. The source can be a 1Password vault read by `op`, an MDM-pushed file, an installer prompt, an interactive `init`, a Vault lookup, an env var the installer reads — anything. The CLI does not care; once `init` (or an equivalent setup step) finishes, the secret is in the keyring and the upstream source is irrelevant. +- **Runtime secrets live in a credential store.** Once a CLI is operating — reading config and making API calls — the only place it looks for a secret is the selected credstore backend: platform keyring by default, or an explicitly selected backend from §1.4. Never plaintext on disk, never a config-file field, never an env var the CLI reads as a primary API credential source. +- **Deployment-time secrets can live anywhere.** Getting a secret *into* the credential store is an install-time concern. The source can be a 1Password vault read by `op`, an MDM-pushed file, an installer prompt, an interactive `init`, a Vault lookup, an env var the installer reads — anything. The default path is still installer-time translation into the platform keyring; explicit runtime backends are opt-in and documented in §1.4 / §1.10. - **Config files are commitable to private / org-controlled stores.** Everything in a CLI's config file is safe for an org to version in a private repo, ship via MDM, or template across machines. This is *not* a guarantee that the file is safe to publish on the public internet — deployment material (§1.2) is org-internal even though it isn't an access secret. - **Multi-tenant ready.** A user can hold credentials for more than one tenant of the same service (two Jira orgs, two Slack workspaces) without collision. - **Org deployment friendly.** An organization can pre-stage keychain entries via MDM / 1Password / its installer of choice, ship a config file pointing at them, and have the CLI Just Work without re-prompting the user for secrets. @@ -24,7 +24,7 @@ The "Deriving work from this standard" bridge between the two parts spells out h ## Non-goals -- Native runtime integration with 1Password / Vault / SSM. Those are *installer-time* secret sources — the installer reads from them and writes to the OS keyring once. The CLI itself only knows about the OS keyring. +- Native runtime integration with Vault / SSM. Those remain *installer-time* secret sources unless a backend is explicitly added to `credstore`. 1Password is the current explicit exception: `credstore` exposes opt-in `op`, `op-connect`, and `op-desktop` backends. - Encrypting the config file. Config files don't contain secrets. ## Threat model @@ -167,6 +167,7 @@ The shared credstore package selects backends in this fixed order: - **Ambiguous failure → fail closed.** If the wrapper cannot confidently distinguish "unavailable" from "denied/locked" — D-Bus answered but returned an opaque error, the backend timed out, the response shape was unexpected — treat the case as denied/locked and fail closed. The fallback path is opt-in only when we are sure no working keyring is present. - **Explicit user opt-in to the file backend** is supported via a config flag (working name `keyring.backend: file` in `config.yml`) or an env var (`_KEYRING_BACKEND=file`). With that set, the CLI uses the file backend unconditionally and never attempts Secret Service. This is the supported path for users who genuinely prefer the file backend. The backend-selector env var is non-secret runtime configuration (it controls *where* the CLI looks, not *what* it finds); it is not the runtime-env-var exception described below. - **Explicit user opt-in to the `pass` backend** is supported via the same knobs (`--backend pass`, `_KEYRING_BACKEND=pass`, or `keyring.backend: pass`). `pass` is the canonical headless-Linux answer for users who already run a `pass` password-store; it requires the `pass` CLI binary on `$PATH` and an initialized password-store (`pass init `). credstore scopes each consumer's entries under a per-service subtree of the password-store (by setting `PassPrefix = `), so two CLIs storing the same key name do not collide. KeePassXC users do not need a separate backend — KeePassXC integrates with Secret Service over D-Bus, so `--backend secret-service` (the default) already covers it. The `pass` backend is `!windows`: requesting it on Windows fails closed with a clear "not supported on Windows" error rather than silently falling back. +- **Explicit user opt-in to a 1Password backend** is supported via the same knobs (`--backend op`, `--backend op-connect`, `--backend op-desktop`, `_KEYRING_BACKEND=...`, or `keyring.backend: ...`). These are never auto-selected. `op` is the 1Password service-account backend, not a shell-out to the `op` CLI; `op-connect` uses 1Password Connect; `op-desktop` uses Desktop app integration. Tokens and account material never live in config: config may hold only non-secret selectors such as vault ID, Connect host, token env-var names, item prefix/tag, and timeout. credstore scopes 1Password item title prefixes and tags to the service by default so two CLIs using the same vault do not collide. The file backend is encrypted with a passphrase. For org-friendly headless use (CI, WSL with no daemon), the passphrase can be supplied via a **per-service** env var named `_KEYRING_PASSPHRASE`, where `SERVICE` is the upper-snake-cased service segment of the `credential_ref`: @@ -224,7 +225,7 @@ This is distinct from *runtime token refresh*. A CLI making normal API calls may `config show` (and `config show --json`) reports: - The contents of the config file (which by definition contain no access secrets — §1.2). - For each access secret expected by the CLI: whether it is present, never the value. -- Which backend the keyring is using on this machine (`keychain`, `wincred`, `secret-service`, or `file`). +- Which backend the keyring is using on this machine (for example `keychain`, `wincred`, `secret-service`, `file`, `pass`, `op`, `op-connect`, or `op-desktop`). - The full `credential_ref` so users can locate the entry in Keychain Access / Credential Manager / `secret-tool`. - **Deployment material is reported by path, presence, and content fingerprint — not inlined.** OAuth client JSONs and similar deployment-material files can be sizable and are org-internal; dumping them into `config show` clutters output and risks copy-paste leakage into chats/tickets. The reported fingerprint is a stable hash prefix (e.g. SHA-256 truncated to 12 hex chars) so operators can verify the file matches what the installer shipped without reading the contents. `config show --verbose` may inline small deployment-material files; the default does not. @@ -304,11 +305,11 @@ An organization deploying a Collective CLI to its users can: This is the path Monit's `claude-desktop-mcp` installer should take. The CLI's interactive `init` becomes the fallback for users without org deployment. -**Keyring entries are user-scoped.** Every backend this standard supports (macOS Keychain login keychain, Windows Credential Manager per-user vault, Linux Secret Service per-user collection, file backend in `~/.local/share/...`) stores entries against the *operating-system user account*, not against the machine. MDM-driven and installer-driven prestaging must therefore run **in the target user's context** — either as that user (sudo -u, `runas`), or by having the installer invoke the CLI (` set-credential` / ` init`) as a step that the target user runs themselves on first login. An installer running as `root` or `SYSTEM` that writes to its own keyring has accomplished nothing useful for the end user. +**Credential entries are principal-scoped.** Platform/file/pass backends store entries against the *operating-system user account*, not against the machine. MDM-driven and installer-driven prestaging for those backends must therefore run **in the target user's context** — either as that user (sudo -u, `runas`), or by having the installer invoke the CLI (` set-credential` / ` init`) as a step that the target user runs themselves on first login. An installer running as `root` or `SYSTEM` that writes to its own keyring has accomplished nothing useful for the end user. 1Password backends are scoped instead by the selected account/vault/Connect server and the credential material or Desktop integration available to the running process. ## §1.10 Note on 1Password / external secret managers -1Password (and Vault, AWS Secrets Manager, etc.) are *installer-time* sources. **`set-credential` is the preferred automation primitive** for translating from an external secret manager into the OS keyring; it's purpose-built for this case (single-secret, stdin/env ingress, no API smoke test, scriptable exit codes — §1.5.2): +For the default installer-time path, 1Password, Vault, AWS Secrets Manager, and similar systems are sources that feed the selected credential store. **`set-credential` is the preferred automation primitive** for translating from an external secret manager into that store; it's purpose-built for this case (single-secret, stdin/env ingress, no API smoke test, scriptable exit codes — §1.5.2): ``` op read 'op://Vault/Item/field' | set-credential --ref --key --stdin @@ -322,17 +323,17 @@ op read 'op://Vault/Item/token' | init --url --email --token In automation, prefer `set-credential` per-secret over `init` for everything: it has a smaller surface, no verification round-trip to fail on, and one secret per invocation maps cleanly onto one `op read`. -**Default path: the CLI does not invoke 1Password at runtime.** Adding an `op` resolver to the CLI's runtime would entangle every CLI with `op`'s availability, version compatibility, and account configuration. The canonical boundary is installer-time: installers translate from external secret managers into the OS keyring, and the CLI reads from the OS keyring. `set-credential` is the preferred automation primitive for that translation. +**Default path: the CLI does not invoke 1Password at runtime.** The canonical boundary for most deployments remains installer-time: installers translate from external secret managers into the platform keyring, and the CLI reads from that keyring. `set-credential` is the preferred automation primitive for that translation. -A note on what credstore exposes: as of #24, `credstore` recognizes six backend names — `keychain`, `wincred`, `secret-service`, `file`, `pass`, `memory`. `pass` is the only external secret manager exposed natively; it shells out to the `pass` CLI binary and has no Go SDK dependencies. KeePassXC users get native runtime resolution today through Secret Service (no separate backend needed). 1Password native backends are deliberately not exposed: ByteNess's `op` / `op-connect` / `op-desktop` openers all depend on the upstream `github.com/1password/onepassword-sdk-go` package, which is still pre-1.0 — exposing them here would put a beta SDK on the credential-access critical path. The "default path" above remains the recommendation for most users; `pass` is an opt-in alternative for users who specifically want runtime resolution and accept the per-backend availability/version coupling. +A note on what credstore exposes: `credstore` recognizes nine backend names — `keychain`, `wincred`, `secret-service`, `file`, `pass`, `op`, `op-connect`, `op-desktop`, `memory`. `pass` shells out to the `pass` CLI binary. KeePassXC users get native runtime resolution through Secret Service (no separate backend needed). The three 1Password backends are explicit opt-in alternatives for users who specifically want runtime resolution and accept per-backend availability/account/version coupling. They use ByteNess's native 1Password integrations and do not require the `op` CLI. Because the upstream `github.com/1password/onepassword-sdk-go` dependency is still pre-1.0, consumers that expose 1Password must treat it as part of their release-risk review rather than an invisible dependency. -**Standard build configuration: keyring opt-out tags.** Not exposing the 1Password backends does not by itself remove their code: `byteness/keyring` `init()`-registers its openers, so without intervention the 1Password SDKs — and transitively a WASM runtime (`wazero` via `extism`) and the archived `jaeger-client-go` — compile into every credstore consumer (measured 2026-06-11 against keyring v1.9.3 on `slck`: 63 packages, ~10.6 MB of attributable symbols). The remediation landed upstream in keyring v1.11.0 (ByteNess/keyring#93/#94, driven from cli-common#57): per-backend opt-out build tags. Every consumer CLI MUST build (Makefile, CI, and `.goreleaser` `flags:`) with: +**Standard build configuration: keyring opt-out tags.** Consumer CLIs that expose 1Password build with 1Password support included and Passage disabled: ``` --tags keyring_no1password,keyring_nopassage +-tags keyring_nopassage ``` -These two are exactly the backends credstore does not expose. `keyring_nofile` and `keyring_nopass` MUST NOT be used: credstore's cgo builds delegate the `file` and `pass` backends to `byteness/keyring`, so those tags would break exposed functionality. cli-common's own CI builds and tests credstore under the standard tag set to keep the configuration green from the library side. +`keyring_no1password` is an allowed opt-out for consumers that intentionally do not expose 1Password, such as compliance-scoped or minimal-dependency builds; requesting `op`, `op-connect`, or `op-desktop` in such a binary must fail closed with `ErrBackendNotImplemented`. `keyring_nofile` and `keyring_nopass` MUST NOT be used: credstore's cgo builds delegate the `file` and `pass` backends to `byteness/keyring`, so those tags would break exposed functionality. cli-common's own CI builds and tests both the standard tag set and the optional `keyring_no1password,keyring_nopassage` opt-out set. ## §1.11 Compliance criteria @@ -418,7 +419,7 @@ The package centers on a service-scoped `Store` returned by `Open`. Backend sele - **Open / lifecycle.** - `credstore.Open(service string, opts *Options) (*Store, error)` — opens a service-scoped store, resolves backend selection at this point (including reading `_KEYRING_BACKEND` and `_KEYRING_PASSPHRASE`), applies the §1.4 Linux classification rules. - `(*Store).Close() error` — releases backend resources; safe to call multiple times. - - `(*Store).Backend() (backend, source Source)` — `backend` is one of `keychain` / `wincred` / `secret-service` / `file`; `source` describes how it was selected (`auto`, `env`, `config`). No error return — the store is already open and the backend is known. Required for `config show` (§1.6). + - `(*Store).Backend() (backend, source Source)` — `backend` is one of the recognized `credstore.Backend` values; `source` describes how it was selected (`auto`, `env`, `config`, or explicit caller option). No error return — the store is already open and the backend is known. Required for `config show` (§1.6). - **Allowed-key validation is CLI-provided.** The shared package validates ref / profile / key *syntax* (the `[A-Za-z0-9_-]` character set, no `/` inside segments — §1.3) but cannot know which key names a given CLI considers valid (`bot_token` for slck, `api_token` for atlassian-cli, etc.). Each CLI passes its own allowed-key allowlist when opening the store: `Options.AllowedKeys []string`. The store then enforces it: `Set` / `SetBundle` / `Delete` reject any key not in the allowlist with a clear error listing the allowed set (per §1.5.2). CLIs that omit `AllowedKeys` get syntax-only validation, useful for tooling like `cli-common`'s own tests; production CLI code always supplies the allowlist. - **Ref handling (package-level, no store needed).** `ParseRef(string) (service, profile, error)`; inverse `FormatRef(service, profile)`. Enforce the `[A-Za-z0-9_-]` character set per §1.3, reject `/` inside any segment, return a typed error so callers can produce actionable messages. `EscapeRefSegment(raw)` helper for CLIs that need to derive a profile from a richer identifier (e.g. a user email). - **Single-key operations on a `Store`.** `Get(profile, key) (value, error)`, `Set(profile, key, value string, opts ...SetOpt) error`, `Delete(profile, key) error`, `Exists(profile, key) (bool, error)`. `SetOpt` carries `Overwrite` (per §1.5). From c8f51692a036753d44d5ae3454c5e9ba0ee5fb4d Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Wed, 17 Jun 2026 17:09:10 -0400 Subject: [PATCH 3/5] docs(credstore): clarify 1password backend auth env vars --- credstore/osbackend_1password_disabled_test.go | 17 +++++++++++++++++ docs/development.md | 2 +- docs/working-with-secrets.md | 12 ++++++------ 3 files changed, 24 insertions(+), 7 deletions(-) create mode 100644 credstore/osbackend_1password_disabled_test.go diff --git a/credstore/osbackend_1password_disabled_test.go b/credstore/osbackend_1password_disabled_test.go new file mode 100644 index 0000000..d5308c6 --- /dev/null +++ b/credstore/osbackend_1password_disabled_test.go @@ -0,0 +1,17 @@ +//go:build keyring_no1password + +package credstore + +import ( + "errors" + "testing" +) + +func TestUnsupportedOnePasswordBackendInBuild_DisabledBuildRejectsOnePasswordBackends(t *testing.T) { + for _, kind := range []Backend{BackendOP, BackendOPConnect, BackendOPDesktop} { + err := unsupportedOnePasswordBackendInBuild(kind) + if !errors.Is(err, ErrBackendNotImplemented) { + t.Fatalf("unsupportedOnePasswordBackendInBuild(%q) = %v, want ErrBackendNotImplemented", kind, err) + } + } +} diff --git a/docs/development.md b/docs/development.md index 0df11bc..cc14111 100644 --- a/docs/development.md +++ b/docs/development.md @@ -15,7 +15,7 @@ profile** in [`repo-layout.md`](repo-layout.md) §2.1. | Package | Implements | |---|---| -| `credstore` | OS-keyring credential store — [`working-with-secrets.md`](working-with-secrets.md) §1.3–§1.5, §1.8, §1.12, §2.1 | +| `credstore` | credential store abstraction — [`working-with-secrets.md`](working-with-secrets.md) §1.3–§1.5, §1.8, §1.12, §2.1 | | `statedir` | config/cache/data path resolver — [`working-with-state.md`](working-with-state.md) §6a | | `statedirtest` | hermetic test helper (8-var env override) — [`working-with-state.md`](working-with-state.md) §3.1 / §5.3 | | `cache` | tier-1 cache core: envelope, atomic write, freshness — [`working-with-state.md`](working-with-state.md) §6b | diff --git a/docs/working-with-secrets.md b/docs/working-with-secrets.md index 6ff65ac..5dff3c3 100644 --- a/docs/working-with-secrets.md +++ b/docs/working-with-secrets.md @@ -88,7 +88,7 @@ The canonical example is a desktop-app OAuth client JSON (Google Cloud, Microsof - A service-account private key (Google, AWS IAM, etc.) used by an org-wide automation → access secret. Possession alone authenticates. - A SaaS API token rotated quarterly and distributed to all engineers → access secret. -These all live in the keyring under a `credential_ref`, *even when* the installer pre-stages them via `op`. "Shared across the org" is not a license to put a token in a plain file. +These all live in the selected credential store under a `credential_ref`, *even when* the installer pre-stages them via `op`. "Shared across the org" is not a license to put a token in a plain file. Deployment material lives in the config directory as plain files (or inline in the config yaml when small). Recommended layout for `gro`: `~/.config/google-readonly/oauth_client.json` with platform-appropriate file permissions for non-secret org-internal data (on POSIX, 0644 is fine; tighter is also fine; the file is not a credential), and an `oauth_client_path` field in `config.yml` (default: that path). Installers ship this file as part of the deployment; no `op` round-trip, no keyring write, no 1Password secret-notes encoding pitfalls. @@ -97,7 +97,7 @@ Deployment material is not safe to *publish* (don't put it in a public repo), bu **Cache directory (`$XDG_CACHE_HOME/` or platform equivalent):** - Anything the CLI computed from API responses and can recompute. Never authoritative; safe to delete at any time. Out of scope for this standard; called out so it doesn't drift into the config file. -**OS credential store (access secrets):** +**Credential store (access secrets):** - Per-user API tokens - Shared org-wide tokens that grant access on their own (per the negative examples above) - OAuth refresh/access tokens (the per-user ones — not the org-wide client JSON, which is deployment material) @@ -178,7 +178,7 @@ The file backend is encrypted with a passphrase. For org-friendly headless use ( Per-service (not a single global passphrase) so that a leak of one env var compromises only one CLI's secrets, and so that org tooling injecting credentials for one CLI doesn't need access to every CLI's keyring. Document the trade-off in `config show`: when the file backend is in use, the entry indicates whether it's passphrase-prompted or env-var-supplied, so the user understands the security posture of their setup. -**Env-var exception, named explicitly.** This standard otherwise bans env vars as a runtime source of *secret material or unlocking material* (a CLI must not read `FOO_TOKEN` and use it as the API credential at call time — §1.11 acceptance item 2). `_KEYRING_PASSPHRASE` is the **one allowed runtime-env-var exception for secret material / unlocking material**, and even it is not a service token: it unlocks the encrypted-file backend so the CLI can then fetch the actual service token from it. The distinction matters because the standard's threat model treats env vars as a leaky channel (process lists, parent-process inheritance, transcripts); the passphrase being there is a deliberate trade-off scoped to the headless-Linux use case, not a general escape hatch. Non-secret runtime env vars (the backend selector above; future runtime knobs that don't carry secret material) are not exceptions because they don't fall under the ban in the first place. +**Env-var exceptions, named explicitly.** This standard otherwise bans env vars as a runtime source of *API credential material* (a CLI must not read `FOO_TOKEN` and use it as the upstream service credential at call time — §1.11 acceptance item 2). Backend-auth material may come from env vars only for explicitly selected backends that document that contract: `_KEYRING_PASSPHRASE` unlocks the encrypted-file backend, and the 1Password backends may read `OP_SERVICE_ACCOUNT_TOKEN` / `OP_CONNECT_TOKEN` (or caller-configured token-env names) to authenticate to the selected 1Password store. These values unlock or authenticate the credential backend so the CLI can fetch the actual service token; they are not the upstream API credential. The distinction matters because the standard's threat model treats env vars as a leaky channel (process lists, parent-process inheritance, transcripts); these exceptions are deliberate backend-specific trade-offs, not a general escape hatch. Non-secret runtime env vars (the backend selector above; future runtime knobs that don't carry secret material) are not exceptions because they don't fall under the ban in the first place. ## §1.5 Credential ingress @@ -339,8 +339,8 @@ A note on what credstore exposes: `credstore` recognizes nine backend names — A CLI is compliant with this standard when all of the following are true at runtime, observable from the user's perspective: -1. **`init` writes no access secret to the config file or to any plaintext file under the CLI's config dir.** Access secrets land in the keyring only. Deployment material (§1.2) may be written to plain files; it is not an access secret. -2. **Normal API commands resolve access secrets from the keyring, not from environment variables or config files.** Env vars and flags carry new secret material into the CLI only via `init` / `set-credential` (§1.5). A normal API command may write a *refreshed* token back to the active ref (§1.5 intro) — that is not ingress of new material from the user/environment, it's persistence of material the upstream service just issued. The one runtime-env-var exception for secret/unlocking material is the file-backend passphrase (§1.4), which is not itself a service token. +1. **`init` writes no access secret to the config file or to any plaintext file under the CLI's config dir.** Access secrets land in the selected credential store only. Deployment material (§1.2) may be written to plain files; it is not an access secret. +2. **Normal API commands resolve access secrets from the selected credential store, not from environment variables or config files.** Env vars and flags carry new upstream API credential material into the CLI only via `init` / `set-credential` (§1.5). Backend-auth env vars are limited to the explicit exceptions in §1.4 and must not be treated as API credentials. A normal API command may write a *refreshed* token back to the active ref (§1.5 intro) — that is not ingress of new material from the user/environment, it's persistence of material the upstream service just issued. 3. **`config show` (and `--json`) reports presence, backend, and ref for every secret — never values.** A user can confirm setup without seeing any secret material. 4. **`config clear` (default scope) removes only the keys under the active `credential_ref`.** Other profiles, other CLIs' keyring entries, and the config file are untouched. 5. **`config clear --all` returns the active profile's config + credentials + cache to a pre-install state** — config file, keyring entries under the active ref, caches, and empty parent directories. Inactive profiles are not touched; the data pillar (`working-with-state.md` §5) is not touched (§1.7.2). @@ -570,7 +570,7 @@ The `granted_scopes` field is preserved from current behavior (used to detect wh - **OAuth client JSON** (deployment material per §1.2). Path: `~/.config/google-readonly/oauth_client.json` with platform-appropriate file permissions for non-secret org-internal data (on POSIX, 0644 is fine; tighter is also fine; the file is not a credential). `oauth_client_path` field in `config.yml` lets an org override the location (default: that path). Installers ship this file as part of the deployment; no keyring round-trip, no `op` integration for this file, no 1Password secret-notes mangling. - **Auto-migrate legacy `credentials.json`** (deployment material): copy to `oauth_client.json` (or wherever `oauth_client_path` points), update config, leave a one-line notice. Source location: `~/.config/google-readonly/credentials.json`. - **Auto-migrate legacy `token.json`** (access secret — this is the bigger lift). `token.json` was the file-fallback location for the per-user OAuth token when the keychain backend wasn't available. Migrate its contents into the keyring under `google-readonly/default/oauth_token`, then **remove the plaintext file**. Conflict rules from §1.8 apply: if a token already exists in the keyring and `token.json` differs, fail loudly, don't pick a winner. -- **OAuth user token** post-migration: keyring only, under the ref, key `oauth_token`. This is what `config clear` rotates. +- **OAuth user token** post-migration: credential store only, under the ref, key `oauth_token`. This is what `config clear` rotates. - Add Windows Credential Manager support for the user token (today `gro` on Windows likely falls through to a plaintext file token — verify and fix as part of the `token.json` migration). - `config show`: backend, ref, presence of `oauth_token`. For the OAuth client JSON, report **path + presence + content fingerprint** per §1.6; do not inline the JSON contents by default. `--verbose` may inline. - **Move the cache out of the config dir.** Today `gro`'s Drive metadata cache lives under the config directory (alongside `config.json` / `credentials.json` / `token.json`), which §1.2 classifies as the wrong place. Move it to the platform cache location: `$XDG_CACHE_HOME/google-readonly` on Linux (default `~/.cache/google-readonly`), `~/Library/Caches/google-readonly` on macOS, `%LOCALAPPDATA%\google-readonly\Cache` on Windows. Auto-migrate by recreating the cache at the new location on first run; the old location can be deleted as part of the same migration since cache contents are recomputable. `config clear --all` (§1.7.2) cleans the new cache location; for one deprecation cycle it also cleans any leftover cache files under the old location. From bc08bb74fa9ac35684ab5781df00ca8e451bf690 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Wed, 17 Jun 2026 17:14:43 -0400 Subject: [PATCH 4/5] test(credstore): pin 1password build modes --- credstore/flag_test.go | 27 ++++++++++++++++--- credstore/osbackend_1password_build_test.go | 17 +----------- .../osbackend_1password_disabled_test.go | 11 ++++++++ credstore/osbackend_1password_enabled_test.go | 25 +++++++++++++++++ 4 files changed, 60 insertions(+), 20 deletions(-) create mode 100644 credstore/osbackend_1password_enabled_test.go diff --git a/credstore/flag_test.go b/credstore/flag_test.go index 1cb5b65..1513637 100644 --- a/credstore/flag_test.go +++ b/credstore/flag_test.go @@ -94,8 +94,9 @@ func TestValidBackendNames_MatchesAllBackends(t *testing.T) { func TestBackendFlagUsage_NamesEveryBackend(t *testing.T) { usage := BackendFlagUsage() + names := backendFlagUsageNames(t, usage) for _, b := range allBackends { - if !strings.Contains(usage, string(b)) { + if !names[string(b)] { t.Errorf("BackendFlagUsage() missing %q: %q", b, usage) } } @@ -107,6 +108,23 @@ func TestBackendFlagUsage_NamesEveryBackend(t *testing.T) { } } +func backendFlagUsageNames(t *testing.T, usage string) map[string]bool { + t.Helper() + _, after, ok := strings.Cut(usage, "one of: ") + if !ok { + t.Fatalf("BackendFlagUsage() missing backend list: %q", usage) + } + list, _, ok := strings.Cut(after, ". Precedence:") + if !ok { + t.Fatalf("BackendFlagUsage() missing precedence suffix: %q", usage) + } + names := map[string]bool{} + for _, name := range strings.Split(list, ", ") { + names[name] = true + } + return names +} + func TestBackendEnvVar(t *testing.T) { cases := []struct { service string @@ -221,15 +239,16 @@ func TestBindBackendFlag_NilOpts(t *testing.T) { // downstream go.mod bump. func TestBackendFlagUsage_IncludesPass(t *testing.T) { usage := BackendFlagUsage() - if !strings.Contains(usage, "pass") { + if !backendFlagUsageNames(t, usage)[string(BackendPass)] { t.Errorf("BackendFlagUsage() should mention pass; got %q", usage) } } func TestBackendFlagUsage_IncludesOnePasswordBackends(t *testing.T) { usage := BackendFlagUsage() - for _, want := range []string{"op", "op-connect", "op-desktop"} { - if !strings.Contains(usage, want) { + names := backendFlagUsageNames(t, usage) + for _, want := range []Backend{BackendOP, BackendOPConnect, BackendOPDesktop} { + if !names[string(want)] { t.Errorf("BackendFlagUsage() should mention %q; got %q", want, usage) } } diff --git a/credstore/osbackend_1password_build_test.go b/credstore/osbackend_1password_build_test.go index c2c20ed..d195661 100644 --- a/credstore/osbackend_1password_build_test.go +++ b/credstore/osbackend_1password_build_test.go @@ -1,9 +1,6 @@ package credstore -import ( - "errors" - "testing" -) +import "testing" func TestUnsupportedOnePasswordBackendInBuild_NonOnePasswordBackendsPass(t *testing.T) { for _, kind := range []Backend{BackendKeychain, BackendWinCred, BackendSecretService, BackendFile, BackendPass, BackendMemory} { @@ -12,15 +9,3 @@ func TestUnsupportedOnePasswordBackendInBuild_NonOnePasswordBackendsPass(t *test } } } - -func TestUnsupportedOnePasswordBackendInBuild_OnePasswordBackendsAreDeterministic(t *testing.T) { - for _, kind := range []Backend{BackendOP, BackendOPConnect, BackendOPDesktop} { - err := unsupportedOnePasswordBackendInBuild(kind) - if err == nil { - continue - } - if !errors.Is(err, ErrBackendNotImplemented) { - t.Fatalf("unsupportedOnePasswordBackendInBuild(%q) = %v, want ErrBackendNotImplemented", kind, err) - } - } -} diff --git a/credstore/osbackend_1password_disabled_test.go b/credstore/osbackend_1password_disabled_test.go index d5308c6..2e3a0d5 100644 --- a/credstore/osbackend_1password_disabled_test.go +++ b/credstore/osbackend_1password_disabled_test.go @@ -15,3 +15,14 @@ func TestUnsupportedOnePasswordBackendInBuild_DisabledBuildRejectsOnePasswordBac } } } + +func TestOpen_OnePasswordDisabledBuildRejectsOnePasswordBackends(t *testing.T) { + for _, kind := range []Backend{BackendOP, BackendOPConnect, BackendOPDesktop} { + t.Run(string(kind), func(t *testing.T) { + _, err := Open("credstore-optest", &Options{Backend: kind}) + if !errors.Is(err, ErrBackendNotImplemented) { + t.Fatalf("Open(%q) = %v, want ErrBackendNotImplemented", kind, err) + } + }) + } +} diff --git a/credstore/osbackend_1password_enabled_test.go b/credstore/osbackend_1password_enabled_test.go new file mode 100644 index 0000000..d7815a8 --- /dev/null +++ b/credstore/osbackend_1password_enabled_test.go @@ -0,0 +1,25 @@ +//go:build !keyring_no1password && !freebsd && (cgo || windows) + +package credstore + +import ( + "errors" + "testing" +) + +func TestOpenOSBackend_OnePasswordEnabledBuildReachesBackendValidation(t *testing.T) { + t.Setenv("OP_VAULT_ID", "") + t.Setenv("OP_CONNECT_HOST", "") + t.Setenv("OP_DESKTOP_ACCOUNT_ID", "") + for _, kind := range []Backend{BackendOP, BackendOPConnect, BackendOPDesktop} { + t.Run(string(kind), func(t *testing.T) { + _, err := openOSBackend(kind, "credstore-optest", &Options{}, func(string) string { return "" }) + if err == nil { + t.Fatal("openOSBackend unexpectedly succeeded without required 1Password configuration") + } + if errors.Is(err, ErrBackendNotImplemented) { + t.Fatalf("openOSBackend(%q) = %v, want enabled backend validation error, not ErrBackendNotImplemented", kind, err) + } + }) + } +} From 410e43f9a2316c5c98868ebc05a01c3044a27669 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Wed, 17 Jun 2026 17:20:42 -0400 Subject: [PATCH 5/5] docs(credstore): document 1password build tradeoffs --- credstore/osbackend_core.go | 2 ++ credstore/osbackend_test.go | 6 +++--- docs/development.md | 12 ++++++------ docs/working-with-secrets.md | 10 ++++++++-- 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/credstore/osbackend_core.go b/credstore/osbackend_core.go index 7eae1ed..d08827d 100644 --- a/credstore/osbackend_core.go +++ b/credstore/osbackend_core.go @@ -146,6 +146,8 @@ func buildKeyringConfig(kind Backend, service string, opts *Options, getenv func case BackendOP, BackendOPConnect, BackendOPDesktop: cfg.opItemTitlePrefix = service cfg.opItemTag = service + // ByteNess requires a non-zero OPTimeout for OP and OPDesktop. + // OPConnect does not consult OPTimeout during construction. if kind == BackendOP || kind == BackendOPDesktop { cfg.opTimeout = DefaultOnePasswordTimeout } diff --git a/credstore/osbackend_test.go b/credstore/osbackend_test.go index e8951d7..1534117 100644 --- a/credstore/osbackend_test.go +++ b/credstore/osbackend_test.go @@ -393,7 +393,7 @@ func TestBuildKeyringConfig_PassSetsPrefixToService(t *testing.T) { t.Run("1password service account: forwards non-secret options", func(t *testing.T) { cfg, err := buildKeyringConfig(BackendOP, "codereview", &Options{ OnePassword: &OnePasswordOptions{ - Timeout: 5, + Timeout: 5 * time.Second, VaultID: "vault-123", ItemTitlePrefix: "cr", ItemTag: "codereview", @@ -405,8 +405,8 @@ func TestBuildKeyringConfig_PassSetsPrefixToService(t *testing.T) { if err != nil { t.Fatalf("buildKeyringConfig op: %v", err) } - if cfg.opTimeout != 5 { - t.Fatalf("opTimeout = %v, want 5", cfg.opTimeout) + if cfg.opTimeout != 5*time.Second { + t.Fatalf("opTimeout = %v, want 5s", cfg.opTimeout) } if cfg.opVaultID != "vault-123" { t.Fatalf("opVaultID = %q, want %q", cfg.opVaultID, "vault-123") diff --git a/docs/development.md b/docs/development.md index cc14111..e9fa6a9 100644 --- a/docs/development.md +++ b/docs/development.md @@ -46,9 +46,9 @@ ported consumer is green against the candidate SHA. ## Keyring opt-out tags -`byteness/keyring` (≥ v1.11.0) supports per-backend opt-out build tags; -consumer CLIs that expose 1Password build with `-tags keyring_nopassage` as -standard configuration. CI here tests that set plus the optional -`keyring_no1password,keyring_nopassage` opt-out path — see -[`working-with-secrets.md`](working-with-secrets.md) §1.10 for the contract and -why `keyring_nofile` / `keyring_nopass` are excluded. +`byteness/keyring` (≥ v1.11.0) supports per-backend opt-out build tags. +Consumer CLIs that expose 1Password build with `-tags keyring_nopassage`. +Consumer CLIs that intentionally skip 1Password support build with +`-tags keyring_no1password,keyring_nopassage`. CI here tests both sets — see +[`working-with-secrets.md`](working-with-secrets.md) §1.10 for the dependency +trade-off and why `keyring_nofile` / `keyring_nopass` are excluded. diff --git a/docs/working-with-secrets.md b/docs/working-with-secrets.md index 5dff3c3..d2f7738 100644 --- a/docs/working-with-secrets.md +++ b/docs/working-with-secrets.md @@ -178,7 +178,7 @@ The file backend is encrypted with a passphrase. For org-friendly headless use ( Per-service (not a single global passphrase) so that a leak of one env var compromises only one CLI's secrets, and so that org tooling injecting credentials for one CLI doesn't need access to every CLI's keyring. Document the trade-off in `config show`: when the file backend is in use, the entry indicates whether it's passphrase-prompted or env-var-supplied, so the user understands the security posture of their setup. -**Env-var exceptions, named explicitly.** This standard otherwise bans env vars as a runtime source of *API credential material* (a CLI must not read `FOO_TOKEN` and use it as the upstream service credential at call time — §1.11 acceptance item 2). Backend-auth material may come from env vars only for explicitly selected backends that document that contract: `_KEYRING_PASSPHRASE` unlocks the encrypted-file backend, and the 1Password backends may read `OP_SERVICE_ACCOUNT_TOKEN` / `OP_CONNECT_TOKEN` (or caller-configured token-env names) to authenticate to the selected 1Password store. These values unlock or authenticate the credential backend so the CLI can fetch the actual service token; they are not the upstream API credential. The distinction matters because the standard's threat model treats env vars as a leaky channel (process lists, parent-process inheritance, transcripts); these exceptions are deliberate backend-specific trade-offs, not a general escape hatch. Non-secret runtime env vars (the backend selector above; future runtime knobs that don't carry secret material) are not exceptions because they don't fall under the ban in the first place. +**Env-var exceptions, named explicitly.** This standard otherwise bans env vars as a runtime source of *API credential material* (a CLI must not read `FOO_TOKEN` and use it as the upstream service credential at call time — §1.11 acceptance item 2). Backend-auth material may come from env vars only for explicitly selected backends that document that contract: `_KEYRING_PASSPHRASE` unlocks the encrypted-file backend, and the 1Password backends may read `OP_SERVICE_ACCOUNT_TOKEN` / `OP_CONNECT_TOKEN` (or caller-configured token-env names) to authenticate to the selected 1Password store. `op-desktop` may also read `OP_DESKTOP_ACCOUNT_ID` as a backend account selector. These values unlock, authenticate, or select the credential backend so the CLI can fetch the actual service token; they are not the upstream API credential. The distinction matters because the standard's threat model treats env vars as a leaky channel (process lists, parent-process inheritance, transcripts); these exceptions are deliberate backend-specific trade-offs, not a general escape hatch. Non-secret runtime env vars (the backend selector above; future runtime knobs that don't carry secret material) are not exceptions because they don't fall under the ban in the first place. ## §1.5 Credential ingress @@ -333,7 +333,13 @@ A note on what credstore exposes: `credstore` recognizes nine backend names — -tags keyring_nopassage ``` -`keyring_no1password` is an allowed opt-out for consumers that intentionally do not expose 1Password, such as compliance-scoped or minimal-dependency builds; requesting `op`, `op-connect`, or `op-desktop` in such a binary must fail closed with `ErrBackendNotImplemented`. `keyring_nofile` and `keyring_nopass` MUST NOT be used: credstore's cgo builds delegate the `file` and `pass` backends to `byteness/keyring`, so those tags would break exposed functionality. cli-common's own CI builds and tests both the standard tag set and the optional `keyring_no1password,keyring_nopassage` opt-out set. +The 1Password SDKs are not free: measured 2026-06-11 against keyring v1.9.3 on `slck`, they pulled in 63 packages and about 10.6 MB of attributable symbols, including a WASM runtime (`wazero` via `extism`) and the archived `jaeger-client-go`. Consumer CLIs that intentionally do not expose 1Password, such as compliance-scoped or minimal-dependency builds, MUST build with: + +``` +-tags keyring_no1password,keyring_nopassage +``` + +Requesting `op`, `op-connect`, or `op-desktop` in such a binary must fail closed with `ErrBackendNotImplemented`. `keyring_nofile` and `keyring_nopass` MUST NOT be used: credstore's cgo builds delegate the `file` and `pass` backends to `byteness/keyring`, so those tags would break exposed functionality. cli-common's own CI builds and tests both the standard tag set and the optional `keyring_no1password,keyring_nopassage` opt-out set. ## §1.11 Compliance criteria