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, so vault create, vault import and init
stop with that error and leave the state directory as it was. 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

@@ -129,6 +129,17 @@ one.
 
 ### Secret Management
 
+#### Secret Size
+
+While `secret` works on a secret's value it keeps the value in locked memory, so
+the value counts against the locked-memory limit (`ulimit -l`). `secret` raises
+that limit to the hard limit when it starts. `secret add`, `secret import` and
+`secret generate secret` refuse a value that does not fit in the limit with 8
+memory pages to spare (32 KiB with 4 KiB pages), with an error that names the
+limit. Linux's usual limit of 8 MiB allows a value of nearly 8 MiB; a larger one
+needs a higher hard limit, such as `memlock` in `/etc/security/limits.conf`. No
+value may be over 100 MiB, whatever the limit.
+
 #### `secret add <secret-name> [--force]`
 
 Adds a secret to the current vault. Reads the secret value from stdin.
@@ -533,7 +544,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

internal/cli/generate.go

@@ -72,6 +72,12 @@ func newGenerateSecretCmd() *cobra.Command {
 				return fmt.Errorf("failed to initialize CLI: %w", err)
 			}
 
+			// Before readSecretEnv, which makes the first locked allocation
+			err = checkLockedMemoryLimit(0)
+			if err != nil {
+				return err
+			}
+
 			destroySecrets := cli.readSecretEnv()
 			defer destroySecrets()
 
@@ -135,10 +141,16 @@ func (cli *Instance) GenerateSecret(
 		return errLengthTooSmall
 	}
 
