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,17 +129,6 @@ 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.

internal/cli/generate.go

@@ -72,12 +72,6 @@ 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()
 
@@ -141,16 +135,10 @@ func (cli *Instance) GenerateSecret(
 		return errLengthTooSmall
 	}
 
-	if length > maxSecretSize {
+	var (
-		return errSecretTooLarge
+		secretValue string
-	}
+		err         error
-
+	)
-	err := checkLockedMemoryLimit(length)
-	if err != nil {
-		return err
-	}
-
-	var secretValue string
 
 	switch secretType {
 	case "base58":

internal/cli/root.go

@@ -14,8 +14,6 @@ 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
@@ -53,19 +51,6 @@ 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,7 +14,6 @@ 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"
 )
@@ -25,30 +24,26 @@ 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
@@ -80,12 +75,6 @@ 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()
 
@@ -185,12 +174,6 @@ 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()
 
@@ -287,105 +270,88 @@ The source secret is deleted after successful copy.`,
 	return cmd
 }
 
-// checkLockedMemoryLimit returns an error naming the locked-memory limit
+// updateBufferSize updates the buffer size based on usage pattern
-// (`ulimit -l`) when a value of size bytes does not fit in it with
+func updateBufferSize(currentSize int, sameSize *int) int {
-// lockedMemoryReservePages. memguard, which holds secret values in locked
+	const (
-// memory, crashes or hangs the process when it cannot lock more, so a
+		doubleAfterBuffers = 2
-// command calls this with size 0 before its first locked allocation, and
+		growthFactor       = 2
-// again with the value's size before a buffer for the value is allocated.
+	)
-func checkLockedMemoryLimit(size int) error {
-	var limit unix.Rlimit
 
-	err := unix.Getrlimit(unix.RLIMIT_MEMLOCK, &limit)
+	*sameSize++
-	if err != nil {
+	if *sameSize >= doubleAfterBuffers {
-		return fmt.Errorf("failed to read the locked-memory limit: %w", err)
+		*sameSize = 0
+
+		return currentSize * growthFactor
 	}
 
-	// memguard locks whole pages, so the value is counted in whole pages.
+	return currentSize
-	pageSize := os.Getpagesize()
-	valuePages := (size + pageSize - 1) / pageSize
-
-	//nolint:gosec // G115: a page count and a page size are never negative
-	needed := uint64((valuePages + lockedMemoryReservePages) * pageSize)
-	if needed > limit.Cur {
-		return fmt.Errorf("%w of %d KiB (ulimit -l)",
-			errLockedMemoryLimit, limit.Cur/kibibyte)
-	}
-
-	return nil
 }
 
-// readSecretFromReader reads all of reader into a new locked buffer, which
+// destroyBuffers destroys every buffer in the list
-// the caller must destroy, leaving out one trailing newline when trimNewline
+func destroyBuffers(buffers []bufferInfo) {
-// is set. It reads a page at a time into a memguard.Stream, which keeps what
+	for _, b := range buffers {
-// it is given encrypted, so that the value is in locked memory only once, in
+		b.buffer.Destroy()
-// 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()
 
-	chunk := memguard.NewBuffer(os.Getpagesize())
+// readSecretFromReader reads all data from reader into protected buffers,
-	defer chunk.Destroy()
+// enforcing the maximum secret size. On failure the accumulated buffers
+// are destroyed; on success the caller must destroy them.
+func readSecretFromReader(reader io.Reader) ([]bufferInfo, int, error) {
+	var buffers []bufferInfo
 
-	size := 0
+	totalSize := 0
-	endsWithNewline := false
+	currentBufferSize := initialBufferSize
+	sameSize := 0
 
 	for {
-		n, err := io.ReadFull(reader, chunk.Bytes())
+		// Create a new buffer
-		if n > 0 {
+		buffer := memguard.NewBuffer(currentBufferSize)
-			size += n
+		n, err := io.ReadFull(reader, buffer.Bytes())
-			if size > maxSecretSize {
+
-				return nil, errSecretTooLarge
+		if n == 0 {
+			// 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
 			}
 
-			endsWithNewline = chunk.Bytes()[n-1] == '\n'
+			// If we filled the buffer, consider growing for next iteration
-
+			if n == currentBufferSize {
-			// Write encrypts the bytes and wipes them from chunk.
+				currentBufferSize = updateBufferSize(currentBufferSize, &sameSize)
-			_, _ = stream.Write(chunk.Bytes()[:n])
+			}
 		}
 
-		if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) {
+		if err == io.EOF || errors.Is(err, io.ErrUnexpectedEOF) {
 			break
-		}
+		} else if err != nil {
+			destroyBuffers(buffers)
 
-		if err != nil {
+			return nil, 0, err
-			return nil, err
 		}
 	}
 
-	// Freed before the value's buffer is allocated, so that the two are
+	return buffers, totalSize, nil
-	// never locked at once.
+}
-	chunk.Destroy()
 
-	if trimNewline && endsWithNewline {
+// combineBuffers copies the used portions of buffers into a single
-		size--
+// protected buffer of totalSize bytes
+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
 	}
 
-	err := checkLockedMemoryLimit(size)
+	return valueBuffer
-	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
@@ -404,20 +370,34 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 
 	secret.Debug("Got current vault", "vault_name", vlt.GetName())
 
-	// Read the secret value, less a trailing newline, into locked memory
+	// Read secret value directly into protected buffers
-	secret.Debug("Reading secret value from stdin into a protected buffer")
+	secret.Debug("Reading secret value from stdin into protected buffers")
 
-	valueBuffer, err := readSecretFromReader(cli.cmd.InOrStdin(), true)
+	buffers, totalSize, err := readSecretFromReader(cli.cmd.InOrStdin())
 	if err != nil {
-		if errors.Is(err, errSecretTooLarge) || errors.Is(err, errLockedMemoryLimit) {
+		if errors.Is(err, errSecretTooLarge) {
 			return err
 		}
 
 		return fmt.Errorf("failed to read secret value: %w", err)
 	}
-	defer valueBuffer.Destroy()
+	defer destroyBuffers(buffers)
 
-	secret.Debug("Read secret value from stdin", "value_length", valueBuffer.Size())
+	// 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()
 
 	// Locked only now that stdin has been read: in `secret encrypt key |
 	// secret add name`, holding the lock while reading would leave each
