diff --git a/config.go b/config.go index 469e45d..f14682c 100644 --- a/config.go +++ b/config.go @@ -78,11 +78,11 @@ func SaveConfigJSON(config Config, path string) error { return fmt.Errorf("failed to marshal config: %w", err) } - if err := os.MkdirAll(filepath.Dir(path), 0750); err != nil { + if err := os.MkdirAll(filepath.Dir(path), vaultDirMode); err != nil { return fmt.Errorf("failed to create config directory: %w", err) } - if err := os.WriteFile(filepath.Clean(path), data, 0600); err != nil { + if err := os.WriteFile(filepath.Clean(path), data, vaultFileMode); err != nil { return fmt.Errorf("failed to write config file: %w", err) } diff --git a/config_test.go b/config_test.go new file mode 100644 index 0000000..3380766 --- /dev/null +++ b/config_test.go @@ -0,0 +1,44 @@ +package vault_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/flowexec/vault" +) + +// A vault config carries the provider's command templates and environment +// values, and its presence alone discloses which secret backends a user has +// configured. The file was already 0600, but the directory SaveConfigJSON +// created for it was group-readable. +func TestSaveConfigJSONWritesOwnerOnly(t *testing.T) { + dir := t.TempDir() + nested := filepath.Join(dir, "configs") + path := filepath.Join(nested, "myvault.json") + + cfg := vault.Config{ + ID: "myvault", + Type: vault.ProviderTypeUnencrypted, + Unencrypted: &vault.UnencryptedConfig{StoragePath: dir}, + } + if err := vault.SaveConfigJSON(cfg, path); err != nil { + t.Fatalf("SaveConfigJSON() error = %v", err) + } + + fileInfo, err := os.Stat(path) + if err != nil { + t.Fatalf("Stat(file) error = %v", err) + } + if perm := fileInfo.Mode().Perm(); perm != 0600 { + t.Errorf("config file mode = %o, want 0600", perm) + } + + dirInfo, err := os.Stat(nested) + if err != nil { + t.Fatalf("Stat(dir) error = %v", err) + } + if perm := dirInfo.Mode().Perm(); perm != 0700 { + t.Errorf("config directory mode = %o, want 0700", perm) + } +} diff --git a/external.go b/external.go index d38f0a7..00faa62 100644 --- a/external.go +++ b/external.go @@ -115,7 +115,7 @@ func (v *ExternalVaultProvider) getSecretLocked(key string) (Secret, error) { return nil, fmt.Errorf("failed to parse output: %w", err) } } else { - secretValue = strings.TrimSpace(output) + secretValue = trimCommandNewline(output) } return NewSecretValue([]byte(secretValue)), nil @@ -308,6 +308,17 @@ func (v *ExternalVaultProvider) hasSecretViaExistsCmd(key string) (bool, error) // A non-zero exit conventionally means "absent", but it is also how an expired // session, a network failure, or a permissions problem surfaces. NotFoundPattern // lets a config say which failures actually mean absence. + // + // It can only say so about failures that produce a message, though. An exists + // command may answer purely by exit status -- `test -f`, `jq -e` -- and then + // there is no text for the pattern to match. Treating that as "the pattern did + // not match, so this is a real error" turns every ordinary miss into a failure, + // so a silent non-zero exit is taken at its word: absent. + var cmdErr *commandError + if errors.As(err, &cmdErr) && cmdErr.diagnostics() == "" { + return false, nil + } + if v.cfg.NotFoundPattern != "" && !strings.Contains(err.Error(), v.cfg.NotFoundPattern) { return false, err } @@ -383,6 +394,27 @@ func (v *ExternalVaultProvider) Metadata() (Metadata, error) { return Metadata{RawData: metadataOutput}, nil } +// commandError carries a failing command's diagnostic output alongside the +// error, so callers can tell "the command answered by exit status alone" from +// "the command complained about something" without parsing an error string. +type commandError struct { + stderr string + err error +} + +func (e *commandError) Error() string { + if e.stderr == "" { + return fmt.Sprintf("command failed: %v", e.err) + } + return fmt.Sprintf("command failed: %v, stderr: %s", e.err, e.stderr) +} + +func (e *commandError) Unwrap() error { return e.err } + +// diagnostics returns the command's output, trimmed. Empty means the command +// said nothing and reported only through its exit status. +func (e *commandError) diagnostics() string { return strings.TrimSpace(e.stderr) } + func (v *ExternalVaultProvider) executeCommand(cmd, input string) (string, error) { ctx := v.ctx if ctx == nil { @@ -396,7 +428,7 @@ func (v *ExternalVaultProvider) executeCommand(cmd, input string) (string, error output, runErr := v.execute(ctx, cmd, input, v.cfg.WorkingDir, v.environmentToSlice()) if runErr != nil { - return "", fmt.Errorf("command failed: %w, stderr: %s", runErr, output) + return "", &commandError{stderr: output, err: runErr} } return output, nil @@ -515,7 +547,25 @@ func execute(ctx context.Context, cmd, input, dir string, envList []string) (str // Only stdout is the result. Merging stderr in on success concatenates any // warning the backend emits (e.g. "gpg: WARNING: unsafe permissions") onto // the secret value itself. stderr is still returned on the error path above. - return strings.TrimSpace(stdOutBuffer.String()), nil + // + // Returned verbatim: trimming here would silently corrupt any secret with + // deliberate leading or trailing whitespace. Callers that want a tidy string + // (list, metadata) trim for themselves; GetSecret strips only the single + // trailing newline a command adds. + return stdOutBuffer.String(), nil +} + +// trimCommandNewline removes the one trailing line ending a command conventionally +// adds to its output, and nothing else. +// +// TrimSpace would take real data with it: a passphrase may legitimately begin or +// end with a space, and a PEM block ends in a newline that some parsers require. +// A secret whose true value ends in a newline is still indistinguishable from one +// that does not -- that is inherent to reading a value off a command's stdout, +// and no amount of trimming policy can recover it. +func trimCommandNewline(s string) string { + s = strings.TrimSuffix(s, "\n") + return strings.TrimSuffix(s, "\r") } // expandEnv returns a new map with environment references expanded. It must not diff --git a/external_test.go b/external_test.go index 3319273..5c7cac4 100644 --- a/external_test.go +++ b/external_test.go @@ -419,9 +419,14 @@ func TestHasSecret_NotFoundPatternDistinguishesRealFailures(t *testing.T) { cfg.Exists.CommandTemplate = "check {{key}}" cfg.NotFoundPattern = "ParameterNotFound" + // execute() reports a generic "exited with non-zero status" error and returns + // the backend's own message as the command output, so these mocks put the + // diagnostic where the real executor puts it. A mock that instead encodes it + // in the error would be testing a shape that cannot occur. t.Run("absent", func(t *testing.T) { provider := newTestProvider(t, cfg) - provider.SetExecutionFunc(capturingExec(&execCapture{}, "", fmt.Errorf("ParameterNotFound: nope"))) + provider.SetExecutionFunc(capturingExec(&execCapture{}, + "ParameterNotFound: nope", fmt.Errorf("exit status 1"))) exists, err := provider.HasSecret("k") if err != nil { @@ -434,7 +439,8 @@ func TestHasSecret_NotFoundPatternDistinguishesRealFailures(t *testing.T) { t.Run("real failure surfaces", func(t *testing.T) { provider := newTestProvider(t, cfg) - provider.SetExecutionFunc(capturingExec(&execCapture{}, "", fmt.Errorf("ExpiredToken: session expired"))) + provider.SetExecutionFunc(capturingExec(&execCapture{}, + "ExpiredToken: session expired", fmt.Errorf("exit status 254"))) if _, err := provider.HasSecret("k"); err == nil { t.Error("HasSecret() error = nil, want the expired-session error to surface") @@ -635,3 +641,118 @@ func TestConcurrentGetSecretDoesNotRaceOnEnvironment(t *testing.T) { t.Errorf("config Environment was mutated: LITERAL = %q, want %q", got, "$(tty)") } } + +// An exists command may answer purely by exit status -- `test -f`, `jq -e` -- +// leaving no message for NotFoundPattern to match. Treating that as "the +// pattern did not match, so this is a real error" turned every ordinary miss +// into a failure. Found by driving the pass preset against a real store. +func TestHasSecret_SilentNonZeroExitMeansAbsent(t *testing.T) { + cfg := validExternalConfig() + cfg.Exists.CommandTemplate = "test -f /nonexistent/{{key}}" + cfg.NotFoundPattern = "is not in the password store" + + provider := newTestProvider(t, cfg) + + exists, err := provider.HasSecret("missing") + if err != nil { + t.Fatalf("HasSecret() on a silent non-zero exit = %v, want a plain false", err) + } + if exists { + t.Error("HasSecret() = true, want false") + } +} + +// A command that *does* complain still gets its message checked, so an expired +// session is not silently reported as "the secret does not exist". +func TestHasSecret_DiagnosticNotMatchingPatternSurfaces(t *testing.T) { + cfg := validExternalConfig() + cfg.Exists.CommandTemplate = "echo 'ExpiredToken: session expired' 1>&2; exit 1" + cfg.NotFoundPattern = "ParameterNotFound" + + provider := newTestProvider(t, cfg) + + if _, err := provider.HasSecret("k"); err == nil { + t.Error("HasSecret() error = nil, want the expired-session failure to surface") + } +} + +func TestHasSecret_DiagnosticMatchingPatternMeansAbsent(t *testing.T) { + cfg := validExternalConfig() + cfg.Exists.CommandTemplate = "echo 'ParameterNotFound: nope' 1>&2; exit 1" + cfg.NotFoundPattern = "ParameterNotFound" + + provider := newTestProvider(t, cfg) + + exists, err := provider.HasSecret("k") + if err != nil { + t.Fatalf("HasSecret() = %v, want a plain false", err) + } + if exists { + t.Error("HasSecret() = true, want false") + } +} + +// GetSecret used to TrimSpace the command's output, so a secret with deliberate +// leading or trailing whitespace was stored correctly by the backend and came +// back mangled. Only the single trailing newline a command adds is removed. +func TestGetSecret_PreservesDeliberateWhitespace(t *testing.T) { + for _, tc := range []struct { + name, stdout, want string + }{ + {"leading space", " value\n", " value"}, + {"trailing space", "value \n", "value "}, + {"only spaces", " \n", " "}, + {"tabs", "\tvalue\t\n", "\tvalue\t"}, + {"internal newlines", "line1\nline2\n", "line1\nline2"}, + {"no trailing newline", "value", "value"}, + {"crlf", "value\r\n", "value"}, + {"empty", "", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + provider := newTestProvider(t, validExternalConfig()) + provider.SetExecutionFunc(func( + _ context.Context, _, _, _ string, _ []string, + ) (string, error) { + return tc.stdout, nil + }) + + secret, err := provider.GetSecret("k") + if err != nil { + t.Fatalf("GetSecret() error = %v", err) + } + if got := secret.PlainTextString(); got != tc.want { + t.Errorf("GetSecret() = %q, want %q", got, tc.want) + } + }) + } +} + +// List and metadata still tidy their output; only the secret value is verbatim. +func TestListAndMetadataStillTrim(t *testing.T) { + cfg := validExternalConfig() + cfg.List.CommandTemplate = "ls" + cfg.Metadata.CommandTemplate = "status" + + provider := newTestProvider(t, cfg) + provider.SetExecutionFunc(func( + _ context.Context, _, _, _ string, _ []string, + ) (string, error) { + return " alpha \n beta \n", nil + }) + + keys, err := provider.ListSecrets() + if err != nil { + t.Fatalf("ListSecrets() error = %v", err) + } + if len(keys) != 2 || keys[0] != "alpha" || keys[1] != "beta" { + t.Errorf("ListSecrets() = %q, want [alpha beta]", keys) + } + + md, err := provider.Metadata() + if err != nil { + t.Fatalf("Metadata() error = %v", err) + } + if strings.HasPrefix(md.RawData, " ") || strings.HasSuffix(md.RawData, "\n") { + t.Errorf("Metadata().RawData = %q, want it trimmed", md.RawData) + } +} diff --git a/keyring.go b/keyring.go index a33fecd..88dedcc 100644 --- a/keyring.go +++ b/keyring.go @@ -58,16 +58,64 @@ func (v *KeyringVault) namespaced(kind, key string) string { return fmt.Sprintf("%d:%s:%s:%s", len(v.id), v.id, kind, key) } -func (v *KeyringVault) metadataKey() string { - return v.namespaced("metadata", "") +// legacyNamespaced reproduces the pre-v0.3.0 entry name. +// +// Renaming the entries fixed a real collision but made every secret in an +// existing keyring vault unreachable: the data is still in the OS keyring, just +// under the old name. Reads fall back to this, and the next write migrates the +// entry, so an existing vault keeps working without the user re-entering +// anything. +func (v *KeyringVault) legacyNamespaced(kind, key string) string { + if key == "" { + return fmt.Sprintf("%s-%s", v.id, kind) + } + return fmt.Sprintf("%s-%s-%s", v.id, kind, key) } -func (v *KeyringVault) secretKey(key string) string { - return v.namespaced("secret", key) +// get reads an entry, falling back to the pre-v0.3.0 name. +// +// Whether the value came from a legacy entry is deliberately not reported. It +// would only be useful for deciding to migrate, and set already deletes the +// legacy entry unconditionally, so the migration happens on the next write +// either way. +func (v *KeyringVault) get(kind, key string) (string, error) { + data, err := keyring.Get(v.service, v.namespaced(kind, key)) + if err == nil { + return data, nil + } + if !errors.Is(err, keyring.ErrNotFound) { + return "", err + } + + data, legacyErr := keyring.Get(v.service, v.legacyNamespaced(kind, key)) + if legacyErr != nil { + // Report the miss against the current name; the legacy lookup is an + // implementation detail and its error would only confuse. + return "", err + } + return data, nil } -func (v *KeyringVault) secretsListKey() string { - return v.namespaced("secrets-list", "") +// set writes an entry under the current name and removes any legacy entry it +// supersedes, so the rename completes on first write rather than leaving two +// copies of a secret in the keyring. +func (v *KeyringVault) set(kind, key, value string) error { + if err := keyring.Set(v.service, v.namespaced(kind, key), value); err != nil { + return err + } + _ = keyring.Delete(v.service, v.legacyNamespaced(kind, key)) + return nil +} + +// remove deletes both the current and legacy entries so a delete cannot leave +// the old copy behind to reappear on the next read. +func (v *KeyringVault) remove(kind, key string) error { + err := keyring.Delete(v.service, v.namespaced(kind, key)) + legacyErr := keyring.Delete(v.service, v.legacyNamespaced(kind, key)) + if err != nil && errors.Is(err, keyring.ErrNotFound) && legacyErr == nil { + return nil + } + return err } func (v *KeyringVault) initMetadata() error { @@ -81,7 +129,7 @@ func (v *KeyringVault) initMetadata() error { } func (v *KeyringVault) loadMetadata() error { - data, err := keyring.Get(v.service, v.metadataKey()) + data, err := v.get("metadata", "") if err != nil { return err } @@ -103,11 +151,11 @@ func (v *KeyringVault) saveMetadata() error { return fmt.Errorf("failed to marshal metadata: %w", err) } - return keyring.Set(v.service, v.metadataKey(), string(data)) + return v.set("metadata", "", string(data)) } func (v *KeyringVault) loadSecretsList() ([]string, error) { - data, err := keyring.Get(v.service, v.secretsListKey()) + data, err := v.get("secrets-list", "") if err != nil { if errors.Is(err, keyring.ErrNotFound) { return []string{}, nil @@ -129,7 +177,7 @@ func (v *KeyringVault) saveSecretsList(secrets []string) error { return fmt.Errorf("failed to marshal secrets list: %w", err) } - return keyring.Set(v.service, v.secretsListKey(), string(data)) + return v.set("secrets-list", "", string(data)) } func (v *KeyringVault) addSecretToList(key string) error { @@ -187,7 +235,7 @@ func (v *KeyringVault) GetSecret(key string) (Secret, error) { return nil, err } - data, err := keyring.Get(v.service, v.secretKey(key)) + data, err := v.get("secret", key) if err != nil { if errors.Is(err, keyring.ErrNotFound) { return nil, ErrSecretNotFound @@ -206,7 +254,7 @@ func (v *KeyringVault) SetSecret(key string, secret Secret) error { return err } - if err := keyring.Set(v.service, v.secretKey(key), secret.PlainTextString()); err != nil { + if err := v.set("secret", key, secret.PlainTextString()); err != nil { return fmt.Errorf("failed to set secret in keyring: %w", err) } @@ -226,7 +274,7 @@ func (v *KeyringVault) DeleteSecret(key string) error { } // Check if secret exists first - _, err := keyring.Get(v.service, v.secretKey(key)) + _, err := v.get("secret", key) if err != nil { if errors.Is(err, keyring.ErrNotFound) { return ErrSecretNotFound @@ -234,7 +282,7 @@ func (v *KeyringVault) DeleteSecret(key string) error { return fmt.Errorf("failed to check secret existence: %w", err) } - if err := keyring.Delete(v.service, v.secretKey(key)); err != nil { + if err := v.remove("secret", key); err != nil { return fmt.Errorf("failed to delete secret from keyring: %w", err) } @@ -269,7 +317,7 @@ func (v *KeyringVault) HasSecret(key string) (bool, error) { return false, err } - _, err := keyring.Get(v.service, v.secretKey(key)) + _, err := v.get("secret", key) if err != nil { if errors.Is(err, keyring.ErrNotFound) { return false, nil diff --git a/keyring_test.go b/keyring_test.go index 3cafc8c..c4685ac 100644 --- a/keyring_test.go +++ b/keyring_test.go @@ -375,3 +375,134 @@ func TestKeyringVault_SortedOutput(t *testing.T) { } } } + +// Renaming keyring entries in v0.3.0 fixed a real collision, but the data for +// every existing keyring vault is still stored under the old names. Without a +// fallback, upgrading silently makes every secret look deleted -- the keyring +// still holds it, the vault just looks in the wrong place. +func TestKeyringVault_ReadsPreV030EntryNames(t *testing.T) { + keyring.MockInit() + + const ( + vaultID = "legacy-vault" + key = "api-key" + value = "legacy-secret-value" + ) + + // Seed the keyring exactly as a pre-v0.3.0 vault would have left it. + if err := keyring.Set(testKeyringService, vaultID+"-secret-"+key, value); err != nil { + t.Fatalf("failed to seed legacy secret: %v", err) + } + if err := keyring.Set(testKeyringService, vaultID+"-secrets-list", `["`+key+`"]`); err != nil { + t.Fatalf("failed to seed legacy secrets list: %v", err) + } + if err := keyring.Set(testKeyringService, vaultID+"-metadata", `{"created":"2024-01-01T00:00:00Z"}`); err != nil { + t.Fatalf("failed to seed legacy metadata: %v", err) + } + + vlt, _, err := vault.New(vaultID, + vault.WithProvider(vault.ProviderTypeKeyring), + vault.WithKeyringService(testKeyringService), + ) + if err != nil { + t.Fatalf("Failed to open a pre-v0.3.0 keyring vault: %v", err) + } + defer vlt.Close() + + secret, err := vlt.GetSecret(key) + if err != nil { + t.Fatalf("GetSecret() on a legacy entry = %v, want the stored value", err) + } + if got := secret.PlainTextString(); got != value { + t.Errorf("GetSecret() = %q, want %q", got, value) + } + + exists, err := vlt.HasSecret(key) + if err != nil || !exists { + t.Errorf("HasSecret() = (%v, %v), want (true, nil)", exists, err) + } + + keys, err := vlt.ListSecrets() + if err != nil { + t.Fatalf("ListSecrets() error = %v", err) + } + if len(keys) != 1 || keys[0] != key { + t.Errorf("ListSecrets() = %v, want [%s]", keys, key) + } +} + +// Writing migrates the entry to the current name and drops the old one, so the +// keyring does not end up holding two copies of the same secret. +func TestKeyringVault_WriteMigratesLegacyEntry(t *testing.T) { + keyring.MockInit() + + const ( + vaultID = "legacy-vault" + key = "api-key" + ) + + if err := keyring.Set(testKeyringService, vaultID+"-secret-"+key, "old-value"); err != nil { + t.Fatalf("failed to seed legacy secret: %v", err) + } + + vlt, _, err := vault.New(vaultID, + vault.WithProvider(vault.ProviderTypeKeyring), + vault.WithKeyringService(testKeyringService), + ) + if err != nil { + t.Fatalf("Failed to create keyring vault: %v", err) + } + defer vlt.Close() + + if err := vlt.SetSecret(key, vault.NewSecretValue([]byte("new-value"))); err != nil { + t.Fatalf("SetSecret() error = %v", err) + } + + // The legacy entry must be gone, not left behind holding a stale secret. + if _, err := keyring.Get(testKeyringService, vaultID+"-secret-"+key); !errors.Is(err, keyring.ErrNotFound) { + t.Errorf("legacy entry still present after write: err = %v", err) + } + + secret, err := vlt.GetSecret(key) + if err != nil { + t.Fatalf("GetSecret() error = %v", err) + } + if got := secret.PlainTextString(); got != "new-value" { + t.Errorf("GetSecret() = %q, want %q", got, "new-value") + } +} + +// Deleting must clear both names, or the legacy copy reappears on the next read. +func TestKeyringVault_DeleteRemovesLegacyEntry(t *testing.T) { + keyring.MockInit() + + const ( + vaultID = "legacy-vault" + key = "api-key" + ) + + if err := keyring.Set(testKeyringService, vaultID+"-secret-"+key, "old-value"); err != nil { + t.Fatalf("failed to seed legacy secret: %v", err) + } + + vlt, _, err := vault.New(vaultID, + vault.WithProvider(vault.ProviderTypeKeyring), + vault.WithKeyringService(testKeyringService), + ) + if err != nil { + t.Fatalf("Failed to create keyring vault: %v", err) + } + defer vlt.Close() + + if err := vlt.DeleteSecret(key); err != nil { + t.Fatalf("DeleteSecret() error = %v", err) + } + + exists, err := vlt.HasSecret(key) + if err != nil { + t.Fatalf("HasSecret() error = %v", err) + } + if exists { + t.Error("secret still readable after delete; the legacy entry survived") + } +}