diff --git a/credstore/filebackend_compat_test.go b/credstore/filebackend_compat_test.go index d88e7e2..01586f6 100644 --- a/credstore/filebackend_compat_test.go +++ b/credstore/filebackend_compat_test.go @@ -97,6 +97,41 @@ func TestFileBackendEncodingMatchesByteNessShape(t *testing.T) { } } +func TestFileBackendEncodingPreservesMetadataFields(t *testing.T) { + passphrase := "metadata-" + "passphrase" + token, err := encodeFileKeyringItem(keyringItem{ + key: "default/tok", + data: []byte("shape-value"), + label: "codereview default/tok", + description: "Credential for codereview default/tok", + keychainNotTrustApplication: true, + keychainNotSynchronizable: true, + }, passphrase) + if err != nil { + t.Fatalf("encodeFileKeyringItem: %v", err) + } + payload, _, err := jose.Decode(token, passphrase) + if err != nil { + t.Fatalf("jose.Decode: %v", err) + } + var got persistedKeyringItem + if err := json.Unmarshal([]byte(payload), &got); err != nil { + t.Fatalf("json.Unmarshal payload: %v", err) + } + if got.Label != "codereview default/tok" { + t.Fatalf("Label = %q, want %q", got.Label, "codereview default/tok") + } + if got.Description != "Credential for codereview default/tok" { + t.Fatalf("Description = %q, want %q", got.Description, "Credential for codereview default/tok") + } + if !got.KeychainNotTrustApplication { + t.Fatal("KeychainNotTrustApplication = false, want true") + } + if !got.KeychainNotSynchronizable { + t.Fatal("KeychainNotSynchronizable = false, want true") + } +} + func writeRawFileBackendItem(t *testing.T, base, service, encodedName, token string) { t.Helper() dir := filepath.Join(base, service, "keyring") diff --git a/credstore/filebackend_format.go b/credstore/filebackend_format.go index 9a4b905..ccea699 100644 --- a/credstore/filebackend_format.go +++ b/credstore/filebackend_format.go @@ -18,7 +18,14 @@ type persistedKeyringItem struct { } func encodeFileKeyringItem(it keyringItem, password string) (string, error) { - bytes, err := json.Marshal(persistedKeyringItem{Key: it.key, Data: it.data}) + bytes, err := json.Marshal(persistedKeyringItem{ + Key: it.key, + Data: it.data, + Label: it.label, + Description: it.description, + KeychainNotTrustApplication: it.keychainNotTrustApplication, + KeychainNotSynchronizable: it.keychainNotSynchronizable, + }) if err != nil { return "", err } diff --git a/credstore/osbackend_byteness.go b/credstore/osbackend_byteness.go index 28b4344..7f9e137 100644 --- a/credstore/osbackend_byteness.go +++ b/credstore/osbackend_byteness.go @@ -11,12 +11,13 @@ import ( func openKeyringBackend(kind Backend, cfg backendConfig) (keyringBackend, error) { kcfg := keyring.Config{ - ServiceName: cfg.serviceName, - AllowedBackends: []keyring.BackendType{keyring.BackendType(cfg.allowedBackend)}, - FileDir: cfg.fileDir, - PassDir: cfg.passDir, - PassCmd: cfg.passCmd, - PassPrefix: cfg.passPrefix, + ServiceName: cfg.serviceName, + AllowedBackends: []keyring.BackendType{keyring.BackendType(cfg.allowedBackend)}, + KeychainTrustApplication: cfg.keychainTrustApplication, + FileDir: cfg.fileDir, + PassDir: cfg.passDir, + PassCmd: cfg.passCmd, + PassPrefix: cfg.passPrefix, } if cfg.filePasswordFunc != nil { kcfg.FilePasswordFunc = keyring.PromptFunc(cfg.filePasswordFunc) @@ -40,11 +41,25 @@ func (b bytenessBackend) get(itemKey string) (keyringItem, error) { } return keyringItem{}, err } - return keyringItem{key: it.Key, data: it.Data}, nil + return keyringItem{ + key: it.Key, + data: it.Data, + label: it.Label, + description: it.Description, + keychainNotTrustApplication: it.KeychainNotTrustApplication, + keychainNotSynchronizable: it.KeychainNotSynchronizable, + }, nil } func (b bytenessBackend) set(it keyringItem) error { - return b.kr.Set(keyring.Item{Key: it.key, Data: it.data}) + return b.kr.Set(keyring.Item{ + Key: it.key, + Data: it.data, + Label: it.label, + Description: it.description, + KeychainNotTrustApplication: it.keychainNotTrustApplication, + KeychainNotSynchronizable: it.keychainNotSynchronizable, + }) } func (b bytenessBackend) remove(itemKey string) error { diff --git a/credstore/osbackend_byteness_test.go b/credstore/osbackend_byteness_test.go new file mode 100644 index 0000000..8b98d5d --- /dev/null +++ b/credstore/osbackend_byteness_test.go @@ -0,0 +1,63 @@ +//go:build cgo || windows + +package credstore + +import ( + "testing" + + "github.com/byteness/keyring" +) + +type captureBytenessKeyring struct { + setItem keyring.Item +} + +func (c *captureBytenessKeyring) Get(string) (keyring.Item, error) { + return keyring.Item{}, keyring.ErrKeyNotFound +} + +func (c *captureBytenessKeyring) GetMetadata(string) (keyring.Metadata, error) { + return keyring.Metadata{}, keyring.ErrMetadataNotSupported +} + +func (c *captureBytenessKeyring) Set(item keyring.Item) error { + c.setItem = item + return nil +} + +func (c *captureBytenessKeyring) Remove(string) error { return nil } + +func (c *captureBytenessKeyring) Keys() ([]string, error) { return nil, nil } + +func TestBytenessBackendSetPassesThroughMetadata(t *testing.T) { + kr := &captureBytenessKeyring{} + be := bytenessBackend{kr: kr} + + item := keyringItem{ + key: "default/git_token", + data: []byte("secret"), + label: "codereview default/git_token", + description: "Credential for codereview default/git_token", + keychainNotTrustApplication: true, + keychainNotSynchronizable: true, + } + if err := be.set(item); err != nil { + t.Fatalf("set: %v", err) + } + + if kr.setItem.Key != item.key || string(kr.setItem.Data) != "secret" { + t.Fatalf("stored item = (%q,%q), want (%q,%q)", kr.setItem.Key, string(kr.setItem.Data), item.key, "secret") + } + if kr.setItem.Label != item.label { + t.Fatalf("Label = %q, want %q", kr.setItem.Label, item.label) + } + if kr.setItem.Description != item.description { + t.Fatalf("Description = %q, want %q", kr.setItem.Description, item.description) + } + if !kr.setItem.KeychainNotTrustApplication { + t.Fatal("KeychainNotTrustApplication = false, want true") + } + if !kr.setItem.KeychainNotSynchronizable { + t.Fatal("KeychainNotSynchronizable = false, want true") + } +} diff --git a/credstore/osbackend_core.go b/credstore/osbackend_core.go index 40a2b36..2d8ef4a 100644 --- a/credstore/osbackend_core.go +++ b/credstore/osbackend_core.go @@ -20,8 +20,12 @@ var errKeyringItemNotFound = errors.New("credstore: keyring item not found") type promptFunc func(string) (string, error) type keyringItem struct { - key string - data []byte + key string + data []byte + label string + description string + keychainNotTrustApplication bool + keychainNotSynchronizable bool } type keyringBackend interface { @@ -32,13 +36,14 @@ type keyringBackend interface { } type backendConfig struct { - serviceName string - allowedBackend Backend - fileDir string - filePasswordFunc promptFunc - passDir string - passCmd string - passPrefix string + serviceName string + allowedBackend Backend + keychainTrustApplication bool + fileDir string + filePasswordFunc promptFunc + passDir string + passCmd string + passPrefix string } // osKeyringBackend adapts a platform keyring implementation to the @@ -47,6 +52,7 @@ type backendConfig struct { type osKeyringBackend struct { kr keyringBackend backendKind Backend + service string } // openOSBackend opens exactly the selected backend. The opener is @@ -64,7 +70,7 @@ func openOSBackend(kind Backend, service string, opts *Options, getenv func(stri if err != nil { return nil, fmt.Errorf("credstore: opening %s backend for service %q: %w", kind, service, err) } - return &osKeyringBackend{kr: kr, backendKind: kind}, nil + return &osKeyringBackend{kr: kr, backendKind: kind, service: service}, nil } // preflightOSBackend runs cheap, actionable pre-construction checks for @@ -118,7 +124,12 @@ func buildKeyringConfig(kind Backend, service string, opts *Options, getenv func // CLI gets its own subtree. cfg.passPrefix = service case BackendKeychain, BackendWinCred, BackendSecretService: - // No additional construction — these use ServiceName directly. + // All three use ServiceName directly. On macOS, trust the current + // signed application by default so newly-written items do not show + // the generic "allow access?" dialog for the same binary. + if kind == BackendKeychain { + cfg.keychainTrustApplication = true + } case BackendMemory: // BackendMemory short-circuits in store.Open before reaching // openOSBackend; this arm exists only to keep the exhaustive @@ -127,6 +138,23 @@ func buildKeyringConfig(kind Backend, service string, opts *Options, getenv func return cfg, nil } +func keyringItemForWrite(service, itemKey string, value []byte) keyringItem { + return keyringItem{ + key: itemKey, + data: value, + label: keyringItemLabel(service, itemKey), + description: keyringItemDescription(service, itemKey), + } +} + +func keyringItemLabel(service, itemKey string) string { + return service + " " + itemKey +} + +func keyringItemDescription(service, itemKey string) string { + return "Credential for " + service + " " + itemKey +} + // fileKeyringDir is the encrypted-file backend location and the test // isolation seam. XDG Base Directory: $XDG_DATA_HOME//keyring, // else $HOME/.local/share//keyring. Tests set XDG_DATA_HOME to @@ -202,7 +230,7 @@ func (b *osKeyringBackend) set(itemKey, value string, overwrite bool) error { return fmt.Errorf("credstore: %s set %q (overwrite pre-check): %w", b.backendKind, itemKey, err) } } - if err := b.kr.set(keyringItem{key: itemKey, data: []byte(value)}); err != nil { + if err := b.kr.set(keyringItemForWrite(b.service, itemKey, []byte(value))); err != nil { return fmt.Errorf("credstore: %s set %q: %w", b.backendKind, itemKey, err) } return nil diff --git a/credstore/osbackend_test.go b/credstore/osbackend_test.go index b61a5d0..f4cad39 100644 --- a/credstore/osbackend_test.go +++ b/credstore/osbackend_test.go @@ -388,6 +388,66 @@ func TestBuildKeyringConfig_PassSetsPrefixToService(t *testing.T) { t.Errorf("fileDir is empty; expected file backend to set it") } }) + + t.Run("keychain: trust current application by default", func(t *testing.T) { + cfg, err := buildKeyringConfig(BackendKeychain, "codereview", &Options{}, emptyEnv) + if err != nil { + t.Fatalf("buildKeyringConfig keychain: %v", err) + } + if !cfg.keychainTrustApplication { + t.Fatal("keychainTrustApplication = false, want true") + } + }) + + t.Run("wincred: do not set keychain trust flag", func(t *testing.T) { + cfg, err := buildKeyringConfig(BackendWinCred, "codereview", &Options{}, emptyEnv) + if err != nil { + t.Fatalf("buildKeyringConfig wincred: %v", err) + } + if cfg.keychainTrustApplication { + t.Fatal("keychainTrustApplication = true, want false") + } + }) +} + +func TestKeyringItemForWriteBuildsInspectableMetadata(t *testing.T) { + got := keyringItemForWrite("codereview", "default/git_token", []byte("secret")) + + if got.key != "default/git_token" || string(got.data) != "secret" { + t.Fatalf("item = (%q,%q), want (default/git_token,secret)", got.key, string(got.data)) + } + if got.label != "codereview default/git_token" { + t.Fatalf("label = %q, want %q", got.label, "codereview default/git_token") + } + if got.description != "Credential for codereview default/git_token" { + t.Fatalf("description = %q, want %q", got.description, "Credential for codereview default/git_token") + } + if got.keychainNotTrustApplication { + t.Fatal("keychainNotTrustApplication = true, want false") + } + if got.keychainNotSynchronizable { + t.Fatal("keychainNotSynchronizable = true, want false") + } +} + +func TestOSKeyringBackendSetStampsMetadataBeforeWrite(t *testing.T) { + f := newFakeKeyring() + b := &osKeyringBackend{kr: f, backendKind: BackendKeychain, service: "codereview"} + + if err := b.set("default/git_token", "secret", true); err != nil { + t.Fatalf("set: %v", err) + } + + got, ok := f.items["default/git_token"] + if !ok { + t.Fatal("backend write did not reach fake keyring") + } + if got.label != "codereview default/git_token" { + t.Fatalf("label = %q, want %q", got.label, "codereview default/git_token") + } + if got.description != "Credential for codereview default/git_token" { + t.Fatalf("description = %q, want %q", got.description, "Credential for codereview default/git_token") + } } // TestOpenOSBackend_PassOnWindows_FailsGracefully pins the Windows diff --git a/docs/working-with-secrets.md b/docs/working-with-secrets.md index c493f80..268a84e 100644 --- a/docs/working-with-secrets.md +++ b/docs/working-with-secrets.md @@ -131,6 +131,14 @@ Within a bundle, individual secrets are addressed by well-known keys defined per So `slack-chat-api/work-account` with secret `bot_token` becomes `ServiceName="slack-chat-api"`, `Item.Key="work-account/bot_token"`. This makes `Keyring.Keys()` enumerable per-service for `config show` and `config clear`, and gives a deterministic, inspectable layout when a user opens Keychain Access / Credential Manager / `secret-tool`. +On macOS, new or overwritten Keychain generic-password items MUST also carry a +deterministic user-facing label and description derived from the same mapping: +`Label = " /"` and `Description = "Credential for + /"`. New or overwritten items trust the calling signed +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. + 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. **Multi-key-per-ref is supported by all three platform backends.** A `Keyring` opened against one `ServiceName` holds any number of `Item`s with distinct `Item.Key` values. On macOS this is one Keychain service with multiple account names; on Windows, one target-name prefix with multiple targets; on Linux Secret Service, one collection with multiple items distinguished by attributes. So `slack-chat-api/work-account` legitimately holds both `work-account/bot_token` and `work-account/user_token` as separate, independently-readable entries.