diff --git a/README.md b/README.md index d64a5de..9bcd9fb 100644 --- a/README.md +++ b/README.md @@ -113,7 +113,9 @@ automatically switch to another vault if removing the current one. Adds a secret to the current vault. Reads the secret value from stdin. - `--force, -f`: Overwrite existing secret -**Secret Name Format:** `[a-z0-9\.\-\_\/]+` +**Secret Name Format:** only ASCII letters, digits, `.`, `-`, `_` and `/` +are allowed, and a name must not be empty, start with `.` or `/`, end with +`/`, contain `//`, or have `..` as a path segment. - Forward slashes (`/`) are converted to percent signs (`%`) for storage - Examples: `database/password`, `api.key`, `ssh_private_key` diff --git a/TODO.md b/TODO.md index 38d9581..e4781c4 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,13 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-03: Every command that builds a path from a secret name + checks the name first with `vault.ValidateSecretName` and touches + nothing when it is invalid: `rm`, `mv` (both names, within a vault + and between vaults, before switching the current vault), `import`, + `version list`/`promote`/`rm`, `encrypt` and `decrypt`. The error + and `README.md` state the naming rule. Before, `secret rm ..` + deleted the whole vault and `secret rm .` every secret in it. - 2026-10-03: The keychain unlocker's age key passphrase stays in locked memory: it is generated into a locked buffer, and the keychain JSON is written and read by `KeychainData` code in @@ -100,8 +107,7 @@ Bring the repo into policy compliance in one commit: to GPGEncryptFunc and EncryptWithPassphrase. - Race conditions: no file locking in vault/secrets.go:142-176; non-atomic writes can leave the vault inconsistent. - - Input validation: dots in secret names risk path traversal - (vault/secrets.go:75-99); no maximum secret size (DoS). + - Input validation: no maximum secret size (DoS). - Timing attacks: bytes.Equal passphrase compare (cli/init.go: 209-216); non-constant-time public key compare (vault.go:95-100). - High priority: diff --git a/internal/cli/crypto.go b/internal/cli/crypto.go index 723b472..5a89414 100644 --- a/internal/cli/crypto.go +++ b/internal/cli/crypto.go @@ -122,6 +122,11 @@ func (cli *Instance) resolveEncryptionKey( // Encrypt encrypts data using an age secret key stored in a secret func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error { + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { @@ -191,6 +196,11 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error { // Decrypt decrypts data using an age secret key stored in a secret func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error { + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { diff --git a/internal/cli/path_traversal_test.go b/internal/cli/path_traversal_test.go new file mode 100644 index 0000000..35889d8 --- /dev/null +++ b/internal/cli/path_traversal_test.go @@ -0,0 +1,267 @@ +package cli_test + +import ( + "maps" + "os" + "slices" + "strings" + "testing" + + "git.eeqj.de/sneak/secret/internal/cli" + "git.eeqj.de/sneak/secret/internal/secret" + "git.eeqj.de/sneak/secret/internal/vault" + "github.com/awnumar/memguard" + "github.com/spf13/afero" + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" +) + +const ( + // testStateDir is the in-memory state directory of the test vaults. + testStateDir = "/test/state" + + // testPassphrase protects the passphrase unlocker of each test vault. + testPassphrase = "test-passphrase" + + // testVersion is a version name in the format the vault uses. + testVersion = "20260101.001" + + // missingFile is an import source that does not exist, so an import + // that opened it before checking the name would fail with another error. + missingFile = "/no/such/file" +) + +// newTwoVaultFs returns an in-memory filesystem holding the vaults "work" +// and "default", the current one. Each holds the secret "x" and a +// passphrase unlocker, so both secrets.d and unlockers.d have contents. +// +//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout +func newTwoVaultFs(t *testing.T) afero.Fs { + t.Helper() + + t.Setenv(secret.EnvMnemonic, testMnemonic) + + fs := afero.NewMemMapFs() + + for _, name := range []string{"work", "default"} { + vlt, err := vault.CreateVault(fs, testStateDir, name) + require.NoError(t, err) + + err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("value")), false) + require.NoError(t, err) + + _, err = vlt.CreatePassphraseUnlocker( + memguard.NewBufferFromBytes([]byte(testPassphrase))) + require.NoError(t, err) + } + + return fs +} + +// snapshotStateDir maps every file under the state directory to its +// contents, and every directory, written with a trailing "/", to "". Two +// snapshots are equal only if nothing in it was added, removed or changed. +func snapshotStateDir(t *testing.T, fs afero.Fs) map[string]string { + t.Helper() + + tree := map[string]string{} + + err := afero.Walk(fs, testStateDir, func( + path string, info os.FileInfo, err error, + ) error { + if err != nil { + return err + } + + if info.IsDir() { + tree[path+"/"] = "" + + return nil + } + + content, err := afero.ReadFile(fs, path) + if err != nil { + return err + } + + tree[path] = string(content) + + return nil + }) + require.NoError(t, err) + + return tree +} + +// newFsFromSnapshot returns a new in-memory filesystem holding exactly the +// directories and files recorded by snapshotStateDir. +// +//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout +func newFsFromSnapshot(t *testing.T, tree map[string]string) afero.Fs { + t.Helper() + + fs := afero.NewMemMapFs() + + // In sorted order every directory comes before its contents. + for _, path := range slices.Sorted(maps.Keys(tree)) { + dir, isDir := strings.CutSuffix(path, "/") + if isDir { + require.NoError(t, fs.MkdirAll(dir, secret.DirPerms)) + + continue + } + + err := afero.WriteFile(fs, path, []byte(tree[path]), secret.FilePerms) + require.NoError(t, err) + } + + return fs +} + +// requireRejectedAndUnchanged runs a command on a copy of the state +// directory recorded in before. It requires exactly the error +// vault.ValidateSecretName gives for the rejected name, so that a later +// check rejecting the name does not count, and everything under the state +// directory as it was: the error alone proves nothing, since it could come +// after the vault had already been deleted. +func requireRejectedAndUnchanged( + t *testing.T, before map[string]string, rejected string, + run func(c *cli.Instance) error, +) { + t.Helper() + + fs := newFsFromSnapshot(t, before) + + err := run(cli.NewCLIInstanceWithStateDir(fs, testStateDir)) + + require.Equal(t, before, snapshotStateDir(t, fs)) + require.ErrorIs(t, err, vault.ErrInvalidSecretName) + require.EqualError(t, err, vault.ValidateSecretName(rejected).Error()) +} + +// TestInvalidSecretNameLeavesVaultsUnchanged is a regression test for +// https://git.eeqj.de/sneak/secret/issues/33, where `secret rm ..` deleted +// the whole vault, and `secret rm .` or `secret rm ""` every secret in it. +// Moves and imports use --force, so that only the name check stands in +// the way. +// +//nolint:paralleltest // newTwoVaultFs uses t.Setenv +func TestInvalidSecretNameLeavesVaultsUnchanged(t *testing.T) { + // Creating a passphrase unlocker is slow by design, so the vaults are + // created once and each case runs on its own copy of them. + before := snapshotStateDir(t, newTwoVaultFs(t)) + + vaultDir := testStateDir + "/vaults.d/default" + require.Contains(t, before, vaultDir+"/secrets.d/x/") + require.Contains(t, before, vaultDir+"/unlockers.d/passphrase/") + require.Equal(t, "default", before[testStateDir+"/currentvault"]) + + cmd := &cobra.Command{} + + tests := []struct { + command string + rejected string // the secret name the command must reject + run func(c *cli.Instance) error + }{ + {"rm ..", "..", func(c *cli.Instance) error { + return c.RemoveSecret(cmd, "..", false) + }}, + {"rm .", ".", func(c *cli.Instance) error { + return c.RemoveSecret(cmd, ".", false) + }}, + {`rm ""`, "", func(c *cli.Instance) error { + return c.RemoveSecret(cmd, "", false) + }}, + {"rm ../../etc", "../../etc", func(c *cli.Instance) error { + return c.RemoveSecret(cmd, "../../etc", false) + }}, + {"mv --force .. x", "..", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "..", "x", true) + }}, + {"mv --force x ..", "..", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "x", "..", true) + }}, + {`mv --force x ""`, "", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "x", "", true) + }}, + // "work" is not the current vault: a move within it must not + // select it when a name is rejected. + {"mv --force work:.. work:x", "..", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "work:..", "work:x", true) + }}, + {"mv --force work:x work:..", "..", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "work:x", "work:..", true) + }}, + {"mv --force default:.. work", "..", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "default:..", "work", true) + }}, + {"mv --force default:.. work:y", "..", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "default:..", "work:y", true) + }}, + {"mv --force default:x work:..", "..", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "default:x", "work:..", true) + }}, + {"import --force ..", "..", func(c *cli.Instance) error { + return c.ImportSecret(cmd, "..", missingFile, true) + }}, + {"import --force .", ".", func(c *cli.Instance) error { + return c.ImportSecret(cmd, ".", missingFile, true) + }}, + {"import --force ../../etc", "../../etc", func(c *cli.Instance) error { + return c.ImportSecret(cmd, "../../etc", missingFile, true) + }}, + {"version list ..", "..", func(c *cli.Instance) error { + return c.ListVersions(cmd, "..") + }}, + {"version promote ..", "..", func(c *cli.Instance) error { + return c.PromoteVersion(cmd, "..", testVersion) + }}, + {"version rm ..", "..", func(c *cli.Instance) error { + return c.RemoveVersion(cmd, "..", testVersion) + }}, + {"encrypt ..", "..", func(c *cli.Instance) error { + return c.Encrypt("..", "", "") + }}, + {"decrypt ..", "..", func(c *cli.Instance) error { + return c.Decrypt("..", "", "") + }}, + } + + for _, tt := range tests { + t.Run(tt.command, func(t *testing.T) { + requireRejectedAndUnchanged(t, before, tt.rejected, tt.run) + }) + } +} + +// TestMoveToVaultNameRenamesInCurrentVault checks that `secret mv x work`, +// where "work" is also the name of a vault, renames the secret "x" to "work" +// in the current vault and changes nothing else. +// +//nolint:paralleltest // newTwoVaultFs uses t.Setenv +func TestMoveToVaultNameRenamesInCurrentVault(t *testing.T) { + before := snapshotStateDir(t, newTwoVaultFs(t)) + fs := newFsFromSnapshot(t, before) + + c := cli.NewCLIInstanceWithStateDir(fs, testStateDir) + err := c.MoveSecret(&cobra.Command{}, "x", "work", false) + require.NoError(t, err) + + // Expected: the state as before, with everything under the current + // vault's secrets.d/x/ now under secrets.d/work/. + oldDir := testStateDir + "/vaults.d/default/secrets.d/x/" + newDir := testStateDir + "/vaults.d/default/secrets.d/work/" + want := map[string]string{} + + for path, content := range before { + rest, found := strings.CutPrefix(path, oldDir) + if found { + path = newDir + rest + } + + want[path] = content + } + + require.Contains(t, want, newDir) + require.Equal(t, want, snapshotStateDir(t, fs)) +} diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index f62ecf0..7fbf6f0 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -603,6 +603,11 @@ func printSecretsTable( func (cli *Instance) ImportSecret( cmd *cobra.Command, secretName, sourceFile string, force bool, ) error { + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { @@ -649,6 +654,11 @@ func (cli *Instance) ImportSecret( // RemoveSecret removes a secret from the vault func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool) error { + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { @@ -702,13 +712,8 @@ func (cli *Instance) MoveSecret( srcVaultName, srcSecretName, srcQualified := ParseVaultSecretRef(source) destVaultName, destSecretName, destQualified := ParseVaultSecretRef(dest) - // If neither is qualified, this is a simple within-vault rename - if !srcQualified && !destQualified { - return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force) - } - // Cross-vault move requires source to be qualified - if !srcQualified { + if !srcQualified && destQualified { return errCrossVaultSourceUnqualified } @@ -716,31 +721,46 @@ func (cli *Instance) MoveSecret( // Format: "work:secret default" means move to vault "default" // Format: "work:secret default:newname" means move to vault "default" // with a new name - if !destQualified { + if srcQualified && !destQualified { // Check if dest is actually a vault name vaults, err := vault.ListVaults(cli.fs, cli.stateDir) if err == nil && slices.Contains(vaults, dest) { // dest is a vault name, use source secret name destVaultName = dest destSecretName = srcSecretName - } - - // If destVaultName is still empty, dest is a secret name in source vault - if destVaultName == "" { + } else { + // dest is a secret name in source vault destVaultName = srcVaultName - destSecretName = dest } } - // If destination secret name is empty, use source secret name - if destSecretName == "" { + // If destination secret name is empty, use source secret name. A plain + // rename keeps it empty, so that the check below rejects it. + if srcQualified && destSecretName == "" { destSecretName = srcSecretName } + // Check both names, for every form of the move, before selecting a vault + // below, so that a rejected move leaves the current vault as it was. + err := vault.ValidateSecretName(srcSecretName) + if err != nil { + return err + } + + err = vault.ValidateSecretName(destSecretName) + if err != nil { + return err + } + + // If neither is qualified, this is a simple within-vault rename + if !srcQualified && !destQualified { + return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force) + } + // Same vault? Use simple rename if possible (optimization) if srcVaultName == destVaultName { // Select the vault and do a simple move - err := vault.SelectVault(cli.fs, cli.stateDir, srcVaultName) + err = vault.SelectVault(cli.fs, cli.stateDir, srcVaultName) if err != nil { return fmt.Errorf("failed to select vault '%s': %w", srcVaultName, err) } @@ -753,7 +773,8 @@ func (cli *Instance) MoveSecret( cmd, srcVaultName, srcSecretName, destVaultName, destSecretName, force) } -// moveSecretWithinVault handles rename within the current vault +// moveSecretWithinVault handles rename within the current vault. Its caller, +// MoveSecret, has already checked both secret names. func (cli *Instance) moveSecretWithinVault( cmd *cobra.Command, source, dest string, force bool, ) error { @@ -808,7 +829,8 @@ func (cli *Instance) moveSecretWithinVault( return nil } -// moveSecretCrossVault handles moving between different vaults +// moveSecretCrossVault handles moving between different vaults. Its caller, +// MoveSecret, has already checked both secret names. func (cli *Instance) moveSecretCrossVault( cmd *cobra.Command, srcVaultName, srcSecretName, diff --git a/internal/cli/version.go b/internal/cli/version.go index 17f113f..feb04d9 100644 --- a/internal/cli/version.go +++ b/internal/cli/version.go @@ -112,6 +112,11 @@ func VersionCommands(cli *Instance) *cobra.Command { func (cli *Instance) ListVersions(cmd *cobra.Command, secretName string) error { secret.Debug("ListVersions called", "secret_name", secretName) + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { @@ -239,6 +244,11 @@ func formatVersionTime(t *time.Time) string { func (cli *Instance) PromoteVersion( cmd *cobra.Command, secretName string, version string, ) error { + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { @@ -282,6 +292,11 @@ func (cli *Instance) PromoteVersion( func (cli *Instance) RemoveVersion( cmd *cobra.Command, secretName string, version string, ) error { + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { diff --git a/internal/secret/secret_test.go b/internal/secret/secret_test.go index 81b6fd2..e76dd63 100644 --- a/internal/secret/secret_test.go +++ b/internal/secret/secret_test.go @@ -310,64 +310,6 @@ func TestPerSecretKeyFunctionality(t *testing.T) { }) } -// For testing purposes only -func isValidSecretName(name string) bool { - if name == "" { - return false - } - // Valid characters for secret names: letters, numbers, dash, dot, underscore, slash - for _, char := range name { - if (char < 'a' || char > 'z') && // lowercase letters - (char < 'A' || char > 'Z') && // uppercase letters - (char < '0' || char > '9') && // numbers - char != '-' && // dash - char != '.' && // dot - char != '_' && // underscore - char != '/' { // slash - return false - } - } - - return true -} - -func TestSecretNameValidation(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - valid bool - }{ - {"valid-name", true}, - {"valid.name", true}, - {"valid_name", true}, - {"valid/path/name", true}, - {"123valid", true}, - {"", false}, - {"Valid-Upper-Name", true}, // uppercase allowed - {"2025-11-21-ber1app1-vaultik-test-bucket-AKI", true}, // real-world uppercase key ID - {"MixedCase/Path/Name", true}, // mixed case with path - {"invalid name", false}, // space not allowed - {"invalid@name", false}, // @ not allowed - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - - result := isValidSecretName(test.name) - if result != test.valid { - t.Errorf( - "isValidSecretName(%q) = %v, want %v", - test.name, - result, - test.valid, - ) - } - }) - } -} - func TestSecretGetValueWithEnvMnemonicUsesVaultDerivationIndex(t *testing.T) { // This test demonstrates the bug where GetValue uses hardcoded index 0 // instead of the vault's actual derivation index when using environment mnemonic diff --git a/internal/vault/errors.go b/internal/vault/errors.go index 7d51e2e..2fb57ee 100644 --- a/internal/vault/errors.go +++ b/internal/vault/errors.go @@ -29,10 +29,11 @@ var ( // ErrNilValueBuffer indicates a nil value buffer was supplied. ErrNilValueBuffer = errors.New("value buffer is nil") - // ErrInvalidSecretName indicates a secret name that does not match - // the allowed pattern [a-z0-9.\-_/]+. Composed as - // "invalid secret name '': must match pattern [a-z0-9.\-_/]+", - // or as "invalid secret name: " by GetSecretObject. + // ErrInvalidSecretName indicates a secret name that breaks the naming + // rule: only ASCII letters, digits, '.', '-', '_' and '/'; not empty; + // no leading '.' or '/', no trailing '/', no '//', no '..' path segment. + // Composed by ValidateSecretName as + // "invalid secret name '': ". ErrInvalidSecretName = errors.New("invalid secret name") // ErrSecretExists indicates the secret already exists and --force diff --git a/internal/vault/secrets.go b/internal/vault/secrets.go index b8ab559..1ae7769 100644 --- a/internal/vault/secrets.go +++ b/internal/vault/secrets.go @@ -79,6 +79,7 @@ func (v *Vault) ListSecrets() ([]string, error) { // - No leading or trailing slashes // - No double slashes // - No names starting with dots +// - No ".." path segments func isValidSecretName(name string) bool { if name == "" { return false @@ -110,6 +111,22 @@ func isValidSecretName(name string) bool { return matched } +// ValidateSecretName returns an error wrapping ErrInvalidSecretName when +// name is not a valid secret name. Call it on the name exactly as the user +// gave it, before building any path from it. +func ValidateSecretName(name string) error { + if !isValidSecretName(name) { + return fmt.Errorf( + "%w '%s': only ASCII letters, digits, '.', '-', '_' and '/' are allowed, "+ + "and a name must not be empty, start with '.' or '/', end with '/', "+ + "contain '//', or have '..' as a path segment", + ErrInvalidSecretName, name, + ) + } + + return nil +} + // AddSecret adds a secret to this vault func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool) error { if value == nil { @@ -124,13 +141,11 @@ func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool) ) // Validate secret name - if !isValidSecretName(name) { + err := ValidateSecretName(name) + if err != nil { secret.Debug("Invalid secret name provided", "secret_name", name) - return fmt.Errorf( - "%w '%s': must match pattern [a-z0-9.\\-_/]+", - ErrInvalidSecretName, name, - ) + return err } secret.Debug("Secret name validation passed", "secret_name", name) @@ -358,8 +373,9 @@ func (v *Vault) UnlockVault() (*age.X25519Identity, error) { // GetSecretObject retrieves a Secret object with metadata loaded from this vault func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) { - if !isValidSecretName(name) { - return nil, fmt.Errorf("%w: %s", ErrInvalidSecretName, name) + err := ValidateSecretName(name) + if err != nil { + return nil, err } // First check if the secret exists by checking for the metadata file @@ -628,13 +644,11 @@ func (v *Vault) updatePreviousVersion( // version exist, and resolves an empty version to the current one. func (v *Vault) resolveSecretVersion(name, version string) (string, error) { // Validate secret name to prevent path traversal - if !isValidSecretName(name) { + err := ValidateSecretName(name) + if err != nil { secret.Debug("Invalid secret name provided", "secret_name", name) - return "", fmt.Errorf( - "%w '%s': must match pattern [a-z0-9.\\-_/]+", - ErrInvalidSecretName, name, - ) + return "", err } // Get vault directory