Give every new unlocker a directory of its own (closes #71)

A passphrase unlocker added to a vault that had one, and a PGP, keychain
or Secure Enclave unlocker added on the same day as another of its type,
were written into the existing unlocker's directory file by file, so a
crash part-way left a current unlocker whose files did not belong
together.

Unlocker directories, keychain items and Secure Enclave keys are now
named with the time to the nanosecond, and secret.WriteDir refuses a
directory that exists. Adding a passphrase unlocker writes the new one,
points current-unlocker at it, and only then removes the vault's other
passphrase unlockers.

Model: opus-5-5

internal/secret/atomic.go

@@ -3,6 +3,7 @@ package secret
 import (
 	"errors"
 	"fmt"
+	"os"
 	"path/filepath"
 
 	"github.com/spf13/afero"
@@ -62,13 +63,12 @@ func TempDirFor(fs afero.Fs, target string) (string, error) {
 	return dir, nil
 }
 
-// WriteDir calls write to write the files of the directory dir. When dir does
-// not exist yet, write writes them into a temporary directory from TempDirFor,
-// which is then renamed to dir, so that neither a failure nor a crash leaves
-// dir half-written; on a failure the temporary directory is removed, and a
-// failure to remove it is returned along with the first. A directory cannot be
-// renamed over one that has files in it, so when dir already exists, write
-// writes into it in place; dir is then never removed.
+// WriteDir calls write to write the files of the new directory dir into a
+// temporary directory from TempDirFor, which is then renamed to dir, so that
+// neither a failure nor a crash leaves dir half-written; on a failure the
+// temporary directory is removed, and a failure to remove it is returned
+// along with the first. A directory cannot be replaced in one rename, so if
+// dir already exists, WriteDir fails without calling write.
 func WriteDir(fs afero.Fs, dir string, write func(dir string) error) error {
 	exists, err := afero.Exists(fs, dir)
 	if err != nil {
@@ -76,7 +76,7 @@ func WriteDir(fs afero.Fs, dir string, write func(dir string) error) error {
 	}
 
 	if exists {
-		return write(dir)
+		return fmt.Errorf("failed to create %s: %w", dir, os.ErrExist)
 	}
 
 	// Create the directory the finished one is renamed into

internal/secret/atomic_test.go

@@ -191,6 +191,22 @@ func dirNames(t *testing.T, fs afero.Fs, dir string) []string {
 	return names
 }
 
+// dirFiles returns the contents of the files in dir, by name.
+func dirFiles(t *testing.T, fs afero.Fs, dir string) map[string]string {
+	t.Helper()
+
+	files := map[string]string{}
+
+	for _, name := range dirNames(t, fs, dir) {
+		data, err := afero.ReadFile(fs, filepath.Join(dir, name))
+		require.NoError(t, err)
+
+		files[name] = string(data)
+	}
+
+	return files
+}
+
 // writeLongTermKey gives the test vault under stateDir a new long-term key
 // and returns it.
 func writeLongTermKey(
@@ -668,14 +684,14 @@ func TestPassphraseUnlockerIsWholeOrAbsent(t *testing.T) {
 			vaultDir, err := vlt.GetDirectory()
 			require.NoError(t, err)
 
-			unlockerDir := filepath.Join(vaultDir, "unlockers.d", "passphrase")
+			// The vault has no unlocker yet, so any directory in here is
+			// the new one
+			unlockersDir := filepath.Join(vaultDir, "unlockers.d")
 
 			fs := hookFs{Fs: base, before: func(string, string) error {
-				exists, err := afero.DirExists(base, unlockerDir)
-				require.NoError(t, err)
-
-				if exists {
-					assert.ElementsMatch(t, files, dirNames(t, base, unlockerDir),
+				for _, name := range dirNames(t, base, unlockersDir) {
+					assert.ElementsMatch(t, files,
+						dirNames(t, base, filepath.Join(unlockersDir, name)),
 						"unlocker directory visible before it was complete")
 				}
 
@@ -688,13 +704,130 @@ func TestPassphraseUnlockerIsWholeOrAbsent(t *testing.T) {
 			hooked := vault.NewVault(fs, stateDir, testVaultName)
 			hooked.Mnemonic = vlt.Mnemonic
 
-			_, err = hooked.CreatePassphraseUnlocker(passphrase)
+			unlocker, err := hooked.CreatePassphraseUnlocker(passphrase)
 			require.NoError(t, err)
-			assert.ElementsMatch(t, files, dirNames(t, base, unlockerDir))
+			assert.ElementsMatch(t, files, dirNames(t, base, unlocker.GetDirectory()))
 		})
 	}
 }
 
+// TestPassphraseUnlockerReplacementKeepsVaultOpen replaces the vault's
+// passphrase unlocker twice, each time with only the current unlocker to open
+// the vault. The first replacement fails right after making the new unlocker
+// current, so the old one is not removed. The second checks, before every
+// change it makes, that the vault opens with the passphrase through its
+// current unlocker, which is what a crash at that change would leave; once it
+// returns, the vault must have one passphrase unlocker left.
+func TestPassphraseUnlockerReplacementKeepsVaultOpen(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			base, stateDir := tfs.open(t)
+			vlt, err := vault.CreateVault(base, stateDir, testVaultName,
+				testMnemonicBuffer(t))
+			require.NoError(t, err)
+
+			ltIdentity, err := vlt.GetOrDeriveLongTermKey()
+			require.NoError(t, err)
+
+			passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
+			defer passphrase.Destroy()
+
+			_, err = vlt.CreatePassphraseUnlocker(passphrase)
+			require.NoError(t, err)
+
+			vaultDir, err := vlt.GetDirectory()
+			require.NoError(t, err)
+
+			currentUnlockerPath := filepath.Join(vaultDir, "current-unlocker")
+
+			// Every change after the switch to the new unlocker fails
+			switched := false
+			failAfterSwitch := hookFs{Fs: base, before: func(op, path string) error {
+				if switched {
+					return errInjected
+				}
+
+				switched = op == opRename && path == currentUnlockerPath
+
+				return nil
+			}}
+
+			replacing := vault.NewVault(failAfterSwitch, stateDir, testVaultName)
+			replacing.Unlock(ltIdentity)
+
+			_, err = replacing.CreatePassphraseUnlocker(passphrase)
+			require.ErrorIs(t, err, errInjected)
+
+			unlockers, err := vlt.ListUnlockers()
+			require.NoError(t, err)
+			assert.Len(t, unlockers, 2, "the old unlocker is left beside the new")
+
+			assertOpens := vaultOpensCheck(t, base, stateDir, ltIdentity, passphrase)
+			checked := hookFs{Fs: base, before: func(string, string) error {
+				assertOpens()
+
+				return nil
+			}}
+
+			replacing = vault.NewVault(checked, stateDir, testVaultName)
+			replacing.Unlock(ltIdentity)
+
+			_, err = replacing.CreatePassphraseUnlocker(passphrase)
+			require.NoError(t, err)
+			assertOpens()
+
+			unlockers, err = vlt.ListUnlockers()
+			require.NoError(t, err)
+			assert.Len(t, unlockers, 1)
+		})
+	}
+}
+
+// vaultOpensCheck returns a function that checks that the test vault under
+// stateDir opens through its current unlocker, with passphrase, to the
+// long-term key ltIdentity. Opening it takes a second, so an unlocker
+// directory it has opened through before is not opened again: it must hold
+// the same files as then.
+func vaultOpensCheck(
+	t *testing.T, fs afero.Fs, stateDir string, ltIdentity *age.X25519Identity,
+	passphrase *memguard.LockedBuffer,
+) func() {
+	t.Helper()
+
+	vaultDir := filepath.Join(stateDir, "vaults.d", testVaultName)
+
+	// The files of each unlocker directory the vault has opened through
+	opened := map[string]map[string]string{}
+
+	return func() {
+		t.Helper()
+
+		current, err := afero.ReadFile(fs, filepath.Join(vaultDir, "current-unlocker"))
+		require.NoError(t, err)
+
+		files := dirFiles(t, fs, filepath.Join(vaultDir, "unlockers.d", string(current)))
+
+		if before, ok := opened[string(current)]; ok {
+			assert.Equal(t, before, files, "unlocker changed since it opened the vault")
+
+			return
+		}
+
+		opener := vault.NewVault(fs, stateDir, testVaultName)
+		opener.UnlockPassphrase = passphrase
+
+		key, err := opener.UnlockVault()
+		require.NoError(t, err)
+		assert.Equal(t, ltIdentity.Recipient().String(), key.Recipient().String())
+
+		opened[string(current)] = files
+	}
+}
+
 // TestWriteDirFailureLeavesNothing makes writing a new directory fail after
 // a file has been written in it, and checks that neither the directory nor
 // its temporary directory is left behind; and, when the temporary directory
@@ -740,10 +873,10 @@ func TestWriteDirFailureLeavesNothing(t *testing.T) {
 	}
 }
 
-// TestWriteDirKeepsExistingDir makes writing into a directory that already
-// exists fail, and checks that the directory, with what was in it, is still
-// there: WriteDir writes into it in place and never removes it.
-func TestWriteDirKeepsExistingDir(t *testing.T) {
+// TestWriteDirRefusesExistingDir checks that WriteDir fails, without calling
+// write, when the directory already exists, and leaves the directory as it
+// was: it never writes into a directory in place.
+func TestWriteDirRefusesExistingDir(t *testing.T) {
 	t.Parallel()
 
 	for _, tfs := range testFilesystems {
@@ -751,17 +884,17 @@ func TestWriteDirKeepsExistingDir(t *testing.T) {
 			t.Parallel()
 
 			fs, dir := tfs.open(t)
-			target := filepath.Join(dir, "unlockers.d", "passphrase")
+			target := filepath.Join(dir, "unlockers.d", "existing")
 			require.NoError(t, fs.MkdirAll(target, secret.DirPerms))
 			require.NoError(t, secret.WriteFileAtomic(fs,
 				filepath.Join(target, unlockerMetadataFile), []byte("{}")))
 
-			err := secret.WriteDir(fs, target, func(got string) error {
-				assert.Equal(t, target, got)
+			err := secret.WriteDir(fs, target, func(string) error {
+				t.Error("write called for a directory that exists")
 
-				return errInjected
+				return nil
 			})
-			require.ErrorIs(t, err, errInjected)
+			require.ErrorIs(t, err, os.ErrExist)
 			assert.Equal(t, []string{unlockerMetadataFile}, dirNames(t, fs, target))
 		})
 	}

internal/secret/constants.go

@@ -16,6 +16,12 @@ const (
 	EnvUnlockPassphrase = "SB_UNLOCK_PASSPHRASE"
 	// EnvGPGKeyID is the environment variable for providing the GPG key ID
 	EnvGPGKeyID = "SB_GPG_KEY_ID"
+
+	// UnlockerTimeFormat is the layout of the time, in UTC, in the name of a
+	// new unlocker's directory, keychain item and Secure Enclave key. It runs
+	// to the nanosecond, so that every new unlocker, even one added right
+	// after another, gets a directory of its own.
+	UnlockerTimeFormat = "2006-01-02.15.04.05.000000000"
 )
 
 // File system permission constants

internal/secret/keychainunlocker.go

@@ -233,10 +233,10 @@ func generateKeychainUnlockerName(vaultName string) (string, error) {
 		return "", fmt.Errorf("failed to get hostname: %w", err)
 	}
 
-	// Format: secret-<vault>-<hostname>-<date>
-	enrollmentDate := time.Now().Format("2006-01-02")
+	// Format: secret-<vault>-<hostname>-<time>
+	enrollmentTime := time.Now().UTC().Format(UnlockerTimeFormat)
 
-	return fmt.Sprintf("secret-%s-%s-%s", vaultName, hostname, enrollmentDate), nil
+	return fmt.Sprintf("secret-%s-%s-%s", vaultName, hostname, enrollmentTime), nil
 }
 
 // getLongTermPrivateKey derives the long-term private key from mnemonic when

internal/secret/pgpunlocker.go

@@ -209,21 +209,20 @@ func (p *PGPUnlocker) GetGPGKeyID() (string, error) {
 }
 
 // generatePGPUnlockerName generates a unique name for the PGP unlocker
-// based on hostname and date
+// based on hostname and time
 func generatePGPUnlockerName() (string, error) {
 	hostname, err := os.Hostname()
 	if err != nil {
 		return "", fmt.Errorf("failed to get hostname: %w", err)
 	}
 
-	// Format: hostname-pgp-YYYY-MM-DD
-	enrollmentDate := time.Now().Format("2006-01-02")
+	enrollmentTime := time.Now().UTC().Format(UnlockerTimeFormat)
 
-	return fmt.Sprintf("%s-pgp-%s", hostname, enrollmentDate), nil
+	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 day.
+// new PGP unlocker, named after the host and the time.
 //
 //nolint:ireturn // the vault is only available behind VaultInterface
 func pgpUnlockerDir(
@@ -235,7 +234,7 @@ func pgpUnlockerDir(
 		return nil, "", fmt.Errorf("failed to get current vault: %w", err)
 	}
 
-	// Generate the unlocker name based on hostname and date
+	// 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)

internal/secret/pgpunlocker_test.go

@@ -7,6 +7,7 @@ import (
 
 	"git.eeqj.de/sneak/secret/internal/secret"
 	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/awnumar/memguard"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -64,3 +65,40 @@ func TestCreatePGPUnlockerFailureWritesNothing(t *testing.T) {
 	require.NoError(t, err)
 	assert.Empty(t, dirNames(t, base, filepath.Join(vaultDir, "unlockers.d")))
 }
+
+// TestPGPUnlockerAddedTwiceKeepsFirst adds two PGP unlockers one right after
+// the other, so on the same host and day, and checks that the second gets a
+// directory of its own and leaves the first one's files as they were.
+// CreatePGPUnlocker does not check whether the GPG key already has an
+// unlocker, so the test key serves for both.
+//
+//nolint:paralleltest // installFakeGPG uses t.Setenv
+func TestPGPUnlockerAddedTwiceKeepsFirst(t *testing.T) {
+	installFakeGPG(t)
+
+	original := secret.GPGEncryptFunc
+
+	t.Cleanup(func() { secret.GPGEncryptFunc = original })
+
+	// Stands in for gpg, which the test does not have: "encrypts" by copying
+	secret.GPGEncryptFunc = func(data *memguard.LockedBuffer, _ string) ([]byte, error) {
+		return []byte(data.String()), nil
+	}
+
+	fs := afero.NewMemMapFs()
+	mnemonic := testMnemonicBuffer(t)
+	_, err := vault.CreateVault(fs, testVaultStateDir, testVaultName, mnemonic)
+	require.NoError(t, err)
+
+	first, err := secret.CreatePGPUnlocker(
+		fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, mnemonic, nil)
+	require.NoError(t, err)
+
+	firstFiles := dirFiles(t, fs, first.GetDirectory())
+
+	second, err := secret.CreatePGPUnlocker(
+		fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, mnemonic, nil)
+	require.NoError(t, err)
+	assert.NotEqual(t, first.GetDirectory(), second.GetDirectory())
+	assert.Equal(t, firstFiles, dirFiles(t, fs, first.GetDirectory()))
+}

internal/secret/seunlocker_darwin.go

@@ -193,14 +193,14 @@ func generateSEKeyLabel(vaultName string) (string, error) {
 		return "", fmt.Errorf("failed to get hostname: %w", err)
 	}
 
-	enrollmentDate := time.Now().UTC().Format("2006-01-02")
+	enrollmentTime := time.Now().UTC().Format(UnlockerTimeFormat)
 
 	return fmt.Sprintf(
 		"%s.%s-%s-%s",
 		seKeyLabelPrefix,
 		vaultName,
 		hostname,
-		enrollmentDate,
+		enrollmentTime,
 	), nil
 }