Write encrypt and decrypt output only once they succeed (closes #143)

`secret encrypt` and `secret decrypt` emptied the file named by
`--output` before reading any input, so a failure left it empty, and a
file given as both `--input` and `--output` was emptied before it was
read. They now write into a temporary file of mode 0600 next to it, or
next to the path it is a symbolic link to, and rename that over it only
on success, using the new `secret.WriteFileAtomicFunc`, which
`secret.WriteFileAtomic` now calls. Ctrl-C or SIGTERM removes the
temporary file. A device or pipe given as `--output`, such as
`/dev/null`, is written to directly.

Judgement call: the device and pipe case goes beyond the plan.

Model: opus-5-5

README.md

@@ -150,13 +150,6 @@ Retrieves and outputs a secret value to stdout.
 
 Lists all secrets in the current vault. Optional filter for substring matching.
 
-The `LAST UPDATED` column, and `updated_at` in the `--json` output, give the
-time the secret's `current` file was last written: every command that adds a
-version replaces it, and so does `secret version promote`. `created_at` gives
-the time the `value.age` of the secret's oldest version was written. Both are
-the files' modification times, so listing asks for no passphrase. Moving a
-secret to another vault keeps both times.
-
 #### `secret remove <secret-name> [--force]` / `secret rm` ⚠️ 🛑
 
 **DANGER**: Permanently removes a secret and ALL its versions. It first asks for
@@ -188,9 +181,7 @@ period.
 #### `secret version promote <secret-name> <version>`
 
 Promotes a specific version to current by rewriting the secret's `current` file
-to name it. The versions' own times are left unchanged, allowing for rollback
-scenarios; `secret list` then gives the time of the promote as the secret's last
-update.
+to name it. Does not modify any timestamps, allowing for rollback scenarios.
 
 #### `secret version remove <secret-name> <version> [--force]` / `secret version rm` ⚠️ 🛑
 
@@ -206,8 +197,7 @@ for confirmation, naming the version, the secret and its vault (see
 
 #### `secret generate mnemonic`
 
-Generates a cryptographically secure BIP39 mnemonic phrase and prints it to
-stdout. The warning to keep it safe goes to stderr.
+Generates a cryptographically secure BIP39 mnemonic phrase.
 
 #### `secret generate secret <name> [--length=16] [--type=base58] [--force]`
 
@@ -251,14 +241,6 @@ that the vault then opens only with its mnemonic (see
 [Confirmation Before Removal](#confirmation-before-removal)). An unlocker
 directory that `secret unlocker list` skips with a warning, because its metadata
 cannot be read or parsed, is removed by the directory name the warning gives.
-Removing the current unlocker makes the first of the vault's other unlockers
-that `secret unlocker select` accepts, in the order `secret unlocker list` shows
-them, the current one, and says which; when there is none, the vault is left
-without a current unlocker. `secret unlocker select` refuses an unlocker whose
-type is not `passphrase` or `pgp`. When `current-unlocker` names an unlocker
-that does not exist and the vault has another that `secret unlocker select`
-accepts, opening the vault fails with an error that says to select one with
-`secret unlocker select`.
 
 - `--force, -f`: Remove without asking, even the last unlocker
 - **CRITICAL WARNING**: Without unlockers and without your mnemonic phrase,
@@ -292,6 +274,14 @@ generates a new Age key.
 
 Decrypts data using an Age key stored as a secret.
 
+Both write the result to a new file next to `--output` and rename it over
+`--output` only once they have succeeded, so a failed or interrupted command
+leaves an existing `--output` as it was and creates none, and `--input` and
+`--output` can be the same file. The file has mode 0600, whatever mode the file
+it replaced had. When `--output` is a symbolic link, the file it points to is
+replaced, or created if it does not exist yet, and the link kept. A device or a
+pipe, such as `/dev/null`, is written to directly.
+
 ## Storage Architecture
 
 ### Directory Structure

TODO.md

@@ -18,38 +18,17 @@ https://git.eeqj.de/sneak/secret/milestone/12
 
 # Completed Steps
 
-- 2026-10-14: `secret list` gives each secret's own creation and last-update
-  times (https://git.eeqj.de/sneak/secret/issues/142). It gave the time of the
-  listing for both, which `Secret.LoadMetadata` set to the current time.
-  `LoadMetadata` now reads file modification times, so listing still needs no
-  passphrase: the last update is when the secret's `current` file was written,
-  and the creation is when its oldest version's `value.age` was written. That
-  version's directory would be wrong, because the next add rewrites the
-  version's `metadata.age` in it. `GetSecretObject`, which `secret get` also
-  uses, no longer loads the times; `secret list` loads them itself. A move to
-  another vault gives each copied `value.age` and `current` the modification
-  time of the file it was copied from, so the moved secret keeps its times.
-- 2026-10-14: `secret generate mnemonic`, `secret vault list` and
-  `secret unlocker list` print their output to stdout, as `secret list` does, so
-  it can be piped or captured (https://git.eeqj.de/sneak/secret/issues/138).
-  They printed with cobra's `cmd.Println` and `cmd.Printf`, which write to
-  stderr when no output writer is set, so `secret vault list --json | jq`
-  received nothing. "No versions found" from `secret version list` moved to
-  stdout too. Messages such as "Created vault", and the warning after the
-  mnemonic, stay on stderr.
-- 2026-10-14: Removing the current unlocker no longer leaves the vault unable to
-  open through the unlockers it still has
-  (https://git.eeqj.de/sneak/secret/issues/137). `current-unlocker` went on
-  naming the removed unlocker, so every command that opened the vault failed,
-  with advice to use the mnemonic. `secret unlocker remove` of the current
-  unlocker now makes the first of the vault's other unlockers that
-  `secret unlocker select` accepts, in the order `secret unlocker list` shows,
-  current before removing it, and says so; when there is none, it also removes
-  `current-unlocker`. `vault.RemoveUnlocker` returns the ID of the unlocker it
-  made current. When `current-unlocker` names an unlocker that does not exist
-  and the vault has another that `secret unlocker select` accepts, the error
-  wraps `vault.ErrUnlockerNotFound` and says to run `secret unlocker select`,
-  without the advice to use the mnemonic.
+- 2026-10-14: A failed `secret encrypt` or `secret decrypt` leaves the file
+  named by `--output` as it was, and the same file can be given as `--input` and
+  `--output` (https://git.eeqj.de/sneak/secret/issues/143). Both emptied the
+  output file before reading any input, so a wrong key left it empty, and a file
+  given as both was emptied before it was read. They now write the result into a
+  temporary file next to it, or next to the file it is a symbolic link to, and
+  rename that over it once they have succeeded, with the new
+  `secret.WriteFileAtomicFunc`, which `secret.WriteFileAtomic` now calls; the
+  output file has mode 0600. Ctrl-C or SIGTERM removes the temporary file. A
+  device or a pipe given as `--output`, such as `/dev/null`, is still written to
+  directly.
 - 2026-10-14: A `secret add --force` that cannot unlock the vault, given a wrong
   passphrase or mnemonic, leaves the secret as it was
   (https://git.eeqj.de/sneak/secret/issues/136). The new version used to be

internal/cli/crypto.go

@@ -5,9 +5,12 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"path/filepath"
+	"sync/atomic"
 
 	"filippo.io/age"
 	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
 	"sneak.berlin/go/secret/internal/secret"
 	"sneak.berlin/go/secret/internal/vault"
@@ -188,36 +191,24 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// Set up output writer
-	output := cli.cmd.OutOrStdout()
-
-	if outputFile != "" {
-		file, err := cli.fs.Create(outputFile)
+	return cli.writeOutput(outputFile, func(output io.Writer) error {
+		encryptor, err := age.Encrypt(output, recipient)
 		if err != nil {
-			return fmt.Errorf("failed to create output file: %w", err)
+			return fmt.Errorf("failed to create age encryptor: %w", err)
 		}
-		defer func() { _ = file.Close() }()
 
-		output = file
-	}
+		_, err = io.Copy(encryptor, input)
+		if err != nil {
+			return fmt.Errorf("failed to encrypt data: %w", err)
+		}
 
-	// Encrypt the data
-	encryptor, err := age.Encrypt(output, recipient)
-	if err != nil {
-		return fmt.Errorf("failed to create age encryptor: %w", err)
-	}
+		err = encryptor.Close()
+		if err != nil {
+			return fmt.Errorf("failed to finalize encryption: %w", err)
+		}
 
-	_, err = io.Copy(encryptor, input)
-	if err != nil {
-		return fmt.Errorf("failed to encrypt data: %w", err)
-	}
-
-	err = encryptor.Close()
-	if err != nil {
-		return fmt.Errorf("failed to finalize encryption: %w", err)
-	}
-
-	return nil
+		return nil
+	})
 }
 
 // Decrypt decrypts data using an age secret key stored in a secret
@@ -278,31 +269,99 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// Set up output writer
-	output := cli.cmd.OutOrStdout()
-
-	if outputFile != "" {
-		file, err := cli.fs.Create(outputFile)
+	return cli.writeOutput(outputFile, func(output io.Writer) error {
+		decryptor, err := age.Decrypt(input, identity)
 		if err != nil {
-			return fmt.Errorf("failed to create output file: %w", err)
+			return fmt.Errorf("failed to create age decryptor: %w", err)
+		}
+
+		_, err = io.Copy(output, decryptor)
+		if err != nil {
+			return fmt.Errorf("failed to decrypt data: %w", err)
+		}
+
+		return nil
+	})
+}
+
+// temporaryOutput is the name of the temporary file writeOutput is filling,
+// until it is renamed into place. The signal handler in Entry removes it,
+// since for decrypt it holds part of the secret.
+//
+//nolint:gochecknoglobals // read by the signal handler, which has no Instance
+var temporaryOutput atomic.Pointer[string]
+
+// writeOutput calls write with stdout when outputFile is empty, and with
+// outputFile itself when it is a device or a pipe, such as /dev/null.
+// Otherwise write fills a new file of mode 0600 that replaces outputFile,
+// or the file it is a symbolic link to, only once write has succeeded, so a
+// failure leaves that file as it was, and it may be the input file too.
+func (cli *Instance) writeOutput(
+	outputFile string, write func(output io.Writer) error,
+) error {
+	if outputFile == "" {
+		return write(cli.cmd.OutOrStdout())
+	}
+
+	info, err := cli.fs.Stat(outputFile)
+	if err == nil && !info.Mode().IsRegular() {
+		file, err := cli.fs.OpenFile(outputFile, os.O_WRONLY, 0)
+		if err != nil {
+			return fmt.Errorf("failed to open output file: %w", err)
 		}
 		defer func() { _ = file.Close() }()
 
-		output = file
+		return write(file)
 	}
 
-	// Decrypt the data
-	decryptor, err := age.Decrypt(input, identity)
+	path, err := outputPath(outputFile)
 	if err != nil {
-		return fmt.Errorf("failed to create age decryptor: %w", err)
+		return fmt.Errorf("failed to open output file: %w", err)
+	}
+
+	err = secret.WriteFileAtomicFunc(cli.fs, path, func(file afero.File) error {
+		name := file.Name()
+		temporaryOutput.Store(&name)
+
+		return write(file)
+	})
+
+	temporaryOutput.Store(nil)
+
+	return err
+}
+
+// outputPath returns the path of the file that writing to outputFile
+// replaces. When outputFile is a symbolic link, as /dev/stdout is when
+// stdout goes to a file, that is the path the link points to, whether or
+// not a file is there yet, so the link is kept, as writing through it would
+// keep it. A link that cannot be followed, such as one that points to
+// itself, is an error.
+func outputPath(outputFile string) (string, error) {
+	path, err := filepath.EvalSymlinks(outputFile)
+	if !errors.Is(err, os.ErrNotExist) {
+		return path, err
+	}
+
+	// No file is at outputFile, or at the end of its links, yet
+	target, err := os.Readlink(outputFile)
+	if errors.Is(err, os.ErrNotExist) {
+		return outputFile, nil
 	}
 
-	_, err = io.Copy(output, decryptor)
 	if err != nil {
-		return fmt.Errorf("failed to decrypt data: %w", err)
+		return "", err
 	}
 
-	return nil
+	if !filepath.IsAbs(target) {
+		// Not filepath.Join, which would remove a ".." in target together
+		// with the name before it, even when that name is a link to a
+		// directory
+		dir, _ := filepath.Split(outputFile)
+		target = dir + target
+	}
+
+	return outputPath(target)
 }
 
 // isValidAgeSecretKey checks if a string is a valid age secret key by

internal/cli/crypto_test.go

@@ -0,0 +1,241 @@
+//nolint:testpackage // sets the unexported fields of Instance
+package cli
+
+import (
+	"io"
+	"os"
+	"path/filepath"
+	"strings"
+	"syscall"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/vault"
+)
+
+// plainText is the content of plain.txt in the directory newCryptoTest makes.
+const plainText = "document"
+
+// newCryptoTest returns an Instance whose current vault, in a new state
+// directory on the real filesystem, holds the age key secret "key", and a
+// new directory holding only plain.txt and plain.txt.age, its encryption
+// with that key.
+func newCryptoTest(t *testing.T) (*Instance, string) {
+	t.Helper()
+
+	fs := afero.NewOsFs()
+	stateDir := t.TempDir()
+
+	_, err := vault.CreateVault(fs, stateDir, "default", testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	cli := NewCLIInstanceWithStateDir(fs, stateDir)
+	cli.Mnemonic = testMnemonicBuffer(t)
+	cli.cmd = &cobra.Command{}
+
+	dir := t.TempDir()
+	plain := filepath.Join(dir, "plain.txt")
+	require.NoError(t, afero.WriteFile(fs, plain, []byte(plainText), 0o600))
+	require.NoError(t, cli.Encrypt("key", plain, plain+".age"))
+
+	return cli, dir
+}
+
+// TestFailedCryptoLeavesOutputUnchanged is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/143, where secret encrypt and
+// secret decrypt emptied the file named by --output before they failed.
+// A failed command must leave an existing file as it was and create none,
+// and give the same error as without --output.
+func TestFailedCryptoLeavesOutputUnchanged(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		run  func(cli *Instance, dir, output string) error
+	}{
+		// Reading a directory fails once encryption has begun
+		{"encrypt", func(cli *Instance, dir, output string) error {
+			return cli.Encrypt("key", dir, output)
+		}},
+		// plain.txt is not an age file
+		{"decrypt", func(cli *Instance, dir, output string) error {
+			return cli.Decrypt("key", filepath.Join(dir, "plain.txt"), output)
+		}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			cli, dir := newCryptoTest(t)
+			existing := filepath.Join(dir, "existing.txt")
+			require.NoError(t, afero.WriteFile(cli.fs, existing, []byte("precious"), 0o600))
+
+			cli.cmd.SetOut(io.Discard)
+			toStdout := tt.run(cli, dir, "")
+			require.Error(t, toStdout)
+
+			require.EqualError(t, tt.run(cli, dir, existing), toStdout.Error())
+			require.EqualError(t, tt.run(cli, dir, filepath.Join(dir, "new.txt")),
+				toStdout.Error())
+
+			content, err := afero.ReadFile(cli.fs, existing)
+			require.NoError(t, err)
+			require.Equal(t, "precious", string(content))
+
+			// No new.txt, and no temporary file left behind
+			entries, err := afero.ReadDir(cli.fs, dir)
+			require.NoError(t, err)
+
+			names := make([]string, 0, len(entries))
+			for _, entry := range entries {
+				names = append(names, entry.Name())
+			}
+
+			require.Equal(t, []string{"existing.txt", "plain.txt", "plain.txt.age"}, names)
+		})
+	}
+}
+
+// TestCryptoInPlace checks that secret encrypt and secret decrypt given
+// one file as both --input and --output replace it with the right result.
+// Before https://git.eeqj.de/sneak/secret/issues/143 they emptied the file
+// before reading it.
+func TestCryptoInPlace(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+	file := filepath.Join(dir, "plain.txt")
+
+	require.NoError(t, cli.Encrypt("key", file, file))
+	require.NoError(t, cli.Decrypt("key", file, file))
+
+	content, err := afero.ReadFile(cli.fs, file)
+	require.NoError(t, err)
+	require.Equal(t, plainText, string(content))
+}
+
+// TestDecryptToPipe checks that secret decrypt writes into a named pipe
+// given as --output, as it must into a device such as /dev/null, instead
+// of replacing it with a file.
+func TestDecryptToPipe(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+	pipe := filepath.Join(dir, "pipe")
+	require.NoError(t, syscall.Mkfifo(pipe, 0o600))
+
+	received := make(chan []byte, 1)
+
+	go func() {
+		content, _ := afero.ReadFile(cli.fs, pipe)
+		received <- content
+	}()
+
+	require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), pipe))
+
+	info, err := os.Lstat(pipe)
+	require.NoError(t, err)
+	require.Equal(t, os.ModeNamedPipe, info.Mode().Type())
+	require.Equal(t, plainText, string(<-received))
+}
+
+// TestDecryptThroughLink checks that secret decrypt given a symbolic link
+// as --output replaces the file the link points to, in another directory,
+// and keeps the link, as writing through the link would.
+func TestDecryptThroughLink(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+	targetDir := t.TempDir()
+	target := filepath.Join(targetDir, "target.txt")
+	require.NoError(t, afero.WriteFile(cli.fs, target, []byte("old"), 0o600))
+
+	link := filepath.Join(dir, "link")
+	require.NoError(t, os.Symlink(target, link))
+
+	require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), link))
+
+	info, err := os.Lstat(link)
+	require.NoError(t, err)
+	require.Equal(t, os.ModeSymlink, info.Mode().Type())
+
+	content, err := afero.ReadFile(cli.fs, target)
+	require.NoError(t, err)
+	require.Equal(t, plainText, string(content))
+
+	// No temporary file left beside the target
+	entries, err := afero.ReadDir(cli.fs, targetDir)
+	require.NoError(t, err)
+	require.Len(t, entries, 1)
+}
+
+// TestDecryptThroughLinkToMissingFile checks that secret decrypt given a
+// symbolic link to a file that does not exist yet as --output creates that
+// file and keeps the link, as writing through the link would. Here --output
+// is a relative link to a second link, which points into another directory.
+func TestDecryptThroughLinkToMissingFile(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+	targetDir := t.TempDir()
+	target := filepath.Join(targetDir, "target.txt")
+
+	link := filepath.Join(dir, "link")
+	require.NoError(t, os.Symlink("second-link", link))
+	require.NoError(t, os.Symlink(target, filepath.Join(dir, "second-link")))
+
+	require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), link))
+
+	for _, name := range []string{"link", "second-link"} {
+		info, err := os.Lstat(filepath.Join(dir, name))
+		require.NoError(t, err)
+		require.Equal(t, os.ModeSymlink, info.Mode().Type(), name)
+	}
+
+	content, err := afero.ReadFile(cli.fs, target)
+	require.NoError(t, err)
+	require.Equal(t, plainText, string(content))
+
+	// No temporary file left beside the target
+	entries, err := afero.ReadDir(cli.fs, targetDir)
+	require.NoError(t, err)
+	require.Len(t, entries, 1)
+}
+
+// TestDecryptThroughLoopingLink checks that secret decrypt given a symbolic
+// link that points to itself as --output fails, as writing through it
+// would, and leaves the link as it was.
+func TestDecryptThroughLoopingLink(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+	link := filepath.Join(dir, "link")
+	require.NoError(t, os.Symlink("link", link))
+
+	require.Error(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), link))
+
+	target, err := os.Readlink(link)
+	require.NoError(t, err)
+	require.Equal(t, "link", target)
+}
+
+// TestDecryptToLongestName checks that secret decrypt writes an --output
+// whose name is as long as a file name can be, 255 bytes on Linux and
+// macOS: the temporary file's name must not be longer.
+func TestDecryptToLongestName(t *testing.T) {
+	t.Parallel()
+
+	const longestName = 255
+
+	cli, dir := newCryptoTest(t)
+	output := filepath.Join(dir, strings.Repeat("a", longestName))
+
+	require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), output))
+
+	content, err := afero.ReadFile(cli.fs, output)
+	require.NoError(t, err)
+	require.Equal(t, plainText, string(content))
+}

