Stop vault creation when a vault's metadata cannot be read (closes #140)

GetNextDerivationIndex passed over a vault whose vault-metadata.json could
not be read or parsed, so the next vault made from the same mnemonic could
get that vault's derivation index and so the same long-term key. It now
returns an error naming the file, and vault create, vault import and init
stop before writing anything. A vault directory with no metadata file is
still skipped.

The regression test breaks one vault's metadata, once with content that
does not parse and once with a test filesystem that fails to open it, and
requires CreateVault with the same mnemonic to fail, naming the file, and
leave no new vault.

Model: opus-5-5

README.md

@@ -150,6 +150,13 @@ 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
@@ -181,7 +188,9 @@ period.
 #### `secret version promote <secret-name> <version>`
 
 Promotes a specific version to current by rewriting the secret's `current` file
-to name it. Does not modify any timestamps, allowing for rollback scenarios.
+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.
 
 #### `secret version remove <secret-name> <version> [--force]` / `secret version rm` ⚠️ 🛑
 
@@ -522,7 +531,11 @@ secret decrypt encryption/mykey --input document.txt.age --output document.txt
 ### Vault Management
 
 - **Derivation Index**: Each vault uses a unique derivation index from the
-  mnemonic, and thus a unique key pair
+  mnemonic, and thus a unique key pair; creating a vault or importing a mnemonic
+  fails, with an error naming the file, when any vault's `vault-metadata.json`
+  exists but cannot be read or parsed; a vault directory with no
+  `vault-metadata.json` is skipped, so the index it uses can be given to a new
+  vault
 - **Public Key Hash**: Double SHA-256 hash of the vault's public key; the same
   hash of the index-0 public key identifies vaults from the same mnemonic
 - **Automatic Key Derivation**: When creating vaults with a mnemonic, keys are

TODO.md

@@ -18,6 +18,14 @@ https://git.eeqj.de/sneak/secret/milestone/12
 
 # Completed Steps
 
