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/docs/src/content/docs/user-guide/cli/commands.mdx b/docs/src/content/docs/user-guide/cli/commands.mdx index 5c5962ec..3f2ff798 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 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 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 +} 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": {