Ask for the new passphrase in unlocker add when the variable is set (closes #154)

With SB_UNLOCK_PASSPHRASE set, `secret unlocker add passphrase` gave the
new unlocker the passphrase in the variable without asking. It now asks
on the terminal for the new passphrase and uses the variable to unlock
the vault. With no controlling terminal the new unlocker takes the
variable's passphrase, and the output says so.

secret.ErrNoTerminal is exported so that the command can tell a missing
terminal from other failures to read, such as nothing typed or a
mismatched confirmation, which still fail the command.

The in-process tests that add a passphrase unlocker with the variable
set rely on the Docker test phase having no controlling terminal; at a
terminal they would ask.

Model: opus-5-5

README.md

@@ -204,6 +204,12 @@ Moves or renames a secret within the current vault.
 Lists all versions of a secret showing creation time, status, and validity
 period.
 
+`STATUS` is `current` for the secret's current version, `expired` for another
+version whose `NOT_AFTER` has passed, and `inactive` for any other version.
+Adding a version sets `NOT_AFTER` on the version that was current;
+`secret version promote` sets no times. A version that is not current therefore
+shows `inactive` when no version was added while it was current.
+
 #### `secret version promote <secret-name> <version>`
 
 Promotes a specific version to current by rewriting the secret's `current` file
@@ -261,6 +267,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,

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/unlockers.go

@@ -314,7 +314,8 @@ func (cli *Instance) UnlockersList(jsonOutput bool) error {
 		return err
 	}
 
-	var unlockers []UnlockerInfo
+	// Not nil, so that a vault with no unlockers prints [] in JSON, not null
+	unlockers := make([]UnlockerInfo, 0, len(unlockerMetadata))
 
 	for _, unlockerID := range slices.Sorted(maps.Keys(unlockerMetadata)) {
 		metadata := unlockerMetadata[unlockerID]
@@ -440,15 +441,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()
 	}
 
@@ -459,6 +465,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/unlockers_list_test.go

@@ -1,13 +1,14 @@
 // Unlocker List Tests
 //
-// Tests for `secret unlocker list` behavior when an unlocker's metadata
-// cannot be read or used:
+// Tests for `secret unlocker list`:
 //
 //   - TestUnlockersListToleratesCorruptMetadata: one unlocker's corrupt
 //     metadata does not stop the others from being listed.
 //   - TestUnlockersListSkipsUnreadableMetadata: an unlocker whose metadata
 //     file cannot be checked for or read is left out, and the other is
 //     still listed.
+//   - TestUnlockersListEmptyJSON: a vault with no unlockers lists them as
+//     an empty JSON list.
 
 //nolint:testpackage // white-box test of unexported internals
 package cli
@@ -321,3 +322,36 @@ func TestUnlockersListSkipsUnreadableMetadata(t *testing.T) {
 		})
 	}
 }
+
+// TestUnlockersListEmptyJSON is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/148: listing the unlockers of a
+// vault that has none, as after its last unlocker is removed, printed
+// "unlockers": null in JSON form instead of an empty list.
+func TestUnlockersListEmptyJSON(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	vaultDir := filepath.Join(listTestStateDir, "vaults.d", listTestVaultName)
+
+	require.NoError(t, afero.WriteFile(
+		fs, filepath.Join(listTestStateDir, "currentvault"),
+		[]byte(listTestVaultName), listTestFilePerm,
+	))
+	require.NoError(t, fs.MkdirAll(
+		filepath.Join(vaultDir, listTestUnlockersDirName), listTestDirPerm,
+	))
+
+	var buf bytes.Buffer
+
+	cmd := &cobra.Command{}
+	cmd.SetOut(&buf)
+
+	instance := &Instance{fs: fs, stateDir: listTestStateDir, cmd: cmd}
+	require.NoError(t, instance.UnlockersList(true))
+
+	var decoded map[string]any
+
+	require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded))
+	assert.Equal(t, []any{}, decoded["unlockers"],
+		"no unlockers must be printed as [], not %s", buf.String())
+}

internal/cli/vault_list_test.go

@@ -0,0 +1,42 @@
+package cli_test
+
+import (
+	"bytes"
+	"encoding/json"
+	"path/filepath"
+	"testing"
+
+	"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"
+)
+
+// TestVaultListEmptyJSON is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/148: listing the vaults when
+// vaults.d holds none printed "vaults": null in JSON form instead of an
+// empty list.
+func TestVaultListEmptyJSON(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	require.NoError(t, fs.MkdirAll(
+		filepath.Join(testStateDir, "vaults.d"), secret.DirPerms,
+	))
+
+	var buf bytes.Buffer
+
+	cmd := &cobra.Command{}
+	cmd.SetOut(&buf)
+
+	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+	require.NoError(t, c.ListVaults(cmd, true))
+
+	var decoded map[string]any
+
+	require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded))
+	assert.Equal(t, []any{}, decoded["vaults"],
+		"no vaults must be printed as [], not %s", buf.String())
+}