internal/cli/entry_test.go

@@ -8,6 +8,7 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/awnumar/memguard"
 	"github.com/stretchr/testify/assert"
@@ -105,3 +106,59 @@ func TestInterruptExitsThroughMemguard(t *testing.T) {
 	require.ErrorAs(t, err, &exitErr)
 	assert.Equal(t, 1, exitErr.ExitCode(), "add ended with %v", err)
 }
+
+// Ctrl-C while encrypt or decrypt waits for input must remove the temporary
+// file it is writing next to --output, which for decrypt holds part of the
+// secret.
+func TestInterruptRemovesTemporaryOutput(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
+	defer cancel()
+
+	secretPath := secretBinaryPath(t)
+	env := []string{
+		secret.EnvStateDir + "=" + t.TempDir(),
+		secret.EnvMnemonic + "=" + testMnemonic,
+		secret.EnvUnlockPassphrase + "=test-passphrase",
+		"PATH=/usr/bin:/bin",
+	}
+
+	//nolint:gosec // G204: test executes the freshly built secret binary
+	initCmd := exec.CommandContext(ctx, secretPath, "init")
+	initCmd.Env = env
+
+	output, err := initCmd.CombinedOutput()
+	require.NoError(t, err, "init should succeed: %s", output)
+
+	// encrypt stores the new key before it starts writing, so decrypt has it
+	for _, command := range []string{"encrypt", "decrypt"} {
+		outputDir := t.TempDir()
+
+		//nolint:gosec // G204: test executes the freshly built secret binary
+		cmd := exec.CommandContext(ctx, secretPath, command, "key",
+			"--output", filepath.Join(outputDir, "out"))
+		cmd.Env = env
+
+		// Held open and never written, so the command keeps waiting for
+		// input. Wait closes it.
+		_, err := cmd.StdinPipe()
+		require.NoError(t, err)
+		require.NoError(t, cmd.Start())
+
+		require.Eventually(t, func() bool {
+			entries, err := os.ReadDir(outputDir)
+
+			return err == nil && len(entries) > 0
+		}, commandWait, 10*time.Millisecond, "%s made no temporary file", command)
+		require.NoError(t, cmd.Process.Signal(os.Interrupt))
+
+		var exitErr *exec.ExitError
+
+		require.ErrorAs(t, cmd.Wait(), &exitErr)
+
+		entries, err := os.ReadDir(outputDir)
+		require.NoError(t, err)
+		assert.Empty(t, entries, "%s left its temporary file", command)
+	}
+}

