Hold the encrypt and decrypt result in memory until it is written (closes #143)

With --output, secret encrypt and secret decrypt emptied that file before
reading any input, so a failed command left it empty, and encrypting a file
onto itself gave the encryption of nothing.

With --output they now hold the result in memory and open --output, with the
same call as before, only once encryption or decryption has succeeded. A
failure to encrypt or decrypt leaves --output untouched, and --input can be the
same file. A failure while writing --output, such as a full disk, can leave it
empty or partly written; an error from closing it is reported. Without
--output the result still goes to stdout as it is produced.

Model: opus-5-5

README.md

@@ -271,6 +271,12 @@ Creates a new unlocker of the specified type:
 A vault has one passphrase unlocker: adding one replaces the one the vault has,
 which is removed only once the new one is the current unlocker.
 
+`secret unlocker add passphrase` asks on the terminal for the new passphrase
+even when `SB_UNLOCK_PASSPHRASE` is set, and uses the variable to unlock the
+vault. With no controlling terminal, as under cron or in a CI job, the new
+unlocker takes its passphrase from `SB_UNLOCK_PASSPHRASE`, and the command says
+so.
+
 #### `secret unlocker remove <unlocker-id> [--force]` / `secret unlocker rm` ⚠️ 🛑
 
 **DANGER**: Permanently removes an unlocker. It first asks for confirmation,
@@ -308,6 +314,12 @@ name. The file's bytes are stored unchanged, a final newline included.
 
 ### Encryption Operations
 
+With `--output`, `secret encrypt` and `secret decrypt` hold the result in memory
+and write it to `--output` only once encryption or decryption has succeeded, so
+a failure to encrypt or decrypt leaves `--output` untouched. A failure while
+writing `--output`, such as a full disk, can leave it empty or partly written.
+The command needs about as much memory as the result.
+
 #### `secret encrypt <secret-name> [--input=file] [--output=file]`
 
 Encrypts data using an Age key stored as a secret. If the secret doesn't exist,
@@ -317,11 +329,6 @@ generates a new Age key.
 
 Decrypts data using an Age key stored as a secret.
 
-With `--output`, both hold the result in memory and write it to `--output` only
-once encryption or decryption has succeeded, so a failure to encrypt or decrypt
-leaves `--output` untouched. A failure while writing `--output`, such as a full
-disk, can leave it empty or partly written.
-
 ## Storage Architecture
 
 ### Directory Structure
@@ -559,7 +566,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 (`secret init` and
+  `secret vault create`) 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/create_vault_test.go

@@ -144,25 +144,20 @@ func TestVaultCreationLeavesNoSecretInEnvironment(t *testing.T) {
 // TestNoVaultImportCommand is a regression test for
 // https://git.eeqj.de/sneak/secret/issues/139: `secret vault import`
 // refused every vault that `secret vault create` makes, because each has a
-// long-term key. The command is gone, so cobra answers it with the help of
-// `secret vault`, as it does any subcommand of `secret vault` it does not
-// know.
+// long-term key. The command is gone, so it fails as any subcommand of
+// `secret vault` that does not exist does.
 func TestNoVaultImportCommand(t *testing.T) {
 	t.Setenv(secret.EnvStateDir, t.TempDir())
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+	t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
 
-	run := func(args ...string) string {
-		t.Setenv(secret.EnvMnemonic, testMnemonic)
-		t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
+	_, err := cli.ExecuteCommandInProcess(
+		strings.Fields("vault create work"), "", nil)
+	require.NoError(t, err)
 
-		output, err := cli.ExecuteCommandInProcess(args, "", nil)
-		require.NoError(t, err, "%v", args)
-
-		return output
-	}
-
-	run("vault", "create", "work")
-
-	require.Equal(t, run("vault"), run("vault", "import", "work"))
+	_, err = cli.ExecuteCommandInProcess(
+		strings.Fields("vault import work"), "", nil)
+	require.EqualError(t, err, `unknown command "import" for "secret vault"`)
 }
 
 // TestStopAtPassphrasePromptLeavesNothing is a regression test for the

internal/cli/crypto_test.go

@@ -46,8 +46,8 @@ func newCryptoTest(t *testing.T) (*Instance, string) {
 // 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.
+// A failure to encrypt or decrypt 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()
 

internal/cli/generate.go

@@ -29,6 +29,12 @@ func newGenerateCmd() *cobra.Command {
 		Use:   "generate",
 		Short: "Generate random data",
 		Long:  `Generate various types of random data including mnemonics and secrets.`,
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newGenerateMnemonicCmd())

internal/cli/passphrase_prompt_test.go

@@ -138,6 +138,148 @@ func TestInitWritesNoBlankLineAfterMnemonic(t *testing.T) {
 		"stderr: %q", stderr.String())
 }
 
+// TestUnlockerAddAsksForNewPassphrase is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/154: with SB_UNLOCK_PASSPHRASE set,
+// `secret unlocker add passphrase` gave the new unlocker the passphrase in
+// the variable without asking. At a terminal it must ask for the new
+// passphrase there, twice, and unlock the vault with the variable, so that
+// `secret get` then opens the vault with the passphrase typed. pty.Open is
+// described above TestRemoveIgnoresTerminalOnStdout.
+func TestUnlockerAddAsksForNewPassphrase(t *testing.T) {
+	t.Parallel()
+
+	const newPassphrase = "new-passphrase"
+
+	stateDir := t.TempDir()
+
+	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
+	defer passphrase.Destroy()
+
+	vlt, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
+		testMnemonicBuffer(t), passphrase)
+	require.NoError(t, err)
+
+	value := memguard.NewBufferFromBytes([]byte("v1"))
+	defer value.Destroy()
+
+	require.NoError(t, vlt.AddSecret("s1", value, false))
+
+	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
+	defer cancel()
+
+	//nolint:gosec // G204: test executes the freshly built secret binary
+	cmd := exec.CommandContext(ctx, secretBinaryPath(t),
+		"unlocker", "add", "passphrase")
+	cmd.Env = []string{
+		secret.EnvStateDir + "=" + stateDir,
+		secret.EnvUnlockPassphrase + "=" + testPassphrase,
+		"PATH=" + os.Getenv("PATH"),
+		"HOME=" + os.Getenv("HOME"),
+	}
+
+	ptmx, tty, err := pty.Open()
+	require.NoError(t, err)
+
+	defer func() { _ = ptmx.Close() }()
+
+	deadline, _ := ctx.Deadline()
+	require.NoError(t, ptmx.SetReadDeadline(deadline))
+
+	cmd.Stderr = tty
+	// Ctty is a descriptor in the new process: 2, its stderr.
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true, Setctty: true, Ctty: 2}
+
+	require.NoError(t, cmd.Start())
+
+	_ = tty.Close()
+
+	var (
+		shown []byte
+		char  byte
+	)
+
+	terminal := bufio.NewReader(ptmx)
+
+	for _, prompt := range []string{
+		"Enter passphrase for unlocker: ", "Confirm passphrase: ",
+	} {
+		for !bytes.HasSuffix(shown, []byte(prompt)) {
+			char, err = terminal.ReadByte()
+			require.NoError(t, err,
+				"secret unlocker add passphrase did not ask on the terminal: %s",
+				shown)
+
+			shown = append(shown, char)
+		}
+
+		_, err = ptmx.WriteString(newPassphrase + "\n")
+		require.NoError(t, err)
+	}
+
+	// The read ends once the command has exited and so closed the terminal.
+	rest, _ := io.ReadAll(terminal)
+
+	require.NoError(t, cmd.Wait(),
+		"secret unlocker add passphrase: %s%s", shown, rest)
+
+	// The binary, not this test process, opens the vault: the new unlocker
+	// has age's full scrypt work factor, which is slow under the race
+	// detector.
+	assert.Equal(t, "v1",
+		runWithoutTerminal(t, stateDir, newPassphrase, "get", "s1"))
+}
+
+// TestUnlockerAddWithoutTerminalSaysPassphraseIsVariable is a regression
+// test for https://git.eeqj.de/sneak/secret/issues/154: with no terminal to
+// ask on, `secret unlocker add passphrase` gives the new unlocker the
+// passphrase in SB_UNLOCK_PASSPHRASE, and its output must say so.
+func TestUnlockerAddWithoutTerminalSaysPassphraseIsVariable(t *testing.T) {
+	t.Parallel()
+
+	stateDir := t.TempDir()
+
+	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
+	defer passphrase.Destroy()
+
+	_, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
+		testMnemonicBuffer(t), passphrase)
+	require.NoError(t, err)
+
+	output := runWithoutTerminal(t, stateDir, testPassphrase,
+		"unlocker", "add", "passphrase")
+
+	assert.Contains(t, output, "The new unlocker takes its passphrase "+
+		"from SB_UNLOCK_PASSPHRASE, since there is no terminal to ask for a "+
+		"new one on\n")
+}
+
+// runWithoutTerminal runs the secret binary with args and SB_UNLOCK_PASSPHRASE
+// set to passphrase, in a new session, which has no terminal, and returns
+// what it printed once it has succeeded.
+func runWithoutTerminal(
+	t *testing.T, stateDir, passphrase string, args ...string,
+) string {
+	t.Helper()
+
+	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
+	defer cancel()
+
+	//nolint:gosec // G204: test executes the freshly built secret binary
+	cmd := exec.CommandContext(ctx, secretBinaryPath(t), args...)
+	cmd.Env = []string{
+		secret.EnvStateDir + "=" + stateDir,
+		secret.EnvUnlockPassphrase + "=" + passphrase,
+		"PATH=" + os.Getenv("PATH"),
+		"HOME=" + os.Getenv("HOME"),
+	}
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}
+
+	output, err := cmd.CombinedOutput()
+	require.NoError(t, err, "secret %v: %s", args, output)
+
+	return string(output)
+}
+
 // runAtTerminal runs the secret binary with args and input on stdin, in a
 // new session whose controlling terminal is a new terminal, which is also
 // its stderr: a command typed at a terminal with its input piped. pty.Open

