diff --git a/internal/cmd/mcp.go b/internal/cmd/mcp.go index 5df2866..cb00799 100644 --- a/internal/cmd/mcp.go +++ b/internal/cmd/mcp.go @@ -38,15 +38,16 @@ Configure in Claude Desktop: func newMcpServeCmd() *cobra.Command { var ( - rateLimit int - readOnly bool - transport string - addr string - port int - corsOrigin string - requireAuth bool - tlsCert string - tlsKey string + rateLimit int + readOnly bool + transport string + addr string + port int + corsOrigin string + requireAuth bool + requireStepUp bool + tlsCert string + tlsKey string ) cmd := &cobra.Command{ @@ -113,6 +114,10 @@ Use JC_PROFILE environment variable to select which JumpCloud org to use.`, if !cmd.Flags().Changed("port") { port = config.MCPSSEPort() } + if !cmd.Flags().Changed("require-step-up") { + requireStepUp = config.MCPRequireStepUp() + } + // Profile-role enforcement: a profile bound to a read-only OAuth // client must not advertise mutation tools. Reject the start // rather than silently coercing — operators who passed @@ -130,7 +135,18 @@ Use JC_PROFILE environment variable to select which JumpCloud org to use.`, if warning != "" { fmt.Fprintln(cmd.ErrOrStderr(), warning) } - return runMcpServe(rateLimit, readOnly, transport, addr, port, corsOrigin, tlsCert, tlsKey, requireAuth) + + // Stdio transport hands stdin/stdout to the JSON-RPC stream; + // a TTY-based step-up prompt has nowhere to go. Warn the + // operator at startup so destructive calls don't silently + // fail closed with "no challenge channel available." + if requireStepUp && transport == "stdio" { + fmt.Fprintln(cmd.ErrOrStderr(), + "jc: --require-step-up is set but transport is stdio; TTY prompts cannot reach the operator. "+ + "Destructive calls will be rejected as 'step-up unavailable'. Use --transport http with a terminal "+ + "session, or wait for an out-of-band authenticator (KLA-408 Slice 3).") + } + return runMcpServe(rateLimit, readOnly, transport, addr, port, corsOrigin, tlsCert, tlsKey, requireAuth, requireStepUp) }, } @@ -143,6 +159,7 @@ Use JC_PROFILE environment variable to select which JumpCloud org to use.`, cmd.Flags().StringVar(&tlsCert, "tls-cert", "", "TLS certificate file for SSE transport") cmd.Flags().StringVar(&tlsKey, "tls-key", "", "TLS private key file for SSE transport") cmd.Flags().BoolVar(&requireAuth, "require-auth", false, "Require x-api-key / Authorization Bearer on every request (http transport). Off by default so local browser clients like basic-host can connect. Turn on when exposing via a tunnel.") + cmd.Flags().BoolVar(&requireStepUp, "require-step-up", false, "Require step-up auth (last-6 of API key prompt) before any destructive tool with execute=true fires. Only effective when stdin is a TTY; not usable in stdio transport mode. Defaults to mcp.require_step_up_for_destructive in config.") return cmd } @@ -220,13 +237,28 @@ func applyProfileRole(activeProfile string, profileReadOnly, flagChanged, readOn return true, fmt.Sprintf("Profile %q is read-only — forcing --read-only and rejecting destructive tools.", activeProfile), nil } -func runMcpServe(rateLimit int, readOnly bool, transport, addr string, port int, corsOrigin, tlsCert, tlsKey string, requireAuth bool) error { +func runMcpServe(rateLimit int, readOnly bool, transport, addr string, port int, corsOrigin, tlsCert, tlsKey string, requireAuth, requireStepUp bool) error { + // Step-up auth needs an API key to derive the challenge answer. Gate + // the read on the explicit opt-in flag and fail-fast at startup if + // it's missing — matches the --require-auth pattern below so an + // operator who turns step-up on without a credential gets a clear + // startup error instead of silent runtime denials. + var stepUpAPIKey string + if requireStepUp { + stepUpAPIKey = config.APIKey() + if stepUpAPIKey == "" { + return fmt.Errorf("--require-step-up needs an API key to derive the challenge answer. Run 'jc auth login' or set JC_API_KEY, or drop --require-step-up") + } + } + server := mcp.NewServer(mcp.Options{ - RateLimit: rateLimit, - ReadOnly: readOnly, - AuditEnabled: config.MCPAuditLog(), - AllowedTools: config.MCPAllowedTools(), - BlockedTools: config.MCPBlockedTools(), + RateLimit: rateLimit, + ReadOnly: readOnly, + AuditEnabled: config.MCPAuditLog(), + AllowedTools: config.MCPAllowedTools(), + BlockedTools: config.MCPBlockedTools(), + RequireStepUp: requireStepUp, + StepUpAPIKey: stepUpAPIKey, }) // Handle graceful shutdown on Ctrl+C / SIGTERM. diff --git a/internal/cmd/mcp_test.go b/internal/cmd/mcp_test.go index 8ea84b1..471fea7 100644 --- a/internal/cmd/mcp_test.go +++ b/internal/cmd/mcp_test.go @@ -157,6 +157,14 @@ func TestMcpServeCmd_FlagDefaults(t *testing.T) { if cors.DefValue != "" { t.Errorf("expected cors-origin default empty, got %s", cors.DefValue) } + + stepUp := mcpCmd.Flags().Lookup("require-step-up") + if stepUp == nil { + t.Fatal("expected require-step-up flag") + } + if stepUp.DefValue != "false" { + t.Errorf("expected require-step-up default false, got %s", stepUp.DefValue) + } } func TestRootCmd_IncludesMcp(t *testing.T) { diff --git a/internal/config/config.go b/internal/config/config.go index e23cedb..9162593 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -35,6 +35,7 @@ mcp: read_only: false audit_log: true plan_first: true + require_step_up_for_destructive: false profiles: default: @@ -98,6 +99,7 @@ func setDefaults() { viper.SetDefault("mcp.read_only", false) viper.SetDefault("mcp.audit_log", true) viper.SetDefault("mcp.plan_first", true) + viper.SetDefault("mcp.require_step_up_for_destructive", false) viper.SetDefault("ask.provider", "disabled") viper.SetDefault("ask.api_key", "") viper.SetDefault("ask.model", "") @@ -393,6 +395,7 @@ var ValidConfigKeys = []string{ "mcp.read_only", "mcp.audit_log", "mcp.plan_first", + "mcp.require_step_up_for_destructive", "mcp.sse_port", "mcp.allowed_tools", "mcp.blocked_tools", @@ -425,7 +428,8 @@ func coerceValue(key, value string) interface{} { return n } case "defaults.confirm_destructive", "defaults.color", "cache.enabled", - "mcp.read_only", "mcp.audit_log", "mcp.plan_first", "ask.confirm_before_execute": + "mcp.read_only", "mcp.audit_log", "mcp.plan_first", + "mcp.require_step_up_for_destructive", "ask.confirm_before_execute": // Attempt bool conversion. switch strings.ToLower(value) { case "true", "1", "yes": @@ -457,6 +461,15 @@ func MCPPlanFirst() bool { return viper.GetBool("mcp.plan_first") } +// MCPRequireStepUp returns true if destructive MCP tool invocations +// (any tool argument with Execute: true) must clear a step-up +// authentication challenge before firing the underlying API call. +// When enabled, the chokepoint in addTypedTool blocks the call until +// the configured authenticator approves it. +func MCPRequireStepUp() bool { + return viper.GetBool("mcp.require_step_up_for_destructive") +} + // MCPSSEPort returns the configured SSE transport port (default 8080). func MCPSSEPort() int { port := viper.GetInt("mcp.sse_port") diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 74ab80b..dacbbaa 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -29,6 +29,7 @@ type Server struct { auditLog *auditLogger readOnly bool toolFilter *toolFilter + stepUp stepUpAuthenticator toolNames []string // registered tool names, in registration order mu sync.Mutex @@ -53,6 +54,20 @@ type Options struct { // BlockedTools is a list of glob patterns for tools that are blocked. // Block list takes precedence over allow list. BlockedTools []string + // RequireStepUp gates every destructive tool invocation (any tool + // argument carrying Execute: true) behind a fresh proof of operator + // presence. When true, the server prompts on its controlling TTY for + // the last 6 chars of StepUpAPIKey before each destructive call. + RequireStepUp bool + // StepUpAPIKey is the credential the TTY authenticator uses to derive + // the prompt's expected answer. Typically the same key the server is + // authenticated with (config.APIKey()). Required when RequireStepUp + // is true; ignored otherwise. + StepUpAPIKey string + // stepUp injects a custom authenticator. Reserved for tests within + // the mcp package; production callers configure step-up via + // RequireStepUp + StepUpAPIKey. + stepUp stepUpAuthenticator } // nowFunc is overridable for tests. @@ -101,12 +116,18 @@ func NewServer(opts Options) *Server { al = &auditLogger{} // no-op: enc is nil, log() returns early } + stepUp := opts.stepUp + if stepUp == nil { + stepUp = newStepUp(opts.RequireStepUp, opts.StepUpAPIKey) + } + s := &Server{ mcpServer: mcpServer, limiter: newRateLimiter(opts.RateLimit), auditLog: al, readOnly: opts.ReadOnly, toolFilter: newToolFilter(opts.AllowedTools, opts.BlockedTools), + stepUp: stepUp, } s.registerTools() diff --git a/internal/mcp/stepup.go b/internal/mcp/stepup.go new file mode 100644 index 0000000..60f500e --- /dev/null +++ b/internal/mcp/stepup.go @@ -0,0 +1,187 @@ +package mcp + +import ( + "bufio" + "context" + "errors" + "fmt" + "io" + "os" + "reflect" + "strings" + "sync" + + "golang.org/x/term" +) + +// isExecutingDestructive returns true if the tool argument carries an +// Execute: true field — the codebase's signal that a tool is about to +// mutate state at the JumpCloud API. The signal is structural: every +// destructive tool input embeds an `Execute bool` field, by convention +// (see `destructiveInput`, `userUpdateInput`, `commandRunInput`, etc.). +func isExecutingDestructive(args any) bool { + v := reflect.ValueOf(args) + if v.Kind() == reflect.Pointer { + if v.IsNil() { + return false + } + v = v.Elem() + } + if v.Kind() != reflect.Struct { + return false + } + f := v.FieldByName("Execute") + if !f.IsValid() || f.Kind() != reflect.Bool { + return false + } + return f.Bool() +} + +// destructiveTarget returns a short string that identifies the resource +// the destructive operation will hit (e.g. the username for users_*, +// the device hostname for devices_*). Used purely for the human-facing +// step-up prompt — never persisted, never sent to the JumpCloud API. +// +// We try a small set of well-known field names in priority order. If +// none are present, return an empty string and the prompt skips the +// "on " clause. +func destructiveTarget(args any) string { + v := reflect.ValueOf(args) + if v.Kind() == reflect.Pointer { + if v.IsNil() { + return "" + } + v = v.Elem() + } + if v.Kind() != reflect.Struct { + return "" + } + for _, name := range []string{"Identifier", "Member", "Group", "Target", "Username", "DeviceID", "Command"} { + f := v.FieldByName(name) + if f.IsValid() && f.Kind() == reflect.String && f.String() != "" { + return f.String() + } + } + return "" +} + +// stepUpAuthenticator gates a destructive MCP operation behind a fresh +// proof of operator presence. Implementations decide what "fresh proof" +// means: a TTY prompt, a Touch ID popup, an out-of-band approval, etc. +// +// The chokepoint in addTypedTool calls authorize() once per destructive +// tool invocation (any tool argument carrying Execute: true). A non-nil +// error blocks the underlying API call. +type stepUpAuthenticator interface { + authorize(ctx context.Context, toolName, target string) error +} + +// noopStepUp permits every operation. Used when the feature is disabled +// (the default) so the chokepoint adds zero overhead. +type noopStepUp struct{} + +func (noopStepUp) authorize(context.Context, string, string) error { return nil } + +// errStepUpDenied indicates the operator declined the prompt. Distinct +// from infrastructure errors so the audit log can record the difference. +var errStepUpDenied = errors.New("operator denied step-up auth challenge") + +// errStepUpUnavailable indicates the authenticator could not present a +// challenge (no TTY in stdio mode, missing keychain key, etc.). The +// caller treats this as a deny — fail closed. +var errStepUpUnavailable = errors.New("step-up auth required but no challenge channel is available") + +// ttyStepUp prompts on the controlling terminal for the last 6 chars of +// the configured API key. It's intentionally a weak challenge — anyone +// holding the key answers the same prompt — so its real value is in +// (a) catching an autonomous agent that flipped Execute: true without +// the operator noticing, and (b) forcing an in-the-loop pause on +// destructive ops. +// +// Stronger authenticators (Touch ID, push-approval) plug into the same +// interface and supersede this one when the platform supports them. +type ttyStepUp struct { + mu sync.Mutex + in io.Reader // defaults to os.Stdin; injectable for tests + out io.Writer // defaults to os.Stderr + stdinFD int // for IsTerminal check; -1 to skip the check (tests) + apiKey string // captured at server start so prompt-time changes don't shift the answer + expectLen int // last-N chars to ask for (default 6) +} + +// newTTYStepUp constructs a TTY-prompt authenticator. apiKey is the +// credential used to derive the challenge answer; if empty, every +// authorize() call returns errStepUpUnavailable. +func newTTYStepUp(apiKey string) *ttyStepUp { + return &ttyStepUp{ + in: os.Stdin, + out: os.Stderr, + stdinFD: int(os.Stdin.Fd()), + apiKey: apiKey, + expectLen: 6, + } +} + +func (t *ttyStepUp) authorize(ctx context.Context, toolName, target string) error { + // Serialize prompts: concurrent destructive ops would interleave on + // stdin and produce nonsense. The MCP transport may multiplex tool + // calls, so this lock matters. + t.mu.Lock() + defer t.mu.Unlock() + + if t.apiKey == "" { + return errStepUpUnavailable + } + if len(t.apiKey) < t.expectLen { + // Defensively fail-closed: a too-short key can't produce a + // meaningful challenge. + return errStepUpUnavailable + } + + // stdinFD -1 skips the terminal check (test injection). In + // production, refuse if stdin isn't a TTY — the prompt would + // otherwise block forever or read from a JSON-RPC pipe in stdio + // transport mode. + if t.stdinFD >= 0 && !term.IsTerminal(t.stdinFD) { + return errStepUpUnavailable + } + + expected := t.apiKey[len(t.apiKey)-t.expectLen:] + + targetClause := "" + if target != "" { + targetClause = fmt.Sprintf(" on %s", target) + } + fmt.Fprintf(t.out, + "\nStep-up auth: %s%s\nEnter the last %d characters of the API key to confirm (or 'no' to deny): ", + toolName, targetClause, t.expectLen) + + // We can't easily call term.ReadPassword here without taking over + // the controlling terminal — and the value isn't a secret in the + // usual sense (the operator already knows it). A line read on stdin + // is enough. + reader := bufio.NewReader(t.in) + line, err := reader.ReadString('\n') + if err != nil { + return fmt.Errorf("reading step-up response: %w", err) + } + line = strings.TrimSpace(line) + + if strings.EqualFold(line, "no") || strings.EqualFold(line, "n") || strings.EqualFold(line, "deny") { + return errStepUpDenied + } + if line != expected { + return errStepUpDenied + } + return nil +} + +// newStepUp returns the authenticator a Server should use given the +// requested configuration. Callers that haven't enabled the feature +// get noopStepUp (zero-cost path). +func newStepUp(required bool, apiKey string) stepUpAuthenticator { + if !required { + return noopStepUp{} + } + return newTTYStepUp(apiKey) +} diff --git a/internal/mcp/stepup_test.go b/internal/mcp/stepup_test.go new file mode 100644 index 0000000..29e82dd --- /dev/null +++ b/internal/mcp/stepup_test.go @@ -0,0 +1,264 @@ +package mcp + +import ( + "bytes" + "context" + "errors" + "strings" + "sync/atomic" + "testing" +) + +// recordingStepUp captures the (toolName, target) of every call and +// returns the configured response. Used by integration tests to verify +// the chokepoint actually invokes the authenticator on Execute: true +// destructive calls and skips it for everything else. +type recordingStepUp struct { + calls atomic.Int64 + lastTool string + lastArg string + deny bool +} + +func (r *recordingStepUp) authorize(_ context.Context, toolName, target string) error { + r.calls.Add(1) + r.lastTool = toolName + r.lastArg = target + if r.deny { + return errStepUpDenied + } + return nil +} + +// stepUpFixture builds a ttyStepUp wired to in-memory I/O and with the +// TTY check skipped, so we can drive the prompt deterministically. +func stepUpFixture(apiKey, response string) (*ttyStepUp, *bytes.Buffer) { + out := new(bytes.Buffer) + su := &ttyStepUp{ + in: strings.NewReader(response), + out: out, + stdinFD: -1, // skip TTY check in tests + apiKey: apiKey, + expectLen: 6, + } + return su, out +} + +func TestNoopStepUp_AlwaysAllows(t *testing.T) { + if err := (noopStepUp{}).authorize(context.Background(), "users_delete", "alice"); err != nil { + t.Errorf("noopStepUp.authorize returned %v, want nil", err) + } +} + +func TestNewStepUp_DisabledReturnsNoop(t *testing.T) { + a := newStepUp(false, "anything") + if _, ok := a.(noopStepUp); !ok { + t.Errorf("newStepUp(false) = %T, want noopStepUp", a) + } +} + +func TestNewStepUp_EnabledReturnsTTY(t *testing.T) { + a := newStepUp(true, "key12345678") + if _, ok := a.(*ttyStepUp); !ok { + t.Errorf("newStepUp(true) = %T, want *ttyStepUp", a) + } +} + +func TestTTYStepUp_CorrectFragmentAllows(t *testing.T) { + // Last 6 chars of "abcdef-12345678" are "345678". + su, out := stepUpFixture("abcdef-12345678", "345678\n") + + if err := su.authorize(context.Background(), "users_delete", "alice"); err != nil { + t.Fatalf("authorize() error: %v", err) + } + if !strings.Contains(out.String(), "users_delete") { + t.Errorf("prompt missing tool name: %q", out.String()) + } + if !strings.Contains(out.String(), "alice") { + t.Errorf("prompt missing target: %q", out.String()) + } +} + +func TestTTYStepUp_WrongFragmentDenies(t *testing.T) { + su, _ := stepUpFixture("abcdef-12345678", "wrong1\n") + + err := su.authorize(context.Background(), "users_delete", "alice") + if !errors.Is(err, errStepUpDenied) { + t.Errorf("authorize() = %v, want errStepUpDenied", err) + } +} + +func TestTTYStepUp_ExplicitNoDenies(t *testing.T) { + for _, response := range []string{"no\n", "n\n", "NO\n", "deny\n", "Deny\n"} { + su, _ := stepUpFixture("abcdef-12345678", response) + err := su.authorize(context.Background(), "users_delete", "alice") + if !errors.Is(err, errStepUpDenied) { + t.Errorf("for response %q: got %v, want errStepUpDenied", strings.TrimSpace(response), err) + } + } +} + +func TestTTYStepUp_EmptyKeyUnavailable(t *testing.T) { + su, _ := stepUpFixture("", "anything\n") + err := su.authorize(context.Background(), "users_delete", "alice") + if !errors.Is(err, errStepUpUnavailable) { + t.Errorf("authorize() = %v, want errStepUpUnavailable", err) + } +} + +func TestTTYStepUp_TooShortKeyUnavailable(t *testing.T) { + // 5 chars < expectLen=6 + su, _ := stepUpFixture("short", "short\n") + err := su.authorize(context.Background(), "users_delete", "alice") + if !errors.Is(err, errStepUpUnavailable) { + t.Errorf("authorize() = %v, want errStepUpUnavailable", err) + } +} + +func TestTTYStepUp_PromptOmitsTargetClauseWhenEmpty(t *testing.T) { + su, out := stepUpFixture("abcdef-12345678", "345678\n") + if err := su.authorize(context.Background(), "policies_delete", ""); err != nil { + t.Fatalf("authorize() error: %v", err) + } + if strings.Contains(out.String(), " on ") { + t.Errorf("prompt should omit ' on ' when target is empty, got: %q", out.String()) + } +} + +func TestIsExecutingDestructive(t *testing.T) { + cases := []struct { + name string + args any + want bool + }{ + {"destructiveInput-execute-true", destructiveInput{Identifier: "alice", Execute: true}, true}, + {"destructiveInput-execute-false", destructiveInput{Identifier: "alice", Execute: false}, false}, + {"destructiveInput-pointer", &destructiveInput{Identifier: "alice", Execute: true}, true}, + {"non-destructive-listInput", listInput{}, false}, + {"plain-string", "not a struct", false}, + {"nil-pointer", (*destructiveInput)(nil), false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := isExecutingDestructive(c.args); got != c.want { + t.Errorf("isExecutingDestructive(%T) = %v, want %v", c.args, got, c.want) + } + }) + } +} + +func TestDestructiveTarget(t *testing.T) { + cases := []struct { + name string + args any + want string + }{ + {"identifier-set", destructiveInput{Identifier: "alice", Execute: true}, "alice"}, + {"identifier-empty", destructiveInput{Identifier: "", Execute: true}, ""}, + {"membership-input", membershipInput{Group: "Engineering", Member: "alice", Execute: true}, "alice"}, + {"non-struct", "string", ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := destructiveTarget(c.args); got != c.want { + t.Errorf("destructiveTarget(%T) = %q, want %q", c.args, got, c.want) + } + }) + } +} + +// Integration: confirm the chokepoint in addTypedTool actually consults +// the step-up authenticator on Execute: true and blocks the call when +// the authenticator denies. + +func TestChokepoint_DestructiveExecuteCallsStepUp(t *testing.T) { + setupToolTest(t) + + users := []map[string]any{ + {"_id": "aabbccddee112233aabbcc01", "username": "alice"}, + } + ts := startV1Server(t, users, nil, nil) + overrideV1ClientForTest(t, ts.URL) + + rec := &recordingStepUp{} + cs := connectToolTestServer(t, Options{stepUp: rec}) + + result := callTool(t, cs, "users_delete", map[string]any{ + "identifier": "aabbccddee112233aabbcc01", + "execute": true, + }) + if result.IsError { + t.Fatalf("expected success, got error: %s", getResultText(t, result)) + } + if got := rec.calls.Load(); got != 1 { + t.Errorf("step-up calls = %d, want 1", got) + } + if rec.lastTool != "users_delete" { + t.Errorf("step-up tool = %q, want users_delete", rec.lastTool) + } +} + +func TestChokepoint_DenialBlocksDestructiveCall(t *testing.T) { + setupToolTest(t) + + rec := &recordingStepUp{deny: true} + cs := connectToolTestServer(t, Options{stepUp: rec}) + + result := callTool(t, cs, "users_delete", map[string]any{ + "identifier": "aabbccddee112233aabbcc01", + "execute": true, + }) + if !result.IsError { + t.Fatal("expected destructive call to be blocked when step-up denies; got success") + } + text := getResultText(t, result) + if !strings.Contains(text, "step-up auth required") { + t.Errorf("error result missing step-up message: %q", text) + } + if got := rec.calls.Load(); got != 1 { + t.Errorf("step-up calls = %d, want 1", got) + } +} + +func TestChokepoint_PlanModeBypassesStepUp(t *testing.T) { + // users_delete with execute=false (plan mode) is not destructive + // from the chokepoint's perspective — the step-up gate must NOT fire. + setupToolTest(t) + + rec := &recordingStepUp{deny: true} + cs := connectToolTestServer(t, Options{stepUp: rec}) + + result := callTool(t, cs, "users_delete", map[string]any{ + "identifier": "aabbccddee112233aabbcc01", + // execute deliberately omitted (defaults to false) + }) + if result.IsError { + t.Fatalf("plan mode should not error: %s", getResultText(t, result)) + } + if got := rec.calls.Load(); got != 0 { + t.Errorf("step-up calls = %d, want 0 (plan mode shouldn't trigger step-up)", got) + } +} + +func TestChokepoint_NonDestructiveBypassesStepUp(t *testing.T) { + // users_list has no Execute field — chokepoint must treat it as + // non-destructive and skip step-up entirely. + setupToolTest(t) + + users := []map[string]any{ + {"_id": "aabbccddee112233aabbcc01", "username": "alice"}, + } + ts := startV1Server(t, users, nil, nil) + overrideV1ClientForTest(t, ts.URL) + + rec := &recordingStepUp{deny: true} + cs := connectToolTestServer(t, Options{stepUp: rec}) + + result := callTool(t, cs, "users_list", map[string]any{}) + if result.IsError { + t.Fatalf("users_list should not error: %s", getResultText(t, result)) + } + if got := rec.calls.Load(); got != 0 { + t.Errorf("step-up calls = %d, want 0 (non-destructive tool)", got) + } +} diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index 7369fa2..b2cebfb 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -5162,6 +5162,20 @@ func addTypedTool[In any](s *Server, name, description string, handler func(ctx return errorResult(err.Error()), nil, nil } + // Step-up auth gate: any tool input carrying Execute: true is a + // destructive operation about to fire against the JumpCloud API. + // When the operator opted in via mcp.require_step_up_for_destructive, + // challenge them before the call. Reflection lets a single + // chokepoint cover all 30+ destructive tool types without a + // per-handler hook. + if isExecutingDestructive(args) { + if err := s.stepUp.authorize(ctx, name, destructiveTarget(args)); err != nil { + msg := fmt.Sprintf("step-up auth required for %s: %v", name, err) + s.auditLog.log(name, req.Params.Arguments, false, msg) + return errorResult(msg), nil, nil + } + } + // Execute the tool. result, out, err := handler(ctx, req, args)