Leave no partial unlocker directory when adding an unlocker fails (closes #48)

CreatePGPUnlocker looked up the GPG key's fingerprint, and the keychain
unlocker got the long-term key, only after writing part of the unlocker,
so a failure there left a directory with no metadata. Both now do every
step that can fail before writing anything. `secret unlocker add pgp`
looks the fingerprint up once, for its duplicate check, and passes it to
CreatePGPUnlocker to record. All four unlocker types write their files
through the new secret.WriteDir, which builds a new directory in a
temporary directory, renames it into place when complete and removes it
on a failure. A directory that already exists, as when an unlocker
replaces one of the same name, is written in place and never removed.

Model: opus-5-5

internal/secret/atomic_test.go

@@ -35,6 +35,10 @@ const currentFile = "current"
 // unlockerMetadataFile is the file a new unlocker writes last.
 const unlockerMetadataFile = "unlocker-metadata.json"
 
+// privKeyFile is the file that holds the encrypted private key of a version
+// or of a passphrase unlocker.
+const privKeyFile = "priv.age"
+
 // unlockerPassphrase protects the passphrase unlockers the tests create.
 //
 //nolint:gosec // G101: test data, not a real credential
@@ -453,7 +457,7 @@ func TestVersionSaveIsWholeOrAbsent(t *testing.T) {
 
 				if exists {
 					assert.ElementsMatch(t,
-						[]string{"pub.age", "value.age", "priv.age", "metadata.age"},
+						[]string{"pub.age", "value.age", privKeyFile, "metadata.age"},
 						dirNames(t, base, versionDir),
 						"version directory visible before it was complete")
 				}
@@ -496,7 +500,7 @@ func TestVersionSaveFailureLeavesNothing(t *testing.T) {
 			writeLongTermKey(t, base, stateDir)
 
 			fs := hookFs{Fs: base, before: func(op, path string) error {
-				if op == opRename && filepath.Base(path) == "priv.age" {
+				if op == opRename && filepath.Base(path) == privKeyFile {
 					return errInjected
 				}
 
@@ -642,37 +646,119 @@ func TestPassphraseUnlockerGetsKeyFirst(t *testing.T) {
 	require.Error(t, err)
 }
 
-// TestPassphraseUnlockerWritesMetadataLast checks that the last file a new
-// passphrase unlocker writes in its directory is its metadata: an unlocker
-// directory without metadata is never used, so one interrupted earlier
-// cannot be.
-func TestPassphraseUnlockerWritesMetadataLast(t *testing.T) {
+// TestPassphraseUnlockerIsWholeOrAbsent checks, before every change that
+// creating a passphrase unlocker makes, that the unlocker's directory either
+// does not exist or holds all of its files: a crash or a failure at any point
+// leaves no partial unlocker.
+//
+//nolint:paralleltest // t.Setenv forbids t.Parallel
+func TestPassphraseUnlockerIsWholeOrAbsent(t *testing.T) {
 	t.Setenv(secret.EnvMnemonic, testMnemonic)
 
-	base := afero.NewMemMapFs()
-	vlt, err := vault.CreateVault(base, testVaultStateDir, testVaultName)
-	require.NoError(t, err)
+	files := []string{"pub.age", privKeyFile, "longterm.age", unlockerMetadataFile}
 
-	vaultDir, err := vlt.GetDirectory()
-	require.NoError(t, err)
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			base, stateDir := tfs.open(t)
+			vlt, err := vault.CreateVault(base, stateDir, testVaultName)
+			require.NoError(t, err)
 
-	unlockerDir := filepath.Join(vaultDir, "unlockers.d", "passphrase")
+			vaultDir, err := vlt.GetDirectory()
+			require.NoError(t, err)
 
-	var last string
+			unlockerDir := filepath.Join(vaultDir, "unlockers.d", "passphrase")
 
-	fs := hookFs{Fs: base, before: func(_, path string) error {
-		if filepath.Dir(path) == unlockerDir {
-			last = filepath.Base(path)
-		}
+			fs := hookFs{Fs: base, before: func(string, string) error {
+				exists, err := afero.DirExists(base, unlockerDir)
+				require.NoError(t, err)
 
-		return nil
-	}}
+				if exists {
+					assert.ElementsMatch(t, files, dirNames(t, base, unlockerDir),
+						"unlocker directory visible before it was complete")
+				}
 
-	passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
-	defer passphrase.Destroy()
+				return nil
+			}}
 
-	_, err = vault.NewVault(fs, testVaultStateDir, testVaultName).
-		CreatePassphraseUnlocker(passphrase)
-	require.NoError(t, err)
-	assert.Equal(t, unlockerMetadataFile, last)
+			passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
+			defer passphrase.Destroy()
+
+			_, err = vault.NewVault(fs, stateDir, testVaultName).
+				CreatePassphraseUnlocker(passphrase)
+			require.NoError(t, err)
+			assert.ElementsMatch(t, files, dirNames(t, base, unlockerDir))
+		})
+	}
+}
+
+// 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
+// cannot be removed either, that both failures are reported.
+func TestWriteDirFailureLeavesNothing(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			base, dir := tfs.open(t)
+			listed := filepath.Join(dir, "unlockers.d")
+			target := filepath.Join(listed, "new")
+
+			writeThenFail := func(tmp string) error {
+				require.NoError(t, secret.WriteFileAtomic(base,
+					filepath.Join(tmp, unlockerMetadataFile), []byte("{}")))
+
+				return errInjected
+			}
+
+			err := secret.WriteDir(base, target, writeThenFail)
+			require.ErrorIs(t, err, errInjected)
+
+			// Nothing in the directory that is listed, nor beside it
+			assert.Empty(t, dirNames(t, base, listed))
+			assert.Equal(t, []string{"unlockers.d"}, dirNames(t, base, dir))
+
+			fs := hookFs{Fs: base, before: func(op, _ string) error {
+				if op == opRemove {
+					return os.ErrPermission
+				}
+
+				return nil
+			}}
+
+			err = secret.WriteDir(fs, target, writeThenFail)
+			require.ErrorIs(t, err, errInjected)
+			require.ErrorIs(t, err, os.ErrPermission)
+			assert.Empty(t, dirNames(t, base, listed))
+		})
+	}
+}
+
+// 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) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			fs, dir := tfs.open(t)
+			target := filepath.Join(dir, "unlockers.d", "passphrase")
+			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)
+
+				return errInjected
+			})
+			require.ErrorIs(t, err, errInjected)
+			assert.Equal(t, []string{unlockerMetadataFile}, dirNames(t, fs, target))
+		})
+	}
 }