diff --git a/README.md b/README.md index 1e2b834..c9e0923 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 42302ae..f50aa57 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: A wrong passphrase, from `SB_UNLOCK_PASSPHRASE` or typed at the prompt, gives the error `wrong passphrase for unlocker ` (https://git.eeqj.de/sneak/secret/issues/157). It used to be age's "no diff --git a/internal/cli/create_vault_test.go b/internal/cli/create_vault_test.go index 403fd31..410b9e7 100644 --- a/internal/cli/create_vault_test.go +++ b/internal/cli/create_vault_test.go @@ -222,12 +222,51 @@ 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: there is no terminal to ask on. "+ - "Please set the SB_SECRET_MNEMONIC environment variable or run "+ + require.Equal(t, "Error: failed to read mnemonic: there is no terminal to "+ + "ask on. 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, +// in a new session, which has no terminal, and with 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) + + // run runs the built binary with args, which must fail, and returns what + // it printed + run := func(args ...string) string { + //nolint:gosec // G204: test executes the freshly built secret binary + cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...) + cmd.Env = []string{ + secret.EnvStateDir + "=" + stateDir, + "PATH=" + os.Getenv("PATH"), + "HOME=" + os.Getenv("HOME"), + } + cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} + + output, err := cmd.CombinedOutput() + require.Error(t, err, "secret %v", args) + + return string(output) + } + + require.Equal(t, + "Error: failed to create default vault: vault default already exists\n", + run("init")) + require.Equal(t, "Error: vault default already exists\n", + run("vault", "create", "default")) +} + // TestStopDuringCreateLeavesWholeVaultOrNone is a regression test for // https://git.eeqj.de/sneak/secret/issues/105: `secret init` or `secret vault // create` killed after the passphrase prompt but before the unlocker was diff --git a/internal/cli/init.go b/internal/cli/init.go index c48f758..003d61c 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -17,6 +17,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{ @@ -92,8 +95,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 @@ -133,7 +138,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) @@ -152,6 +157,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 {