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 {