diff --git a/docs/src/content/docs/user-guide/cli/commands.mdx b/docs/src/content/docs/user-guide/cli/commands.mdx index a439ba22..1f4c7519 100644 --- a/docs/src/content/docs/user-guide/cli/commands.mdx +++ b/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. diff --git a/docs/src/content/docs/user-guide/cli/mfa-verification.mdx b/docs/src/content/docs/user-guide/cli/mfa-verification.mdx index 2ba4cd8a..538416cd 100644 --- a/docs/src/content/docs/user-guide/cli/mfa-verification.mdx +++ b/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 diff --git a/internal/cli/AGENTS.md b/internal/cli/AGENTS.md index c9a6355e..676dd29a 100644 --- a/internal/cli/AGENTS.md +++ b/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). diff --git a/internal/cli/convox_process_client.go b/internal/cli/convox_process_client.go index 60bc96d9..0af68167 100644 --- a/internal/cli/convox_process_client.go +++ b/internal/cli/convox_process_client.go @@ -6,23 +6,21 @@ 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. @@ -30,26 +28,19 @@ func (c *processClient) ProcessRun(app, service string, opts structs.ProcessRunO 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) { @@ -57,22 +48,15 @@ func newProcessClient( 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() -} diff --git a/internal/cli/convox_process_client_test.go b/internal/cli/convox_process_client_test.go index e0ebfb10..827b5e99 100644 --- a/internal/cli/convox_process_client_test.go +++ b/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}) diff --git a/internal/cli/convox_process_command_test.go b/internal/cli/convox_process_command_test.go index ffe78070..1dd000a5 100644 --- a/internal/cli/convox_process_command_test.go +++ b/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) diff --git a/internal/cli/convox_processes_test.go b/internal/cli/convox_processes_test.go index 5fb446b1..c82735af 100644 --- a/internal/cli/convox_processes_test.go +++ b/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) }