internal/cli/generate.go

@@ -103,7 +103,7 @@ func (cli *Instance) GenerateMnemonic(cmd *cobra.Command) error {
 	}
 
 	// Output mnemonic to stdout
-	_, _ = fmt.Fprintln(cmd.OutOrStdout(), mnemonic)
+	cmd.Println(mnemonic)
 
 	// Output helpful information to stderr
 	fmt.Fprintln(os.Stderr, "")

internal/cli/list_times_test.go

@@ -1,161 +0,0 @@
-package cli_test
-
-import (
-	"bytes"
-	"encoding/json"
-	"testing"
-	"time"
-
-	"github.com/awnumar/memguard"
-	"github.com/spf13/afero"
-	"github.com/spf13/cobra"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/cli"
-	"sneak.berlin/go/secret/internal/secret"
-	"sneak.berlin/go/secret/internal/vault"
-)
-
-const (
-	// listClockStep is how long TestListShowsWhenSecretWasWritten waits
-	// between its steps, so that a time read in one step cannot be the
-	// time read in the next.
-	listClockStep = 200 * time.Millisecond
-
-	// fileTimeSlack allows for the clock the kernel stamps files with,
-	// which advances only every few milliseconds: a file's modification
-	// time can be slightly earlier than a time.Now() read before the file
-	// was written.
-	fileTimeSlack = 100 * time.Millisecond
-)
-
-// TestListShowsWhenSecretWasWritten is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/142, where `secret list` gave
-// the time of the listing as every secret's creation and last-update time.
-// Two listings with a clock step between must be the same, and give the
-// time of the add; after a forced add, the creation time is unchanged and
-// the last update is the time of that add. It runs on a real filesystem,
-// where writing a file into a directory also changes the directory's time.
-func TestListShowsWhenSecretWasWritten(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewOsFs()
-	stateDir := t.TempDir()
-
-	vlt, err := vault.CreateVault(fs, stateDir, "default", testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	vlt.Mnemonic = testMnemonicBuffer(t)
-	c := cli.NewCLIInstanceWithStateDir(fs, stateDir)
-
-	beforeAdd := time.Now()
-	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("one")), false)
-	require.NoError(t, err)
-
-	afterAdd := time.Now()
-
-	firstListing := listSecrets(t, c, true)
-
-	time.Sleep(listClockStep)
-	require.Equal(t, firstListing, listSecrets(t, c, true))
-
-	added := listedTimes(t, firstListing)
-	assert.WithinRange(t, added.CreatedAt, beforeAdd.Add(-fileTimeSlack), afterAdd)
-	assert.WithinRange(t, added.UpdatedAt, beforeAdd.Add(-fileTimeSlack), afterAdd)
-
-	time.Sleep(listClockStep)
-
-	beforeUpdate := time.Now()
-	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("two")), true)
-	require.NoError(t, err)
-
-	afterUpdate := time.Now()
-
-	updated := listedTimes(t, listSecrets(t, c, true))
-	assert.True(t, added.CreatedAt.Equal(updated.CreatedAt),
-		"created %s, then %s", added.CreatedAt, updated.CreatedAt)
-	assert.WithinRange(t, updated.UpdatedAt,
-		beforeUpdate.Add(-fileTimeSlack), afterUpdate)
-
-	// The table's LAST UPDATED column gives the same time, to the minute
-	assert.Contains(t, listSecrets(t, c, false),
-		updated.UpdatedAt.Format("2006-01-02 15:04"))
-}
-
-// TestMovedSecretKeepsItsTimes checks that a secret moved to another vault
-// is listed there with the times it was listed with before the move, and
-// not with the time of the move.
-func TestMovedSecretKeepsItsTimes(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewOsFs()
-	stateDir := t.TempDir()
-
-	_, err := vault.CreateVault(fs, stateDir, "other", testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	// "default" is created last, so it is the current vault
-	vlt, err := vault.CreateVault(fs, stateDir, "default", testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	vlt.Mnemonic = testMnemonicBuffer(t)
-
-	// Two versions written apart, so that the creation and the last update
-	// are different times
-	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("one")), false)
-	require.NoError(t, err)
-
-	time.Sleep(listClockStep)
-
-	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("two")), true)
-	require.NoError(t, err)
-
-	c := cli.NewCLIInstanceWithStateDir(fs, stateDir)
-	c.Mnemonic = testMnemonicBuffer(t)
-	beforeMove := listedTimes(t, listSecrets(t, c, true))
-
-	time.Sleep(listClockStep)
-	require.NoError(t, c.MoveSecret(&cobra.Command{}, "default:x", "other", false))
-	require.NoError(t, vault.SelectVault(fs, stateDir, "other"))
-
-	afterMove := listedTimes(t, listSecrets(t, c, true))
-	assert.True(t, beforeMove.CreatedAt.Equal(afterMove.CreatedAt),
-		"created %s, then %s", beforeMove.CreatedAt, afterMove.CreatedAt)
-	assert.True(t, beforeMove.UpdatedAt.Equal(afterMove.UpdatedAt),
-		"updated %s, then %s", beforeMove.UpdatedAt, afterMove.UpdatedAt)
-}
-
-// listSecrets returns the output of `secret list`, or of
-// `secret list --json` when jsonOutput is set.
-func listSecrets(t *testing.T, c *cli.Instance, jsonOutput bool) string {
-	t.Helper()
-
-	var out bytes.Buffer
-
-	cmd := &cobra.Command{}
-	cmd.SetOut(&out)
-	require.NoError(t, c.ListSecrets(cmd, jsonOutput, false, ""))
-
-	return out.String()
-}
-
-// listedTimes returns the times that the output of `secret list --json`
-// gives for the one secret in the vault.
-func listedTimes(t *testing.T, listing string) secret.Metadata {
-	t.Helper()
-
-	var parsed struct {
-		Secrets []map[string]string `json:"secrets"`
-	}
-
-	require.NoError(t, json.Unmarshal([]byte(listing), &parsed))
-	require.Len(t, parsed.Secrets, 1)
-
-	createdAt, err := time.Parse(time.RFC3339Nano, parsed.Secrets[0]["created_at"])
-	require.NoError(t, err)
-
-	updatedAt, err := time.Parse(time.RFC3339Nano, parsed.Secrets[0]["updated_at"])
-	require.NoError(t, err)
-
-	return secret.Metadata{CreatedAt: createdAt, UpdatedAt: updatedAt}
-}

