Delete the keychain item or Secure Enclave key of a failed unlocker add (closes #89)

A Secure Enclave unlocker add gets the long-term key before it creates
the Secure Enclave key, so a wrong passphrase creates none, and deletes
the key if encrypting with it or writing the unlocker then fails. A
keychain unlocker add writes all of the unlocker's files before it
stores the keychain item, and deletes the item if moving the unlocker
into place then fails. A failure to delete is reported along with the
original error.

These files build only on macOS: the code is type-checked and linted
from Linux by script/lint-darwin; the new tests run only on a Mac.

Model: opus-5-5

internal/secret/atomic_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"filippo.io/age"
+	"git.eeqj.de/sneak/secret/internal/macse"
 	"git.eeqj.de/sneak/secret/internal/secret"
 	"git.eeqj.de/sneak/secret/internal/vault"
 	"github.com/awnumar/memguard"
@@ -899,3 +900,42 @@ func TestWriteDirRefusesExistingDir(t *testing.T) {
 		})
 	}
 }
+
+// TestSecureEnclaveUnlockerFailureDeletesKey makes moving a new Secure
+// Enclave unlocker into place fail after its Secure Enclave key is created:
+// the key must be deleted again. Skipped when the add fails before that, as
+// it does everywhere but in a macOS build with cgo on a Mac with a Secure
+// Enclave.
+func TestSecureEnclaveUnlockerFailureDeletesKey(t *testing.T) {
+	t.Parallel()
+
+	mnemonic := testMnemonicBuffer(t)
+	base := afero.NewMemMapFs()
+	_, err := vault.CreateVault(base, testVaultStateDir, testVaultName, mnemonic)
+	require.NoError(t, err)
+
+	// The unlocker's directory is named se-<label of its Secure Enclave key>
+	var seKeyLabel string
+
+	fs := hookFs{Fs: base, before: func(op, path string) error {
+		if op == opRename && filepath.Base(filepath.Dir(path)) == "unlockers.d" {
+			seKeyLabel = strings.TrimPrefix(filepath.Base(path), "se-")
+
+			return errInjected
+		}
+
+		return nil
+	}}
+
+	_, err = secret.CreateSecureEnclaveUnlocker(fs, testVaultStateDir, mnemonic,
+		nil)
+
+	if seKeyLabel == "" {
+		t.Skipf("the add failed before moving the unlocker into place: %v", err)
+	}
+
+	require.ErrorIs(t, err, errInjected)
+
+	_, err = macse.Encrypt(seKeyLabel, []byte("test"))
+	assert.Error(t, err, "Secure Enclave key left behind")
+}

internal/secret/keychainunlocker.go

