diff --git a/TODO.md b/TODO.md index 94d3603..41d7517 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,15 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-04: `secret init` refuses when the default vault exists, and + `secret vault create NAME` when `NAME` does, with "vault NAME already + exists", before writing anything. The check is in `vault.CreateVault`, + which both commands call while holding the state directory lock, so two + creates of one vault at once cannot both pass the check. Before, either + command replaced the vault's metadata, passphrase unlocker and + `longterm.age`, so none of its secrets could be decrypted any more. Both + commands now ask for the unlocker passphrase before creating the vault, + so one stopped at that prompt leaves no vault behind. - 2026-10-04: A PGP unlocker whose metadata has no usable GPG key ID no longer panics: `GetID()` warns with the unlocker's directory and returns `pgp-unknown`. `ListUnlockers` skips, with a warning, an @@ -57,9 +66,9 @@ Bring the repo into policy compliance in one commit: has one, and to a PGP, keychain or Secure Enclave unlocker added on the same host and day as another of its type (https://git.eeqj.de/sneak/secret/issues/71); - - from `vault create` stopped at the passphrase prompt, a new vault - with no unlocker that is already the current vault; from `init` - stopped there, the default vault with no unlocker; + - from `init` or `vault create` killed after the passphrase prompt + but before the unlocker is written, a vault with no unlocker, + which `vault create` has already made the current vault; - from an unlocker add stopped before its metadata is written, a directory that `unlocker list` warns about and `unlocker rm` cannot remove; diff --git a/internal/cli/create_vault_test.go b/internal/cli/create_vault_test.go new file mode 100644 index 0000000..55b9d52 --- /dev/null +++ b/internal/cli/create_vault_test.go @@ -0,0 +1,144 @@ +package cli_test + +import ( + "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" +) + +// TestCreateExistingVaultChangesNothing is a regression test for +// https://git.eeqj.de/sneak/secret/issues/74, where running `secret init` +// a second time, or `secret vault create` with the name of an existing +// vault, replaced that vault's keys, so that none of its secrets could be +// decrypted any more. Each must refuse, change nothing, and leave every +// vault's secret readable through its passphrase unlocker. +func TestCreateExistingVaultChangesNothing(t *testing.T) { + t.Setenv(secret.EnvMnemonic, testMnemonic) + t.Setenv(secret.EnvUnlockPassphrase, testPassphrase) + + // `secret init`, `secret vault create work`, `secret vault select + // default`, and the secret "x" in each vault. "work" is then not the + // current vault, which creating it again must not change. + fs := afero.NewMemMapFs() + c := cli.NewCLIInstanceWithStateDir(fs, testStateDir) + cmd := &cobra.Command{} + + require.NoError(t, c.Init(cmd)) + require.NoError(t, c.CreateVault(cmd, "work")) + require.NoError(t, c.SelectVault(cmd, "default")) + + vaults, err := vault.ListVaults(fs, testStateDir) + require.NoError(t, err) + require.Len(t, vaults, 2) + + for _, name := range vaults { + value := memguard.NewBufferFromBytes([]byte("value")) + err := vault.NewVault(fs, testStateDir, name).AddSecret("x", value, false) + require.NoError(t, err) + } + + before := snapshotStateDir(t, fs) + + tests := []struct { + command string + want string + run func(c *cli.Instance) error + }{ + { + "init", + "failed to create default vault: vault default already exists", + func(c *cli.Instance) error { return c.Init(cmd) }, + }, + { + "vault create default", + "vault default already exists", + func(c *cli.Instance) error { return c.CreateVault(cmd, "default") }, + }, + { + "vault create work", + "vault work already exists", + func(c *cli.Instance) error { return c.CreateVault(cmd, "work") }, + }, + } + + for _, tt := range tests { + t.Run(tt.command, func(t *testing.T) { + fs := newFsFromSnapshot(t, before) + + err := tt.run(cli.NewCLIInstanceWithStateDir(fs, testStateDir)) + + require.EqualError(t, err, tt.want) + require.Equal(t, before, snapshotStateDir(t, fs)) + }) + } + + // Every case left the state directory exactly as recorded in before, so + // reading each vault's secret once from it shows that it still decrypts + // after each case. Without the mnemonic, reading a secret goes through + // the vault's passphrase unlocker, which is slow. + t.Setenv(secret.EnvMnemonic, "") + + for _, name := range vaults { + value, err := vault.NewVault(fs, testStateDir, name).GetSecret("x") + require.NoError(t, err) + require.Equal(t, "value", string(value)) + } +} + +// TestStopAtPassphrasePromptLeavesNothing is a regression test for the +// review of https://git.eeqj.de/sneak/secret/pulls/82: `secret init` or +// `secret vault create` stopped at the passphrase prompt left a vault with +// no unlocker, which neither command would then create again. Each must ask +// for the passphrase before writing anything. +func TestStopAtPassphrasePromptLeavesNothing(t *testing.T) { + t.Setenv(secret.EnvMnemonic, testMnemonic) + + // Without the passphrase in the environment, both commands prompt for + // it, which fails because the tests do not run in a terminal. + t.Setenv(secret.EnvUnlockPassphrase, "") + + // An empty state directory for `secret init`, and one holding the vault + // "default" for `secret vault create work`. + empty := afero.NewMemMapFs() + require.NoError(t, empty.MkdirAll(testStateDir, secret.DirPerms)) + + withDefault := afero.NewMemMapFs() + _, err := vault.CreateVault(withDefault, testStateDir, "default") + require.NoError(t, err) + + cmd := &cobra.Command{} + + tests := []struct { + command string + fs afero.Fs + run func(c *cli.Instance) error + }{ + { + "init", + empty, + func(c *cli.Instance) error { return c.Init(cmd) }, + }, + { + "vault create work", + withDefault, + func(c *cli.Instance) error { return c.CreateVault(cmd, "work") }, + }, + } + + for _, tt := range tests { + t.Run(tt.command, func(t *testing.T) { + before := snapshotStateDir(t, tt.fs) + + err := tt.run(cli.NewCLIInstanceWithStateDir(tt.fs, testStateDir)) + + require.ErrorContains(t, err, "failed to read passphrase") + require.Equal(t, before, snapshotStateDir(t, tt.fs)) + }) + } +} diff --git a/internal/cli/init.go b/internal/cli/init.go index b943ef2..b8bfd09 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -160,6 +160,14 @@ func (cli *Instance) initialize(cmd *cobra.Command) error { errInvalidMnemonicPhrase) } + // Ask for the unlocker passphrase before creating the vault, so that + // stopping at the prompt leaves no vault without an unlocker behind + passphraseBuffer, err := resolvePassphrase() + if err != nil { + return err + } + defer passphraseBuffer.Destroy() + // Set mnemonic in environment for CreateVault to use restoreMnemonicEnv := setMnemonicEnv(mnemonicStr) defer restoreMnemonicEnv() @@ -175,13 +183,6 @@ func (cli *Instance) initialize(cmd *cobra.Command) error { // Unlock the vault with the derived long-term key vlt.Unlock(ltIdentity) - // Prompt for passphrase for unlocker - passphraseBuffer, err := resolvePassphrase() - if err != nil { - return err - } - defer passphraseBuffer.Destroy() - // Create passphrase-protected unlocker secret.Debug("Creating passphrase-protected unlocker") diff --git a/internal/cli/lock_test.go b/internal/cli/lock_test.go index 15274df..8353b51 100644 --- a/internal/cli/lock_test.go +++ b/internal/cli/lock_test.go @@ -274,11 +274,12 @@ func stateDirModTimes(t *testing.T, fs afero.Fs) map[string]int64 { } // setupEveryCommand makes what each command in -// TestChangingCommandsWaitForLock needs: the current vault "default" with -// two versions of "test/secret", the vault "other" without a long-term key, -// for vault import, and the file testInput. If withUnlocker is set, it also -// gives "default" a passphrase unlocker, which is slow. It returns the older -// version and the unlocker's ID. +// TestChangingCommandsWaitForLock needs: the current vault "work" with two +// versions of "test/secret", the vault "other" without a long-term key, for +// vault import, and the file testInput. There is no vault "default", which +// init creates. If withUnlocker is set, it also gives "work" a passphrase +// unlocker, which is slow. It returns the older version and the unlocker's +// ID. func setupEveryCommand( t *testing.T, fs afero.Fs, withUnlocker bool, ) (string, string) { @@ -291,7 +292,7 @@ func setupEveryCommand( require.NoError(t, err) require.NoError(t, fs.Remove(filepath.Join(otherDir, "pub.age"))) - vlt, err := vault.CreateVault(fs, testStateDir, "default") + vlt, err := vault.CreateVault(fs, testStateDir, "work") require.NoError(t, err) addTestSecret(t, vlt, []byte("older"), false) diff --git a/internal/cli/vault.go b/internal/cli/vault.go index 32ce02a..3862019 100644 --- a/internal/cli/vault.go +++ b/internal/cli/vault.go @@ -309,6 +309,14 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error { return errInvalidMnemonicPhrase } + // Ask for the unlocker passphrase before creating the vault, so that + // stopping at the prompt leaves no vault without an unlocker behind + passphraseBuffer, err := resolvePassphrase() + if err != nil { + return err + } + defer passphraseBuffer.Destroy() + // Set mnemonic in environment for CreateVault to use restoreMnemonicEnv := setMnemonicEnv(mnemonicStr) defer restoreMnemonicEnv() @@ -336,13 +344,6 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error { // Unlock the vault with the derived long-term key vlt.Unlock(ltIdentity) - // Get or prompt for passphrase - passphraseBuffer, err := resolvePassphrase() - if err != nil { - return err - } - defer passphraseBuffer.Destroy() - // Create passphrase-protected unlocker secret.Debug("Creating passphrase-protected unlocker") diff --git a/internal/vault/errors.go b/internal/vault/errors.go index 99a4bae..37af7c7 100644 --- a/internal/vault/errors.go +++ b/internal/vault/errors.go @@ -26,6 +26,10 @@ var ( // as "vault does not exist". ErrVaultNotFound = errors.New("does not exist") + // ErrVaultExists indicates that a vault to be created already exists. + // Composed as "vault already exists". + ErrVaultExists = errors.New("already exists") + // ErrNilValueBuffer indicates a nil value buffer was supplied. ErrNilValueBuffer = errors.New("value buffer is nil") diff --git a/internal/vault/management.go b/internal/vault/management.go index 9cfa4d9..af545d6 100644 --- a/internal/vault/management.go +++ b/internal/vault/management.go @@ -191,7 +191,11 @@ func processMnemonicForVault( return derivationIndex, publicKeyHash, familyHash, nil } -// CreateVault creates a new vault +// CreateVault creates a new vault and selects it as the current vault. It +// refuses a vault that already exists before writing anything: creating it +// again would replace its keys, and its secrets could no longer be +// decrypted. The commands that call it hold the state directory lock, so no +// other command can create the vault between the check and the writes. func CreateVault(fs afero.Fs, stateDir string, name string) (*Vault, error) { secret.Debug("Creating new vault", "name", name, "state_dir", stateDir) @@ -207,12 +211,22 @@ func CreateVault(fs afero.Fs, stateDir string, name string) (*Vault, error) { secret.Debug("Vault name validation passed", "vault_name", name) - // Create vault directory structure vaultDir := filepath.Join(stateDir, "vaults.d", name) + + exists, err := afero.DirExists(fs, vaultDir) + if err != nil { + return nil, fmt.Errorf("failed to check if vault exists: %w", err) + } + + if exists { + return nil, fmt.Errorf("vault %s %w", name, ErrVaultExists) + } + + // Create vault directory structure secret.Debug("Creating vault directory structure", "vault_dir", vaultDir) // Create main vault directory - err := fs.MkdirAll(vaultDir, secret.DirPerms) + err = fs.MkdirAll(vaultDir, secret.DirPerms) if err != nil { return nil, fmt.Errorf("failed to create vault directory: %w", err) }