diff --git a/TODO.md b/TODO.md index 1e88c5e..f04fe2f 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,17 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-04: A failed unlocker add no longer leaves a partial unlocker + directory (https://git.eeqj.de/sneak/secret/issues/48). + `secret unlocker add pgp` resolves the GPG key's fingerprint once, for + its duplicate check, and passes it to `CreatePGPUnlocker` to record. + `CreatePGPUnlocker` and `CreateKeychainUnlocker` get the long-term key + and encrypt everything before writing anything. All four unlocker + types write their files through `secret.WriteDir`: a new unlocker is + built in a temporary directory, renamed into place when complete and + removed on a failure. One added under the directory name of an + existing unlocker is still written into that directory in place + (https://git.eeqj.de/sneak/secret/issues/71). - 2026-10-04: `secret unlocker select` and `secret unlocker remove` skip, with the warning `unlocker list` gives, an unlocker directory whose metadata file cannot be checked for, read or parsed, instead of @@ -129,13 +140,10 @@ Bring the repo into policy compliance in one commit: - from `init` or `vault create` killed after the passphrase prompt but before the unlocker is written, a vault with no unlocker, which `vault create` has already made the current vault; - - from an unlocker add stopped before its metadata is written, a - directory that `unlocker list` warns about and `unlocker rm` - removes only by its directory name; - - data under a `.tmp-` name in the state directory: a secret or - version being added, or the secret, version, unlocker or vault - being removed, encrypted keys included. Nothing deletes it; it - must be deleted by hand + - data under a `.tmp-` name in the state directory: a secret, + version or unlocker being added, or the secret, version, unlocker + or vault being removed, encrypted keys included. Nothing deletes + it; it must be deleted by hand (https://git.eeqj.de/sneak/secret/issues/75). - 2026-10-03: The checks run before changing a vault now stop with an error naming the path and cause when they cannot read what they diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index 181431c..2e48c0e 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -685,7 +685,8 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error { return fmt.Errorf("failed to get current vault: %w", err) } - // Resolve the GPG key ID to its fingerprint + // 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) if err != nil { return fmt.Errorf("failed to resolve GPG key fingerprint: %w", err) @@ -706,7 +707,8 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error { return fmt.Errorf("GPG key %s %w", gpgKeyID, errGPGKeyAlreadyUnlocker) } - pgpUnlocker, err := secret.CreatePGPUnlocker(cli.fs, cli.stateDir, gpgKeyID) + pgpUnlocker, err := secret.CreatePGPUnlocker( + cli.fs, cli.stateDir, gpgKeyID, fingerprint) if err != nil { return err } diff --git a/internal/cli/unlockers_add_test.go b/internal/cli/unlockers_add_test.go new file mode 100644 index 0000000..be538ba --- /dev/null +++ b/internal/cli/unlockers_add_test.go @@ -0,0 +1,35 @@ +//nolint:testpackage // white-box test of unexported internals +package cli + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +// unknownTestGPGUserID is a GPG user ID that no key in the test keyring has. +const unknownTestGPGUserID = "not-in-keyring@example.com" + +// TestAddPGPUnlockerUnknownKey asserts that adding a PGP unlocker for a key +// the keyring does not hold fails at looking up the key's fingerprint and +// leaves no new unlocker directory. The error must come from the lookup: a +// lookup moved after anything is written would also come after getting the +// vault's long-term key, which fails first on every platform but macOS +// (https://git.eeqj.de/sneak/secret/issues/88). +// +//nolint:paralleltest // t.Setenv (GNUPGHOME) forbids parallel tests +func TestAddPGPUnlockerUnknownKey(t *testing.T) { + newTestGPGKey(t) + + base := newListTestVault(t, 1) + instance, cmd := newTestInstance(base) + cmd.Flags().String("keyid", unknownTestGPGUserID, "") + + err := instance.addPGPUnlocker(cmd) + + require.ErrorContains(t, err, "failed to resolve GPG key fingerprint") + assertDirEntries(t, base, + filepath.Join(testVaultDir(listTestVaultName), listTestUnlockersDirName), + listTestUnlockerDirOne) +} diff --git a/internal/secret/atomic.go b/internal/secret/atomic.go index f9c85fb..7a36289 100644 --- a/internal/secret/atomic.go +++ b/internal/secret/atomic.go @@ -1,6 +1,7 @@ package secret import ( + "errors" "fmt" "path/filepath" @@ -61,6 +62,52 @@ 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. +func WriteDir(fs afero.Fs, dir string, write func(dir string) error) error { + exists, err := afero.Exists(fs, dir) + if err != nil { + return fmt.Errorf("failed to check for %s: %w", dir, err) + } + + if exists { + return write(dir) + } + + // Create the directory the finished one is renamed into + err = fs.MkdirAll(filepath.Dir(dir), DirPerms) + if err != nil { + return fmt.Errorf("failed to create %s: %w", filepath.Dir(dir), err) + } + + tmp, err := TempDirFor(fs, dir) + if err != nil { + return err + } + + err = write(tmp) + if err == nil { + err = fs.Rename(tmp, dir) + } + + if err != nil { + removeErr := fs.RemoveAll(tmp) + if removeErr != nil { + err = errors.Join(err, + fmt.Errorf("failed to remove %s: %w", tmp, removeErr)) + } + + return err + } + + return nil +} + // RemoveDirAtomic deletes the directory dir so that it disappears in one // rename: dir is moved into a new directory from TempDirFor, which is then // deleted. A crash part-way leaves only that temporary directory behind. diff --git a/internal/secret/atomic_test.go b/internal/secret/atomic_test.go index 739c09b..553f450 100644 --- a/internal/secret/atomic_test.go +++ b/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)) + }) + } } diff --git a/internal/secret/keychainunlocker.go b/internal/secret/keychainunlocker.go index f3f5a6f..7139b14 100644 --- a/internal/secret/keychainunlocker.go +++ b/internal/secret/keychainunlocker.go @@ -341,16 +341,13 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er return nil, fmt.Errorf("failed to generate keychain item name: %w", err) } - // Create unlocker directory using the keychain item name as the directory name + // The unlocker directory is named after the keychain item vaultDir, err := vault.GetDirectory() if err != nil { return nil, fmt.Errorf("failed to get vault directory: %w", err) } unlockerDir := filepath.Join(vaultDir, "unlockers.d", keychainItemName) - if err := fs.MkdirAll(unlockerDir, DirPerms); err != nil { - return nil, fmt.Errorf("failed to create unlocker directory: %w", err) - } // Step 1: Generate a new age keypair for the keychain unlocker ageIdentity, err := age.GenerateX25519Identity() @@ -358,6 +355,8 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er return nil, fmt.Errorf("failed to generate age keypair: %w", err) } + ageRecipient := ageIdentity.Recipient().String() + // Step 2: Generate a random passphrase for encrypting the age private key agePrivKeyPassphrase, err := generateRandomPassphrase(agePrivKeyPassphraseLength) if err != nil { @@ -365,14 +364,7 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er } defer agePrivKeyPassphrase.Destroy() - // Step 3: Store age recipient as plaintext - ageRecipient := ageIdentity.Recipient().String() - recipientPath := filepath.Join(unlockerDir, "pub.txt") - if err := WriteFileAtomic(fs, recipientPath, []byte(ageRecipient)); err != nil { - return nil, fmt.Errorf("failed to write age recipient: %w", err) - } - - // Step 4: Encrypt age private key with the generated passphrase and store on disk + // Step 3: Encrypt age private key with the generated passphrase // Create a secure buffer for the private key agePrivKeyStr := ageIdentity.String() agePrivKeyBuffer := memguard.NewBufferFromBytes([]byte(agePrivKeyStr)) @@ -383,31 +375,20 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er return nil, fmt.Errorf("failed to encrypt age private key with passphrase: %w", err) } - agePrivKeyPath := filepath.Join(unlockerDir, "priv.age") - if err := WriteFileAtomic(fs, agePrivKeyPath, encryptedAgePrivKey); err != nil { - return nil, fmt.Errorf("failed to write encrypted age private key: %w", err) - } - - // Step 5: Get or derive the long-term private key + // Step 4: Get or derive the long-term private key ltPrivKeyData, err := getLongTermPrivateKey(fs, vault) if err != nil { return nil, err } defer ltPrivKeyData.Destroy() - // Step 6: Encrypt long-term private key to the new age unlocker + // Step 5: Encrypt long-term private key to the new age unlocker encryptedLtPrivKeyToAge, err := EncryptToRecipient(ltPrivKeyData, ageIdentity.Recipient()) if err != nil { return nil, fmt.Errorf("failed to encrypt long-term private key to age unlocker: %w", err) } - // Write encrypted long-term private key - ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age") - if err := WriteFileAtomic(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge); err != nil { - return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err) - } - - // Step 7: Prepare keychain data + // Step 6: Prepare keychain data keychainData := KeychainData{ AgePublicKey: ageRecipient, AgePrivKeyPassphrase: agePrivKeyPassphrase, @@ -420,12 +401,7 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er } defer keychainDataBuffer.Destroy() - // Step 8: Store data in keychain - if err := storeInKeychain(keychainItemName, keychainDataBuffer); err != nil { - return nil, fmt.Errorf("failed to store data in keychain: %w", err) - } - - // Step 9: Create and write enhanced metadata + // Step 7: Prepare enhanced metadata keychainMetadata := KeychainUnlockerMetadata{ UnlockerMetadata: UnlockerMetadata{ Type: "keychain", @@ -440,10 +416,37 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er return nil, fmt.Errorf("failed to marshal unlocker metadata: %w", err) } - if err := WriteFileAtomic(fs, - filepath.Join(unlockerDir, "unlocker-metadata.json"), - metadataBytes); err != nil { - return nil, fmt.Errorf("failed to write unlocker metadata: %w", err) + // Step 8: Write the unlocker's files and store the data in the keychain, + // the metadata last + err = WriteDir(fs, unlockerDir, func(dir string) error { + pubPath := filepath.Join(dir, "pub.txt") + if err := WriteFileAtomic(fs, pubPath, []byte(ageRecipient)); err != nil { + return fmt.Errorf("failed to write age recipient: %w", err) + } + + privPath := filepath.Join(dir, "priv.age") + if err := WriteFileAtomic(fs, privPath, encryptedAgePrivKey); err != nil { + return fmt.Errorf("failed to write encrypted age private key: %w", err) + } + + ltKeyPath := filepath.Join(dir, "longterm.age") + if err := WriteFileAtomic(fs, ltKeyPath, encryptedLtPrivKeyToAge); err != nil { + return fmt.Errorf("failed to write encrypted long-term private key: %w", err) + } + + if err := storeInKeychain(keychainItemName, keychainDataBuffer); err != nil { + return fmt.Errorf("failed to store data in keychain: %w", err) + } + + metadataPath := filepath.Join(dir, "unlocker-metadata.json") + if err := WriteFileAtomic(fs, metadataPath, metadataBytes); err != nil { + return fmt.Errorf("failed to write unlocker metadata: %w", err) + } + + return nil + }) + if err != nil { + return nil, err } return &KeychainUnlocker{ diff --git a/internal/secret/pgpunlock_test.go b/internal/secret/pgpunlock_test.go index 1291534..ffd535c 100644 --- a/internal/secret/pgpunlock_test.go +++ b/internal/secret/pgpunlock_test.go @@ -290,7 +290,7 @@ Passphrase: ` + testPassphrase + ` } // Now create a PGP unlock key (this will use our custom GPGEncryptFunc) - pgpUnlocker, err := secret.CreatePGPUnlocker(fs, stateDir, keyID) + pgpUnlocker, err := secret.CreatePGPUnlocker(fs, stateDir, 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 4ce3664..4eafaa6 100644 --- a/internal/secret/pgpunlocker.go +++ b/internal/secret/pgpunlocker.go @@ -222,20 +222,13 @@ func generatePGPUnlockerName() (string, error) { return fmt.Sprintf("%s-pgp-%s", hostname, enrollmentDate), nil } -// preparePGPUnlockerDir checks GPG availability and creates the -// unlocker directory in the current vault, returning the vault and the -// directory path. +// pgpUnlockerDir returns the current vault and the directory in it for a +// new PGP unlocker, named after the host and the day. // //nolint:ireturn // the vault is only available behind VaultInterface -func preparePGPUnlockerDir( +func pgpUnlockerDir( fs afero.Fs, stateDir string, ) (VaultInterface, string, error) { - // Check if GPG is available - err := checkGPGAvailable() - if err != nil { - return nil, "", err - } - // Get current vault vault, err := GetCurrentVault(fs, stateDir) if err != nil { @@ -248,27 +241,29 @@ func preparePGPUnlockerDir( return nil, "", fmt.Errorf("failed to generate unlocker name: %w", err) } - // Create unlocker directory using the generated name vaultDir, err := vault.GetDirectory() if err != nil { return nil, "", fmt.Errorf("failed to get vault directory: %w", err) } - unlockerDir := filepath.Join(vaultDir, "unlockers.d", unlockerName) - - err = fs.MkdirAll(unlockerDir, DirPerms) - if err != nil { - return nil, "", fmt.Errorf("failed to create unlocker directory: %w", err) - } - - return vault, unlockerDir, nil + return vault, 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 the 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. func CreatePGPUnlocker( - fs afero.Fs, stateDir string, gpgKeyID string, + fs afero.Fs, stateDir, gpgKeyID, fingerprint string, ) (*PGPUnlocker, error) { - vault, unlockerDir, err := preparePGPUnlockerDir(fs, stateDir) + err := checkGPGAvailable() + if err != nil { + return nil, err + } + + vault, unlockerDir, err := pgpUnlockerDir(fs, stateDir) if err != nil { return nil, err } @@ -279,77 +274,13 @@ func CreatePGPUnlocker( return nil, fmt.Errorf("failed to generate age keypair: %w", err) } - // Step 2: Store age recipient as plaintext - ageRecipient := ageIdentity.Recipient().String() - recipientPath := filepath.Join(unlockerDir, "pub.txt") - - err = WriteFileAtomic(fs, recipientPath, []byte(ageRecipient)) - if err != nil { - return nil, fmt.Errorf("failed to write age recipient: %w", err) - } - - // Step 3: Get or derive the long-term private key - ltPrivKeyData, err := getLongTermPrivateKey(fs, vault) + // Step 2: Encrypt the long-term private key to the new keypair, and the + // keypair's private key to the GPG key + encryptedLtPrivKey, encryptedAgePrivKey, err := encryptPGPUnlockerKeys( + fs, vault, ageIdentity, gpgKeyID) if err != nil { return nil, err } - defer ltPrivKeyData.Destroy() - - // Step 7: Encrypt long-term private key to the new age unlocker - encryptedLtPrivKeyToAge, err := EncryptToRecipient( - ltPrivKeyData, ageIdentity.Recipient()) - if err != nil { - return nil, fmt.Errorf( - "failed to encrypt long-term private key to age unlocker: %w", err) - } - - // Write encrypted long-term private key - ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age") - - err = WriteFileAtomic(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge) - if err != nil { - return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err) - } - - // Step 8: Encrypt age private key to the GPG key ID - // Use memguard to protect the private key in memory - agePrivateKeyBuffer := memguard.NewBufferFromBytes([]byte(ageIdentity.String())) - defer agePrivateKeyBuffer.Destroy() - - encryptedAgePrivKey, err := GPGEncryptFunc(agePrivateKeyBuffer, gpgKeyID) - if err != nil { - return nil, fmt.Errorf("failed to encrypt age private key with GPG: %w", err) - } - - agePrivKeyPath := filepath.Join(unlockerDir, "priv.age.gpg") - - err = WriteFileAtomic(fs, agePrivKeyPath, encryptedAgePrivKey) - if err != nil { - return nil, fmt.Errorf("failed to write encrypted age private key: %w", err) - } - - // Steps 9-10: Resolve the fingerprint and write enhanced metadata - pgpMetadata, err := writePGPUnlockerMetadata(fs, unlockerDir, gpgKeyID) - if err != nil { - return nil, err - } - - return &PGPUnlocker{ - Directory: unlockerDir, - Metadata: pgpMetadata.UnlockerMetadata, - fs: fs, - }, nil -} - -// writePGPUnlockerMetadata resolves the GPG key fingerprint and writes -// the unlocker metadata file, returning the metadata written. -func writePGPUnlockerMetadata( - fs afero.Fs, unlockerDir string, gpgKeyID string, -) (*PGPUnlockerMetadata, error) { - fingerprint, err := ResolveGPGKeyFingerprint(gpgKeyID) - if err != nil { - return nil, fmt.Errorf("failed to resolve GPG key fingerprint: %w", err) - } pgpMetadata := PGPUnlockerMetadata{ UnlockerMetadata: UnlockerMetadata{ @@ -365,13 +296,85 @@ func writePGPUnlockerMetadata( return nil, fmt.Errorf("failed to marshal unlocker metadata: %w", err) } - err = WriteFileAtomic(fs, - filepath.Join(unlockerDir, "unlocker-metadata.json"), metadataBytes) + // Step 3: Write the unlocker's files, the metadata last + err = WriteDir(fs, unlockerDir, func(dir string) error { + return writePGPUnlockerFiles(fs, dir, ageIdentity.Recipient(), + encryptedLtPrivKey, encryptedAgePrivKey, metadataBytes) + }) if err != nil { - return nil, fmt.Errorf("failed to write unlocker metadata: %w", err) + return nil, err } - return &pgpMetadata, nil + return &PGPUnlocker{ + Directory: unlockerDir, + Metadata: pgpMetadata.UnlockerMetadata, + fs: fs, + }, nil +} + +// encryptPGPUnlockerKeys returns the vault's long-term private key encrypted +// to the new PGP unlocker's age keypair, and that keypair's private key +// encrypted to the GPG key gpgKeyID. +func encryptPGPUnlockerKeys( + fs afero.Fs, vault VaultInterface, + ageIdentity *age.X25519Identity, gpgKeyID string, +) ([]byte, []byte, error) { + // Get or derive the long-term private key + ltPrivKeyData, err := getLongTermPrivateKey(fs, vault) + if err != nil { + return nil, nil, err + } + defer ltPrivKeyData.Destroy() + + encryptedLtPrivKey, err := EncryptToRecipient( + ltPrivKeyData, ageIdentity.Recipient()) + if err != nil { + return nil, nil, fmt.Errorf( + "failed to encrypt long-term private key to age unlocker: %w", err) + } + + // Use memguard to protect the private key in memory + agePrivateKeyBuffer := memguard.NewBufferFromBytes([]byte(ageIdentity.String())) + defer agePrivateKeyBuffer.Destroy() + + encryptedAgePrivKey, err := GPGEncryptFunc(agePrivateKeyBuffer, gpgKeyID) + if err != nil { + return nil, nil, fmt.Errorf( + "failed to encrypt age private key with GPG: %w", err) + } + + return encryptedLtPrivKey, encryptedAgePrivKey, nil +} + +// writePGPUnlockerFiles writes the files of a PGP unlocker into dir, the +// metadata last. +func writePGPUnlockerFiles( + fs afero.Fs, dir string, ageRecipient *age.X25519Recipient, + encryptedLtPrivKey, encryptedAgePrivKey, metadataBytes []byte, +) error { + err := WriteFileAtomic(fs, filepath.Join(dir, "pub.txt"), + []byte(ageRecipient.String())) + if err != nil { + return fmt.Errorf("failed to write age recipient: %w", err) + } + + err = WriteFileAtomic(fs, filepath.Join(dir, "longterm.age"), encryptedLtPrivKey) + if err != nil { + return fmt.Errorf("failed to write encrypted long-term private key: %w", err) + } + + err = WriteFileAtomic(fs, filepath.Join(dir, "priv.age.gpg"), encryptedAgePrivKey) + if err != nil { + return fmt.Errorf("failed to write encrypted age private key: %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) + } + + return nil } // validateGPGKeyID validates that a GPG key ID is safe for command execution diff --git a/internal/secret/pgpunlocker_test.go b/internal/secret/pgpunlocker_test.go new file mode 100644 index 0000000..7d95a32 --- /dev/null +++ b/internal/secret/pgpunlocker_test.go @@ -0,0 +1,67 @@ +package secret_test + +import ( + "os" + "path/filepath" + "testing" + + "git.eeqj.de/sneak/secret/internal/secret" + "git.eeqj.de/sneak/secret/internal/vault" + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The GPG key ID and fingerprint passed to CreatePGPUnlocker. +const ( + testGPGKeyID = "0123456789ABCDEF" + testGPGFingerprint = "0123456789ABCDEF0123456789ABCDEF01234567" +) + +// fakeGPGScript is a gpg for which `gpg --version` succeeds and anything +// else fails. +const fakeGPGScript = `#!/bin/sh +[ "$*" = --version ] +` + +// installFakeGPG makes fakeGPGScript the only gpg on PATH for the test. +func installFakeGPG(t *testing.T) { + t.Helper() + + dir := t.TempDir() + + //nolint:gosec // G306: the script must be executable + err := os.WriteFile(filepath.Join(dir, "gpg"), []byte(fakeGPGScript), 0o700) + require.NoError(t, err) + + t.Setenv("PATH", dir) +} + +// TestCreatePGPUnlockerFailureWritesNothing makes CreatePGPUnlocker fail at +// getting the vault's long-term key, which used to come after part of the +// unlocker was written, and asserts that nothing is written. Getting the key +// fails because on macOS there is no mnemonic and no current unlocker, and +// on every other platform it always fails +// (https://git.eeqj.de/sneak/secret/issues/88). +func TestCreatePGPUnlockerFailureWritesNothing(t *testing.T) { + installFakeGPG(t) + t.Setenv(secret.EnvMnemonic, "") + + base := afero.NewMemMapFs() + vlt, err := vault.CreateVault(base, testVaultStateDir, testVaultName) + require.NoError(t, err) + + fs := hookFs{Fs: base, before: func(_, path string) error { + t.Errorf("changed %s", path) + + return nil + }} + + _, err = secret.CreatePGPUnlocker( + fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint) + require.Error(t, err) + + vaultDir, err := vlt.GetDirectory() + require.NoError(t, err) + assert.Empty(t, dirNames(t, base, filepath.Join(vaultDir, "unlockers.d"))) +} diff --git a/internal/secret/seunlocker_darwin.go b/internal/secret/seunlocker_darwin.go index 69e8ea8..6e1824e 100644 --- a/internal/secret/seunlocker_darwin.go +++ b/internal/secret/seunlocker_darwin.go @@ -254,7 +254,7 @@ func CreateSecureEnclaveUnlocker( ) } - // Step 4: Create unlocker directory and write files + // Step 4: Prepare the unlocker directory's path and metadata vaultDir, err := vault.GetDirectory() if err != nil { return nil, fmt.Errorf("failed to get vault directory: %w", err) @@ -262,23 +262,7 @@ func CreateSecureEnclaveUnlocker( unlockerDirName := fmt.Sprintf("se-%s", filepath.Base(seKeyLabel)) unlockerDir := filepath.Join(vaultDir, "unlockers.d", unlockerDirName) - if err := fs.MkdirAll(unlockerDir, DirPerms); err != nil { - return nil, fmt.Errorf( - "failed to create unlocker directory: %w", - err, - ) - } - // Write SE-encrypted long-term key - ltKeyPath := filepath.Join(unlockerDir, seLongtermFilename) - if err := WriteFileAtomic(fs, ltKeyPath, encryptedLtKey); err != nil { - return nil, fmt.Errorf( - "failed to write SE-encrypted long-term key: %w", - err, - ) - } - - // Write metadata seMetadata := SecureEnclaveUnlockerMetadata{ UnlockerMetadata: UnlockerMetadata{ Type: seUnlockerType, @@ -294,9 +278,25 @@ func CreateSecureEnclaveUnlocker( return nil, fmt.Errorf("failed to marshal metadata: %w", err) } - metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json") - if err := WriteFileAtomic(fs, metadataPath, metadataBytes); err != nil { - return nil, fmt.Errorf("failed to write metadata: %w", err) + // Step 5: Write the SE-encrypted long-term key, then the metadata + err = WriteDir(fs, unlockerDir, func(dir string) error { + ltKeyPath := filepath.Join(dir, seLongtermFilename) + if err := WriteFileAtomic(fs, ltKeyPath, encryptedLtKey); err != nil { + return fmt.Errorf( + "failed to write SE-encrypted long-term key: %w", + err, + ) + } + + metadataPath := filepath.Join(dir, "unlocker-metadata.json") + if err := WriteFileAtomic(fs, metadataPath, metadataBytes); err != nil { + return fmt.Errorf("failed to write metadata: %w", err) + } + + return nil + }) + if err != nil { + return nil, err } return &SecureEnclaveUnlocker{ diff --git a/internal/vault/unlockers.go b/internal/vault/unlockers.go index 105c046..af5ecdb 100644 --- a/internal/vault/unlockers.go +++ b/internal/vault/unlockers.go @@ -357,26 +357,14 @@ func (v *Vault) CreatePassphraseUnlocker( return nil, fmt.Errorf("failed to get long-term key: %w", err) } - // Create unlocker directory unlockerDir := filepath.Join(vaultDir, "unlockers.d", unlockerTypePassphrase) - err = v.fs.MkdirAll(unlockerDir, secret.DirPerms) - if err != nil { - return nil, fmt.Errorf("failed to create unlocker directory: %w", err) - } - // Generate new age keypair for unlocker unlockerIdentity, err := age.GenerateX25519Identity() if err != nil { return nil, fmt.Errorf("failed to generate unlocker: %w", err) } - // Write the unlocker keypair (public and passphrase-encrypted private) - err = v.writeUnlockerKeypair(unlockerDir, unlockerIdentity, passphrase) - if err != nil { - return nil, err - } - // Encrypt long-term private key to this unlocker ltPrivKeyBuffer := memguard.NewBufferFromBytes([]byte(ltIdentity.String())) defer ltPrivKeyBuffer.Destroy() @@ -387,15 +375,6 @@ func (v *Vault) CreatePassphraseUnlocker( return nil, fmt.Errorf("failed to encrypt long-term private key: %w", err) } - ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age") - - err = secret.WriteFileAtomic(v.fs, ltPrivKeyPath, encryptedLtPrivKey) - if err != nil { - return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err) - } - - // Write the metadata last: readers skip an unlocker directory without - // it, so an unlocker interrupted before this point is never used. metadata := UnlockerMetadata{ Type: unlockerTypePassphrase, CreatedAt: time.Now(), @@ -407,11 +386,13 @@ func (v *Vault) CreatePassphraseUnlocker( return nil, fmt.Errorf("failed to marshal metadata: %w", err) } - metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json") - - err = secret.WriteFileAtomic(v.fs, metadataPath, metadataBytes) + // Write the unlocker's files, the metadata last + err = secret.WriteDir(v.fs, unlockerDir, func(dir string) error { + return v.writeUnlockerFiles(dir, unlockerIdentity, passphrase, + encryptedLtPrivKey, metadataBytes) + }) if err != nil { - return nil, fmt.Errorf("failed to write unlocker metadata: %w", err) + return nil, err } // Create the unlocker instance @@ -457,12 +438,14 @@ func (v *Vault) readUnlockerMetadata(unlockerDir string) (UnlockerMetadata, erro return metadata, nil } -// writeUnlockerKeypair writes the unlocker's public key and its -// passphrase-encrypted private key into the unlocker directory. -func (v *Vault) writeUnlockerKeypair( +// writeUnlockerFiles writes the files of a passphrase unlocker into +// unlockerDir: its public key, its passphrase-encrypted private key, the +// long-term private key encrypted to it, and its metadata, last. +func (v *Vault) writeUnlockerFiles( unlockerDir string, unlockerIdentity *age.X25519Identity, passphrase *memguard.LockedBuffer, + encryptedLtPrivKey, metadataBytes []byte, ) error { // Write public key pubKeyPath := filepath.Join(unlockerDir, "pub.age") @@ -492,5 +475,17 @@ func (v *Vault) writeUnlockerKeypair( return fmt.Errorf("failed to write encrypted unlocker private key: %w", err) } + err = secret.WriteFileAtomic(v.fs, + filepath.Join(unlockerDir, "longterm.age"), encryptedLtPrivKey) + if err != nil { + return fmt.Errorf("failed to write encrypted long-term private key: %w", err) + } + + err = secret.WriteFileAtomic(v.fs, + filepath.Join(unlockerDir, "unlocker-metadata.json"), metadataBytes) + if err != nil { + return fmt.Errorf("failed to write unlocker metadata: %w", err) + } + return nil }