Accept a version argument only if the secret has that version (closes #67) #77

Merged
clawbot merged 1 commits from issue-67-check-version-argument into next 2026-10-04 03:25:01 +02:00
11 changed files with 193 additions and 73 deletions
+7
View File
@@ -25,6 +25,13 @@ Bring the repo into policy compliance in one commit:
# Completed Steps # Completed Steps
- 2026-10-03: `version rm`, `version promote` and `get --version`
accept a version only if it is one of the versions `version list`
lists for that secret, compared as typed before any path is built
(`secret.VersionExists`), and touch nothing otherwise. An empty
`--version` is rejected instead of meaning the current version.
Before, `secret version rm x ../../..` deleted the whole vault,
`secret version rm x ..` the secret, and `.` or `""` every version.
- 2026-10-03: Key material is wiped on every exit: `Entry()` returns - 2026-10-03: Key material is wiped on every exit: `Entry()` returns
the exit code after its deferred `memguard.Purge()` has run, and only the exit code after its deferred `memguard.Purge()` has run, and only
`main` calls `os.Exit`. SIGINT and SIGTERM go through memguard's `main` calls `os.Exit`. SIGINT and SIGTERM go through memguard's
+8
View File
@@ -829,6 +829,14 @@ func test09GetSpecificVersion(t *testing.T, tempDir, testMnemonic string, runSec
require.NoError(t, err, "get current version should succeed") require.NoError(t, err, "get current version should succeed")
assert.Equal(t, "newpassword456", strings.TrimSpace(output), "should return new secret value without --version") assert.Equal(t, "newpassword456", strings.TrimSpace(output), "should return new secret value without --version")
// An empty --version is not a version; it does not mean the current one
output, err = runSecretWithEnv(map[string]string{
secret.EnvMnemonic: testMnemonic,
}, "get", "--version", "", "database/password")
require.Error(t, err, "get with an empty version should fail")
assert.Contains(t, output, "version '' not found", "should reject the empty version")
} }
func test10PromoteVersion(t *testing.T, tempDir, testMnemonic string, runSecret func(...string) (string, error), runSecretWithEnv func(map[string]string, ...string) (string, error)) { func test10PromoteVersion(t *testing.T, tempDir, testMnemonic string, runSecret func(...string) (string, error), runSecretWithEnv func(map[string]string, ...string) (string, error)) {
+90 -9
View File
@@ -1,6 +1,7 @@
package cli_test package cli_test
import ( import (
"fmt"
"maps" "maps"
"os" "os"
"slices" "slices"
@@ -119,13 +120,13 @@ func newFsFromSnapshot(t *testing.T, tree map[string]string) afero.Fs {
} }
// requireRejectedAndUnchanged runs a command on a copy of the state // requireRejectedAndUnchanged runs a command on a copy of the state
// directory recorded in before. It requires exactly the error // directory recorded in before. It requires an error with exactly the
// vault.ValidateSecretName gives for the rejected name, so that a later // message of want, so that a later check rejecting the argument does not
// check rejecting the name does not count, and everything under the state // count, and everything under the state directory as it was: the error
// directory as it was: the error alone proves nothing, since it could come // alone proves nothing, since it could come after the vault had already
// after the vault had already been deleted. // been deleted.
func requireRejectedAndUnchanged( func requireRejectedAndUnchanged(
t *testing.T, before map[string]string, rejected string, t *testing.T, before map[string]string, want error,
run func(c *cli.Instance) error, run func(c *cli.Instance) error,
) { ) {
t.Helper() t.Helper()
@@ -135,8 +136,7 @@ func requireRejectedAndUnchanged(
err := run(cli.NewCLIInstanceWithStateDir(fs, testStateDir)) err := run(cli.NewCLIInstanceWithStateDir(fs, testStateDir))
require.Equal(t, before, snapshotStateDir(t, fs)) require.Equal(t, before, snapshotStateDir(t, fs))
require.ErrorIs(t, err, vault.ErrInvalidSecretName) require.EqualError(t, err, want.Error())
require.EqualError(t, err, vault.ValidateSecretName(rejected).Error())
} }
// TestInvalidSecretNameLeavesVaultsUnchanged is a regression test for // TestInvalidSecretNameLeavesVaultsUnchanged is a regression test for
@@ -229,11 +229,92 @@ func TestInvalidSecretNameLeavesVaultsUnchanged(t *testing.T) {
for _, tt := range tests { for _, tt := range tests {
t.Run(tt.command, func(t *testing.T) { t.Run(tt.command, func(t *testing.T) {
requireRejectedAndUnchanged(t, before, tt.rejected, tt.run) requireRejectedAndUnchanged(t, before, vault.ValidateSecretName(tt.rejected), tt.run)
}) })
} }
} }
// TestInvalidVersionLeavesVaultsUnchanged is a regression test for
// https://git.eeqj.de/sneak/secret/issues/67, where
// `secret version rm x ../../..` deleted the whole vault,
// `secret version rm x ..` the secret x, and `secret version rm x .` or
// `secret version rm x ""` every version of x. A version argument is
// accepted only if it is one of the versions `secret version list` lists.
//
//nolint:paralleltest // newTwoVaultFs uses t.Setenv
func TestInvalidVersionLeavesVaultsUnchanged(t *testing.T) {
before := snapshotStateDir(t, newTwoVaultFs(t))
cmd := &cobra.Command{}
commands := []struct {
command string
run func(c *cli.Instance, version string) error
}{
{"version rm x", func(c *cli.Instance, version string) error {
return c.RemoveVersion(cmd, "x", version)
}},
{"version promote x", func(c *cli.Instance, version string) error {
return c.PromoteVersion(cmd, "x", version)
}},
{"get x --version", func(c *cli.Instance, version string) error {
return c.GetSecretWithVersion(cmd, "x", version)
}},
}
for _, tt := range commands {
for _, version := range []string{"", ".", "..", "../../..", "a/b"} {
t.Run(fmt.Sprintf("%s %q", tt.command, version), func(t *testing.T) {
want := fmt.Errorf("version '%s' %w '%s'",
version, vault.ErrVersionNotFound, "x")
requireRejectedAndUnchanged(t, before, want,
func(c *cli.Instance) error { return tt.run(c, version) })
})
}
}
}
// TestRemoveVersionRemovesOnlyThatVersion checks that `secret version rm`
// with a version that is not the current one removes that version and
// changes nothing else.
//
//nolint:paralleltest // newTwoVaultFs uses t.Setenv
func TestRemoveVersionRemovesOnlyThatVersion(t *testing.T) {
fs := newTwoVaultFs(t)
vlt, err := vault.GetCurrentVault(fs, testStateDir)
require.NoError(t, err)
// A second version of "x" becomes the current one.
err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("new")), true)
require.NoError(t, err)
secretDir := testStateDir + "/vaults.d/default/secrets.d/x"
versions, err := secret.ListVersions(fs, secretDir)
require.NoError(t, err)
require.Len(t, versions, 2)
// ListVersions lists the newest version first.
oldDir := secretDir + "/versions/" + versions[1] + "/"
before := snapshotStateDir(t, fs)
require.Contains(t, before, oldDir)
c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
err = c.RemoveVersion(&cobra.Command{}, "x", versions[1])
require.NoError(t, err)
// Expected: the state as before without everything under oldDir.
want := map[string]string{}
for path, content := range before {
if !strings.HasPrefix(path, oldDir) {
want[path] = content
}
}
require.Equal(t, want, snapshotStateDir(t, fs))
}
// TestMoveToVaultNameRenamesInCurrentVault checks that `secret mv x work`, // TestMoveToVaultNameRenamesInCurrentVault checks that `secret mv x work`,
// where "work" is also the name of a vault, renames the secret "x" to "work" // where "work" is also the name of a vault, renames the secret "x" to "work"
// in the current vault and changes nothing else. // in the current vault and changes nothing else.
+30 -10
View File
@@ -109,6 +109,12 @@ func newGetCmd() *cobra.Command {
return fmt.Errorf("failed to initialize CLI: %w", err) return fmt.Errorf("failed to initialize CLI: %w", err)
} }
// Without --version, get the current version. A given
// --version is checked as typed, so an empty one is rejected.
if !cmd.Flags().Changed("version") {
return cli.GetSecret(cmd, args[0])
}
return cli.GetSecretWithVersion(cmd, args[0], version) return cli.GetSecretWithVersion(cmd, args[0], version)
}, },
} }
@@ -393,12 +399,32 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
return nil return nil
} }
// GetSecret retrieves and prints a secret from the current vault // GetSecret retrieves and prints the current version of a secret
func (cli *Instance) GetSecret(cmd *cobra.Command, secretName string) error { func (cli *Instance) GetSecret(cmd *cobra.Command, secretName string) error {
return cli.GetSecretWithVersion(cmd, secretName, "") secret.Debug("GetSecret called", "secretName", secretName)
// Store the command for output
cli.cmd = cmd
// Get current vault
vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
if err != nil {
return err
}
value, err := vlt.GetSecret(secretName)
if err != nil {
return err
}
// Print the secret value to stdout
_, _ = cli.Print(string(value))
return nil
} }
// GetSecretWithVersion retrieves and prints a specific version of a secret // GetSecretWithVersion retrieves and prints a specific version of a secret.
// The version must be one of the secret's versions.
func (cli *Instance) GetSecretWithVersion( func (cli *Instance) GetSecretWithVersion(
cmd *cobra.Command, secretName string, version string, cmd *cobra.Command, secretName string, version string,
) error { ) error {
@@ -417,13 +443,7 @@ func (cli *Instance) GetSecretWithVersion(
} }
// Get the secret value // Get the secret value
var value []byte value, err := vlt.GetSecretVersion(secretName, version)
if version == "" {
value, err = vlt.GetSecret(secretName)
} else {
value, err = vlt.GetSecretVersion(secretName, version)
}
if err != nil { if err != nil {
secret.Debug("Failed to get secret", "error", err) secret.Debug("Failed to get secret", "error", err)
+4 -6
View File
@@ -265,9 +265,7 @@ func (cli *Instance) PromoteVersion(
secretDir := filepath.Join(vaultDir, "secrets.d", encodedName) secretDir := filepath.Join(vaultDir, "secrets.d", encodedName)
// Check if version exists // Check if version exists
versionDir := filepath.Join(secretDir, "versions", version) exists, err := secret.VersionExists(cli.fs, secretDir, version)
exists, err := afero.DirExists(cli.fs, versionDir)
if err != nil { if err != nil {
return fmt.Errorf("failed to check if version exists: %w", err) return fmt.Errorf("failed to check if version exists: %w", err)
} }
@@ -323,9 +321,7 @@ func (cli *Instance) RemoveVersion(
} }
// Check if version exists // Check if version exists
versionDir := filepath.Join(secretDir, "versions", version) exists, err = secret.VersionExists(cli.fs, secretDir, version)
exists, err = afero.DirExists(cli.fs, versionDir)
if err != nil { if err != nil {
return fmt.Errorf("failed to check if version exists: %w", err) return fmt.Errorf("failed to check if version exists: %w", err)
} }
@@ -348,6 +344,8 @@ func (cli *Instance) RemoveVersion(
} }
// Remove the version directory // Remove the version directory
versionDir := filepath.Join(secretDir, "versions", version)
err = cli.fs.RemoveAll(versionDir) err = cli.fs.RemoveAll(versionDir)
if err != nil { if err != nil {
return fmt.Errorf("failed to remove version: %w", err) return fmt.Errorf("failed to remove version: %w", err)
+2 -2
View File
@@ -276,8 +276,8 @@ func TestGetSecretWithVersion(t *testing.T) {
var buf bytes.Buffer var buf bytes.Buffer
cmd.SetOut(&buf) cmd.SetOut(&buf)
// Test getting current version (empty version string) // Test getting the current version
err = cli.GetSecretWithVersion(cmd, "test/secret", "") err = cli.GetSecret(cmd, "test/secret")
require.NoError(t, err) require.NoError(t, err)
assert.Equal(t, "version-2", buf.String()) assert.Equal(t, "version-2", buf.String())
+13
View File
@@ -6,6 +6,7 @@ import (
"fmt" "fmt"
"log/slog" "log/slog"
"path/filepath" "path/filepath"
"slices"
"sort" "sort"
"strings" "strings"
"time" "time"
@@ -524,6 +525,18 @@ func ListVersions(fs afero.Fs, secretDir string) ([]string, error) {
return versions, nil return versions, nil
} }
// VersionExists reports whether version is one of the versions ListVersions
// lists for the secret in secretDir. It only compares names, so a version
// the user typed can be checked with it before any path is built from it.
func VersionExists(fs afero.Fs, secretDir string, version string) (bool, error) {
versions, err := ListVersions(fs, secretDir)
if err != nil {
return false, err
}
return slices.Contains(versions, version), nil
}
// GetCurrentVersion returns the version that the "current" file points to // GetCurrentVersion returns the version that the "current" file points to
// The file contains just the version name (e.g., "20231215.001") // The file contains just the version name (e.g., "20231215.001")
func GetCurrentVersion(fs afero.Fs, secretDir string) (string, error) { func GetCurrentVersion(fs afero.Fs, secretDir string) (string, error) {
+1 -1
View File
@@ -49,7 +49,7 @@ var (
// ErrVersionNotFound indicates the requested secret version does not // ErrVersionNotFound indicates the requested secret version does not
// exist. Composed as // exist. Composed as
// "version <version> not found for secret <name>". // "version '<version>' not found for secret '<name>'".
ErrVersionNotFound = errors.New("not found for secret") ErrVersionNotFound = errors.New("not found for secret")
// ErrNoVersions indicates the source secret has no versions. Composed // ErrNoVersions indicates the source secret has no versions. Composed
+4 -4
View File
@@ -235,10 +235,10 @@ func testRetrieveSpecificVersions(
require.NoError(t, err) require.NoError(t, err)
assert.Equal(t, []byte("version-3-data"), value3) assert.Equal(t, []byte("version-3-data"), value3)
// Empty version should return current // An empty version is not one of the versions; GetSecret gets the
valueCurrent, err := vault.GetSecretVersion(secretName, "") // current one
require.NoError(t, err) _, err = vault.GetSecretVersion(secretName, "")
assert.Equal(t, []byte("version-3-data"), valueCurrent) require.ErrorIs(t, err, ErrVersionNotFound)
} }
func testPromoteOldVersion( func testPromoteOldVersion(
+30 -37
View File
@@ -259,18 +259,31 @@ func updateVersionMetadata(
return nil return nil
} }
// GetSecret retrieves a secret from this vault // GetSecret retrieves the current version of a secret from this vault
func (v *Vault) GetSecret(name string) ([]byte, error) { func (v *Vault) GetSecret(name string) ([]byte, error) {
secret.DebugWith("Getting secret from vault", secret.DebugWith("Getting secret from vault",
slog.String("vault_name", v.Name), slog.String("vault_name", v.Name),
slog.String("secret_name", name), slog.String("secret_name", name),
) )
return v.GetSecretVersion(name, "") // GetSecretObject validates the name and checks that the secret exists
secretObj, err := v.GetSecretObject(name)
if err != nil {
return nil, err
}
currentVersion, err := secret.GetCurrentVersion(v.fs, secretObj.Directory)
if err != nil {
secret.Debug("Failed to get current version", "error", err, "secret_name", name)
return nil, fmt.Errorf("failed to get current version: %w", err)
}
return v.GetSecretVersion(name, currentVersion)
} }
// GetSecretVersion retrieves a specific version of a secret (empty version // GetSecretVersion retrieves a specific version of a secret. The version
// means current) // must be one of the secret's versions; GetSecret gets the current one.
func (v *Vault) GetSecretVersion(name string, version string) ([]byte, error) { func (v *Vault) GetSecretVersion(name string, version string) ([]byte, error) {
secret.DebugWith("Getting secret version from vault", secret.DebugWith("Getting secret version from vault",
slog.String("vault_name", v.Name), slog.String("vault_name", v.Name),
@@ -278,8 +291,8 @@ func (v *Vault) GetSecretVersion(name string, version string) ([]byte, error) {
slog.String("version", version), slog.String("version", version),
) )
// Validate the name and resolve the version to fetch // Validate the name and check that the version exists
version, err := v.resolveSecretVersion(name, version) err := v.checkSecretVersion(name, version)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@@ -640,15 +653,15 @@ func (v *Vault) updatePreviousVersion(
return nil return nil
} }
// resolveSecretVersion validates the secret name, verifies the secret and // checkSecretVersion validates the secret name and verifies that the secret
// version exist, and resolves an empty version to the current one. // exists and that version is one of its versions.
func (v *Vault) resolveSecretVersion(name, version string) (string, error) { func (v *Vault) checkSecretVersion(name, version string) error {
// Validate secret name to prevent path traversal // Validate secret name to prevent path traversal
err := ValidateSecretName(name) err := ValidateSecretName(name)
if err != nil { if err != nil {
secret.Debug("Invalid secret name provided", "secret_name", name) secret.Debug("Invalid secret name provided", "secret_name", name)
return "", err return err
} }
// Get vault directory // Get vault directory
@@ -656,7 +669,7 @@ func (v *Vault) resolveSecretVersion(name, version string) (string, error) {
if err != nil { if err != nil {
secret.Debug("Failed to get vault directory", "error", err, "vault_name", v.Name) secret.Debug("Failed to get vault directory", "error", err, "vault_name", v.Name)
return "", err return err
} }
// Convert slashes to percent signs for storage // Convert slashes to percent signs for storage
@@ -668,50 +681,30 @@ func (v *Vault) resolveSecretVersion(name, version string) (string, error) {
if err != nil { if err != nil {
secret.Debug("Failed to check if secret exists", "error", err, "secret_name", name) secret.Debug("Failed to check if secret exists", "error", err, "secret_name", name)
return "", fmt.Errorf("failed to check if secret exists: %w", err) return fmt.Errorf("failed to check if secret exists: %w", err)
} }
if !exists { if !exists {
secret.Debug("Secret not found in vault", "secret_name", name, "vault_name", v.Name) secret.Debug("Secret not found in vault", "secret_name", name, "vault_name", v.Name)
return "", fmt.Errorf("secret %s %w", name, ErrSecretNotFound) return fmt.Errorf("secret %s %w", name, ErrSecretNotFound)
}
// Determine which version to get
if version == "" {
// Get current version
currentVersion, err := secret.GetCurrentVersion(v.fs, secretDir)
if err != nil {
secret.Debug("Failed to get current version", "error", err, "secret_name", name)
return "", fmt.Errorf("failed to get current version: %w", err)
}
version = currentVersion
secret.Debug("Using current version", "version", version, "secret_name", name)
} }
// Check if version exists // Check if version exists
versionPath := filepath.Join(secretDir, "versions", version) exists, err = secret.VersionExists(v.fs, secretDir, version)
exists, err = afero.DirExists(v.fs, versionPath)
if err != nil { if err != nil {
secret.Debug("Failed to check if version exists", "error", err, "version", version) secret.Debug("Failed to check if version exists", "error", err, "version", version)
return "", fmt.Errorf("failed to check if version exists: %w", err) return fmt.Errorf("failed to check if version exists: %w", err)
} }
if !exists { if !exists {
secret.Debug("Version not found", "version", version, "secret_name", name) secret.Debug("Version not found", "version", version, "secret_name", name)
return "", fmt.Errorf( return fmt.Errorf("version '%s' %w '%s'", version, ErrVersionNotFound, name)
"version %s %w %s",
version, ErrVersionNotFound, name,
)
} }
return version, nil return nil
} }
// createAndSaveVersion generates a new version name, sets the version // createAndSaveVersion generates a new version name, sets the version
+4 -4
View File
@@ -202,10 +202,10 @@ func TestVaultGetSecretVersion(t *testing.T) {
require.NoError(t, err) require.NoError(t, err)
assert.Equal(t, []byte("version-2"), value) assert.Equal(t, []byte("version-2"), value)
// Get current (empty version) // An empty version is not one of the versions; GetSecret gets the
value, err = vault.GetSecretVersion(testSecretPath, "") // current one
require.NoError(t, err) _, err = vault.GetSecretVersion(testSecretPath, "")
assert.Equal(t, []byte("version-2"), value) require.ErrorIs(t, err, ErrVersionNotFound)
} }
//nolint:paralleltest // createTestVaultWithKey uses t.Setenv //nolint:paralleltest // createTestVaultWithKey uses t.Setenv