From 4dea82aaa72dd64cf4acd4b55847f3c655774a0a Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Wed, 15 Apr 2026 17:56:49 -0400 Subject: [PATCH 1/7] test: add failing tests for KeychainProvider 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 --- go.mod | 3 ++ go.sum | 6 +++ secrets/keychain_provider_test.go | 64 +++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+) create mode 100644 secrets/keychain_provider_test.go diff --git a/go.mod b/go.mod index 85a8a5be..31884015 100644 --- a/go.mod +++ b/go.mod @@ -135,6 +135,7 @@ require ( github.com/containerd/errdefs/pkg v0.3.0 // indirect github.com/cucumber/gherkin/go/v26 v26.2.0 // indirect github.com/cucumber/messages/go/v21 v21.0.1 // indirect + github.com/danieljoos/wincred v1.2.3 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/deckarep/golang-set/v2 v2.8.0 // indirect github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f // indirect @@ -169,6 +170,7 @@ require ( github.com/go-openapi/swag/typeutils v0.25.5 // indirect github.com/go-openapi/swag/yamlutils v0.25.5 // indirect github.com/gobwas/glob v0.2.3 // indirect + github.com/godbus/dbus/v5 v5.2.2 // indirect github.com/gofrs/uuid v4.4.0+incompatible // indirect github.com/golobby/cast v1.3.3 // indirect github.com/google/btree v1.1.3 // indirect @@ -279,6 +281,7 @@ require ( github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect github.com/yosida95/uritemplate/v3 v3.0.2 // indirect github.com/yuin/gopher-lua v1.1.1 // indirect + github.com/zalando/go-keyring v0.2.8 // indirect github.com/zeebo/xxh3 v1.1.0 // indirect go.etcd.io/bbolt v1.4.3 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect diff --git a/go.sum b/go.sum index 68a6be01..0060fb33 100644 --- a/go.sum +++ b/go.sum @@ -232,6 +232,8 @@ github.com/cucumber/godog v0.15.1/go.mod h1:qju+SQDewOljHuq9NSM66s0xEhogx0q30flf github.com/cucumber/messages/go/v21 v21.0.1 h1:wzA0LxwjlWQYZd32VTlAVDTkW6inOFmSM+RuOwHZiMI= github.com/cucumber/messages/go/v21 v21.0.1/go.mod h1:zheH/2HS9JLVFukdrsPWoPdmUtmYQAQPLk7w5vWsk5s= github.com/cucumber/messages/go/v22 v22.0.0/go.mod h1:aZipXTKc0JnjCsXrJnuZpWhtay93k7Rn3Dee7iyPJjs= +github.com/danieljoos/wincred v1.2.3 h1:v7dZC2x32Ut3nEfRH+vhoZGvN72+dQ/snVXo/vMFLdQ= +github.com/danieljoos/wincred v1.2.3/go.mod h1:6qqX0WNrS4RzPZ1tnroDzq9kY3fu1KwE7MRLQK4X0bs= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= @@ -347,6 +349,8 @@ github.com/go-viper/mapstructure/v2 v2.5.0 h1:vM5IJoUAy3d7zRSVtIwQgBj7BiWtMPfmPE github.com/go-viper/mapstructure/v2 v2.5.0/go.mod h1:oJDH3BJKyqBA2TXFhDsKDGDTlndYOZ6rGS0BRZIxGhM= github.com/gobwas/glob v0.2.3 h1:A4xDbljILXROh+kObIiy5kIaPYD8e96x1tgBhUI5J+Y= github.com/gobwas/glob v0.2.3/go.mod h1:d3Ez4x06l9bZtSvzIay5+Yzi0fmZzPgnTbPcKjJAkT8= +github.com/godbus/dbus/v5 v5.2.2 h1:TUR3TgtSVDmjiXOgAAyaZbYmIeP3DPkld3jgKGV8mXQ= +github.com/godbus/dbus/v5 v5.2.2/go.mod h1:3AAv2+hPq5rdnr5txxxRwiGjPXamgoIHgz9FPBfOp3c= github.com/gofrs/uuid v4.2.0+incompatible/go.mod h1:b2aQJv3Z4Fp6yNu3cdSllBxTCLRxnplIgP/c0N/04lM= github.com/gofrs/uuid v4.3.1+incompatible/go.mod h1:b2aQJv3Z4Fp6yNu3cdSllBxTCLRxnplIgP/c0N/04lM= github.com/gofrs/uuid v4.4.0+incompatible h1:3qXRTX8/NbyulANqlc0lchS1gqAVxRgsuW1YrTJupqA= @@ -807,6 +811,8 @@ github.com/yuin/gopher-lua v1.1.1 h1:kYKnWBjvbNP4XLT3+bPEwAXJx262OhaHDWDVOPjL46M github.com/yuin/gopher-lua v1.1.1/go.mod h1:GBR0iDaNXjAgGg9zfCvksxSRnQx76gclCIb7kdAd1Pw= github.com/yusufpapurcu/wmi v1.2.4 h1:zFUKzehAFReQwLys1b/iSMl+JQGSCSjtVqQn9bBrPo0= github.com/yusufpapurcu/wmi v1.2.4/go.mod h1:SBZ9tNy3G9/m5Oi98Zks0QjeHVDvuK0qfxQmPyzfmi0= +github.com/zalando/go-keyring v0.2.8 h1:6sD/Ucpl7jNq10rM2pgqTs0sZ9V3qMrqfIIy5YPccHs= +github.com/zalando/go-keyring v0.2.8/go.mod h1:tsMo+VpRq5NGyKfxoBVjCuMrG47yj8cmakZDO5QGii0= github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ= github.com/zeebo/assert v1.3.0/go.mod h1:Pq9JiuJQpG8JLJdtkwrJESF0Foym2/D9XMU5ciN/wJ0= github.com/zeebo/xxh3 v1.1.0 h1:s7DLGDK45Dyfg7++yxI0khrfwq9661w9EN78eP/UZVs= diff --git a/secrets/keychain_provider_test.go b/secrets/keychain_provider_test.go new file mode 100644 index 00000000..6a6f4505 --- /dev/null +++ b/secrets/keychain_provider_test.go @@ -0,0 +1,64 @@ +package secrets_test + +import ( + "context" + "testing" + + "github.com/GoCodeAlone/workflow/secrets" + "github.com/zalando/go-keyring" +) + +func TestKeychainProvider_SetAndGet(t *testing.T) { + keyring.MockInit() + p := secrets.NewKeychainProvider("test-service") + + ctx := context.Background() + if err := p.Set(ctx, "api_key", "secret-123"); err != nil { + t.Fatalf("Set: %v", err) + } + + got, err := p.Get(ctx, "api_key") + if err != nil { + t.Fatalf("Get: %v", err) + } + if got != "secret-123" { + t.Errorf("got %q, want secret-123", got) + } +} + +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") + } +} + +func TestKeychainProvider_Delete(t *testing.T) { + keyring.MockInit() + p := secrets.NewKeychainProvider("test-service") + ctx := context.Background() + _ = p.Set(ctx, "x", "1") + if err := p.Delete(ctx, "x"); err != nil { + t.Fatalf("Delete: %v", err) + } + if _, err := p.Get(ctx, "x"); err == nil { + t.Fatal("expected error after Delete") + } +} + +func TestKeychainProvider_List(t *testing.T) { + keyring.MockInit() + p := secrets.NewKeychainProvider("test-service") + ctx := context.Background() + _ = p.Set(ctx, "a", "1") + _ = p.Set(ctx, "b", "2") + keys, err := p.List(ctx) + if err != nil { + t.Fatalf("List: %v", err) + } + if len(keys) != 2 { + t.Errorf("got %d keys, want 2", len(keys)) + } +} From 2c4eb6b19b70202f855bb29928a0dd608d407440 Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Wed, 15 Apr 2026 20:36:03 -0400 Subject: [PATCH 2/7] feat(secrets): add KeychainProvider backed by go-keyring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- secrets/keychain_provider.go | 86 ++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 secrets/keychain_provider.go diff --git a/secrets/keychain_provider.go b/secrets/keychain_provider.go new file mode 100644 index 00000000..bf56acb7 --- /dev/null +++ b/secrets/keychain_provider.go @@ -0,0 +1,86 @@ +package secrets + +import ( + "context" + "errors" + "fmt" + + "github.com/zalando/go-keyring" +) + +// KeychainProvider implements Provider using the OS credential store +// (macOS Keychain, Linux Secret Service, Windows Credential Manager). +// +// All keys are namespaced under a single "service" string so multiple +// workflow services on the same machine don't collide. +// +// On Linux, requires a running Secret Service implementation (libsecret, +// gnome-keyring, or KWallet). Headless servers without one should use +// FileProvider or VaultProvider instead. +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{} +} + +// NewKeychainProvider returns a provider namespaced to the given service name. +func NewKeychainProvider(service string) *KeychainProvider { + return &KeychainProvider{ + service: service, + trackedKeys: make(map[string]struct{}), + } +} + +func (p *KeychainProvider) Name() string { return "keychain" } + +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: %w", ErrNotFound) + } + return "", fmt.Errorf("secrets.keychain get %q: %w", key, err) + } + return v, nil +} + +func (p *KeychainProvider) Set(ctx context.Context, key, value string) error { + if err := ctx.Err(); err != nil { + return err + } + if err := keyring.Set(p.service, key, value); err != nil { + return fmt.Errorf("secrets.keychain set %q: %w", key, err) + } + p.trackedKeys[key] = struct{}{} + return nil +} + +func (p *KeychainProvider) Delete(ctx context.Context, key string) error { + if err := ctx.Err(); err != nil { + return err + } + if err := keyring.Delete(p.service, key); err != nil { + if errors.Is(err, keyring.ErrNotFound) { + return nil // idempotent + } + return fmt.Errorf("secrets.keychain delete %q: %w", key, err) + } + delete(p.trackedKeys, key) + return nil +} + +func (p *KeychainProvider) List(ctx context.Context) ([]string, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + out := make([]string, 0, len(p.trackedKeys)) + for k := range p.trackedKeys { + out = append(out, k) + } + return out, nil +} From 8bd85acb256dfec98fb56a280b158ba68ffd41df Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Wed, 15 Apr 2026 20:51:18 -0400 Subject: [PATCH 3/7] feat(secrets): register keychain provider in factory - 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 --- DOCUMENTATION.md | 1 + README.md | 2 +- cmd/wfctl/infra_secrets.go | 9 +++- cmd/wfctl/infra_secrets_test.go | 28 ++++++++++++ cmd/wfctl/type_registry.go | 6 +++ module/secrets_keychain.go | 80 +++++++++++++++++++++++++++++++++ plugins/secrets/plugin.go | 17 ++++--- plugins/secrets/plugin_test.go | 26 ++++++++--- 8 files changed, 157 insertions(+), 12 deletions(-) create mode 100644 module/secrets_keychain.go diff --git a/DOCUMENTATION.md b/DOCUMENTATION.md index 6af25186..fc96c35e 100644 --- a/DOCUMENTATION.md +++ b/DOCUMENTATION.md @@ -581,6 +581,7 @@ Strict mode applies to **both** direct dot-access (`{{ .steps.auth.field }}`) an |------|-------------|--------| | `secrets.vault` | HashiCorp Vault integration | secrets | | `secrets.aws` | AWS Secrets Manager integration | secrets | +| `secrets.keychain` | OS credential store (macOS Keychain, Linux Secret Service, Windows Credential Manager); requires libsecret/gnome-keyring/KWallet on Linux | secrets | ### Event Sourcing & Messaging Services | Type | Description | Plugin | diff --git a/README.md b/README.md index 5cb6e901..c1e54805 100644 --- a/README.md +++ b/README.md @@ -59,7 +59,7 @@ workflows: | **Storage/Persistence** | 7 | database.workflow, persistence.store, storage.s3, storage.gcs, storage.local, storage.sqlite, static.fileserver | | **Observability** | 4 | metrics.collector, health.checker, observability.otel, log.collector | | **Auth** | 2 | auth.jwt, auth.user-store | -| **Other** | 6 | data.transformer, webhook.sender, dynamic.component, secrets.vault, secrets.aws, workflow.registry | +| **Other** | 7 | data.transformer, webhook.sender, dynamic.component, secrets.vault, secrets.aws, secrets.keychain, workflow.registry | | **Triggers** | 5 | http, schedule, event, eventbus, mock | ### Security diff --git a/cmd/wfctl/infra_secrets.go b/cmd/wfctl/infra_secrets.go index 01a818d9..e9a57608 100644 --- a/cmd/wfctl/infra_secrets.go +++ b/cmd/wfctl/infra_secrets.go @@ -98,7 +98,14 @@ func resolveSecretsProvider(cfg *SecretsConfig) (secrets.Provider, error) { prefix, _ := c["prefix"].(string) return secrets.NewEnvProvider(prefix), nil + case "keychain": + service, _ := c["service"].(string) + if service == "" { + return nil, fmt.Errorf("secrets.keychain: 'service' is required") + } + return secrets.NewKeychainProvider(service), nil + default: - return nil, fmt.Errorf("unknown secrets provider %q (supported: github, vault, aws, env)", cfg.Provider) + return nil, fmt.Errorf("unknown secrets provider %q (supported: github, vault, aws, env, keychain)", cfg.Provider) } } diff --git a/cmd/wfctl/infra_secrets_test.go b/cmd/wfctl/infra_secrets_test.go index aebaf64f..6bd8c6c7 100644 --- a/cmd/wfctl/infra_secrets_test.go +++ b/cmd/wfctl/infra_secrets_test.go @@ -155,5 +155,33 @@ func TestParseSecretsConfig_MissingFile(t *testing.T) { } } +func TestResolveSecretsProvider_KeychainProvider(t *testing.T) { + cfg := &SecretsConfig{ + Provider: "keychain", + Config: map[string]any{"service": "test-workflow-app"}, + } + p, err := resolveSecretsProvider(cfg) + if err != nil { + t.Fatalf("resolveSecretsProvider keychain: %v", err) + } + if p.Name() != "keychain" { + t.Errorf("provider name = %q, want %q", p.Name(), "keychain") + } +} + +func TestResolveSecretsProvider_KeychainMissingService(t *testing.T) { + cfg := &SecretsConfig{ + Provider: "keychain", + Config: map[string]any{}, + } + _, err := resolveSecretsProvider(cfg) + if err == nil { + t.Error("expected error when 'service' is missing") + } +} + // Ensure GitHubSecretsProvider satisfies secrets.Provider interface. var _ secrets.Provider = (*secrets.GitHubSecretsProvider)(nil) + +// Ensure KeychainProvider satisfies secrets.Provider interface. +var _ secrets.Provider = (*secrets.KeychainProvider)(nil) diff --git a/cmd/wfctl/type_registry.go b/cmd/wfctl/type_registry.go index 689e090c..216b41b8 100644 --- a/cmd/wfctl/type_registry.go +++ b/cmd/wfctl/type_registry.go @@ -359,6 +359,12 @@ func KnownModuleTypes() map[string]ModuleTypeInfo { Stateful: false, ConfigKeys: []string{"region", "accessKeyId", "secretAccessKey"}, }, + "secrets.keychain": { + Type: "secrets.keychain", + Plugin: "secrets", + Stateful: false, + ConfigKeys: []string{"service"}, + }, // ai plugin "dynamic.component": { diff --git a/module/secrets_keychain.go b/module/secrets_keychain.go new file mode 100644 index 00000000..42edb0e0 --- /dev/null +++ b/module/secrets_keychain.go @@ -0,0 +1,80 @@ +package module + +import ( + "context" + "fmt" + + "github.com/GoCodeAlone/modular" + "github.com/GoCodeAlone/workflow/secrets" +) + +// SecretsKeychainModule provides an OS-keychain-backed secret provider as a modular service. +// It uses the macOS Keychain, Linux Secret Service, or Windows Credential Manager +// via github.com/zalando/go-keyring. +type SecretsKeychainModule struct { + name string + service string + provider *secrets.KeychainProvider + logger modular.Logger +} + +// NewSecretsKeychainModule creates a new OS keychain secrets module. +func NewSecretsKeychainModule(name string) *SecretsKeychainModule { + return &SecretsKeychainModule{ + name: name, + logger: &noopLogger{}, + } +} + +func (m *SecretsKeychainModule) Name() string { return m.name } + +func (m *SecretsKeychainModule) Init(app modular.Application) error { + m.logger = app.Logger() + return nil +} + +func (m *SecretsKeychainModule) ProvidesServices() []modular.ServiceProvider { + return []modular.ServiceProvider{ + { + Name: m.name, + Description: "OS Keychain Secrets Provider", + Instance: m, + }, + } +} + +func (m *SecretsKeychainModule) RequiresServices() []modular.ServiceDependency { + return nil +} + +// SetService sets the keychain service namespace. +func (m *SecretsKeychainModule) SetService(service string) { m.service = service } + +// Start initializes the keychain provider. +func (m *SecretsKeychainModule) Start(_ context.Context) error { + if m.service == "" { + return fmt.Errorf("secrets.keychain: 'service' is required") + } + m.provider = secrets.NewKeychainProvider(m.service) + m.logger.Info("Keychain secrets provider started", "service", m.service) + return nil +} + +// Stop is a no-op. +func (m *SecretsKeychainModule) Stop(_ context.Context) error { + m.logger.Info("Keychain secrets provider stopped") + return nil +} + +// Provider returns the underlying secrets.Provider. +func (m *SecretsKeychainModule) Provider() secrets.Provider { + return m.provider +} + +// Get retrieves a secret from the OS keychain. +func (m *SecretsKeychainModule) Get(ctx context.Context, key string) (string, error) { + if m.provider == nil { + return "", fmt.Errorf("secrets.keychain: provider not initialized") + } + return m.provider.Get(ctx, key) +} diff --git a/plugins/secrets/plugin.go b/plugins/secrets/plugin.go index 839e3fb1..d0c2de20 100644 --- a/plugins/secrets/plugin.go +++ b/plugins/secrets/plugin.go @@ -1,6 +1,6 @@ // Package secrets provides a plugin that registers secrets management modules: -// secrets.vault (HashiCorp Vault) and secrets.aws (AWS Secrets Manager), -// as well as the step.secret_rotate pipeline step type. +// secrets.vault (HashiCorp Vault), secrets.aws (AWS Secrets Manager), +// secrets.keychain (OS credential store), and the step.secret_rotate pipeline step type. package secrets import ( @@ -22,15 +22,15 @@ func New() *Plugin { BaseNativePlugin: plugin.BaseNativePlugin{ PluginName: "secrets", PluginVersion: "1.0.0", - PluginDescription: "Secrets management modules (Vault, AWS Secrets Manager)", + PluginDescription: "Secrets management modules (Vault, AWS Secrets Manager, OS Keychain)", }, Manifest: plugin.PluginManifest{ Name: "secrets", Version: "1.0.0", Author: "GoCodeAlone", - Description: "Secrets management modules (Vault, AWS Secrets Manager)", + Description: "Secrets management modules (Vault, AWS Secrets Manager, OS Keychain)", Tier: plugin.TierCore, - ModuleTypes: []string{"secrets.vault", "secrets.aws"}, + ModuleTypes: []string{"secrets.vault", "secrets.aws", "secrets.keychain"}, StepTypes: []string{"step.secret_rotate"}, Capabilities: []plugin.CapabilityDecl{ {Name: "secrets-management", Role: "provider", Priority: 50}, @@ -85,6 +85,13 @@ func (p *Plugin) ModuleFactories() map[string]plugin.ModuleFactory { } return am }, + "secrets.keychain": func(name string, config map[string]any) modular.Module { + km := module.NewSecretsKeychainModule(name) + if svc, ok := config["service"].(string); ok && svc != "" { + km.SetService(svc) + } + return km + }, } } diff --git a/plugins/secrets/plugin_test.go b/plugins/secrets/plugin_test.go index 988ab2a2..176acdf9 100644 --- a/plugins/secrets/plugin_test.go +++ b/plugins/secrets/plugin_test.go @@ -30,13 +30,13 @@ func TestModuleFactories(t *testing.T) { p := New() factories := p.ModuleFactories() - for _, name := range []string{"secrets.vault", "secrets.aws"} { + for _, name := range []string{"secrets.vault", "secrets.aws", "secrets.keychain"} { if _, ok := factories[name]; !ok { t.Errorf("missing module factory: %s", name) } } - if len(factories) != 2 { - t.Errorf("expected 2 module factories, got %d", len(factories)) + if len(factories) != 3 { + t.Errorf("expected 3 module factories, got %d", len(factories)) } } @@ -77,6 +77,22 @@ func TestAWSModuleFactory(t *testing.T) { } } +func TestKeychainModuleFactory(t *testing.T) { + p := New() + factories := p.ModuleFactories() + factory := factories["secrets.keychain"] + + mod := factory("my-keychain", map[string]any{ + "service": "my-app", + }) + if mod == nil { + t.Fatal("keychain factory returned nil") + } + if mod.Name() != "my-keychain" { + t.Errorf("expected name my-keychain, got %s", mod.Name()) + } +} + func TestPluginLoads(t *testing.T) { p := New() loader := plugin.NewPluginLoader(capability.NewRegistry(), schema.NewModuleSchemaRegistry()) @@ -85,7 +101,7 @@ func TestPluginLoads(t *testing.T) { } modules := loader.ModuleFactories() - if len(modules) != 2 { - t.Fatalf("expected 2 module factories after load, got %d", len(modules)) + if len(modules) != 3 { + t.Fatalf("expected 3 module factories after load, got %d", len(modules)) } } From 2b749784cf069096da85cf256fe5f6bfd41e76e3 Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Wed, 15 Apr 2026 21:15:25 -0400 Subject: [PATCH 4/7] =?UTF-8?q?fix(secrets):=20address=20review=20feedback?= =?UTF-8?q?=20=E2=80=94=20race=20safety=20+=20error=20context=20+=20assert?= =?UTF-8?q?ion=20placement?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- cmd/wfctl/infra_secrets_test.go | 3 --- secrets/keychain_provider.go | 19 ++++++++++++++++++- secrets/keychain_provider_test.go | 15 +++++++++++++-- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/cmd/wfctl/infra_secrets_test.go b/cmd/wfctl/infra_secrets_test.go index 6bd8c6c7..1d8d2772 100644 --- a/cmd/wfctl/infra_secrets_test.go +++ b/cmd/wfctl/infra_secrets_test.go @@ -182,6 +182,3 @@ func TestResolveSecretsProvider_KeychainMissingService(t *testing.T) { // Ensure GitHubSecretsProvider satisfies secrets.Provider interface. var _ secrets.Provider = (*secrets.GitHubSecretsProvider)(nil) - -// Ensure KeychainProvider satisfies secrets.Provider interface. -var _ secrets.Provider = (*secrets.KeychainProvider)(nil) diff --git a/secrets/keychain_provider.go b/secrets/keychain_provider.go index bf56acb7..dd36ec16 100644 --- a/secrets/keychain_provider.go +++ b/secrets/keychain_provider.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "sync" "github.com/zalando/go-keyring" ) @@ -17,11 +18,16 @@ import ( // On Linux, requires a running Secret Service implementation (libsecret, // gnome-keyring, or KWallet). Headless servers without one should use // FileProvider or VaultProvider instead. +// +// KeychainProvider is safe for concurrent use. mu guards all access to +// trackedKeys. type KeychainProvider struct { service string + mu sync.RWMutex // 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. + // All reads and writes are protected by mu. trackedKeys map[string]struct{} } @@ -33,8 +39,10 @@ func NewKeychainProvider(service string) *KeychainProvider { } } +// Name returns the provider identifier "keychain". func (p *KeychainProvider) Name() string { return "keychain" } +// Get retrieves the secret stored under the given key from the OS keychain. func (p *KeychainProvider) Get(ctx context.Context, key string) (string, error) { if err := ctx.Err(); err != nil { return "", err @@ -42,13 +50,14 @@ func (p *KeychainProvider) Get(ctx context.Context, key string) (string, error) v, err := keyring.Get(p.service, key) if err != nil { if errors.Is(err, keyring.ErrNotFound) { - return "", fmt.Errorf("secrets.keychain: %w", ErrNotFound) + return "", fmt.Errorf("secrets.keychain: key %q: %w", key, ErrNotFound) } return "", fmt.Errorf("secrets.keychain get %q: %w", key, err) } return v, nil } +// Set stores the given value under key in the OS keychain. func (p *KeychainProvider) Set(ctx context.Context, key, value string) error { if err := ctx.Err(); err != nil { return err @@ -56,10 +65,13 @@ func (p *KeychainProvider) Set(ctx context.Context, key, value string) error { if err := keyring.Set(p.service, key, value); err != nil { return fmt.Errorf("secrets.keychain set %q: %w", key, err) } + p.mu.Lock() p.trackedKeys[key] = struct{}{} + p.mu.Unlock() return nil } +// Delete removes the secret stored under key from the OS keychain. func (p *KeychainProvider) Delete(ctx context.Context, key string) error { if err := ctx.Err(); err != nil { return err @@ -70,14 +82,19 @@ func (p *KeychainProvider) Delete(ctx context.Context, key string) error { } return fmt.Errorf("secrets.keychain delete %q: %w", key, err) } + p.mu.Lock() delete(p.trackedKeys, key) + p.mu.Unlock() return nil } +// List returns all keys that have been set during the lifetime of this provider instance. func (p *KeychainProvider) List(ctx context.Context) ([]string, error) { if err := ctx.Err(); err != nil { return nil, err } + p.mu.RLock() + defer p.mu.RUnlock() out := make([]string, 0, len(p.trackedKeys)) for k := range p.trackedKeys { out = append(out, k) diff --git a/secrets/keychain_provider_test.go b/secrets/keychain_provider_test.go index 6a6f4505..c21a1d49 100644 --- a/secrets/keychain_provider_test.go +++ b/secrets/keychain_provider_test.go @@ -2,12 +2,18 @@ package secrets_test import ( "context" + "errors" + "reflect" + "sort" "testing" "github.com/GoCodeAlone/workflow/secrets" "github.com/zalando/go-keyring" ) +// Compile-time assertion: KeychainProvider must satisfy secrets.Provider. +var _ secrets.Provider = (*secrets.KeychainProvider)(nil) + func TestKeychainProvider_SetAndGet(t *testing.T) { keyring.MockInit() p := secrets.NewKeychainProvider("test-service") @@ -33,6 +39,9 @@ func TestKeychainProvider_GetMissing(t *testing.T) { if err == nil { t.Fatal("expected error for missing key, got nil") } + if !errors.Is(err, secrets.ErrNotFound) { + t.Errorf("expected ErrNotFound, got %v", err) + } } func TestKeychainProvider_Delete(t *testing.T) { @@ -58,7 +67,9 @@ func TestKeychainProvider_List(t *testing.T) { if err != nil { t.Fatalf("List: %v", err) } - if len(keys) != 2 { - t.Errorf("got %d keys, want 2", len(keys)) + sort.Strings(keys) + want := []string{"a", "b"} + if !reflect.DeepEqual(keys, want) { + t.Errorf("List() = %v, want %v", keys, want) } } From b8d6c7dca9637b905ce1f7c2bf304d430c5c9512 Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Thu, 16 Apr 2026 09:51:16 -0400 Subject: [PATCH 5/7] fix(secrets): empty-key validation, idempotent-delete trackedKeys cleanup, 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 --- go.mod | 2 +- secrets/keychain_provider.go | 15 ++++++++++- secrets/keychain_provider_test.go | 42 +++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 31884015..5d7d3ccf 100644 --- a/go.mod +++ b/go.mod @@ -59,6 +59,7 @@ require ( github.com/tliron/glsp v0.2.2 github.com/tochemey/goakt/v4 v4.1.1 github.com/xdg-go/scram v1.2.0 + github.com/zalando/go-keyring v0.2.8 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.68.0 go.opentelemetry.io/otel v1.43.0 go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.43.0 @@ -281,7 +282,6 @@ require ( github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect github.com/yosida95/uritemplate/v3 v3.0.2 // indirect github.com/yuin/gopher-lua v1.1.1 // indirect - github.com/zalando/go-keyring v0.2.8 // indirect github.com/zeebo/xxh3 v1.1.0 // indirect go.etcd.io/bbolt v1.4.3 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect diff --git a/secrets/keychain_provider.go b/secrets/keychain_provider.go index dd36ec16..9518ce9c 100644 --- a/secrets/keychain_provider.go +++ b/secrets/keychain_provider.go @@ -47,6 +47,9 @@ func (p *KeychainProvider) Get(ctx context.Context, key string) (string, error) if err := ctx.Err(); err != nil { return "", err } + if key == "" { + return "", ErrInvalidKey + } v, err := keyring.Get(p.service, key) if err != nil { if errors.Is(err, keyring.ErrNotFound) { @@ -62,6 +65,9 @@ func (p *KeychainProvider) Set(ctx context.Context, key, value string) error { if err := ctx.Err(); err != nil { return err } + if key == "" { + return ErrInvalidKey + } if err := keyring.Set(p.service, key, value); err != nil { return fmt.Errorf("secrets.keychain set %q: %w", key, err) } @@ -76,9 +82,16 @@ func (p *KeychainProvider) Delete(ctx context.Context, key string) error { if err := ctx.Err(); err != nil { return err } + if key == "" { + return ErrInvalidKey + } if err := keyring.Delete(p.service, key); err != nil { if errors.Is(err, keyring.ErrNotFound) { - return nil // idempotent + // Idempotent: still clean up trackedKeys so List() stays consistent. + p.mu.Lock() + delete(p.trackedKeys, key) + p.mu.Unlock() + return nil } return fmt.Errorf("secrets.keychain delete %q: %w", key, err) } diff --git a/secrets/keychain_provider_test.go b/secrets/keychain_provider_test.go index c21a1d49..7f3fd37c 100644 --- a/secrets/keychain_provider_test.go +++ b/secrets/keychain_provider_test.go @@ -73,3 +73,45 @@ func TestKeychainProvider_List(t *testing.T) { t.Errorf("List() = %v, want %v", keys, want) } } + +func TestKeychainProvider_EmptyKey(t *testing.T) { + keyring.MockInit() + p := secrets.NewKeychainProvider("test-service") + ctx := context.Background() + + if _, err := p.Get(ctx, ""); err != secrets.ErrInvalidKey { + t.Errorf("Get empty key: expected ErrInvalidKey, got %v", err) + } + if err := p.Set(ctx, "", "val"); err != secrets.ErrInvalidKey { + t.Errorf("Set empty key: expected ErrInvalidKey, got %v", err) + } + if err := p.Delete(ctx, ""); err != secrets.ErrInvalidKey { + t.Errorf("Delete empty key: expected ErrInvalidKey, got %v", err) + } +} + +func TestKeychainProvider_DeleteIdempotent_CleansTrackedKeys(t *testing.T) { + keyring.MockInit() + p := secrets.NewKeychainProvider("test-service") + ctx := context.Background() + + // Set then delete a key. + _ = p.Set(ctx, "ephemeral", "val") + _ = p.Delete(ctx, "ephemeral") + + // Delete again (idempotent, key already gone from keyring). + if err := p.Delete(ctx, "ephemeral"); err != nil { + t.Fatalf("second Delete: %v", err) + } + + // List should not contain the deleted key. + keys, err := p.List(ctx) + if err != nil { + t.Fatalf("List: %v", err) + } + for _, k := range keys { + if k == "ephemeral" { + t.Error("List() still contains deleted key 'ephemeral'") + } + } +} From 08d35983fed0818f39bd71b61c83ea73aeb1dfe4 Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Thu, 16 Apr 2026 10:10:28 -0400 Subject: [PATCH 6/7] chore: go mod tidy in example/ for go-keyring transitive deps 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 --- example/go.mod | 3 +++ example/go.sum | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/example/go.mod b/example/go.mod index f8a6f2b0..beb95ce1 100644 --- a/example/go.mod +++ b/example/go.mod @@ -74,6 +74,7 @@ require ( github.com/cncf/xds/go v0.0.0-20260202195803-dba9d589def2 // indirect github.com/containerd/errdefs v1.0.0 // indirect github.com/containerd/errdefs/pkg v0.3.0 // indirect + github.com/danieljoos/wincred v1.2.3 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/deckarep/golang-set/v2 v2.8.0 // indirect github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f // indirect @@ -111,6 +112,7 @@ require ( github.com/go-openapi/swag/typeutils v0.25.5 // indirect github.com/go-openapi/swag/yamlutils v0.25.5 // indirect github.com/gobwas/glob v0.2.3 // indirect + github.com/godbus/dbus/v5 v5.2.2 // indirect github.com/golang-jwt/jwt/v5 v5.3.1 // indirect github.com/golobby/cast v1.3.3 // indirect github.com/google/btree v1.1.3 // indirect @@ -195,6 +197,7 @@ require ( github.com/xdg-go/pbkdf2 v1.0.0 // indirect github.com/xdg-go/scram v1.2.0 // indirect github.com/xdg-go/stringprep v1.0.4 // indirect + github.com/zalando/go-keyring v0.2.8 // indirect github.com/zeebo/xxh3 v1.1.0 // indirect go.etcd.io/bbolt v1.4.3 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect diff --git a/example/go.sum b/example/go.sum index 1988f24c..ec57d9ad 100644 --- a/example/go.sum +++ b/example/go.sum @@ -187,6 +187,8 @@ github.com/cucumber/godog v0.15.1 h1:rb/6oHDdvVZKS66hrhpjFQFHjthFSrQBCOI1LwshNTI github.com/cucumber/godog v0.15.1/go.mod h1:qju+SQDewOljHuq9NSM66s0xEhogx0q30flfxL4WUk8= github.com/cucumber/messages/go/v21 v21.0.1 h1:wzA0LxwjlWQYZd32VTlAVDTkW6inOFmSM+RuOwHZiMI= github.com/cucumber/messages/go/v21 v21.0.1/go.mod h1:zheH/2HS9JLVFukdrsPWoPdmUtmYQAQPLk7w5vWsk5s= +github.com/danieljoos/wincred v1.2.3 h1:v7dZC2x32Ut3nEfRH+vhoZGvN72+dQ/snVXo/vMFLdQ= +github.com/danieljoos/wincred v1.2.3/go.mod h1:6qqX0WNrS4RzPZ1tnroDzq9kY3fu1KwE7MRLQK4X0bs= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= @@ -297,6 +299,8 @@ github.com/go-viper/mapstructure/v2 v2.5.0 h1:vM5IJoUAy3d7zRSVtIwQgBj7BiWtMPfmPE github.com/go-viper/mapstructure/v2 v2.5.0/go.mod h1:oJDH3BJKyqBA2TXFhDsKDGDTlndYOZ6rGS0BRZIxGhM= github.com/gobwas/glob v0.2.3 h1:A4xDbljILXROh+kObIiy5kIaPYD8e96x1tgBhUI5J+Y= github.com/gobwas/glob v0.2.3/go.mod h1:d3Ez4x06l9bZtSvzIay5+Yzi0fmZzPgnTbPcKjJAkT8= +github.com/godbus/dbus/v5 v5.2.2 h1:TUR3TgtSVDmjiXOgAAyaZbYmIeP3DPkld3jgKGV8mXQ= +github.com/godbus/dbus/v5 v5.2.2/go.mod h1:3AAv2+hPq5rdnr5txxxRwiGjPXamgoIHgz9FPBfOp3c= github.com/gofrs/uuid v4.4.0+incompatible h1:3qXRTX8/NbyulANqlc0lchS1gqAVxRgsuW1YrTJupqA= github.com/gofrs/uuid v4.4.0+incompatible/go.mod h1:b2aQJv3Z4Fp6yNu3cdSllBxTCLRxnplIgP/c0N/04lM= github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ= @@ -660,6 +664,8 @@ github.com/yuin/gopher-lua v1.1.1 h1:kYKnWBjvbNP4XLT3+bPEwAXJx262OhaHDWDVOPjL46M github.com/yuin/gopher-lua v1.1.1/go.mod h1:GBR0iDaNXjAgGg9zfCvksxSRnQx76gclCIb7kdAd1Pw= github.com/yusufpapurcu/wmi v1.2.4 h1:zFUKzehAFReQwLys1b/iSMl+JQGSCSjtVqQn9bBrPo0= github.com/yusufpapurcu/wmi v1.2.4/go.mod h1:SBZ9tNy3G9/m5Oi98Zks0QjeHVDvuK0qfxQmPyzfmi0= +github.com/zalando/go-keyring v0.2.8 h1:6sD/Ucpl7jNq10rM2pgqTs0sZ9V3qMrqfIIy5YPccHs= +github.com/zalando/go-keyring v0.2.8/go.mod h1:tsMo+VpRq5NGyKfxoBVjCuMrG47yj8cmakZDO5QGii0= github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ= github.com/zeebo/assert v1.3.0/go.mod h1:Pq9JiuJQpG8JLJdtkwrJESF0Foym2/D9XMU5ciN/wJ0= github.com/zeebo/xxh3 v1.1.0 h1:s7DLGDK45Dyfg7++yxI0khrfwq9661w9EN78eP/UZVs= From bbada0858bea9bc4b632d6e99b832c31fa9a6968 Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Thu, 16 Apr 2026 10:38:33 -0400 Subject: [PATCH 7/7] fix(secrets): validate empty service in NewKeychainProvider + assert 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 --- cmd/wfctl/infra_secrets.go | 2 +- module/secrets_keychain.go | 6 +++- secrets/keychain_provider.go | 9 ++++-- secrets/keychain_provider_test.go | 49 ++++++++++++++++++++++++------- 4 files changed, 51 insertions(+), 15 deletions(-) diff --git a/cmd/wfctl/infra_secrets.go b/cmd/wfctl/infra_secrets.go index 9fbffed6..56218ed2 100644 --- a/cmd/wfctl/infra_secrets.go +++ b/cmd/wfctl/infra_secrets.go @@ -103,7 +103,7 @@ func resolveSecretsProvider(cfg *SecretsConfig) (secrets.Provider, error) { if service == "" { return nil, fmt.Errorf("secrets.keychain: 'service' is required") } - return secrets.NewKeychainProvider(service), nil + return secrets.NewKeychainProvider(service) default: return nil, fmt.Errorf("unknown secrets provider %q (supported: github, vault, aws, env, keychain)", cfg.Provider) diff --git a/module/secrets_keychain.go b/module/secrets_keychain.go index 42edb0e0..f45fca30 100644 --- a/module/secrets_keychain.go +++ b/module/secrets_keychain.go @@ -55,7 +55,11 @@ func (m *SecretsKeychainModule) Start(_ context.Context) error { if m.service == "" { return fmt.Errorf("secrets.keychain: 'service' is required") } - m.provider = secrets.NewKeychainProvider(m.service) + provider, err := secrets.NewKeychainProvider(m.service) + if err != nil { + return err + } + m.provider = provider m.logger.Info("Keychain secrets provider started", "service", m.service) return nil } diff --git a/secrets/keychain_provider.go b/secrets/keychain_provider.go index 9518ce9c..b3edeaf9 100644 --- a/secrets/keychain_provider.go +++ b/secrets/keychain_provider.go @@ -32,11 +32,16 @@ type KeychainProvider struct { } // NewKeychainProvider returns a provider namespaced to the given service name. -func NewKeychainProvider(service string) *KeychainProvider { +// Service must not be empty — an empty service stores secrets in a shared +// namespace where they can collide across applications. +func NewKeychainProvider(service string) (*KeychainProvider, error) { + if service == "" { + return nil, fmt.Errorf("secrets.keychain: service name must not be empty") + } return &KeychainProvider{ service: service, trackedKeys: make(map[string]struct{}), - } + }, nil } // Name returns the provider identifier "keychain". diff --git a/secrets/keychain_provider_test.go b/secrets/keychain_provider_test.go index 7f3fd37c..f37b3e20 100644 --- a/secrets/keychain_provider_test.go +++ b/secrets/keychain_provider_test.go @@ -14,9 +14,19 @@ import ( // Compile-time assertion: KeychainProvider must satisfy secrets.Provider. var _ secrets.Provider = (*secrets.KeychainProvider)(nil) +// mustNewKeychainProvider is a test helper that creates a provider or fails the test. +func mustNewKeychainProvider(t *testing.T, service string) *secrets.KeychainProvider { + t.Helper() + p, err := secrets.NewKeychainProvider(service) + if err != nil { + t.Fatalf("NewKeychainProvider(%q): %v", service, err) + } + return p +} + func TestKeychainProvider_SetAndGet(t *testing.T) { keyring.MockInit() - p := secrets.NewKeychainProvider("test-service") + p := mustNewKeychainProvider(t, "test-service") ctx := context.Background() if err := p.Set(ctx, "api_key", "secret-123"); err != nil { @@ -34,7 +44,7 @@ func TestKeychainProvider_SetAndGet(t *testing.T) { func TestKeychainProvider_GetMissing(t *testing.T) { keyring.MockInit() - p := secrets.NewKeychainProvider("test-service") + p := mustNewKeychainProvider(t, "test-service") _, err := p.Get(context.Background(), "absent") if err == nil { t.Fatal("expected error for missing key, got nil") @@ -46,9 +56,11 @@ func TestKeychainProvider_GetMissing(t *testing.T) { func TestKeychainProvider_Delete(t *testing.T) { keyring.MockInit() - p := secrets.NewKeychainProvider("test-service") + p := mustNewKeychainProvider(t, "test-service") ctx := context.Background() - _ = p.Set(ctx, "x", "1") + if err := p.Set(ctx, "x", "1"); err != nil { + t.Fatalf("setup Set: %v", err) + } if err := p.Delete(ctx, "x"); err != nil { t.Fatalf("Delete: %v", err) } @@ -59,10 +71,14 @@ func TestKeychainProvider_Delete(t *testing.T) { func TestKeychainProvider_List(t *testing.T) { keyring.MockInit() - p := secrets.NewKeychainProvider("test-service") + p := mustNewKeychainProvider(t, "test-service") ctx := context.Background() - _ = p.Set(ctx, "a", "1") - _ = p.Set(ctx, "b", "2") + if err := p.Set(ctx, "a", "1"); err != nil { + t.Fatalf("setup Set a: %v", err) + } + if err := p.Set(ctx, "b", "2"); err != nil { + t.Fatalf("setup Set b: %v", err) + } keys, err := p.List(ctx) if err != nil { t.Fatalf("List: %v", err) @@ -74,9 +90,16 @@ func TestKeychainProvider_List(t *testing.T) { } } +func TestKeychainProvider_EmptyService(t *testing.T) { + _, err := secrets.NewKeychainProvider("") + if err == nil { + t.Fatal("expected error for empty service name") + } +} + func TestKeychainProvider_EmptyKey(t *testing.T) { keyring.MockInit() - p := secrets.NewKeychainProvider("test-service") + p := mustNewKeychainProvider(t, "test-service") ctx := context.Background() if _, err := p.Get(ctx, ""); err != secrets.ErrInvalidKey { @@ -92,12 +115,16 @@ func TestKeychainProvider_EmptyKey(t *testing.T) { func TestKeychainProvider_DeleteIdempotent_CleansTrackedKeys(t *testing.T) { keyring.MockInit() - p := secrets.NewKeychainProvider("test-service") + p := mustNewKeychainProvider(t, "test-service") ctx := context.Background() // Set then delete a key. - _ = p.Set(ctx, "ephemeral", "val") - _ = p.Delete(ctx, "ephemeral") + if err := p.Set(ctx, "ephemeral", "val"); err != nil { + t.Fatalf("setup Set: %v", err) + } + if err := p.Delete(ctx, "ephemeral"); err != nil { + t.Fatalf("setup Delete: %v", err) + } // Delete again (idempotent, key already gone from keyring). if err := p.Delete(ctx, "ephemeral"); err != nil {