@@ -687,7 +667,7 @@ func (cli *Instance) ImportSecret(
 
 	vlt.Mnemonic, vlt.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 
-	// Read secret value from the source file into a protected buffer
+	// Read secret value from the source file into protected buffers
 	file, err := cli.fs.Open(sourceFile)
 	if err != nil {
 		return fmt.Errorf("failed to open file %s: %w", sourceFile, err)
@@ -699,10 +679,14 @@ func (cli *Instance) ImportSecret(
 		}
 	}()
 
-	valueBuffer, err := readSecretFromReader(file, false)
+	buffers, totalSize, err := readSecretFromReader(file)
 	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,23 +3,17 @@ 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"
 )
@@ -214,25 +208,23 @@ func TestImportSecretVariousSizes(t *testing.T) {
 	}
 }
 
-// TestAddSecretAroundPageSize tests that secret add, which reads its input
+// TestAddSecretBufferGrowth tests that our buffer growth strategy works correctly
-// 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 TestAddSecretAroundPageSize(t *testing.T) {
+func TestAddSecretBufferGrowth(t *testing.T) {
-	// Sizes around one and two pages of 4 KiB, then larger ones up to 1 MiB
+	// Test various sizes that should trigger buffer growth
 	sizes := []int{
 		1,       // Single byte
 		100,     // Small
-		4095,    // Just under 4KB
+		4095,    // Just under initial 4KB
 		4096,    // Exactly 4KB
 		4097,    // Just over 4KB
-		8191,    // Just under 8KB
+		8191,    // Just under 8KB (first double)
 		8192,    // Exactly 8KB
 		8193,    // Just over 8KB
-		12288,   // 12KB
+		12288,   // 12KB (should trigger second double)
 		16384,   // 16KB
-		32768,   // 32KB
+		32768,   // 32KB (after more doublings)
 		65536,   // 64KB
 		131072,  // 128KB
 		524288,  // 512KB
@@ -323,144 +315,6 @@ 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