-	var (
+	if length > maxSecretSize {
-		secretValue string
+		return errSecretTooLarge
-		err         error
+	}
-	)
+
+	err := checkLockedMemoryLimit(length)
+	if err != nil {
+		return err
+	}
+
+	var secretValue string
 
 	switch secretType {
 	case "base58":

internal/cli/root.go

@@ -14,6 +14,8 @@ import (
 // every memguard buffer before it returns, so the caller must do nothing
 // but exit with the code.
 func Entry() int {
+	raiseLockedMemoryLimit()
+
 	// On SIGINT or SIGTERM memguard runs this function, wipes every buffer
 	// and exits with status 1. The passphrase prompt turns echo off on the
 	// controlling terminal until the read finishes, so a signal there would
@@ -51,6 +53,19 @@ func Entry() int {
 	return 0
 }
 
+// raiseLockedMemoryLimit raises this process's limit on locked memory, which
+// holds every secret value, to the hard limit, the most a process may set
+// without privileges. When that fails the limit stays as it was, and
+// checkLockedMemoryLimit names it in its error.
+func raiseLockedMemoryLimit() {
+	var limit unix.Rlimit
+
+	if unix.Getrlimit(unix.RLIMIT_MEMLOCK, &limit) == nil {
+		limit.Cur = limit.Max
+		_ = unix.Setrlimit(unix.RLIMIT_MEMLOCK, &limit)
+	}
+}
+
 func newRootCmd() *cobra.Command {
 	secret.Debug("newRootCmd starting")
 

internal/cli/secrets.go

@@ -14,6 +14,7 @@ import (
 	"github.com/awnumar/memguard"
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
+	"golang.org/x/sys/unix"
 	"sneak.berlin/go/secret/internal/secret"
 	"sneak.berlin/go/secret/internal/vault"
 )
@@ -24,26 +25,30 @@ const (
 	// vaultSecretParts is the number of parts when splitting vault:secret
 	vaultSecretParts = 2
 
-	// initialBufferSize is the starting size for secret read buffers (4KB)
-	initialBufferSize = 4 * 1024
 	// maxSecretSize is the maximum allowed size of a secret (100MB)
 	maxSecretSize = 100 * 1024 * 1024
+
+	// lockedMemoryReservePages is the locked memory, in pages, that `secret
+	// add`, `secret import` and `secret generate secret` may need besides
+	// the value they store, for keys and the read buffer: memguard locks
+	// whole pages, at least one for each buffer. `secret add` was measured
+	// to need 7 with both SB_SECRET_MNEMONIC and SB_UNLOCK_PASSPHRASE set,
+	// and 6 with one; the eighth is to spare.
+	lockedMemoryReservePages = 8
+	// kibibyte is the unit of the locked-memory limit in `ulimit -l`.
+	kibibyte = 1024
 )
 
 // Sentinel errors for secret operations
 var (
-	errSecretTooLarge              = errors.New("secret too large: exceeds 100MB limit")
+	errSecretTooLarge    = errors.New("secret too large: exceeds 100MB limit")
+	errLockedMemoryLimit = errors.New(
+		"secret too large for the locked-memory limit")
 	errCrossVaultSourceUnqualified = errors.New(
 		"source must specify vault (e.g., vault:secret) for cross-vault move")
 	errMoveOntoItself = errors.New("cannot be moved onto itself")
 )
 
-// bufferInfo tracks a protected buffer and the number of bytes used in it
-type bufferInfo struct {
-	buffer *memguard.LockedBuffer
-	used   int
-}
-
 // ParseVaultSecretRef parses a "vault:secret" or just "secret" reference
 // Returns (vaultName, secretName, isQualified)
 // If no vault is specified, returns empty vaultName and isQualified=false
@@ -75,6 +80,12 @@ func newAddCmd() *cobra.Command {
 
 			cli.cmd = cmd // Set the command for stdin access
 
+			// Before readSecretEnv, which makes the first locked allocation
+			err = checkLockedMemoryLimit(0)
+			if err != nil {
+				return err
+			}
+
 			destroySecrets := cli.readSecretEnv()
 			defer destroySecrets()
 
@@ -174,6 +185,12 @@ func newImportCmd() *cobra.Command {
 				return fmt.Errorf("failed to initialize CLI: %w", err)
 			}
 
+			// Before readSecretEnv, which makes the first locked allocation
+			err = checkLockedMemoryLimit(0)
+			if err != nil {
+				return err
+			}
+
 			destroySecrets := cli.readSecretEnv()
 			defer destroySecrets()
 
@@ -270,88 +287,105 @@ The source secret is deleted after successful copy.`,
 	return cmd
 }
 
-// updateBufferSize updates the buffer size based on usage pattern
+// checkLockedMemoryLimit returns an error naming the locked-memory limit
-func updateBufferSize(currentSize int, sameSize *int) int {
+// (`ulimit -l`) when a value of size bytes does not fit in it with
-	const (
+// lockedMemoryReservePages. memguard, which holds secret values in locked
-		doubleAfterBuffers = 2
+// memory, crashes or hangs the process when it cannot lock more, so a
-		growthFactor       = 2
+// command calls this with size 0 before its first locked allocation, and
-	)
+// again with the value's size before a buffer for the value is allocated.
+func checkLockedMemoryLimit(size int) error {
+	var limit unix.Rlimit
 
-	*sameSize++
+	err := unix.Getrlimit(unix.RLIMIT_MEMLOCK, &limit)
-	if *sameSize >= doubleAfterBuffers {
+	if err != nil {
-		*sameSize = 0
+		return fmt.Errorf("failed to read the locked-memory limit: %w", err)
-
-		return currentSize * growthFactor
 	}
 
-	return currentSize
+	// memguard locks whole pages, so the value is counted in whole pages.
-}
+	pageSize := os.Getpagesize()
+	valuePages := (size + pageSize - 1) / pageSize
 
-// destroyBuffers destroys every buffer in the list
+	//nolint:gosec // G115: a page count and a page size are never negative
-func destroyBuffers(buffers []bufferInfo) {
+	needed := uint64((valuePages + lockedMemoryReservePages) * pageSize)
-	for _, b := range buffers {
+	if needed > limit.Cur {
-		b.buffer.Destroy()
+		return fmt.Errorf("%w of %d KiB (ulimit -l)",
+			errLockedMemoryLimit, limit.Cur/kibibyte)
 	}
+
+	return nil
 }
 
-// readSecretFromReader reads all data from reader into protected buffers,
+// readSecretFromReader reads all of reader into a new locked buffer, which
-// enforcing the maximum secret size. On failure the accumulated buffers
+// the caller must destroy, leaving out one trailing newline when trimNewline
-// are destroyed; on success the caller must destroy them.
+// is set. It reads a page at a time into a memguard.Stream, which keeps what
-func readSecretFromReader(reader io.Reader) ([]bufferInfo, int, error) {
+// it is given encrypted, so that the value is in locked memory only once, in
-	var buffers []bufferInfo
+// the buffer returned, and both size checks come before that buffer is
+// allocated. It takes the value back out with Stream.Next, not Stream.Read:
+// Read puts back the part of a piece that does not fit, and memguard hangs
+// when it runs out of locked memory doing that.
+func readSecretFromReader(
+	reader io.Reader, trimNewline bool,
+) (*memguard.LockedBuffer, error) {
+	stream := memguard.NewStream()
 
-	totalSize := 0
+	chunk := memguard.NewBuffer(os.Getpagesize())
-	currentBufferSize := initialBufferSize
+	defer chunk.Destroy()
-	sameSize := 0
+
+	size := 0
+	endsWithNewline := false
 
 	for {
-		// Create a new buffer
+		n, err := io.ReadFull(reader, chunk.Bytes())
-		buffer := memguard.NewBuffer(currentBufferSize)
+		if n > 0 {
-		n, err := io.ReadFull(reader, buffer.Bytes())
+			size += n
-
+			if size > maxSecretSize {
-		if n == 0 {
+				return nil, errSecretTooLarge
-			// No data read, destroy the unused buffer
-			buffer.Destroy()
-		} else {
-			buffers = append(buffers, bufferInfo{buffer: buffer, used: n})
-			totalSize += n
-
-			if totalSize > maxSecretSize {
-				destroyBuffers(buffers)
-
-				return nil, 0, errSecretTooLarge
 			}
 
-			// If we filled the buffer, consider growing for next iteration
+			endsWithNewline = chunk.Bytes()[n-1] == '\n'
-			if n == currentBufferSize {
+
-				currentBufferSize = updateBufferSize(currentBufferSize, &sameSize)
+			// Write encrypts the bytes and wipes them from chunk.
-			}
+			_, _ = stream.Write(chunk.Bytes()[:n])
 		}
 
-		if err == io.EOF || errors.Is(err, io.ErrUnexpectedEOF) {
+		if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) {
 			break
-		} else if err != nil {
+		}
-			destroyBuffers(buffers)
 
-			return nil, 0, err
+		if err != nil {
+			return nil, err
 		}
 	}
 
-	return buffers, totalSize, nil
+	// Freed before the value's buffer is allocated, so that the two are
-}
+	// never locked at once.
+	chunk.Destroy()
 
-// combineBuffers copies the used portions of buffers into a single
+	if trimNewline && endsWithNewline {
-// protected buffer of totalSize bytes
+		size--
-func combineBuffers(buffers []bufferInfo, totalSize int) *memguard.LockedBuffer {
-	valueBuffer := memguard.NewBuffer(totalSize)
-
-	offset := 0
-	for _, b := range buffers {
-		copy(valueBuffer.Bytes()[offset:], b.buffer.Bytes()[:b.used])
-		offset += b.used
 	}
 
-	return valueBuffer
+	err := checkLockedMemoryLimit(size)
+	if err != nil {
+		return nil, err
+	}
+
+	value := memguard.NewBuffer(size)
+
+	// copy stops at the value's size, so a trimmed newline is not copied.
+	for offset := 0; offset < size; {
+		decrypted, err := stream.Next()
+		if err != nil {
+			value.Destroy()
+
+			return nil, fmt.Errorf("failed to decrypt the value read: %w", err)
+		}
+
+		offset += copy(value.Bytes()[offset:], decrypted.Bytes())
+		decrypted.Destroy()
+	}
+
+	return value, nil
 }
 
 // AddSecret adds a secret to the current vault
@@ -370,35 +404,21 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 
 	secret.Debug("Got current vault", "vault_name", vlt.GetName())
 
-	// Read secret value directly into protected buffers
+	// Read the secret value, less a trailing newline, into locked memory
-	secret.Debug("Reading secret value from stdin into protected buffers")
+	secret.Debug("Reading secret value from stdin into a protected buffer")
 
-	buffers, totalSize, err := readSecretFromReader(cli.cmd.InOrStdin())
+	valueBuffer, err := readSecretFromReader(cli.cmd.InOrStdin(), true)
 	if err != nil {
-		if errors.Is(err, errSecretTooLarge) {
+		if errors.Is(err, errSecretTooLarge) || errors.Is(err, errLockedMemoryLimit) {
 			return err
 		}
 
 		return fmt.Errorf("failed to read secret value: %w", err)
 	}
-	defer destroyBuffers(buffers)
-
-	// Check for trailing newline in the last buffer
-	if len(buffers) > 0 && totalSize > 0 {
-		lastBuffer := &buffers[len(buffers)-1]
-		if lastBuffer.buffer.Bytes()[lastBuffer.used-1] == '\n' {
-			lastBuffer.used--
-			totalSize--
-		}
-	}
-
-	secret.Debug("Read secret value from stdin",
-		"value_length", totalSize, "buffers", len(buffers))
-
-	// Combine all buffers into a single protected buffer
-	valueBuffer := combineBuffers(buffers, totalSize)
 	defer valueBuffer.Destroy()
 
+	secret.Debug("Read secret value from stdin", "value_length", valueBuffer.Size())
+
 	// Locked only now that stdin has been read: in `secret encrypt key |
 	// secret add name`, holding the lock while reading would leave each
 	// command waiting for the other.
@@ -667,7 +687,7 @@ func (cli *Instance) ImportSecret(
 
 	vlt.Mnemonic, vlt.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 
-	// Read secret value from the source file into protected buffers
+	// Read secret value from the source file into a protected buffer
 	file, err := cli.fs.Open(sourceFile)
 	if err != nil {
 		return fmt.Errorf("failed to open file %s: %w", sourceFile, err)
@@ -679,14 +699,10 @@ func (cli *Instance) ImportSecret(
 		}
 	}()
 
-	buffers, totalSize, err := readSecretFromReader(file)
+	valueBuffer, err := readSecretFromReader(file, false)
 	if err != nil {
 		return fmt.Errorf("failed to read secret from file %s: %w", sourceFile, err)
 	}
-	defer destroyBuffers(buffers)
-
-	// Combine all buffers into a single protected buffer
-	valueBuffer := combineBuffers(buffers, totalSize)
 	defer valueBuffer.Destroy()
 
 	// Locked only now that the file has been read, as in AddSecret: the

internal/cli/secrets_size_test.go

@@ -3,17 +3,23 @@ package cli
 
 import (
 	"bytes"
+	"context"
 	"crypto/rand"
 	"fmt"
 	"io"
+	"os"
+	"os/exec"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
 	"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"
 )
@@ -208,23 +214,25 @@ func TestImportSecretVariousSizes(t *testing.T) {
 	}
 }
 
-// TestAddSecretBufferGrowth tests that our buffer growth strategy works correctly
+// TestAddSecretAroundPageSize tests that secret add, which reads its input
+// one memory page (usually 4 KiB) at a time, stores values just under, at and
+// just over one and two pages unchanged.
 //
 //nolint:paralleltest // in parallel, size tests could exceed the memlock limit
-func TestAddSecretBufferGrowth(t *testing.T) {
+func TestAddSecretAroundPageSize(t *testing.T) {
-	// Test various sizes that should trigger buffer growth
+	// Sizes around one and two pages of 4 KiB, then larger ones up to 1 MiB
 	sizes := []int{
 		1,       // Single byte
 		100,     // Small
-		4095,    // Just under initial 4KB
+		4095,    // Just under 4KB
 		4096,    // Exactly 4KB
 		4097,    // Just over 4KB
-		8191,    // Just under 8KB (first double)
+		8191,    // Just under 8KB
 		8192,    // Exactly 8KB
 		8193,    // Just over 8KB
-		12288,   // 12KB (should trigger second double)
+		12288,   // 12KB
 		16384,   // 16KB
-		32768,   // 32KB (after more doublings)
+		32768,   // 32KB
 		65536,   // 64KB
 		131072,  // 128KB
 		524288,  // 512KB
@@ -315,6 +323,144 @@ func TestAddSecretStreamingBehavior(t *testing.T) {
 		"Retrieved secret should match original")
 }
 
+// newBinaryTestStateDir returns a new state directory holding a vault made
+// from the test mnemonic, for the secret binary to use.
+func newBinaryTestStateDir(t *testing.T) string {
+	t.Helper()
+
+	stateDir := t.TempDir()
+	_, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	return stateDir
+}
+
+// runAtLockedMemoryLimit runs the secret binary with args, stdin, the state
+// directory stateDir and the test mnemonic, its locked-memory limit (ulimit
+// -l) lowered to limitKiB, and returns what it printed.
+func runAtLockedMemoryLimit(
+	t *testing.T, stateDir string, limitKiB int, stdin io.Reader,
+	args ...string,
+) (string, error) {
+	t.Helper()
+
+	const commandWait = 10 * time.Second
+
+	secretPath, err := filepath.Abs("../../secret")
+	require.NoError(t, err)
+
+	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
+	defer cancel()
+
+	// The shell lowers the limit for itself and the binary only.
+	script := fmt.Sprintf(`ulimit -l %d && exec "$0" "$@"`, limitKiB)
+
+	//nolint:gosec // G204: runs the freshly built secret binary
+	cmd := exec.CommandContext(ctx, "sh",
+		append([]string{"-c", script, secretPath}, args...)...)
+	cmd.Env = []string{
+		secret.EnvStateDir + "=" + stateDir,
+		secret.EnvMnemonic + "=" + testMnemonic,
+	}
+	cmd.Stdin = stdin
+
+	output, err := cmd.CombinedOutput()
+
+	return string(output), err
+}
+
+// requireRefusedAtLockedMemoryLimit runs the secret binary as
+// runAtLockedMemoryLimit does, and requires it to exit with status 1 and
+// print wantError, with no panic.
+func requireRefusedAtLockedMemoryLimit(
+	t *testing.T, stateDir string, limitKiB int, wantError string,
+	stdin io.Reader, args ...string,
+) {
+	t.Helper()
+
+	output, err := runAtLockedMemoryLimit(t, stateDir, limitKiB, stdin, args...)
+
+	var exitErr *exec.ExitError
+
+	require.ErrorAs(t, err, &exitErr, "%v: %s", args, output)
+	assert.Equal(t, 1, exitErr.ExitCode(), "%v: %s", args, output)
+	assert.Contains(t, output, wantError, args)
+	assert.NotContains(t, output, "panic", args)
+}
+
+// TestSecretsAtLockedMemoryLimit runs the secret binary with the
+// locked-memory limit (ulimit -l) lowered to 1 MiB, where memguard would crash
+// the process with a panic instead of returning an error. add must store the
+// largest value that fits, which takes holding it in locked memory only once.
+// add, import and generate secret must refuse a value one byte larger with
+// exit status 1 and an error saying why.
+func TestSecretsAtLockedMemoryLimit(t *testing.T) {
+	t.Parallel()
+
+	const limitKiB = 1024
+
+	fits := limitKiB*kibibyte - lockedMemoryReservePages*os.Getpagesize()
+	limitError := fmt.Sprintf("locked-memory limit of %d KiB", limitKiB)
+	stateDir := newBinaryTestStateDir(t)
+
+	tooBigFile := filepath.Join(t.TempDir(), "too-big")
+	require.NoError(t, os.WriteFile(tooBigFile, make([]byte, fits+1), 0o600))
+
+	output, err := runAtLockedMemoryLimit(t, stateDir, limitKiB,
+		bytes.NewReader(make([]byte, fits)), "add", "fits")
+	require.NoError(t, err, "the largest value that fits: %s", output)
+
+	requireRefusedAtLockedMemoryLimit(t, stateDir, limitKiB, limitError,
+		bytes.NewReader(make([]byte, fits+1)), "add", "too-big")
+	requireRefusedAtLockedMemoryLimit(t, stateDir, limitKiB, limitError,
+		nil, "import", "too-big", "--source", tooBigFile)
+	requireRefusedAtLockedMemoryLimit(t, stateDir, limitKiB, limitError,
+		nil, "generate", "secret", "too-big", "--length", strconv.Itoa(fits+1))
+}
+
+// TestSecretsUnderSmallLockedMemoryLimits runs add, import and generate
+// secret with a short value under two small locked-memory limits (ulimit
+// -l). Under 20 KiB, too little for the buffers they need besides the value,
+// each must refuse it with exit status 1 and an error naming the limit, where
+// memguard would panic or hang. Under 64 KiB, the default of older Linux
+// kernels, each must store it.
+func TestSecretsUnderSmallLockedMemoryLimits(t *testing.T) {
+	t.Parallel()
+
+	const (
+		tooSmallLimitKiB   = 20
+		oldDefaultLimitKiB = 64
+		shortValue         = "hunter2"
+	)
+
+	tooSmallLimitError := fmt.Sprintf("locked-memory limit of %d KiB",
+		tooSmallLimitKiB)
+	stateDir := newBinaryTestStateDir(t)
+
+	shortFile := filepath.Join(t.TempDir(), "short")
+	require.NoError(t, os.WriteFile(shortFile, []byte(shortValue), 0o600))
+
+	requireRefusedAtLockedMemoryLimit(t, stateDir, tooSmallLimitKiB,
+		tooSmallLimitError, strings.NewReader(shortValue), "add", "added")
+	requireRefusedAtLockedMemoryLimit(t, stateDir, tooSmallLimitKiB,
+		tooSmallLimitError, nil, "import", "imported", "--source", shortFile)
+	requireRefusedAtLockedMemoryLimit(t, stateDir, tooSmallLimitKiB,
+		tooSmallLimitError, nil, "generate", "secret", "generated")
+
+	output, err := runAtLockedMemoryLimit(t, stateDir, oldDefaultLimitKiB,
+		strings.NewReader(shortValue), "add", "added")
+	require.NoError(t, err, "add: %s", output)
+
+	output, err = runAtLockedMemoryLimit(t, stateDir, oldDefaultLimitKiB,
+		nil, "import", "imported", "--source", shortFile)
+	require.NoError(t, err, "import: %s", output)
+
+	output, err = runAtLockedMemoryLimit(t, stateDir, oldDefaultLimitKiB,
+		nil, "generate", "secret", "generated")
+	require.NoError(t, err, "generate secret: %s", output)
+}
+
 // slowReader simulates a reader that returns data in small chunks
 type slowReader struct {
 	data      []byte

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 {
+		if errors.Is(err, os.ErrNotExist) {
-			// Skip vaults without metadata
+			// 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
+			return 0, fmt.Errorf("failed to parse vault metadata %s: %w", metadataPath, err)
-			continue
 		}
 
 		// 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 leaving no new vault, 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()