From a17633e7e20316f6ba875792eeb6054790e89e9f Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 26 Jul 2026 14:42:40 -0400 Subject: [PATCH 1/7] fix(external)!: stop routing secret values through the shell SetSecret rendered the plaintext into a command string that is then parsed and executed by a shell, and the template engine does no quoting. An ordinary password was enough to break it: "p@$$w0rd" had $$ expanded to the PID and a *different* secret was stored, silently; "correct horse battery" word-split and stored "correct"; a value containing ";" ran arbitrary commands. Secrets now travel over stdin only. ExternalConfig.Validate rejects configs that still reference {{ value }} or {{ password }} in any command template, so the unsafe shape fails loudly at load instead of silently corrupting. Fixing that surfaced the reason it was never noticed: the input-template plumbing did not work. SetSecret, DeleteSecret and ListSecrets each guarded on their own InputTemplate but rendered Get's, Metadata gated on List's, and the value was never exposed to input templates at all -- so the shipped pass example's stdin-based set has never worked. Also in this pass: - execute() no longer merges stderr into stdout on success, which was concatenating backend warnings onto the secret value itself. - expandEnv returns a new map instead of mutating the shared config under a read lock (an unrecoverable "concurrent map writes" fault; guaranteed to recur with the pass example, whose "$(tty)" never stops looking expandable). - HasSecret delegates to an unlocked helper rather than re-entering RLock, which deadlocks if a writer arrives in between. - os.ExpandEnv no longer runs on command templates. It ran before the shell parsed them, destroying $VAR/${VAR}/$1/$?/$@, making a literal $ unwritable, and substituting before quoting. The interpreter already resolves $VAR from the same environment, correctly. - Metadata() returns an error instead of an empty struct, so a broken command, a timeout and "not configured" stay distinguishable. - Every method reports ErrVaultClosed after Close() instead of proceeding. - New optional not_found_pattern separates "absent" from "the backend is broken", which HasSecret could not previously tell apart. - SetExecutionFunc takes the mutex; the execution context is settable. - Timeout is parsed and validated at load, not at first use. BREAKING CHANGE: Provider.Metadata() now returns (Metadata, error), and external configs that interpolate the secret value into a command template are rejected. Tests: the previous mock ignored both cmd and input and returned the first value in a map, so nothing could observe a wrongly rendered template -- which is precisely why these shipped. Replaced with a capturing mock, plus a round-trip through the real shell asserting that values containing $(id), backticks, quotes, globs and newlines survive byte-exact and that an injection payload does not execute. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HtuDuGkSqTfAXSepf8b51p --- aes.go | 6 +- age.go | 6 +- config.go | 47 +++ errors.go | 2 + examples/main.go | 2 +- external.go | 317 +++++++++++------- external_test.go | 797 ++++++++++++++++++++++++++++---------------- keyring.go | 4 +- keyring_test.go | 6 +- unencrypted.go | 6 +- unencrypted_test.go | 6 +- vault.go | 6 +- vault_test.go | 2 +- 13 files changed, 763 insertions(+), 444 deletions(-) diff --git a/aes.go b/aes.go index 9ea13af..d4d98c0 100644 --- a/aes.go +++ b/aes.go @@ -192,14 +192,14 @@ func (v *AES256Vault) ID() string { return v.id } -func (v *AES256Vault) Metadata() Metadata { +func (v *AES256Vault) Metadata() (Metadata, error) { v.mu.RLock() defer v.mu.RUnlock() if v.state == nil { - return Metadata{} + return Metadata{}, ErrVaultClosed } - return v.state.Metadata + return v.state.Metadata, nil } func (v *AES256Vault) GetSecret(key string) (Secret, error) { diff --git a/age.go b/age.go index 43b842b..e876637 100644 --- a/age.go +++ b/age.go @@ -190,14 +190,14 @@ func (v *AgeVault) ID() string { return v.id } -func (v *AgeVault) Metadata() Metadata { +func (v *AgeVault) Metadata() (Metadata, error) { v.mu.RLock() defer v.mu.RUnlock() if v.state == nil { - return Metadata{} + return Metadata{}, ErrVaultClosed } - return v.state.Metadata + return v.state.Metadata, nil } func (v *AgeVault) GetSecret(key string) (Secret, error) { diff --git a/config.go b/config.go index b25eccc..b7afe57 100644 --- a/config.go +++ b/config.go @@ -5,6 +5,8 @@ import ( "fmt" "os" "path/filepath" + "regexp" + "time" ) type ProviderType string @@ -214,12 +216,57 @@ type ExternalConfig struct { // WorkingDir for command execution WorkingDir string `json:"working_dir,omitempty"` + + // NotFoundPattern is matched against a failing command's error output to tell + // "this secret does not exist" apart from a real failure (an expired session, + // a network error, a permissions problem). Without it, any non-zero exit is + // read as absence. Example: "ParameterNotFound". + NotFoundPattern string `json:"not_found_pattern,omitempty"` +} + +// timeoutDuration parses the configured timeout. An empty timeout means no limit. +func (c *ExternalConfig) timeoutDuration() (time.Duration, error) { + if c.Timeout == "" { + return 0, nil + } + return time.ParseDuration(c.Timeout) } +// secretValueRefs matches a template action referencing the secret value. These +// are rejected in command templates: the rendered command is executed by a shell +// and the template engine performs no quoting, so interpolating a secret there is +// a command-injection sink and silently corrupts values containing shell +// metacharacters. Secrets must travel over stdin via an input template. +var secretValueRefs = regexp.MustCompile(`{{[^}]*\b(value|password)\b[^}]*}}`) + func (c *ExternalConfig) Validate() error { if c.Get.CommandTemplate == "" || c.Set.CommandTemplate == "" { return fmt.Errorf("%w: get and set args template required for external vault", ErrInvalidConfig) } + + cmdTemplates := map[string]string{ + "get": c.Get.CommandTemplate, + "set": c.Set.CommandTemplate, + "delete": c.Delete.CommandTemplate, + "list": c.List.CommandTemplate, + "exists": c.Exists.CommandTemplate, + "metadata": c.Metadata.CommandTemplate, + } + for op, tmpl := range cmdTemplates { + if secretValueRefs.MatchString(tmpl) { + return fmt.Errorf( + "%w: the %s command template references the secret value, which is unsafe: "+ + "the command is run by a shell and the value is not quoted. "+ + `Move it to an input template instead, e.g. "input": "{{ value }}"`, + ErrInvalidConfig, op, + ) + } + } + + if _, err := c.timeoutDuration(); err != nil { + return fmt.Errorf("%w: invalid timeout duration %q: %w", ErrInvalidConfig, c.Timeout, err) + } + return nil } diff --git a/errors.go b/errors.go index 21cf2b2..b12d0c0 100644 --- a/errors.go +++ b/errors.go @@ -14,6 +14,8 @@ var ( ErrDecryptionFailed = errors.New("decryption failed") ErrInvalidRecipient = errors.New("invalid recipient") ErrPathNotSecure = errors.New("path is not secure") + ErrVaultClosed = errors.New("vault is closed") + ErrVaultCorrupt = errors.New("vault file is corrupt") ) type VaultPathError struct { diff --git a/examples/main.go b/examples/main.go index 89b8084..2053409 100644 --- a/examples/main.go +++ b/examples/main.go @@ -91,7 +91,7 @@ func main() { } fmt.Println("Getting vault metadata...") - metadata := provider.Metadata() + metadata, _ := provider.Metadata() fmt.Printf("Metadata: %s\n", metadata.RawData) fmt.Println("Cleaning up test secret...") diff --git a/external.go b/external.go index 5b57d73..d38f0a7 100644 --- a/external.go +++ b/external.go @@ -16,23 +16,33 @@ import ( ) type ExternalVaultProvider struct { - ctx context.Context mu sync.RWMutex id string execute func(ctx context.Context, cmd, input, dir string, envList []string) (string, error) + ctx context.Context + timeout time.Duration + closed bool + cfg *ExternalConfig } func NewExternalVaultProvider(cfg *Config) (*ExternalVaultProvider, error) { if cfg.External == nil { - return nil, fmt.Errorf("external configuration is required") + return nil, fmt.Errorf("%w: external configuration is required", ErrInvalidConfig) } + if err := cfg.Validate(); err != nil { + return nil, err + } + + // Timeout is validated by ExternalConfig.Validate, so this cannot fail here. + timeout, _ := cfg.External.timeoutDuration() vault := &ExternalVaultProvider{ ctx: context.Background(), id: cfg.ID, cfg: cfg.External, + timeout: timeout, execute: execute, } @@ -43,16 +53,41 @@ func (v *ExternalVaultProvider) ID() string { return v.id } +// SetContext replaces the context used for command execution, allowing callers to +// cancel in-flight operations. The Provider interface does not thread a context +// through its methods, so this is the supported way to make external commands +// cancellable. +func (v *ExternalVaultProvider) SetContext(ctx context.Context) { + if ctx == nil { + ctx = context.Background() + } + v.mu.Lock() + defer v.mu.Unlock() + v.ctx = ctx +} + func (v *ExternalVaultProvider) GetSecret(key string) (Secret, error) { v.mu.RLock() defer v.mu.RUnlock() + return v.getSecretLocked(key) +} + +// getSecretLocked implements GetSecret and assumes the caller already holds at +// least a read lock. HasSecret needs to delegate here rather than to GetSecret: +// sync.RWMutex does not support recursive read locking, so a writer arriving +// between the two RLock calls would deadlock both. +func (v *ExternalVaultProvider) getSecretLocked(key string) (Secret, error) { if err := ValidateSecretKey(key); err != nil { return nil, err } + if v.closed { + return nil, ErrVaultClosed + } + if v.cfg.Get.CommandTemplate == "" { - return nil, fmt.Errorf("get operation not configured") + return nil, fmt.Errorf("%w: get operation not configured", ErrInvalidConfig) } cmd, err := v.renderCmdTemplate(v.cfg.Get.CommandTemplate, key) @@ -94,26 +129,38 @@ func (v *ExternalVaultProvider) SetSecret(key string, value Secret) error { return err } + if v.closed { + return ErrVaultClosed + } + if v.cfg.Set.CommandTemplate == "" { - return fmt.Errorf("set operation not configured") + return fmt.Errorf("%w: set operation not configured", ErrInvalidConfig) } - cmd, err := v.renderCmdTemplateWithValue(v.cfg.Set.CommandTemplate, key, value.PlainTextString()) + // The secret value is deliberately not available to the command template. The + // rendered command is parsed and run by a shell, and the template engine does + // no quoting, so interpolating a secret there is a command-injection sink and + // silently corrupts any value containing shell metacharacters. Values travel + // over stdin via the set input template instead. ExternalConfig.Validate + // rejects configs that still reference {{ value }} in the set command. + cmd, err := v.renderCmdTemplate(v.cfg.Set.CommandTemplate, key) if err != nil { return fmt.Errorf("failed to render set cmd: %w", err) } var input string if v.cfg.Set.InputTemplate != "" { - input, err = v.renderInputTemplate(v.cfg.Get.InputTemplate, key) + input, err = v.renderInputTemplateWithValue(v.cfg.Set.InputTemplate, key, value.PlainTextString()) if err != nil { return fmt.Errorf("failed to render input template: %w", err) } } - out, err := v.executeCommand(cmd, input) - if err != nil { - return fmt.Errorf("failed to set secret: %w stdErr: %s", err, out) + // The error from executeCommand already carries stderr. Do not add the command + // output here: a backend that echoes the failing command back would put the + // secret value into the returned error string. + if _, err := v.executeCommand(cmd, input); err != nil { + return fmt.Errorf("failed to set secret: %w", err) } return nil @@ -127,8 +174,12 @@ func (v *ExternalVaultProvider) DeleteSecret(key string) error { return err } + if v.closed { + return ErrVaultClosed + } + if v.cfg.Delete.CommandTemplate == "" { - return fmt.Errorf("delete operation not configured") + return fmt.Errorf("%w: delete operation not configured", ErrInvalidConfig) } cmd, err := v.renderCmdTemplate(v.cfg.Delete.CommandTemplate, key) @@ -138,7 +189,7 @@ func (v *ExternalVaultProvider) DeleteSecret(key string) error { var input string if v.cfg.Delete.InputTemplate != "" { - input, err = v.renderInputTemplate(v.cfg.Get.InputTemplate, key) + input, err = v.renderInputTemplate(v.cfg.Delete.InputTemplate, key) if err != nil { return fmt.Errorf("failed to render input template: %w", err) } @@ -155,8 +206,12 @@ func (v *ExternalVaultProvider) ListSecrets() ([]string, error) { v.mu.RLock() defer v.mu.RUnlock() + if v.closed { + return nil, ErrVaultClosed + } + if v.cfg.List.CommandTemplate == "" { - return nil, fmt.Errorf("list operation not configured") + return nil, fmt.Errorf("%w: list operation not configured", ErrInvalidConfig) } cmd, err := v.renderCmdTemplate(v.cfg.List.CommandTemplate, "") @@ -166,7 +221,7 @@ func (v *ExternalVaultProvider) ListSecrets() ([]string, error) { var input string if v.cfg.List.InputTemplate != "" { - input, err = v.renderInputTemplate(v.cfg.Get.InputTemplate, "") + input, err = v.renderInputTemplate(v.cfg.List.InputTemplate, "") if err != nil { return nil, fmt.Errorf("failed to render input template: %w", err) } @@ -196,7 +251,7 @@ func (v *ExternalVaultProvider) ListSecrets() ([]string, error) { sep = "\n" } secrets := strings.Split(secretsList, sep) - var result []string + result := make([]string, 0, len(secrets)) for _, secret := range secrets { secret = strings.TrimSpace(secret) if secret != "" { @@ -215,94 +270,127 @@ func (v *ExternalVaultProvider) HasSecret(key string) (bool, error) { return false, err } + if v.closed { + return false, ErrVaultClosed + } + if v.cfg.Exists.CommandTemplate != "" { - cmd, err := v.renderCmdTemplate(v.cfg.Exists.CommandTemplate, key) - if err != nil { - return false, fmt.Errorf("failed to render exists cmd: %w", err) - } + return v.hasSecretViaExistsCmd(key) + } - var input string - if v.cfg.Exists.InputTemplate != "" { - input, err = v.renderInputTemplate(v.cfg.Exists.InputTemplate, key) - if err != nil { - return false, fmt.Errorf("failed to render input template: %w", err) - } + if _, err := v.getSecretLocked(key); err != nil { + if v.isNotFoundErr(err) { + return false, nil } - - _, err = v.executeCommand(cmd, input) - // typically, exists commands return non-zero exit code if secret doesn't exist - return err == nil, nil + return false, err } + return true, nil +} - _, err := v.GetSecret(key) +func (v *ExternalVaultProvider) hasSecretViaExistsCmd(key string) (bool, error) { + cmd, err := v.renderCmdTemplate(v.cfg.Exists.CommandTemplate, key) if err != nil { - if strings.Contains(err.Error(), "not found") || - strings.Contains(err.Error(), "not exist") || - strings.Contains(err.Error(), "not in") { - return false, nil + return false, fmt.Errorf("failed to render exists cmd: %w", err) + } + + var input string + if v.cfg.Exists.InputTemplate != "" { + input, err = v.renderInputTemplate(v.cfg.Exists.InputTemplate, key) + if err != nil { + return false, fmt.Errorf("failed to render input template: %w", err) } + } + + if _, err = v.executeCommand(cmd, input); err == nil { + return true, nil + } + + // 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. + if v.cfg.NotFoundPattern != "" && !strings.Contains(err.Error(), v.cfg.NotFoundPattern) { return false, err } - return true, nil + return false, nil +} + +func (v *ExternalVaultProvider) isNotFoundErr(err error) bool { + if v.cfg.NotFoundPattern != "" { + return strings.Contains(err.Error(), v.cfg.NotFoundPattern) + } + msg := err.Error() + return strings.Contains(msg, "not found") || + strings.Contains(msg, "not exist") || + strings.Contains(msg, "not in") } func (v *ExternalVaultProvider) Close() error { + v.mu.Lock() + defer v.mu.Unlock() + + v.closed = true return nil } func (v *ExternalVaultProvider) SetExecutionFunc( fn func(ctx context.Context, cmd, input, dir string, envList []string) (string, error), ) { + v.mu.Lock() + defer v.mu.Unlock() v.execute = fn } -func (v *ExternalVaultProvider) Metadata() Metadata { +func (v *ExternalVaultProvider) Metadata() (Metadata, error) { v.mu.RLock() defer v.mu.RUnlock() + if v.closed { + return Metadata{}, ErrVaultClosed + } + if v.cfg.Metadata.CommandTemplate == "" { - return Metadata{} + return Metadata{}, nil } cmd, err := v.renderCmdTemplate(v.cfg.Metadata.CommandTemplate, "") if err != nil { - return Metadata{} + return Metadata{}, fmt.Errorf("failed to render metadata cmd: %w", err) } + var input string - if v.cfg.List.InputTemplate != "" { + if v.cfg.Metadata.InputTemplate != "" { input, err = v.renderInputTemplate(v.cfg.Metadata.InputTemplate, "") if err != nil { - return Metadata{} + return Metadata{}, fmt.Errorf("failed to render input template: %w", err) } } output, err := v.executeCommand(cmd, input) if err != nil { - return Metadata{} + return Metadata{}, fmt.Errorf("failed to read metadata: %w", err) } var metadataOutput string if v.cfg.Metadata.OutputTemplate != "" { metadataOutput, err = v.renderOutputTemplate(v.cfg.Metadata.OutputTemplate, output) if err != nil { - return Metadata{} + return Metadata{}, fmt.Errorf("failed to parse metadata output: %w", err) } } else { metadataOutput = strings.TrimSpace(output) } - return Metadata{RawData: metadataOutput} + return Metadata{RawData: metadataOutput}, nil } func (v *ExternalVaultProvider) executeCommand(cmd, input string) (string, error) { ctx := v.ctx - if v.cfg.Timeout != "" { + if ctx == nil { + ctx = context.Background() + } + if v.timeout > 0 { var cancel context.CancelFunc - dur, parseErr := time.ParseDuration(v.cfg.Timeout) - if parseErr != nil { - return "", fmt.Errorf("invalid timeout duration: %w", parseErr) - } - ctx, cancel = context.WithTimeout(v.ctx, dur) + ctx, cancel = context.WithTimeout(ctx, v.timeout) defer cancel() } @@ -315,103 +403,71 @@ func (v *ExternalVaultProvider) executeCommand(cmd, input string) (string, error } func (v *ExternalVaultProvider) environmentToSlice() []string { - var envSlice []string - for key, value := range expandEnv(v.cfg.Environment) { + expanded := expandEnv(v.cfg.Environment) + envSlice := make([]string, 0, len(expanded)) + for key, value := range expanded { envSlice = append(envSlice, fmt.Sprintf("%s=%s", key, value)) } return envSlice } -func (v *ExternalVaultProvider) renderCmdTemplate(template, key string) (string, error) { - data := map[string]interface{}{ - "env": expandEnv(v.cfg.Environment), - "key": key, - "ref": key, - "id": key, - "name": key, - "template": template, +// templateData is the variable set shared by the command and input templates. +// The secret value is intentionally absent; only renderInputTemplateWithValue +// adds it. +func (v *ExternalVaultProvider) templateData(key string) map[string]interface{} { + return map[string]interface{}{ + "env": expandEnv(v.cfg.Environment), + "key": key, + "ref": key, + "id": key, + "name": key, } +} - template = os.ExpandEnv(template) - tmpl := expression.NewTemplate(fmt.Sprintf("%s-args-template", v.id), data) - err := tmpl.Parse(template) - if err != nil { - return "", fmt.Errorf("parsing args template: %w", err) +func (v *ExternalVaultProvider) render(name, template string, data map[string]interface{}) (string, error) { + // os.ExpandEnv is deliberately not applied here. It runs before the shell + // parses the command, so it destroys $VAR, ${VAR}, $1, $? and $@, makes a + // literal $ unwritable, and applies substitution before quoting -- the wrong + // order for injection safety. execute() already appends the configured + // environment to os.Environ(), so the interpreter resolves $VAR itself, with + // correct quoting semantics and with cfg.Environment actually in scope. + tmpl := expression.NewTemplate(fmt.Sprintf("%s-%s-template", v.id, name), data) + if err := tmpl.Parse(template); err != nil { + return "", fmt.Errorf("parsing %s template: %w", name, err) } result, err := tmpl.ExecuteToString() if err != nil { - return "", fmt.Errorf("evaluating args template: %w", err) + return "", fmt.Errorf("evaluating %s template: %w", name, err) } return result, nil } -func (v *ExternalVaultProvider) renderCmdTemplateWithValue(template, key, value string) (string, error) { - data := map[string]interface{}{ - "env": expandEnv(v.cfg.Environment), - "key": key, - "ref": key, - "id": key, - "name": key, - "value": value, - "password": value, - "template": template, - } - - template = os.ExpandEnv(template) - tmpl := expression.NewTemplate(fmt.Sprintf("%s-args-template", v.id), data) - err := tmpl.Parse(template) - if err != nil { - return "", fmt.Errorf("parsing args template: %w", err) - } - - result, err := tmpl.ExecuteToString() - if err != nil { - return "", fmt.Errorf("evaluating args template: %w", err) - } - return result, nil +func (v *ExternalVaultProvider) renderCmdTemplate(template, key string) (string, error) { + return v.render("args", template, v.templateData(key)) } func (v *ExternalVaultProvider) renderInputTemplate(template, input string) (string, error) { - data := map[string]interface{}{ - "env": expandEnv(v.cfg.Environment), - "input": input, - "template": template, - } - - template = os.ExpandEnv(template) - tmpl := expression.NewTemplate(fmt.Sprintf("%s-input-template", v.id), data) - err := tmpl.Parse(template) - if err != nil { - return "", fmt.Errorf("parsing input template: %w", err) - } + return v.renderInputTemplateWithValue(template, input, "") +} - result, err := tmpl.ExecuteToString() - if err != nil { - return "", fmt.Errorf("evaluating input template: %w", err) - } - return result, nil +func (v *ExternalVaultProvider) renderInputTemplateWithValue(template, input, value string) (string, error) { + data := v.templateData(input) + data["input"] = input + data["value"] = value + data["password"] = value + return v.render("input", template, data) } func (v *ExternalVaultProvider) renderOutputTemplate(template, output string) (string, error) { data := map[string]interface{}{ - "env": expandEnv(v.cfg.Environment), - "output": output, - "template": template, - } - - template = os.ExpandEnv(template) - tmpl := expression.NewTemplate(fmt.Sprintf("%s-output-template", v.id), data) - err := tmpl.Parse(template) - if err != nil { - return "", fmt.Errorf("parsing output template: %w", err) + "env": expandEnv(v.cfg.Environment), + "output": output, } - result, err := tmpl.ExecuteToString() - if err != nil { - return "", fmt.Errorf("evaluating output template: %w", err) - } - return result, nil + // Unlike command templates, output templates are never handed to a shell, so + // environment expansion here is safe and preserved for compatibility. + return v.render("output", os.ExpandEnv(template), data) } func execute(ctx context.Context, cmd, input, dir string, envList []string) (string, error) { @@ -455,18 +511,25 @@ func execute(ctx context.Context, cmd, input, dir string, envList []string) (str } return stdErrBuffer.String(), fmt.Errorf("encountered an error executing command - %w", err) } - output := stdOutBuffer.String() - if stderr := stdErrBuffer.String(); stderr != "" { - output += "\n" + stderr - } - return strings.TrimSpace(output), nil + + // 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 } +// expandEnv returns a new map with environment references expanded. It must not +// mutate the input: the caller's map is the shared provider config, and the read +// paths (GetSecret, ListSecrets, HasSecret, Metadata) hold only a read lock, so +// writing to it concurrently is an unrecoverable "concurrent map writes" fault. func expandEnv(env map[string]string) map[string]string { + out := make(map[string]string, len(env)) for k, v := range env { if strings.Contains(v, "$") || strings.Contains(v, "{") { - env[k] = os.ExpandEnv(v) + out[k] = os.ExpandEnv(v) + } else { + out[k] = v } } - return env + return out } diff --git a/external_test.go b/external_test.go index 54a967c..3319273 100644 --- a/external_test.go +++ b/external_test.go @@ -2,13 +2,61 @@ package vault_test import ( "context" + "errors" "fmt" + "os" + "path/filepath" "strings" + "sync" "testing" "github.com/flowexec/vault" ) +const testSecretValue = "s3cr3t" + +// validExternalConfig returns a config that satisfies ExternalConfig.Validate. +// Get and Set command templates are mandatory, so every fixture needs them even +// when the test only exercises another operation. +func validExternalConfig() *vault.ExternalConfig { + return &vault.ExternalConfig{ + Get: vault.CommandConfig{CommandTemplate: "vault kv get -format=json {{key}}"}, + Set: vault.CommandConfig{CommandTemplate: "vault kv put {{key}}"}, + } +} + +func newTestProvider(t *testing.T, cfg *vault.ExternalConfig) *vault.ExternalVaultProvider { + t.Helper() + provider, err := vault.NewExternalVaultProvider(&vault.Config{ + ID: "test-vault", + Type: vault.ProviderTypeExternal, + External: cfg, + }) + if err != nil { + t.Fatalf("Failed to create provider: %v", err) + } + return provider +} + +// execCapture records what the provider actually asked the shell to run. The +// older mockCommandContext observes neither cmd nor input, so it cannot catch a +// provider rendering the wrong template or leaking a secret into a command. +type execCapture struct { + cmd, input, dir string + env []string + calls int +} + +func capturingExec(c *execCapture, out string, err error) func( + context.Context, string, string, string, []string, +) (string, error) { + return func(_ context.Context, cmd, input, dir string, envList []string) (string, error) { + c.cmd, c.input, c.dir, c.env = cmd, input, dir, envList + c.calls++ + return out, err + } +} + func TestNewExternalVaultProvider(t *testing.T) { tests := []struct { name string @@ -18,16 +66,9 @@ func TestNewExternalVaultProvider(t *testing.T) { { name: "valid config", config: &vault.Config{ - ID: "test-vault", - Type: vault.ProviderTypeExternal, - External: &vault.ExternalConfig{ - Get: vault.CommandConfig{ - CommandTemplate: "vault kv get -format=json {{key}}", - }, - Set: vault.CommandConfig{ - CommandTemplate: "vault kv put {{key}} value={{value}}", - }, - }, + ID: "test-vault", + Type: vault.ProviderTypeExternal, + External: validExternalConfig(), }, wantErr: false, }, @@ -39,6 +80,28 @@ func TestNewExternalVaultProvider(t *testing.T) { }, wantErr: true, }, + { + name: "missing get and set templates", + config: &vault.Config{ + ID: "test-vault", + Type: vault.ProviderTypeExternal, + External: &vault.ExternalConfig{}, + }, + wantErr: true, + }, + { + name: "invalid timeout", + config: &vault.Config{ + ID: "test-vault", + Type: vault.ProviderTypeExternal, + External: func() *vault.ExternalConfig { + c := validExternalConfig() + c.Timeout = "not-a-duration" + return c + }(), + }, + wantErr: true, + }, } for _, tt := range tests { @@ -48,240 +111,271 @@ func TestNewExternalVaultProvider(t *testing.T) { t.Errorf("NewExternalVaultProvider() error = %v, wantErr %v", err, tt.wantErr) return } - if !tt.wantErr && provider == nil { - t.Error("NewExternalVaultProvider() returned nil provider") + if tt.wantErr { + return + } + if provider == nil { + t.Fatal("NewExternalVaultProvider() returned nil provider") } - if !tt.wantErr && provider.ID() != tt.config.ID { + if provider.ID() != tt.config.ID { t.Errorf("NewExternalVaultProvider() ID = %v, want %v", provider.ID(), tt.config.ID) } }) } } -func TestExternalVaultProvider_GetSecret(t *testing.T) { - config := &vault.Config{ - ID: "test-vault", - Type: vault.ProviderTypeExternal, - External: &vault.ExternalConfig{ - Get: vault.CommandConfig{ - CommandTemplate: "vault kv get -format=json {{key}}", - }, - }, +// A secret interpolated into a command template is a command-injection sink: the +// rendered string is parsed and run by a shell and the template engine does no +// quoting. Configs that try must be rejected at load, not silently accepted. +func TestConfigRejectsSecretValueInCommandTemplates(t *testing.T) { + for _, tmpl := range []string{ + "vault kv put {{key}} value={{value}}", + "vault kv put {{key}} value={{ value }}", + "vault kv put {{key}} pw={{password}}", + } { + cfg := validExternalConfig() + cfg.Set.CommandTemplate = tmpl + + _, err := vault.NewExternalVaultProvider(&vault.Config{ + ID: "test-vault", Type: vault.ProviderTypeExternal, External: cfg, + }) + if err == nil { + t.Errorf("template %q was accepted, want rejection", tmpl) + continue + } + if !errors.Is(err, vault.ErrInvalidConfig) { + t.Errorf("template %q: error = %v, want ErrInvalidConfig", tmpl, err) + } } +} - provider, err := vault.NewExternalVaultProvider(config) - if err != nil { - t.Fatalf("Failed to create provider: %v", err) - } +func TestSetSecret_ValueTravelsOverStdinNotTheCommand(t *testing.T) { + cfg := validExternalConfig() + cfg.Set.CommandTemplate = "store {{key}}" + cfg.Set.InputTemplate = "{{ value }}" + // A Get input template that must NOT be used for the set operation. + cfg.Get.InputTemplate = "WRONG-TEMPLATE" - tests := []struct { - name string - key string - mockOutputs map[string]string - mockErrors map[string]error - wantSecret string - wantErr bool - errorContains string - }{ - { - name: "successful get", - key: "test-key", - mockOutputs: map[string]string{ - "vault kv get -format=json test-key": "secret-value", - }, - wantSecret: "secret-value", - wantErr: false, - }, - { - name: "command fails", - key: "test-key", - mockErrors: map[string]error{ - "vault kv get -format=json test-key": fmt.Errorf("command failed"), - }, - wantErr: true, - errorContains: "failed to get secret", - }, - { - name: "invalid key", - key: "", - wantErr: true, - errorContains: "invalid secret key", - }, + provider := newTestProvider(t, cfg) + rec := &execCapture{} + provider.SetExecutionFunc(capturingExec(rec, "", nil)) + + if err := provider.SetSecret("test-key", vault.NewSecretValue([]byte(testSecretValue))); err != nil { + t.Fatalf("SetSecret() error = %v", err) } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - testProvider := provider - if tt.name == "get operation not configured" { - testConfig := &vault.Config{ - ID: "test-vault", - Type: vault.ProviderTypeExternal, - External: &vault.ExternalConfig{}, - } - var err error - testProvider, err = vault.NewExternalVaultProvider(testConfig) - if err != nil { - t.Fatalf("Failed to create test provider: %v", err) - } - } + if rec.input != testSecretValue { + t.Errorf("stdin = %q, want %q (set input template was not rendered)", rec.input, testSecretValue) + } + if strings.Contains(rec.cmd, testSecretValue) { + t.Errorf("secret leaked into the command string: %q", rec.cmd) + } + if rec.cmd != "store test-key" { + t.Errorf("cmd = %q, want %q", rec.cmd, "store test-key") + } +} - testProvider.SetExecutionFunc(mockCommandContext(tt.mockOutputs, tt.mockErrors)) +// A value containing shell metacharacters must round-trip byte-exact. Before the +// fix, "p@$$w0rd" had $$ expanded to the PID and a different secret was stored. +func TestSetSecret_ValueWithShellMetacharactersIsUntouched(t *testing.T) { + for _, value := range []string{ + `p@$$w0rd`, + `correct horse battery`, + `hunter2; echo pwned`, + "back`tick`", + `quote'and"quote`, + "multi\nline", + } { + cfg := validExternalConfig() + cfg.Set.CommandTemplate = "store {{key}}" + cfg.Set.InputTemplate = "{{ value }}" + + provider := newTestProvider(t, cfg) + rec := &execCapture{} + provider.SetExecutionFunc(capturingExec(rec, "", nil)) + + if err := provider.SetSecret("k", vault.NewSecretValue([]byte(value))); err != nil { + t.Fatalf("SetSecret(%q) error = %v", value, err) + } + if rec.input != value { + t.Errorf("stdin = %q, want %q", rec.input, value) + } + if strings.Contains(rec.cmd, value) { + t.Errorf("value %q leaked into command %q", value, rec.cmd) + } + } +} - secret, err := testProvider.GetSecret(tt.key) - if (err != nil) != tt.wantErr { - t.Errorf("GetSecret() error = %v, wantErr %v", err, tt.wantErr) - return - } +func TestSetSecret_InputTemplateSeesTheKey(t *testing.T) { + cfg := validExternalConfig() + cfg.Set.CommandTemplate = "store" + cfg.Set.InputTemplate = "{{ key }}:{{ value }}" - if err != nil && tt.errorContains != "" { - if !strings.Contains(err.Error(), tt.errorContains) { - t.Errorf("GetSecret() error = %v, want error containing %v", err, tt.errorContains) - } - return - } + provider := newTestProvider(t, cfg) + rec := &execCapture{} + provider.SetExecutionFunc(capturingExec(rec, "", nil)) - if !tt.wantErr && secret.PlainTextString() != tt.wantSecret { - t.Errorf("GetSecret() secret = %v, want %v", secret.PlainTextString(), tt.wantSecret) - } - }) + if err := provider.SetSecret("my-key", vault.NewSecretValue([]byte(testSecretValue))); err != nil { + t.Fatalf("SetSecret() error = %v", err) + } + if want := "my-key:" + testSecretValue; rec.input != want { + t.Errorf("stdin = %q, want %q", rec.input, want) } } -func TestExternalVaultProvider_SetSecret(t *testing.T) { - config := &vault.Config{ - ID: "test-vault", - Type: vault.ProviderTypeExternal, - External: &vault.ExternalConfig{ - Set: vault.CommandConfig{ - CommandTemplate: "vault kv put {{key}} value={{value}}", - }, - }, - } +// Each operation must render its own input template. All of these previously +// rendered Get's template instead. +func TestOperationsRenderTheirOwnInputTemplate(t *testing.T) { + t.Run("delete", func(t *testing.T) { + cfg := validExternalConfig() + cfg.Get.InputTemplate = "WRONG" + cfg.Delete.CommandTemplate = "rm {{key}}" + cfg.Delete.InputTemplate = "delete:{{ input }}" + + provider := newTestProvider(t, cfg) + rec := &execCapture{} + provider.SetExecutionFunc(capturingExec(rec, "", nil)) + + if err := provider.DeleteSecret("k"); err != nil { + t.Fatalf("DeleteSecret() error = %v", err) + } + if rec.input != "delete:k" { + t.Errorf("stdin = %q, want %q", rec.input, "delete:k") + } + }) - provider, err := vault.NewExternalVaultProvider(config) - if err != nil { - t.Fatalf("Failed to create provider: %v", err) + t.Run("list", func(t *testing.T) { + cfg := validExternalConfig() + cfg.Get.InputTemplate = "WRONG" + cfg.List.CommandTemplate = "ls" + cfg.List.InputTemplate = "list-input" + + provider := newTestProvider(t, cfg) + rec := &execCapture{} + provider.SetExecutionFunc(capturingExec(rec, "a\nb", nil)) + + if _, err := provider.ListSecrets(); err != nil { + t.Fatalf("ListSecrets() error = %v", err) + } + if rec.input != "list-input" { + t.Errorf("stdin = %q, want %q", rec.input, "list-input") + } + }) + + // Metadata previously gated on List's input template, so a configured + // metadata input was ignored unless list.input happened to be set too. + t.Run("metadata", func(t *testing.T) { + cfg := validExternalConfig() + cfg.Metadata.CommandTemplate = "status" + cfg.Metadata.InputTemplate = "meta-input" + + provider := newTestProvider(t, cfg) + rec := &execCapture{} + provider.SetExecutionFunc(capturingExec(rec, "ok", nil)) + + if _, err := provider.Metadata(); err != nil { + t.Fatalf("Metadata() error = %v", err) + } + if rec.input != "meta-input" { + t.Errorf("stdin = %q, want %q", rec.input, "meta-input") + } + }) +} + +// Configs written against the old behaviour set only Get.InputTemplate; that +// must keep working. +func TestGetInputTemplateBackCompat(t *testing.T) { + cfg := validExternalConfig() + cfg.Get.InputTemplate = "{{ input }}" + + provider := newTestProvider(t, cfg) + rec := &execCapture{} + provider.SetExecutionFunc(capturingExec(rec, "value", nil)) + + if _, err := provider.GetSecret("my-key"); err != nil { + t.Fatalf("GetSecret() error = %v", err) } + if rec.input != "my-key" { + t.Errorf("stdin = %q, want %q", rec.input, "my-key") + } +} +func TestExternalVaultProvider_GetSecret(t *testing.T) { tests := []struct { name string key string - value string - mockOutputs map[string]string - mockErrors map[string]error + out string + execErr error + wantSecret string wantErr bool errorContains string }{ + {name: "successful get", key: "test-key", out: "secret-value", wantSecret: "secret-value"}, { - name: "successful set", - key: "test-key", - value: "test-value", - mockOutputs: map[string]string{ - "vault kv put test-key value=test-value": "success", - }, - wantErr: false, - }, - { - name: "command fails", - key: "test-key", - value: "test-value", - mockErrors: map[string]error{ - "vault kv put test-key value=test-value": fmt.Errorf("command failed"), - }, - wantErr: true, - errorContains: "failed to set secret", + name: "command fails", key: "test-key", execErr: fmt.Errorf("command failed"), + wantErr: true, errorContains: "failed to get secret", }, + {name: "invalid key", key: "", wantErr: true, errorContains: "invalid secret key"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - testProvider := provider - testProvider.SetExecutionFunc(mockCommandContext(tt.mockOutputs, tt.mockErrors)) + provider := newTestProvider(t, validExternalConfig()) + provider.SetExecutionFunc(capturingExec(&execCapture{}, tt.out, tt.execErr)) - secret := vault.NewSecretValue([]byte(tt.value)) - err := testProvider.SetSecret(tt.key, secret) + secret, err := provider.GetSecret(tt.key) if (err != nil) != tt.wantErr { - t.Errorf("SetSecret() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("GetSecret() error = %v, wantErr %v", err, tt.wantErr) return } - - if err != nil && tt.errorContains != "" { - if !strings.Contains(err.Error(), tt.errorContains) { - t.Errorf("SetSecret() error = %v, want error containing %v", err, tt.errorContains) + if err != nil { + if tt.errorContains != "" && !strings.Contains(err.Error(), tt.errorContains) { + t.Errorf("GetSecret() error = %v, want error containing %v", err, tt.errorContains) } + return + } + if secret.PlainTextString() != tt.wantSecret { + t.Errorf("GetSecret() secret = %v, want %v", secret.PlainTextString(), tt.wantSecret) } }) } } func TestExternalVaultProvider_ListSecrets(t *testing.T) { - config := &vault.Config{ - ID: "test-vault", - Type: vault.ProviderTypeExternal, - External: &vault.ExternalConfig{ - List: vault.CommandConfig{ - CommandTemplate: "vault kv list", - }, - }, - } - - provider, err := vault.NewExternalVaultProvider(config) - if err != nil { - t.Fatalf("Failed to create provider: %v", err) - } - tests := []struct { name string - mockOutputs map[string]string - mockErrors map[string]error + out string + execErr error wantSecrets []string wantErr bool }{ - { - name: "successful list", - mockOutputs: map[string]string{ - "vault kv list": "secret1\nsecret2\nsecret3", - }, - wantSecrets: []string{"secret1", "secret2", "secret3"}, - wantErr: false, - }, - { - name: "empty list", - mockOutputs: map[string]string{ - "vault kv list": "", - }, - wantSecrets: []string{}, - wantErr: false, - }, - { - name: "command fails", - mockErrors: map[string]error{ - "vault kv list": fmt.Errorf("command failed"), - }, - wantErr: true, - }, + {name: "successful list", out: "secret1\nsecret2\nsecret3", wantSecrets: []string{"secret1", "secret2", "secret3"}}, + {name: "empty list", out: "", wantSecrets: []string{}}, + {name: "command fails", execErr: fmt.Errorf("command failed"), wantErr: true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - testProvider := provider - testProvider.SetExecutionFunc(mockCommandContext(tt.mockOutputs, tt.mockErrors)) + cfg := validExternalConfig() + cfg.List.CommandTemplate = "vault kv list" + + provider := newTestProvider(t, cfg) + provider.SetExecutionFunc(capturingExec(&execCapture{}, tt.out, tt.execErr)) - secrets, err := testProvider.ListSecrets() + secrets, err := provider.ListSecrets() if (err != nil) != tt.wantErr { t.Errorf("ListSecrets() error = %v, wantErr %v", err, tt.wantErr) return } - - if !tt.wantErr { - if len(secrets) != len(tt.wantSecrets) { - t.Errorf("ListSecrets() returned %d secrets, want %d", len(secrets), len(tt.wantSecrets)) - return - } - for i, secret := range secrets { - if secret != tt.wantSecrets[i] { - t.Errorf("ListSecrets() secret[%d] = %v, want %v", i, secret, tt.wantSecrets[i]) - } + if tt.wantErr { + return + } + if len(secrets) != len(tt.wantSecrets) { + t.Fatalf("ListSecrets() returned %d secrets, want %d", len(secrets), len(tt.wantSecrets)) + } + for i, secret := range secrets { + if secret != tt.wantSecrets[i] { + t.Errorf("ListSecrets() secret[%d] = %v, want %v", i, secret, tt.wantSecrets[i]) } } }) @@ -289,60 +383,28 @@ func TestExternalVaultProvider_ListSecrets(t *testing.T) { } func TestExternalVaultProvider_HasSecret(t *testing.T) { - config := &vault.Config{ - ID: "test-vault", - Type: vault.ProviderTypeExternal, - External: &vault.ExternalConfig{ - Exists: vault.CommandConfig{ - CommandTemplate: "vault kv get {{key}}", - }, - }, - } - - provider, err := vault.NewExternalVaultProvider(config) - if err != nil { - t.Fatalf("Failed to create provider: %v", err) - } - tests := []struct { - name string - key string - mockOutputs map[string]string - mockErrors map[string]error - wantExists bool - wantErr bool + name string + key string + execErr error + wantExists bool }{ - { - name: "secret exists", - key: "existing-key", - mockOutputs: map[string]string{ - "vault kv get existing-key": "some-value", - }, - wantExists: true, - wantErr: false, - }, - { - name: "secret does not exist", - key: "nonexistent-key", - mockErrors: map[string]error{ - "vault kv get nonexistent-key": fmt.Errorf("not found"), - }, - wantExists: false, - wantErr: false, - }, + {name: "secret exists", key: "existing-key", wantExists: true}, + {name: "secret does not exist", key: "nonexistent-key", execErr: fmt.Errorf("not found"), wantExists: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - testProvider := provider - testProvider.SetExecutionFunc(mockCommandContext(tt.mockOutputs, tt.mockErrors)) + cfg := validExternalConfig() + cfg.Exists.CommandTemplate = "vault kv get {{key}}" - exists, err := testProvider.HasSecret(tt.key) - if (err != nil) != tt.wantErr { - t.Errorf("HasSecret() error = %v, wantErr %v", err, tt.wantErr) - return - } + provider := newTestProvider(t, cfg) + provider.SetExecutionFunc(capturingExec(&execCapture{}, "some-value", tt.execErr)) + exists, err := provider.HasSecret(tt.key) + if err != nil { + t.Fatalf("HasSecret() error = %v", err) + } if exists != tt.wantExists { t.Errorf("HasSecret() = %v, want %v", exists, tt.wantExists) } @@ -350,83 +412,226 @@ func TestExternalVaultProvider_HasSecret(t *testing.T) { } } +// NotFoundPattern separates "absent" from "the backend is broken". Without it, +// an expired session reports the secret as simply missing. +func TestHasSecret_NotFoundPatternDistinguishesRealFailures(t *testing.T) { + cfg := validExternalConfig() + cfg.Exists.CommandTemplate = "check {{key}}" + cfg.NotFoundPattern = "ParameterNotFound" + + t.Run("absent", func(t *testing.T) { + provider := newTestProvider(t, cfg) + provider.SetExecutionFunc(capturingExec(&execCapture{}, "", fmt.Errorf("ParameterNotFound: nope"))) + + exists, err := provider.HasSecret("k") + if err != nil { + t.Fatalf("HasSecret() error = %v", err) + } + if exists { + t.Error("HasSecret() = true, want false") + } + }) + + t.Run("real failure surfaces", func(t *testing.T) { + provider := newTestProvider(t, cfg) + provider.SetExecutionFunc(capturingExec(&execCapture{}, "", fmt.Errorf("ExpiredToken: session expired"))) + + if _, err := provider.HasSecret("k"); err == nil { + t.Error("HasSecret() error = nil, want the expired-session error to surface") + } + }) +} + +// HasSecret with no exists command delegates to the get path. Doing that through +// the exported GetSecret would take a second read lock, which deadlocks if a +// writer arrives in between, because sync.RWMutex is not reentrant. +func TestHasSecret_WithoutExistsCommandDoesNotDeadlock(t *testing.T) { + provider := newTestProvider(t, validExternalConfig()) + provider.SetExecutionFunc(capturingExec(&execCapture{}, "value", nil)) + + done := make(chan struct{}) + go func() { + defer close(done) + for i := 0; i < 50; i++ { + _, _ = provider.HasSecret("k") + } + }() + // Contend with writers so a queued writer sits between the two read locks. + for i := 0; i < 50; i++ { + _ = provider.SetSecret("k", vault.NewSecretValue([]byte("v"))) + } + <-done +} + func TestExternalVaultProvider_Metadata(t *testing.T) { - tests := []struct { - name string - config *vault.ExternalConfig - mockOutputs map[string]string - mockErrors map[string]error - wantRawData string - }{ - { - name: "successful metadata retrieval", - config: &vault.ExternalConfig{ - Metadata: vault.CommandConfig{ - CommandTemplate: "vault status", - }, - }, - mockOutputs: map[string]string{ - "vault status": "vault is healthy", - }, - wantRawData: "vault is healthy", - }, - { - name: "no metadata command configured", - config: &vault.ExternalConfig{}, - wantRawData: "", - }, - { - name: "metadata command fails", - config: &vault.ExternalConfig{ - Metadata: vault.CommandConfig{ - CommandTemplate: "vault status", - }, - }, - mockErrors: map[string]error{ - "vault status": fmt.Errorf("command failed"), - }, - wantRawData: "", - }, + t.Run("successful retrieval", func(t *testing.T) { + cfg := validExternalConfig() + cfg.Metadata.CommandTemplate = "vault status" + + provider := newTestProvider(t, cfg) + provider.SetExecutionFunc(capturingExec(&execCapture{}, "vault is healthy", nil)) + + metadata, err := provider.Metadata() + if err != nil { + t.Fatalf("Metadata() error = %v", err) + } + if metadata.RawData != "vault is healthy" { + t.Errorf("Metadata().RawData = %v, want %v", metadata.RawData, "vault is healthy") + } + }) + + t.Run("not configured is not an error", func(t *testing.T) { + provider := newTestProvider(t, validExternalConfig()) + metadata, err := provider.Metadata() + if err != nil { + t.Fatalf("Metadata() error = %v", err) + } + if metadata.RawData != "" { + t.Errorf("Metadata().RawData = %v, want empty", metadata.RawData) + } + }) + + // Previously every failure path returned an empty Metadata{}, so a broken + // command, a timeout and "not configured" were indistinguishable. + t.Run("command failure surfaces as an error", func(t *testing.T) { + cfg := validExternalConfig() + cfg.Metadata.CommandTemplate = "vault status" + + provider := newTestProvider(t, cfg) + provider.SetExecutionFunc(capturingExec(&execCapture{}, "", fmt.Errorf("command failed"))) + + if _, err := provider.Metadata(); err == nil { + t.Error("Metadata() error = nil, want the command failure to surface") + } + }) +} + +func TestClosedProviderReturnsErrVaultClosed(t *testing.T) { + cfg := validExternalConfig() + cfg.List.CommandTemplate = "ls" + cfg.Metadata.CommandTemplate = "status" + + provider := newTestProvider(t, cfg) + provider.SetExecutionFunc(capturingExec(&execCapture{}, "ok", nil)) + if err := provider.Close(); err != nil { + t.Fatalf("Close() error = %v", err) } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - config := &vault.Config{ - ID: "test-vault", - Type: vault.ProviderTypeExternal, - External: tt.config, - } + if _, err := provider.GetSecret("k"); !errors.Is(err, vault.ErrVaultClosed) { + t.Errorf("GetSecret() after Close = %v, want ErrVaultClosed", err) + } + if err := provider.SetSecret("k", vault.NewSecretValue([]byte("v"))); !errors.Is(err, vault.ErrVaultClosed) { + t.Errorf("SetSecret() after Close = %v, want ErrVaultClosed", err) + } + if err := provider.DeleteSecret("k"); !errors.Is(err, vault.ErrVaultClosed) { + t.Errorf("DeleteSecret() after Close = %v, want ErrVaultClosed", err) + } + if _, err := provider.ListSecrets(); !errors.Is(err, vault.ErrVaultClosed) { + t.Errorf("ListSecrets() after Close = %v, want ErrVaultClosed", err) + } + if _, err := provider.HasSecret("k"); !errors.Is(err, vault.ErrVaultClosed) { + t.Errorf("HasSecret() after Close = %v, want ErrVaultClosed", err) + } + if _, err := provider.Metadata(); !errors.Is(err, vault.ErrVaultClosed) { + t.Errorf("Metadata() after Close = %v, want ErrVaultClosed", err) + } +} - provider, err := vault.NewExternalVaultProvider(config) - if err != nil { - t.Fatalf("Failed to create provider: %v", err) - } +// Exercises the real execute(), not a mock. stderr merged into stdout on success +// concatenates backend warnings onto the secret value itself. +func TestExecute_StderrIsNotMergedIntoTheSecret(t *testing.T) { + cfg := validExternalConfig() + cfg.Get.CommandTemplate = "printf 'the-secret'; printf 'gpg: WARNING: unsafe permissions' 1>&2" - testProvider := provider - testProvider.SetExecutionFunc(mockCommandContext(tt.mockOutputs, tt.mockErrors)) + provider := newTestProvider(t, cfg) - metadata := testProvider.Metadata() - if metadata.RawData != tt.wantRawData { - t.Errorf("Metadata().RawData = %v, want %v", metadata.RawData, tt.wantRawData) - } - }) + secret, err := provider.GetSecret("k") + if err != nil { + t.Fatalf("GetSecret() error = %v", err) + } + if got := secret.PlainTextString(); got != "the-secret" { + t.Errorf("GetSecret() = %q, want %q (stderr leaked into the value)", got, "the-secret") } } -// mockCommandContext creates mock commands for testing -func mockCommandContext( - outputs map[string]string, - errors map[string]error, -) func(ctx context.Context, cmd, input, dir string, envList []string) (string, error) { - return func(ctx context.Context, cmd, input, dir string, envList []string) (string, error) { - for _, output := range outputs { - return output, nil +// End-to-end through the real shell, not a mock: a secret full of shell +// metacharacters must survive a set/get round trip byte-exact. This is the +// behaviour the injection fix exists to guarantee. +func TestExternalProvider_RoundTripThroughRealShell(t *testing.T) { + dir := t.TempDir() + store := filepath.Join(dir, "secret.txt") + + cfg := &vault.ExternalConfig{ + Get: vault.CommandConfig{CommandTemplate: "cat '" + store + "'"}, + Set: vault.CommandConfig{ + CommandTemplate: "cat > '" + store + "'", + InputTemplate: "{{ value }}", + }, + } + provider := newTestProvider(t, cfg) + + for _, value := range []string{ + `p@$$w0rd`, + `correct horse battery`, + `hunter2; echo pwned > ` + filepath.Join(dir, "injected"), + "back`echo tick`", + `quote'and"quote`, + `glob*star?`, + `$(id)`, + `${HOME}`, + } { + if err := provider.SetSecret("k", vault.NewSecretValue([]byte(value))); err != nil { + t.Fatalf("SetSecret(%q) error = %v", value, err) } - for range errors { - return "", fmt.Errorf("mock error") + secret, err := provider.GetSecret("k") + if err != nil { + t.Fatalf("GetSecret() after setting %q error = %v", value, err) + } + if got := secret.PlainTextString(); got != value { + t.Errorf("round trip: got %q, want %q", got, value) } + } + + // The injection attempt above must not have run. + if _, err := os.Stat(filepath.Join(dir, "injected")); !os.IsNotExist(err) { + t.Error("command injection succeeded: the payload created a file") + } +} + +// expandEnv used to mutate the shared config map while callers held only a read +// lock, which is an unrecoverable "concurrent map writes" fault. Run with -race. +func TestConcurrentGetSecretDoesNotRaceOnEnvironment(t *testing.T) { + cfg := validExternalConfig() + cfg.Environment = map[string]string{ + "HOME_REF": "$HOME", + "LITERAL": "$(tty)", + "PLAIN": "value", + } + + provider := newTestProvider(t, cfg) + // Reads are concurrent by design, so the exec func must be stateless here -- + // a shared execCapture would itself race and mask what we are testing. + provider.SetExecutionFunc(func( + _ context.Context, _, _, _ string, _ []string, + ) (string, error) { + return "value", nil + }) + + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(1) + go func() { + defer wg.Done() + _, _ = provider.GetSecret("k") + _, _ = provider.ListSecrets() + _, _ = provider.Metadata() + }() + } + wg.Wait() - return "mock", nil + // The config map itself must be unchanged: expansion returns a new map. + if got := cfg.Environment["LITERAL"]; got != "$(tty)" { + t.Errorf("config Environment was mutated: LITERAL = %q, want %q", got, "$(tty)") } } diff --git a/keyring.go b/keyring.go index 47ed228..300e8ed 100644 --- a/keyring.go +++ b/keyring.go @@ -154,11 +154,11 @@ func (v *KeyringVault) ID() string { return v.id } -func (v *KeyringVault) Metadata() Metadata { +func (v *KeyringVault) Metadata() (Metadata, error) { v.mu.RLock() defer v.mu.RUnlock() - return v.metadata + return v.metadata, nil } func (v *KeyringVault) GetSecret(key string) (Secret, error) { diff --git a/keyring_test.go b/keyring_test.go index 6e8d1b6..3cafc8c 100644 --- a/keyring_test.go +++ b/keyring_test.go @@ -253,7 +253,7 @@ func TestKeyringVault_Persistence(t *testing.T) { } // Verify metadata is accessible - metadata := vault2.Metadata() + metadata, _ := vault2.Metadata() if metadata.Created.IsZero() { t.Error("Expected creation time to be set") } @@ -273,7 +273,7 @@ func TestKeyringVault_Metadata(t *testing.T) { } defer vlt.Close() - metadata := vlt.Metadata() + metadata, _ := vlt.Metadata() if metadata.Created.IsZero() { t.Error("Expected creation time to be set") } @@ -289,7 +289,7 @@ func TestKeyringVault_Metadata(t *testing.T) { t.Fatalf("Failed to set secret: %v", err) } - newMetadata := vlt.Metadata() + newMetadata, _ := vlt.Metadata() if !newMetadata.LastModified.After(oldModified) { t.Error("Expected last modified time to be updated after setting secret") } diff --git a/unencrypted.go b/unencrypted.go index 681b741..78d8f37 100644 --- a/unencrypted.go +++ b/unencrypted.go @@ -137,14 +137,14 @@ func (v *UnencryptedVault) ID() string { return v.id } -func (v *UnencryptedVault) Metadata() Metadata { +func (v *UnencryptedVault) Metadata() (Metadata, error) { v.mu.RLock() defer v.mu.RUnlock() if v.state == nil { - return Metadata{} + return Metadata{}, ErrVaultClosed } - return v.state.Metadata + return v.state.Metadata, nil } func (v *UnencryptedVault) GetSecret(key string) (Secret, error) { diff --git a/unencrypted_test.go b/unencrypted_test.go index a7c88c6..b080073 100644 --- a/unencrypted_test.go +++ b/unencrypted_test.go @@ -243,7 +243,7 @@ func TestUnencryptedVault_Persistence(t *testing.T) { } // Verify metadata is preserved - metadata := vault2.Metadata() + metadata, _ := vault2.Metadata() if metadata.Created.IsZero() { t.Error("Expected creation time to be preserved") } @@ -264,7 +264,7 @@ func TestUnencryptedVault_Metadata(t *testing.T) { } defer vlt.Close() - metadata := vlt.Metadata() + metadata, _ := vlt.Metadata() if metadata.Created.IsZero() { t.Error("Expected creation time to be set") } @@ -281,7 +281,7 @@ func TestUnencryptedVault_Metadata(t *testing.T) { t.Fatalf("Failed to set secret: %v", err) } - newMetadata := vlt.Metadata() + newMetadata, _ := vlt.Metadata() if !newMetadata.LastModified.After(oldModified) { t.Error("Expected last modified time to be updated after setting secret") } diff --git a/vault.go b/vault.go index a681699..4504c1f 100644 --- a/vault.go +++ b/vault.go @@ -14,8 +14,10 @@ type Provider interface { // ID returns a unique identifier for this vault instance ID() string - // Metadata returns vault metadata such as creation time - Metadata() Metadata + // Metadata returns vault metadata such as creation time. It returns an error + // rather than a zero value so that a failing backend command, a timeout, and + // "no metadata configured" stay distinguishable. + Metadata() (Metadata, error) Close() error } diff --git a/vault_test.go b/vault_test.go index e1dba44..8a3293d 100644 --- a/vault_test.go +++ b/vault_test.go @@ -276,7 +276,7 @@ func testPersistence(t *testing.T, v vault.Provider, provider vault.ProviderType } // Test that Metadata is preserved - metadata := newVault.Metadata() + metadata, _ := newVault.Metadata() if metadata.Created.IsZero() { t.Error("Metadata creation time should not be zero") } From 3ee8ac6762966e08f24cde446f0bd2648adc8299 Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 26 Jul 2026 15:28:22 -0400 Subject: [PATCH 2/7] fix(local)!: stop losing secrets on empty files and concurrent writes Three ways the local providers (aes, age, unencrypted) could destroy a vault, all silent: 1. A zero-length vault file was read as "no vault here". load() returned nil leaving state nil, the constructor took that for a new vault, and init() immediately save()d over it. Every secret gone, success reported. An existing but empty file is now ErrVaultCorrupt, and the file is left untouched for the user to restore. 2. The temp file had a fixed ".tmp" name written with os.WriteFile, so two concurrent savers truncated each other's partial write, and a symlink planted at that predictable path was followed -- with the 0600 mode silently not applied to the pre-existing target. Now os.CreateTemp (random name, O_EXCL), explicit chmod, fsync before the rename, and an fsync of the directory after it. 3. Nothing synchronized writers across processes. load() ran once at construction and every save rewrote the whole file from that snapshot, so two `flow secret set` runs silently lost one of the two updates. The per-instance RWMutex never helped: it does not span two providers in one process, let alone two processes. Mutations now take an advisory lock on a .lock sidecar and re-load inside it before applying and saving. Verified rather than assumed: with the lock disabled, the new concurrency test collapses 8 concurrent writes to 1 surviving secret. Alongside those, in the same files: - Every accessor guards a nil state instead of dereferencing it. GetSecret, SetSecret, DeleteSecret, ListSecrets, HasSecret and the age recipient methods all panicked after Close(); only Metadata() checked. - save() reports ErrVaultClosed instead of silently doing nothing. - ValidateSecretKey is applied on every entry point, not just SetSecret. - Vault directories are created 0700 rather than group-readable 0750. - Read failures that are not "not exist" (EACCES, I/O errors) no longer masquerade as ErrVaultNotFound, which callers read as "make a new one". - AgeVault refuses to create a vault, or remove a recipient, that would leave no recipient matching your own identity -- previously a single mistyped public key produced a permanently unreadable vault, and RemoveRecipient guarded only the *last* recipient, not yours. - RemoveRecipient commits to state only after the new set parses, so a failure cannot leave memory inconsistent with disk. BREAKING CHANGE: an existing empty vault file is now an error instead of being silently reinitialized, and closed vaults return ErrVaultClosed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HtuDuGkSqTfAXSepf8b51p --- aes.go | 101 +++++++++++++-------- age.go | 205 ++++++++++++++++++++++++++++++------------ go.mod | 4 +- go.sum | 13 ++- storage.go | 147 ++++++++++++++++++++++++++++++ storage_test.go | 233 ++++++++++++++++++++++++++++++++++++++++++++++++ unencrypted.go | 96 ++++++++++++-------- 7 files changed, 662 insertions(+), 137 deletions(-) create mode 100644 storage.go create mode 100644 storage_test.go diff --git a/aes.go b/aes.go index d4d98c0..89a3dd5 100644 --- a/aes.go +++ b/aes.go @@ -1,10 +1,9 @@ package vault import ( - "errors" "fmt" - "os" "path/filepath" + "sort" "sync" "time" @@ -119,20 +118,16 @@ func (v *AES256Vault) init() error { Secrets: make(map[string]string), } - return v.save() + return withVaultLock(v.fullPath, v.save) } // load retrieves the AESState from the vault file, decrypts it, and unmarshals it into an AESState struct. func (v *AES256Vault) load() error { - data, err := os.ReadFile(filepath.Clean(v.fullPath)) + data, exists, err := readVaultFile(v.fullPath) if err != nil { - if errors.Is(err, os.ErrNotExist) { - return nil - } - return fmt.Errorf("%w: failed to read vault file %s: %w", ErrVaultNotFound, v.fullPath, err) + return err } - - if len(data) == 0 { + if !exists { return nil } @@ -154,7 +149,7 @@ func (v *AES256Vault) load() error { // save encrypts and writes the vault contents to disk func (v *AES256Vault) save() error { if v.state == nil { - return nil + return ErrVaultClosed } if v.dek == "" { @@ -171,21 +166,25 @@ func (v *AES256Vault) save() error { return fmt.Errorf("failed to encrypt vault state: %w", err) } - // write to the file atomically - if err := os.MkdirAll(filepath.Dir(v.fullPath), 0750); err != nil { - return fmt.Errorf("failed to create vault directory: %w", err) - } - tempFile := v.fullPath + ".tmp" - if err := os.WriteFile(tempFile, []byte(encryptedDataStr), 0600); err != nil { - return fmt.Errorf("failed to write temp vault file: %w", err) - } - - if err := os.Rename(tempFile, v.fullPath); err != nil { - _ = os.Remove(tempFile) - return fmt.Errorf("failed to move vault file: %w", err) - } + return writeVaultFileAtomic(v.fullPath, []byte(encryptedDataStr)) +} - return nil +// mutate runs a read-modify-write cycle under the cross-process vault lock. +// +// Reloading inside the lock is the point: in-memory state is a snapshot taken +// when the provider was constructed, and every save rewrites the whole file. +// Writing that snapshot back without refreshing silently discards whatever +// another process stored in the meantime. +func (v *AES256Vault) mutate(apply func() error) error { + return withVaultLock(v.fullPath, func() error { + if err := v.load(); err != nil { + return err + } + if err := apply(); err != nil { + return err + } + return v.save() + }) } func (v *AES256Vault) ID() string { @@ -206,6 +205,13 @@ func (v *AES256Vault) GetSecret(key string) (Secret, error) { v.mu.RLock() defer v.mu.RUnlock() + if err := ValidateSecretKey(key); err != nil { + return nil, err + } + if v.state == nil { + return nil, ErrVaultClosed + } + value, exists := v.state.Secrets[key] if !exists { return nil, ErrSecretNotFound @@ -221,36 +227,54 @@ func (v *AES256Vault) SetSecret(key string, secret Secret) error { if err := ValidateSecretKey(key); err != nil { return err } - - if v.state.Secrets == nil { - v.state.Secrets = make(map[string]string) + if v.state == nil { + return ErrVaultClosed } - v.state.Secrets[key] = secret.PlainTextString() - return v.save() + return v.mutate(func() error { + if v.state.Secrets == nil { + v.state.Secrets = make(map[string]string) + } + v.state.Secrets[key] = secret.PlainTextString() + return nil + }) } func (v *AES256Vault) DeleteSecret(key string) error { v.mu.Lock() defer v.mu.Unlock() - _, exists := v.state.Secrets[key] - if !exists { - return ErrSecretNotFound + if err := ValidateSecretKey(key); err != nil { + return err + } + if v.state == nil { + return ErrVaultClosed } - delete(v.state.Secrets, key) - return v.save() + // The existence check runs inside mutate, after the reload, so it sees the + // current on-disk contents rather than a stale snapshot. + return v.mutate(func() error { + if _, exists := v.state.Secrets[key]; !exists { + return ErrSecretNotFound + } + delete(v.state.Secrets, key) + return nil + }) } func (v *AES256Vault) ListSecrets() ([]string, error) { v.mu.RLock() defer v.mu.RUnlock() + if v.state == nil { + return nil, ErrVaultClosed + } + keys := make([]string, 0, len(v.state.Secrets)) for k := range v.state.Secrets { keys = append(keys, k) } + sort.Strings(keys) return keys, nil } @@ -258,6 +282,13 @@ func (v *AES256Vault) HasSecret(key string) (bool, error) { v.mu.RLock() defer v.mu.RUnlock() + if err := ValidateSecretKey(key); err != nil { + return false, err + } + if v.state == nil { + return false, ErrVaultClosed + } + _, exists := v.state.Secrets[key] return exists, nil } diff --git a/age.go b/age.go index e876637..99ffe45 100644 --- a/age.go +++ b/age.go @@ -4,8 +4,8 @@ import ( "bytes" "encoding/json" "fmt" - "os" "path/filepath" + "sort" "sync" "time" @@ -101,24 +101,31 @@ func (v *AgeVault) init() error { return fmt.Errorf("no recipients available for encryption, please add at least one recipient") } + // Creating a vault encrypted only to someone else's key produces a file + // that cannot be opened again -- including by the process that just wrote + // it. Catch that here rather than at the next load. + if !v.canDecryptWith(v.state.Recipients) { + return fmt.Errorf( + "%w: none of the configured recipients match your identity, "+ + "so the vault would be unreadable as soon as it is written", + ErrInvalidRecipient, + ) + } + if err := v.parseRecipients(); err != nil { return fmt.Errorf("failed to parse recipients: %w", err) } - return v.save() + return withVaultLock(v.fullPath, v.save) } // load reads the vault file and decrypts its contents func (v *AgeVault) load() error { - data, err := os.ReadFile(v.fullPath) + data, exists, err := readVaultFile(v.fullPath) if err != nil { - if os.IsNotExist(err) { - return nil - } - return fmt.Errorf("failed to read vault file: %w", err) + return err } - - if len(data) == 0 { + if !exists { return nil } @@ -143,7 +150,7 @@ func (v *AgeVault) load() error { // save encrypts and writes the vault contents to disk func (v *AgeVault) save() error { if v.state == nil { - return nil + return ErrVaultClosed } if len(v.recipients) == 0 { @@ -169,21 +176,21 @@ func (v *AgeVault) save() error { return fmt.Errorf("failed to finalize encryption: %w", err) } - // write to the file atomically - if err := os.MkdirAll(filepath.Dir(v.fullPath), 0750); err != nil { - return fmt.Errorf("failed to create vault directory: %w", err) - } - tempFile := v.fullPath + ".tmp" - if err := os.WriteFile(tempFile, buf.Bytes(), 0600); err != nil { - return fmt.Errorf("failed to write temp vault file: %w", err) - } - - if err := os.Rename(tempFile, v.fullPath); err != nil { - _ = os.Remove(tempFile) - return fmt.Errorf("failed to move vault file: %w", err) - } + return writeVaultFileAtomic(v.fullPath, buf.Bytes()) +} - return nil +// mutate runs a read-modify-write cycle under the cross-process vault lock. +// See AES256Vault.mutate for why the reload inside the lock is required. +func (v *AgeVault) mutate(apply func() error) error { + return withVaultLock(v.fullPath, func() error { + if err := v.load(); err != nil { + return err + } + if err := apply(); err != nil { + return err + } + return v.save() + }) } func (v *AgeVault) ID() string { @@ -204,6 +211,13 @@ func (v *AgeVault) GetSecret(key string) (Secret, error) { v.mu.RLock() defer v.mu.RUnlock() + if err := ValidateSecretKey(key); err != nil { + return nil, err + } + if v.state == nil { + return nil, ErrVaultClosed + } + value, exists := v.state.Secrets[key] if !exists { return nil, ErrSecretNotFound @@ -219,36 +233,54 @@ func (v *AgeVault) SetSecret(key string, value Secret) error { if err := ValidateSecretKey(key); err != nil { return err } - - if v.state.Secrets == nil { - v.state.Secrets = make(map[string]string) + if v.state == nil { + return ErrVaultClosed } - v.state.Secrets[key] = value.PlainTextString() - return v.save() + return v.mutate(func() error { + if v.state.Secrets == nil { + v.state.Secrets = make(map[string]string) + } + v.state.Secrets[key] = value.PlainTextString() + return nil + }) } func (v *AgeVault) DeleteSecret(key string) error { v.mu.Lock() defer v.mu.Unlock() - _, exists := v.state.Secrets[key] - if !exists { - return ErrSecretNotFound + if err := ValidateSecretKey(key); err != nil { + return err + } + if v.state == nil { + return ErrVaultClosed } - delete(v.state.Secrets, key) - return v.save() + // The existence check runs inside mutate, after the reload, so it sees the + // current on-disk contents rather than a stale snapshot. + return v.mutate(func() error { + if _, exists := v.state.Secrets[key]; !exists { + return ErrSecretNotFound + } + delete(v.state.Secrets, key) + return nil + }) } func (v *AgeVault) ListSecrets() ([]string, error) { v.mu.RLock() defer v.mu.RUnlock() + if v.state == nil { + return nil, ErrVaultClosed + } + keys := make([]string, 0, len(v.state.Secrets)) for k := range v.state.Secrets { keys = append(keys, k) } + sort.Strings(keys) return keys, nil } @@ -256,10 +288,41 @@ func (v *AgeVault) HasSecret(key string) (bool, error) { v.mu.RLock() defer v.mu.RUnlock() + if err := ValidateSecretKey(key); err != nil { + return false, err + } + if v.state == nil { + return false, ErrVaultClosed + } + _, exists := v.state.Secrets[key] return exists, nil } +// canDecryptWith reports whether any resolved identity appears in recipients. +// Encrypting to a set that excludes your own key produces a vault you can never +// reopen, and a single mistyped public key is enough to do it. +func (v *AgeVault) canDecryptWith(recipients []string) bool { + own := make(map[string]struct{}, len(v.identities)) + for _, id := range v.identities { + if x, ok := id.(*age.X25519Identity); ok { + own[x.Recipient().String()] = struct{}{} + } + } + if len(own) == 0 { + // An identity type we cannot map to a recipient string; do not block on + // a check we are unable to perform. + return true + } + + for _, r := range recipients { + if _, ok := own[r]; ok { + return true + } + } + return false +} + func (v *AgeVault) Close() error { // clear the secret state from memory v.mu.Lock() @@ -276,49 +339,77 @@ func (v *AgeVault) AddRecipient(publicKey string) error { v.mu.Lock() defer v.mu.Unlock() - if err := v.addRecipientToState(publicKey); err != nil { - return err - } - if err := v.parseRecipients(); err != nil { - return fmt.Errorf("failed to parse recipients: %w", err) + if v.state == nil { + return ErrVaultClosed } - return v.save() + return v.mutate(func() error { + if err := v.addRecipientToState(publicKey); err != nil { + return err + } + return v.parseRecipients() + }) } func (v *AgeVault) RemoveRecipient(publicKey string) error { v.mu.Lock() defer v.mu.Unlock() - // Don't allow removing the last recipient - if len(v.state.Recipients) <= 1 { - return fmt.Errorf("cannot remove the last recipient - at least one recipient is required for encryption") + if v.state == nil { + return ErrVaultClosed } - found := false - for i, rec := range v.state.Recipients { - if rec == publicKey { - v.state.Recipients = append(v.state.Recipients[:i], v.state.Recipients[i+1:]...) - found = true - break + return v.mutate(func() error { + // Don't allow removing the last recipient + if len(v.state.Recipients) <= 1 { + return fmt.Errorf("cannot remove the last recipient - at least one recipient is required for encryption") } - } - if !found { - return fmt.Errorf("recipient %s not found", publicKey) - } + remaining := make([]string, 0, len(v.state.Recipients)) + found := false + for _, rec := range v.state.Recipients { + if rec == publicKey { + found = true + continue + } + remaining = append(remaining, rec) + } - if err := v.parseRecipients(); err != nil { - return fmt.Errorf("failed to parse recipients: %w", err) - } + if !found { + return fmt.Errorf("recipient %s not found", publicKey) + } + + // Refusing here is the difference between "you removed a colleague" and + // "you locked yourself out permanently". The previous check only + // guarded the *last* recipient, not your own. + if !v.canDecryptWith(remaining) { + return fmt.Errorf( + "%w: removing %s would leave no recipient matching your identity, "+ + "making the vault permanently unreadable", + ErrInvalidRecipient, publicKey, + ) + } - return v.save() + // Commit to state only once the new set is known to parse, so a failure + // cannot leave in-memory recipients inconsistent with what is on disk. + previous := v.state.Recipients + v.state.Recipients = remaining + if err := v.parseRecipients(); err != nil { + v.state.Recipients = previous + return fmt.Errorf("failed to parse recipients: %w", err) + } + return nil + }) } func (v *AgeVault) ListRecipients() ([]string, error) { v.mu.RLock() defer v.mu.RUnlock() + if v.state == nil { + return nil, ErrVaultClosed + } + recipients := make([]string, len(v.state.Recipients)) copy(recipients, v.state.Recipients) // prevent modification of internal state return recipients, nil diff --git a/go.mod b/go.mod index f36a8e9..381637c 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.24.0 require ( filippo.io/age v1.2.1 + github.com/gofrs/flock v0.13.0 github.com/jahvon/expression v0.1.3 github.com/zalando/go-keyring v0.2.6 golang.org/x/crypto v0.41.0 @@ -16,7 +17,6 @@ require ( github.com/danieljoos/wincred v1.2.2 // indirect github.com/expr-lang/expr v1.17.5 // indirect github.com/godbus/dbus/v5 v5.1.0 // indirect - golang.org/x/sys v0.35.0 // indirect + golang.org/x/sys v0.37.0 // indirect golang.org/x/term v0.34.0 // indirect - gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect ) diff --git a/go.sum b/go.sum index 704d259..c2a4056 100644 --- a/go.sum +++ b/go.sum @@ -16,17 +16,16 @@ github.com/go-quicktest/qt v1.101.0 h1:O1K29Txy5P2OK0dGo59b7b0LR6wKfIhttaAhHUyn7 github.com/go-quicktest/qt v1.101.0/go.mod h1:14Bz/f7NwaXPtdYEgzsx46kqSxVwTbzVZsDC26tQJow= github.com/godbus/dbus/v5 v5.1.0 h1:4KLkAxT3aOY8Li4FRJe/KvhoNFFxo0m6fNuFUO8QJUk= github.com/godbus/dbus/v5 v5.1.0/go.mod h1:xhWf0FNVPg57R7Z0UbKHbJfkEywrmjJnf7w5xrFpKfA= +github.com/gofrs/flock v0.13.0 h1:95JolYOvGMqeH31+FC7D2+uULf6mG61mEZ/A8dRYMzw= +github.com/gofrs/flock v0.13.0/go.mod h1:jxeyy9R1auM5S6JYDBhDt+E2TCo7DkratH4Pgi8P+Z0= github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 h1:El6M4kTTCOh6aBiKaUGG7oYTSPP8MxqL4YI3kZKwcP4= github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510/go.mod h1:pupxD2MaaD3pAXIBCelhxNneeOaAeabZDe5s4K6zSpQ= github.com/jahvon/expression v0.1.3 h1:ZOH6tj1zK9h+M+eDy/n1Fz6XL/vA3lCV6bzHe6YWAvY= github.com/jahvon/expression v0.1.3/go.mod h1:4HJB2k+epW5vFeptF6ILlXbFRQ+CuCyCSO4QdnGT3AE= -github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= -github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= -github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= @@ -35,14 +34,14 @@ github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0t github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc= github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY= github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= -github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= -github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= +github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= +github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/zalando/go-keyring v0.2.6 h1:r7Yc3+H+Ux0+M72zacZoItR3UDxeWfKTcabvkI8ua9s= github.com/zalando/go-keyring v0.2.6/go.mod h1:2TCrxYrbUNYfNS/Kgy/LSrkSQzZ5UPVH85RwfczwvcI= golang.org/x/crypto v0.41.0 h1:WKYxWedPGCTVVl5+WHSSrOBT0O8lx32+zxmHxijgXp4= golang.org/x/crypto v0.41.0/go.mod h1:pO5AFd7FA68rFak7rOAGVuygIISepHftHnr8dr6+sUc= -golang.org/x/sys v0.35.0 h1:vz1N37gP5bs89s7He8XuIYXpyY0+QlsKmzipCbUtyxI= -golang.org/x/sys v0.35.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= +golang.org/x/sys v0.37.0 h1:fdNQudmxPjkdUTPnLn5mdQv7Zwvbvpaxqs831goi9kQ= +golang.org/x/sys v0.37.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= golang.org/x/term v0.34.0 h1:O/2T7POpk0ZZ7MAzMeWFSg6S5IpWd/RXDlM9hgM3DR4= golang.org/x/term v0.34.0/go.mod h1:5jC53AEywhIVebHgPVeg0mj8OD3VO9OzclacVrqpaAw= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/storage.go b/storage.go new file mode 100644 index 0000000..7119784 --- /dev/null +++ b/storage.go @@ -0,0 +1,147 @@ +package vault + +import ( + "context" + "fmt" + "os" + "path/filepath" + "time" + + "github.com/gofrs/flock" +) + +const ( + // vaultLockTimeout bounds how long a process waits for another process to + // finish its read-modify-write cycle before giving up. + vaultLockTimeout = 10 * time.Second + // vaultLockRetry is how often the lock is re-attempted while waiting. + vaultLockRetry = 50 * time.Millisecond + // vaultDirMode keeps the vault directory owner-only. Secrets live here, so + // the group-readable 0750 the providers used before is too permissive. + vaultDirMode = 0700 + // vaultFileMode keeps the vault file owner-only. + vaultFileMode = 0600 +) + +// readVaultFile reads a vault file from disk. +// +// The boolean reports whether the file exists. Only a genuinely absent file +// permits a caller to initialize a fresh vault; an existing but zero-length file +// is reported as corrupt. Treating "empty" as "absent" is how a truncated vault +// used to be silently reinitialized and then overwritten by the constructor, +// destroying every secret without reporting an error. +func readVaultFile(path string) ([]byte, bool, error) { + data, err := os.ReadFile(filepath.Clean(path)) + if err != nil { + if os.IsNotExist(err) { + return nil, false, nil + } + // Anything else -- a permissions problem, an I/O error -- must not be + // reported as "not found", which callers reasonably read as "create a + // new one". + return nil, false, fmt.Errorf("failed to read vault file %s: %w", path, err) + } + + if len(data) == 0 { + return nil, true, fmt.Errorf( + "%w: vault file %s is empty; refusing to overwrite it. "+ + "Restore it from a backup, or delete it to start a new vault", + ErrVaultCorrupt, path, + ) + } + + return data, true, nil +} + +// writeVaultFileAtomic writes data to path via a temp file and a rename. +func writeVaultFileAtomic(path string, data []byte) error { + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, vaultDirMode); err != nil { + return fmt.Errorf("failed to create vault directory: %w", err) + } + + // A random name via os.CreateTemp (which uses O_EXCL) rather than a fixed + // ".tmp". The fixed name let two concurrent savers truncate each + // other's partial write, and let a pre-planted symlink at that predictable + // path redirect the write -- os.WriteFile follows symlinks and does not + // apply the 0600 mode to an already-existing target. + tmp, err := os.CreateTemp(dir, ".vault-*") + if err != nil { + return fmt.Errorf("failed to create temp vault file: %w", err) + } + tmpName := tmp.Name() + + discard := func() { + _ = tmp.Close() + _ = os.Remove(tmpName) + } + + if err := tmp.Chmod(vaultFileMode); err != nil { + discard() + return fmt.Errorf("failed to set vault file permissions: %w", err) + } + if _, err := tmp.Write(data); err != nil { + discard() + return fmt.Errorf("failed to write temp vault file: %w", err) + } + // Flush to stable storage before the rename. Without this, a crash can leave + // the renamed file present but zero-length -- exactly the corrupt state + // readVaultFile now has to reject. + if err := tmp.Sync(); err != nil { + discard() + return fmt.Errorf("failed to flush vault file: %w", err) + } + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpName) + return fmt.Errorf("failed to close temp vault file: %w", err) + } + + if err := os.Rename(tmpName, path); err != nil { + _ = os.Remove(tmpName) + return fmt.Errorf("failed to move vault file into place: %w", err) + } + + syncDir(dir) + return nil +} + +// syncDir flushes a directory entry so a completed rename survives a crash. +// Best effort: not all platforms or filesystems support fsync on a directory +// (Windows in particular), and by this point the data is already in place, so +// there is nothing a caller could do with the error. +func syncDir(dir string) { + d, err := os.Open(filepath.Clean(dir)) + if err != nil { + return + } + defer func() { _ = d.Close() }() + _ = d.Sync() +} + +// withVaultLock runs fn while holding an exclusive advisory lock on the vault. +// +// The per-instance RWMutex only serializes goroutines sharing one provider +// value. It does nothing about a second provider in the same process, or -- the +// common case -- a second `flow secret set` running concurrently. Because every +// save rewrites the whole file from an in-memory snapshot, two unsynchronized +// writers silently lose one of the two updates. +func withVaultLock(vaultPath string, fn func() error) error { + if err := os.MkdirAll(filepath.Dir(vaultPath), vaultDirMode); err != nil { + return fmt.Errorf("failed to create vault directory: %w", err) + } + + lock := flock.New(vaultPath + ".lock") + ctx, cancel := context.WithTimeout(context.Background(), vaultLockTimeout) + defer cancel() + + locked, err := lock.TryLockContext(ctx, vaultLockRetry) + if err != nil { + return fmt.Errorf("failed to acquire vault lock for %s: %w", vaultPath, err) + } + if !locked { + return fmt.Errorf("timed out waiting for the vault lock on %s", vaultPath) + } + defer func() { _ = lock.Unlock() }() + + return fn() +} diff --git a/storage_test.go b/storage_test.go new file mode 100644 index 0000000..de750d0 --- /dev/null +++ b/storage_test.go @@ -0,0 +1,233 @@ +package vault_test + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/flowexec/vault" +) + +const testVaultID = "v1" + +func unencryptedConfig(dir string) *vault.Config { + return &vault.Config{ + ID: testVaultID, + Type: vault.ProviderTypeUnencrypted, + Unencrypted: &vault.UnencryptedConfig{StoragePath: dir}, + } +} + +func vaultFilePath(dir string) string { + return filepath.Join(dir, fmt.Sprintf("vault-%s.json", testVaultID)) +} + +// A zero-length vault file used to be read as "no vault here", which made the +// constructor initialize a fresh one and immediately save over it -- destroying +// every secret and reporting success. +func TestEmptyVaultFileIsRejectedRatherThanOverwritten(t *testing.T) { + dir := t.TempDir() + path := vaultFilePath(dir) + + if err := os.WriteFile(path, []byte{}, 0600); err != nil { + t.Fatalf("failed to seed empty vault file: %v", err) + } + + _, err := vault.NewUnencryptedVault(unencryptedConfig(dir)) + if err == nil { + t.Fatal("NewUnencryptedVault() accepted an empty vault file, want an error") + } + if !errors.Is(err, vault.ErrVaultCorrupt) { + t.Errorf("error = %v, want ErrVaultCorrupt", err) + } + + // The crucial part: the file must be left exactly as it was. + info, statErr := os.Stat(path) + if statErr != nil { + t.Fatalf("vault file disappeared: %v", statErr) + } + if info.Size() != 0 { + t.Errorf("vault file was rewritten (size %d), want it left untouched", info.Size()) + } +} + +// A truncated vault must not be silently replaced with an empty one either. +func TestTruncatedVaultFileIsNotSilentlyReinitialized(t *testing.T) { + dir := t.TempDir() + + v, err := vault.NewUnencryptedVault(unencryptedConfig(dir)) + if err != nil { + t.Fatalf("NewUnencryptedVault() error = %v", err) + } + if err := v.SetSecret("keep-me", vault.NewSecretValue([]byte("value"))); err != nil { + t.Fatalf("SetSecret() error = %v", err) + } + + path := vaultFilePath(dir) + if err := os.WriteFile(path, []byte("{not json"), 0600); err != nil { + t.Fatalf("failed to corrupt vault file: %v", err) + } + + if _, err := vault.NewUnencryptedVault(unencryptedConfig(dir)); err == nil { + t.Fatal("NewUnencryptedVault() accepted a corrupt vault file, want an error") + } + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("failed to read vault file: %v", err) + } + if string(data) != "{not json" { + t.Errorf("corrupt vault file was overwritten with %q", string(data)) + } +} + +// Every save rewrites the whole file from an in-memory snapshot taken when the +// provider was built. Two providers writing without a shared lock silently lose +// one of the two updates -- the per-instance RWMutex does nothing here. +func TestConcurrentWritesFromSeparateProvidersDoNotLoseUpdates(t *testing.T) { + dir := t.TempDir() + + if _, err := vault.NewUnencryptedVault(unencryptedConfig(dir)); err != nil { + t.Fatalf("NewUnencryptedVault() error = %v", err) + } + + const writers = 8 + var wg sync.WaitGroup + errs := make(chan error, writers) + + for i := 0; i < writers; i++ { + wg.Add(1) + go func(n int) { + defer wg.Done() + // A separate provider instance per writer: distinct in-memory state, + // so only the cross-process file lock can serialize them. + v, err := vault.NewUnencryptedVault(unencryptedConfig(dir)) + if err != nil { + errs <- fmt.Errorf("writer %d: open: %w", n, err) + return + } + key := fmt.Sprintf("key-%d", n) + if err := v.SetSecret(key, vault.NewSecretValue([]byte("value"))); err != nil { + errs <- fmt.Errorf("writer %d: set: %w", n, err) + } + }(i) + } + wg.Wait() + close(errs) + for err := range errs { + t.Fatalf("%v", err) + } + + final, err := vault.NewUnencryptedVault(unencryptedConfig(dir)) + if err != nil { + t.Fatalf("reopen error = %v", err) + } + keys, err := final.ListSecrets() + if err != nil { + t.Fatalf("ListSecrets() error = %v", err) + } + if len(keys) != writers { + t.Errorf("vault holds %d secrets (%v), want all %d -- updates were lost", len(keys), keys, writers) + } +} + +func TestAtomicWriteLeavesNoTempFilesAndKeepsModeOwnerOnly(t *testing.T) { + dir := t.TempDir() + + v, err := vault.NewUnencryptedVault(unencryptedConfig(dir)) + if err != nil { + t.Fatalf("NewUnencryptedVault() error = %v", err) + } + for i := 0; i < 5; i++ { + if err := v.SetSecret(fmt.Sprintf("k%d", i), vault.NewSecretValue([]byte("v"))); err != nil { + t.Fatalf("SetSecret() error = %v", err) + } + } + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir() error = %v", err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), ".vault-") || strings.HasSuffix(e.Name(), ".tmp") { + t.Errorf("temp file %q was left behind", e.Name()) + } + } + + info, err := os.Stat(vaultFilePath(dir)) + if err != nil { + t.Fatalf("Stat() error = %v", err) + } + if perm := info.Mode().Perm(); perm != 0600 { + t.Errorf("vault file mode = %o, want 0600", perm) + } +} + +// A fixed ".tmp" name meant os.WriteFile would follow a symlink planted +// there and write the vault contents through it. +func TestAtomicWriteDoesNotFollowAPlantedTempSymlink(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(t.TempDir(), "victim.txt") + if err := os.WriteFile(target, []byte("original"), 0600); err != nil { + t.Fatalf("failed to create target: %v", err) + } + + if err := os.MkdirAll(dir, 0700); err != nil { + t.Fatalf("MkdirAll() error = %v", err) + } + if err := os.Symlink(target, vaultFilePath(dir)+".tmp"); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + v, err := vault.NewUnencryptedVault(unencryptedConfig(dir)) + if err != nil { + t.Fatalf("NewUnencryptedVault() error = %v", err) + } + if err := v.SetSecret("k", vault.NewSecretValue([]byte("secret"))); err != nil { + t.Fatalf("SetSecret() error = %v", err) + } + + data, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile() error = %v", err) + } + if string(data) != "original" { + t.Errorf("the planted symlink was followed: target now holds %q", string(data)) + } +} + +func TestLocalProvidersReportClosedRatherThanPanicking(t *testing.T) { + dir := t.TempDir() + + v, err := vault.NewUnencryptedVault(unencryptedConfig(dir)) + if err != nil { + t.Fatalf("NewUnencryptedVault() error = %v", err) + } + if err := v.Close(); err != nil { + t.Fatalf("Close() error = %v", err) + } + + // Every one of these dereferenced a nil state before the guards were added. + if _, err := v.GetSecret("k"); !errors.Is(err, vault.ErrVaultClosed) { + t.Errorf("GetSecret() after Close = %v, want ErrVaultClosed", err) + } + if err := v.SetSecret("k", vault.NewSecretValue([]byte("v"))); !errors.Is(err, vault.ErrVaultClosed) { + t.Errorf("SetSecret() after Close = %v, want ErrVaultClosed", err) + } + if err := v.DeleteSecret("k"); !errors.Is(err, vault.ErrVaultClosed) { + t.Errorf("DeleteSecret() after Close = %v, want ErrVaultClosed", err) + } + if _, err := v.ListSecrets(); !errors.Is(err, vault.ErrVaultClosed) { + t.Errorf("ListSecrets() after Close = %v, want ErrVaultClosed", err) + } + if _, err := v.HasSecret("k"); !errors.Is(err, vault.ErrVaultClosed) { + t.Errorf("HasSecret() after Close = %v, want ErrVaultClosed", err) + } + if _, err := v.Metadata(); !errors.Is(err, vault.ErrVaultClosed) { + t.Errorf("Metadata() after Close = %v, want ErrVaultClosed", err) + } +} diff --git a/unencrypted.go b/unencrypted.go index 78d8f37..1365e4b 100644 --- a/unencrypted.go +++ b/unencrypted.go @@ -2,9 +2,7 @@ package vault import ( "encoding/json" - "errors" "fmt" - "os" "path/filepath" "sort" "sync" @@ -74,20 +72,16 @@ func (v *UnencryptedVault) init() error { Secrets: make(map[string]string), } - return v.save() + return withVaultLock(v.fullPath, v.save) } // load retrieves the vault contents from the file and parses it into the state. func (v *UnencryptedVault) load() error { - data, err := os.ReadFile(filepath.Clean(v.fullPath)) + data, exists, err := readVaultFile(v.fullPath) if err != nil { - if errors.Is(err, os.ErrNotExist) { - return nil - } - return fmt.Errorf("%w: failed to read vault file %s: %w", ErrVaultNotFound, v.fullPath, err) + return err } - - if len(data) == 0 { + if !exists { return nil } @@ -104,7 +98,7 @@ func (v *UnencryptedVault) load() error { // save writes the vault contents to disk in JSON format func (v *UnencryptedVault) save() error { if v.state == nil { - return nil + return ErrVaultClosed } v.state.LastModified = time.Now() @@ -115,22 +109,21 @@ func (v *UnencryptedVault) save() error { return fmt.Errorf("failed to marshal vault state: %w", err) } - // Write to file atomically - if err := os.MkdirAll(filepath.Dir(v.fullPath), 0750); err != nil { - return fmt.Errorf("failed to create vault directory: %w", err) - } - - tempFile := v.fullPath + ".tmp" - if err := os.WriteFile(tempFile, data, 0600); err != nil { - return fmt.Errorf("failed to write temp vault file: %w", err) - } - - if err := os.Rename(tempFile, v.fullPath); err != nil { - _ = os.Remove(tempFile) - return fmt.Errorf("failed to move vault file: %w", err) - } + return writeVaultFileAtomic(v.fullPath, data) +} - return nil +// mutate runs a read-modify-write cycle under the cross-process vault lock. +// See AES256Vault.mutate for why the reload inside the lock is required. +func (v *UnencryptedVault) mutate(apply func() error) error { + return withVaultLock(v.fullPath, func() error { + if err := v.load(); err != nil { + return err + } + if err := apply(); err != nil { + return err + } + return v.save() + }) } func (v *UnencryptedVault) ID() string { @@ -151,6 +144,13 @@ func (v *UnencryptedVault) GetSecret(key string) (Secret, error) { v.mu.RLock() defer v.mu.RUnlock() + if err := ValidateSecretKey(key); err != nil { + return nil, err + } + if v.state == nil { + return nil, ErrVaultClosed + } + value, exists := v.state.Secrets[key] if !exists { return nil, ErrSecretNotFound @@ -166,32 +166,49 @@ func (v *UnencryptedVault) SetSecret(key string, secret Secret) error { if err := ValidateSecretKey(key); err != nil { return err } - - if v.state.Secrets == nil { - v.state.Secrets = make(map[string]string) + if v.state == nil { + return ErrVaultClosed } - v.state.Secrets[key] = secret.PlainTextString() - return v.save() + return v.mutate(func() error { + if v.state.Secrets == nil { + v.state.Secrets = make(map[string]string) + } + v.state.Secrets[key] = secret.PlainTextString() + return nil + }) } func (v *UnencryptedVault) DeleteSecret(key string) error { v.mu.Lock() defer v.mu.Unlock() - _, exists := v.state.Secrets[key] - if !exists { - return ErrSecretNotFound + if err := ValidateSecretKey(key); err != nil { + return err + } + if v.state == nil { + return ErrVaultClosed } - delete(v.state.Secrets, key) - return v.save() + // The existence check runs inside mutate, after the reload, so it sees the + // current on-disk contents rather than a stale snapshot. + return v.mutate(func() error { + if _, exists := v.state.Secrets[key]; !exists { + return ErrSecretNotFound + } + delete(v.state.Secrets, key) + return nil + }) } func (v *UnencryptedVault) ListSecrets() ([]string, error) { v.mu.RLock() defer v.mu.RUnlock() + if v.state == nil { + return nil, ErrVaultClosed + } + keys := make([]string, 0, len(v.state.Secrets)) for k := range v.state.Secrets { keys = append(keys, k) @@ -206,6 +223,13 @@ func (v *UnencryptedVault) HasSecret(key string) (bool, error) { v.mu.RLock() defer v.mu.RUnlock() + if err := ValidateSecretKey(key); err != nil { + return false, err + } + if v.state == nil { + return false, ErrVaultClosed + } + _, exists := v.state.Secrets[key] return exists, nil } From 4065d14a3288fcb11e768ef8cd3fc20939705a79 Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 26 Jul 2026 15:51:47 -0400 Subject: [PATCH 3/7] fix(crypto)!: reject weak keys and stop deriving unsalted KEKs DeriveKey's "generate a random salt" branch tested salt == nil, but []byte("") from a variable string is non-nil. The natural call -- DeriveEncryptionKey(passphrase, "") meaning "I have no salt, please make one" -- therefore ran scrypt with a zero-length salt and returned "" as the salt. The result was an unsalted, fully deterministic KEK: identical for every user with the same passphrase, and precomputable. The tests only ever covered nil, never []byte(""), so it went unnoticed. Now len(salt) == 0, and salts shorter than 16 bytes are rejected outright. aes.NewCipher accepts 16, 24 and 32 byte keys, so a short key silently downgraded an "AES256" vault to AES-128 or AES-192 with no warning -- and any 32-character passphrase made of base64 alphabet characters decodes cleanly to 24 bytes and was accepted as a key. EncryptValue and DecryptValue now require exactly 32 bytes. scrypt ran at N=1<<20, r=8: roughly 1 GiB of memory and several seconds per derivation, which thrashes or OOMs on a modest machine and makes concurrent derivations a trivial local denial of service. Lowered to N=1<<16 (~64 MiB). The crypto test suite drops from 35s to 0.7s as a direct result. Because changing the cost silently changes every derived key, the salt now carries the parameters that produced it ("scrypt$N=...,r=...,p=...$"). Salts without a parameter block predate this and still derive with the original cost, so nothing already issued changes meaning. Verified that nothing outside this package derives keys: flow uses GenerateEncryptionKey, which is unaffected. The AES-256-GCM construction itself was already correct -- fresh random nonce per seal, standard Seal(nonce, nonce, ...) idiom, length-checked open, real authentication -- and is unchanged. BREAKING CHANGE: keys that are not exactly 32 bytes are now rejected, and DeriveKey returns a parameter-tagged salt that must be passed back verbatim rather than base64-decoded first. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HtuDuGkSqTfAXSepf8b51p --- aes.go | 15 ++++- crypto/crypto.go | 136 +++++++++++++++++++++++++++++++++++++----- crypto/crypto_test.go | 126 ++++++++++++++++++++++++-------------- 3 files changed, 215 insertions(+), 62 deletions(-) diff --git a/aes.go b/aes.go index 89a3dd5..551fdaf 100644 --- a/aes.go +++ b/aes.go @@ -42,13 +42,22 @@ func GenerateEncryptionKey() (string, error) { return crypto.GenerateKey() } -// DeriveEncryptionKey derives an AES encryption key from a passphrase +// DeriveEncryptionKey derives an AES encryption key from a passphrase. +// +// An empty sal requests a freshly generated salt. Pass the returned salt back +// verbatim to re-derive the same key; it carries the parameters it was made +// with, so future changes to the defaults cannot alter an existing key. func DeriveEncryptionKey(passphrase, sal string) (string, string, error) { - key, salt, err := crypto.DeriveKey([]byte(passphrase), []byte(sal)) + var salt []byte + if sal != "" { + salt = []byte(sal) + } + + key, salt2, err := crypto.DeriveKey([]byte(passphrase), salt) if err != nil { return "", "", fmt.Errorf("failed to derive encryption key: %w", err) } - return key, salt, nil + return key, salt2, nil } // ValidateEncryptionKey checks if a key is valid by attempting to encrypt/decrypt test data diff --git a/crypto/crypto.go b/crypto/crypto.go index e30948d..397ce99 100644 --- a/crypto/crypto.go +++ b/crypto/crypto.go @@ -7,13 +7,44 @@ import ( "encoding/base64" "fmt" "io" + "strings" "golang.org/x/crypto/scrypt" ) +const ( + // KeyLen is the required key size. AES-256 means 32 bytes and nothing else: + // aes.NewCipher also accepts 16 and 24, so without an explicit check a short + // key silently downgrades an "AES256" vault to AES-128 or AES-192. + KeyLen = 32 + + // SaltLen is the size of a generated salt. + SaltLen = 32 + // MinSaltLen is the smallest salt accepted for derivation. + MinSaltLen = 16 + + // scryptN is the CPU/memory cost. Memory used is roughly 128 * N * r, so + // this is ~64 MiB per derivation. The previous value of 1<<20 required + // ~1 GiB and several seconds, which thrashes or OOMs on a modest machine + // and makes concurrent derivations a trivial local denial of service. + scryptN = 1 << 16 + scryptR = 8 + scryptP = 1 + + // legacyScryptN is the original cost. Salts issued before parameters were + // recorded carry no parameter block, and must keep deriving the same key. + legacyScryptN = 1 << 20 + + saltPrefix = "scrypt" + saltSep = "$" + + // maxPlaintextLen bounds a single encryption. + maxPlaintextLen = 64 * 1024 * 1024 +) + // GenerateKey generates a random 32 byte key and returns it as a base64 encoded string. func GenerateKey() (string, error) { - key := make([]byte, 32) + key := make([]byte, KeyLen) _, err := rand.Read(key) if err != nil { return "", fmt.Errorf("error reading random bytes: %w", err) @@ -21,23 +52,85 @@ func GenerateKey() (string, error) { return EncodeValue(key), nil } -// DeriveKey derives a 32 byte key from the provided password and salt and returns -// the key and salt as base64 encoded strings. -// If salt is nil, a random salt will be generated. +// DeriveKey derives a 32 byte key from the provided password and salt, returning +// the base64 encoded key and the salt that produced it. +// +// If salt is empty a fresh random one is generated. The returned salt carries +// the parameters used ("scrypt$N=...,r=...,p=...$") so that changing the +// defaults later cannot silently change the key derived from an existing salt. +// Pass the returned salt back verbatim to re-derive the same key. func DeriveKey(password, salt []byte) (string, string, error) { - if salt == nil { - salt = make([]byte, 32) - if _, err := rand.Read(salt); err != nil { + // Deliberately len()==0 rather than salt==nil. []byte("") from a variable + // string is non-nil, so a nil check let the natural "I have no salt, make + // one" call fall through to an *unsalted*, fully deterministic derivation. + if len(salt) == 0 { + generated := make([]byte, SaltLen) + if _, err := rand.Read(generated); err != nil { + return "", "", fmt.Errorf("error generating salt: %w", err) + } + key, err := deriveScrypt(password, generated, scryptN) + if err != nil { return "", "", err } + return key, formatSalt(generated), nil } - key, err := scrypt.Key(password, salt, 1048576, 8, 1, 32) + raw, n, err := parseSalt(salt) + if err != nil { + return "", "", err + } + key, err := deriveScrypt(password, raw, n) if err != nil { return "", "", err } + return key, string(salt), nil +} + +func formatSalt(raw []byte) string { + return fmt.Sprintf("%s%sN=%d,r=%d,p=%d%s%s", + saltPrefix, saltSep, scryptN, scryptR, scryptP, saltSep, EncodeValue(raw)) +} + +// parseSalt splits a salt into its raw bytes and cost parameter. A salt with no +// parameter block predates them and is used as-is with the original cost. +func parseSalt(salt []byte) ([]byte, int, error) { + s := string(salt) + if !strings.HasPrefix(s, saltPrefix+saltSep) { + if len(salt) < MinSaltLen { + return nil, 0, fmt.Errorf("salt must be at least %d bytes, got %d", MinSaltLen, len(salt)) + } + return salt, legacyScryptN, nil + } + + parts := strings.Split(s, saltSep) + if len(parts) != 3 { + return nil, 0, fmt.Errorf("malformed salt: expected %s$$", saltPrefix) + } + + var n, r, p int + if _, err := fmt.Sscanf(parts[1], "N=%d,r=%d,p=%d", &n, &r, &p); err != nil { + return nil, 0, fmt.Errorf("malformed salt parameters %q: %w", parts[1], err) + } + if r != scryptR || p != scryptP { + return nil, 0, fmt.Errorf("unsupported salt parameters r=%d p=%d", r, p) + } - return EncodeValue(key), EncodeValue(salt), nil + raw, err := DecodeValue(parts[2]) + if err != nil { + return nil, 0, fmt.Errorf("malformed salt encoding: %w", err) + } + if len(raw) < MinSaltLen { + return nil, 0, fmt.Errorf("salt must be at least %d bytes, got %d", MinSaltLen, len(raw)) + } + return raw, n, nil +} + +func deriveScrypt(password, salt []byte, n int) (string, error) { + key, err := scrypt.Key(password, salt, n, scryptR, scryptP, KeyLen) + if err != nil { + return "", fmt.Errorf("error deriving key: %w", err) + } + return EncodeValue(key), nil } // EncodeValue encodes a byte slice as a base64 encoded string. @@ -54,12 +147,27 @@ func DecodeValue(s string) ([]byte, error) { return data, nil } +// decodeKey decodes and length-checks an encryption key. +func decodeKey(encryptionKey string) ([]byte, error) { + key, err := DecodeValue(encryptionKey) + if err != nil { + return nil, fmt.Errorf("error decoding master key: %w", err) + } + if len(key) != KeyLen { + return nil, fmt.Errorf( + "encryption key must be %d bytes, got %d; expected a base64 encoded 256-bit key", + KeyLen, len(key), + ) + } + return key, nil +} + // EncryptValue encrypts a string using AES-256-GCM and returns the encrypted value as a base64 encoded string. // The encryption key used for encryption must be a base64 encoded string. func EncryptValue(encryptionKey string, text string) (string, error) { - decodedMasterKey, err := DecodeValue(encryptionKey) + decodedMasterKey, err := decodeKey(encryptionKey) if err != nil { - return "", fmt.Errorf("error decoding master key: %w", err) + return "", err } block, err := aes.NewCipher(decodedMasterKey) if err != nil { @@ -73,7 +181,7 @@ func EncryptValue(encryptionKey string, text string) (string, error) { plaintext := []byte(text) // verify that the plaintext is not too long to fit in an int - if len(plaintext) > 64*1024*1024 { + if len(plaintext) > maxPlaintextLen { return "", fmt.Errorf("plaintext too long to encrypt") } @@ -88,9 +196,9 @@ func EncryptValue(encryptionKey string, text string) (string, error) { // DecryptValue decrypts a string using AES-256-GCM and returns the decrypted value as a string. // The master key used for decryption must be a base64 encoded string. func DecryptValue(encryptionKey string, text string) (string, error) { - decodedMasterKey, err := DecodeValue(encryptionKey) + decodedMasterKey, err := decodeKey(encryptionKey) if err != nil { - return "", fmt.Errorf("error decoding master key: %w", err) + return "", err } block, err := aes.NewCipher(decodedMasterKey) if err != nil { diff --git a/crypto/crypto_test.go b/crypto/crypto_test.go index c787353..204aab8 100644 --- a/crypto/crypto_test.go +++ b/crypto/crypto_test.go @@ -34,76 +34,91 @@ func TestGenerateKey(t *testing.T) { } } -func TestDeriveKeyWithProvidedSalt(t *testing.T) { - salt, err := crypto.GenerateKey() - if err != nil { - t.Fatalf("Failed to generate salt: %v", err) - } - decodedSalt, err := crypto.DecodeValue(salt) - if err != nil { - t.Fatalf("Failed to decode salt: %v", err) - } - if len(decodedSalt) == 0 { - t.Error("Decoded salt should not be empty") - } +// The returned salt is passed back verbatim; it carries the parameters it was +// produced with so a later change to the defaults cannot silently derive a +// different key from the same salt. +func TestDeriveKeyRoundTripsItsOwnSalt(t *testing.T) { + password := []byte("password") - inputPassword := []byte("password") - derivedKey, outSalt, err := crypto.DeriveKey(inputPassword, decodedSalt) + derivedKey, outSalt, err := crypto.DeriveKey(password, nil) if err != nil { - t.Fatalf("Failed to derive key: %v", err) - } - if derivedKey == "" { - t.Error("Derived key should not be empty") + t.Fatalf("Failed to derive key without salt: %v", err) } - if outSalt != salt { - t.Errorf("Output salt should equal input salt, got %s, expected %s", outSalt, salt) + if derivedKey == "" || outSalt == "" { + t.Fatal("Derived key and salt should not be empty") } decodedDerivedKey, err := crypto.DecodeValue(derivedKey) if err != nil { t.Fatalf("Failed to decode derived key: %v", err) } - if len(decodedDerivedKey) == 0 { - t.Error("Decoded derived key should not be empty") + if len(decodedDerivedKey) != crypto.KeyLen { + t.Errorf("Derived key is %d bytes, want %d", len(decodedDerivedKey), crypto.KeyLen) } -} -func TestDeriveKeyWithoutSalt(t *testing.T) { - inputPassword := []byte("password") - derivedKey, outSalt, err := crypto.DeriveKey(inputPassword, nil) + derivedKey2, outSalt2, err := crypto.DeriveKey(password, []byte(outSalt)) if err != nil { - t.Fatalf("Failed to derive key without salt: %v", err) + t.Fatalf("Failed to derive key with the returned salt: %v", err) } - if derivedKey == "" { - t.Error("Derived key should not be empty") + if derivedKey != derivedKey2 { + t.Error("Keys derived with the same password and salt should be identical") } - if outSalt == "" { - t.Error("Generated salt should not be empty") + if outSalt != outSalt2 { + t.Errorf("Salt should round trip unchanged, got %s, want %s", outSalt2, outSalt) } +} - decodedDerivedKey, err := crypto.DecodeValue(derivedKey) +func TestDeriveKeyUsesDistinctSaltsPerCall(t *testing.T) { + password := []byte("password") + + key1, salt1, err := crypto.DeriveKey(password, nil) if err != nil { - t.Fatalf("Failed to decode derived key: %v", err) + t.Fatalf("Failed to derive first key: %v", err) } - if len(decodedDerivedKey) == 0 { - t.Error("Decoded derived key should not be empty") + key2, salt2, err := crypto.DeriveKey(password, nil) + if err != nil { + t.Fatalf("Failed to derive second key: %v", err) } - // Test reproducibility with same salt - decodedSalt, err := crypto.DecodeValue(outSalt) - if err != nil { - t.Fatalf("Failed to decode output salt: %v", err) + if salt1 == salt2 { + t.Error("Each derivation should generate a fresh salt") + } + if key1 == key2 { + t.Error("The same password with different salts must not produce the same key") } +} + +// []byte("") from a variable string is non-nil, so the old salt==nil check +// never fired for the natural "I have no salt, generate one" call. That derived +// an unsalted, fully deterministic key -- identical for every user with the +// same passphrase, and precomputable. +func TestDeriveKeyTreatsEmptySaltAsAbsentNotUnsalted(t *testing.T) { + password := []byte("password") + empty := "" - derivedKey2, outSalt2, err := crypto.DeriveKey(inputPassword, decodedSalt) + key1, salt1, err := crypto.DeriveKey(password, []byte(empty)) if err != nil { - t.Fatalf("Failed to derive key with same salt: %v", err) + t.Fatalf("Failed to derive key with empty salt: %v", err) } - if derivedKey != derivedKey2 { - t.Error("Keys derived with same password and salt should be identical") + key2, salt2, err := crypto.DeriveKey(password, []byte(empty)) + if err != nil { + t.Fatalf("Failed to derive second key with empty salt: %v", err) } - if outSalt != outSalt2 { - t.Error("Output salt should be same when input salt is provided") + + if salt1 == "" || salt2 == "" { + t.Fatal("An empty salt must produce a freshly generated one") + } + if salt1 == salt2 { + t.Error("An empty salt produced the same salt twice; it is not being generated") + } + if key1 == key2 { + t.Error("An empty salt derived a deterministic key; the salt is not being applied") + } +} + +func TestDeriveKeyRejectsShortSalts(t *testing.T) { + if _, _, err := crypto.DeriveKey([]byte("password"), []byte("tiny")); err == nil { + t.Error("Expected a short salt to be rejected") } } @@ -246,6 +261,27 @@ func TestInvalidKeys(t *testing.T) { } } +// aes.NewCipher accepts 16, 24 and 32 byte keys, so without an explicit length +// check a short key silently downgrades an "AES256" vault to AES-128 or -192. +func TestNonAES256KeysAreRejected(t *testing.T) { + for _, size := range []int{8, 16, 24, 31, 33, 64} { + key := crypto.EncodeValue(make([]byte, size)) + + if _, err := crypto.EncryptValue(key, "data"); err == nil { + t.Errorf("EncryptValue accepted a %d byte key, want rejection", size) + } + if _, err := crypto.DecryptValue(key, crypto.EncodeValue(make([]byte, 64))); err == nil { + t.Errorf("DecryptValue accepted a %d byte key, want rejection", size) + } + } + + // The correct size still works. + valid := crypto.EncodeValue(make([]byte, crypto.KeyLen)) + if _, err := crypto.EncryptValue(valid, "data"); err != nil { + t.Errorf("EncryptValue rejected a %d byte key: %v", crypto.KeyLen, err) + } +} + func TestInvalidCiphertext(t *testing.T) { key, err := crypto.GenerateKey() if err != nil { From 9878c9434b815f751bcc1a0d81bfc93f348c8e02 Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 26 Jul 2026 15:56:18 -0400 Subject: [PATCH 4/7] fix: validate vault IDs, namespace keyring entries, repair path expansion A vault ID is interpolated straight into a filename, and filepath.Clean does not sanitize that -- it *resolves* it. Clean("vault-../../../tmp/evil.enc") is "../tmp/evil.enc", because the first ".." pops the literal "vault-.." element and the rest survive; joining that onto the storage directory escapes it, so a crafted ID could make save() overwrite an arbitrary file. Config.Validate only checked that the ID was non-empty. IDs are now charset-validated, and the resolved path is separately proved to stay inside the storage directory. Keyring entry names were built as "%s-secret-%s" over an unvalidated ID and a key charset that permits dashes, so vault "a" with key "b-secret-c" and vault "a-secret-b" with key "c" both mapped to "a-secret-b-secret-c" -- one vault silently reading, overwriting or deleting another's secret. Length-prefixed now. Also: - KeyringVault only initializes metadata on keyring.ErrNotFound. Treating every error as "vault doesn't exist" meant a locked keychain, a DBus failure or a dismissed prompt overwrote the real Created timestamp. - New() returns an explicit nil Provider on error. Returning the typed pointer wrapped a nil *AgeVault in a non-nil interface, so the usual `if provider != nil` guard passed and the next call panicked. - Every exported constructor calls Config.Validate(); previously only vault.New() did, so the direct constructors bypassed validation entirely. - StoragePath now goes through expandPath/validateSecurePath like key and identity paths always did. The README's own WithAESPath("~/secrets.vault") created a literal "~" directory in the working tree. - expandPath: ".config/x" no longer loses its leading dot, "../x" no longer silently drops the parent reference, "~user/x" no longer becomes "user/x", and "$HOME/vault" resolves instead of looking up a variable named "HOME/vault". An unset variable is an error rather than expanding to "/vault". - validateSecurePath compares path elements instead of substrings, so "my..backup" is allowed and "/etcetera" is no longer caught by "/etc". - ValidateSecretKey rejects a leading dash (flag injection into backend CLIs) and "."/".." (path elements), is applied at every provider entry point rather than only SetSecret, and compiles its regex once instead of on every call. - KeyResolver propagates key-file failures. `err == nil &&` discarded typos and permission errors, surfacing them as a generic "no encryption keys found" or silently falling through to another source. - SecretValue.String() takes a value receiver so a dereferenced copy is masked too; Zero() drops its forced per-call runtime.GC() and the cargo-cult random prefill, and now documents what it cannot guarantee. - DefaultVaultKeyEnv is a const; as a var any dependency could repoint every default key lookup in the process. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HtuDuGkSqTfAXSepf8b51p --- aes.go | 13 ++-- aes_key.go | 18 ++++- age.go | 13 ++-- config.go | 6 +- keyring.go | 26 ++++++-- local.go | 84 +++++++++++++++--------- secret.go | 83 ++++++++++++++++++----- storage.go | 30 +++++++++ unencrypted.go | 13 ++-- validation_test.go | 159 +++++++++++++++++++++++++++++++++++++++++++++ vault.go | 31 +++++++-- 11 files changed, 401 insertions(+), 75 deletions(-) create mode 100644 validation_test.go diff --git a/aes.go b/aes.go index 551fdaf..77c07bd 100644 --- a/aes.go +++ b/aes.go @@ -2,7 +2,6 @@ package vault import ( "fmt" - "path/filepath" "sort" "sync" "time" @@ -85,10 +84,14 @@ func NewAES256Vault(cfg *Config) (*AES256Vault, error) { return nil, fmt.Errorf("AES configuration is required") } - path := filepath.Join( - filepath.Clean(cfg.Aes.StoragePath), - filepath.Clean(fmt.Sprintf("%s-%s.%s", vaultFileBase, cfg.ID, aesVaultFileExt)), - ) + if err := cfg.Validate(); err != nil { + return nil, err + } + + path, err := resolveVaultPath(cfg.Aes.StoragePath, cfg.ID, aesVaultFileExt) + if err != nil { + return nil, err + } vault := &AES256Vault{ id: cfg.ID, diff --git a/aes_key.go b/aes_key.go index e15dfee..9df63fe 100644 --- a/aes_key.go +++ b/aes_key.go @@ -26,21 +26,37 @@ func NewKeyResolver(sources []KeySource) *KeyResolver { func (r *KeyResolver) ResolveKeys() ([]string, error) { var keys []string + var failures []string for _, source := range r.sources { switch source.Type { case envSource: if key := r.fromEnvironment(source.Name); key != "" { keys = append(keys, key) + } else { + failures = append(failures, fmt.Sprintf("env %s: not set or empty", source.Name)) } case fileSource: - if key, err := r.fromFile(source.Path); err == nil && key != "" { + // Errors here used to be discarded outright (`err == nil &&`), so a + // typo'd path, a permissions problem or a truncated key file all + // surfaced as the generic "no encryption keys found" -- or worse, + // silently fell through to a different source. + key, err := r.fromFile(source.Path) + switch { + case err != nil: + failures = append(failures, fmt.Sprintf("file %s: %v", source.Path, err)) + case key == "": + failures = append(failures, fmt.Sprintf("file %s: is empty", source.Path)) + default: keys = append(keys, key) } } } if len(keys) == 0 { + if len(failures) > 0 { + return nil, fmt.Errorf("%w: no encryption keys found (%s)", ErrNoAccess, strings.Join(failures, "; ")) + } return nil, fmt.Errorf("%w: no encryption keys found", ErrNoAccess) } diff --git a/age.go b/age.go index 99ffe45..06f6d09 100644 --- a/age.go +++ b/age.go @@ -4,7 +4,6 @@ import ( "bytes" "encoding/json" "fmt" - "path/filepath" "sort" "sync" "time" @@ -46,10 +45,14 @@ func NewAgeVault(cfg *Config) (*AgeVault, error) { return nil, fmt.Errorf("age configuration is required") } - path := filepath.Join( - filepath.Clean(cfg.Age.StoragePath), - filepath.Clean(fmt.Sprintf("%s-%s.%s", vaultFileBase, cfg.ID, ageVaultFileExt)), - ) + if err := cfg.Validate(); err != nil { + return nil, err + } + + path, err := resolveVaultPath(cfg.Age.StoragePath, cfg.ID, ageVaultFileExt) + if err != nil { + return nil, err + } vault := &AgeVault{ mu: sync.RWMutex{}, diff --git a/config.go b/config.go index b7afe57..c40b52a 100644 --- a/config.go +++ b/config.go @@ -30,8 +30,10 @@ type Config struct { } func (c *Config) Validate() error { - if c.ID == "" { - return fmt.Errorf("%w: vault ID is required", ErrInvalidConfig) + // The ID becomes part of a filename and of keyring entry names, so it needs + // a real charset check, not just a non-empty check. + if err := ValidateVaultID(c.ID); err != nil { + return err } switch c.Type { diff --git a/keyring.go b/keyring.go index 300e8ed..a33fecd 100644 --- a/keyring.go +++ b/keyring.go @@ -25,13 +25,22 @@ func NewKeyringVault(cfg *Config) (*KeyringVault, error) { return nil, fmt.Errorf("keyring configuration is required") } + if err := cfg.Validate(); err != nil { + return nil, err + } + vault := &KeyringVault{ id: cfg.ID, service: cfg.Keyring.Service, } - // Try to load metadata or initialize if not exists + // Only a genuinely absent record means "new vault". Treating every error as + // absence meant a locked keychain, a DBus failure or a dismissed prompt made + // initMetadata() write a fresh record over the real Created timestamp. if err := vault.loadMetadata(); err != nil { + if !errors.Is(err, keyring.ErrNotFound) { + return nil, fmt.Errorf("failed to read keyring vault metadata: %w", err) + } if err := vault.initMetadata(); err != nil { return nil, fmt.Errorf("failed to initialize keyring vault metadata: %w", err) } @@ -40,16 +49,25 @@ func NewKeyringVault(cfg *Config) (*KeyringVault, error) { return vault, nil } +// Keyring entry names are a flat namespace shared by every vault using the same +// service, so the components are length-prefixed to make them unambiguous. +// Plain "%s-secret-%s" collided: vault "a" with key "b-secret-c" and vault +// "a-secret-b" with key "c" both produced "a-secret-b-secret-c", letting one +// vault silently read, overwrite or delete another's secret. +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 fmt.Sprintf("%s-metadata", v.id) + return v.namespaced("metadata", "") } func (v *KeyringVault) secretKey(key string) string { - return fmt.Sprintf("%s-secret-%s", v.id, key) + return v.namespaced("secret", key) } func (v *KeyringVault) secretsListKey() string { - return fmt.Sprintf("%s-secrets-list", v.id) + return v.namespaced("secrets-list", "") } func (v *KeyringVault) initMetadata() error { diff --git a/local.go b/local.go index 80cc138..2480111 100644 --- a/local.go +++ b/local.go @@ -14,9 +14,10 @@ const ( fileSource = "file" ) -var ( - DefaultVaultKeyEnv = "VAULT_KEY" -) +// DefaultVaultKeyEnv is the environment variable consulted when no key source +// is configured. A const rather than a var: as a package-level var, any +// dependency could repoint every default key lookup in the process. +const DefaultVaultKeyEnv = "VAULT_KEY" type Metadata struct { Created time.Time `json:"created"` @@ -30,27 +31,34 @@ func validateSecurePath(path string) error { return fmt.Errorf("path cannot be empty") } - // Check for directory traversal attempts - cleanPath := filepath.Clean(path) - if strings.Contains(cleanPath, "..") { - return NewVaultPathError(path) - } - // Check for null bytes if strings.Contains(path, "\x00") { return NewVaultPathError(path) } // Ensure the path is absolute after expansion - absPath, err := filepath.Abs(cleanPath) + absPath, err := filepath.Abs(filepath.Clean(path)) if err != nil { return fmt.Errorf("failed to get absolute path: %w", err) } - // Basic check that we're not accessing sensitive system directories + // Compare path elements rather than substrings. strings.Contains(clean, "..") + // rejected legitimate names like "my..backup" while catching almost nothing + // real, since Clean on an absolute path has already resolved any genuine .. + // elements away. + for _, elem := range strings.Split(absPath, string(filepath.Separator)) { + if elem == ".." { + return NewVaultPathError(path) + } + } + + // Basic check that we're not writing into sensitive system directories. + // Compared as path prefixes, so "/etcetera" is no longer caught by "/etc". + // This is a guard rail, not a security boundary -- it is Unix-only and a + // symlink can still lead elsewhere. systemDirs := []string{"/etc", "/sys", "/proc", "/dev"} for _, sysDir := range systemDirs { - if strings.HasPrefix(absPath, sysDir) { + if absPath == sysDir || strings.HasPrefix(absPath, sysDir+string(filepath.Separator)) { return NewVaultPathError(path) } } @@ -65,38 +73,50 @@ func expandPath(path string) (string, error) { var expandedPath string - switch path[0] { - case '~': + switch { + // Only "~" itself or a "~/..." prefix. Slicing path[1:] unconditionally + // turned "~user/x" into "user/x", silently addressing the wrong file. + case path == "~": homeDir, err := os.UserHomeDir() if err != nil { return "", fmt.Errorf("failed to get user home directory: %w", err) } - expandedPath = homeDir + path[1:] - case '/': - expandedPath = path - case '.': - wd, err := os.Getwd() + expandedPath = homeDir + case strings.HasPrefix(path, "~/"): + homeDir, err := os.UserHomeDir() if err != nil { - return "", fmt.Errorf("failed to get working directory: %w", err) + return "", fmt.Errorf("failed to get user home directory: %w", err) } - expandedPath = wd + "/" + path[1:] - case '$': - envVar := path[1:] - if value, exists := os.LookupEnv(envVar); exists { - expandedPath = value - } else { - return "", fmt.Errorf("environment variable %s not found", envVar) + expandedPath = filepath.Join(homeDir, path[2:]) + case strings.HasPrefix(path, "$"): + // Split the leading variable from the rest so "$HOME/vault" works. The + // previous code treated the whole remainder as the variable name and so + // looked up an env var literally called "HOME/vault". Resolving via + // os.ExpandEnv instead would be worse: an unset variable expands to "" + // and "$NOPE/vault" would silently become "/vault". + name, rest := path[1:], "" + if i := strings.IndexAny(name, `/\`); i >= 0 { + name, rest = name[:i], name[i+1:] } + name = strings.TrimSuffix(strings.TrimPrefix(name, "{"), "}") + + value, exists := os.LookupEnv(name) + if !exists || value == "" { + return "", fmt.Errorf("environment variable %s not found", name) + } + expandedPath = filepath.Join(value, rest) + case filepath.IsAbs(path): + expandedPath = path default: + // Everything relative -- including "./x", "../x" and ".config/x" -- + // joins against the working directory. The old code sliced path[1:] for + // anything starting with a dot, which dropped the leading dot from + // ".config/x" and silently swallowed the parent reference in "../x". wd, err := os.Getwd() if err != nil { return "", fmt.Errorf("failed to get working directory: %w", err) } - if wd[len(wd)-1] == '/' { - expandedPath = wd + path - } else { - expandedPath = wd + "/" + path - } + expandedPath = filepath.Join(wd, path) } if err := validateSecurePath(expandedPath); err != nil { diff --git a/secret.go b/secret.go index 3434c1c..867e9fb 100644 --- a/secret.go +++ b/secret.go @@ -1,10 +1,9 @@ package vault import ( - "crypto/rand" "fmt" "regexp" - "runtime" + "strings" ) type Secret interface { @@ -24,18 +23,25 @@ type Secret interface { // SecureBytes is a wrapper around []byte that provides secure memory handling type SecureBytes []byte -// Zero securely clears the byte slice +// Zero clears the byte slice. +// +// Note on what this can and cannot guarantee: it overwrites *this* buffer only. +// Any Go string derived from it (see PlainTextString) is immutable and cannot be +// zeroed, and the runtime may have copied the backing array during a heap move. +// Treat this as reducing exposure, not eliminating it. func (s *SecureBytes) Zero() { - if s != nil && len(*s) > 0 { - // The series of steps below ensures that the memory is cleared securely. It prevents the compiler from - // optimizing away the zeroing operation and is recommended to securely clear sensitive data in Go. - _, _ = rand.Read(*s) - for i := range *s { - (*s)[i] = 0 - } - *s = (*s)[:0] - runtime.GC() + if s == nil || len(*s) == 0 { + return } + // The previous implementation filled with random bytes first and then forced + // a full runtime.GC() on every call. Neither helped: Go does not + // dead-store-eliminate writes through a pointer-reachable slice, and a + // forced GC per secret is a serious performance footgun in a library while + // still not guaranteeing that stale copies are collected or overwritten. + for i := range *s { + (*s)[i] = 0 + } + *s = (*s)[:0] } // Copy creates a secure copy of the bytes @@ -58,11 +64,21 @@ func NewSecretValue(value []byte) *SecretValue { return &SecretValue{value: secureValue} } +// PlainTextString returns the secret as a string. +// +// The result is an immutable Go string and therefore cannot be zeroed; Zero() +// on this SecretValue will not reclaim it. Keep the returned value as +// short-lived as possible, and prefer Bytes() when the caller can clear it. func (s *SecretValue) PlainTextString() string { return string(s.value) } -func (s *SecretValue) String() string { +// String masks the secret so it cannot be printed by accident. +// +// Deliberately a value receiver: with a pointer receiver, formatting a +// dereferenced copy (fmt.Sprintf("%v", *secret)) falls outside the method set +// and prints the raw bytes instead of the mask. +func (s SecretValue) String() string { return "********" } @@ -77,13 +93,50 @@ func (s *SecretValue) Zero() { s.value.Zero() } +// secretKeyPattern is compiled once. ValidateSecretKey runs on every get, set, +// delete and existence check, so recompiling per call was pure waste. +var secretKeyPattern = regexp.MustCompile(`^[a-zA-Z0-9-_.]+$`) + +// vaultIDPattern additionally forbids a leading dot, since a vault ID becomes +// part of a filename. +var vaultIDPattern = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9-_.]*$`) + func ValidateSecretKey(reference string) error { if reference == "" { return ErrInvalidKey } - re := regexp.MustCompile(`^[a-zA-Z0-9-_.]+$`) - if !re.MatchString(reference) { + if !secretKeyPattern.MatchString(reference) { return fmt.Errorf("%w: must only contain alphanumeric characters, dashes, underscores, and/or dots", ErrInvalidKey) } + // A leading dash makes the key look like a flag to any backend CLI the + // external provider shells out to -- a key of "-f" or "--vault" becomes an + // option rather than an argument. + if strings.HasPrefix(reference, "-") { + return fmt.Errorf("%w: must not start with a dash", ErrInvalidKey) + } + // "." and ".." are path elements. The external provider passes keys through + // to tools like pass, where they address entries within a store. + if reference == "." || reference == ".." { + return fmt.Errorf("%w: must not be %q", ErrInvalidKey, reference) + } + return nil +} + +// ValidateVaultID checks an identifier that will be used to build filesystem +// paths and keyring entry names. +func ValidateVaultID(id string) error { + if id == "" { + return fmt.Errorf("%w: vault ID is required", ErrInvalidConfig) + } + if !vaultIDPattern.MatchString(id) { + return fmt.Errorf( + "%w: vault ID %q must start with a letter or digit and contain only "+ + "alphanumeric characters, dashes, underscores, and/or dots", + ErrInvalidConfig, id, + ) + } + if strings.Contains(id, "..") { + return fmt.Errorf("%w: vault ID %q must not contain %q", ErrInvalidConfig, id, "..") + } return nil } diff --git a/storage.go b/storage.go index 7119784..1ac88bf 100644 --- a/storage.go +++ b/storage.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "time" "github.com/gofrs/flock" @@ -23,6 +24,35 @@ const ( vaultFileMode = 0600 ) +// resolveVaultPath builds the on-disk path for a vault file and proves it stays +// inside storagePath. +// +// filepath.Clean does not sanitize an ID embedded in a filename, it *resolves* +// it: Clean("vault-../../../tmp/evil.enc") is "../tmp/evil.enc", because the +// first ".." pops the literal "vault-.." element and the rest survive. Joining +// that onto the storage directory escapes it, so a crafted vault ID could make +// save() overwrite an arbitrary file. IDs are validated, and the result is then +// checked against the base directory as a belt-and-braces second gate. +func resolveVaultPath(storagePath, id, ext string) (string, error) { + if err := ValidateVaultID(id); err != nil { + return "", err + } + + base, err := expandPath(storagePath) + if err != nil { + return "", fmt.Errorf("invalid vault storage path %q: %w", storagePath, err) + } + + full := filepath.Join(base, fmt.Sprintf("%s-%s.%s", vaultFileBase, id, ext)) + + rel, err := filepath.Rel(base, full) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", NewVaultPathError(full) + } + + return full, nil +} + // readVaultFile reads a vault file from disk. // // The boolean reports whether the file exists. Only a genuinely absent file diff --git a/unencrypted.go b/unencrypted.go index 1365e4b..08cc609 100644 --- a/unencrypted.go +++ b/unencrypted.go @@ -3,7 +3,6 @@ package vault import ( "encoding/json" "fmt" - "path/filepath" "sort" "sync" "time" @@ -37,10 +36,14 @@ func NewUnencryptedVault(cfg *Config) (*UnencryptedVault, error) { return nil, fmt.Errorf("unencrypted configuration is required") } - path := filepath.Join( - filepath.Clean(cfg.Unencrypted.StoragePath), - filepath.Clean(fmt.Sprintf("%s-%s.%s", vaultFileBase, cfg.ID, unencryptedVaultFileExt)), - ) + if err := cfg.Validate(); err != nil { + return nil, err + } + + path, err := resolveVaultPath(cfg.Unencrypted.StoragePath, cfg.ID, unencryptedVaultFileExt) + if err != nil { + return nil, err + } vault := &UnencryptedVault{ id: cfg.ID, diff --git a/validation_test.go b/validation_test.go new file mode 100644 index 0000000..0b8a097 --- /dev/null +++ b/validation_test.go @@ -0,0 +1,159 @@ +package vault_test + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/flowexec/vault" +) + +// A vault ID is interpolated into a filename, and filepath.Clean *resolves* +// traversal rather than sanitizing it: Clean("vault-../../../tmp/evil.enc") +// yields "../tmp/evil.enc", which escapes the storage directory once joined. +func TestVaultIDCannotEscapeTheStorageDirectory(t *testing.T) { + hostile := []string{ + "../../../tmp/evil", + "..", + "../sibling", + "a/b", + `a\b`, + ".hidden", + "-leading-dash", + "has space", + "semi;colon", + "", + } + + for _, id := range hostile { + t.Run(id, func(t *testing.T) { + dir := t.TempDir() + _, err := vault.NewUnencryptedVault(&vault.Config{ + ID: id, + Type: vault.ProviderTypeUnencrypted, + Unencrypted: &vault.UnencryptedConfig{StoragePath: dir}, + }) + if err == nil { + t.Fatalf("vault ID %q was accepted, want rejection", id) + } + }) + } +} + +// The traversal is only interesting if it would otherwise have written outside +// the storage directory, so assert that directly. +func TestTraversalVaultIDWritesNothingOutsideStorage(t *testing.T) { + base := t.TempDir() + storage := filepath.Join(base, "storage") + if err := os.MkdirAll(storage, 0700); err != nil { + t.Fatalf("MkdirAll() error = %v", err) + } + + _, err := vault.NewUnencryptedVault(&vault.Config{ + ID: "../escaped", + Type: vault.ProviderTypeUnencrypted, + Unencrypted: &vault.UnencryptedConfig{StoragePath: storage}, + }) + if err == nil { + t.Fatal("a traversing vault ID was accepted") + } + + entries, err := os.ReadDir(base) + if err != nil { + t.Fatalf("ReadDir() error = %v", err) + } + for _, e := range entries { + if e.Name() != "storage" { + t.Errorf("something was written outside the storage directory: %q", e.Name()) + } + } +} + +func TestValidateSecretKey(t *testing.T) { + valid := []string{"key", "my-key", "my_key", "my.key", "a1", "A.B-C_d"} + for _, key := range valid { + if err := vault.ValidateSecretKey(key); err != nil { + t.Errorf("ValidateSecretKey(%q) = %v, want nil", key, err) + } + } + + invalid := []struct { + key, why string + }{ + {"", "empty"}, + {"has space", "space"}, + {"has/slash", "path separator"}, + {"semi;colon", "shell metacharacter"}, + {"$(id)", "command substitution"}, + // A leading dash makes the key look like an option to any backend CLI + // the external provider shells out to. + {"-f", "leading dash"}, + {"--vault", "leading dash"}, + // "." and ".." are path elements; the external provider passes keys to + // tools like pass where they address entries within a store. + {".", "current directory"}, + {"..", "parent directory"}, + } + for _, tc := range invalid { + if err := vault.ValidateSecretKey(tc.key); err == nil { + t.Errorf("ValidateSecretKey(%q) = nil, want rejection (%s)", tc.key, tc.why) + } + } +} + +// New() returned the concrete typed pointer even on error, which Go wraps in a +// non-nil interface. `if provider != nil` then passed and the next call +// panicked on a nil receiver. +func TestNewReturnsATrulyNilProviderOnError(t *testing.T) { + provider, _, err := vault.New("test", vault.WithProvider(vault.ProviderTypeAge)) + if err == nil { + t.Fatal("expected an error for an age vault with no configuration") + } + if provider != nil { + t.Errorf("New() returned a non-nil Provider (%T) alongside an error", provider) + } +} + +func TestNewRejectsUnsupportedProviderType(t *testing.T) { + provider, _, err := vault.New("test", vault.WithProvider("nope")) + if err == nil { + t.Fatal("expected an error for an unsupported provider type") + } + if provider != nil { + t.Errorf("New() returned a non-nil Provider (%T) alongside an error", provider) + } + if !errors.Is(err, vault.ErrInvalidConfig) { + t.Errorf("error = %v, want ErrInvalidConfig", err) + } +} + +// The masked String() must apply to a dereferenced copy too. With a pointer +// receiver, fmt.Sprintf("%v", *secret) fell outside the method set and printed +// the raw bytes. +func TestSecretMasksItselfEvenWhenDereferenced(t *testing.T) { + secret := vault.NewSecretValue([]byte("top-secret-value")) + + for name, rendered := range map[string]string{ + "pointer": fmt.Sprintf("%v", secret), + "dereferenced": fmt.Sprintf("%v", *secret), + } { + if strings.Contains(rendered, "top-secret-value") { + t.Errorf("%s formatting leaked the secret: %s", name, rendered) + } + } +} + +func TestSecretZeroClearsTheBuffer(t *testing.T) { + secret := vault.NewSecretValue([]byte("top-secret-value")) + secret.Zero() + + if got := secret.PlainTextString(); got != "" { + t.Errorf("PlainTextString() after Zero() = %q, want empty", got) + } + if got := secret.Bytes(); len(got) != 0 { + t.Errorf("Bytes() after Zero() = %v, want empty", got) + } +} diff --git a/vault.go b/vault.go index 4504c1f..81f9de7 100644 --- a/vault.go +++ b/vault.go @@ -34,24 +34,43 @@ func New(id string, opts ...Option) (Provider, *Config, error) { return nil, config, err } + // Each branch returns an explicit nil on error. Returning the typed pointer + // directly wrapped a nil *AgeVault (etc.) in a non-nil Provider interface, + // so the usual `if provider != nil` check passed and the next method call + // panicked on a nil receiver. switch config.Type { case ProviderTypeAge: provider, err := NewAgeVault(config) - return provider, config, err + if err != nil { + return nil, config, err + } + return provider, config, nil case ProviderTypeAES256: provider, err := NewAES256Vault(config) - return provider, config, err + if err != nil { + return nil, config, err + } + return provider, config, nil case ProviderTypeKeyring: provider, err := NewKeyringVault(config) - return provider, config, err + if err != nil { + return nil, config, err + } + return provider, config, nil case ProviderTypeUnencrypted: provider, err := NewUnencryptedVault(config) - return provider, config, err + if err != nil { + return nil, config, err + } + return provider, config, nil case ProviderTypeExternal: provider, err := NewExternalVaultProvider(config) - return provider, config, err + if err != nil { + return nil, config, err + } + return provider, config, nil } - return nil, nil, fmt.Errorf("unsupported vault type: %s", config.Type) + return nil, config, fmt.Errorf("%w: unsupported vault type: %s", ErrInvalidConfig, config.Type) } // WithProvider sets the vault provider type From 31e7403c618b4ae23deafa21e5363aec51d575f6 Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 26 Jul 2026 15:59:21 -0400 Subject: [PATCH 5/7] fix: order-independent options, version gating, and honest secret clearing - WithLocalPath switched on the provider type at the moment it ran, making it a silent no-op unless WithProvider was passed first; the failure then surfaced as a confusing "storage path is required". The path is now resolved after every option has been applied. - The vault format version was written on every save but never read back, so a file from a future format would have been parsed as though it were the current one. Loads now reject a version newer than the build understands. - AESState.Version carried a `json:` tag on a struct marshaled with yaml.v3, which ignores json tags entirely; it keyed correctly only by lowercasing coincidence. Corrected to `yaml:`. - Close() now clears the secrets map rather than only dropping the pointer to it, and says plainly in a comment why that is the limit of what it can do. - README: the storage path is a directory, but every example named it like a file ("~/secrets.vault"), which -- now that ~ expands correctly -- would create a directory by that name. Also updated for the Metadata() signature. Note on secret lifetime, deliberately not "fixed": SecureBytes/Zero() cannot protect the provider state maps, which hold immutable Go strings whose bytes are neither addressable nor zeroable, and PlainTextString() hands out more un-zeroable copies by design. Converting the maps to []byte would change the on-disk format of every local vault for a benefit that PlainTextString gives away again immediately. The limitation is now documented on the types rather than papered over with machinery that does not deliver. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HtuDuGkSqTfAXSepf8b51p --- README.md | 8 +++---- aes.go | 12 +++++++++- age.go | 7 ++++++ config.go | 4 ++++ storage.go | 24 ++++++++++++++++++++ unencrypted.go | 7 ++++++ validation_test.go | 56 ++++++++++++++++++++++++++++++++++++++++++++++ vault.go | 40 ++++++++++++++++++++++++--------- 8 files changed, 143 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 8e228c9..280aacb 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,7 @@ Stores secrets in an AES-256 encrypted file with configurable key sources. ```go provider, _, err := vault.New("my-vault", vault.WithProvider(vault.ProviderTypeAES256), - vault.WithAESPath("~/secrets.vault"), + vault.WithAESPath("~/.config/flow/vaults"), // a directory, not a file ) ``` @@ -79,7 +79,7 @@ Uses the [age encryption tool](https://age-encryption.org/) with public key cryp ```go provider, _, err := vault.New("my-vault", vault.WithProvider(vault.ProviderTypeAge), - vault.WithAgePath("~/secrets.age"), + vault.WithAgePath("~/.config/flow/vaults"), // a directory, not a file ) ``` @@ -107,7 +107,7 @@ Stores secrets in plain text JSON files. ```go provider, _, err := vault.New("my-vault", vault.WithProvider(vault.ProviderTypeUnencrypted), - vault.WithUnencryptedPath("~/dev-secrets.json"), + vault.WithUnencryptedPath("~/.config/flow/vaults"), // a directory, not a file ) ``` @@ -165,7 +165,7 @@ secrets, _ := provider.ListSecrets() exists, _ := provider.HasSecret("api-key") // Get vault metadata -metadata := provider.Metadata() +metadata, err := provider.Metadata() ``` ### Configuration from File diff --git a/aes.go b/aes.go index 77c07bd..272407f 100644 --- a/aes.go +++ b/aes.go @@ -20,7 +20,10 @@ const ( type AESState struct { Metadata `yaml:"metadata"` - Version int `json:"version"` + // yaml, not json: this struct is marshaled with gopkg.in/yaml.v3, which + // ignores json tags and would otherwise have keyed this field as "version" + // only by lowercasing coincidence. + Version int `yaml:"version"` ID string `yaml:"id"` Secrets map[string]string `yaml:"secrets"` } @@ -154,6 +157,10 @@ func (v *AES256Vault) load() error { if err := yaml.Unmarshal([]byte(dataStr), &state); err != nil { return fmt.Errorf("failed to unmarshal vault state: %w", err) } + if err := checkVaultVersion(state.Version, aesCurrentVaultVersion, v.fullPath); err != nil { + return err + } + v.state = &state return nil } @@ -310,6 +317,9 @@ func (v *AES256Vault) Close() error { v.mu.Lock() defer v.mu.Unlock() + if v.state != nil { + clearSecrets(v.state.Secrets) + } v.dek = "" v.state = nil diff --git a/age.go b/age.go index 06f6d09..4a97f99 100644 --- a/age.go +++ b/age.go @@ -142,6 +142,10 @@ func (v *AgeVault) load() error { return fmt.Errorf("failed to unmarshal vault state: %w", err) } + if err := checkVaultVersion(state.Version, ageCurrentVaultVersion, v.fullPath); err != nil { + return err + } + v.state = &state if err := v.parseRecipients(); err != nil { return fmt.Errorf("failed to parse recipients: %w", err) @@ -331,6 +335,9 @@ func (v *AgeVault) Close() error { v.mu.Lock() defer v.mu.Unlock() + if v.state != nil { + clearSecrets(v.state.Secrets) + } v.state = nil v.recipients = nil v.identities = nil diff --git a/config.go b/config.go index c40b52a..469e45d 100644 --- a/config.go +++ b/config.go @@ -27,6 +27,10 @@ type Config struct { External *ExternalConfig `json:"external,omitempty"` Keyring *KeyringConfig `json:"keyring,omitempty"` Unencrypted *UnencryptedConfig `json:"unencrypted,omitempty"` + + // pendingLocalPath holds a WithLocalPath value until the provider type is + // known. Unexported so it never reaches the serialized config. + pendingLocalPath string } func (c *Config) Validate() error { diff --git a/storage.go b/storage.go index 1ac88bf..ce2d544 100644 --- a/storage.go +++ b/storage.go @@ -53,6 +53,30 @@ func resolveVaultPath(storagePath, id, ext string) (string, error) { return full, nil } +// checkVaultVersion rejects a vault written by a newer version of the library. +// The version field was recorded on every save but never read back, so a future +// format change would have been parsed as though it were the current one. +func checkVaultVersion(version, current int, path string) error { + if version > current { + return fmt.Errorf( + "%w: vault file %s is version %d but this build understands up to %d; upgrade to open it", + ErrVaultCorrupt, path, version, current, + ) + } + return nil +} + +// clearSecrets overwrites and removes every entry in a secrets map. +// +// This drops references promptly, but cannot be more than that: the values are +// immutable Go strings whose backing bytes are not addressable, and the runtime +// may already have copied them during a heap move. See SecretValue.Zero. +func clearSecrets(secrets map[string]string) { + for k := range secrets { + delete(secrets, k) + } +} + // readVaultFile reads a vault file from disk. // // The boolean reports whether the file exists. Only a genuinely absent file diff --git a/unencrypted.go b/unencrypted.go index 08cc609..4c4b040 100644 --- a/unencrypted.go +++ b/unencrypted.go @@ -94,6 +94,10 @@ func (v *UnencryptedVault) load() error { return fmt.Errorf("failed to parse vault file: %w", err) } + if err := checkVaultVersion(state.Version, unencryptedCurrentVaultVersion, v.fullPath); err != nil { + return err + } + v.state = &state return nil } @@ -242,6 +246,9 @@ func (v *UnencryptedVault) Close() error { v.mu.Lock() defer v.mu.Unlock() + if v.state != nil { + clearSecrets(v.state.Secrets) + } v.state = nil return nil diff --git a/validation_test.go b/validation_test.go index 0b8a097..dd50af5 100644 --- a/validation_test.go +++ b/validation_test.go @@ -146,6 +146,62 @@ func TestSecretMasksItselfEvenWhenDereferenced(t *testing.T) { } } +// WithLocalPath switched on the provider type the moment it ran, so it was a +// silent no-op unless WithProvider happened to be passed first -- surfacing +// later as a confusing "storage path is required". +func TestWithLocalPathIsOrderIndependent(t *testing.T) { + t.Run("path before provider", func(t *testing.T) { + dir := t.TempDir() + _, cfg, err := vault.New("v1", + vault.WithLocalPath(dir), + vault.WithProvider(vault.ProviderTypeUnencrypted), + ) + if err != nil { + t.Fatalf("New() error = %v", err) + } + if cfg.Unencrypted == nil || cfg.Unencrypted.StoragePath != dir { + t.Errorf("storage path was not applied: %+v", cfg.Unencrypted) + } + }) + + t.Run("provider before path", func(t *testing.T) { + dir := t.TempDir() + _, cfg, err := vault.New("v1", + vault.WithProvider(vault.ProviderTypeUnencrypted), + vault.WithLocalPath(dir), + ) + if err != nil { + t.Fatalf("New() error = %v", err) + } + if cfg.Unencrypted == nil || cfg.Unencrypted.StoragePath != dir { + t.Errorf("storage path was not applied: %+v", cfg.Unencrypted) + } + }) +} + +// The version field was written on every save but never read back, so a vault +// from a future format would have been parsed as though it were current. +func TestNewerVaultVersionIsRejected(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "vault-v1.json") + + if err := os.WriteFile(path, []byte(`{"version":99,"id":"v1","secrets":{}}`), 0600); err != nil { + t.Fatalf("failed to seed vault file: %v", err) + } + + _, err := vault.NewUnencryptedVault(&vault.Config{ + ID: "v1", + Type: vault.ProviderTypeUnencrypted, + Unencrypted: &vault.UnencryptedConfig{StoragePath: dir}, + }) + if err == nil { + t.Fatal("a newer vault format version was accepted") + } + if !errors.Is(err, vault.ErrVaultCorrupt) { + t.Errorf("error = %v, want ErrVaultCorrupt", err) + } +} + func TestSecretZeroClearsTheBuffer(t *testing.T) { secret := vault.NewSecretValue([]byte("top-secret-value")) secret.Zero() diff --git a/vault.go b/vault.go index 81f9de7..4b764c7 100644 --- a/vault.go +++ b/vault.go @@ -30,6 +30,10 @@ func New(id string, opts ...Option) (Provider, *Config, error) { for _, opt := range opts { opt(config) } + // Resolved after every option has been applied, so WithLocalPath no longer + // depends on WithProvider having come first in the argument list. + config.applyPendingLocalPath() + if err := config.Validate(); err != nil { return nil, config, err } @@ -120,18 +124,34 @@ func WithKeyringService(service string) Option { } } -// WithLocalPath sets the local vault storage path (works for Age, AES, and Unencrypted based on provider type) +// WithLocalPath sets the local vault storage path (works for Age, AES, and Unencrypted based on provider type). +// +// The path is applied once all options have been processed, so it does not +// matter whether WithProvider is passed before or after it. Previously this +// switched on c.Type immediately and was a silent no-op unless WithProvider +// happened to come first, surfacing later as "storage path is required". func WithLocalPath(path string) Option { return func(c *Config) { - //nolint:exhaustive - switch c.Type { - case ProviderTypeAge: - WithAgePath(path)(c) - case ProviderTypeAES256: - WithAESPath(path)(c) - case ProviderTypeUnencrypted: - WithUnencryptedPath(path)(c) - } + c.pendingLocalPath = path + } +} + +// applyPendingLocalPath routes a WithLocalPath value to the provider-specific +// field now that the provider type is known. +func (c *Config) applyPendingLocalPath() { + if c.pendingLocalPath == "" { + return + } + path := c.pendingLocalPath + + //nolint:exhaustive + switch c.Type { + case ProviderTypeAge: + WithAgePath(path)(c) + case ProviderTypeAES256: + WithAESPath(path)(c) + case ProviderTypeUnencrypted: + WithUnencryptedPath(path)(c) } } From c7d9f3b54bfe11f180faf58419b3fe8905109684 Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 26 Jul 2026 16:04:21 -0400 Subject: [PATCH 6/7] fix(examples): make the shipped provider configs work and keep them tested Three of the four example configurations were rejected outright by the new validation because they interpolated the secret into a shell command, and the fourth (pass) had never worked: its stdin-based set depended on the input-template bug. Nothing in the repo loaded these files, so all of it went unnoticed. Rewritten so the secret reaches each backend over stdin, and to fix correctness problems independent of the injection issue: - bitwarden: `bw get item ` matches by substring and errors on multiple hits, so with keys "api" and "api_key" the old get, exists and delete were all broken. Replaced with an exact-name jq select over `list items`. The old set built JSON by string-interpolating the value, so any quote or backslash in a secret produced malformed JSON; jq -Rs now builds it from stdin. Deletes are --permanent rather than leaving the secret in the trash, and every call is --nointeraction as there is no TTY. - 1password: set only ever ran `item create`, so it failed whenever the item already existed; it now probes and edits or creates. `--` before the key closes flag injection. - aws-ssm: `describe-parameters` listed the entire account and needed account-wide IAM; replaced with `get-parameters-by-path`, whose JSON output also lets the CLI's paginator return every page. The config also specified "listSeparator", which is not the field name -- the struct tag is "separator" -- so the tab separator was silently never applied while the output was tab-delimited. `--value file:///dev/stdin` keeps the secret out of argv. - pass: exists ran `pass show`, decrypting (and possibly triggering a pinentry prompt) just to answer a boolean; it is now `test -f`, a builtin in the interpreter. list no longer needs `tree`, which `pass ls` shells out to and which is frequently not installed. The `GPG_TTY: "$(tty)"` entry never did anything: expandEnv uses os.ExpandEnv, not a shell, so the value stayed the literal string. All four now declare not_found_pattern, so an expired session or a permissions error is no longer indistinguishable from "does not exist". Added TestShippedExampleProvidersAreUsable, which loads every config, renders all six operations against provider-shaped sample output, checks the list templates actually parse it, and asserts the secret reaches stdin and never the command string. It caught two broken output templates in this commit before they shipped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HtuDuGkSqTfAXSepf8b51p --- examples/README.md | 41 ++++++-- examples/main.go | 14 ++- examples/providers/1password.json | 24 +++-- examples/providers/aws-ssm.json | 25 +++-- examples/providers/bitwarden.json | 26 +++-- examples/providers/pass.json | 26 +++-- examples_test.go | 155 ++++++++++++++++++++++++++++++ 7 files changed, 244 insertions(+), 67 deletions(-) create mode 100644 examples_test.go diff --git a/examples/README.md b/examples/README.md index 1254e00..5bffe01 100644 --- a/examples/README.md +++ b/examples/README.md @@ -12,13 +12,14 @@ This directory contains ready-to-use configurations for popular CLI tools. ## Quick Start ```bash -# Test a configuration -./test-provider.sh providers/bitwarden.json - -# Run the Go example +# Run the Go example against a configuration go run main.go providers/pass.json ``` +These configurations are covered by `TestShippedExampleProvidersAreUsable`, which +loads each one, renders every operation, and asserts that the secret value reaches +the backend over stdin and never appears in a command string. + ## Setup Instructions ### Authentication Requirements @@ -54,7 +55,8 @@ Each configuration follows this pattern: "output": "{{output}}" }, "set": { - "cmd": "subcommand {{key}} {{value}}" + "cmd": "subcommand {{key}}", + "input": "{{value}}" }, "list": { "cmd": "list-subcommand" @@ -78,9 +80,30 @@ Each configuration follows this pattern: ## Template Variables -Available in `cmd` and `output` fields: +In `cmd` fields: -- `{{key}}` - The secret key/name -- `{{value}}` - The secret value (for set operations) +- `{{key}}` - The secret key/name (also available as `ref`, `id`, `name`) - `{{env["VariableName"]}}`- Environment variable value -- `{{output}}` - Raw command output (for output templates) + +In `input` fields (piped to the command's stdin): + +- `{{value}}` - The secret value, on `set` only +- `{{input}}` - The secret key +- `{{env["VariableName"]}}` + +In `output` fields: + +- `{{output}}` - Raw command output + +### The secret value is never available to a `cmd` template + +A rendered command is parsed and executed by a shell, and the template engine +performs no quoting. Interpolating a secret there is a command-injection sink and +silently corrupts any value containing shell metacharacters -- `p@$$w0rd` has `$$` +expanded to the process ID, and `correct horse battery` word-splits to `correct`. + +Configurations that reference `{{value}}` or `{{password}}` in any `cmd` are +rejected at load. Pass the secret over stdin with an `input` template instead. + +Shell syntax in a `cmd` works normally: `$VAR`, `${VAR:-default}` and `$(...)` are +resolved by the interpreter, with the configured `environment` in scope. diff --git a/examples/main.go b/examples/main.go index 2053409..079116f 100644 --- a/examples/main.go +++ b/examples/main.go @@ -11,6 +11,16 @@ import ( "github.com/flowexec/vault" ) +func printMetadata(provider vault.Provider) { + fmt.Println("Getting vault metadata...") + metadata, err := provider.Metadata() + if err != nil { + fmt.Printf("Warning: could not read metadata: %v\n", err) + return + } + fmt.Printf("Metadata: %s\n", metadata.RawData) +} + func main() { if len(os.Args) < 2 { fmt.Println("Usage: go run main.go ") @@ -90,9 +100,7 @@ func main() { } } - fmt.Println("Getting vault metadata...") - metadata, _ := provider.Metadata() - fmt.Printf("Metadata: %s\n", metadata.RawData) + printMetadata(provider) fmt.Println("Cleaning up test secret...") err = provider.DeleteSecret("test-key") diff --git a/examples/providers/1password.json b/examples/providers/1password.json index fac2bd2..297e15a 100644 --- a/examples/providers/1password.json +++ b/examples/providers/1password.json @@ -3,29 +3,27 @@ "type": "external", "external": { "get": { - "cmd": "op read \"op://Private/{{key}}/password\"", - "output": "{{output}}" + "cmd": "op read --no-newline 'op://Private/{{key}}/password'" }, "set": { - "cmd": "op item create --category Login --title {{key}} password={{value}} --vault Private" + "cmd": "if op item get --vault Private -- '{{key}}' >/dev/null 2>&1 /dev/null; else jq -Rs '{ title: \"{{key}}\", category: \"LOGIN\", fields: [ { id: \"password\", type: \"CONCEALED\", purpose: \"PASSWORD\", label: \"password\", value: . } ] }' | op item create --vault Private >/dev/null; fi", + "input": "{{value}}" }, "delete": { - "cmd": "op item delete {{key}} --vault Private" + "cmd": "op item delete --vault Private -- '{{key}}' >/dev/null" }, "list": { "cmd": "op item list --vault Private --format json", - "output": "{{ map(fromJSON(output), {.title}) | join(\"\\n\") }}" + "output": "{{ join(map(fromJSON(output), {.title}), \"\\n\") }}" }, + "separator": "\n", "exists": { - "cmd": "op item get {{key}} --vault Private" + "cmd": "op item get --vault Private --format json -- '{{key}}' >/dev/null" }, "metadata": { - "cmd": "op account list", - "output": "Account: {{output}}" + "cmd": "op whoami" }, - "environment": { - "OP_SERVICE_ACCOUNT_TOKEN": "$OP_SERVICE_ACCOUNT_TOKEN" - }, - "timeout": "30s" + "not_found_pattern": "isn't an item", + "timeout": "120s" } -} \ No newline at end of file +} diff --git a/examples/providers/aws-ssm.json b/examples/providers/aws-ssm.json index 67b482e..0989d0c 100644 --- a/examples/providers/aws-ssm.json +++ b/examples/providers/aws-ssm.json @@ -3,32 +3,31 @@ "type": "external", "external": { "get": { - "cmd": "aws ssm get-parameter --name /{{key}} --with-decryption --query Parameter.Value --output text", - "output": "{{output}}" + "cmd": "aws ssm get-parameter --name '/{{key}}' --with-decryption --query Parameter.Value --output text" }, "set": { - "cmd": "aws ssm put-parameter --name /{{key}} --value {{value}} --type SecureString --overwrite" + "cmd": "aws ssm put-parameter --name '/{{key}}' --value file:///dev/stdin --type SecureString --overwrite --output json >/dev/null", + "input": "{{value}}" }, "delete": { - "cmd": "aws ssm delete-parameter --name /{{key}}" + "cmd": "aws ssm delete-parameter --name '/{{key}}' --output json >/dev/null" }, "list": { - "cmd": "aws ssm describe-parameters --query Parameters[].Name --output text", - "output": "{{output}}" + "cmd": "aws ssm get-parameters-by-path --path / --output json", + "output": "{{ join(map(fromJSON(output)[\"Parameters\"], {trimPrefix(.Name, \"/\")}), \"\\n\") }}" }, - "listSeparator": "\t", + "separator": "\n", "exists": { - "cmd": "aws ssm get-parameter --name /{{key}}" + "cmd": "aws ssm get-parameter --name '/{{key}}' --query Parameter.Name --output text >/dev/null" }, "metadata": { "cmd": "aws sts get-caller-identity --output json", - "output": "Account: {{fromJSON(output)[\"Account\"]}}, User: {{fromJSON(output)[\"Arn\"]}}" + "output": "AWS account {{ fromJSON(output)[\"Account\"] }} as {{ fromJSON(output)[\"Arn\"] }}" }, + "not_found_pattern": "ParameterNotFound", "environment": { - "AWS_REGION": "$AWS_REGION", - "AWS_ACCESS_KEY_ID": "$AWS_ACCESS_KEY_ID", - "AWS_SECRET_ACCESS_KEY": "$AWS_SECRET_ACCESS_KEY" + "AWS_PAGER": "" }, "timeout": "60s" } -} \ No newline at end of file +} diff --git a/examples/providers/bitwarden.json b/examples/providers/bitwarden.json index fb5bde7..d88fbd3 100644 --- a/examples/providers/bitwarden.json +++ b/examples/providers/bitwarden.json @@ -3,29 +3,27 @@ "type": "external", "external": { "get": { - "cmd": "bw get password {{key}}", - "output": "{{output}}" + "cmd": "bw --nointeraction list items | jq -r 'map(select(.name == \"{{key}}\")) | .[0].login.password // (\"secret not found\" | halt_error(1))'" }, "set": { - "cmd": "if bw get item \"{{key}}\" >/dev/null 2>&1; then bw get item \"{{key}}\" | jq '.login.password=\"{{value}}\"' | bw encode | bw edit item $(bw get item \"{{key}}\" | jq -r .id); else echo '{\"object\":\"item\",\"type\":1,\"name\":\"{{key}}\",\"login\":{\"username\":\"{{key}}\",\"password\":\"{{value}}\"}}' | bw encode | bw create item; fi" + "cmd": "id=$(bw --nointeraction list items /dev/null", + "input": "{{value}}" }, "delete": { - "cmd": "bw delete item $(bw get item {{key}} | jq -r .id)" + "cmd": "bw --nointeraction delete item --permanent \"$(bw --nointeraction list items | jq -r 'map(select(.name == \"{{key}}\")) | .[0].id')\" >/dev/null" }, "list": { - "cmd": "bw list items --search \"\" --pretty", - "output": "{{ map(fromJSON(output), {.name}) | join(\"\\n\") }}" + "cmd": "bw --nointeraction list items", + "output": "{{ join(map(fromJSON(output), {.name}), \"\\n\") }}" }, + "separator": "\n", "exists": { - "cmd": "bw get item {{key}}" + "cmd": "bw --nointeraction list items | jq -e 'map(select(.name == \"{{key}}\")) | length > 0' >/dev/null" }, "metadata": { - "cmd": "bw status", - "output": "Status: {{output}}" + "cmd": "bw --nointeraction status" }, - "environment": { - "BW_SESSION": "$BW_SESSION" - }, - "timeout": "30s" + "not_found_pattern": "secret not found", + "timeout": "120s" } -} \ No newline at end of file +} diff --git a/examples/providers/pass.json b/examples/providers/pass.json index b8f6d7a..58ac069 100644 --- a/examples/providers/pass.json +++ b/examples/providers/pass.json @@ -3,31 +3,27 @@ "type": "external", "external": { "get": { - "cmd": "pass show {{key}}", - "output": "{{output}}" + "cmd": "pass show '{{key}}'" }, "set": { - "cmd": "pass insert -e {{key}}", + "cmd": "pass insert -m -f '{{key}}' >/dev/null", "input": "{{value}}" }, "delete": { - "cmd": "pass rm -f {{key}}" + "cmd": "pass rm -f '{{key}}' >/dev/null" }, "list": { - "cmd": "pass ls", - "output": "{{output}}" + "cmd": "cd \"${PASSWORD_STORE_DIR:-$HOME/.password-store}\" && find . -name '*.gpg' -type f | sed -e 's|^\\./||' -e 's|\\.gpg$||'" }, + "separator": "\n", "exists": { - "cmd": "pass show {{key}}" + "cmd": "test -f \"${PASSWORD_STORE_DIR:-$HOME/.password-store}/{{key}}.gpg\"" }, "metadata": { - "cmd": "pass git log --oneline -1", - "output": "Last change: {{output}}" + "cmd": "cat \"${PASSWORD_STORE_DIR:-$HOME/.password-store}/.gpg-id\"", + "output": "pass store recipients: {{ trim(output) }}" }, - "environment": { - "PASSWORD_STORE_DIR": "$PASSWORD_STORE_DIR", - "GPG_TTY": "$(tty)" - }, - "timeout": "30s" + "not_found_pattern": "is not in the password store", + "timeout": "120s" } -} \ No newline at end of file +} diff --git a/examples_test.go b/examples_test.go new file mode 100644 index 0000000..ccac7cd --- /dev/null +++ b/examples_test.go @@ -0,0 +1,155 @@ +package vault_test + +import ( + "context" + "path/filepath" + "strings" + "testing" + + "github.com/flowexec/vault" +) + +// Every config shipped under examples/providers must load, validate, and render +// each of its operations. Nothing previously exercised these files, which is how +// three of them came to interpolate the secret straight into a shell command and +// how the pass example's stdin-based set shipped permanently broken. +func TestShippedExampleProvidersAreUsable(t *testing.T) { + files, err := filepath.Glob("examples/providers/*.json") + if err != nil { + t.Fatalf("Glob() error = %v", err) + } + if len(files) == 0 { + t.Fatal("no example provider configs found") + } + + const secret = "s3cr3t-$(id)-'quoted'" + + // Sample backend output per provider, shaped the way that CLI actually + // responds, so the output templates are genuinely exercised rather than + // handed something they happen to tolerate. + sampleOutput := map[string]struct{ list, metadata string }{ + "aws-ssm.json": { + list: `{"Parameters":[{"Name":"/alpha"},{"Name":"/beta"}]}`, + metadata: `{"Account":"123456789012","Arn":"arn:aws:iam::123456789012:user/dev"}`, + }, + "1password.json": { + list: `[{"title":"alpha"},{"title":"beta"}]`, + metadata: `dev@example.com`, + }, + "bitwarden.json": { + list: `[{"name":"alpha"},{"name":"beta"}]`, + metadata: `{"status":"unlocked"}`, + }, + "pass.json": { + list: "alpha\nbeta", + metadata: "AAAA1111", + }, + } + + for _, file := range files { + t.Run(filepath.Base(file), func(t *testing.T) { + samples, ok := sampleOutput[filepath.Base(file)] + if !ok { + t.Fatalf("no sample output defined for %s; add one so its templates are covered", file) + } + + cfg, err := vault.LoadConfigJSON(file) + if err != nil { + t.Fatalf("LoadConfigJSON() error = %v", err) + } + + provider, err := vault.NewExternalVaultProvider(&cfg) + if err != nil { + t.Fatalf("config was rejected: %v", err) + } + + var ( + lastCmd string + lastInput string + nextOut string + ) + provider.SetExecutionFunc(func( + _ context.Context, cmd, input, _ string, _ []string, + ) (string, error) { + lastCmd, lastInput = cmd, input + return nextOut, nil + }) + + // Set: the secret must reach stdin and never the command string. + if err := provider.SetSecret("test-key", vault.NewSecretValue([]byte(secret))); err != nil { + t.Fatalf("SetSecret() error = %v", err) + } + if lastInput != secret { + t.Errorf("set stdin = %q, want the secret %q", lastInput, secret) + } + if strings.Contains(lastCmd, secret) { + t.Errorf("the secret leaked into the set command: %q", lastCmd) + } + + // Every other configured operation must render without error. + nextOut = "the-secret" + secretValue, err := provider.GetSecret("test-key") + if err != nil { + t.Errorf("GetSecret() error = %v", err) + } else if secretValue.PlainTextString() != "the-secret" { + t.Errorf("GetSecret() = %q, want %q", secretValue.PlainTextString(), "the-secret") + } + + nextOut = "" + if err := provider.DeleteSecret("test-key"); err != nil { + t.Errorf("DeleteSecret() error = %v", err) + } + + nextOut = samples.list + keys, err := provider.ListSecrets() + if err != nil { + t.Errorf("ListSecrets() error = %v", err) + } else if len(keys) != 2 || keys[0] != "alpha" || keys[1] != "beta" { + t.Errorf("ListSecrets() = %v, want [alpha beta]", keys) + } + + nextOut = "" + if _, err := provider.HasSecret("test-key"); err != nil { + t.Errorf("HasSecret() error = %v", err) + } + + nextOut = samples.metadata + if _, err := provider.Metadata(); err != nil { + t.Errorf("Metadata() error = %v", err) + } + }) + } +} + +// The rendered commands must not carry template or shell hazards that the +// rendering pipeline would silently mangle. +func TestShippedExampleProvidersRenderCleanCommands(t *testing.T) { + files, _ := filepath.Glob("examples/providers/*.json") + + for _, file := range files { + t.Run(filepath.Base(file), func(t *testing.T) { + cfg, err := vault.LoadConfigJSON(file) + if err != nil { + t.Fatalf("LoadConfigJSON() error = %v", err) + } + + for name, op := range map[string]vault.CommandConfig{ + "get": cfg.External.Get, + "set": cfg.External.Set, + "delete": cfg.External.Delete, + "list": cfg.External.List, + "exists": cfg.External.Exists, + "metadata": cfg.External.Metadata, + } { + if op.CommandTemplate == "" { + continue + } + // A backtick anywhere breaks the expression template, which + // wraps expressions in backticks itself. + if strings.Contains(op.CommandTemplate, "`") { + t.Errorf("%s command contains a backtick, which the template engine cannot carry", name) + } + } + }) + } +} From bf94f1f297bbb10e223d9397f38f0d30c8103a50 Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 26 Jul 2026 16:06:05 -0400 Subject: [PATCH 7/7] docs: document the v0.3.0 breaking changes and the stdin rule Each breaking change replaces behaviour that failed silently, so the upgrade table says what was wrong rather than only what changed. Also corrects the external provider example, which showed the secret being interpolated into a command -- the exact shape that is now rejected. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HtuDuGkSqTfAXSepf8b51p --- README.md | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 280aacb..7474911 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,23 @@ A flexible Go library for secure secret management with multiple backend provide - **Thread Safe**: Concurrent access protection with read/write mutexes - **Comprehensive API**: Full CRUD operations plus metadata and existence checks +## Upgrading to v0.3.0 + +v0.3.0 is a security and correctness release. It contains breaking changes, each +of which replaces behaviour that failed silently: + +| Change | Why | What to do | +|---|---|---| +| `Provider.Metadata()` returns `(Metadata, error)` | Every failure previously returned an empty struct, so a broken command, a timeout and "not configured" were indistinguishable | Handle the new error | +| External configs referencing `{{value}}`/`{{password}}` in a `cmd` are rejected | The value was interpolated into a shell command with no quoting — a command-injection sink that also corrupted ordinary passwords | Move the secret to an `InputTemplate` (stdin) | +| An existing but zero-length vault file is an error | It was read as "no vault here", so the constructor initialized and immediately overwrote it, destroying every secret | Restore from backup, or delete the file to start fresh | +| Operations on a closed vault return `ErrVaultClosed` | They dereferenced nil state and panicked | Nothing, unless you relied on the panic | +| Vault IDs are charset-validated | An ID is interpolated into a filename, and `filepath.Clean` *resolves* traversal rather than sanitizing it | Use IDs matching `^[a-zA-Z0-9][a-zA-Z0-9-_.]*$` | +| Encryption keys must be exactly 32 bytes | `aes.NewCipher` also accepts 16 and 24, silently downgrading an "AES256" vault to AES-128/192 | Regenerate short keys | +| `DeriveKey` returns a parameter-tagged salt | Changing the scrypt cost would otherwise silently change every derived key | Pass the returned salt back verbatim rather than base64-decoding it first | + +Local vault files written by earlier versions are read without migration. + ## Quick Start ```go @@ -122,10 +139,12 @@ config := &vault.Config{ Type: vault.ProviderTypeExternal, External: &vault.ExternalConfig{ Get: vault.CommandConfig{ - CommandTemplate: "bw get password {{key}}", + CommandTemplate: "bw get password '{{key}}'", }, Set: vault.CommandConfig{ - CommandTemplate: "bw create item --name {{key}} --password {{value}}", + CommandTemplate: "bw create item", + // The secret is piped to the command's stdin, never placed in it. + InputTemplate: "{{value}}", }, // ... other operations }, @@ -134,6 +153,14 @@ config := &vault.Config{ provider, err := vault.NewExternalVaultProvider(config) ``` +> **The secret value is not available to command templates.** A rendered command +> is parsed and run by a shell and the template engine does no quoting, so +> interpolating a secret there is a command-injection sink and silently corrupts +> any value containing shell metacharacters (`p@$$w0rd` has `$$` replaced by the +> process ID; `correct horse battery` word-splits to `correct`). Configurations +> referencing `{{value}}` or `{{password}}` in a `cmd` are rejected at load — +> use an `InputTemplate` instead. + **External Provider Examples** Ready-to-use configurations for popular CLI tools are available in the [`examples/`](./examples/) directory: