diff --git a/README.md b/README.md index 4651c36..b114b3c 100644 --- a/README.md +++ b/README.md @@ -90,7 +90,8 @@ remove things pass `--force`. #### `secret init` Initializes the secret manager with a default vault. Prompts for a BIP39 -mnemonic phrase and creates the initial directory structure. +mnemonic phrase and creates the initial directory structure. When the vault +`default` already exists, it fails before asking for the mnemonic or passphrase. **Environment Variables:** @@ -105,7 +106,8 @@ Lists all available vaults. The current vault is marked. #### `secret vault create ` -Creates a new vault with the specified name. +Creates a new vault with the specified name. When a vault of that name already +exists, it fails before asking for the mnemonic or passphrase. **Vault Name Format:** only lowercase ASCII letters, digits, `.`, `-` and `_` are allowed, and a name must not be empty, `.` or `..`. diff --git a/TODO.md b/TODO.md index 857a05f..f594c25 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,15 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-14: `secret init` and `secret vault create` refuse a vault that + already exists before asking for the mnemonic or the unlocker passphrase + (https://git.eeqj.de/sneak/secret/issues/151). They used to ask for both and + only then refuse, and `secret init` printed "Initialized secrets manager at" + before the refusal. The new `vault.CheckNewVaultName`, which + `vault.CreateVault` also calls, checks the name while the command holds the + state directory lock, before the first prompt; `secret init` prints that line + only once the vault is created. `secret vault create` with an invalid name + also fails before asking. - 2026-10-14: `secret generate mnemonic`, `secret vault list` and `secret unlocker list` print their output to stdout, as `secret list` does, so it can be piped or captured (https://git.eeqj.de/sneak/secret/issues/138). diff --git a/internal/cli/create_vault_test.go b/internal/cli/create_vault_test.go index 17838d7..3dc1fa9 100644 --- a/internal/cli/create_vault_test.go +++ b/internal/cli/create_vault_test.go @@ -220,10 +220,53 @@ func TestMnemonicNotReadNamesOnlyMnemonic(t *testing.T) { output, err := cmd.CombinedOutput() require.Error(t, err) - require.Equal(t, "Initialized secrets manager at: "+stateDir+"\n"+ - "Error: failed to read mnemonic: stdin is not a terminal (piped input "+ - "or script). Please set the SB_SECRET_MNEMONIC environment variable "+ - "or run interactively\n", string(output)) + require.Equal(t, "Error: failed to read mnemonic: stdin is not a terminal "+ + "(piped input or script). Please set the SB_SECRET_MNEMONIC environment "+ + "variable or run interactively\n", string(output)) +} + +// TestCreateExistingVaultAsksForNothing is a regression test for +// https://git.eeqj.de/sneak/secret/issues/151: `secret init` and `secret +// vault create` asked for the mnemonic and the unlocker passphrase before +// refusing a vault that already exists, and `secret init` printed +// "Initialized secrets manager at" first. Each runs here as the built binary, +// with no terminal and neither SB_SECRET_MNEMONIC nor SB_UNLOCK_PASSPHRASE +// set, so asking for either fails. Each must fail saying only that the vault +// exists. +func TestCreateExistingVaultAsksForNothing(t *testing.T) { + t.Parallel() + + stateDir := t.TempDir() + _, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default", nil, nil) + require.NoError(t, err) + + tests := []struct { + args []string + output string + }{ + { + []string{"init"}, + "Error: failed to create default vault: vault default already exists\n", + }, + { + []string{"vault", "create", "default"}, + "Error: vault default already exists\n", + }, + } + + for _, tt := range tests { + //nolint:gosec // G204: test executes the freshly built secret binary + cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), tt.args...) + cmd.Env = []string{ + secret.EnvStateDir + "=" + stateDir, + "PATH=" + os.Getenv("PATH"), + "HOME=" + os.Getenv("HOME"), + } + + output, err := cmd.CombinedOutput() + require.Error(t, err, "secret %v", tt.args) + require.Equal(t, tt.output, string(output), "secret %v", tt.args) + } } // TestStopDuringCreateLeavesWholeVaultOrNone is a regression test for diff --git a/internal/cli/init.go b/internal/cli/init.go index 83b88a7..02f049c 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -18,6 +18,9 @@ import ( // errPassphraseMismatch is returned when passphrase confirmation fails var errPassphraseMismatch = errors.New("passphrases do not match") +// defaultVaultName is the name of the vault `secret init` creates +const defaultVaultName = "default" + // NewInitCmd creates the init command func NewInitCmd() *cobra.Command { return &cobra.Command{ @@ -95,8 +98,10 @@ func (cli *Instance) initialize(cmd *cobra.Command) error { return fmt.Errorf("failed to create state directory: %w", err) } - if cmd != nil { - cmd.Printf("Initialized secrets manager at: %s\n", stateDir) + // Refuse an existing default vault before asking for anything + err = vault.CheckNewVaultName(cli.fs, cli.stateDir, defaultVaultName) + if err != nil { + return fmt.Errorf("failed to create default vault: %w", err) } // Prompt for mnemonic @@ -136,7 +141,7 @@ func (cli *Instance) initialize(cmd *cobra.Command) error { // Create the default vault with its passphrase unlocker secret.Debug("Creating default vault") - vlt, err := vault.CreateVault(cli.fs, cli.stateDir, "default", + vlt, err := vault.CreateVault(cli.fs, cli.stateDir, defaultVaultName, mnemonic, passphraseBuffer) if err != nil { secret.Debug("Failed to create default vault", "error", err) @@ -155,6 +160,7 @@ func (cli *Instance) initialize(cmd *cobra.Command) error { } if cmd != nil { + cmd.Printf("Initialized secrets manager at: %s\n", stateDir) cmd.Printf("\nDefault vault created and configured\n") cmd.Printf("Long-term public key: %s\n", ltIdentity.Recipient().String()) cmd.Printf("Unlocker ID: %s\n", unlocker.GetID()) diff --git a/internal/cli/vault.go b/internal/cli/vault.go index d88e051..7e58949 100644 --- a/internal/cli/vault.go +++ b/internal/cli/vault.go @@ -267,6 +267,12 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error { } defer release() + // Refuse an invalid name or an existing vault before asking for anything + err = vault.CheckNewVaultName(cli.fs, cli.stateDir, name) + if err != nil { + return err + } + mnemonic, cleanupMnemonic, err := cli.promptMnemonic() if err != nil { return err diff --git a/internal/vault/management.go b/internal/vault/management.go index 5bf9f3f..951ffbf 100644 --- a/internal/vault/management.go +++ b/internal/vault/management.go @@ -212,16 +212,40 @@ func processMnemonicForVault( return ltIdentity, derivationIndex, publicKeyHash, familyHash, nil } +// CheckNewVaultName returns an error when name cannot be the name of a new +// vault: one wrapping ErrInvalidVaultName when it is not a valid vault name, +// and one wrapping ErrVaultExists when that vault already exists. Creating an +// existing vault again would replace its keys, and its secrets could no +// longer be decrypted. CreateVault checks this before writing anything; the +// commands that create a vault also check it before they ask for the +// mnemonic or passphrase, holding the state directory lock. +func CheckNewVaultName(fs afero.Fs, stateDir string, name string) error { + err := ValidateVaultName(name) + if err != nil { + return err + } + + exists, err := afero.DirExists(fs, filepath.Join(stateDir, "vaults.d", name)) + if err != nil { + return fmt.Errorf("failed to check if vault exists: %w", err) + } + + if exists { + return fmt.Errorf("vault %s %w", name, ErrVaultExists) + } + + return nil +} + // CreateVault creates a new vault and selects it as the current vault. When // mnemonic is not nil, the vault's long-term key is derived from it, and the // returned vault has it as its Mnemonic; when it is nil, the vault has no // long-term key until one is imported. When passphrase is not nil, the vault // gets a passphrase unlocker protected by it, as its current unlocker; that -// needs a mnemonic. 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. +// needs a mnemonic. It refuses an invalid name or a vault that already exists, +// as CheckNewVaultName does, before writing anything. The commands that call +// it hold the state directory lock, so no other command can create the vault +// between the check and the writes. // // The vault is written whole into a temporary directory, which is renamed // into vaults.d only once complete, and only then selected: a crash at any @@ -233,30 +257,17 @@ func CreateVault( ) (*Vault, error) { secret.Debug("Creating new vault", "name", name, "state_dir", stateDir) - err := ValidateVaultName(name) + err := CheckNewVaultName(fs, stateDir, name) if err != nil { - secret.Debug("Invalid vault name provided", "vault_name", name) - return nil, err } - secret.Debug("Vault name validation passed", "vault_name", name) - - 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) - } - if passphrase != nil && mnemonic == nil { return nil, fmt.Errorf("vault %s %w", name, ErrUnlockerWithoutMnemonic) } + vaultDir := filepath.Join(stateDir, "vaults.d", name) + secret.Debug("Creating vault directory structure", "vault_dir", vaultDir) err = secret.WriteDir(fs, vaultDir, func(dir string) error {