internal/cli/version.go

@@ -229,9 +229,13 @@ func printVersionRow(
 	}
 
 	// Determine status
-	status := "expired"
-	if version == currentVersion {
+	status := "inactive"
+
+	switch {
+	case version == currentVersion:
 		status = "current"
+	case sv.Metadata.NotAfter != nil && sv.Metadata.NotAfter.Before(time.Now()):
+		status = "expired"
 	}
 
 	// Format timestamps

internal/cli/version_test.go

@@ -5,6 +5,8 @@
 // - TestListVersionsCommand: Tests `secret version list` command output
 // - TestListVersionsNonExistentSecret: Tests error handling for missing secrets
 // - TestPromoteVersionCommand: Tests `secret version promote` command
+// - TestListVersionsStatusAfterPromote: Tests each version's status after
+//   promoting an older version
 // - TestPromoteNonExistentVersion: Tests error handling for invalid promotion
 // - TestPromoteHelpSaysCurrentFileIsRewritten: Tests `secret version promote
 //   --help` text
@@ -244,6 +246,66 @@ func TestPromoteVersionCommand(t *testing.T) {
 	assert.Equal(t, []byte("version-1"), promoted.Bytes())
 }
 
+func TestListVersionsStatusAfterPromote(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+	cli.Mnemonic = testMnemonicBuffer(t)
+
+	setupTestVault(t, fs)
+
+	vlt, err := vault.GetCurrentVault(fs, testStateDir)
+	require.NoError(t, err)
+
+	vlt.Mnemonic = cli.Mnemonic
+
+	addTestSecret(t, vlt, []byte("version-1"), false)
+	addTestSecret(t, vlt, []byte("version-2"), true)
+	addTestSecret(t, vlt, []byte("version-3"), true)
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+
+	// Newest first
+	versions, err := secret.ListVersions(fs,
+		filepath.Join(vaultDir, "secrets.d", "test%secret"))
+	require.NoError(t, err)
+	require.Len(t, versions, 3)
+
+	newest, middle, oldest := versions[0], versions[1], versions[2]
+
+	cmd := newRootCmd()
+
+	var buf bytes.Buffer
+	cmd.SetOut(&buf)
+	cmd.SetErr(&buf)
+
+	err = cli.PromoteVersion(cmd, "test/secret", oldest)
+	require.NoError(t, err)
+
+	buf.Reset()
+
+	err = cli.ListVersions(cmd, "test/secret")
+	require.NoError(t, err)
+
+	// STATUS is the fourth field of a row, since CREATED is a date and a time
+	statuses := map[string]string{}
+
+	for line := range strings.SplitSeq(buf.String(), "\n") {
+		fields := strings.Fields(line)
+		if len(fields) > 3 {
+			statuses[fields[0]] = fields[3]
+		}
+	}
+
+	assert.Equal(t, "current", statuses[oldest])
+	// Its NOT_AFTER was set when the newest version was added
+	assert.Equal(t, "expired", statuses[middle])
+	// It has no NOT_AFTER
+	assert.Equal(t, "inactive", statuses[newest])
+}
+
 func TestPromoteNonExistentVersion(t *testing.T) {
 	t.Parallel()
 

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/secret/version.go

@@ -32,7 +32,8 @@ type VersionMetadata struct {
 	ID        string     `json:"id"`                  // ULID
 	CreatedAt *time.Time `json:"createdAt,omitempty"` // When version was created
 	NotBefore *time.Time `json:"notBefore,omitempty"` // When this version becomes active
-	NotAfter  *time.Time `json:"notAfter,omitempty"`  // Expiry (nil = current)
+	// NotAfter is set when another version is added while this one is current
+	NotAfter *time.Time `json:"notAfter,omitempty"`
 }
 
 // Version represents a version of a secret

internal/vault/management.go

@@ -140,8 +140,9 @@ func ListVaults(fs afero.Fs, stateDir string) ([]string, error) {
 		return nil, fmt.Errorf("failed to read vaults directory: %w", err)
 	}
 
-	// Extract vault names
-	var vaults []string
+	// Extract vault names. Not nil, so that no vaults prints [] in JSON,
+	// not null.
+	vaults := make([]string, 0, len(entries))
 
 	for _, entry := range entries {
 		if entry.IsDir() {