From 44fd790bba2eb48d120433893e85a3d0077b840c Mon Sep 17 00:00:00 2001 From: sarin Date: Sun, 2 Aug 2026 19:33:08 +0800 Subject: [PATCH 1/2] Stop edit from losing the secret you just typed edit checked nothing about the destination until after the editor had closed, and removed the plaintext on the way out, so writing into a directory that does not exist threw away what had just been typed. Fresh repositories have no secretsDir, which makes the very first secret the one most likely to be lost. Create the directory before the editor runs, and replace the file by rename so a write that fails partway cannot take the previous ciphertext with it. --- cmd/edit.go | 53 +++++++++++++++++++++++- cmd/edit_test.go | 106 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 2 deletions(-) create mode 100644 cmd/edit_test.go diff --git a/cmd/edit.go b/cmd/edit.go index f70a448..7655359 100644 --- a/cmd/edit.go +++ b/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) diff --git a/cmd/edit_test.go b/cmd/edit_test.go new file mode 100644 index 0000000..28199c3 --- /dev/null +++ b/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) + } +} From 5dac367c658c14b41fdba131a6d1a22cf0cd7bf4 Mon Sep 17 00:00:00 2001 From: sarin Date: Sun, 2 Aug 2026 19:33:08 +0800 Subject: [PATCH 2/2] Accept a recipient with the newline a file ends with `kix.hostPubkey = ./secrets/host.pub` hands the file's contents to the parser as they are. agessh tolerates the trailing newline and age's own parser does not, so an age host key failed where an SSH one worked, and only in seal, after the identity had already been unlocked. --- cmd/recipients_test.go | 14 ++++++++++++++ cmd/root.go | 6 ++++++ 2 files changed, 20 insertions(+) diff --git a/cmd/recipients_test.go b/cmd/recipients_test.go index 4e9ad35..13acabb 100644 --- a/cmd/recipients_test.go +++ b/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) { diff --git a/cmd/root.go b/cmd/root.go index ab683dc..644125a 100644 --- a/cmd/root.go +++ b/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) }