internal/cli/root.go

@@ -19,6 +19,8 @@ func Entry() int {
 	// off until the read finishes, so a signal there would leave echo off.
 	// Only a process in the terminal's foreground process group may reset
 	// it: one in the background that tries is stopped instead of exiting.
+	// The function also removes the temporary file that encrypt or decrypt
+	// is writing.
 	terminalState, terminalErr := term.GetState(unix.Stdin)
 
 	memguard.CatchSignal(func(os.Signal) {
@@ -26,6 +28,11 @@ func Entry() int {
 		if terminalErr == nil && err == nil && foreground == unix.Getpgrp() {
 			_ = term.Restore(unix.Stdin, terminalState)
 		}
+
+		name := temporaryOutput.Load()
+		if name != nil {
+			_ = os.Remove(*name)
+		}
 	}, os.Interrupt, unix.SIGTERM)
 
 	defer memguard.Purge()

internal/cli/secrets.go

@@ -541,12 +541,8 @@ func printSecretsJSON(
 			"name": secretName,
 		}
 
-		// A secret whose times cannot be read is listed without them
+		// Try to get metadata using GetSecretObject
 		secretObj, err := vlt.GetSecretObject(secretName)
-		if err == nil {
-			err = secretObj.LoadMetadata()
-		}
-
 		if err == nil {
 			metadata := secretObj.GetMetadata()
 			secretInfo["created_at"] = metadata.CreatedAt
@@ -622,10 +618,6 @@ func printSecretsTable(
 		lastUpdated := "unknown"
 
 		secretObj, err := vlt.GetSecretObject(secretName)
-		if err == nil {
-			err = secretObj.LoadMetadata()
-		}
-
 		if err == nil {
 			metadata := secretObj.GetMetadata()
 			lastUpdated = metadata.UpdatedAt.Format("2006-01-02 15:04")

internal/cli/stdout_stderr_test.go

@@ -2,7 +2,6 @@ package cli_test
 
 import (
 	"bytes"
-	"encoding/json"
 	"os/exec"
 	"path/filepath"
 	"strings"
@@ -10,7 +9,6 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/bip39"
 	"sneak.berlin/go/secret/internal/secret"
 )
 
@@ -92,66 +90,3 @@ func TestGetCommandOutputsToStdout(t *testing.T) {
 	// Nothing should be in stderr
 	assert.Empty(t, stderr.String(), "stderr should be empty")
 }
-
-// TestMnemonicAndListingsOutputToStdout tests that 'secret generate
-// mnemonic', 'secret vault list' and 'secret unlocker list' print their
-// output to stdout, so that it can be piped
-func TestMnemonicAndListingsOutputToStdout(t *testing.T) {
-	t.Parallel()
-
-	tempDir := t.TempDir()
-	secretPath := secretBinaryPath(t)
-
-	// run runs the secret binary and returns what it wrote to stdout and to
-	// stderr
-	run := func(args ...string) (string, string) {
-		//nolint:gosec // G204: test executes the freshly built secret binary
-		cmd := exec.CommandContext(t.Context(), secretPath, args...)
-		cmd.Env = []string{
-			secret.EnvStateDir + "=" + tempDir,
-			secret.EnvMnemonic + "=" + testMnemonic,
-			secret.EnvUnlockPassphrase + "=test-passphrase",
-			"PATH=/usr/bin:/bin",
-		}
-
-		var stdout, stderr bytes.Buffer
-
-		cmd.Stdout = &stdout
-		cmd.Stderr = &stderr
-
-		err := cmd.Run()
-		require.NoError(t, err, "%v should succeed: %s", args, stderr.String())
-
-		return stdout.String(), stderr.String()
-	}
-
-	stdout, stderr := run("generate", "mnemonic")
-	assert.True(t, bip39.IsMnemonicValid(strings.TrimSpace(stdout)),
-		"stdout should hold the mnemonic, not %q", stdout)
-	assert.Contains(t, stderr, "Save this mnemonic phrase securely",
-		"the warning should be on stderr")
-
-	run("init")
-
-	stdout, _ = run("vault", "list", "--json")
-
-	var vaults map[string]any
-
-	require.NoError(t, json.Unmarshal([]byte(stdout), &vaults),
-		"stdout should hold the vaults as JSON, not %q", stdout)
-	assert.Equal(t, "default", vaults["currentVault"])
-
-	stdout, _ = run("unlocker", "list", "--json")
-
-	var unlockers map[string]any
-
-	require.NoError(t, json.Unmarshal([]byte(stdout), &unlockers),
-		"stdout should hold the unlockers as JSON, not %q", stdout)
-	assert.Len(t, unlockers["unlockers"], 1)
-
-	stdout, _ = run("vault", "list")
-	assert.Contains(t, stdout, "default (current)")
-
-	stdout, _ = run("unlocker", "list")
-	assert.Contains(t, stdout, "Total: 1 unlocker(s)")
-}

internal/cli/unlock_failure_test.go

@@ -4,11 +4,9 @@
 // file the unlocker needs is missing or the passphrase is wrong, the error
 // keeps its cause and ends by saying that the mnemonic still opens that
 // vault, but only for a vault that the mnemonic does open, and not when the
-// passphrase could not be read at all. When the current unlocker does not
-// exist and the vault has another that `secret unlocker select` accepts, the
-// error says to select one instead.
-// When a secret's current file is missing, the error says how to make a
-// version current again. Each test that pins such advice also follows it.
+// passphrase could not be read at all. When a secret's current file is
+// missing, the error says how to make a version current again. Each test
+// that pins such advice also follows it.
 
 package cli_test
 
@@ -224,69 +222,6 @@ func TestMoveUnlockFailureNamesVault(t *testing.T) {
 	assert.Equal(t, "value", getSecret(t, c, "y"))
 }
 
-// TestMissingCurrentUnlockerNamesSelect checks the error of `secret get`
-// when current-unlocker names an unlocker that does not exist and the vault
-// has another: it names the missing unlocker and says to select another
-// one, without the advice to use the mnemonic. The test then follows that
-// advice.
-func TestMissingCurrentUnlockerNamesSelect(t *testing.T) {
-	t.Parallel()
-
-	fs := newTwoVaultFs(t)
-	unlockerDir, c := newUnlockTestCLI(t, fs)
-
-	require.NoError(t, afero.WriteFile(fs,
-		filepath.Join(unlockTestVaultDir, "current-unlocker"),
-		[]byte("missing"), secret.FilePerms))
-
-	err := c.GetSecret(discardCmd(), "x")
-
-	require.ErrorIs(t, err, vault.ErrUnlockerNotFound)
-	require.EqualError(t, err, "failed to unlock vault: "+
-		"failed to get long-term key: failed to get current unlocker: "+
-		"unlocker with ID missing not found; the vault 'default' has other "+
-		"unlockers: run 'secret unlocker select' with the ID of one of them "+
-		"to make it the current unlocker")
-
-	require.NoError(t, c.UnlockerSelect(filepath.Base(unlockerDir)))
-	assert.Equal(t, "value", getSecret(t, c, "x"))
-}
-
-// TestMissingCurrentUnlockerNamesMnemonic checks the error of `secret get`
-// when the unlocker that current-unlocker names does not exist and the
-// vault's only other unlocker is the directory "odd", whose metadata names a
-// type that `secret unlocker select` refuses: the error ends with the advice
-// that the mnemonic still opens the vault, not with advice to select another
-// unlocker. The test then follows that advice.
-func TestMissingCurrentUnlockerNamesMnemonic(t *testing.T) {
-	t.Parallel()
-
-	fs := newTwoVaultFs(t)
-	unlockerDir, c := newUnlockTestCLI(t, fs)
-
-	require.NoError(t, fs.RemoveAll(unlockerDir))
-	require.NoError(t, afero.WriteFile(fs,
-		filepath.Join(unlockTestVaultDir, "unlockers.d", "odd",
-			"unlocker-metadata.json"),
-		[]byte(`{"type": "odd"}`), secret.FilePerms))
-
-	err := c.GetSecret(discardCmd(), "x")
-
-	var cause *os.PathError
-
-	require.ErrorAs(t, err, &cause)
-	require.NotErrorIs(t, err, vault.ErrUnlockerNotFound)
-	require.EqualError(t, err, "failed to unlock vault: "+
-		"failed to get long-term key: failed to get current unlocker: "+
-		"failed to read unlocker metadata: "+cause.Error()+mnemonicAdvice)
-
-	c.Mnemonic = testMnemonicBuffer(t)
-	require.NoError(t, c.UnlockersAdd("passphrase", discardCmd()))
-
-	c.Mnemonic = nil
-	assert.Equal(t, "value", getSecret(t, c, "x"))
-}
-
 // TestPassphraseNotReadNamesNoMnemonic runs `secret get x` on the built
 // binary without SB_UNLOCK_PASSPHRASE and with a stdin that is not a
 // terminal, so the passphrase cannot be read. The unlocker was not tried,

internal/cli/unlockers.go

@@ -222,11 +222,8 @@ func newUnlockerRemoveCmd() *cobra.Command {
 		Long: `Remove an unlocker from the current vault. Asks for ` +
 			`confirmation first, saying whether it is the vault's last ` +
 			`unlocker; when stdin is not a terminal, fails unless --force ` +
-			`is given. Removing the current unlocker makes another of the ` +
-			`vault's unlockers current, if it has one that ` +
-			`'secret unlocker select' accepts. Warning: Without ` +
-			`unlockers and without your mnemonic, vault data will be ` +
-			`permanently inaccessible.`,
+			`is given. Warning: Without unlockers and without your ` +
+			`mnemonic, vault data will be permanently inaccessible.`,
 		Args:              cobra.ExactArgs(1),
 		ValidArgsFunction: getUnlockerIDsCompletionFunc(cli.fs, cli.stateDir),
 		RunE: func(cmd *cobra.Command, args []string) error {
@@ -326,25 +323,22 @@ func (cli *Instance) printUnlockersJSON(
 		return fmt.Errorf("failed to marshal JSON: %w", err)
 	}
 
-	_, _ = fmt.Fprintln(cli.cmd.OutOrStdout(), string(jsonBytes))
+	cli.cmd.Println(string(jsonBytes))
 
 	return nil
 }
 
 // printUnlockersTable prints unlockers in a formatted table
 func (cli *Instance) printUnlockersTable(unlockers []UnlockerInfo) error {
-	out := cli.cmd.OutOrStdout()
-
 	if len(unlockers) == 0 {
-		_, _ = fmt.Fprintln(out, "No unlockers found in current vault.")
-		_, _ = fmt.Fprintln(out, "Run 'secret unlocker add passphrase' to create one.")
+		cli.cmd.Println("No unlockers found in current vault.")
+		cli.cmd.Println("Run 'secret unlocker add passphrase' to create one.")
 
 		return nil
 	}
 
-	_, _ = fmt.Fprintf(out, "  %-40s %-12s %-20s %s\n",
-		"UNLOCKER ID", "TYPE", "CREATED", "FLAGS")
-	_, _ = fmt.Fprintf(out, "  %-40s %-12s %-20s %s\n",
+	cli.cmd.Printf("  %-40s %-12s %-20s %s\n", "UNLOCKER ID", "TYPE", "CREATED", "FLAGS")
+	cli.cmd.Printf("  %-40s %-12s %-20s %s\n",
 		strings.Repeat("-", unlockerIDWidth), strings.Repeat("-", unlockerTypeWidth),
 		strings.Repeat("-", unlockerDateWidth), strings.Repeat("-", unlockerFlagsWidth))
 
@@ -359,7 +353,7 @@ func (cli *Instance) printUnlockersTable(unlockers []UnlockerInfo) error {
 			prefix = "* "
 		}
 
-		_, _ = fmt.Fprintf(out, "%s%-40s %-12s %-20s %s\n",
+		cli.cmd.Printf("%s%-40s %-12s %-20s %s\n",
 			prefix,
 			unlocker.ID,
 			unlocker.Type,
@@ -367,7 +361,7 @@ func (cli *Instance) printUnlockersTable(unlockers []UnlockerInfo) error {
 			flags)
 	}
 
-	_, _ = fmt.Fprintf(out, "\nTotal: %d unlocker(s)\n", len(unlockers))
+	cli.cmd.Printf("\nTotal: %d unlocker(s)\n", len(unlockers))
 
 	return nil
 }
@@ -618,17 +612,13 @@ func (cli *Instance) removeUnlocker(
 			"have your mnemonic phrase to access this vault again!")
 	}
 
-	newCurrentID, err := found.vlt.RemoveUnlocker(unlockerID)
+	err := found.vlt.RemoveUnlocker(unlockerID)
 	if err != nil {
 		return err
 	}
 
 	cmd.Printf("Removed unlocker '%s'\n", unlockerID)
 
-	if newCurrentID != "" {
-		cmd.Printf("Switched current unlocker to '%s'\n", newCurrentID)
-	}
-
 	return nil
 }
 

internal/cli/unlockers_remove_test.go

@@ -1,94 +0,0 @@
-//nolint:testpackage // white-box test of unexported internals
-package cli
-
-import (
-	"bytes"
-	"path/filepath"
-	"testing"
-
-	"github.com/awnumar/memguard"
-	"github.com/spf13/afero"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/secret"
-	"sneak.berlin/go/secret/internal/vault"
-)
-
-// TestUnlockerRemoveCurrent is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/137. A vault has a passphrase
-// unlocker and a PGP unlocker, the current one. Removing the PGP unlocker
-// makes the passphrase unlocker current and says so, and the vault then
-// opens through it. Removing the passphrase unlocker too removes
-// current-unlocker.
-//
-// The second case also gives the vault the unlocker directory "odd", whose
-// metadata names a type that `secret unlocker select` refuses, and which
-// sorts before the passphrase unlocker: neither removal makes it current.
-//
-//nolint:paralleltest // t.Setenv (GNUPGHOME) forbids parallel tests
-func TestUnlockerRemoveCurrent(t *testing.T) {
-	tests := []struct {
-		name    string
-		withOdd bool
-	}{
-		{name: "two unlockers", withOdd: false},
-		{name: "and the directory odd", withOdd: true},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			newTestGPGKey(t)
-
-			passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
-			t.Cleanup(passphrase.Destroy)
-
-			fs := afero.NewMemMapFs()
-			vlt, err := vault.CreateVault(fs, listTestStateDir,
-				listTestVaultName, testMnemonicBuffer(t), nil)
-			require.NoError(t, err)
-
-			passphraseUnlocker, err := vlt.CreatePassphraseUnlocker(passphrase)
-			require.NoError(t, err)
-
-			if tt.withOdd {
-				require.NoError(t, afero.WriteFile(fs,
-					filepath.Join(testVaultDir(listTestVaultName), "unlockers.d",
-						"odd", "unlocker-metadata.json"),
-					[]byte(`{"type": "odd"}`), secret.FilePerms))
-			}
-
-			instance, cmd := newTestInstance(fs)
-			instance.UnlockPassphrase = passphrase
-
-			cmd.Flags().String("keyid", unreadableTestGPGUserID, "")
-			require.NoError(t, instance.UnlockersAdd(unlockerTypePGP, cmd))
-
-			pgpUnlocker, err := vlt.GetCurrentUnlocker()
-			require.NoError(t, err)
-			require.Equal(t, unlockerTypePGP, pgpUnlocker.GetType())
-
-			var output bytes.Buffer
-
-			cmd.SetOut(&output)
-			require.NoError(t,
-				instance.UnlockersRemove(pgpUnlocker.GetID(), true, cmd))
-			assert.Contains(t, output.String(),
-				"Switched current unlocker to '"+passphraseUnlocker.GetID()+"'")
-
-			reopened := vault.NewVault(fs, listTestStateDir, listTestVaultName)
-			reopened.UnlockPassphrase = passphrase
-
-			_, err = reopened.GetOrDeriveLongTermKey()
-			require.NoError(t, err)
-
-			require.NoError(t,
-				instance.UnlockersRemove(passphraseUnlocker.GetID(), true, cmd))
-
-			exists, err := afero.Exists(fs, filepath.Join(
-				testVaultDir(listTestVaultName), "current-unlocker"))
-			require.NoError(t, err)
-			assert.False(t, exists,
-				"current-unlocker is left after the passphrase unlocker")
-		})
-	}
-}

internal/cli/vault.go

@@ -186,8 +186,6 @@ func (cli *Instance) ListVaults(cmd *cobra.Command, jsonOutput bool) error {
 		return err
 	}
 
-	out := cmd.OutOrStdout()
-
 	if jsonOutput { //nolint:nestif // Separate JSON and text output formatting logic
 		// Get current vault name for context
 		currentVault := ""
@@ -207,13 +205,13 @@ func (cli *Instance) ListVaults(cmd *cobra.Command, jsonOutput bool) error {
 			return err
 		}
 
-		_, _ = fmt.Fprintln(out, string(jsonBytes))
+		cmd.Println(string(jsonBytes))
 	} else {
 		// Text output
-		_, _ = fmt.Fprintln(out, "Available vaults:")
+		cmd.Println("Available vaults:")
 
 		if len(vaults) == 0 {
-			_, _ = fmt.Fprintln(out, "  (none)")
+			cmd.Println("  (none)")
 		} else {
 			// Try to get current vault for marking
 			currentVault := ""
@@ -225,9 +223,9 @@ func (cli *Instance) ListVaults(cmd *cobra.Command, jsonOutput bool) error {
 
 			for _, vaultName := range vaults {
 				if vaultName == currentVault {
-					_, _ = fmt.Fprintf(out, "  %s (current)\n", vaultName)
+					cmd.Printf("  %s (current)\n", vaultName)
 				} else {
-					_, _ = fmt.Fprintf(out, "  %s\n", vaultName)
+					cmd.Printf("  %s\n", vaultName)
 				}
 			}
 		}

internal/cli/version.go

@@ -65,9 +65,7 @@ func VersionCommands(cli *Instance) *cobra.Command {
 		Use:   "promote <secret-name> <version>",
 		Short: "Promote a specific version to current",
 		Long: "Updates the current symlink to point to the specified " +
-			"version. The versions' own times are left unchanged; secret " +
-			"list then gives the time of the promote as the secret's last " +
-			"update.",
+			"version without modifying timestamps",
 		Args: cobra.ExactArgs(2), //nolint:mnd // secret-name and version args
 		ValidArgsFunction: func(
 			cmd *cobra.Command, args []string, toComplete string,
@@ -169,7 +167,7 @@ func (cli *Instance) ListVersions(cmd *cobra.Command, secretName string) error {
 	}
 
 	if len(versions) == 0 {
-		_, _ = fmt.Fprintln(cmd.OutOrStdout(), "No versions found")
+		cmd.Println("No versions found")
 
 		return nil
 	}

internal/secret/atomic.go

@@ -10,8 +10,8 @@ import (
 	"github.com/spf13/afero"
 )
 
-// tempNamePart is in the name of every temporary file WriteFileAtomic makes,
-// ".NAME.tmp-123", and every temporary directory TempDirFor makes, ".tmp-123".
+// tempNamePart starts the name of every temporary file WriteFileAtomicFunc
+// makes and every temporary directory TempDirFor makes, ".tmp-123".
 const tempNamePart = ".tmp-"
 
 // WriteFileAtomic replaces the file at path with data so that a reader, or
@@ -21,17 +21,38 @@ const tempNamePart = ".tmp-"
 // within one filesystem), is synced to disk, and is renamed over path. The
 // temporary file is removed if any step fails.
 func WriteFileAtomic(fs afero.Fs, path string, data []byte) error {
-	tmp, err := afero.TempFile(fs, filepath.Dir(path),
-		"."+filepath.Base(path)+tempNamePart+"*")
+	return WriteFileAtomicFunc(fs, path, func(file afero.File) error {
+		_, err := file.Write(data)
+		if err != nil {
+			return fmt.Errorf("failed to write %s: %w", path, err)
+		}
+
+		return nil
+	})
+}
+
+// WriteFileAtomicFunc is WriteFileAtomic for content that write streams
+// into the temporary file, which it is given, instead of holding it in
+// memory. An error from write is returned as it is. The temporary file's
+// name leaves out path's, which may already be as long as a name can be.
+func WriteFileAtomicFunc(
+	fs afero.Fs, path string, write func(file afero.File) error,
+) error {
+	tmp, err := afero.TempFile(fs, filepath.Dir(path), tempNamePart)
 	if err != nil {
 		return fmt.Errorf("failed to create temporary file for %s: %w", path, err)
 	}
 
-	_, err = tmp.Write(data)
-	if err == nil {
-		err = tmp.Sync()
+	err = write(tmp)
+	if err != nil {
+		_ = tmp.Close()
+		_ = fs.Remove(tmp.Name())
+
+		return err
 	}
 
+	err = tmp.Sync()
+
 	closeErr := tmp.Close()
 	if err == nil {
 		err = closeErr

internal/secret/secret.go

@@ -2,11 +2,10 @@ package secret
 
 import (
 	"errors"
-	"fmt"
 	"log/slog"
-	"os"
 	"path/filepath"
 	"strings"
+	"time"
 
 	"filippo.io/age"
 	"github.com/awnumar/memguard"
@@ -67,53 +66,31 @@ func NewSecret(vault VaultInterface, name string) *Secret {
 		Name:      name,
 		Directory: secretDir,
 		vault:     vault,
+		Metadata: Metadata{
+			CreatedAt: time.Now(),
+			UpdatedAt: time.Now(),
+		},
 	}
 }
 
-// LoadMetadata sets the secret's times from the modification times of its
-// files, which can be read without unlocking the vault. UpdatedAt is when
-// the secret's current file was last written, by an add or a promote.
-// CreatedAt is when its oldest version's value.age was written. That file
-// is never rewritten; the version's directory is no use, as its time
-// changes when the next add rewrites the version's metadata.age.
+// LoadMetadata is deprecated - metadata is now per-version and encrypted
 func (s *Secret) LoadMetadata() error {
-	fs := s.vault.GetFilesystem()
-
-	current, err := fs.Stat(filepath.Join(s.Directory, "current"))
-	if err != nil {
-		return fmt.Errorf("failed to read when secret %s was updated: %w",
-			s.Name, err)
-	}
-
-	versions, err := ListVersions(fs, s.Directory)
-	if err != nil {
-		return err
-	}
-
-	if len(versions) == 0 {
-		return fmt.Errorf("secret %s has no versions: %w", s.Name, os.ErrNotExist)
-	}
-
-	// ListVersions lists the newest version first
-	oldest := versions[len(versions)-1]
-
-	value, err := fs.Stat(
-		filepath.Join(s.Directory, "versions", oldest, "value.age"))
-	if err != nil {
-		return fmt.Errorf("failed to read when secret %s was created: %w",
-			s.Name, err)
-	}
-
+	Debug("LoadMetadata called but is deprecated in versioned model",
+		"secret_name", s.Name)
+	// For backward compatibility, we'll populate with basic info
+	now := time.Now()
 	s.Metadata = Metadata{
-		CreatedAt: value.ModTime(),
-		UpdatedAt: current.ModTime(),
+		CreatedAt: now,
+		UpdatedAt: now,
 	}
 
 	return nil
 }
 
-// GetMetadata returns the times LoadMetadata read
+// GetMetadata returns the secret metadata (deprecated)
 func (s *Secret) GetMetadata() Metadata {
+	Debug("GetMetadata called but is deprecated in versioned model", "secret_name", s.Name)
+
 	return s.Metadata
 }
 

internal/vault/errors_test.go

@@ -81,9 +81,7 @@ func TestVaultErrors(t *testing.T) {
 			return vlt.CopySecretAllVersions(vlt, versionless, "copy", false)
 		}, vault.ErrNoVersions},
 		{"remove a missing unlocker", func(vlt *vault.Vault) error {
-			_, err := vlt.RemoveUnlocker(missingName)
-
-			return err
+			return vlt.RemoveUnlocker(missingName)
 		}, vault.ErrUnlockerNotFound},
 		{"select a missing unlocker", func(vlt *vault.Vault) error {
 			return vlt.SelectUnlocker(missingName)

internal/vault/secrets.go

@@ -427,8 +427,7 @@ func (v *Vault) UnlockVault() (*age.X25519Identity, error) {
 	return longTermIdentity, nil
 }
 
-// GetSecretObject retrieves a Secret object from this vault. Its times are
-// zero until its LoadMetadata reads them.
+// GetSecretObject retrieves a Secret object with metadata loaded from this vault
 func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
 	err := ValidateSecretName(name)
 	if err != nil {
@@ -455,7 +454,16 @@ func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
 		return nil, fmt.Errorf("secret %s %w", name, ErrSecretNotFound)
 	}
 
-	return secret.NewSecret(v, name), nil
+	// Create a Secret object
+	secretObj := secret.NewSecret(v, name)
+
+	// Load the metadata from disk
+	err = secretObj.LoadMetadata()
+	if err != nil {
+		return nil, err
+	}
+
+	return secretObj, nil
 }
 
 // CopySecretVersion copies a single version from source into destSecretDir
@@ -573,8 +581,8 @@ func (v *Vault) CopySecretAllVersions(
 	}
 
 	// Copy each version and the current pointer, then move the copy into place
-	err = v.copyVersions(srcVault, srcIdentity, srcSecretName, srcSecretDir,
-		destSecretName, destSecretDir, versions, currentVersion)
+	err = v.copyVersions(srcVault, srcIdentity,
+		srcSecretName, destSecretName, destSecretDir, versions, currentVersion)
 	if err != nil {
 		return err
 	}
@@ -793,13 +801,10 @@ func (v *Vault) createAndSaveVersion(
 // pointer into a temporary directory, then moves that directory to
 // destSecretDir, replacing a secret already there. Nothing in this vault
 // changes until the copy is complete, so an interrupted copy leaves only a
-// temporary directory behind. Each copied value.age and current file gets
-// the modification time of the file it was copied from, so `secret list`,
-// which reads a secret's times from them, lists the copy with the times of
-// the source.
+// temporary directory behind.
 func (v *Vault) copyVersions(
 	srcVault *Vault, srcIdentity *age.X25519Identity,
-	srcSecretName, srcSecretDir, destSecretName, destSecretDir string,
+	srcSecretName, destSecretName, destSecretDir string,
 	versions []string, currentVersion string,
 ) error {
 	buildDir, err := secret.TempDirFor(v.fs, destSecretDir)
@@ -818,14 +823,6 @@ func (v *Vault) copyVersions(
 		if err != nil {
 			return fmt.Errorf("failed to copy version %s: %w", versionName, err)
 		}
-
-		valuePath := filepath.Join("versions", versionName, "value.age")
-
-		err = copyModTime(srcVault.fs, filepath.Join(srcSecretDir, valuePath),
-			v.fs, filepath.Join(buildDir, valuePath))
-		if err != nil {
-			return err
-		}
 	}
 
 	err = secret.SetCurrentVersion(v.fs, buildDir, currentVersion)
@@ -833,12 +830,6 @@ func (v *Vault) copyVersions(
 		return fmt.Errorf("failed to set current version: %w", err)
 	}
 
-	err = copyModTime(srcVault.fs, filepath.Join(srcSecretDir, "current"),
-		v.fs, filepath.Join(buildDir, "current"))
-	if err != nil {
-		return err
-	}
-
 	// With --force, the secret being replaced goes only now that its
 	// replacement is complete
 	exists, err := afero.DirExists(v.fs, destSecretDir)
@@ -863,26 +854,6 @@ func (v *Vault) copyVersions(
 	return nil
 }
 
-// copyModTime gives destPath in destFs the modification time of srcPath in
-// srcFs.
-func copyModTime(
-	srcFs afero.Fs, srcPath string, destFs afero.Fs, destPath string,
-) error {
-	info, err := srcFs.Stat(srcPath)
-	if err != nil {
-		return fmt.Errorf("failed to read modification time of %s: %w",
-			srcPath, err)
-	}
-
-	err = destFs.Chtimes(destPath, info.ModTime(), info.ModTime())
-	if err != nil {
-		return fmt.Errorf("failed to set modification time of %s: %w",
-			destPath, err)
-	}
-
-	return nil
-}
-
 // checkCopyDestination refuses to copy over an existing secret unless force
 // is set. A secret being replaced is removed by copyVersions, once its
 // replacement is complete.

internal/vault/unlockers.go

@@ -5,10 +5,8 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
-	"maps"
 	"os"
 	"path/filepath"
-	"slices"
 	"strings"
 	"time"
 
@@ -21,7 +19,6 @@ import (
 // Unlocker metadata type strings.
 const (
 	unlockerTypePassphrase = "passphrase"
-	unlockerTypePGP        = "pgp"
 )
 
 // GetCurrentUnlocker returns the current unlocker for this vault
@@ -55,11 +52,6 @@ func (v *Vault) GetCurrentUnlocker() (secret.Unlocker, error) {
 		return nil, err
 	}
 
-	err = v.checkCurrentUnlockerExists(unlockerDir)
-	if err != nil {
-		return nil, err
-	}
-
 	secret.DebugWith("Resolved unlocker directory",
 		slog.String("unlocker_dir", unlockerDir),
 		slog.String("vault_name", v.Name),
@@ -82,7 +74,7 @@ func (v *Vault) GetCurrentUnlocker() (secret.Unlocker, error) {
 		passphraseUnlocker := secret.NewPassphraseUnlocker(v.fs, unlockerDir, metadata)
 		passphraseUnlocker.Passphrase = v.UnlockPassphrase
 		unlocker = passphraseUnlocker
-	case unlockerTypePGP:
+	case "pgp":
 		secret.Debug("Creating PGP unlocker instance", "unlocker_type", metadata.Type)
 
 		unlocker = secret.NewPGPUnlocker(v.fs, unlockerDir, metadata)
@@ -127,37 +119,6 @@ func (v *Vault) resolveUnlockerDirectory(currentUnlockerPath string) (string, er
 	return absolutePath, nil
 }
 
-// checkCurrentUnlockerExists fails with ErrUnlockerNotFound, saying to
-// select another unlocker, when unlockerDir, the unlocker directory that
-// current-unlocker names, does not exist and the vault has another unlocker
-// that SelectUnlocker accepts. When it has none, it returns nil: reading the
-// missing unlocker then fails, with the advice that the mnemonic still opens
-// the vault.
-func (v *Vault) checkCurrentUnlockerExists(unlockerDir string) error {
-	exists, err := afero.DirExists(v.fs, unlockerDir)
-	if err != nil {
-		return fmt.Errorf("failed to check for the current unlocker: %w", err)
-	}
-
-	if exists {
-		return nil
-	}
-
-	otherID, err := v.firstOtherUnlockerID(filepath.Base(unlockerDir))
-	if err != nil {
-		return err
-	}
-
-	if otherID == "" {
-		return nil
-	}
-
-	return fmt.Errorf("unlocker with ID %s %w; the vault '%s' has other "+
-		"unlockers: run %s with the ID of one of them to make it the current "+
-		"unlocker", filepath.Base(unlockerDir), ErrUnlockerNotFound, v.Name,
-		v.withVaultSelect("'secret unlocker select'"))
-}
-
 // findUnlockerByID finds an unlocker by its ID and returns the unlocker
 // instance and its directory path. A directory that ListUnlockers skips is
 // skipped here too, with the same warning. Such a directory has no ID: if
@@ -198,7 +159,7 @@ func (v *Vault) findUnlockerByID(
 		switch metadata.Type {
 		case unlockerTypePassphrase:
 			tempUnlocker = secret.NewPassphraseUnlocker(v.fs, unlockerDirPath, metadata)
-		case unlockerTypePGP:
+		case "pgp":
 			tempUnlocker = secret.NewPGPUnlocker(v.fs, unlockerDirPath, metadata)
 		default:
 			continue
@@ -321,15 +282,10 @@ func (v *Vault) HasUnlocker(unlockerID string) (bool, error) {
 // RemoveUnlocker removes an unlocker from this vault. An unlocker
 // directory that ListUnlockers skips is removed by its directory name; its
 // type is unknown, so only the directory is removed.
-//
-// It returns the ID of the unlocker it made current, or "" when it made none
-// current. If the removed unlocker is the current one, the unlocker
-// firstOtherUnlockerID returns is made current before the removal; with
-// none, current-unlocker is removed after it.
-func (v *Vault) RemoveUnlocker(unlockerID string) (string, error) {
+func (v *Vault) RemoveUnlocker(unlockerID string) error {
 	vaultDir, err := v.GetDirectory()
 	if err != nil {
-		return "", err
+		return err
 	}
 
 	// Find the unlocker directory and create the unlocker instance
@@ -338,92 +294,19 @@ func (v *Vault) RemoveUnlocker(unlockerID string) (string, error) {
 	// Find the unlocker by ID
 	unlocker, unlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID)
 	if err != nil {
-		return "", err
+		return err
 	}
 
 	if unlockerDir == "" {
-		return "", fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
-	}
-
-	currentUnlockerPath := filepath.Join(vaultDir, "current-unlocker")
-
-	currentDir, err := v.resolveUnlockerDirectory(currentUnlockerPath)
-	if err != nil && !errors.Is(err, os.ErrNotExist) {
-		return "", err
-	}
-
-	isCurrent := currentDir == unlockerDir
-	newCurrentID := ""
-
-	if isCurrent {
-		newCurrentID, err = v.selectOtherUnlocker(unlockerID)
-		if err != nil {
-			return "", err
-		}
+		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
 	}
 
 	if unlocker == nil {
-		err = secret.RemoveDirAtomic(v.fs, unlockerDir)
-	} else {
-		err = unlocker.Remove()
+		return secret.RemoveDirAtomic(v.fs, unlockerDir)
 	}
 
-	if err != nil {
-		return "", err
-	}
-
-	if isCurrent && newCurrentID == "" {
-		err = v.fs.Remove(currentUnlockerPath)
-		if err != nil {
-			return "", fmt.Errorf("failed to remove current-unlocker file: %w", err)
-		}
-	}
-
-	return newCurrentID, nil
-}
-
-// selectOtherUnlocker makes the unlocker firstOtherUnlockerID returns for
-// unlockerID the current unlocker and returns its ID. It returns "" when
-// there is no such unlocker.
-func (v *Vault) selectOtherUnlocker(unlockerID string) (string, error) {
-	otherID, err := v.firstOtherUnlockerID(unlockerID)
-	if err != nil {
-		return "", err
-	}
-
-	if otherID == "" {
-		return "", nil
-	}
-
-	err = v.SelectUnlocker(otherID)
-	if err != nil {
-		return "", err
-	}
-
-	return otherID, nil
-}
-
-// firstOtherUnlockerID returns the ID of the vault's first unlocker other
-// than unlockerID, in ID order, the order `secret unlocker list` shows, that
-// SelectUnlocker accepts, or "" when there is none. SelectUnlocker refuses
-// an unlocker whose metadata names a type other than passphrase or pgp,
-// which ListUnlockers still returns.
-func (v *Vault) firstOtherUnlockerID(unlockerID string) (string, error) {
-	unlockers, err := v.ListUnlockers()
-	if err != nil {
-		return "", err
-	}
-
-	for _, id := range slices.Sorted(maps.Keys(unlockers)) {
-		unlockerType := unlockers[id].Type
-
-		if id != unlockerID && (unlockerType == unlockerTypePassphrase ||
-			unlockerType == unlockerTypePGP) {
-			return id, nil
-		}
-	}
-
-	return "", nil
+	// Use the unlocker's Remove method
+	return unlocker.Remove()
 }
 
 // SelectUnlocker selects an unlocker as current for this vault

internal/vault/vault.go

@@ -291,11 +291,9 @@ func (v *Vault) unlockLongTermKey(
 // without a mnemonic records none, and without its metadata the key cannot
 // be derived. It is not added when the passphrase could not be read: the
 // unlocker was not tried, and adding one would need a passphrase read the
-// same way. Nor is it added when the current unlocker does not exist and the
-// vault has others: the error already says to select one of them.
+// same way.
 func (v *Vault) withMnemonicAdvice(err error) error {
-	if errors.Is(err, secret.ErrPassphraseNotRead) ||
-		errors.Is(err, ErrUnlockerNotFound) {
+	if errors.Is(err, secret.ErrPassphraseNotRead) {
 		return err
 	}
 
@@ -306,21 +304,15 @@ func (v *Vault) withMnemonicAdvice(err error) error {
 		return err
 	}
 
-	return fmt.Errorf("%w; the vault '%s' still opens with its mnemonic: run "+
-		"%s with %s set to the mnemonic to give it a new unlocker",
-		err, v.Name, v.withVaultSelect("'secret unlocker add passphrase'"),
-		secret.EnvMnemonic)
-}
+	// 'secret unlocker add' acts on the current vault only.
+	steps := "'secret unlocker add passphrase'"
 
-// withVaultSelect returns command, a quoted 'secret unlocker' command, for
-// advice about this vault. Such a command acts on the current vault only,
-// so when this vault is not the current one, the command that selects it
-// comes first.
-func (v *Vault) withVaultSelect(command string) string {
-	current, err := GetCurrentVault(v.fs, v.stateDir)
-	if err != nil || current.Name != v.Name {
-		return fmt.Sprintf("'secret vault select %s', then %s", v.Name, command)
+	current, currentErr := GetCurrentVault(v.fs, v.stateDir)
+	if currentErr != nil || current.Name != v.Name {
+		steps = fmt.Sprintf("'secret vault select %s', then %s", v.Name, steps)
 	}
 
-	return command
+	return fmt.Errorf("%w; the vault '%s' still opens with its mnemonic: run "+
+		"%s with %s set to the mnemonic to give it a new unlocker",
+		err, v.Name, steps, secret.EnvMnemonic)
 }