From f6d613727d24cfcf08896be04901905394161517 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 08:01:45 +0000 Subject: [PATCH] Keep the mnemonic out of the ssh and sftp children (closes #16) ssh to and ssh install started the system ssh and sftp with the tool's whole environment, so a mnemonic taken from KEYFUNC_MNEMONIC stayed in the child's environment for as long as it ran, readable by the same user and forwardable to the host through a SendEnv line. ssh to keeps the private key inside the tool; the mnemonic must not leave it either. A shared helper in the ssh cli package hands both children the tool's environment with KEYFUNC_MNEMONIC and KEYFUNC_MNEMONIC_COMMAND removed. The mnemonic command itself still runs with the full environment. The stand-in ssh and sftp in the tests now record their environment, and two tests show neither variable reaches them while another one does. Model: opus-4-8 --- README.md | 5 +++ internal/cli/ssh/install.go | 1 + internal/cli/ssh/ssh.go | 23 ++++++++++++ internal/cli/ssh/to.go | 1 + internal/cli/ssh_test.go | 75 +++++++++++++++++++++++++++++++++---- 5 files changed, 98 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index aa78dc6..975ecd7 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,11 @@ If none of these is available and standard input is not a terminal, the tool refuses and exits with status 1. A mnemonic that fails the BIP-39 checksum is refused with a message saying so. +`KEYFUNC_MNEMONIC` and `KEYFUNC_MNEMONIC_COMMAND` are removed from the +environment before the system `ssh` (`keyfunc ssh to`) and `sftp` +(`keyfunc ssh install`) are started, so the mnemonic is never handed on to +them. + Every command takes `--index` / `-n` and `--mnemonic-command`, and has `--help`. `keyfunc --version` prints the version. `make build` stamps it; a binary installed with `go install` reports the module version instead. diff --git a/internal/cli/ssh/install.go b/internal/cli/ssh/install.go index 0a6cb1a..761fe3c 100644 --- a/internal/cli/ssh/install.go +++ b/internal/cli/ssh/install.go @@ -166,6 +166,7 @@ func session( //nolint:gosec // the options are the user's own, meant for sftp command := exec.CommandContext(cmd.Context(), "sftp", argv...) + command.Env = childEnv() command.Stdin = strings.NewReader(strings.Join(batch, "\n") + "\n") command.Stdout = &said command.Stderr = &said diff --git a/internal/cli/ssh/ssh.go b/internal/cli/ssh/ssh.go index b15a08b..3df7344 100644 --- a/internal/cli/ssh/ssh.go +++ b/internal/cli/ssh/ssh.go @@ -3,9 +3,12 @@ package ssh import ( "fmt" + "os" + "strings" "git.eeqj.de/sneak/keyfunc/internal/cli/options" "git.eeqj.de/sneak/keyfunc/internal/derive" + "git.eeqj.de/sneak/keyfunc/internal/mnemonic" "git.eeqj.de/sneak/keyfunc/internal/sshkey" "github.com/spf13/cobra" ) @@ -84,6 +87,26 @@ func write(cmd *cobra.Command, text string) error { return nil } +// childEnv is the tool's environment with the mnemonic variables taken +// out, for the ssh and sftp children it starts. "ssh to" exists so the +// private key never leaves the tool; the mnemonic, from either variable, +// must not leave it either. +func childEnv() []string { + environ := os.Environ() + kept := make([]string, 0, len(environ)) + + for _, entry := range environ { + name, _, _ := strings.Cut(entry, "=") + if name == mnemonic.Variable || name == mnemonic.CommandVariable { + continue + } + + kept = append(kept, entry) + } + + return kept +} + // addComment gives a command its comment flag. func addComment(cmd *cobra.Command) { cmd.Flags().String( diff --git a/internal/cli/ssh/to.go b/internal/cli/ssh/to.go index 8dc5149..1afd7a9 100644 --- a/internal/cli/ssh/to.go +++ b/internal/cli/ssh/to.go @@ -70,6 +70,7 @@ func to() *cobra.Command { func connect(ctx context.Context, argv []string) error { //nolint:gosec // the arguments are the user's own, meant for ssh command := exec.CommandContext(ctx, "ssh", argv...) + command.Env = childEnv() command.Stdin = os.Stdin command.Stdout = os.Stdout command.Stderr = os.Stderr diff --git a/internal/cli/ssh_test.go b/internal/cli/ssh_test.go index 9fd4ae9..775d1e9 100644 --- a/internal/cli/ssh_test.go +++ b/internal/cli/ssh_test.go @@ -60,13 +60,19 @@ const ( // an authorized_keys file. const keyLine = vectorZero + " keyfunc/ssh/0\n" +// marker is a variable set beside the mnemonic ones and expected to +// reach the stand-in, so a scrubbed environment is told apart from an +// empty one. +const marker = "KEYFUNC_TEST_MARKER" + // installer is a stand-in for the system sftp for the install // command. It writes down the arguments and every command of the -// batch it is given, echoes each command as sftp does, and carries -// the commands out against a directory standing in for the host's -// home directory, so that what keyfunc sends can be watched doing its -// work. A command that begins with a dash may fail; any other failure -// ends the session, as it does in sftp's own batch mode. +// batch it is given, echoes each command as sftp does, writes down its +// own environment when a test asks for it, and carries the commands out +// against a directory standing in for the host's home directory, so that +// what keyfunc sends can be watched doing its work. A command that +// begins with a dash may fail; any other failure ends the session, as it +// does in sftp's own batch mode. // // The listing and the two ways a get can fail are worded as the // OpenSSH client words them, each naming the path the server expanded. @@ -81,6 +87,7 @@ const keyLine = vectorZero + " keyfunc/ssh/0\n" // draws the warning ssh writes for it, which carries the wording of a // missing file into a session that goes on to authenticate. const installer = ` +[ -n "$KEYFUNC_TEST_ENVIRONMENT" ] && env > "$KEYFUNC_TEST_ENVIRONMENT" previous= for argument in "$@"; do printf '%s\n' "$argument" >> "$KEYFUNC_TEST_ARGUMENTS" @@ -144,9 +151,11 @@ done // caller is a stand-in for the system ssh for the to command. It // writes down the arguments it was given, notes the agent socket if -// there really is one at the path it was handed, and ends with the -// status the test asked for. +// there really is one at the path it was handed, writes down its own +// environment when a test asks for it, and ends with the status the +// test asked for. const caller = ` +[ -n "$KEYFUNC_TEST_ENVIRONMENT" ] && env > "$KEYFUNC_TEST_ENVIRONMENT" for argument in "$@"; do printf '%s\n' "$argument" >> "$KEYFUNC_TEST_ARGUMENTS" done @@ -444,6 +453,36 @@ func TestTheToolEndsWithTheStatusSSHEndedWith(t *testing.T) { require.Equal(t, failingStatus, cli.Main()) } +func TestTheMnemonicIsNotHandedToSFTP(t *testing.T) { + t.Setenv(mnemonic.CommandVariable, "echo "+example()) + t.Setenv(mnemonic.Variable, example()) + t.Setenv(marker, "reaches the stand-in") + + pretendHost(t) + environment := recordEnvironment(t) + + install(t, host) + + mnemonicWithheld(t, read(t, environment)) +} + +func TestTheMnemonicIsNotHandedToSSH(t *testing.T) { + t.Setenv(mnemonic.CommandVariable, "echo "+example()) + t.Setenv(mnemonic.Variable, example()) + t.Setenv(marker, "reaches the stand-in") + + pretendCall(t) + environment := recordEnvironment(t) + + _, err := execute(t, subcommand, "to", host, "uptime") + + var passed ssh.StatusError + + require.ErrorAs(t, err, &passed) + + mnemonicWithheld(t, read(t, environment)) +} + // pretendHost puts the install stand-in on the path and gives back the // places it writes to. func pretendHost(t *testing.T) pretended { @@ -592,6 +631,28 @@ func standIn(t *testing.T, name, body string) { ) } +// recordEnvironment asks the stand-in to write its environment down and +// gives back the file it writes it to. +func recordEnvironment(t *testing.T) string { + t.Helper() + + path := filepath.Join(t.TempDir(), "environment") + t.Setenv("KEYFUNC_TEST_ENVIRONMENT", path) + + return path +} + +// mnemonicWithheld requires that neither mnemonic variable reached the +// stand-in and that the marker set beside them did, so an empty +// environment does not pass for a scrubbed one. +func mnemonicWithheld(t *testing.T, environment string) { + t.Helper() + + require.NotContains(t, environment, mnemonic.Variable+"=") + require.NotContains(t, environment, mnemonic.CommandVariable+"=") + require.Contains(t, environment, marker+"=") +} + // read returns what is in a file. func read(t *testing.T, path string) string { t.Helper()