feat(cli): take the security key PIN in the script review dialog

convox.yml

@@ -30,7 +30,7 @@ services:
     # Pinned to an immutable release tag. scripts/bump-version.sh sets it to the new
     # version (:vX.Y.Z); the release workflow publishes that tag when the v* git tag is pushed.
     # :e327add is the v0.1.1 release (identical to :latest when this was pinned).
-    image: docker.io/docspringcom/rack-gateway:v0.1.21
+    image: docker.io/docspringcom/rack-gateway:v0.1.22
     command: ./scripts/start-gateway.sh
     environment:
       - PORT=8080
@@ -54,7 +54,7 @@ services:
 
   admin:
     # Uses same image as gateway service (kept in sync by scripts/bump-version.sh)
-    image: docker.io/docspringcom/rack-gateway:v0.1.21
+    image: docker.io/docspringcom/rack-gateway:v0.1.22
     command: echo "Use this service to run migrations, database admin tasks, etc."
     environment:
       - PORT=8080

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

@@ -374,8 +374,13 @@ fit in the audit log (50 KB).
   button is Cancel. Characters that would make the script read differently from what runs (zero-width spaces,
   right-to-left overrides) show as `<U+XXXX>`, with a warning. API tokens run unattended, so their scripts aren't
   reviewed.
+- **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
+  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.
+  reviewed. When the review dialog took the PIN, there's no PIN dialog: the review dialog showed the same lines.
 - **`--dry-run` stops after the review** and prints what the PIN dialog would show and how many bytes would be
   sent, without asking for MFA or sending anything. Use it to check a script before running it for real.
 - **The audit log records the script readably:** the command as you'd run it locally (`rails runner

internal/cli/AGENTS.md

@@ -249,7 +249,12 @@ prompt, `scriptreview.Review` shows it with numbered lines, hidden characters as
 highlighting (`highlight.go`: styled spans, ANSI colors in a terminal, macOS system colors in the dialog; a lexer
 that fails or takes over 2 seconds leaves the script plain): a y/N prompt on a
 terminal, else a macOS JXA dialog (Cancel is the default; the texts go in as arguments, never into the JXA source),
-else refused. API tokens skip the review. The PIN dialog shows the script's name, line count and SHA-256, not its
+else refused. API tokens skip the review. The dialog also takes the security key PIN (`convox_process_review.go`
+`reviewPINCheck`) when the command will ask for a WebAuthn proof (not a dry run, no `--mfa-code`, `selectMFAMethod`
+picks WebAuthn) and exactly one key is plugged in and has a PIN (`webauthn.SingleKeyHasPIN`). Run is then the
+default. `webauthn.CheckPIN` checks each PIN without a touch (`CredentialsInfo`; a key without credential management
+leaves it to the assertion), a wrong one re-shows the dialog with the retries left, and the PIN goes to
+`newProcessClient`, so the assertion only needs the touch. The PIN dialog shows the script's name, line count and SHA-256, not its
 text. `--dry-run` stops after the checks and the review (`runProcessCommand`): it prints the PIN dialog summary and
 the command's size, with no MFA prompt and no request to the rack.
 

internal/cli/convox_env_protected.go

@@ -128,7 +128,7 @@ func protectedEnvMFAAuth(cmd *cobra.Command, commandName string, keys []string)
 			return "", err
 		}
 	}
-	return freshMFAAuth(cmd, prompt)
+	return freshMFAAuth(cmd, prompt, "")
 }
 
 // protectedEnvPrompt describes a protected env change for the person approving it: one summary line per key,

internal/cli/convox_env_protected_test.go

@@ -21,12 +21,14 @@ import (
 	"github.com/DocSpring/rack-gateway/internal/gateway/envutil"
 )
 
