feat(cli): ask for a command's MFA touch before its process starts

docs/src/content/docs/user-guide/cli/commands.mdx

@@ -344,8 +344,9 @@ rack-gateway run web "rake db:migrate" -a myapp \
   --justification "Run the migration for the new invoices table, which the deploy skipped"
 ```
 
-Every `run` asks for MFA, even within the step-up window. It starts the process first and asks once the process
-is running, just before the command runs (see [Running commands](/user-guide/cli/mfa-verification/#running-commands)).
+Every `run` asks for MFA, even within the step-up window, before it starts the process. An attached `run` sends
+the proof with the command once the process is running, and `run --detach` with the process start (see
+[Running commands](/user-guide/cli/mfa-verification/#running-commands)).
 
 #### Justifications
 
@@ -379,7 +380,7 @@ fit in the audit log (50 KB).
 - **The dialog takes your security key PIN** when the command will use WebAuthn and exactly one security key with
   a PIN is plugged in. Entering the PIN and pressing Return approves the script. The key checks the PIN straight
   away, without a touch: a wrong PIN shows the dialog again, with the tries left before the key locks when the key
-  reports them. When the command runs, you only touch the key. With TOTP, `--mfa-code`, several keys or a key
+  reports them. Then you touch the key, before the process starts. With TOTP, `--mfa-code`, several keys or a key
   without a PIN, the dialog only asks for approval, and the command asks for MFA as usual.
 - **The PIN dialog shows the script's name, size and SHA-256**, so you can check the touch is for the script you
   reviewed. When the review dialog took the PIN, there's no PIN dialog: the review dialog showed the same lines.

docs/src/content/docs/user-guide/cli/mfa-verification.mdx

@@ -127,9 +127,9 @@ time, and the gateway refuses the request unless it carries a proof it verifies
 ### Running Commands
 
 `run` and `exec` can read, change or delete any of an app's data, so each one asks for MFA, even right after
-another command. `run` starts its process first and asks once the process is running, just before the command
-runs in it: a TOTP code or a WebAuthn challenge doesn't expire while a slow process starts. The PIN dialog shows
-the app, the rack, the command and its justification.
+another command. They ask before anything is sent. An attached `run` then starts its process and sends the proof
+with the command once the process is running; `run --detach` sends it with the process start, which runs the
+command. The PIN dialog shows the app, the rack, the command and its justification.
 
 That first process only runs `sleep 3600`, so starting it needs no MFA of its own, including for another of the
 app's releases (`run --release`). It can't use a custom image, privileged options or the service's lifecycle

internal/cli/AGENTS.md

@@ -232,11 +232,11 @@ quotes, backslashes or anything outside printable ASCII, escaping non-ASCII char
 
 The gateway needs an MFA proof on every exec and every process start that runs a command, whatever the step-up
 window says. `convox run` (attached) starts a placeholder process (`convox.AttachedRunPlaceholder`), waits for
-it, then execs the command into it. `processClient` (`convox_process_client.go`) wraps the SDK client: the
-placeholder start and the waits go without a proof, and the proof is collected just before the exec (or a
-detached run's start), with the terminal taken out of the raw mode Convox put it in, so a TOTP code or a WebAuthn
-challenge doesn't expire while a slow process starts. The PIN dialog summary names the app, rack, command and
-justification (`commandPrompt`). `--justification` travels in `audit.JustificationHeader` (percent-encoded), which
+it, then execs the command into it. `newProcessClient` (`convox_process_client.go`) collects the command's proof
+before anything goes to the rack, so the key touch comes straight after the PIN, and `processClient` sends it with
+the exec (or a detached run's start) only: the placeholder start and the waits go without one. A process starts in
+seconds, well within a WebAuthn challenge's five minutes or a TOTP code's 30 seconds or more. The PIN dialog
+summary names the app, rack, command and justification (`commandPrompt`). `--justification` travels in `audit.JustificationHeader` (percent-encoded), which
 the gateway records and doesn't forward. When `require_command_justification` is on (gateway-wide or the app's),
 the CLI refuses a command without one before anything starts (it reads the global and app settings; API tokens
 can't, and rely on the gateway's refusal).

internal/cli/convox_process_client.go

@@ -6,73 +6,57 @@ import (
 	"github.com/convox/convox/pkg/structs"
 	"github.com/convox/convox/sdk"
 	"github.com/spf13/cobra"
-	"golang.org/x/term"
 
-	"github.com/DocSpring/rack-gateway/internal/cli/terminal"
 	"github.com/DocSpring/rack-gateway/internal/cli/webauthn"
 	"github.com/DocSpring/rack-gateway/internal/convox"
 )
 
 // processClient is the Convox SDK client exec and run use. The gateway refuses a command run without an MFA proof
-// of its own, whatever the step-up window says, and a proof is single-use and short-lived (a TOTP code lasts a
-// minute at most, a WebAuthn challenge five), while a run can wait minutes for its process to start. So the proof
-// is collected when the command is about to run and goes with that request only: the exec, or a detached run's
-// process start. `convox run`'s placeholder start and the requests around the exec (waiting for the process,
-// stopping it) carry none; the exec's proof opens the step-up window that stopping the process needs.
+// of its own, whatever the step-up window says, and a proof is single-use. The proof is collected before anything
+// is sent, so the key touch follows the PIN at once, and goes with the one request that runs the command: the exec,
+// or a detached run's process start. `convox run`'s placeholder start and the requests around the exec (waiting for
+// the process, stopping it) carry none; the exec's proof opens the step-up window that stopping the process needs.
+// A process takes seconds to start, and a WebAuthn challenge lasts five minutes (a TOTP code at least 30 seconds).
 type processClient struct {
 	*sdk.Client
-	// withProof returns a client whose requests carry a newly collected MFA proof.
-	withProof func() (*sdk.Client, error)
+	// proof is the client whose requests carry the command's MFA proof.
+	proof *sdk.Client
 }
 
 // ProcessRun starts a process: `convox run`'s placeholder without a proof, any other command with one.
 func (c *processClient) ProcessRun(app, service string, opts structs.ProcessRunOptions) (*structs.Process, error) {
 	if opts.Command != nil && *opts.Command == convox.AttachedRunPlaceholder {
 		return c.Client.ProcessRun(app, service, opts)
 	}
-	client, err := c.withProof()
-	if err != nil {
-		return nil, err
-	}
-	return client.ProcessRun(app, service, opts)
+	return c.proof.ProcessRun(app, service, opts)
 }
 
-// ProcessExec runs command in a running process, with an MFA proof collected now.
+// ProcessExec runs command in a running process, with the command's MFA proof.
 func (c *processClient) ProcessExec(
 	app, pid, command string, rw io.ReadWriter, opts structs.ProcessExecOptions,
 ) (int, error) {
-	client, err := c.withProof()
-	if err != nil {
-		return 0, err
-	}
-	return client.ProcessExec(app, pid, command, rw, opts)
+	return c.proof.ProcessExec(app, pid, command, rw, opts)
 }
 
-// newProcessClient returns the client for an exec or run command, whose requests all carry headers. prompt says
-// what the command's MFA proof approves; pin is the security key PIN already entered for it, or "".
+// newProcessClient collects the MFA proof of an exec or run command and returns the command's client, whose
+// requests all carry headers. prompt says what the proof approves; pin is the security key PIN already entered for
+// it, or "".
 func newProcessClient(
 	cobraCmd *cobra.Command, headers map[string]string, prompt webauthn.Prompt, pin string,
 ) (*processClient, error) {
 	client, err := newHeaderClient("", headers)
 	if err != nil {
 		return nil, err
 	}
-	// The prompts run while Convox has the terminal in raw mode for the command's input, so they get back the state
-	// it had before.
-	before := stdinTerminalState()
-	withProof := func() (*sdk.Client, error) {
-		var mfaAuth string
-		err := withTerminalState(before, func() error {
-			var err error
-			mfaAuth, err = freshMFAAuth(cobraCmd, prompt, pin)
-			return err
-		})
-		if err != nil {
-			return nil, err
-		}
-		return newHeaderClient(mfaAuth, headers)
+	mfaAuth, err := freshMFAAuth(cobraCmd, prompt, pin)
+	if err != nil {
+		return nil, err
+	}
+	proof, err := newHeaderClient(mfaAuth, headers)
+	if err != nil {
+		return nil, err
 	}
-	return &processClient{Client: client, withProof: withProof}, nil
+	return &processClient{Client: client, proof: proof}, nil
 }
 
 // newHeaderClient returns a client carrying mfaAuth (see newConvoxClient) that adds headers to every request.
@@ -91,30 +75,3 @@ func newHeaderClient(mfaAuth string, headers map[string]string) (*sdk.Client, er
 	}
 	return client, nil
 }
-
-// stdinTerminalState returns stdin's terminal state, or nil when stdin isn't a terminal.
-func stdinTerminalState() *term.State {
-	fd, ok := terminal.StdinFD()
-	if !ok {
-		return nil
-	}
-	state, err := term.GetState(fd)
-	if err != nil {
-		return nil
-	}
-	return state
-}
-
-// withTerminalState runs fn with stdin's terminal in state (nil: as it is), then puts it back as it was.
-func withTerminalState(state *term.State, fn func() error) error {
-	fd, ok := terminal.StdinFD()
-	if state == nil || !ok {
-		return fn()
-	}
-	current, err := term.GetState(fd)
-	if err != nil || term.Restore(fd, state) != nil {
-		return fn()
-	}
-	defer func() { _ = term.Restore(fd, current) }()
-	return fn()
-}

internal/cli/convox_process_client_test.go

@@ -27,7 +27,7 @@ func newTestProcessClient(t *testing.T, cmd *cobra.Command) (*processClient, *en
 	cmd.SetErr(io.Discard)
 	client, err := newProcessClient(cmd, nil, webauthn.Prompt{}, "")
 	require.NoError(t, err)
-	require.Empty(t, gateway.requests(), "nothing is sent before the command runs")
+	require.Empty(t, gateway.requests(), "nothing goes to the rack before the command runs")
 	return client, gateway
 }
 
@@ -63,8 +63,9 @@ func TestProcessClientSendsTheProofWithTheExec(t *testing.T) {
 }
 
 // A WebAuthn proof signs with the security key PIN entered when the script was reviewed, so the command asks only
-// for the touch.
-func TestProcessClientSignsWithTheReviewPIN(t *testing.T) {
+// for the touch, and asks for it straight after the PIN: before anything goes to the rack, rather than once the
+// process has started.
+func TestProcessClientSignsWithTheReviewPINBeforeTheProcessStarts(t *testing.T) {
 	gateway := newEnvChangeGateway(t)
 	gateway.mfaMethods = []MFAMethodResponse{{ID: 2, Type: "webauthn", Label: "YubiKey"}}
 	var pins []string
@@ -78,6 +79,8 @@ func TestProcessClientSignsWithTheReviewPIN(t *testing.T) {
 	cmd.SetErr(io.Discard)
 	client, err := newProcessClient(cmd, nil, webauthn.Prompt{}, "123456")
 	require.NoError(t, err)
+	require.Equal(t, []string{"123456"}, pins, "the touch comes before the process starts")
+	require.Empty(t, gateway.requests())
 
 	command := "rails runner 'puts 1'"
 	_, _ = client.ProcessRun("myapp", "web", structs.ProcessRunOptions{Command: &command})

internal/cli/convox_process_command_test.go

@@ -25,9 +25,14 @@ import (
 const testJustification = "Count the templates of account 42 — checking a support ticket"
 
 // setupProcessCommand prepares and connects an exec or run command as runProcessCommand does, without running it.
+// Connecting collects the command's MFA proof, which --mfa-code 123456 answers.
 func setupProcessCommand(
-	cobraCmd *cobra.Command, args []string, action string,
+	t *testing.T, cobraCmd *cobra.Command, args []string, action string,
 ) (*processClient, *stdcli.Context, error) {
+	t.Helper()
+	previous := MFACodeFlag
+	MFACodeFlag = "123456"
+	t.Cleanup(func() { MFACodeFlag = previous })
 	process, err := prepareProcessCommand(cobraCmd, args, action)
 	if err != nil {
 		return nil, nil, err
@@ -60,7 +65,7 @@ func TestProcessCommandRefusedWithoutAJustificationTheAppRequires(t *testing.T)
 			set(gateway)
 			for _, cmd := range []*cobra.Command{ExecCommand(), RunCommand()} {
 				cmd = processCobraCommand(t, cmd, nil)
-				_, _, err := setupProcessCommand(cmd, []string{"web", "rails", "runner", "puts 1"}, cmd.Name())
+				_, _, err := setupProcessCommand(t, cmd, []string{"web", "rails", "runner", "puts 1"}, cmd.Name())
 				require.ErrorContains(t, err, "app myapp requires a justification for every command")
 			}
 			require.Empty(t, gateway.requests())
@@ -72,15 +77,14 @@ func TestProcessCommandRefusedWithoutAJustificationTheAppRequires(t *testing.T)
 // requests, as the header the gateway records.
 func TestProcessCommandSendsItsJustification(t *testing.T) {
 	gateway := newEnvChangeGateway(t)
-	MFACodeFlag = "123456"
 	args := []string{"web", "rails", "runner", "puts 1"}
 
 	short := processCobraCommand(t, RunCommand(), map[string]string{justificationFlag: "checking"})
-	_, _, err := setupProcessCommand(short, args, "run")
+	_, _, err := setupProcessCommand(t, short, args, "run")
 	require.ErrorContains(t, err, "the justification is too short")
 
 	cmd := processCobraCommand(t, RunCommand(), map[string]string{justificationFlag: testJustification})
-	client, _, err := setupProcessCommand(cmd, args, "run")
+	client, _, err := setupProcessCommand(t, cmd, args, "run")
 	require.NoError(t, err)
 	placeholder := convox.AttachedRunPlaceholder
 	_, _ = client.ProcessRun("myapp", "web", structs.ProcessRunOptions{Command: &placeholder})
@@ -147,7 +151,7 @@ func TestProcessCommandSendsAnApprovedScript(t *testing.T) {
 	path := writeScript(t, "count pro.rb", text)
 	for _, cmd := range []*cobra.Command{ExecCommand(), RunCommand()} {
 		cmd = processCobraCommand(t, cmd, map[string]string{scriptFlag: path, justificationFlag: testJustification})
-		_, ctx, err := setupProcessCommand(cmd, []string{"web", "rails", "runner"}, cmd.Name())
+		_, ctx, err := setupProcessCommand(t, cmd, []string{"web", "rails", "runner"}, cmd.Name())
 		require.NoError(t, err)
 		require.Equal(t, []string{"web", scriptrun.Build("count_pro.rb", []byte(text), "rails runner")}, ctx.Args)
 	}
@@ -166,13 +170,13 @@ func TestProcessCommandScriptReview(t *testing.T) {
 	flags := map[string]string{scriptFlag: writeScript(t, "count.rb", "puts 1\n"), justificationFlag: testJustification}
 	args := []string{"web", "rails", "runner"}
 
-	_, _, err := setupProcessCommand(processCobraCommand(t, RunCommand(), flags), args, "run")
+	_, _, err := setupProcessCommand(t, processCobraCommand(t, RunCommand(), flags), args, "run")
 	require.ErrorIs(t, err, scriptreview.ErrDeclined)
 	require.Len(t, *reviewed, 1)
 	require.Empty(t, gateway.requests())
 
 	t.Setenv("RACK_GATEWAY_API_TOKEN", "rgw_test_token")
-	_, _, err = setupProcessCommand(processCobraCommand(t, RunCommand(), flags), args, "run")
+	_, _, err = setupProcessCommand(t, processCobraCommand(t, RunCommand(), flags), args, "run")
 	require.NoError(t, err)
 	require.Len(t, *reviewed, 1)
 }
@@ -203,7 +207,7 @@ func TestProcessCommandRefusesAScriptItCantSend(t *testing.T) {
 			flags[justificationFlag] = tc.justification
 		}
 		cmd := processCobraCommand(t, ExecCommand(), flags)
-		_, _, err := setupProcessCommand(cmd, append([]string{"p-1"}, tc.words...), "exec")
+		_, _, err := setupProcessCommand(t, cmd, append([]string{"p-1"}, tc.words...), "exec")
 		require.ErrorContains(t, err, tc.want, name)
 	}
 	require.Empty(t, *reviewed)

internal/cli/convox_processes_test.go

@@ -108,7 +108,7 @@ func TestProcessCommandsRefuseCommandsTooLongToAudit(t *testing.T) {
 		if err := cmd.Flags().Set("app", "myapp"); err != nil {
 			t.Fatal(err)
 		}
-		_, _, err := setupProcessCommand(cmd, []string{"web", "echo", long}, cmd.Name())
+		_, _, err := setupProcessCommand(t, cmd, []string{"web", "echo", long}, cmd.Name())
 		if err == nil || !strings.Contains(err.Error(), "too long for the audit log") {
 			t.Fatalf("%s: want the command refused as too long, got %v", cmd.Name(), err)
 		}