Fix edit losing the typed secret, and recipients with a trailing newline

cmd/edit.go

@@ -7,6 +7,7 @@ import (
 	"log/slog"
 	"os"
 	"os/exec"
+	"path/filepath"
 
 	"filippo.io/age"
 	"github.com/spf13/cobra"
@@ -75,6 +76,14 @@ func runEdit(file, manifestPath, identityPath string, recipients []string) error
 		recips = append(recips, recip)
 	}
 
+	// Before the editor rather than after. Everything typed into it is lost if
+	// the write at the end fails, and a missing parent directory is how that
+	// happens: secretsDir does not exist yet in a fresh repository, so the
+	// very first secret would be typed out and thrown away.
+	if err := os.MkdirAll(filepath.Dir(file), 0o755); err != nil {
+		return fmt.Errorf("creating the directory for %q: %w", file, err)
+	}
+
 	tmp, err := createPlaintextFile(plaintextDir())
 	if err != nil {
 		return fmt.Errorf("creating temp file: %w", err)
@@ -133,8 +142,8 @@ func runEdit(file, manifestPath, identityPath string, recipients []string) error
 	if err != nil {
 		return fmt.Errorf("encrypting: %w", err)
 	}
-	if err := os.WriteFile(file, encrypted, 0o644); err != nil {
-		return fmt.Errorf("writing encrypted file: %w", err)
+	if err := writeFileAtomic(file, encrypted, 0o644); err != nil {
+		return err
 	}
 
 	if existing {
@@ -145,6 +154,46 @@ func runEdit(file, manifestPath, identityPath string, recipients []string) error
 	return nil
 }
 
+// writeFileAtomic replaces path in a single step.
+//
+// By the time this runs the plaintext exists nowhere else: the editor's copy
+// is removed as this function returns, so the file being replaced holds the
+// only other copy of the secret. A partial write over it would take both.
+func writeFileAtomic(path string, data []byte, perm os.FileMode) error {
+	// Alongside the target, so the rename stays within one filesystem.
+	tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".*")
+	if err != nil {
+		return fmt.Errorf("creating a temporary file next to %q: %w", path, err)
+	}
+	defer os.Remove(tmp.Name())
+
+	if err := func() error {
+		if _, err := tmp.Write(data); err != nil {
+			return fmt.Errorf("writing %q: %w", tmp.Name(), err)
+		}
+		if err := tmp.Chmod(perm); err != nil {
+			return fmt.Errorf("chmod %q: %w", tmp.Name(), err)
+		}
+		// The rename is only atomic with respect to the file's contents once
+		// those contents have reached the filesystem.
+		if err := tmp.Sync(); err != nil {
+			return fmt.Errorf("syncing %q: %w", tmp.Name(), err)
+		}
+		return nil
+	}(); err != nil {
+		tmp.Close()
+		return err
+	}
+	if err := tmp.Close(); err != nil {
+		return fmt.Errorf("closing %q: %w", tmp.Name(), err)
+	}
+
+	if err := os.Rename(tmp.Name(), path); err != nil {
+		return fmt.Errorf("replacing %q: %w", path, err)
+	}
+	return nil
+}
+
 func blake2bHex(data []byte) string {
 	h, _ := blake2b.New256(nil)
 	h.Write(data)

cmd/edit_test.go

@@ -0,0 +1,106 @@
+package cmd
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+// scriptEditor stands in for $EDITOR, writing body into whatever file it is
+// handed. The body goes through a file of its own so nothing has to be quoted
+// into the script.
+func scriptEditor(t *testing.T, body string) string {
+	t.Helper()
+	dir := t.TempDir()
+
+	content := filepath.Join(dir, "content")
+	if err := os.WriteFile(content, []byte(body), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	editor := filepath.Join(dir, "editor")
+	script := "#!/bin/sh\ncat " + content + " > \"$1\"\n"
+	if err := os.WriteFile(editor, []byte(script), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	return editor
+}
+
+// secretsDir does not exist yet in a fresh repository, and edit used to notice
+// only after the editor had closed -- by which point the plaintext existed
+// nowhere but the temporary file it was about to remove.
+func TestEditCreatesASecretUnderADirectoryThatDoesNotExist(t *testing.T) {
+	dir := t.TempDir()
+	id := x25519(t)
+	t.Setenv("EDITOR", scriptEditor(t, "the-secret\n"))
+
+	target := filepath.Join(dir, "secrets", "new.age")
+	if err := runEdit(target, "", writeIdentityFile(t, dir, id), nil); err != nil {
+		t.Fatalf("editing into a directory that does not exist: %v", err)
+	}
+
+	if got := decryptFile(t, target, id); got != "the-secret\n" {
+		t.Errorf("secret is %q, want %q", got, "the-secret\n")
+	}
+}
+
+func TestWriteFileAtomicLeavesNothingBehind(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "secret.age")
+
+	if err := writeFileAtomic(path, []byte("ciphertext"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(entries) != 1 || entries[0].Name() != "secret.age" {
+		names := make([]string, 0, len(entries))
+		for _, e := range entries {
+			names = append(names, e.Name())
+		}
+		t.Errorf("directory holds %v, want just secret.age", names)
+	}
+
+	fi, err := os.Stat(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := fi.Mode().Perm(); got != 0o644 {
+		t.Errorf("mode is %v, want 0644", got)
+	}
+}
+
+// The file being replaced holds the only other copy of the secret, so a write
+// that cannot finish has to leave it alone rather than truncate it first.
+func TestWriteFileAtomicKeepsTheTargetWhenItCannotFinish(t *testing.T) {
+	dir := t.TempDir()
+
+	// A directory in the target's place: os.Rename refuses it, standing in for
+	// the writes that fail for reasons a test cannot arrange.
+	path := filepath.Join(dir, "secret.age")
+	if err := os.Mkdir(path, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	survivor := filepath.Join(path, "survivor")
+	if err := os.WriteFile(survivor, []byte("x"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := writeFileAtomic(path, []byte("ciphertext"), 0o644); err == nil {
+		t.Fatal("a write that cannot complete reported success")
+	}
+
+	if _, err := os.Stat(survivor); err != nil {
+		t.Errorf("the target was destroyed by a write that failed: %v", err)
+	}
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(entries) != 1 {
+		t.Errorf("a temporary file was left behind: %d entries in %s", len(entries), dir)
+	}
+}

cmd/recipients_test.go

@@ -310,6 +310,20 @@ func TestRefreshRecipientsRefusesUnwritableSourceWithoutTouchingAnything(t *test
 	}
 }
 
+// `kix.hostPubkey = ./secrets/host.pub` hands the file's contents straight to
+// the parser, and a file ends in a newline. agessh accepts it; age's own
+// parser used to reject it, so an age host key failed where an SSH one worked.
+func TestParseRecipientToleratesTheNewlineAFileEndsWith(t *testing.T) {
+	for _, key := range []string{
+		"age1kqn3nznrh0hmmkrvszcxzc2k8mlc94efqnm803axk4nt8p4wjv9szlzu2x",
+		"ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIN8x0GNwFpNmVDLBHVJ5tQnFAF7mV8vNBOZ0aQKmm4mm root@host",
+	} {
+		if _, err := parseRecipient(key+"\n", nil); err != nil {
+			t.Errorf("newline-terminated recipient rejected: %v", err)
+		}
+	}
+}
+
 // A PQ identity is one parseIdentity accepts, so it must not fall through to
 // the extra recipients alone.
 func TestIdentityRecipientHybrid(t *testing.T) {

cmd/root.go

@@ -84,6 +84,12 @@ func terminalUI() *plugin.ClientUI {
 }
 
 func parseRecipient(s string, ui *plugin.ClientUI) (age.Recipient, error) {
+	// A recipient often reaches us as the contents of a file, by way of
+	// `kix.hostPubkey = ./secrets/host.pub`, and a file ends in a newline.
+	// agessh accepts the trailing whitespace and age's own parser does not,
+	// so without this an age host key works everywhere an SSH one does not.
+	s = strings.TrimSpace(s)
+
 	if strings.HasPrefix(s, "ssh-") {
 		return agessh.ParseRecipient(s)
 	}