-// envChangeGateway is a fake gateway whose MFA status reports a TOTP method and a step-up window that is still
-// open, and whose apps have appSettings (none by default). It records the headers of every rack-proxy request.
+// envChangeGateway is a fake gateway whose MFA status reports mfaMethods (a TOTP method by default) and a step-up
+// window that is still open, that starts WebAuthn assertions, and whose apps have appSettings (none by default). It
+// records the headers of every rack-proxy request.
 type envChangeGateway struct {
 	mu          sync.Mutex
 	proxied     []http.Header
 	appSettings string
+	mfaMethods  []MFAMethodResponse
 }
 
 func (g *envChangeGateway) handler(w http.ResponseWriter, r *http.Request) {
@@ -41,13 +43,20 @@ func (g *envChangeGateway) handler(w http.ResponseWriter, r *http.Request) {
 		_, _ = w.Write([]byte(appSettings))
 		return
 	}
+	if r.URL.Path == "/api/v1/auth/mfa/webauthn/assertion/start" {
+		_, _ = w.Write([]byte(`{"options":{"publicKey":{"challenge":"Y2hhbGxlbmdl"}},"session_data":"session"}`))
+		return
+	}
 	if r.URL.Path == "/api/v1/auth/mfa/status" {
 		expiresAt := time.Now().Add(5 * time.Minute)
+		g.mu.Lock()
+		methods := g.mfaMethods
+		g.mu.Unlock()
+		if methods == nil {
+			methods = []MFAMethodResponse{{ID: 1, Type: "totp", Label: "Phone"}}
+		}
 		_ = json.NewEncoder(w).Encode(MFAStatusResponse{
-			Enrolled:              true,
-			Required:              true,
-			Methods:               []MFAMethodResponse{{ID: 1, Type: "totp", Label: "Phone"}},
-			RecentStepUpExpiresAt: &expiresAt,
+			Enrolled: true, Required: true, Methods: methods, RecentStepUpExpiresAt: &expiresAt,
 		})
 		return
 	}

internal/cli/convox_process_approval.go

@@ -92,7 +92,7 @@ func commandPrompt(process processCommand) webauthn.Prompt {
 
 // freshMFAAuth collects an MFA proof for the selected rack (one prompt or key touch, or --mfa-code), whatever the
 // step-up window says, with prompt describing what it approves. API tokens have no MFA.
-func freshMFAAuth(cmd *cobra.Command, prompt webauthn.Prompt) (string, error) {
+func freshMFAAuth(cmd *cobra.Command, prompt webauthn.Prompt, pin string) (string, error) {
 	if apiTokenOverride() != "" {
 		return "", nil
 	}
@@ -104,6 +104,6 @@ func freshMFAAuth(cmd *cobra.Command, prompt webauthn.Prompt) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	auth, _, err := GetMFAAuthWithPIN(cmd, gatewayURL, bearer, rack, "", prompt)
+	auth, _, err := GetMFAAuthWithPIN(cmd, gatewayURL, bearer, rack, pin, prompt)
 	return auth, err
 }

internal/cli/convox_process_client.go

@@ -49,9 +49,9 @@ func (c *processClient) ProcessExec(
 }
 
 // newProcessClient returns the client for an exec or run command, whose requests all carry headers. prompt says
-// what the command's MFA proof approves.
+// what the command's MFA 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,
+	cobraCmd *cobra.Command, headers map[string]string, prompt webauthn.Prompt, pin string,
 ) (*processClient, error) {
 	client, err := newHeaderClient("", headers)
 	if err != nil {
@@ -64,7 +64,7 @@ func newProcessClient(
 		var mfaAuth string
 		err := withTerminalState(before, func() error {
 			var err error
-			mfaAuth, err = freshMFAAuth(cobraCmd, prompt)
+			mfaAuth, err = freshMFAAuth(cobraCmd, prompt, pin)
 			return err
 		})
 		if err != nil {

internal/cli/convox_process_client_test.go

@@ -25,7 +25,7 @@ func newTestProcessClient(t *testing.T, cmd *cobra.Command) (*processClient, *en
 	gateway := newEnvChangeGateway(t)
 	MFACodeFlag = "123456"
 	cmd.SetErr(io.Discard)
-	client, err := newProcessClient(cmd, nil, webauthn.Prompt{})
+	client, err := newProcessClient(cmd, nil, webauthn.Prompt{}, "")
 	require.NoError(t, err)
 	require.Empty(t, gateway.requests(), "nothing is sent before the command runs")
 	return client, gateway
@@ -61,3 +61,29 @@ func TestProcessClientSendsTheProofWithTheExec(t *testing.T) {
 		require.Equal(t, authWithProof, basicCredential(t, header))
 	}
 }
+
+// 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) {
+	gateway := newEnvChangeGateway(t)
+	gateway.mfaMethods = []MFAMethodResponse{{ID: 2, Type: "webauthn", Label: "YubiKey"}}
+	var pins []string
+	previous := getWebAuthnAssertion
+	getWebAuthnAssertion = func(_ webauthn.AssertionOptions, pin string) (*webauthn.AssertionResponse, string, error) {
+		pins = append(pins, pin)
+		return &webauthn.AssertionResponse{CredentialID: "key"}, pin, nil
+	}
+	t.Cleanup(func() { getWebAuthnAssertion = previous })
+	cmd := RunCommand()
+	cmd.SetErr(io.Discard)
+	client, err := newProcessClient(cmd, nil, webauthn.Prompt{}, "123456")
+	require.NoError(t, err)
+
+	command := "rails runner 'puts 1'"
+	_, _ = client.ProcessRun("myapp", "web", structs.ProcessRunOptions{Command: &command})
+
+	require.Equal(t, []string{"123456"}, pins)
+	proxied := gateway.requests()
+	require.Len(t, proxied, 1)
+	require.Contains(t, basicCredential(t, proxied[0]), authWithoutProof+".webauthn.")
+}

internal/cli/convox_process_command.go

@@ -11,7 +11,6 @@ import (
 	"github.com/convox/stdcli"
 	"github.com/spf13/cobra"
 
-	"github.com/DocSpring/rack-gateway/internal/cli/scriptreview"
 	"github.com/DocSpring/rack-gateway/internal/gateway/audit"
 	"github.com/DocSpring/rack-gateway/internal/scriptrun"
 )
@@ -46,6 +45,9 @@ type processCommand struct {
 	script *scriptrun.Script
 	// dryRun stops the command before anything is sent (--dry-run).
 	dryRun bool
+	// pin is the security key PIN entered in the script review dialog, for the command's MFA proof ("" when the
+	// PIN is asked for when the command runs). It's only ever passed to the security key.
+	pin string
 }
 
 // where names the command's app and rack.
@@ -105,13 +107,14 @@ func prepareProcessCommand(cobraCmd *cobra.Command, args []string, action string
 	if process.dryRun, err = cobraCmd.Flags().GetBool(dryRunFlag); err != nil {
 		return process, err
 	}
-	return process, reviewScript(process)
+	process.pin, err = reviewScript(process)
+	return process, err
 }
 
 // connect returns the client and Convox context that run the command, args being the command line's arguments.
 // A script's command replaces the command words.
 func (p processCommand) connect(cobraCmd *cobra.Command, args []string) (*processClient, *stdcli.Context, error) {
-	client, err := newProcessClient(cobraCmd, p.headers(), commandPrompt(p))
+	client, err := newProcessClient(cobraCmd, p.headers(), commandPrompt(p), p.pin)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -199,22 +202,6 @@ func readScriptFile(path string) ([]byte, error) {
 	return text, nil
 }
 
-// reviewScript has the person running the command approve its script before anything is sent. API tokens run
-// unattended: there's no one to ask.
-func reviewScript(process processCommand) error {
-	if process.script == nil || apiTokenOverride() != "" {
-		return nil
-	}
-	return reviewScriptWith(process)
-}
-
-// reviewScriptWith shows a script for approval (scriptreview.Review). Tests replace it.
-var reviewScriptWith = func(process processCommand) error {
-	return scriptreview.Review(scriptreview.Request{
-		Where: process.where(), Justification: process.justification, Script: *process.script,
-	})
-}
-
 // headerLineBreaks are what Go's HTTP client replaces with spaces in a header value.
 var headerLineBreaks = strings.NewReplacer("\n", " ", "\r", " ")
 

internal/cli/convox_process_command_test.go

@@ -121,9 +121,9 @@ func reviewedScripts(t *testing.T, err error) *[]processCommand {
 	t.Helper()
 	var reviewed []processCommand
 	previous := reviewScriptWith
-	reviewScriptWith = func(process processCommand) error {
+	reviewScriptWith = func(process processCommand) (string, error) {
 		reviewed = append(reviewed, process)
-		return err
+		return "", err
 	}
 	t.Cleanup(func() { reviewScriptWith = previous })
 	return &reviewed

internal/cli/convox_process_review.go

@@ -0,0 +1,76 @@
+package cli
+
+import (
+	"errors"
+	"strings"
+
+	"github.com/DocSpring/rack-gateway/internal/cli/scriptreview"
+	"github.com/DocSpring/rack-gateway/internal/cli/terminal"
+	"github.com/DocSpring/rack-gateway/internal/cli/webauthn"
+)
+
+// reviewScript has the person running the command approve its script before anything is sent, and returns the
+// security key PIN they entered with the approval, or "". API tokens run unattended: there's no one to ask.
+func reviewScript(process processCommand) (string, error) {
+	if process.script == nil || apiTokenOverride() != "" {
+		return "", nil
+	}
+	return reviewScriptWith(process)
+}
+
+// reviewScriptWith shows a script for approval (scriptreview.Review), asking for the PIN as well when
+// reviewPINCheck says so. Tests replace it.
+var reviewScriptWith = func(process processCommand) (string, error) {
+	checkPIN, err := reviewPINCheck(process)
+	if err != nil {
+		return "", err
+	}
+	return scriptreview.Review(scriptreview.Request{
+		Where: process.where(), Justification: process.justification, Script: *process.script, CheckPIN: checkPIN,
+	})
+}
+
+// reviewPINCheck returns the check of the PIN the review dialog asks for, so that approving the script and entering
+// the security key PIN are one step, and only the touch is left for when the command runs. It returns nil, and the
+// PIN is asked for when the command runs as for any other, unless the review is a dialog (no terminal), the command
+// will ask for MFA (not a dry run, and no --mfa-code), the MFA method is WebAuthn, and exactly one security key is
+// connected and it has a PIN (see webauthn.SingleKeyHasPIN).
+func reviewPINCheck(process processCommand) (scriptreview.PINCheck, error) {
+	if _, ok := terminal.StdinFD(); ok || process.dryRun {
+		return nil, nil
+	}
+	if _, ok := inlineTotpFromFlag(); ok {
+		return nil, nil
+	}
+	gatewayURL, bearer, err := LoadRackAuth(process.rack)
+	if err != nil {
+		return nil, err
+	}
+	status, err := loadMFAStatus(gatewayURL, bearer)
+	if err != nil {
+		return nil, err
+	}
+	method, err := selectMFAMethod(status, process.rack)
+	if err != nil {
+		return nil, err
+	}
+	if method.Type != "webauthn" || !securityKeyHasPIN() {
+		return nil, nil
+	}
+	return checkReviewPIN, nil
+}
+
+// securityKeyHasPIN reports whether the one connected security key has a PIN (webauthn.SingleKeyHasPIN). Tests
+// replace it.
+var securityKeyHasPIN = webauthn.SingleKeyHasPIN
+
+// checkReviewPIN checks a PIN entered in the review dialog on the security key, without a touch. A wrong PIN is
+// asked for again.
+func checkReviewPIN(pin string) (string, error) {
+	err := webauthn.CheckPIN(pin)
+	if errors.Is(err, webauthn.ErrWrongPIN) {
+		message := err.Error()
+		return strings.ToUpper(message[:1]) + message[1:] + ". Try again.", nil
+	}
+	return "", err
+}

internal/cli/convox_process_review_test.go

@@ -0,0 +1,58 @@
+package cli
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// The PIN entered in the review dialog is kept for the command's MFA proof, so the PIN dialog doesn't ask again.
+func TestProcessCommandKeepsThePINFromTheReview(t *testing.T) {
+	newEnvChangeGateway(t)
+	previous := reviewScriptWith
+	reviewScriptWith = func(processCommand) (string, error) { return "123456", nil }
+	t.Cleanup(func() { reviewScriptWith = previous })
+	flags := map[string]string{scriptFlag: writeScript(t, "count.rb", "puts 1\n"), justificationFlag: testJustification}
+
+	process, err := prepareProcessCommand(processCobraCommand(t, RunCommand(), flags),
+		[]string{"web", "rails", "runner"}, "run")
+	require.NoError(t, err)
+	require.Equal(t, "123456", process.pin)
+}
+
+// The review dialog asks for the PIN only when the command will ask for a WebAuthn proof and the one connected
+// security key has a PIN: not for another MFA method, a dry run, or a TOTP code given with --mfa-code.
+func TestReviewPINCheck(t *testing.T) {
+	gateway := newEnvChangeGateway(t)
+	hasPIN := true
+	previous := securityKeyHasPIN
+	securityKeyHasPIN = func() bool { return hasPIN }
+	t.Cleanup(func() { securityKeyHasPIN = previous })
+	rack, err := SelectedRack()
+	require.NoError(t, err)
+	process := processCommand{rack: rack}
+	asksForPIN := func() bool {
+		t.Helper()
+		check, err := reviewPINCheck(process)
+		require.NoError(t, err)
+		return check != nil
+	}
+
+	require.False(t, asksForPIN(), "a TOTP method")
+
+	gateway.mu.Lock()
+	gateway.mfaMethods = []MFAMethodResponse{{ID: 2, Type: "webauthn", Label: "YubiKey"}}
+	gateway.mu.Unlock()
+	require.True(t, asksForPIN())
+
+	hasPIN = false
+	require.False(t, asksForPIN(), "several keys, or a key without a PIN")
+	hasPIN = true
+
+	process.dryRun = true
+	require.False(t, asksForPIN(), "a dry run asks for no MFA")
+	process.dryRun = false
+
+	MFACodeFlag = "123456"
+	require.False(t, asksForPIN(), "a TOTP code given with --mfa-code")
+}

internal/cli/mfa_helpers.go

@@ -287,7 +287,7 @@ func collectWebAuthnAssertionWithPIN(
 	options.Prompt = prompt
 	options.Prompt.Notify = touchNotificationsEnabled()
 
-	assertion, pinUsed, err := webauthn.GetAssertionWithCachedPIN(options, cachedPIN)
+	assertion, pinUsed, err := getWebAuthnAssertion(options, cachedPIN)
 	if err != nil {
 		return "", "", err
 	}
@@ -310,6 +310,9 @@ func collectWebAuthnAssertionWithPIN(
 	return base64.StdEncoding.EncodeToString(jsonData), pinUsed, nil
 }
 
+// getWebAuthnAssertion asks the security key for an assertion (webauthn.GetAssertionWithCachedPIN). Tests replace it.
+var getWebAuthnAssertion = webauthn.GetAssertionWithCachedPIN
+
 func marshalWebAuthnResponse(assertion *webauthn.AssertionResponse) (string, error) {
 	response := map[string]any{
 		"id":    assertion.CredentialID,