Say why a mnemonic is refused (closes #76)

Every way a mnemonic could be invalid was reported as a checksum
failure, including an empty one. It is now refused as empty, as having
a word count other than 12, 15, 18, 21 or 24, as having a word that is
not on the BIP-39 word list, or as failing the checksum. A word not on
the list is named by its position, never by the word itself.

Model: opus-5-5

README.md

@@ -126,6 +126,11 @@ the list is named by its position, never printed. Keys are derived from the
 mnemonic's words joined by single spaces, whatever whitespace is around or
 between them, so one word per line, tabs or extra spaces give the same keys.
 
+The key index, `--words`, the `--to` recipients and the file `age encrypt` or
+`age decrypt` is given to read are checked before the mnemonic is looked for, so
+a run refused for one of them never prompts for the mnemonic or runs the
+mnemonic command.
+
 `KEYFUNC_MNEMONIC` and `KEYFUNC_MNEMONIC_COMMAND` are removed from the
 environment before the mnemonic command, the system `ssh` (`keyfunc ssh to`) and
 `sftp` (`keyfunc ssh install`) are started, so the mnemonic is never handed on
@@ -222,11 +227,15 @@ first, or pass `-o StrictHostKeyChecking=accept-new` after `--`.
 
 Derives the key, serves it from an SSH agent that runs inside the tool on a unix
 socket in a new private `0700` temporary directory, then runs the system `ssh`
-with `-o IdentityAgent=<that socket>` followed by the host and all remaining
-arguments unchanged. The tool exits with `ssh`'s exit status and removes the
-socket and directory on the way out. The private key is never written to disk. A
-SIGINT, SIGTERM or SIGHUP ends `ssh` and still removes the socket and directory,
-and the tool then exits with status 1 unless `ssh` reported one of its own.
+with `-o IdentityAgent=<that socket> -o IdentitiesOnly=no` followed by the host
+and all remaining arguments unchanged. Without `IdentitiesOnly=no`, an
+`IdentitiesOnly yes` in the user's `ssh` configuration keeps the derived key
+from being offered. The agent holds no other key, so turning it off adds nothing
+else to what `ssh` offers. The tool exits with `ssh`'s exit status and removes
+the socket and directory on the way out. The private key is never written to
+disk. A SIGINT, SIGTERM or SIGHUP ends `ssh` and still removes the socket and
+directory, and the tool then exits with status 1 unless `ssh` reported one of
+its own.
 
 ## age identities: `keyfunc age`
 

internal/agekey/agekey.go

@@ -94,15 +94,12 @@ func (k *Key) Identity() string {
 }
 
 // Encrypt copies src to dst, encrypted to this key and to every extra
-// recipient named, so the same mnemonic can always read it back again.
+// recipient given, so the same mnemonic can always read it back again.
 // Armored output is the text form age also reads.
 func (k *Key) Encrypt(
-	dst io.Writer, src io.Reader, to []string, armored bool,
+	dst io.Writer, src io.Reader, to []age.Recipient, armored bool,
 ) error {
-	all, err := recipients(k.identity.Recipient(), to)
-	if err != nil {
-		return err
-	}
+	all := append([]age.Recipient{k.identity.Recipient()}, to...)
 
 	out := dst
 
@@ -113,7 +110,7 @@ func (k *Key) Encrypt(
 		out = text
 	}
 
-	err = encrypt(out, src, all)
+	err := encrypt(out, src, all)
 	if err != nil {
 		return err
 	}
@@ -130,12 +127,12 @@ func (k *Key) Encrypt(
 	return nil
 }
 
-// recipients returns the key's own recipient followed by the ones
-// named on the command line, so what the key encrypts it can read.
-func recipients(mine age.Recipient, to []string) ([]age.Recipient, error) {
-	list := []age.Recipient{mine}
+// Recipients reads the recipients named on the command line, for
+// Encrypt to add to the key's own.
+func Recipients(names []string) ([]age.Recipient, error) {
+	list := make([]age.Recipient, 0, len(names))
 
-	for _, name := range to {
+	for _, name := range names {
 		parsed, err := age.ParseX25519Recipient(name)
 		if err != nil {
 			return nil, fmt.Errorf("reading recipient %q: %w", name, err)

internal/agekey/agekey_test.go

@@ -119,12 +119,12 @@ func TestAnExtraRecipientCanReadItTooAndSoCanTheDerivedOne(t *testing.T) {
 
 	mine, theirs := forIndex(t, 0), forIndex(t, 1)
 
+	to, err := agekey.Recipients([]string{theirs.Recipient()})
+	require.NoError(t, err)
+
 	var sealed bytes.Buffer
 
-	err := mine.Encrypt(
-		&sealed, strings.NewReader("hello"),
-		[]string{theirs.Recipient()}, false,
-	)
+	err = mine.Encrypt(&sealed, strings.NewReader("hello"), to, false)
 	require.NoError(t, err)
 
 	for _, key := range []*agekey.Key{mine, theirs} {
@@ -138,10 +138,7 @@ func TestAnExtraRecipientCanReadItTooAndSoCanTheDerivedOne(t *testing.T) {
 func TestARecipientThatIsNotOneIsRefused(t *testing.T) {
 	t.Parallel()
 
-	err := forIndex(t, 0).Encrypt(
-		&bytes.Buffer{}, strings.NewReader("hello"),
-		[]string{"not a recipient"}, false,
-	)
+	_, err := agekey.Recipients([]string{"not a recipient"})
 	require.Error(t, err)
 }
 

internal/childmnemonic/childmnemonic.go

@@ -29,6 +29,17 @@ const DefaultWords = twelve
 // ErrWordCount is returned for a length this tool does not offer.
 var ErrWordCount = errors.New("a child mnemonic has 12, 18 or 24 words")
 
+// CheckWords refuses a length this tool does not offer, so that the
+// command can turn it down before it looks for the mnemonic.
+func CheckWords(words uint32) error {
+	switch words {
+	case twelve, eighteen, twentyFour:
+		return nil
+	default:
+		return fmt.Errorf("%w, not %d", ErrWordCount, words)
+	}
+}
+
 // Derive returns the English child mnemonic of that many words at that
 // key index, taken from the master key with BIP-85's own mnemonic
 // application, number 39. The words come straight from the BIP-85
@@ -38,13 +49,12 @@ func Derive(
 	master *hdkeychain.ExtendedKey,
 	words, index uint32,
 ) (string, error) {
-	switch words {
-	case twelve, eighteen, twentyFour:
-	default:
-		return "", fmt.Errorf("%w, not %d", ErrWordCount, words)
+	err := CheckWords(words)
+	if err != nil {
+		return "", err
 	}
 
-	err := derive.CheckIndex(index)
+	err = derive.CheckIndex(index)
 	if err != nil {
 		return "", err
 	}

internal/cli/age/age.go

@@ -108,47 +108,25 @@ func decrypt() *cobra.Command {
 }
 
 // runEncrypt encrypts to the derived recipient and any others given.
+// The recipients are read and the input opened before the key is
+// derived, so that a run refused for either never asks for the
+// mnemonic.
 func runEncrypt(cmd *cobra.Command, args []string) error {
-	key, err := derived(cmd)
-	if err != nil {
-		return err
-	}
-
-	to, err := cmd.Flags().GetStringArray("to")
+	names, err := cmd.Flags().GetStringArray("to")
 	if err != nil {
 		return fmt.Errorf("reading the recipients: %w", err)
 	}
 
+	to, err := agekey.Recipients(names)
+	if err != nil {
+		return err
+	}
+
 	armored, err := cmd.Flags().GetBool("armor")
 	if err != nil {
 		return fmt.Errorf("reading the armor flag: %w", err)
 	}
 
-	return through(cmd, args, func(dst io.Writer, src io.Reader) error {
-		return key.Encrypt(dst, src, to, armored)
-	})
-}
-
-// runDecrypt decrypts with the derived identity.
-func runDecrypt(cmd *cobra.Command, args []string) error {
-	key, err := derived(cmd)
-	if err != nil {
-		return err
-	}
-
-	return through(cmd, args, key.Decrypt)
-}
-
-// through opens the input the arguments ask for and hands it to the
-// work, with the file --output names to write to, or the command's own
-// output when it names none or names the same file as that output, and
-// the command's own error output when it names the same file as that.
-// Those two are the streams the tool already has, so whatever they are
-// redirected to is written as the redirect says, never replaced.
-func through(
-	cmd *cobra.Command, args []string,
-	work func(io.Writer, io.Reader) error,
-) error {
 	src, closeSrc, err := input(cmd, args)
 	if err != nil {
 		return err
@@ -156,6 +134,44 @@ func through(
 
 	defer closeSrc()
 
+	key, err := derived(cmd)
+	if err != nil {
+		return err
+	}
+
+	return through(cmd, src, func(dst io.Writer, plain io.Reader) error {
+		return key.Encrypt(dst, plain, to, armored)
+	})
+}
+
+// runDecrypt decrypts with the derived identity. The input is opened
+// before the key is derived, as for runEncrypt.
+func runDecrypt(cmd *cobra.Command, args []string) error {
+	src, closeSrc, err := input(cmd, args)
+	if err != nil {
+		return err
+	}
+
+	defer closeSrc()
+
+	key, err := derived(cmd)
+	if err != nil {
+		return err
+	}
+
+	return through(cmd, src, key.Decrypt)
+}
+
+// through hands the input to the work, with the file --output names to
+// write to, or the command's own output when it names none or names the
+// same file as that output, and the command's own error output when it
+// names the same file as that. Those two are the streams the tool
+// already has, so whatever they are redirected to is written as the
+// redirect says, never replaced.
+func through(
+	cmd *cobra.Command, src io.Reader,
+	work func(io.Writer, io.Reader) error,
+) error {
 	name, err := cmd.Flags().GetString("output")
 	if err != nil {
 		return fmt.Errorf("reading the output file: %w", err)

internal/cli/age_test.go

@@ -17,6 +17,7 @@ import (
 	"sneak.berlin/go/keyfunc/internal/agekey"
 	"sneak.berlin/go/keyfunc/internal/cli"
 	"sneak.berlin/go/keyfunc/internal/cli/age"
+	"sneak.berlin/go/keyfunc/internal/derive"
 	"sneak.berlin/go/keyfunc/internal/mnemonic"
 )
 
@@ -69,6 +70,44 @@ func TestAnotherRecipientIsAddedAndTheDerivedOneStays(t *testing.T) {
 	)
 }
 
+// The mnemonic command fails in these runs, so the refusal of the index
+// shows that it was checked before the mnemonic was looked for.
+func TestAnAgeIndexWithNoHardenedChildIsRefused(t *testing.T) {
+	t.Setenv(mnemonic.CommandVariable, failingMnemonicCommand)
+
+	for _, command := range []string{"pub", "priv", "encrypt", "decrypt"} {
+		out, err := execute(t, "age", command, "-n", tooLarge)
+		require.ErrorIs(t, err, derive.ErrIndexTooLarge, command)
+		require.Empty(t, out, command)
+	}
+}
+
+// The mnemonic command fails in this run, so the refusal of the
+// recipient shows that it was read before the mnemonic was looked for.
+func TestAnEncryptionToARecipientThatIsNotOneIsRefused(t *testing.T) {
+	t.Setenv(mnemonic.CommandVariable, failingMnemonicCommand)
+
+	plain := written(t, "notes.txt", "the secret\n")
+
+	out, err := execute(t, "age", "encrypt", "--to", "bogus", plain)
+	require.ErrorContains(t, err, `reading recipient "bogus"`)
+	require.Empty(t, out)
+}
+
+// The mnemonic command fails in these runs, so the refusal of the input
+// file shows that it was opened before the mnemonic was looked for.
+func TestAnInputFileThatCannotBeOpenedIsRefused(t *testing.T) {
+	t.Setenv(mnemonic.CommandVariable, failingMnemonicCommand)
+
+	missing := filepath.Join(t.TempDir(), "missing")
+
+	for _, operation := range []string{"encrypt", "decrypt"} {
+		out, err := execute(t, "age", operation, missing)
+		require.ErrorIs(t, err, fs.ErrNotExist, operation)
+		require.Empty(t, out, operation)
+	}
+}
+
 func TestAFileForAnotherKeyIsRefused(t *testing.T) {
 	t.Setenv(mnemonic.Variable, example())
 

internal/cli/cli_test.go

@@ -34,6 +34,14 @@ const (
 	twentyFour = 24
 )
 
+// tooLarge is the smallest key index with no hardened child.
+const tooLarge = "2147483648"
+
+// failingMnemonicCommand fails loudly, so a run that looks for the
+// mnemonic ends with that failure. A run that ends with the refusal of
+// an argument instead shows that the argument was checked first.
+const failingMnemonicCommand = "echo the mnemonic was looked for >&2; exit 1"
+
 // example returns the mnemonic the README gives its test vectors for:
 // eleven abandons and about.
 func example() string {
@@ -99,12 +107,25 @@ func TestThePrivateKeyMatchesThePublicOne(t *testing.T) {
 	require.Equal(t, vectorOne, back)
 }
 
+// The mnemonic command fails in these runs, so the refusal of the index
+// shows that it was checked before the mnemonic was looked for. The age
+// commands have a test of their own.
 func TestAnIndexWithNoHardenedChildIsRefused(t *testing.T) {
-	t.Setenv(mnemonic.Variable, example())
+	t.Setenv(mnemonic.CommandVariable, failingMnemonicCommand)
 
-	out, err := execute(t, "ssh", "pub", "-n", "2147483648")
-	require.ErrorIs(t, err, derive.ErrIndexTooLarge)
-	require.Empty(t, out)
+	for _, args := range [][]string{
+		{subcommand, "pub", "-n", tooLarge},
+		{subcommand, "priv", "-n", tooLarge},
+		{subcommand, installing, "-n", tooLarge, host},
+		{subcommand, "to", "-n", tooLarge, host},
+		{"mnemonic", "-n", tooLarge},
+	} {
+		name := strings.Join(args, " ")
+
+		out, err := execute(t, args...)
+		require.ErrorIs(t, err, derive.ErrIndexTooLarge, name)
+		require.Empty(t, out, name)
+	}
 }
 
 func TestAnIndexWithALeadingZeroIsDecimal(t *testing.T) {
@@ -181,8 +202,10 @@ func TestAChildMnemonicIsPrinted(t *testing.T) {
 	require.NotEqual(t, short, next)
 }
 
+// The mnemonic command fails in this run, so the refusal of the length
+// shows that it was checked before the mnemonic was looked for.
 func TestALengthTheToolDoesNotOfferIsRefused(t *testing.T) {
-	t.Setenv(mnemonic.Variable, example())
+	t.Setenv(mnemonic.CommandVariable, failingMnemonicCommand)
 
 	out, err := execute(t, "mnemonic", "--words", "15")
 	require.ErrorIs(t, err, childmnemonic.ErrWordCount)

internal/cli/mnemonic/mnemonic.go

@@ -52,6 +52,11 @@ func derived(cmd *cobra.Command) (string, error) {
 		return "", fmt.Errorf("reading the word count: %w", err)
 	}
 
+	err = childmnemonic.CheckWords(words)
+	if err != nil {
+		return "", err
+	}
+
 	parent, err := options.Mnemonic(cmd)
 	if err != nil {
 		return "", err

internal/cli/options/options.go

@@ -7,6 +7,7 @@ import (
 	"strconv"
 
 	"github.com/spf13/cobra"
+	"sneak.berlin/go/keyfunc/internal/derive"
 	"sneak.berlin/go/keyfunc/internal/mnemonic"
 )
 
@@ -23,13 +24,20 @@ func Add(cmd *cobra.Command) {
 	)
 }
 
-// Index returns the key index the user asked for.
+// Index returns the key index the user asked for, and refuses one above
+// derive.MaxIndex. Every command calls it before it looks for the
+// mnemonic.
 func Index(cmd *cobra.Command) (uint32, error) {
 	index, err := cmd.Flags().GetUint32("index")
 	if err != nil {
 		return 0, fmt.Errorf("reading the index: %w", err)
 	}
 
+	err = derive.CheckIndex(index)
+	if err != nil {
+		return 0, err
+	}
+
 	return index, nil
 }
 

internal/cli/ssh/to.go

@@ -58,8 +58,13 @@ func to() *cobra.Command {
 
 			defer served.Stop()
 
+			// IdentitiesOnly yes in the user's ssh configuration
+			// would keep the agent's key from being offered. The
+			// agent holds only the derived key, so turning it off
+			// adds no other key.
 			argv := slices.Concat([]string{
 				"-o", "IdentityAgent=" + served.Socket(),
+				"-o", "IdentitiesOnly=no",
 			}, args)
 
 			return connect(ctx, argv)

internal/cli/ssh_test.go

@@ -587,7 +587,7 @@ func TestSSHIsPointedAtTheAgentAndItsStatusIsHandedOn(t *testing.T) {
 
 	given := recorded(t, arguments)
 	require.Equal(t, "-o", given[0])
-	require.Equal(t, []string{host, remoteCommand}, given[2:])
+	require.Equal(t, []string{host, remoteCommand}, given[4:])
 
 	// The stand-in wrote the path down only because there really was
 	// a socket there while it ran.
@@ -596,6 +596,26 @@ func TestSSHIsPointedAtTheAgentAndItsStatusIsHandedOn(t *testing.T) {
 	require.NoDirExists(t, filepath.Dir(socket))
 }
 
+func TestIdentitiesOnlyIsTurnedOffBeforeTheHost(t *testing.T) {
+	t.Setenv(mnemonic.Variable, example())
+
+	arguments, _ := pretendCall(t)
+
+	_, err := execute(t, subcommand, "to", host, remoteCommand)
+
+	var passed ssh.StatusError
+
+	require.ErrorAs(t, err, &passed)
+
+	// IdentitiesOnly yes in the user's ssh configuration keeps ssh
+	// from offering the agent's key. An option given before the host
+	// wins over the configuration.
+	given := recorded(t, arguments)
+	require.Equal(t,
+		[]string{"-o", "IdentitiesOnly=no", host, remoteCommand}, given[2:],
+	)
+}
+
 func TestTheToolEndsWithTheStatusSSHEndedWith(t *testing.T) {
 	t.Setenv(mnemonic.Variable, example())
 

internal/mnemonic/mnemonic_test.go

@@ -97,6 +97,27 @@ func TestAFailingCommandSaysWhatWentWrong(t *testing.T) {
 	require.Contains(t, err.Error(), "nothing here")
 }
 
+func TestValidMnemonicsOf15To24WordsAreAccepted(t *testing.T) {
+	t.Setenv(mnemonic.CommandVariable, "")
+
+	// abandon repeated, then the last word that makes the checksum
+	// match: 15, 18, 21 and 24 words.
+	valid := []string{
+		strings.Repeat("abandon ", 14) + "address",
+		strings.Repeat("abandon ", 17) + "agent",
+		strings.Repeat("abandon ", 20) + "admit",
+		strings.Repeat("abandon ", 23) + "art",
+	}
+
+	for _, words := range valid {
+		t.Setenv(mnemonic.Variable, words)
+
+		got, err := mnemonic.Read(t.Context(), "")
+		require.NoError(t, err)
+		require.Equal(t, words, got)
+	}
+}
+
 func TestABadChecksumIsRefused(t *testing.T) {
 	t.Setenv(mnemonic.CommandVariable, "")
 	t.Setenv(mnemonic.Variable, broken())