From 4ae2f47a6d178833242819ba3d2fffc17a37862a Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 05:04:44 +0000 Subject: [PATCH] Ask for the mnemonic and passphrase before taking the state directory lock (closes #152) init, vault create, unlocker add, and add, import and generate secret with --force asked for the mnemonic or passphrase, or unlocked the vault, while holding the state directory lock, so every other command that changes anything waited at the prompt. They now ask and unlock first and lock after. What they checked before asking is checked again under the lock: unlocker add fails if the vault was removed meanwhile and refuses a GPG key added meanwhile, and a forced add fails if another command created the secret meanwhile, rather than ask under the lock. secret.CreatePGPUnlocker now takes the vault, already unlocked. Not done: secret move between vaults still unlocks both under the lock; the issue does not name it. Model: opus-5-5 --- README.md | 9 + internal/cli/generate.go | 8 +- internal/cli/init.go | 60 +++--- internal/cli/lock_test.go | 291 ++++++++++++++++++++++++++-- internal/cli/secrets.go | 77 ++++++-- internal/cli/unlockers.go | 92 +++++++-- internal/cli/unreadable_dir_test.go | 11 +- internal/cli/vault.go | 21 +- internal/secret/pgpunlock_test.go | 3 +- internal/secret/pgpunlocker.go | 41 ++-- internal/secret/pgpunlocker_test.go | 11 +- internal/vault/management.go | 6 +- 12 files changed, 492 insertions(+), 138 deletions(-) diff --git a/README.md b/README.md index 76f377a..b46361e 100644 --- a/README.md +++ b/README.md @@ -428,6 +428,15 @@ asks when stdin is a pipe, as in `echo value | secret add name --force` or `cat file | secret encrypt key`. With no controlling terminal, as under cron or in a CI job, the command fails and names the variable to set. +A command that changes anything locks the state directory, through its file +`lock`, and another such command waits until the lock is released. +`secret init`, `secret vault create`, `secret unlocker add`, and `secret add`, +`secret import` and `secret generate secret` with `--force`, ask for the +mnemonic or passphrase before they take the lock, so a prompt left open in one +terminal holds up no command in another. A forced add of a secret that does not +exist yet fails, and changes nothing, if another command creates the secret +while it waits for the lock; run it again to replace the secret. + ## Security Features ### Encryption diff --git a/internal/cli/generate.go b/internal/cli/generate.go index eef79c6..164aeac 100644 --- a/internal/cli/generate.go +++ b/internal/cli/generate.go @@ -175,12 +175,6 @@ func (cli *Instance) GenerateSecret( return fmt.Errorf("failed to generate random secret: %w", err) } - release, err := vault.LockStateDir(cli.fs, cli.stateDir) - if err != nil { - return err - } - defer release() - // Store the secret in the vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { @@ -193,7 +187,7 @@ func (cli *Instance) GenerateSecret( secretBuffer := memguard.NewBufferFromBytes([]byte(secretValue)) defer secretBuffer.Destroy() - err = vlt.AddSecret(secretName, secretBuffer, force) + err = cli.storeSecret(vlt, secretName, secretBuffer, force) if err != nil { return err } diff --git a/internal/cli/init.go b/internal/cli/init.go index 003d61c..98d41f8 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -67,36 +67,15 @@ func (cli *Instance) promptMnemonic() (*memguard.LockedBuffer, func(), error) { return mnemonicBuffer, mnemonicBuffer.Destroy, nil } -// Init initializes the secret manager, holding the state directory lock -// while initialize runs +// Init initializes the secret manager. It asks for the mnemonic and the +// passphrase before it takes the state directory lock, so that no other +// command waits while the user answers, and holds the lock while +// initialize runs. func (cli *Instance) Init(cmd *cobra.Command) error { - release, err := vault.LockStateDir(cli.fs, cli.stateDir) - if err != nil { - return err - } - defer release() - - return cli.initialize(cmd) -} - -// initialize creates the state directory, the default vault and its first -// unlocker -func (cli *Instance) initialize(cmd *cobra.Command) error { secret.Debug("Starting secret manager initialization") - // Create state directory - stateDir := cli.GetStateDir() - secret.DebugWith("Creating state directory", slog.String("path", stateDir)) - - err := cli.fs.MkdirAll(stateDir, secret.DirPerms) - if err != nil { - secret.Debug("Failed to create state directory", "error", err) - - return fmt.Errorf("failed to create state directory: %w", err) - } - // Refuse an existing default vault before asking for anything - err = vault.CheckNewVaultName(cli.fs, cli.stateDir, defaultVaultName) + err := vault.CheckNewVaultName(cli.fs, cli.stateDir, defaultVaultName) if err != nil { return fmt.Errorf("failed to create default vault: %w", err) } @@ -135,11 +114,38 @@ func (cli *Instance) initialize(cmd *cobra.Command) error { } defer cleanupPassphrase() + release, err := vault.LockStateDir(cli.fs, cli.stateDir) + if err != nil { + return err + } + defer release() + + return cli.initialize(cmd, mnemonic, passphraseBuffer) +} + +// initialize creates the state directory, and the default vault from +// mnemonic with a passphrase unlocker protected by passphrase. Another +// command may have created the vault since Init checked: vault.CreateVault +// checks again. +func (cli *Instance) initialize( + cmd *cobra.Command, mnemonic, passphrase *memguard.LockedBuffer, +) error { + // Create state directory + stateDir := cli.GetStateDir() + secret.DebugWith("Creating state directory", slog.String("path", stateDir)) + + err := cli.fs.MkdirAll(stateDir, secret.DirPerms) + if err != nil { + secret.Debug("Failed to create state directory", "error", err) + + return fmt.Errorf("failed to create state directory: %w", err) + } + // Create the default vault with its passphrase unlocker secret.Debug("Creating default vault") vlt, err := vault.CreateVault(cli.fs, cli.stateDir, defaultVaultName, - mnemonic, passphraseBuffer) + mnemonic, passphrase) if err != nil { secret.Debug("Failed to create default vault", "error", err) diff --git a/internal/cli/lock_test.go b/internal/cli/lock_test.go index b199f0d..be6d840 100644 --- a/internal/cli/lock_test.go +++ b/internal/cli/lock_test.go @@ -280,9 +280,9 @@ func stateDirModTimes(t *testing.T, fs afero.Fs) map[string]int64 { return modTimes } -// setupEveryCommand makes what each command in -// TestChangingCommandsWaitForLock needs: the current vault "work" with two -// versions of "test/secret", the vault "other", and the file testInput. +// setupEveryCommand makes what the commands that the lock tests below run +// need: the current vault "work" with two versions of "test/secret", the +// vault "other", 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. @@ -346,6 +346,25 @@ func waitingForLock() bool { return false } +// requireWaitingForLock returns once a command waits for the state +// directory lock. It fails the test if the command finishes first, sending +// its error on done, or never waits. +func requireWaitingForLock(t *testing.T, done <-chan error) { + t.Helper() + + timeout := time.After(lockWait) + + for !waitingForLock() { + select { + case err := <-done: + t.Fatalf("finished while the lock was held, with error %v", err) + case <-timeout: + t.Fatal("never waited for the lock") + case <-time.After(time.Millisecond): + } + } +} + // requireWaitsForLock runs a command, given what setupEveryCommand made, // while holding the state directory lock. The command must neither finish // nor change anything before it waits for the lock, and must succeed once @@ -385,17 +404,7 @@ func requireWaitsForLock( go func() { done <- run(cli, olderVersion, unlockerID) }() - timeout := time.After(lockWait) - - for !waitingForLock() { - select { - case err := <-done: - t.Fatalf("finished while the lock was held, with error %v", err) - case <-timeout: - t.Fatal("never waited for the lock") - case <-time.After(time.Millisecond): - } - } + requireWaitingForLock(t, done) assert.Equal(t, before, stateDirModTimes(t, fs), "changed the state directory before waiting for the lock") @@ -472,6 +481,260 @@ func TestChangingCommandsWaitForLock(t *testing.T) { } } +// TestCommandsAskBeforeLocking is a regression test for +// https://git.eeqj.de/sneak/secret/issues/152: these commands took the +// state directory lock before asking for the mnemonic or a passphrase, so +// while one waited at its prompt, every other command that changes anything +// waited for it. Each runs here while the test holds the lock, given what +// takes it to the last step at which it can ask: the passphrase prompt of +// init and vault create, and the unlock of the vault, which asks for the +// passphrase of its passphrase unlocker, for the others. The tests have no +// terminal, so a prompt fails at once. Each command must get there, and +// fail, without waiting for the lock. +// +//nolint:paralleltest // waitingForLock sees any test's command waiting for the lock +func TestCommandsAskBeforeLocking(t *testing.T) { + newTestGPGKey(t) + + mnemonic := testMnemonicBuffer(t) + + wrongPassphrase := memguard.NewBufferFromBytes([]byte("wrong-passphrase")) + t.Cleanup(wrongPassphrase.Destroy) + + for _, tc := range []struct { + name string + run func(cli *Instance) error + // want is wrapped in the error the command fails with + want error + }{ + {"init", func(cli *Instance) error { + cli.Mnemonic = mnemonic + + return cli.Init(cli.cmd) + }, secret.ErrNoTerminal}, + {"vault create", func(cli *Instance) error { + cli.Mnemonic = mnemonic + + return cli.CreateVault(cli.cmd, "created") + }, secret.ErrNoTerminal}, + // With no terminal to ask for the new passphrase on, the new + // unlocker takes the one in SB_UNLOCK_PASSPHRASE, which then fails + // to unlock the vault + {"unlocker add passphrase", func(cli *Instance) error { + cli.UnlockPassphrase = wrongPassphrase + + return cli.UnlockersAdd("passphrase", cli.cmd) + }, secret.ErrWrongPassphrase}, + {"unlocker add pgp", func(cli *Instance) error { + cli.cmd.Flags().String("keyid", unreadableTestGPGUserID, "") + + return cli.UnlockersAdd("pgp", cli.cmd) + }, secret.ErrNoTerminal}, + {"add --force", func(cli *Instance) error { + return cli.AddSecret("test/secret", true) + }, secret.ErrNoTerminal}, + {"import --force", func(cli *Instance) error { + return cli.ImportSecret(cli.cmd, "test/secret", testInput, true) + }, secret.ErrNoTerminal}, + {"generate secret --force", func(cli *Instance) error { + return cli.GenerateSecret(cli.cmd, "test/secret", 16, "base58", true) + }, secret.ErrNoTerminal}, + } { + t.Run(tc.name, func(t *testing.T) { + requireAsksBeforeLocking(t, tc.run, tc.want) + }) + } +} + +// startWhileLocked makes what setupEveryCommand makes, with the passphrase +// unlocker, on fs, an empty in-memory filesystem, takes the state directory +// lock there, and starts run, given a CLI instance on fs with "value" on +// stdin. It returns the function that releases the lock, which does so only +// the first time it is called, and the channel that run's error comes on. +// The lock is also released when the test ends, so that later tests can +// take it. +func startWhileLocked( + t *testing.T, fs afero.Fs, run func(cli *Instance) error, +) (func(), <-chan error) { + t.Helper() + + setupEveryCommand(t, fs, true) + + release, err := vault.LockStateDir(fs, testStateDir) + require.NoError(t, err) + + release = sync.OnceFunc(release) + t.Cleanup(release) + + cli := NewCLIInstanceWithStateDir(fs, testStateDir) + cli.cmd = &cobra.Command{} + cli.cmd.SetIn(strings.NewReader("value")) + cli.cmd.SetOut(io.Discard) + + done := make(chan error, 1) + + go func() { done <- run(cli) }() + + return release, done +} + +// requireAsksBeforeLocking starts run while holding the state directory +// lock, as startWhileLocked does. run must fail, with an error wrapping +// want, without waiting for the lock. +func requireAsksBeforeLocking( + t *testing.T, run func(cli *Instance) error, want error, +) { + t.Helper() + + release, done := startWhileLocked(t, afero.NewMemMapFs(), run) + + timeout := time.After(lockWait) + + for !waitingForLock() { + select { + case err := <-done: + require.ErrorIs(t, err, want) + + return + case <-timeout: + t.Fatal("neither failed nor waited for the lock") + case <-time.After(time.Millisecond): + } + } + + // Let the command go on, so that it releases the lock + release() + + select { + case <-done: + case <-time.After(lockWait): + } + + t.Fatal("waited for the lock before asking") +} + +// TestCommandsCheckAgainOnceLocked checks that commands which check what +// they change before they ask for anything, and so before they take the +// state directory lock, check again once they hold it: another command may +// have changed it in the meantime. Each runs while the test holds the lock, +// given what it needs to ask for nothing. Once it waits for the lock, the +// test makes the change another command could have made, then releases the +// lock. The command must fail, and change nothing. +// +//nolint:paralleltest // waitingForLock sees any test's command waiting for the lock +func TestCommandsCheckAgainOnceLocked(t *testing.T) { + fingerprint := newTestGPGKey(t) + + mnemonic := testMnemonicBuffer(t) + + passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase)) + t.Cleanup(passphrase.Destroy) + + // work returns the vault "work" on fs, opened with the mnemonic + work := func(fs afero.Fs) *vault.Vault { + vlt := vault.NewVault(fs, testStateDir, "work") + vlt.Mnemonic = mnemonic + + return vlt + } + + for _, tc := range []struct { + name string + run func(cli *Instance) error + // change is what another command changes while run waits + change func(fs afero.Fs) error + // want is wrapped in the error the command fails with + want error + }{ + { + "unlocker add, vault removed", + func(cli *Instance) error { + cli.Mnemonic, cli.UnlockPassphrase = mnemonic, passphrase + + return cli.UnlockersAdd("passphrase", cli.cmd) + }, + func(fs afero.Fs) error { + return fs.RemoveAll(testStateDir + "/vaults.d/work") + }, + vault.ErrVaultNotFound, + }, + { + "unlocker add pgp, key added", + func(cli *Instance) error { + cli.Mnemonic = mnemonic + cli.cmd.Flags().String("keyid", unreadableTestGPGUserID, "") + + return cli.UnlockersAdd("pgp", cli.cmd) + }, + func(fs afero.Fs) error { + _, err := secret.CreatePGPUnlocker(work(fs), + unreadableTestGPGUserID, fingerprint) + + return err + }, + errGPGKeyAlreadyUnlocker, + }, + { + // Without the mnemonic or a passphrase, replacing the secret + // would ask for the passphrase; it does not exist yet, so the + // add asks for nothing + "add --force, secret added", + func(cli *Instance) error { + return cli.AddSecret("added", true) + }, + func(fs afero.Fs) error { + value := memguard.NewBufferFromBytes([]byte("other")) + defer value.Destroy() + + return work(fs).AddSecret("added", value, false) + }, + errAddedMeanwhile, + }, + } { + t.Run(tc.name, func(t *testing.T) { + requireChecksAgainOnceLocked(t, tc.run, tc.change, tc.want) + }) + } +} + +// requireChecksAgainOnceLocked starts run while holding the state directory +// lock, as startWhileLocked does. Once run waits for the lock, change makes +// the change that another command could have made in the meantime, and the +// lock is released. run must then fail, with an error wrapping want, and +// change nothing. +func requireChecksAgainOnceLocked( + t *testing.T, run func(cli *Instance) error, + change func(fs afero.Fs) error, want error, +) { + t.Helper() + + fs := afero.NewMemMapFs() + release, done := startWhileLocked(t, fs, run) + + requireWaitingForLock(t, done) + require.NoError(t, change(fs)) + + // Left out: taking and releasing the lock writes the lock file + lockFile := testStateDir + "/lock" + + before := stateDirModTimes(t, fs) + delete(before, lockFile) + + release() + + select { + case err := <-done: + require.ErrorIs(t, err, want) + case <-time.After(lockWait): + t.Fatal("did not finish once the lock was released") + } + + after := stateDirModTimes(t, fs) + delete(after, lockFile) + + assert.Equal(t, before, after, "changed the state directory") +} + // TestEncryptWithExistingKeyTakesNoLock checks that secret encrypt with a // key that already exists, which only reads the state directory, finishes // while another command holds the state directory lock. diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 59c64e9..8edb2e4 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -47,6 +47,10 @@ var ( errCrossVaultSourceUnqualified = errors.New( "source must specify vault (e.g., vault:secret) for cross-vault move") errMoveOntoItself = errors.New("cannot be moved onto itself") + // errAddedMeanwhile carries only the message tail; storeSecret composes + // "secret was added by another command ...". + errAddedMeanwhile = errors.New("was added by another command in the " + + "meantime, so nothing was changed; run the command again to replace it") ) // ParseVaultSecretRef parses a "vault:secret" or just "secret" reference @@ -422,26 +426,70 @@ func (cli *Instance) AddSecret(secretName string, force bool) error { // Locked only now that stdin has been read: in `secret encrypt key | // secret add name`, holding the lock while reading would leave each // command waiting for the other. + return cli.storeSecret(vlt, secretName, valueBuffer, force) +} + +// storeSecret adds the secret name with value to vlt, as vault.AddSecret +// does, holding the state directory lock while it does. With force, adding +// an existing secret replaces its current version, which needs the vault's +// long-term key. Getting the key can ask for the passphrase, so storeSecret +// gets it before it takes the lock, and no other command waits while the +// user answers. If, once the lock is held, the add replaces a version after +// all, because another command added the secret in the meantime, it fails +// rather than ask with the lock held. +func (cli *Instance) storeSecret( + vlt *vault.Vault, name string, value *memguard.LockedBuffer, force bool, +) error { + err := vault.ValidateSecretName(name) + if err != nil { + return err + } + + replacing, err := replacesVersion(vlt, name, force) + if err != nil { + return err + } + + if replacing { + _, err = vlt.GetOrDeriveLongTermKey() + if err != nil { + return fmt.Errorf("failed to get long-term key: %w", err) + } + } + release, err := vault.LockStateDir(cli.fs, cli.stateDir) if err != nil { return err } defer release() - // Add the secret to the vault - secret.Debug("Calling vault.AddSecret", "secret_name", secretName, - "value_length", valueBuffer.Size(), "force", force) + if !replacing { + replacing, err = replacesVersion(vlt, name, force) + if err != nil { + return err + } - err = vlt.AddSecret(secretName, valueBuffer, force) - if err != nil { - secret.Debug("vault.AddSecret failed", "error", err) - - return err + if replacing { + return fmt.Errorf("secret %s %w", name, errAddedMeanwhile) + } } - secret.Debug("vault.AddSecret completed successfully") + return vlt.AddSecret(name, value, force) +} - return nil +// replacesVersion reports whether adding the secret name to vlt replaces +// the secret's current version: whether force is set and the secret has one. +func replacesVersion(vlt *vault.Vault, name string, force bool) (bool, error) { + if !force { + return false, nil + } + + exists, err := secret.NewSecret(vlt, name).Exists() + if err != nil { + return false, fmt.Errorf("failed to check if secret exists: %w", err) + } + + return exists, nil } // GetSecret retrieves and prints the current version of a secret @@ -707,14 +755,7 @@ func (cli *Instance) ImportSecret( // Locked only now that the file has been read, as in AddSecret: the // file may be a pipe written by another secret command. - release, err := vault.LockStateDir(cli.fs, cli.stateDir) - if err != nil { - return err - } - defer release() - - // Store the secret in the vault - err = vlt.AddSecret(secretName, valueBuffer, force) + err = cli.storeSecret(vlt, secretName, valueBuffer, force) if err != nil { return err } diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index c64a8e8..026cfa7 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -404,12 +404,6 @@ func (cli *Instance) printUnlockersTable(unlockers []UnlockerInfo) error { // UnlockersAdd adds a new unlocker func (cli *Instance) UnlockersAdd(unlockerType string, cmd *cobra.Command) error { - release, err := vault.LockStateDir(cli.fs, cli.stateDir) - if err != nil { - return err - } - defer release() - switch unlockerType { case unlockerTypePassphrase: return cli.addPassphraseUnlocker(cmd) @@ -423,6 +417,34 @@ func (cli *Instance) UnlockersAdd(unlockerType string, cmd *cobra.Command) error } } +// unlockThenLock gets the long-term key of vlt, the vault an unlocker is +// added to, then takes the state directory lock and returns the function +// that releases it. Getting the key can ask for a passphrase, so it comes +// before the lock, as the question of askThenLock does, and no other +// command waits while the user answers. Once the lock is held, it fails if +// another command has removed the vault in the meantime, since writing the +// unlocker would create the vault's directory again, holding nothing else. +func (cli *Instance) unlockThenLock(vlt *vault.Vault) (func(), error) { + _, err := vlt.GetOrDeriveLongTermKey() + if err != nil { + return nil, fmt.Errorf("failed to get long-term key: %w", err) + } + + release, err := vault.LockStateDir(cli.fs, cli.stateDir) + if err != nil { + return nil, err + } + + _, err = cli.existingVault(vlt.GetName()) + if err != nil { + release() + + return nil, err + } + + return release, nil +} + // autoSelectUnlocker selects the newly created unlocker as current, // printing a warning if selection fails func autoSelectUnlocker(cmd *cobra.Command, vlt *vault.Vault, unlockerID string) { @@ -442,9 +464,6 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error { return fmt.Errorf("failed to get current vault: %w", err) } - // For passphrase unlockers, we don't need the vault to be unlocked - // The CreatePassphraseUnlocker method will handle getting the - // long-term key vlt.Mnemonic, vlt.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase // The new passphrase is asked for even when SB_UNLOCK_PASSPHRASE is @@ -464,6 +483,12 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error { defer passphraseBuffer.Destroy() } + release, err := cli.unlockThenLock(vlt) + if err != nil { + return err + } + defer release() + passphraseUnlocker, err := vlt.CreatePassphraseUnlocker(passphraseBuffer) if err != nil { return err @@ -502,12 +527,13 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error { cmd.Printf("Using default GPG key: %s\n", gpgKeyID) } - // Check if this key is already added as an unlocker vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { return fmt.Errorf("failed to get current vault: %w", err) } + vlt.Mnemonic, vlt.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase + // Resolve the GPG key ID to its fingerprint, once: the duplicate check // and the new unlocker's metadata both use this result fingerprint, err := secret.ResolveGPGKeyFingerprint(gpgKeyID) @@ -515,21 +541,26 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error { return fmt.Errorf("failed to resolve GPG key fingerprint: %w", err) } - // Check if this GPG key is already added - exists, err := cli.pgpUnlockerExists(vlt, fingerprint) + // Refuse a key that is already an unlocker before asking for anything, + // and again once the lock is held, since another command may have added + // it in the meantime + err = cli.checkPGPKeyNotAdded(vlt, gpgKeyID, fingerprint) if err != nil { - return fmt.Errorf( - "could not check whether GPG key %s is already an unlocker: %w", - gpgKeyID, err, - ) + return err } - if exists { - return fmt.Errorf("GPG key %s %w", gpgKeyID, errGPGKeyAlreadyUnlocker) + release, err := cli.unlockThenLock(vlt) + if err != nil { + return err + } + defer release() + + err = cli.checkPGPKeyNotAdded(vlt, gpgKeyID, fingerprint) + if err != nil { + return err } - pgpUnlocker, err := secret.CreatePGPUnlocker(cli.fs, cli.stateDir, - gpgKeyID, fingerprint, cli.Mnemonic, cli.UnlockPassphrase) + pgpUnlocker, err := secret.CreatePGPUnlocker(vlt, gpgKeyID, fingerprint) if err != nil { return err } @@ -704,6 +735,27 @@ func (cli *Instance) UnlockerSelect(unlockerID string) error { return vlt.SelectUnlocker(unlockerID) } +// checkPGPKeyNotAdded fails when vlt already has a PGP unlocker for the GPG +// key gpgKeyID, whose fingerprint is fingerprint, or when that cannot be +// checked. +func (cli *Instance) checkPGPKeyNotAdded( + vlt *vault.Vault, gpgKeyID, fingerprint string, +) error { + exists, err := cli.pgpUnlockerExists(vlt, fingerprint) + if err != nil { + return fmt.Errorf( + "could not check whether GPG key %s is already an unlocker: %w", + gpgKeyID, err, + ) + } + + if exists { + return fmt.Errorf("GPG key %s %w", gpgKeyID, errGPGKeyAlreadyUnlocker) + } + + return nil +} + // pgpUnlockerExists reports whether the vault already has a PGP unlocker // for the GPG key with the given fingerprint. It returns an error, and no // answer, when unlockers.d or an unlocker's metadata file cannot be read; diff --git a/internal/cli/unreadable_dir_test.go b/internal/cli/unreadable_dir_test.go index fef88c2..9494729 100644 --- a/internal/cli/unreadable_dir_test.go +++ b/internal/cli/unreadable_dir_test.go @@ -9,11 +9,12 @@ // "nothing there" and go ahead. // // The tests make the look fail with a wrapper around the in-memory -// filesystem, which the state directory lock refuses. So they call the -// function each command runs once it holds the lock, such as addPGPUnlocker -// for UnlockersAdd, or, for a removal, the function that makes its checks, -// such as findVaultToRemove for RemoveVault, which runs again under the -// lock before anything is removed, with --force or without. +// filesystem, which the state directory lock refuses. So they call a +// function that makes the look before any lock is taken: addPGPUnlocker for +// UnlockersAdd, which looks again once it holds the lock, or, for a removal, +// the function that makes its checks, such as findVaultToRemove for +// RemoveVault, which runs again under the lock before anything is removed, +// with --force or without. //nolint:testpackage // white-box test of unexported internals package cli diff --git a/internal/cli/vault.go b/internal/cli/vault.go index 0e2cd22..d82fa09 100644 --- a/internal/cli/vault.go +++ b/internal/cli/vault.go @@ -222,18 +222,14 @@ func (cli *Instance) resolvePassphrase() (*memguard.LockedBuffer, func(), error) return passphraseBuffer, passphraseBuffer.Destroy, nil } -// CreateVault creates a new vault +// CreateVault creates a new vault. It asks for the mnemonic and the +// passphrase before it takes the state directory lock, so that no other +// command waits while the user answers. func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error { secret.Debug("Creating new vault", "name", name, "state_dir", cli.stateDir) - release, err := vault.LockStateDir(cli.fs, cli.stateDir) - if err != nil { - return err - } - defer release() - // Refuse an invalid name or an existing vault before asking for anything - err = vault.CheckNewVaultName(cli.fs, cli.stateDir, name) + err := vault.CheckNewVaultName(cli.fs, cli.stateDir, name) if err != nil { return err } @@ -265,7 +261,14 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error { } defer cleanupPassphrase() - // Create the vault with its passphrase unlocker + release, err := vault.LockStateDir(cli.fs, cli.stateDir) + if err != nil { + return err + } + defer release() + + // Create the vault with its passphrase unlocker. Another command may + // have created it since the check above: vault.CreateVault checks again. vlt, err := vault.CreateVault(cli.fs, cli.stateDir, name, mnemonic, passphraseBuffer) if err != nil { diff --git a/internal/secret/pgpunlock_test.go b/internal/secret/pgpunlock_test.go index cd2b955..db7089d 100644 --- a/internal/secret/pgpunlock_test.go +++ b/internal/secret/pgpunlock_test.go @@ -375,8 +375,7 @@ func testCreatePGPUnlocker( } // Now create a PGP unlock key (this will use our custom GPGEncryptFunc) - pgpUnlocker, err := secret.CreatePGPUnlocker( - fs, stateDir, keyID, fingerprint, mnemonic, nil) + pgpUnlocker, err := secret.CreatePGPUnlocker(vlt, keyID, fingerprint) if err != nil { t.Fatalf("Failed to create PGP unlock key: %v", err) } diff --git a/internal/secret/pgpunlocker.go b/internal/secret/pgpunlocker.go index 27b2f24..24686bf 100644 --- a/internal/secret/pgpunlocker.go +++ b/internal/secret/pgpunlocker.go @@ -192,58 +192,45 @@ func generatePGPUnlockerName() (string, error) { return fmt.Sprintf("%s-pgp-%s", hostname, enrollmentTime), nil } -// pgpUnlockerDir returns the current vault and the directory in it for a -// new PGP unlocker, named after the host and the time. -// -//nolint:ireturn // the vault is only available behind VaultInterface -func pgpUnlockerDir( - fs afero.Fs, stateDir string, -) (VaultInterface, string, error) { - // Get current vault - vault, err := GetCurrentVault(fs, stateDir) - if err != nil { - return nil, "", fmt.Errorf("failed to get current vault: %w", err) - } - +// pgpUnlockerDir returns the directory in vault for a new PGP unlocker, +// named after the host and the time. +func pgpUnlockerDir(vault VaultInterface) (string, error) { // Generate the unlocker name based on hostname and time unlockerName, err := generatePGPUnlockerName() if err != nil { - return nil, "", fmt.Errorf("failed to generate unlocker name: %w", err) + return "", fmt.Errorf("failed to generate unlocker name: %w", err) } vaultDir, err := vault.GetDirectory() if err != nil { - return nil, "", fmt.Errorf("failed to get vault directory: %w", err) + return "", fmt.Errorf("failed to get vault directory: %w", err) } - return vault, filepath.Join(vaultDir, "unlockers.d", unlockerName), nil + return filepath.Join(vaultDir, "unlockers.d", unlockerName), nil } -// CreatePGPUnlocker creates a new PGP unlocker and stores it in the vault. +// CreatePGPUnlocker creates a new PGP unlocker and stores it in vault. // It encrypts to the GPG key gpgKeyID and records fingerprint, that key's // fingerprint as ResolveGPGKeyFingerprint returns it, in the metadata. // Everything that can fail short of writing a file is done before anything // is written, and the files are written through WriteDir, so a failure -// leaves no partial unlocker. The long-term key comes from mnemonic when it -// is not nil, else from the current unlocker, which is given passphrase when -// it is a passphrase unlocker. +// leaves no partial unlocker. The long-term key comes from vault's +// GetOrDeriveLongTermKey: the key it holds when it is unlocked, else the +// key from its mnemonic or its current unlocker. func CreatePGPUnlocker( - fs afero.Fs, stateDir, gpgKeyID, fingerprint string, - mnemonic, passphrase *memguard.LockedBuffer, + vault VaultInterface, gpgKeyID, fingerprint string, ) (*PGPUnlocker, error) { err := checkGPGAvailable() if err != nil { return nil, err } - vault, unlockerDir, err := pgpUnlockerDir(fs, stateDir) + unlockerDir, err := pgpUnlockerDir(vault) if err != nil { return nil, err } - // The vault's GetOrDeriveLongTermKey, in step 2, uses both - vault.SetMnemonic(mnemonic) - vault.SetUnlockPassphrase(passphrase) + fs := vault.GetFilesystem() // Step 1: Generate a new age keypair for the PGP unlocker ageIdentity, err := age.GenerateX25519Identity() @@ -295,7 +282,7 @@ func CreatePGPUnlocker( func encryptPGPUnlockerKeys( vault VaultInterface, ageIdentity *age.X25519Identity, gpgKeyID string, ) ([]byte, []byte, error) { - // From the mnemonic or the current unlocker, as for a passphrase unlocker + // The key vault holds, or else from its mnemonic or current unlocker ltIdentity, err := vault.GetOrDeriveLongTermKey() if err != nil { return nil, nil, fmt.Errorf("failed to get long-term key: %w", err) diff --git a/internal/secret/pgpunlocker_test.go b/internal/secret/pgpunlocker_test.go index 1d20326..4709570 100644 --- a/internal/secret/pgpunlocker_test.go +++ b/internal/secret/pgpunlocker_test.go @@ -58,7 +58,8 @@ func TestCreatePGPUnlockerFailureWritesNothing(t *testing.T) { }} _, err = secret.CreatePGPUnlocker( - fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, nil, nil) + vault.NewVault(fs, testVaultStateDir, testVaultName), + testGPGKeyID, testGPGFingerprint) require.Error(t, err) vaultDir, err := vlt.GetDirectory() @@ -87,17 +88,15 @@ func TestPGPUnlockerAddedTwiceKeepsFirst(t *testing.T) { fs := afero.NewMemMapFs() mnemonic := testMnemonicBuffer(t) - _, err := vault.CreateVault(fs, testVaultStateDir, testVaultName, mnemonic, nil) + vlt, err := vault.CreateVault(fs, testVaultStateDir, testVaultName, mnemonic, nil) require.NoError(t, err) - first, err := secret.CreatePGPUnlocker( - fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, mnemonic, nil) + first, err := secret.CreatePGPUnlocker(vlt, testGPGKeyID, testGPGFingerprint) require.NoError(t, err) firstFiles := dirFiles(t, fs, first.GetDirectory()) - second, err := secret.CreatePGPUnlocker( - fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, mnemonic, nil) + second, err := secret.CreatePGPUnlocker(vlt, testGPGKeyID, testGPGFingerprint) require.NoError(t, err) assert.NotEqual(t, first.GetDirectory(), second.GetDirectory()) assert.Equal(t, firstFiles, dirFiles(t, fs, first.GetDirectory())) diff --git a/internal/vault/management.go b/internal/vault/management.go index c3ed373..c881df1 100644 --- a/internal/vault/management.go +++ b/internal/vault/management.go @@ -217,9 +217,9 @@ func processMnemonicForVault( // 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. +// longer be decrypted. The commands that create a vault check it before they +// ask for the mnemonic or passphrase, and CreateVault checks it again, under +// the state directory lock, before writing anything. func CheckNewVaultName(fs afero.Fs, stateDir string, name string) error { err := ValidateVaultName(name) if err != nil {