feat(secrets): add KeychainProvider#403
Merged
Merged
Conversation
Four test functions covering Set+Get, GetMissing, Delete, and List, all using keyring.MockInit() for hermetic in-process testing. Tests fail with "undefined: secrets.NewKeychainProvider" until the implementation is added. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implements secrets.Provider using the OS credential store via github.com/zalando/go-keyring (macOS Keychain, Linux Secret Service, Windows Credential Manager). Design notes: - ErrNotFound already existed in secrets/secrets.go; reused as-is. - trackedKeys is an in-process map for List() support because go-keyring has no native list-by-service operation. Keys set in prior processes will not appear in List() — documented as a known limitation. - Delete() is idempotent: keyring.ErrNotFound is silently swallowed. - Verified against four unit tests using keyring.MockInit() for hermeticity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- plugins/secrets/plugin.go: add "secrets.keychain" to ModuleTypes and ModuleFactories, dispatching to the new SecretsKeychainModule - module/secrets_keychain.go: new modular module wrapping KeychainProvider; follows the same pattern as SecretsAWSModule (Init/Start/Stop/Provider/Get) - cmd/wfctl/infra_secrets.go: add case "keychain" to resolveSecretsProvider; 'service' config key is required, returns an error if absent - cmd/wfctl/type_registry.go: register "secrets.keychain" in KnownModuleTypes so TestKnownModuleTypesCoverAllPlugins stays in sync - cmd/wfctl/infra_secrets_test.go: factory dispatch tests (happy path + missing-service error) and compile-time interface assertion - README.md: add secrets.keychain to the module-type count/list in the table (Other: 6 -> 7) Linux caveat: secrets.keychain requires a running Secret Service (libsecret/gnome-keyring/KWallet); documented in KeychainProvider godoc. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an OS keychain–backed secrets provider/module to the workflow engine, enabling local single-user deployments to store and fetch secrets via the native credential store (macOS Keychain, Linux Secret Service, Windows Credential Manager).
Changes:
- Introduces
secrets.KeychainProviderbacked bygithub.com/zalando/go-keyring, with unit tests usingkeyring.MockInit(). - Adds
secrets.keychainas a first-class module type in the secrets plugin and wfctl module type registry. - Updates CLI secrets-provider resolution and docs to include the new keychain option.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| secrets/keychain_provider.go | New OS keychain provider implementation. |
| secrets/keychain_provider_test.go | Unit tests for keychain provider behavior via go-keyring mock. |
| module/secrets_keychain.go | New modular service wrapper for the keychain provider (secrets.keychain). |
| plugins/secrets/plugin.go | Registers secrets.keychain module type in the secrets plugin factories/manifest. |
| plugins/secrets/plugin_test.go | Updates plugin tests to expect/register the new module type. |
| cmd/wfctl/type_registry.go | Adds secrets.keychain to wfctl’s known module types list. |
| cmd/wfctl/infra_secrets.go | Adds keychain option to wfctl infra bootstrap secrets provider resolution. |
| cmd/wfctl/infra_secrets_test.go | Tests wfctl’s keychain provider resolution + interface satisfaction check. |
| go.mod | Adds go-keyring and platform dependencies. |
| go.sum | Adds checksums for new dependencies. |
| README.md | Updates module type counts/listing to include secrets.keychain. |
| DOCUMENTATION.md | Documents secrets.keychain module type and Linux requirements. |
| } | ||
| if len(keys) != 2 { | ||
| t.Errorf("got %d keys, want 2", len(keys)) | ||
| } |
There was a problem hiding this comment.
List() returns keys in non-deterministic map iteration order, so checking only len(keys) can miss cases where the wrong keys are returned. Consider asserting the returned slice contains both "a" and "b" (order-independent) to make the test meaningful.
Suggested change
| } | |
| } | |
| gotKeys := make(map[string]struct{}, len(keys)) | |
| for _, key := range keys { | |
| gotKeys[key] = struct{}{} | |
| } | |
| if _, ok := gotKeys["a"]; !ok { | |
| t.Errorf("List() missing key %q; got %v", "a", keys) | |
| } | |
| if _, ok := gotKeys["b"]; !ok { | |
| t.Errorf("List() missing key %q; got %v", "b", keys) | |
| } |
Comment on lines
+20
to
+26
| type KeychainProvider struct { | ||
| service string | ||
| // trackedKeys is maintained in-process for List() support, because the | ||
| // go-keyring API doesn't provide a native list-by-service operation. | ||
| // On cold start, List() returns only keys set during this process. | ||
| trackedKeys map[string]struct{} | ||
| } |
There was a problem hiding this comment.
KeychainProvider stores trackedKeys in a plain map without synchronization. Since workflows can execute steps concurrently, concurrent Set/Delete/List calls can race or panic with "concurrent map writes". Add a sync.RWMutex (or sync.Map) and lock around all trackedKeys reads/writes.
Comment on lines
+38
to
+43
| func (p *KeychainProvider) Get(ctx context.Context, key string) (string, error) { | ||
| if err := ctx.Err(); err != nil { | ||
| return "", err | ||
| } | ||
| v, err := keyring.Get(p.service, key) | ||
| if err != nil { |
There was a problem hiding this comment.
Unlike the other secrets.Provider implementations, KeychainProvider doesn't validate that key is non-empty. For consistency and to avoid creating unnamed keychain entries, return secrets.ErrInvalidKey when key == "" in Get/Set/Delete (and consider validating service, too).
Comment on lines
+29
to
+35
| func TestKeychainProvider_GetMissing(t *testing.T) { | ||
| keyring.MockInit() | ||
| p := secrets.NewKeychainProvider("test-service") | ||
| _, err := p.Get(context.Background(), "absent") | ||
| if err == nil { | ||
| t.Fatal("expected error for missing key, got nil") | ||
| } |
There was a problem hiding this comment.
This test only checks err != nil for a missing key. Since KeychainProvider is expected to map keyring.ErrNotFound to secrets.ErrNotFound, please assert errors.Is(err, secrets.ErrNotFound) to lock in the contract and prevent regressions.
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
… assertion placement - Add sync.RWMutex to KeychainProvider; guard trackedKeys in Set/Delete (Lock) and List (RLock) — provider is now safe for concurrent use - Include key name in Get() ErrNotFound message: key %q: %w, matching vault/env convention - Move compile-time interface assertion var _ secrets.Provider = (*secrets.KeychainProvider)(nil) from cmd/wfctl/infra_secrets_test.go to secrets/keychain_provider_test.go - Add godoc comments (name-first) to all exported methods: Name, Get, Set, Delete, List - TestKeychainProvider_GetMissing: add errors.Is(err, secrets.ErrNotFound) check - TestKeychainProvider_List: sort slice and compare with reflect.DeepEqual instead of len-only check Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines
+79
to
+82
| if err := keyring.Delete(p.service, key); err != nil { | ||
| if errors.Is(err, keyring.ErrNotFound) { | ||
| return nil // idempotent | ||
| } |
There was a problem hiding this comment.
Delete returns nil on keyring.ErrNotFound, but it exits before removing the key from trackedKeys. That can leave List() reporting keys that were deleted (or never existed). Consider (1) returning secrets.ErrInvalidKey when key == "" and (2) deleting from trackedKeys even when the backend reports ErrNotFound to keep in-process state consistent with idempotent delete semantics.
Suggested change
| if err := keyring.Delete(p.service, key); err != nil { | |
| if errors.Is(err, keyring.ErrNotFound) { | |
| return nil // idempotent | |
| } | |
| if key == "" { | |
| return ErrInvalidKey | |
| } | |
| if err := keyring.Delete(p.service, key); err != nil && !errors.Is(err, keyring.ErrNotFound) { |
Comment on lines
+46
to
+56
| func (p *KeychainProvider) Get(ctx context.Context, key string) (string, error) { | ||
| if err := ctx.Err(); err != nil { | ||
| return "", err | ||
| } | ||
| v, err := keyring.Get(p.service, key) | ||
| if err != nil { | ||
| if errors.Is(err, keyring.ErrNotFound) { | ||
| return "", fmt.Errorf("secrets.keychain: key %q: %w", key, ErrNotFound) | ||
| } | ||
| return "", fmt.Errorf("secrets.keychain get %q: %w", key, err) | ||
| } |
There was a problem hiding this comment.
KeychainProvider methods don’t validate an empty key. Other secrets.Provider implementations return secrets.ErrInvalidKey when key == "" (e.g., EnvProvider/FileProvider/AWS/Vault). Add the same guard here so callers get a consistent error contract (and avoid passing empty keys down into go-keyring).
| func (p *KeychainProvider) Set(ctx context.Context, key, value string) error { | ||
| if err := ctx.Err(); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Set should reject an empty key with secrets.ErrInvalidKey (consistent with other providers). Without this, an empty key may be stored in the OS keychain or produce backend-specific errors that don’t match the secrets.Provider contract.
Suggested change
| } | |
| } | |
| if key == "" { | |
| return ErrInvalidKey | |
| } |
…anup, go mod tidy Address Copilot review feedback on PR #403: - Add ErrInvalidKey guard on Get/Set/Delete for empty keys, matching VaultProvider/GitHubProvider/AWSProvider convention. - Delete: clean up trackedKeys even on idempotent keyring.ErrNotFound path, so List() stays consistent with actual keychain state. - go mod tidy: move go-keyring from indirect to direct (it's imported by non-test code). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The example/ sub-module's go.sum was missing transitive dependencies introduced by go-keyring (danieljoos/wincred, godbus/dbus). CI's "go mod tidy" check diffs against committed state, so these need to be checked in. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines
+34
to
+40
| // NewKeychainProvider returns a provider namespaced to the given service name. | ||
| func NewKeychainProvider(service string) *KeychainProvider { | ||
| return &KeychainProvider{ | ||
| service: service, | ||
| trackedKeys: make(map[string]struct{}), | ||
| } | ||
| } |
There was a problem hiding this comment.
NewKeychainProvider allows an empty service value, but the provider’s doc comment says the service namespaces keys to avoid collisions. With service == "", secrets will be stored in a shared/empty namespace and can collide across apps. Consider validating service (e.g., return ErrProviderInit from Get/Set/Delete/List when service is empty, or add a validated constructor) so misconfiguration fails fast with a clear error.
…setup errors in tests Address Copilot review feedback on PR #403: - NewKeychainProvider now returns (*KeychainProvider, error) and rejects empty service names, preventing secrets stored in a shared namespace that can collide across applications. - Test setup calls (Set/Delete) now assert err == nil instead of using _ = discard, so mock changes or underlying failures surface correctly. - Add TestKeychainProvider_EmptyService for the new validation. - Update all callers: infra_secrets.go and secrets_keychain.go. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
KeychainProvider, a newsecrets.Providerimplementation backed by the OS credential store (macOS Keychain, Linux Secret Service, Windows Credential Manager) viagithub.com/zalando/go-keyring.secrets.keychainas a first-class module type in the plugin system, factory, and type registry.SecretsKeychainModulefollows the same modular pattern asSecretsAWSModuleandSecretsVaultModule.Motivation
Single-user local services (e.g., MCP servers) benefit from storing OAuth tokens and app credentials in the OS credential store rather than plain files or environment variables. This is the minimal upstream addition required by
zoom-mcp; other workflow-based single-user services benefit the same way.Surface
Use with
step.secret_fetch:Notes
KeychainProvidergodoc. Headless servers should useFileProviderorVaultProvider.List()limitation: returns only keys set during the current process becausego-keyringhas no native list-by-service operation. Noted in code comment. Keys persist across restarts (they live in the OS keychain); only in-processtrackedKeysis used forList().Delete()is idempotent:keyring.ErrNotFoundis silently swallowed.ErrNotFoundreused from existingsecrets/secrets.go— no new error types introduced.Test plan
keyring.MockInit(): SetAndGet, GetMissing, Delete, ListserviceerrorTestKeychainModuleFactory,TestModuleFactories(count: 2→3),TestPluginLoads(count: 2→3)DOCUMENTATION.mdcoverage test passes (TestDocumentationCoverage)go test ./...passes (only pre-existingmcp.registryfailure incmd/wfctlunrelated to this PR)🤖 Generated with Claude Code