internal/cli/unknown_subcommand_test.go

@@ -0,0 +1,58 @@
+package cli_test
+
+import (
+	"fmt"
+	"os/exec"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/secret"
+)
+
+// TestUnknownSubcommandFails is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/183: given a subcommand it does not
+// have, `secret vault`, `secret unlocker`, `secret version` and `secret
+// generate` printed their help and exited 0, so a script could not tell a
+// mistyped command from a successful one. Each runs here as the built binary
+// and must exit 1 naming the subcommand, as an unknown command does at the
+// top level, and must still print its help and exit 0 when given nothing.
+func TestUnknownSubcommandFails(t *testing.T) {
+	t.Parallel()
+
+	stateDir := t.TempDir()
+
+	// run runs the built binary with args and returns what it printed and
+	// its exit code
+	run := func(args ...string) (string, int) {
+		//nolint:gosec // G204: test executes the freshly built secret binary
+		cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
+		cmd.Env = []string{secret.EnvStateDir + "=" + stateDir}
+
+		output, err := cmd.CombinedOutput()
+		if err != nil {
+			var exitErr *exec.ExitError
+			require.ErrorAs(t, err, &exitErr, "secret %v did not run", args)
+		}
+
+		return string(output), cmd.ProcessState.ExitCode()
+	}
+
+	for _, call := range []string{
+		"vault improt", "unlocker foo", "version foo", "generate foo",
+	} {
+		parent, unknown, _ := strings.Cut(call, " ")
+
+		output, exitCode := run(parent, unknown)
+		assert.Equal(t, 1, exitCode, "secret %s should fail:\n%s", call, output)
+		assert.Contains(t, output, fmt.Sprintf(
+			"unknown command %q for \"secret %s\"", unknown, parent))
+
+		output, exitCode = run(parent)
+		assert.Equal(t, 0, exitCode,
+			"secret %s should succeed:\n%s", parent, output)
+		assert.Contains(t, output, usageHeading,
+			"secret %s should print its help", parent)
+	}
+}