@@ -490,6 +490,8 @@ func CreateKeychainUnlocker(
 
 // writeKeychainUnlocker writes a new keychain unlocker into unlockerDir and
 // stores its data in the keychain (steps 7 and 8 of CreateKeychainUnlocker).
+// The data is stored after the unlocker's files are written, and the keychain
+// item is deleted again if moving the unlocker into place then fails.
 func writeKeychainUnlocker(
 	fs afero.Fs, unlockerDir, keychainItemName, ageRecipient string,
 	encryptedAgePrivKey, encryptedLtPrivKey []byte,
@@ -510,8 +512,10 @@ func writeKeychainUnlocker(
 		return nil, fmt.Errorf("failed to marshal unlocker metadata: %w", err)
 	}
 
-	// Step 8: Write the unlocker's files and store the data in the keychain,
-	// the metadata last
+	// Step 8: Write the unlocker's files, the metadata last, then store the
+	// data in the keychain
+	stored := false
+
 	err = WriteDir(fs, unlockerDir, func(dir string) error {
 		err := WriteFileAtomic(fs, filepath.Join(dir, "pub.txt"), []byte(ageRecipient))
 		if err != nil {
@@ -528,19 +532,29 @@ func writeKeychainUnlocker(
 			return fmt.Errorf("failed to write encrypted long-term private key: %w", err)
 		}
 
-		err = storeInKeychain(keychainItemName, keychainDataBuffer)
-		if err != nil {
-			return fmt.Errorf("failed to store data in keychain: %w", err)
-		}
-
 		err = WriteFileAtomic(fs, filepath.Join(dir, "unlocker-metadata.json"),
 			metadataBytes)
 		if err != nil {
 			return fmt.Errorf("failed to write unlocker metadata: %w", err)
 		}
 
+		err = storeInKeychain(keychainItemName, keychainDataBuffer)
+		if err != nil {
+			return fmt.Errorf("failed to store data in keychain: %w", err)
+		}
+
+		stored = true
+
 		return nil
 	})
+	if err != nil && stored {
+		deleteErr := deleteFromKeychain(keychainItemName)
+		if deleteErr != nil {
+			err = errors.Join(err, fmt.Errorf(
+				"failed to delete keychain item %s: %w", keychainItemName, deleteErr))
+		}
+	}
+
 	if err != nil {
 		return nil, err
 	}

internal/secret/keychainunlocker_test.go

@@ -4,10 +4,13 @@ package secret
 
 import (
 	"encoding/hex"
+	"os"
+	"path/filepath"
 	"runtime"
 	"testing"
 
 	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -185,3 +188,27 @@ func TestDeleteNonExistentKeychainItem(t *testing.T) {
 	assert.NoError(t, err,
 		"Deleting non-existent keychain item should not return an error")
 }
+
+// TestWriteKeychainUnlockerFailureDeletesItem makes moving a new keychain
+// unlocker into place fail after its data is stored in the keychain: the
+// keychain item must be deleted again.
+func TestWriteKeychainUnlockerFailureDeletesItem(t *testing.T) {
+	testItemName := "test-secret-keychain-unlocker-cleanup"
+	_ = deleteFromKeychain(testItemName)
+
+	// Moving the unlocker into a read-only directory fails
+	unlockersDir := filepath.Join(t.TempDir(), "unlockers.d")
+	require.NoError(t, os.Mkdir(unlockersDir, 0o500))
+
+	testBuffer := memguard.NewBufferFromBytes([]byte("test-keychain-data"))
+	defer testBuffer.Destroy()
+
+	_, err := writeKeychainUnlocker(afero.NewOsFs(),
+		filepath.Join(unlockersDir, testItemName), testItemName, "age1test",
+		[]byte("test-priv"), []byte("test-longterm"), testBuffer)
+	require.ErrorIs(t, err, os.ErrPermission,
+		"moving the unlocker into place should fail")
+
+	_, err = retrieveFromKeychain(testItemName)
+	assert.Error(t, err, "keychain item left behind")
+}

internal/secret/seunlocker_darwin.go

@@ -4,6 +4,7 @@ package secret
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log/slog"
 	"os"
@@ -216,6 +217,8 @@ func generateSEKeyLabel(vaultName string) (string, error) {
 // using ECIES. No intermediate age keypair is used.
 // The long-term key comes from mnemonic when it is not nil, else from the
 // current unlocker, as getLongTermKeyForSE describes.
+// The SE key is created only once everything that does not need it has
+// succeeded, and is deleted again if writing the unlocker fails.
 func CreateSecureEnclaveUnlocker(
 	fs afero.Fs,
 	stateDir string,
@@ -237,17 +240,7 @@ func CreateSecureEnclaveUnlocker(
 		return nil, fmt.Errorf("failed to generate SE key label: %w", err)
 	}
 
-	// Step 1: Create P-256 key in the Secure Enclave via sc_auth
-	Debug("Creating Secure Enclave key", "label", seKeyLabel)
-
-	_, seKeyHash, err := macse.CreateKey(seKeyLabel)
-	if err != nil {
-		return nil, fmt.Errorf("failed to create SE key: %w", err)
-	}
-
-	Debug("Created SE key", "label", seKeyLabel, "hash", seKeyHash)
-
-	// Step 2: Get the vault's long-term private key
+	// Step 1: Get the vault's long-term private key
 	ltPrivKeyData, err := getLongTermKeyForSE(fs, vault, mnemonic, passphrase)
 	if err != nil {
 		return nil, fmt.Errorf(
@@ -257,16 +250,7 @@ func CreateSecureEnclaveUnlocker(
 	}
 	defer ltPrivKeyData.Destroy()
 
-	// Step 3: Encrypt the long-term key directly with the SE (ECIES)
-	encryptedLtKey, err := macse.Encrypt(seKeyLabel, ltPrivKeyData.Bytes())
-	if err != nil {
-		return nil, fmt.Errorf(
-			"failed to encrypt long-term key with SE: %w",
-			err,
-		)
-	}
-
-	// Step 4: Prepare the unlocker directory's path and metadata
+	// Step 2: Prepare the unlocker directory's path
 	vaultDir, err := vault.GetDirectory()
 	if err != nil {
 		return nil, fmt.Errorf("failed to get vault directory: %w", err)
@@ -275,6 +259,49 @@ func CreateSecureEnclaveUnlocker(
 	unlockerDirName := "se-" + filepath.Base(seKeyLabel)
 	unlockerDir := filepath.Join(vaultDir, "unlockers.d", unlockerDirName)
 
+	// Step 3: Create P-256 key in the Secure Enclave via sc_auth
+	Debug("Creating Secure Enclave key", "label", seKeyLabel)
+
+	_, seKeyHash, err := macse.CreateKey(seKeyLabel)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create SE key: %w", err)
+	}
+
+	Debug("Created SE key", "label", seKeyLabel, "hash", seKeyHash)
+
+	// Steps 4 and 5: Write the unlocker, or delete the SE key if that fails
+	unlocker, err := writeSEUnlocker(fs, unlockerDir, seKeyLabel, seKeyHash,
+		ltPrivKeyData)
+	if err != nil {
+		deleteErr := macse.DeleteKey(seKeyHash)
+		if deleteErr != nil {
+			err = errors.Join(err, fmt.Errorf(
+				"failed to delete SE key %s: %w", seKeyLabel, deleteErr))
+		}
+
+		return nil, err
+	}
+
+	return unlocker, nil
+}
+
+// writeSEUnlocker encrypts the long-term key with the SE key and writes the
+// new unlocker into unlockerDir (steps 4 and 5 of
+// CreateSecureEnclaveUnlocker).
+func writeSEUnlocker(
+	fs afero.Fs, unlockerDir, seKeyLabel, seKeyHash string,
+	ltPrivKeyData *memguard.LockedBuffer,
+) (*SecureEnclaveUnlocker, error) {
+	// Step 4: Encrypt the long-term key directly with the SE (ECIES), and
+	// prepare the metadata
+	encryptedLtKey, err := macse.Encrypt(seKeyLabel, ltPrivKeyData.Bytes())
+	if err != nil {
+		return nil, fmt.Errorf(
+			"failed to encrypt long-term key with SE: %w",
+			err,
+		)
+	}
+
 	seMetadata := SecureEnclaveUnlockerMetadata{
 		UnlockerMetadata: UnlockerMetadata{
 			Type:      seUnlockerType,