+- 2026-10-14: A wrong passphrase, from `SB_UNLOCK_PASSPHRASE` or typed at the
+  prompt, gives the error `wrong passphrase for unlocker <id>`
+  (https://git.eeqj.de/sneak/secret/issues/157). It used to be age's "no
+  identity matched any of the recipients", followed by advice to give the vault
+  a new unlocker from its mnemonic, which would have replaced an unlocker that
+  works. The error now wraps `secret.ErrWrongPassphrase` and carries no advice
+  to use the mnemonic. A `priv.age` that is not an age file still gets that
+  advice.
 - 2026-10-14: The passphrase and mnemonic prompts ask on the controlling
   terminal (`/dev/tty`) instead of stdin, so `echo v | secret add s1 --force`,
   `cat file | secret encrypt key` and `cat file.age | secret decrypt key` ask

internal/cli/list_times_test.go

@@ -0,0 +1,161 @@
+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/secrets.go

@@ -541,8 +541,12 @@ func printSecretsJSON(
 			"name": secretName,
 		}
 
-		// Try to get metadata using GetSecretObject
+		// A secret whose times cannot be read is listed without them
 		secretObj, err := vlt.GetSecretObject(secretName)
+		if err == nil {
+			err = secretObj.LoadMetadata()
+		}
+
 		if err == nil {
 			metadata := secretObj.GetMetadata()
 			secretInfo["created_at"] = metadata.CreatedAt
@@ -618,6 +622,10 @@ 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/unlock_failure_test.go

@@ -1,12 +1,12 @@
 // Unlock Failure Tests
 //
 // When a vault cannot be opened through its current unlocker, because a
-// 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.
+// file the unlocker needs is missing, 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. A wrong passphrase gives an error that says so, without that advice.
+// 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.
 
@@ -21,7 +21,6 @@ import (
 	"syscall"
 	"testing"
 
-	"filippo.io/age"
 	"github.com/awnumar/memguard"
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
@@ -165,26 +164,48 @@ func TestUnlockFailureNamesMnemonic(t *testing.T) {
 	}
 }
 
-// TestWrongPassphraseNamesMnemonic checks the error of `secret get` given a
-// passphrase that does not decrypt the passphrase unlocker: it keeps age's
-// error and ends with the advice that the mnemonic still opens the vault.
-func TestWrongPassphraseNamesMnemonic(t *testing.T) {
+// TestWrongPassphraseNamesPassphrase checks the error of `secret get` given
+// a passphrase that does not decrypt the passphrase unlocker: it says that
+// the passphrase is wrong, without the advice to use the mnemonic, since the
+// unlocker still opens the vault with the right passphrase. The test then
+// gives the right passphrase.
+func TestWrongPassphraseNamesPassphrase(t *testing.T) {
 	t.Parallel()
 
-	_, c := newUnlockTestCLI(t, newTwoVaultFs(t))
+	unlockerDir, c := newUnlockTestCLI(t, newTwoVaultFs(t))
 	c.UnlockPassphrase = memguard.NewBufferFromBytes([]byte("wrong passphrase"))
 	t.Cleanup(c.UnlockPassphrase.Destroy)
 
 	err := c.GetSecret(discardCmd(), "x")
 
-	var noMatch *age.NoIdentityMatchError
-
-	require.ErrorAs(t, err, &noMatch)
-
+	require.ErrorIs(t, err, secret.ErrWrongPassphrase)
+	assert.NotContains(t, err.Error(), mnemonicAdvice)
 	require.EqualError(t, err, "failed to unlock vault: "+
 		"failed to get long-term key: failed to get unlocker identity: "+
-		"failed to decrypt unlocker private key: failed to create decryptor: "+
-		noMatch.Error()+mnemonicAdvice)
+		"wrong passphrase for unlocker "+filepath.Base(unlockerDir))
+
+	c.UnlockPassphrase = memguard.NewBufferFromBytes([]byte(testPassphrase))
+	t.Cleanup(c.UnlockPassphrase.Destroy)
+	assert.Equal(t, "value", getSecret(t, c, "x"))
+}
+
+// TestDamagedUnlockerNamesMnemonic checks the error of `secret get` when the
+// passphrase unlocker's priv.age is not an age file: the passphrase is not
+// blamed, and the error ends with the advice that the mnemonic still opens
+// the vault.
+func TestDamagedUnlockerNamesMnemonic(t *testing.T) {
+	t.Parallel()
+
+	fs := newTwoVaultFs(t)
+	unlockerDir, c := newUnlockTestCLI(t, fs)
+
+	require.NoError(t, afero.WriteFile(fs, filepath.Join(unlockerDir, "priv.age"),
+		[]byte("damaged"), secret.FilePerms))
+
+	err := c.GetSecret(discardCmd(), "x")
+
+	require.NotErrorIs(t, err, secret.ErrWrongPassphrase)
+	require.ErrorContains(t, err, mnemonicAdvice)
 }
 
 // TestMoveUnlockFailureNamesVault checks the error of `secret move` into

internal/cli/unlockers.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"log"
 	"maps"
+	"net/url"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -66,12 +67,13 @@ func getDefaultGPGKey() (string, error) {
 
 	output, err := cmd.Output()
 	if err == nil {
-		for line := range strings.SplitSeq(string(output), "\n") {
-			fields := strings.Split(line, ":")
-			if len(fields) > 9 && fields[0] == "default-key" && fields[9] != "" {
-				// The default key is in field 10 (index 9)
-				return fields[9], nil
-			}
+		defaultKey, parseErr := parseDefaultGPGKey(string(output))
+		if parseErr != nil {
+			return "", parseErr
+		}
+
+		if defaultKey != "" {
+			return defaultKey, nil
 		}
 	}
 
@@ -98,6 +100,27 @@ func getDefaultGPGKey() (string, error) {
 	return "", errNoGPGSecretKeys
 }
 
+// parseDefaultGPGKey returns the default-key value in the output of
+// `gpgconf --list-options gpg`, or "" when gpg.conf sets none. gpgconf(1)
+// prints a string value with a leading `"` and percent-escaped.
+func parseDefaultGPGKey(gpgconfOutput string) (string, error) {
+	for line := range strings.SplitSeq(gpgconfOutput, "\n") {
+		fields := strings.Split(line, ":")
+		if len(fields) > 9 && fields[0] == "default-key" && fields[9] != "" {
+			// The default key is in field 10 (index 9)
+			keyID, err := url.PathUnescape(strings.TrimPrefix(fields[9], `"`))
+			if err != nil {
+				return "", fmt.Errorf("failed to decode gpgconf default-key %s: %w",
+					fields[9], err)
+			}
+
+			return keyID, nil
+		}
+	}
+
+	return "", nil
+}
+
 func newUnlockerCmd() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "unlocker",

internal/cli/unlockers_add_test.go

@@ -2,6 +2,8 @@
 package cli
 
 import (
+	"bytes"
+	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -83,6 +85,77 @@ func TestAddPGPUnlocker(t *testing.T) {
 	}
 }
 
+// TestAddPGPUnlockerDefaultKey is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/141: with default-key set in
+// gpg.conf and no --keyid, `secret unlocker add pgp` uses that key.
+// gpg.conf names the key by its user ID, and the printed key ID must be that
+// user ID. The first secret key, which is used when gpg.conf sets no
+// default-key, prints as a hex key ID.
+func TestAddPGPUnlockerDefaultKey(t *testing.T) {
+	newTestGPGKey(t)
+	// The key comes from neither --keyid nor the environment.
+	t.Setenv(secret.EnvGPGKeyID, "")
+
+	gpgConf := filepath.Join(os.Getenv("GNUPGHOME"), "gpg.conf")
+	//nolint:gosec // G703: GNUPGHOME is the directory newTestGPGKey made
+	require.NoError(t, os.WriteFile(gpgConf,
+		[]byte("default-key "+unreadableTestGPGUserID+"\n"), 0o600))
+
+	fs := afero.NewMemMapFs()
+	_, err := vault.CreateVault(fs, listTestStateDir, listTestVaultName,
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	instance, cmd := newTestInstance(fs)
+	instance.Mnemonic = testMnemonicBuffer(t)
+
+	var output bytes.Buffer
+	cmd.SetOut(&output)
+
+	require.NoError(t, instance.UnlockersAdd(unlockerTypePGP, cmd))
+	assert.Contains(t, output.String(),
+		"Using default GPG key: "+unreadableTestGPGUserID+"\n")
+}
+
+// TestParseDefaultGPGKey is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/141: gpgconf prints the
+// default-key value with a leading `"` and percent-escaped, and the key ID
+// is that value decoded.
+func TestParseDefaultGPGKey(t *testing.T) {
+	t.Parallel()
+
+	// gpgconfLine is the default-key line of `gpgconf --list-options gpg`
+	// up to the value, its last field.
+	const gpgconfLine = "default-key:0:0:use NAME as default secret key:" +
+		"1:1:NAME:::"
+
+	tests := []struct {
+		name  string
+		value string
+		want  string
+	}{
+		{
+			"fingerprint", `"56163D7F3FAFF709608855BEC1A360216D007F41`,
+			"56163D7F3FAFF709608855BEC1A360216D007F41",
+		},
+		{
+			"user ID with a comma", `"Doe%2c Alice <alice@example.com>`,
+			"Doe, Alice <alice@example.com>",
+		},
+		{"no default-key in gpg.conf", "", ""},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			keyID, err := parseDefaultGPGKey(gpgconfLine + test.value + "\n")
+			require.NoError(t, err)
+			assert.Equal(t, test.want, keyID)
+		})
+	}
+}
+
 // 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

internal/cli/version.go

@@ -65,7 +65,9 @@ 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 without modifying timestamps",
+			"version. The versions' own times are left unchanged; secret " +
+			"list then gives the time of the promote as the secret's last " +
+			"update.",
 		Args: cobra.ExactArgs(2), //nolint:mnd // secret-name and version args
 		ValidArgsFunction: func(
 			cmd *cobra.Command, args []string, toComplete string,

internal/secret/passphraseunlocker.go

@@ -16,6 +16,10 @@ import (
 // empty. A passphrase unlocker that fails with it was not tried.
 var ErrPassphraseNotRead = errors.New("failed to read passphrase")
 
+// ErrWrongPassphrase is wrapped in the error of GetIdentity when the
+// passphrase does not decrypt the unlocker's private key.
+var ErrWrongPassphrase = errors.New("wrong passphrase")
+
 // PassphraseUnlocker represents a passphrase-protected unlocker
 type PassphraseUnlocker struct {
 	Directory  string
@@ -73,6 +77,12 @@ func (p *PassphraseUnlocker) GetIdentity() (*age.X25519Identity, error) {
 		Debug("Failed to decrypt unlocker private key",
 			"error", err, "unlocker_id", p.GetID())
 
+		// age says that no identity matched when the passphrase is wrong
+		var noMatch *age.NoIdentityMatchError
+		if errors.As(err, &noMatch) {
+			return nil, fmt.Errorf("%w for unlocker %s", ErrWrongPassphrase, p.GetID())
+		}
+
 		return nil, fmt.Errorf("failed to decrypt unlocker private key: %w", err)
 	}
 	defer privKeyBuffer.Destroy()

internal/secret/secret.go

@@ -2,10 +2,11 @@ package secret
 
 import (
 	"errors"
+	"fmt"
 	"log/slog"
+	"os"
 	"path/filepath"
 	"strings"
-	"time"
 
 	"filippo.io/age"
 	"github.com/awnumar/memguard"
@@ -66,31 +67,53 @@ func NewSecret(vault VaultInterface, name string) *Secret {
 		Name:      name,
 		Directory: secretDir,
 		vault:     vault,
-		Metadata: Metadata{
-			CreatedAt: time.Now(),
-			UpdatedAt: time.Now(),
-		},
 	}
 }
 
-// LoadMetadata is deprecated - metadata is now per-version and encrypted
+// 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.
 func (s *Secret) LoadMetadata() error {
-	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()
+	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)
+	}
+
 	s.Metadata = Metadata{
-		CreatedAt: now,
-		UpdatedAt: now,
+		CreatedAt: value.ModTime(),
+		UpdatedAt: current.ModTime(),
 	}
 
 	return nil
 }
 
-// GetMetadata returns the secret metadata (deprecated)
+// GetMetadata returns the times LoadMetadata read
 func (s *Secret) GetMetadata() Metadata {
-	Debug("GetMetadata called but is deprecated in versioned model", "secret_name", s.Name)
-
 	return s.Metadata
 }
 

internal/vault/metadata.go

@@ -4,7 +4,9 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
+	"errors"
 	"fmt"
+	"os"
 	"path/filepath"
 
 	"github.com/spf13/afero"
@@ -33,7 +35,10 @@ func ComputeDoubleSHA256(data []byte) string {
 }
 
 // GetNextDerivationIndex finds the next available derivation index for a given mnemonic
-// by deriving the public key for index 0 and using its hash to identify related vaults
+// by deriving the public key for index 0 and using its hash to identify related vaults.
+// It fails when a vault's metadata file exists but cannot be read or parsed: the
+// index that vault uses is then unknown, and handing it out again would give a
+// second vault the same long-term key.
 func GetNextDerivationIndex(
 	fs afero.Fs, stateDir string, mnemonic string,
 ) (uint32, error) {
@@ -72,21 +77,23 @@ func GetNextDerivationIndex(
 			continue
 		}
 
-		// Try to read vault metadata
 		metadataPath := filepath.Join(vaultsDir, entry.Name(), "vault-metadata.json")
 
 		metadataBytes, err := afero.ReadFile(fs, metadataPath)
-		if err != nil {
-			// Skip vaults without metadata
+		if errors.Is(err, os.ErrNotExist) {
+			// Skip directories without metadata
 			continue
 		}
 
+		if err != nil {
+			return 0, fmt.Errorf("failed to read vault metadata %s: %w", metadataPath, err)
+		}
+
 		var metadata Metadata
 
 		err = json.Unmarshal(metadataBytes, &metadata)
 		if err != nil {
-			// Skip vaults with invalid metadata
-			continue
+			return 0, fmt.Errorf("failed to parse vault metadata %s: %w", metadataPath, err)
 		}
 
 		// Check if this vault uses the same mnemonic by comparing family hashes

internal/vault/metadata_test.go

@@ -1,11 +1,15 @@
 package vault_test
 
 import (
+	"errors"
 	"path/filepath"
 	"strings"
 	"testing"
 
 	"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"
 	"sneak.berlin/go/secret/pkg/agehd"
 )
@@ -163,6 +167,88 @@ func testGetNextDerivationIndex(t *testing.T, fs afero.Fs) {
 	}
 }
 
+// errMetadataUnreadable is returned by the test filesystem in place of a
+// successful open of one vault's metadata file.
+var errMetadataUnreadable = errors.New("input/output error")
+
+// metadataReadFailFs fails every open of the file at unreadablePath. The
+// file still exists, so checking for it succeeds and only reading it fails.
+type metadataReadFailFs struct {
+	afero.Fs
+
+	unreadablePath string
+}
+
+//nolint:ireturn // afero.File is the interface required by afero.Fs
+func (f *metadataReadFailFs) Open(name string) (afero.File, error) {
+	if name == f.unreadablePath {
+		return nil, errMetadataUnreadable
+	}
+
+	return f.Fs.Open(name)
+}
+
+// TestCreateVaultWithCorruptMetadataFails checks that creating a vault from
+// a mnemonic fails, naming the file and writing nothing, when another
+// vault's metadata file exists but cannot be parsed or read. That vault's
+// derivation index is then unknown, so the new vault could get the same one
+// and the same key.
+func TestCreateVaultWithCorruptMetadataFails(t *testing.T) {
+	t.Parallel()
+
+	vaultsDir := filepath.Join(testStateDir, "vaults.d")
+	metadataPath := filepath.Join(vaultsDir, "work", "vault-metadata.json")
+
+	tests := []struct {
+		name string
+		// breakMetadata makes the work vault's metadata file unusable on
+		// base and returns the filesystem to create the second vault on.
+		breakMetadata func(t *testing.T, base afero.Fs) afero.Fs
+	}{
+		{
+			name: "parsing the file fails",
+			breakMetadata: func(t *testing.T, base afero.Fs) afero.Fs {
+				t.Helper()
+
+				err := afero.WriteFile(base, metadataPath, []byte("{"),
+					secret.FilePerms)
+				require.NoError(t, err)
+
+				return base
+			},
+		},
+		{
+			name: "reading the file fails",
+			breakMetadata: func(_ *testing.T, base afero.Fs) afero.Fs {
+				return &metadataReadFailFs{Fs: base, unreadablePath: metadataPath}
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			base := afero.NewMemMapFs()
+
+			_, err := vault.CreateVault(base, testStateDir, "work",
+				testMnemonicBuffer(t), nil)
+			require.NoError(t, err)
+
+			fs := tt.breakMetadata(t, base)
+
+			_, err = vault.CreateVault(fs, testStateDir, "other",
+				testMnemonicBuffer(t), nil)
+			require.ErrorContains(t, err, metadataPath)
+
+			entries, err := afero.ReadDir(base, vaultsDir)
+			require.NoError(t, err)
+			require.Len(t, entries, 1)
+			assert.Equal(t, "work", entries[0].Name())
+		})
+	}
+}
+
 func testMetadataPersistence(t *testing.T, fs afero.Fs) {
 	t.Helper()
 

internal/vault/secrets.go

@@ -427,7 +427,8 @@ func (v *Vault) UnlockVault() (*age.X25519Identity, error) {
 	return longTermIdentity, nil
 }
 
-// GetSecretObject retrieves a Secret object with metadata loaded from this vault
+// GetSecretObject retrieves a Secret object from this vault. Its times are
+// zero until its LoadMetadata reads them.
 func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
 	err := ValidateSecretName(name)
 	if err != nil {
@@ -454,16 +455,7 @@ func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
 		return nil, fmt.Errorf("secret %s %w", name, ErrSecretNotFound)
 	}
 
-	// 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
+	return secret.NewSecret(v, name), nil
 }
 
 // CopySecretVersion copies a single version from source into destSecretDir
@@ -581,8 +573,8 @@ func (v *Vault) CopySecretAllVersions(
 	}
 
 	// Copy each version and the current pointer, then move the copy into place
-	err = v.copyVersions(srcVault, srcIdentity,
-		srcSecretName, destSecretName, destSecretDir, versions, currentVersion)
+	err = v.copyVersions(srcVault, srcIdentity, srcSecretName, srcSecretDir,
+		destSecretName, destSecretDir, versions, currentVersion)
 	if err != nil {
 		return err
 	}
@@ -801,10 +793,13 @@ 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.
+// 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.
 func (v *Vault) copyVersions(
 	srcVault *Vault, srcIdentity *age.X25519Identity,
-	srcSecretName, destSecretName, destSecretDir string,
+	srcSecretName, srcSecretDir, destSecretName, destSecretDir string,
 	versions []string, currentVersion string,
 ) error {
 	buildDir, err := secret.TempDirFor(v.fs, destSecretDir)
@@ -823,6 +818,14 @@ 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)
@@ -830,6 +833,12 @@ 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)
@@ -854,6 +863,26 @@ 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/vault.go

@@ -291,10 +291,13 @@ 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. Nor is it added when the passphrase is wrong: the unlocker still
+// opens the vault with the right one. 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.
 func (v *Vault) withMnemonicAdvice(err error) error {
 	if errors.Is(err, secret.ErrPassphraseNotRead) ||
+		errors.Is(err, secret.ErrWrongPassphrase) ||
 		errors.Is(err, ErrUnlockerNotFound) {
 		return err
 	}

internal/vault/vault_error_test.go

@@ -5,7 +5,6 @@ import (
 	"path/filepath"
 	"testing"
 
-	"filippo.io/age"
 	"github.com/awnumar/memguard"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
@@ -140,9 +139,7 @@ func TestAddSecretForceWithWrongPassphraseChangesNothing(t *testing.T) {
 
 	err = vlt.AddSecret(testSecretName, second, true)
 
-	var noMatch *age.NoIdentityMatchError
-
-	require.ErrorAs(t, err, &noMatch)
+	require.ErrorIs(t, err, secret.ErrWrongPassphrase)
 	assert.Equal(t, before, dirContents(t, fs, secretDir))
 }