internal/cli/unlockers.go

@@ -126,6 +126,12 @@ func newUnlockerCmd() *cobra.Command {
 		Use:   "unlocker",
 		Short: "Manage unlockers",
 		Long:  `Create, list, and remove unlockers for the current vault.`,
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newUnlockerListCmd())
@@ -441,15 +447,20 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error {
 	// long-term key
 	vlt.Mnemonic, vlt.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 
-	// The new unlocker gets the passphrase from the environment, which also
-	// unlocks the current passphrase unlocker, else the one entered here
-	passphraseBuffer := cli.UnlockPassphrase
-	if passphraseBuffer == nil {
-		// Use secure passphrase input with confirmation
-		passphraseBuffer, err = readSecurePassphrase("Enter passphrase for unlocker: ")
-		if err != nil {
-			return err
-		}
+	// The new passphrase is asked for even when SB_UNLOCK_PASSPHRASE is
+	// set, and the variable unlocks the vault. With no terminal to ask on,
+	// the new unlocker takes the passphrase in the variable.
+	fromVariable := false
+
+	passphraseBuffer, err := readSecurePassphrase("Enter passphrase for unlocker: ")
+
+	switch {
+	case errors.Is(err, secret.ErrNoTerminal) && cli.UnlockPassphrase != nil:
+		passphraseBuffer = cli.UnlockPassphrase
+		fromVariable = true
+	case err != nil:
+		return err
+	default:
 		defer passphraseBuffer.Destroy()
 	}
 
@@ -460,6 +471,12 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error {
 
 	cmd.Printf("Created passphrase unlocker: %s\n", passphraseUnlocker.GetID())
 
+	if fromVariable {
+		cmd.Printf("The new unlocker takes its passphrase from %s, since "+
+			"there is no terminal to ask for a new one on\n",
+			secret.EnvUnlockPassphrase)
+	}
+
 	// CreatePassphraseUnlocker has already made it the current unlocker
 	cmd.Printf("Automatically selected as current unlocker\n")
 

internal/cli/vault.go

@@ -29,6 +29,12 @@ func newVaultCmd() *cobra.Command {
 		Use:   "vault",
 		Short: "Manage vaults",
 		Long:  `Create, list, and select vaults for organizing secrets.`,
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newVaultListCmd())

internal/cli/version.go

@@ -43,6 +43,12 @@ func VersionCommands(cli *Instance) *cobra.Command {
 		Short: "Manage secret versions",
 		Long: "Commands for managing secret versions including listing, " +
 			"promoting, and retrieving specific versions",
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	// List versions command

internal/secret/crypto.go

@@ -15,10 +15,13 @@ import (
 
 var (
 	errNilPassphraseBuffer = errors.New("passphrase buffer is nil")
-	errNoTerminal          = errors.New("there is no terminal to ask on")
 	errNothingEntered      = errors.New("nothing was entered")
 )
 
+// ErrNoTerminal is wrapped in the error of ReadPassphrase and ReadMnemonic
+// when the process has no controlling terminal to ask on.
+var ErrNoTerminal = errors.New("there is no terminal to ask on")
+
 // ErrMnemonicNotRead is wrapped in every error of ReadMnemonic: there is no
 // terminal to read the mnemonic from, reading it failed, or it was empty.
 var ErrMnemonicNotRead = errors.New("failed to read mnemonic")
@@ -203,7 +206,7 @@ func readFromTerminal(
 
 		return nil, fmt.Errorf(
 			"%w: %w. Please set the %s environment variable or run interactively",
-			notRead, errNoTerminal, envVar)
+			notRead, ErrNoTerminal, envVar)
 	}
 	defer func() { _ = terminal.Close() }()
 

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 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()