From 8c826e3f556ba1694a38fc3d1ab50b932fc3d7b3 Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Sun, 25 Oct 2026 17:22:43 +1300 Subject: [PATCH 1/3] feat(cli): take the security key PIN in the script review dialog When a --script command has no terminal, the macOS review dialog now asks for the security key PIN too, so approving the script and entering the PIN are one step and only the touch is left for when the command runs. It does this when the command will ask for a WebAuthn proof (not a dry run, no --mfa-code) and exactly one security key is plugged in and has a PIN. The PIN is checked on the key straight away, without a touch, by reading its credential metadata. A wrong PIN re-shows the dialog with the tries left before the key locks; a blocked key ends the review. A key without credential management leaves the check to the assertion. Run is the default button in this mode, and the dialog shows a lock icon instead of osascript's folder. --- .../content/docs/user-guide/cli/commands.mdx | 7 +- internal/cli/AGENTS.md | 7 +- internal/cli/convox_env_protected.go | 2 +- internal/cli/convox_env_protected_test.go | 21 ++- internal/cli/convox_process_approval.go | 4 +- internal/cli/convox_process_client.go | 6 +- internal/cli/convox_process_client_test.go | 28 +++- internal/cli/convox_process_command.go | 25 +--- internal/cli/convox_process_command_test.go | 4 +- internal/cli/convox_process_review.go | 76 +++++++++++ internal/cli/convox_process_review_test.go | 58 ++++++++ internal/cli/mfa_helpers.go | 5 +- internal/cli/scriptreview/scriptreview.go | 126 ++++++++++++++---- .../cli/scriptreview/scriptreview_test.go | 61 ++++++++- internal/cli/webauthn/assertion_flow.go | 2 +- internal/cli/webauthn/pin_check.go | 82 ++++++++++++ internal/cli/webauthn/pin_check_test.go | 82 ++++++++++++ internal/cli/webauthn/types.go | 8 +- internal/cli/webauthn/webauthn.go | 21 ++- internal/cli/webauthn/webauthn_stub.go | 10 ++ 20 files changed, 562 insertions(+), 73 deletions(-) create mode 100644 internal/cli/convox_process_review.go create mode 100644 internal/cli/convox_process_review_test.go create mode 100644 internal/cli/webauthn/pin_check.go create mode 100644 internal/cli/webauthn/pin_check_test.go diff --git a/docs/src/content/docs/user-guide/cli/commands.mdx b/docs/src/content/docs/user-guide/cli/commands.mdx index 5c5962ec..74b3ffac 100644 --- a/docs/src/content/docs/user-guide/cli/commands.mdx +++ b/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 ``, 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 + 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 diff --git a/internal/cli/AGENTS.md b/internal/cli/AGENTS.md index 84351a85..ea38f672 100644 --- a/internal/cli/AGENTS.md +++ b/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. diff --git a/internal/cli/convox_env_protected.go b/internal/cli/convox_env_protected.go index c61e49a4..1568a27d 100644 --- a/internal/cli/convox_env_protected.go +++ b/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, diff --git a/internal/cli/convox_env_protected_test.go b/internal/cli/convox_env_protected_test.go index 2847f576..ff97372b 100644 --- a/internal/cli/convox_env_protected_test.go +++ b/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 } diff --git a/internal/cli/convox_process_approval.go b/internal/cli/convox_process_approval.go index 3c22261b..5aee099a 100644 --- a/internal/cli/convox_process_approval.go +++ b/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 } diff --git a/internal/cli/convox_process_client.go b/internal/cli/convox_process_client.go index 1ecd7661..60bc96d9 100644 --- a/internal/cli/convox_process_client.go +++ b/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 { diff --git a/internal/cli/convox_process_client_test.go b/internal/cli/convox_process_client_test.go index 6cde6810..e0ebfb10 100644 --- a/internal/cli/convox_process_client_test.go +++ b/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.") +} diff --git a/internal/cli/convox_process_command.go b/internal/cli/convox_process_command.go index 8a29f370..6e5a1224 100644 --- a/internal/cli/convox_process_command.go +++ b/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", " ") diff --git a/internal/cli/convox_process_command_test.go b/internal/cli/convox_process_command_test.go index 1bba332f..73d0d382 100644 --- a/internal/cli/convox_process_command_test.go +++ b/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 diff --git a/internal/cli/convox_process_review.go b/internal/cli/convox_process_review.go new file mode 100644 index 00000000..3d2bdc67 --- /dev/null +++ b/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 +} diff --git a/internal/cli/convox_process_review_test.go b/internal/cli/convox_process_review_test.go new file mode 100644 index 00000000..52c4dc98 --- /dev/null +++ b/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") +} diff --git a/internal/cli/mfa_helpers.go b/internal/cli/mfa_helpers.go index 3f94ba94..61096fbc 100644 --- a/internal/cli/mfa_helpers.go +++ b/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, diff --git a/internal/cli/scriptreview/scriptreview.go b/internal/cli/scriptreview/scriptreview.go index 0e242437..514c8310 100644 --- a/internal/cli/scriptreview/scriptreview.go +++ b/internal/cli/scriptreview/scriptreview.go @@ -1,6 +1,7 @@ // Package scriptreview shows a script to the person running it, before it's sent to a rack, and asks them to // approve it: in the terminal, or in a macOS dialog when there's no terminal (an AI agent running the CLI). The PIN -// dialog is no place to read a script: it has room for a few lines, and gives up after a few minutes. +// dialog is no place to read a script: it has room for a few lines, and gives up after a few minutes. The review +// dialog can take the security key PIN instead, so approving the script and entering the PIN are one step. package scriptreview import ( @@ -30,8 +31,15 @@ type Request struct { Justification string // Script is the script and the command that runs it, as the gateway will record them. Script scriptrun.Script + // CheckPIN, when set, has the dialog ask for the security key PIN as well, and checks each PIN entered. A + // terminal ignores it: the PIN is asked for there when the command runs. + CheckPIN PINCheck } +// PINCheck checks a PIN entered in the review dialog. again is a reason to ask for the PIN again (a wrong PIN); +// err ends the review. +type PINCheck func(pin string) (again string, err error) + // ErrDeclined is a script the person didn't approve. var ErrDeclined = errors.New("the script wasn't approved to run") @@ -39,15 +47,16 @@ var ErrDeclined = errors.New("the script wasn't approved to run") // left waiting doesn't run long after it was asked for. const reviewTimeout = 15 * time.Minute -// Review shows req's script and returns nil once the person approves it, or ErrDeclined. -func Review(req Request) error { +// Review shows req's script and returns once the person approves it, with the PIN they entered when the dialog +// asked for one (see Request.CheckPIN), or "". A script that isn't approved returns ErrDeclined. +func Review(req Request) (string, error) { if _, ok := terminal.StdinFD(); ok { color := terminal.StderrIsTerminal() && os.Getenv("NO_COLOR") == "" - return reviewInTerminal(req, os.Stdin, os.Stderr, color) + return "", reviewInTerminal(req, os.Stdin, os.Stderr, color) } if runtime.GOOS != "darwin" { - return errors.New("a script needs approving before it runs, and there's no terminal to show it in: run the " + - "command in a terminal") + return "", errors.New("a script needs approving before it runs, and there's no terminal to show it in: run " + + "the command in a terminal") } _, _ = fmt.Fprintln(os.Stderr, "Waiting for the script to be approved in the rack-gateway dialog...") return reviewInDialog(context.Background(), runOsascript, req) @@ -113,27 +122,70 @@ func oneLine(text string) string { // dialogRunner runs a JavaScript for Automation script with osascript, passing args to its run handler. type dialogRunner func(ctx context.Context, script string, args ...string) (stdout, stderr string, err error) -// reviewInDialog shows the script in a macOS dialog and approves it when the person clicks Run. A dialog left -// open for reviewTimeout is closed, and the script isn't approved. -func reviewInDialog(ctx context.Context, run dialogRunner, req Request) error { +// pinInstructions tell the person what the PIN in the review dialog is for. +const pinInstructions = "Enter your security key PIN to run it. You'll touch the key once the command starts." + +// enterPIN asks again for a PIN when Run was clicked without one. +const enterPIN = "Enter your security key PIN to run the script." + +// reviewInDialog shows the script in a macOS dialog and approves it when the person clicks Run, after entering a +// PIN that req.CheckPIN accepts when it's set. It returns that PIN. A dialog left open for reviewTimeout (all its +// tries together) is closed, and the script isn't approved. +func reviewInDialog(ctx context.Context, run dialogRunner, req Request) (string, error) { ctx, cancel := context.WithTimeout(ctx, reviewTimeout) defer cancel() spans, _ := listing(req.Script) listingJSON, err := json.Marshal(spans) if err != nil { - return fmt.Errorf("failed to show the script review dialog: %w", err) + return "", fmt.Errorf("failed to show the script review dialog: %w", err) + } + dialog := reviewDialog{run: run, header: Header(req), listing: string(listingJSON)} + if req.CheckPIN == nil { + _, err := dialog.show(ctx, "") + return "", err + } + dialog.header += "\n\n" + pinInstructions + dialog.mode = "pin" + for problem := ""; ; { + pin, err := dialog.show(ctx, problem) + if err != nil { + return "", err + } + if pin == "" { + problem = enterPIN + continue + } + if problem, err = req.CheckPIN(pin); err != nil { + return "", err + } + if problem == "" { + return pin, nil + } } - stdout, stderr, err := run(ctx, dialogScript, Header(req), string(listingJSON)) +} + +// reviewDialog is the review dialog's arguments: the header, the listing (its spans, as JSON) and the mode ("pin" +// to ask for the security key PIN too, or ""). +type reviewDialog struct { + run dialogRunner + header, listing, mode string +} + +// show shows the dialog, with problem (why it's shown again, or "") as its title. It returns the PIN entered once +// Run is clicked, or ErrDeclined. +func (d reviewDialog) show(ctx context.Context, problem string) (string, error) { + stdout, stderr, err := d.run(ctx, dialogScript, d.header, d.listing, d.mode, problem) if errors.Is(ctx.Err(), context.DeadlineExceeded) { - return fmt.Errorf("%w: the review dialog timed out", ErrDeclined) + return "", fmt.Errorf("%w: the review dialog timed out", ErrDeclined) } if err != nil { - return fmt.Errorf("failed to show the script review dialog: %w: %s", err, strings.TrimSpace(stderr)) + return "", fmt.Errorf("failed to show the script review dialog: %w: %s", err, strings.TrimSpace(stderr)) } - if strings.TrimSpace(stdout) != "run" { - return ErrDeclined + answer, pin, _ := strings.Cut(strings.TrimSuffix(stdout, "\n"), "\n") + if answer != "run" { + return "", ErrDeclined } - return nil + return pin, nil } func runOsascript(ctx context.Context, script string, args ...string) (string, string, error) { @@ -145,10 +197,12 @@ func runOsascript(ctx context.Context, script string, args ...string) (string, s return stdout.String(), stderr.String(), err } -// dialogScript shows an alert with the header as its text and the listing (its spans, as JSON) in a scrollable, -// read-only, monospaced text view, colored by style in the system's colors for the current appearance (light or -// dark). It prints "run" when Run is clicked. Cancel is the default button (Return), so approving takes a click. -// The texts arrive as arguments, never as part of the script. +// dialogScript shows an alert, with a lock icon and the header as its text, and the listing (its spans, as JSON) in +// a scrollable, read-only, monospaced text view, colored by style in the system's colors for the current appearance +// (light or dark). It prints "run" when Run is clicked. Cancel is the default button (Return), so approving takes a +// click. In "pin" mode it has a security key PIN field under the listing, which has the focus, and Run is the +// default button: the PIN is the approval. It then prints "run", a line break and the PIN. A problem (a wrong PIN) +// replaces the title. The texts arrive as arguments, never as part of the script. const dialogScript = ` ObjC.import('Cocoa'); function run(argv) { @@ -172,13 +226,35 @@ function run(argv) { scroll.documentView = view; var alert = $.NSAlert.alloc.init; - alert.messageText = 'Approve this rack-gateway script?'; + // A 48-point lock.shield symbol, regular weight (0), large scale (3), instead of osascript's folder icon. + alert.icon = $.NSImage.imageWithSystemSymbolNameAccessibilityDescription('lock.shield', 'rack-gateway') + .imageWithSymbolConfiguration($.NSImageSymbolConfiguration.configurationWithPointSizeWeightScale(48, 0, 3)); + alert.messageText = argv[3] || 'Approve this rack-gateway script?'; alert.informativeText = argv[0]; - alert.accessoryView = scroll; - alert.addButtonWithTitle('Cancel'); + if (argv[2] !== 'pin') { + alert.accessoryView = scroll; + alert.addButtonWithTitle('Cancel'); + alert.addButtonWithTitle('Run'); + alert.window.initialFirstResponder = view; + return alert.runModal == $.NSAlertSecondButtonReturn ? 'run' : 'cancel'; + } + + var content = $.NSView.alloc.initWithFrame($.NSMakeRect(0, 0, 820, 500)); + scroll.setFrameOrigin($.NSMakePoint(0, 40)); + content.addSubview(scroll); + var label = $.NSTextField.labelWithString('Security key PIN:'); + label.setFrame($.NSMakeRect(0, 9, 120, 18)); + content.addSubview(label); + var field = $.NSSecureTextField.alloc.initWithFrame($.NSMakeRect(124, 6, 220, 24)); + content.addSubview(field); + alert.accessoryView = content; alert.addButtonWithTitle('Run'); - alert.window.initialFirstResponder = view; - return alert.runModal == $.NSAlertSecondButtonReturn ? 'run' : 'cancel'; + alert.addButtonWithTitle('Cancel'); + alert.window.initialFirstResponder = field; + if (alert.runModal != $.NSAlertFirstButtonReturn) { + return 'cancel'; + } + return 'run\n' + field.stringValue.js; } function listing(spans) { diff --git a/internal/cli/scriptreview/scriptreview_test.go b/internal/cli/scriptreview/scriptreview_test.go index 5cafb85e..bf5479cf 100644 --- a/internal/cli/scriptreview/scriptreview_test.go +++ b/internal/cli/scriptreview/scriptreview_test.go @@ -80,15 +80,15 @@ func TestReviewInDialog(t *testing.T) { gotScript, gotArgs = script, args return stdout, "", nil } - err := reviewInDialog(context.Background(), run, req) + pin, err := reviewInDialog(context.Background(), run, req) if approved { require.NoError(t, err) } else { require.ErrorIs(t, err, ErrDeclined) } + require.Empty(t, pin) require.Equal(t, dialogScript, gotScript) - require.Len(t, gotArgs, 2) - require.Equal(t, Header(req), gotArgs[0]) + require.Equal(t, []string{Header(req), gotArgs[1], "", ""}, gotArgs) var spans []span require.NoError(t, json.Unmarshal([]byte(gotArgs[1]), &spans)) require.Equal(t, "1 | puts 'run'", plainText(spans)) @@ -97,7 +97,58 @@ func TestReviewInDialog(t *testing.T) { failing := func(context.Context, string, ...string) (string, string, error) { return "", "execution error", errors.New("exit status 1") } - require.ErrorContains(t, reviewInDialog(context.Background(), failing, req), "execution error") + _, err := reviewInDialog(context.Background(), failing, req) + require.ErrorContains(t, err, "execution error") +} + +// answers is a dialog that gives each of its answers in turn, and records the problem each was asked with. +func answers(t *testing.T, stdouts ...string) (dialogRunner, *[]string) { + t.Helper() + var problems []string + return func(_ context.Context, _ string, args ...string) (string, string, error) { + require.Len(t, args, 4) + require.Equal(t, "pin", args[2]) + require.Contains(t, args[0], pinInstructions) + problems = append(problems, args[3]) + require.LessOrEqual(t, len(problems), len(stdouts), "the dialog was shown too often") + return stdouts[len(problems)-1], "", nil + }, &problems +} + +// With CheckPIN, the dialog asks for the security key PIN too: Run without a PIN, or with one CheckPIN rejects, +// shows it again with the reason. The PIN it accepts is returned exactly as typed. +func TestReviewInDialogAsksForThePIN(t *testing.T) { + req := testRequest(t, "puts 1\n") + var checked []string + req.CheckPIN = func(pin string) (string, error) { + checked = append(checked, pin) + if pin != " 1234 " { + return "wrong security key PIN: 7 tries left before the key locks", nil + } + return "", nil + } + run, problems := answers(t, "run\n\n", "run\n123456\n", "run\n 1234 \n") + pin, err := reviewInDialog(context.Background(), run, req) + require.NoError(t, err) + require.Equal(t, " 1234 ", pin) + require.Equal(t, []string{"123456", " 1234 "}, checked) + require.Equal(t, []string{"", enterPIN, "wrong security key PIN: 7 tries left before the key locks"}, *problems) +} + +// Cancel declines the script whatever was typed, and an error from CheckPIN (a locked key) ends the review. +func TestReviewInDialogWithPINDeclinedOrFailed(t *testing.T) { + req := testRequest(t, "puts 1\n") + req.CheckPIN = func(string) (string, error) { return "", errors.New("the key is locked") } + + run, _ := answers(t, "cancel\n") + pin, err := reviewInDialog(context.Background(), run, req) + require.ErrorIs(t, err, ErrDeclined) + require.Empty(t, pin) + + run, _ = answers(t, "run\n123456\n") + pin, err = reviewInDialog(context.Background(), run, req) + require.EqualError(t, err, "the key is locked") + require.Empty(t, pin) } // A dialog nobody answers is closed when it times out, and the script isn't approved. @@ -108,7 +159,7 @@ func TestReviewInDialogTimesOut(t *testing.T) { <-ctx.Done() return "", "", errors.New("signal: killed") } - err := reviewInDialog(ctx, waiting, testRequest(t, "puts 1\n")) + _, err := reviewInDialog(ctx, waiting, testRequest(t, "puts 1\n")) require.ErrorIs(t, err, ErrDeclined) require.ErrorContains(t, err, "the review dialog timed out") } diff --git a/internal/cli/webauthn/assertion_flow.go b/internal/cli/webauthn/assertion_flow.go index 252fee92..3cde37c7 100644 --- a/internal/cli/webauthn/assertion_flow.go +++ b/internal/cli/webauthn/assertion_flow.go @@ -188,7 +188,7 @@ func cannotListCredentials(err error) bool { func pinError(err error) error { switch { case errors.Is(err, libfido2.ErrPinInvalid): - return fmt.Errorf("wrong security key PIN (%w); try again", err) + return fmt.Errorf("%w (%w); try again", ErrWrongPIN, err) case errors.Is(err, libfido2.ErrPinAuthBlocked): return fmt.Errorf("too many wrong PINs in a row (%w); unplug the security key, plug it back in and try again", err) diff --git a/internal/cli/webauthn/pin_check.go b/internal/cli/webauthn/pin_check.go new file mode 100644 index 00000000..db2c6a42 --- /dev/null +++ b/internal/cli/webauthn/pin_check.go @@ -0,0 +1,82 @@ +//go:build !nofido + +package webauthn + +import ( + "errors" + "fmt" + "unicode/utf8" + + "github.com/keys-pub/go-libfido2" +) + +// minPINLength is the shortest PIN a FIDO2 security key takes. +const minPINLength = 4 + +// pinKey is what checking a PIN before an assertion needs from a security key. *libfido2.Device implements it. +type pinKey interface { + Info() (*libfido2.DeviceInfo, error) + CredentialsInfo(pin string) (*libfido2.CredentialsInfo, error) + RetryCount() (int, error) +} + +// SingleKeyHasPIN reports whether exactly one security key is connected and it has a PIN set. Its PIN can then be +// asked for before the gateway sends a challenge: it's the key that will sign it. With several keys connected, the +// key to use is only known from the challenge's credentials. +func SingleKeyHasPIN() bool { + keys, err := openPINKeys() + return err == nil && onlyKeyHasPIN(keys) +} + +// CheckPIN checks pin on the one connected security key, without a touch. A PIN the key refuses returns an error +// wrapping ErrWrongPIN, which says how many tries are left. A key that can't check a PIN this way (it has no +// credential management) returns nil, and the assertion checks the PIN. +func CheckPIN(pin string) error { + keys, err := openPINKeys() + if err != nil { + return err + } + if len(keys) != 1 { + return fmt.Errorf("%d security keys are connected: connect only the one you use with rack-gateway", len(keys)) + } + return verifyPIN(keys[0], pin) +} + +func openPINKeys() ([]pinKey, error) { + devices, err := openDevices() + if err != nil { + return nil, err + } + keys := make([]pinKey, 0, len(devices)) + for _, device := range devices { + keys = append(keys, device) + } + return keys, nil +} + +func onlyKeyHasPIN(keys []pinKey) bool { + if len(keys) != 1 { + return false + } + info, err := keys[0].Info() + return err == nil && optionEnabled(info, "clientPin") +} + +// verifyPIN asks key for its credentials' metadata with pin, which needs the right PIN and no touch. +func verifyPIN(key pinKey, pin string) error { + if utf8.RuneCountInString(pin) < minPINLength { + return fmt.Errorf("%w: a PIN has at least %d characters", ErrWrongPIN, minPINLength) + } + _, err := key.CredentialsInfo(pin) + if err == nil { + return nil + } + if errors.Is(err, libfido2.ErrPinInvalid) { + tries, countErr := key.RetryCount() + if countErr != nil { + return ErrWrongPIN + } + return fmt.Errorf("%w: %d tries left before the key locks", ErrWrongPIN, tries) + } + return pinError(err) +} diff --git a/internal/cli/webauthn/pin_check_test.go b/internal/cli/webauthn/pin_check_test.go new file mode 100644 index 00000000..90c462be --- /dev/null +++ b/internal/cli/webauthn/pin_check_test.go @@ -0,0 +1,82 @@ +//go:build !nofido + +package webauthn + +import ( + "errors" + "testing" + + "github.com/keys-pub/go-libfido2" + "github.com/stretchr/testify/require" +) + +// fakePINKey is a pinKey with canned answers. +type fakePINKey struct { + options []libfido2.Option + infoErr error + checkErr error + checked []string + tries int + triesErr error +} + +func (k *fakePINKey) Info() (*libfido2.DeviceInfo, error) { + if k.infoErr != nil { + return nil, k.infoErr + } + return &libfido2.DeviceInfo{Options: k.options}, nil +} + +func (k *fakePINKey) CredentialsInfo(pin string) (*libfido2.CredentialsInfo, error) { + k.checked = append(k.checked, pin) + if k.checkErr != nil { + return nil, k.checkErr + } + return &libfido2.CredentialsInfo{}, nil +} + +func (k *fakePINKey) RetryCount() (int, error) { + return k.tries, k.triesErr +} + +// The PIN can be asked for ahead only when one key is connected and it has a PIN: with several, the challenge's +// credentials say which one signs. +func TestOnlyKeyHasPIN(t *testing.T) { + withPIN := &fakePINKey{options: pinOptions} + require.True(t, onlyKeyHasPIN([]pinKey{withPIN})) + require.False(t, onlyKeyHasPIN([]pinKey{withPIN, &fakePINKey{options: pinOptions}})) + require.False(t, onlyKeyHasPIN(nil)) + require.False(t, onlyKeyHasPIN([]pinKey{&fakePINKey{}})) + require.False(t, onlyKeyHasPIN([]pinKey{&fakePINKey{options: pinOptions, infoErr: libfido2.ErrTX}})) +} + +// A wrong PIN says how many tries are left; a PIN too short to be one isn't sent to the key. A key that can't check +// a PIN this way leaves it to the assertion, and a locked one ends the review. +func TestVerifyPIN(t *testing.T) { + key := &fakePINKey{} + require.NoError(t, verifyPIN(key, "123456")) + require.Equal(t, []string{"123456"}, key.checked) + + key = &fakePINKey{checkErr: libfido2.ErrPinInvalid, tries: 6} + err := verifyPIN(key, "123457") + require.ErrorIs(t, err, ErrWrongPIN) + require.EqualError(t, err, "wrong security key PIN: 6 tries left before the key locks") + + key.triesErr = errors.New("no answer") + require.Equal(t, ErrWrongPIN, verifyPIN(key, "123457")) + + key = &fakePINKey{} + err = verifyPIN(key, "123") + require.ErrorIs(t, err, ErrWrongPIN) + require.ErrorContains(t, err, "a PIN has at least 4 characters") + require.Empty(t, key.checked) + + for _, unsupported := range []error{libfido2.ErrInvalidCommand, libfido2.ErrUnsupportedOption, libfido2.ErrTX} { + require.NoError(t, verifyPIN(&fakePINKey{checkErr: unsupported}, "123456")) + } + + err = verifyPIN(&fakePINKey{checkErr: libfido2.ErrPinAuthBlocked}, "123456") + require.ErrorIs(t, err, libfido2.ErrPinAuthBlocked) + require.NotErrorIs(t, err, ErrWrongPIN) + require.ErrorContains(t, err, "unplug the security key") +} diff --git a/internal/cli/webauthn/types.go b/internal/cli/webauthn/types.go index dc0864c0..de8068f3 100644 --- a/internal/cli/webauthn/types.go +++ b/internal/cli/webauthn/types.go @@ -2,7 +2,13 @@ // has no security key support (the nofido build tag). package webauthn -import "net/url" +import ( + "errors" + "net/url" +) + +// ErrWrongPIN is a security key PIN the key refused. +var ErrWrongPIN = errors.New("wrong security key PIN") // AssertionOptions contains the challenge and credential info from the server type AssertionOptions struct { diff --git a/internal/cli/webauthn/webauthn.go b/internal/cli/webauthn/webauthn.go index 776f481e..ca338668 100644 --- a/internal/cli/webauthn/webauthn.go +++ b/internal/cli/webauthn/webauthn.go @@ -56,6 +56,19 @@ func promptTouch(options AssertionOptions) { // openFIDODevices opens every connected security key that can be opened. func openFIDODevices() ([]securityKey, error) { + devices, err := openDevices() + if err != nil { + return nil, err + } + keys := make([]securityKey, 0, len(devices)) + for _, device := range devices { + keys = append(keys, device) + } + return keys, nil +} + +// openDevices opens every connected FIDO2 device that can be opened. +func openDevices() ([]*libfido2.Device, error) { locs, err := libfido2.DeviceLocations() if err != nil { return nil, fmt.Errorf("failed to list FIDO2 devices: %w", err) @@ -63,7 +76,7 @@ func openFIDODevices() ([]securityKey, error) { if len(locs) == 0 { return nil, fmt.Errorf("no FIDO2 devices found") } - keys := make([]securityKey, 0, len(locs)) + devices := make([]*libfido2.Device, 0, len(locs)) var openErr error for _, loc := range locs { device, err := libfido2.NewDevice(loc.Path) @@ -71,12 +84,12 @@ func openFIDODevices() ([]securityKey, error) { openErr = err continue } - keys = append(keys, device) + devices = append(devices, device) } - if len(keys) == 0 { + if len(devices) == 0 { return nil, fmt.Errorf("failed to open FIDO2 device: %w", openErr) } - return keys, nil + return devices, nil } func decodeCredentialIDs(ids []string) ([][]byte, error) { diff --git a/internal/cli/webauthn/webauthn_stub.go b/internal/cli/webauthn/webauthn_stub.go index da2d096e..2c97118f 100644 --- a/internal/cli/webauthn/webauthn_stub.go +++ b/internal/cli/webauthn/webauthn_stub.go @@ -18,3 +18,13 @@ func GetAssertionWithCachedPIN(options AssertionOptions, cachedPIN string) (*Ass func CheckAvailability() bool { return false } + +// SingleKeyHasPIN stub - no security key support in this build +func SingleKeyHasPIN() bool { + return false +} + +// CheckPIN stub - no security key support in this build +func CheckPIN(string) error { + return nil +} From 6fb5b454339c340048deb14cc88ad813e57c1ca5 Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Sun, 25 Oct 2026 17:23:03 +1300 Subject: [PATCH 2/3] chore: bump version to v0.1.22 --- convox.yml | 4 ++-- web/package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/convox.yml b/convox.yml index e868caa3..25f4b979 100644 --- a/convox.yml +++ b/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 diff --git a/web/package.json b/web/package.json index bfc7cfef..ad679f1e 100644 --- a/web/package.json +++ b/web/package.json @@ -1,6 +1,6 @@ { "name": "web", - "version": "0.1.21", + "version": "0.1.22", "private": true, "type": "module", "scripts": { From 5e2a6aefcaec8fa2d306038d754fc76360e1440e Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Sun, 25 Oct 2026 17:32:24 +1300 Subject: [PATCH 3/3] docs: the review dialog shows the tries left when the key reports them --- docs/src/content/docs/user-guide/cli/commands.mdx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/src/content/docs/user-guide/cli/commands.mdx b/docs/src/content/docs/user-guide/cli/commands.mdx index 74b3ffac..3f2ff798 100644 --- a/docs/src/content/docs/user-guide/cli/commands.mdx +++ b/docs/src/content/docs/user-guide/cli/commands.mdx @@ -376,9 +376,9 @@ fit in the audit log (50 KB). 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 - 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. + 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. 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