From 965a444672fd19f5b4ae30402b9630c06d9ebff4 Mon Sep 17 00:00:00 2001 From: Juergen Klaassen Date: Tue, 28 Apr 2026 13:49:52 -0600 Subject: [PATCH 1/2] feat(mcp): Ed25519 op-envelope signing for destructive ops (KLA-411) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Direct response to the user feedback ask in KLA-408: > For critical tools or actions, such as deletion in a production > environment, it would be useful to request JIT authz to have a > cryptographic proof of the operation so that responsibility can be > traced. Phase 1 (PR #23, v1.16.0) shipped TTY step-up. This slice adds the *cryptographic proof* piece — every successful destructive MCP op emits a signed manifest to a forensic audit log that's hard to forge without also stealing the per-profile keychain entry. What ships: - internal/mcp/sign.go — manifestSigner interface, noopSigner (zero cost when disabled), ed25519Signer, VerifyManifestStream() helper. Pure Go, no cgo. Thread-safe via sync.Mutex. - Lazy per-profile keypair generation on first signed op. Private key in OS keychain (service="jc", account=":signing_key"); pubkey in config (profiles..signing_pubkey). No plaintext fallback — fail closed if keychain unavailable. - Manifest shape: { tool, args_redacted, target, timestamp, nonce, operator_pubkey, signature }. Sensitive params are redacted via the existing audit-log redaction pass before signing. - internal/keychain/keychain.go — SetSigningKey/GetSigningKey/Delete helpers, mirroring the client-secret pattern. - internal/config/config.go — mcp.sign_destructive_ops bool config, MCPSignDestructiveOps() getter, ValidConfigKeys + coerceValue. SigningPubkey()/SetSigningPubkey() for profile-level pubkey access. - internal/mcp/tools.go — chokepoint in addTypedTool's wrappedHandler emits a signed manifest *after* a destructive op succeeds. Failures don't sign (the op didn't happen). Signing errors warn on stderr but do not roll back the op (the API call already fired). - internal/cmd/mcp.go — --sign-destructive flag wired through to Options.SignDestructiveOps + SigningProfile. - internal/cmd/audit.go (new) — `jc audit verify` subcommand with --profile / --log / --pubkey flags. Returns OK + count on success; exits non-zero with offending manifest's nonce on tamper. - docs/AUTH.md — new "Signed audit log" section, updated threat-model table to reflect tamper-evident detection on credential exfiltration and compromised-agent scenarios. Tests: - 10 sign unit tests (noop, factory, signer round-trip, keypair reuse across ops, nonce uniqueness, verify happy path, tamper detection, wrong-pubkey rejection, sensitive-arg redaction) - 3 chokepoint integration tests (signs successful destructive ops, skips failed ops, skips plan mode) - 4 audit-verify CLI tests (OK path, tamper detection exits non-zero, no-pubkey error, command registration) - Flag-defaults test for --sign-destructive This closes the original "cryptographic proof" half of the user feedback. The remaining slices (Touch ID for stdio-mode prompts, out-of-band approval for dual-control workflows) are KLA-412 and KLA-413 respectively. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/AUTH.md | 40 +++- internal/cmd/audit.go | 92 +++++++++ internal/cmd/audit_test.go | 179 ++++++++++++++++++ internal/cmd/mcp.go | 45 +++-- internal/cmd/mcp_test.go | 8 + internal/cmd/root.go | 2 +- internal/config/config.go | 32 +++- internal/keychain/keychain.go | 32 ++++ internal/mcp/server.go | 23 +++ internal/mcp/sign.go | 247 +++++++++++++++++++++++++ internal/mcp/sign_test.go | 338 ++++++++++++++++++++++++++++++++++ internal/mcp/tools.go | 14 ++ 12 files changed, 1021 insertions(+), 31 deletions(-) create mode 100644 internal/cmd/audit.go create mode 100644 internal/cmd/audit_test.go create mode 100644 internal/mcp/sign.go create mode 100644 internal/mcp/sign_test.go diff --git a/docs/AUTH.md b/docs/AUTH.md index e3f9a53..cc408a7 100644 --- a/docs/AUTH.md +++ b/docs/AUTH.md @@ -277,13 +277,33 @@ Before a record is written, parameters are walked and any of the following keys The redaction is shallow (top-level keys only). Tool authors who add new sensitive fields must add them to `sensitiveParamKeys` in `internal/mcp/server.go`. -### Limitations +### Signed audit log (opt-in) -- **No per-operator identity is captured.** The audit log says "the credential called users_delete," not "Jane called users_delete." Two operators sharing one API key are indistinguishable in the log. -- **No signature.** A laptop-local attacker who can write to the log file can forge or scrub entries. -- **Stdio transport carries no session metadata** — there is no client identifier in the record beyond the tool call itself. +Enable `mcp.sign_destructive_ops: true` (or pass `--sign-destructive` to `jc mcp serve`) to emit a tamper-evident manifest to `~/.config/jc/mcp-audit-signed.log` for every successful destructive op. Each manifest carries an Ed25519 signature over the tool name, redacted args, target, timestamp, and a 32-byte nonce. + +Storage: + +- A per-profile keypair is generated lazily on the first signed op. +- Private key lives in the OS keychain at `service="jc"`, account `":signing_key"`. No plaintext fallback — fail closed if the keychain is unavailable. +- Public key is persisted to config at `profiles..signing_pubkey` so verifiers can trust the chain on first use without consulting the keychain. -This is tracked in [KLA-408](https://linear.app/klaassenconsulting/issue/KLA-408): the proposed Slice 2 is a signed manifest per destructive op (Ed25519 signature over `{tool, args, target_ids, timestamp, nonce, operator_pubkey}`). +Verification: + +```bash +jc audit verify # active profile, default log path +jc audit verify --profile production # named profile +jc audit verify --pubkey # override the configured pubkey +jc audit verify --log /path/to/audit.log # alternate log path +``` + +A successful run reports the count of verified records and exits 0; any signature mismatch, truncation, or decode error exits non-zero with the offending manifest's nonce + tool so operators can grep the file. + +This is detection, not prevention: a successful credential exfiltration still produces forensic record signed with the original keypair, which is hard to forge without also stealing the keychain entry. Pair it with TTY step-up (`mcp.require_step_up_for_destructive`) for in-the-loop pause + signed trail. + +### Other audit-log limitations + +- **No per-operator identity at the JumpCloud Console level.** The signed manifest binds an op to *the local Ed25519 keypair*, not to a specific person — multiple operators sharing one jc profile are still indistinguishable. Real per-user audit identity needs OAuth Device Flow, tracked in [KLA-414](https://linear.app/klaassenconsulting/issue/KLA-414). +- **Stdio transport carries no session metadata** — there is no client identifier in the record beyond the tool call itself. ## Threat model summary @@ -291,8 +311,8 @@ This is tracked in [KLA-408](https://linear.app/klaassenconsulting/issue/KLA-408 |---|---|---| | **Laptop is online, operator is at the keyboard** | TTY confirmation, `--plan`, `--read-only` MCP mode, keychain storage | — | | **Laptop is stolen with screen unlocked** | Credentials are in the keychain, not on disk; the OS lock screen is the boundary | jc itself does not require re-auth on resume | -| **API key exfiltrated** | Validated keys hit the org's normal API rate limits and audit log on JumpCloud | jc has no key-rotation reminder, no per-op fingerprinting, no anomaly detection | -| **Compromised MCP agent prompt** | Read-only mode, tool allow/block lists, rate limiting, the `execute: true` gate | The agent controls `execute: true`; nothing forces a human in the loop | +| **API key exfiltrated** | Validated keys hit the org's normal API rate limits and audit log on JumpCloud; `mcp.sign_destructive_ops` produces a tamper-evident local trail | jc has no key-rotation reminder; signed manifests detect rather than prevent | +| **Compromised MCP agent prompt** | Read-only mode, tool allow/block lists, rate limiting, the `execute: true` gate, optional TTY step-up, optional signed-manifest audit trail | The agent controls `execute: true` unless step-up is enabled; signing is post-hoc detection, not prevention | | **MCP server exposed via tunnel without `--require-auth`** | Loopback default + explicit warning when binding non-loopback without TLS | Once exposed, anyone reaching the URL has full credential privilege | | **Plaintext config used (`--allow-plaintext`)** | Mode-`0600` config file in a `0700` dir | jc cannot prevent backups, sync clients, or `cat ~/.config/jc/config.yaml` from leaking it | @@ -300,9 +320,9 @@ This is tracked in [KLA-408](https://linear.app/klaassenconsulting/issue/KLA-408 Active backlog tickets that close gaps called out in this document: -- **[KLA-407](https://linear.app/klaassenconsulting/issue/KLA-407) — Default to OAuth + roadmap to user-bound auth (device flow).** Slice A flips `jc auth login` to recommend OAuth client credentials over personal API keys; Slice B adds OAuth Device Flow once the JumpCloud platform exposes the endpoint. -- **[KLA-408](https://linear.app/klaassenconsulting/issue/KLA-408) — Step-up auth gate for MCP-invoked destructive ops.** Touch ID / PIN reprompt before any `execute: true` destructive call, plus signed-manifest audit entries. -- **[KLA-409](https://linear.app/klaassenconsulting/issue/KLA-409) — MCP server tool allowlist + read-only mode.** Most of this already shipped (allow/block lists, `--read-only`); the remaining slice is profile-level read-only enforcement so a profile bound to a read-only OAuth client can't be misconfigured into a mutation-capable session. +- **[KLA-412](https://linear.app/klaassenconsulting/issue/KLA-412) — Touch ID / WebAuthn step-up authenticator.** macOS Touch ID via `LocalAuthentication.framework` so the prompt works in stdio transport (Claude Desktop). Today's TTY step-up only works in HTTP/SSE transport with a real terminal attached. +- **[KLA-413](https://linear.app/klaassenconsulting/issue/KLA-413) — Out-of-band approval for MCP destructive ops.** Webhook-based dual-control for "delete in production" workflows. +- **[KLA-414](https://linear.app/klaassenconsulting/issue/KLA-414) — OAuth Device Flow.** Per-operator audit identity once JumpCloud exposes `/oauth2/device`. Currently platform-blocked. ## Quick reference diff --git a/internal/cmd/audit.go b/internal/cmd/audit.go new file mode 100644 index 0000000..1ba2a91 --- /dev/null +++ b/internal/cmd/audit.go @@ -0,0 +1,92 @@ +package cmd + +import ( + "crypto/ed25519" + "encoding/base64" + "fmt" + "os" + "path/filepath" + + "github.com/spf13/cobra" + + "github.com/klaassen-consulting/jc/internal/config" + "github.com/klaassen-consulting/jc/internal/mcp" +) + +func newAuditCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "audit", + Short: "Inspect and verify MCP audit logs", + Long: "Subcommands for working with MCP audit logs, including signed-manifest verification (KLA-411).", + } + + cmd.AddCommand(newAuditVerifyCmd()) + return cmd +} + +func newAuditVerifyCmd() *cobra.Command { + var profileFlag, logPath, pubkeyOverride string + + cmd := &cobra.Command{ + Use: "verify", + Short: "Verify signatures in the signed MCP audit log", + Long: `Read every signed manifest in the MCP signed-audit-log and verify the +chain against the configured per-profile public key. + +Used to catch tampering with the audit trail. A successful run reports +the count of verified records and exits 0; any signature mismatch, +truncation, or decode error exits with a non-zero code. + +By default, reads ~/.config/jc/mcp-audit-signed.log and uses the active +profile's signing pubkey from config (set the first time +'mcp.sign_destructive_ops' is enabled and a destructive op fires). + +Pass --pubkey to verify against an externally-attested key — useful when +investigating a host whose config might itself be tampered with.`, + RunE: func(cmd *cobra.Command, args []string) error { + profile := profileFlag + if profile == "" { + profile = config.ActiveProfile() + } + + pubkeyB64 := pubkeyOverride + if pubkeyB64 == "" { + pubkeyB64 = config.SigningPubkey(profile) + } + if pubkeyB64 == "" { + return fmt.Errorf("no signing public key for profile %q. Either run a signed destructive op first to bootstrap, or pass --pubkey explicitly", profile) + } + pubBytes, err := base64.StdEncoding.DecodeString(pubkeyB64) + if err != nil { + return fmt.Errorf("decoding stored pubkey: %w", err) + } + if len(pubBytes) != ed25519.PublicKeySize { + return fmt.Errorf("stored pubkey is %d bytes, want %d", len(pubBytes), ed25519.PublicKeySize) + } + + path := logPath + if path == "" { + path = filepath.Join(config.ConfigDir(), "mcp-audit-signed.log") + } + f, err := os.Open(path) + if err != nil { + return fmt.Errorf("opening signed audit log %s: %w", path, err) + } + defer f.Close() + + verified, verr := mcp.VerifyManifestStream(f, ed25519.PublicKey(pubBytes)) + if verr != nil { + fmt.Fprintf(cmd.ErrOrStderr(), "Verified %d manifest(s) before failure: %v\n", verified, verr) + return &ExitError{Code: 1, Err: verr} + } + fmt.Fprintf(cmd.OutOrStdout(), "OK — verified %d signed manifest(s) for profile %q.\n", verified, profile) + return nil + }, + } + + cmd.Flags().StringVar(&profileFlag, "profile", "", "Profile whose signing pubkey to use (defaults to active profile)") + cmd.Flags().StringVar(&logPath, "log", "", "Path to the signed audit log (defaults to ~/.config/jc/mcp-audit-signed.log)") + cmd.Flags().StringVar(&pubkeyOverride, "pubkey", "", "Verify against this base64 Ed25519 pubkey instead of the configured one (override when investigating a possibly-tampered host)") + + return cmd +} diff --git a/internal/cmd/audit_test.go b/internal/cmd/audit_test.go new file mode 100644 index 0000000..b014f2b --- /dev/null +++ b/internal/cmd/audit_test.go @@ -0,0 +1,179 @@ +package cmd + +import ( + "bytes" + "crypto/ed25519" + "crypto/rand" + "encoding/base64" + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/cobra" +) + +// writeTestManifest writes one JSON-encoded signed manifest line to path. +// The shape mirrors mcp.signedManifest; we hand-roll it here to avoid an +// import cycle with the mcp package's tests. +func writeTestManifest(t *testing.T, path string, priv ed25519.PrivateKey) { + t.Helper() + pub := priv.Public().(ed25519.PublicKey) + + type manifest struct { + Tool string `json:"tool"` + ArgsRedacted json.RawMessage `json:"args_redacted"` + Target string `json:"target,omitempty"` + Timestamp string `json:"timestamp"` + Nonce string `json:"nonce"` + OperatorPubkey string `json:"operator_pubkey"` + Signature string `json:"signature,omitempty"` + } + + m := manifest{ + Tool: "users_delete", + ArgsRedacted: json.RawMessage(`{"identifier":"alice"}`), + Target: "alice", + Timestamp: "2026-04-28T20:00:00Z", + Nonce: base64.StdEncoding.EncodeToString([]byte("nonce-fixture-12345678901234567")), + OperatorPubkey: base64.StdEncoding.EncodeToString(pub), + } + canonical, _ := json.Marshal(m) + m.Signature = base64.StdEncoding.EncodeToString(ed25519.Sign(priv, canonical)) + + out, _ := json.Marshal(m) + out = append(out, '\n') + + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(path, out, 0600); err != nil { + t.Fatalf("write: %v", err) + } +} + +func TestAuditVerify_OK(t *testing.T) { + setupTestConfig(t, `active_profile: default +profiles: + default: + api_key: "" +`) + + pub, priv, _ := ed25519.GenerateKey(rand.Reader) + pubB64 := base64.StdEncoding.EncodeToString(pub) + + tmp := t.TempDir() + logPath := filepath.Join(tmp, "mcp-audit-signed.log") + writeTestManifest(t, logPath, priv) + + rootCmd := NewRootCmd() + stdout := new(bytes.Buffer) + stderr := new(bytes.Buffer) + rootCmd.SetOut(stdout) + rootCmd.SetErr(stderr) + rootCmd.SetArgs([]string{"audit", "verify", "--log", logPath, "--pubkey", pubB64}) + + if err := rootCmd.Execute(); err != nil { + t.Fatalf("audit verify failed: %v\nstderr: %s", err, stderr.String()) + } + if !strings.Contains(stdout.String(), "OK") { + t.Errorf("expected OK message, got: %s", stdout.String()) + } + if !strings.Contains(stdout.String(), "1 signed manifest") { + t.Errorf("expected count=1 in output, got: %s", stdout.String()) + } +} + +func TestAuditVerify_DetectsTamper(t *testing.T) { + setupTestConfig(t, `active_profile: default +profiles: + default: + api_key: "" +`) + + pub, priv, _ := ed25519.GenerateKey(rand.Reader) + pubB64 := base64.StdEncoding.EncodeToString(pub) + + tmp := t.TempDir() + logPath := filepath.Join(tmp, "mcp-audit-signed.log") + writeTestManifest(t, logPath, priv) + + // Tamper with the on-disk manifest. + data, _ := os.ReadFile(logPath) + tampered := bytes.Replace(data, []byte(`"target":"alice"`), []byte(`"target":"bob"`), 1) + _ = os.WriteFile(logPath, tampered, 0600) + + rootCmd := NewRootCmd() + rootCmd.SilenceUsage = true + rootCmd.SilenceErrors = true + rootCmd.SetOut(new(bytes.Buffer)) + rootCmd.SetErr(new(bytes.Buffer)) + rootCmd.SetArgs([]string{"audit", "verify", "--log", logPath, "--pubkey", pubB64}) + + err := rootCmd.Execute() + if err == nil { + t.Fatal("expected verify to fail on tampered manifest, got nil") + } + var exitErr *ExitError + if !errors.As(err, &exitErr) { + t.Fatalf("expected ExitError, got %T: %v", err, err) + } + if exitErr.Code == 0 { + t.Errorf("expected non-zero exit code on tamper, got %d", exitErr.Code) + } +} + +func TestAuditVerify_NoPubkey(t *testing.T) { + setupTestConfig(t, `active_profile: default +profiles: + default: + api_key: "" +`) + + tmp := t.TempDir() + logPath := filepath.Join(tmp, "mcp-audit-signed.log") + if err := os.WriteFile(logPath, []byte(""), 0600); err != nil { + t.Fatal(err) + } + + rootCmd := NewRootCmd() + rootCmd.SilenceUsage = true + rootCmd.SilenceErrors = true + rootCmd.SetOut(new(bytes.Buffer)) + rootCmd.SetErr(new(bytes.Buffer)) + rootCmd.SetArgs([]string{"audit", "verify", "--log", logPath}) + + err := rootCmd.Execute() + if err == nil { + t.Fatal("expected error when no pubkey configured and no --pubkey passed, got nil") + } + if !strings.Contains(err.Error(), "no signing public key") { + t.Errorf("error should mention missing pubkey, got: %v", err) + } +} + +func TestAuditCommandRegistered(t *testing.T) { + rootCmd := NewRootCmd() + var found *cobra.Command + for _, c := range rootCmd.Commands() { + if c.Use == "audit" { + found = c + break + } + } + if found == nil { + t.Fatal("expected 'audit' command to be registered") + } + var verifyFound bool + for _, sub := range found.Commands() { + if sub.Name() == "verify" { + verifyFound = true + break + } + } + if !verifyFound { + t.Error("expected 'verify' subcommand under 'audit'") + } +} diff --git a/internal/cmd/mcp.go b/internal/cmd/mcp.go index cb00799..04efb1f 100644 --- a/internal/cmd/mcp.go +++ b/internal/cmd/mcp.go @@ -38,16 +38,17 @@ Configure in Claude Desktop: func newMcpServeCmd() *cobra.Command { var ( - rateLimit int - readOnly bool - transport string - addr string - port int - corsOrigin string - requireAuth bool - requireStepUp bool - tlsCert string - tlsKey string + rateLimit int + readOnly bool + transport string + addr string + port int + corsOrigin string + requireAuth bool + requireStepUp bool + signDestructiveOps bool + tlsCert string + tlsKey string ) cmd := &cobra.Command{ @@ -117,6 +118,9 @@ Use JC_PROFILE environment variable to select which JumpCloud org to use.`, if !cmd.Flags().Changed("require-step-up") { requireStepUp = config.MCPRequireStepUp() } + if !cmd.Flags().Changed("sign-destructive") { + signDestructiveOps = config.MCPSignDestructiveOps() + } // Profile-role enforcement: a profile bound to a read-only OAuth // client must not advertise mutation tools. Reject the start @@ -146,7 +150,7 @@ Use JC_PROFILE environment variable to select which JumpCloud org to use.`, "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) + return runMcpServe(rateLimit, readOnly, transport, addr, port, corsOrigin, tlsCert, tlsKey, requireAuth, requireStepUp, signDestructiveOps) }, } @@ -160,6 +164,7 @@ Use JC_PROFILE environment variable to select which JumpCloud org to use.`, 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.") + cmd.Flags().BoolVar(&signDestructiveOps, "sign-destructive", false, "Append a signed Ed25519 manifest to ~/.config/jc/mcp-audit-signed.log for every successful destructive op. Generates a per-profile keypair on first use; private key in keychain, pubkey in config. Verify chain with 'jc audit verify'. Defaults to mcp.sign_destructive_ops in config.") return cmd } @@ -237,7 +242,7 @@ 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, requireStepUp bool) error { +func runMcpServe(rateLimit int, readOnly bool, transport, addr string, port int, corsOrigin, tlsCert, tlsKey string, requireAuth, requireStepUp, signDestructiveOps 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 @@ -252,13 +257,15 @@ func runMcpServe(rateLimit int, readOnly bool, transport, addr string, port int, } server := mcp.NewServer(mcp.Options{ - RateLimit: rateLimit, - ReadOnly: readOnly, - AuditEnabled: config.MCPAuditLog(), - AllowedTools: config.MCPAllowedTools(), - BlockedTools: config.MCPBlockedTools(), - RequireStepUp: requireStepUp, - StepUpAPIKey: stepUpAPIKey, + RateLimit: rateLimit, + ReadOnly: readOnly, + AuditEnabled: config.MCPAuditLog(), + AllowedTools: config.MCPAllowedTools(), + BlockedTools: config.MCPBlockedTools(), + RequireStepUp: requireStepUp, + StepUpAPIKey: stepUpAPIKey, + SignDestructiveOps: signDestructiveOps, + SigningProfile: config.ActiveProfile(), }) // Handle graceful shutdown on Ctrl+C / SIGTERM. diff --git a/internal/cmd/mcp_test.go b/internal/cmd/mcp_test.go index 471fea7..953d8eb 100644 --- a/internal/cmd/mcp_test.go +++ b/internal/cmd/mcp_test.go @@ -165,6 +165,14 @@ func TestMcpServeCmd_FlagDefaults(t *testing.T) { if stepUp.DefValue != "false" { t.Errorf("expected require-step-up default false, got %s", stepUp.DefValue) } + + signDest := mcpCmd.Flags().Lookup("sign-destructive") + if signDest == nil { + t.Fatal("expected sign-destructive flag") + } + if signDest.DefValue != "false" { + t.Errorf("expected sign-destructive default false, got %s", signDest.DefValue) + } } func TestRootCmd_IncludesMcp(t *testing.T) { diff --git a/internal/cmd/root.go b/internal/cmd/root.go index b894a42..da436ae 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -149,7 +149,7 @@ interface.`, // Setup & Config addToGroup(rootCmd, "config", - newSetupCmd(), newAuthCmd(), newConfigCmd(), + newSetupCmd(), newAuthCmd(), newAuditCmd(), newConfigCmd(), newVersionCmd(), newCompletionCmd(), newTUICmd(), ) diff --git a/internal/config/config.go b/internal/config/config.go index 9162593..33dbe36 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -36,6 +36,7 @@ mcp: audit_log: true plan_first: true require_step_up_for_destructive: false + sign_destructive_ops: false profiles: default: @@ -100,6 +101,7 @@ func setDefaults() { viper.SetDefault("mcp.audit_log", true) viper.SetDefault("mcp.plan_first", true) viper.SetDefault("mcp.require_step_up_for_destructive", false) + viper.SetDefault("mcp.sign_destructive_ops", false) viper.SetDefault("ask.provider", "disabled") viper.SetDefault("ask.api_key", "") viper.SetDefault("ask.model", "") @@ -396,6 +398,7 @@ var ValidConfigKeys = []string{ "mcp.audit_log", "mcp.plan_first", "mcp.require_step_up_for_destructive", + "mcp.sign_destructive_ops", "mcp.sse_port", "mcp.allowed_tools", "mcp.blocked_tools", @@ -429,7 +432,8 @@ func coerceValue(key, value string) interface{} { } case "defaults.confirm_destructive", "defaults.color", "cache.enabled", "mcp.read_only", "mcp.audit_log", "mcp.plan_first", - "mcp.require_step_up_for_destructive", "ask.confirm_before_execute": + "mcp.require_step_up_for_destructive", "mcp.sign_destructive_ops", + "ask.confirm_before_execute": // Attempt bool conversion. switch strings.ToLower(value) { case "true", "1", "yes": @@ -470,6 +474,32 @@ func MCPRequireStepUp() bool { return viper.GetBool("mcp.require_step_up_for_destructive") } +// MCPSignDestructiveOps returns true if every successful destructive MCP +// op should produce a signed manifest in the audit log. Used by the +// chokepoint to opt the Ed25519 signer in. +func MCPSignDestructiveOps() bool { + return viper.GetBool("mcp.sign_destructive_ops") +} + +// SigningPubkey returns the Ed25519 public key (base64-encoded) recorded +// for the named profile. The pubkey is persisted to config so the +// `jc audit verify` command can validate the signature chain on first +// use without trusting the keychain. Empty string means no signing key +// has been generated for this profile yet. +func SigningPubkey(profile string) string { + if profile == "" { + profile = ActiveProfile() + } + return viper.GetString("profiles." + profile + ".signing_pubkey") +} + +// SetSigningPubkey writes the (base64-encoded) public key to the named +// profile. Called once when the signer generates a fresh keypair on +// first destructive op. +func SetSigningPubkey(profile, pubkeyB64 string) error { + return SetProfileField(profile, "signing_pubkey", pubkeyB64) +} + // MCPSSEPort returns the configured SSE transport port (default 8080). func MCPSSEPort() int { port := viper.GetInt("mcp.sse_port") diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go index 35c46f4..78c487c 100644 --- a/internal/keychain/keychain.go +++ b/internal/keychain/keychain.go @@ -122,3 +122,35 @@ func DeleteClientSecret(profile string) error { func ClientSecretURI(profile string) string { return URIPrefix + profile + ":client_secret" } + +// SetSigningKey stores an Ed25519 signing private key (raw 64-byte seed+pub +// blob, base64-encoded) in the OS keychain for the given profile. Used by +// the MCP destructive-op signer (KLA-411) to attest each destructive call +// without ever writing the private key to disk. +func SetSigningKey(profile, encodedKey string) error { + account := profile + ":signing_key" + if err := keyring.Set(ServiceName, account, encodedKey); err != nil { + return fmt.Errorf("failed to store signing key in keychain for profile %q: %w", profile, err) + } + return nil +} + +// GetSigningKey retrieves the Ed25519 signing private key (base64-encoded) +// from the OS keychain for the given profile. +func GetSigningKey(profile string) (string, error) { + account := profile + ":signing_key" + secret, err := keyring.Get(ServiceName, account) + if err != nil { + return "", fmt.Errorf("failed to retrieve signing key from keychain for profile %q: %w", profile, err) + } + return secret, nil +} + +// DeleteSigningKey removes the signing key from the OS keychain. +func DeleteSigningKey(profile string) error { + account := profile + ":signing_key" + if err := keyring.Delete(ServiceName, account); err != nil { + return fmt.Errorf("failed to remove signing key from keychain for profile %q: %w", profile, err) + } + return nil +} diff --git a/internal/mcp/server.go b/internal/mcp/server.go index dacbbaa..ebd700c 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -30,6 +30,7 @@ type Server struct { readOnly bool toolFilter *toolFilter stepUp stepUpAuthenticator + signer manifestSigner toolNames []string // registered tool names, in registration order mu sync.Mutex @@ -68,6 +69,18 @@ type Options struct { // the mcp package; production callers configure step-up via // RequireStepUp + StepUpAPIKey. stepUp stepUpAuthenticator + // SignDestructiveOps enables Ed25519 op-envelope signing — every + // successful destructive op (any tool input with Execute: true) is + // recorded in ~/.config/jc/mcp-audit-signed.log with a per-profile + // signature. Verifiable post-hoc via `jc audit verify`. Layers on + // top of (does not replace) RequireStepUp. Default false. + SignDestructiveOps bool + // SigningProfile selects which profile's keychain entry holds the + // signing key. Empty defaults to the active profile. Required when + // SignDestructiveOps is true. + SigningProfile string + // signer injects a custom manifestSigner. Reserved for tests. + signer manifestSigner } // nowFunc is overridable for tests. @@ -121,6 +134,15 @@ func NewServer(opts Options) *Server { stepUp = newStepUp(opts.RequireStepUp, opts.StepUpAPIKey) } + signer := opts.signer + if signer == nil { + profile := opts.SigningProfile + if profile == "" { + profile = config.ActiveProfile() + } + signer = newSigner(opts.SignDestructiveOps, profile) + } + s := &Server{ mcpServer: mcpServer, limiter: newRateLimiter(opts.RateLimit), @@ -128,6 +150,7 @@ func NewServer(opts Options) *Server { readOnly: opts.ReadOnly, toolFilter: newToolFilter(opts.AllowedTools, opts.BlockedTools), stepUp: stepUp, + signer: signer, } s.registerTools() diff --git a/internal/mcp/sign.go b/internal/mcp/sign.go new file mode 100644 index 0000000..bdf70b6 --- /dev/null +++ b/internal/mcp/sign.go @@ -0,0 +1,247 @@ +package mcp + +import ( + "crypto/ed25519" + "crypto/rand" + "encoding/base64" + "encoding/json" + "fmt" + "io" + "os" + "path/filepath" + "sync" + "time" + + "github.com/klaassen-consulting/jc/internal/config" + "github.com/klaassen-consulting/jc/internal/keychain" +) + +// manifestSigner produces a tamper-evident record for a destructive MCP +// op. Implementations are called from the chokepoint in addTypedTool +// after the underlying JumpCloud API call has succeeded — the manifest +// is a forensic attestation, not a precondition. A nil error means the +// manifest was emitted; a non-nil error is logged but does not roll the +// op back (the op already happened upstream). +type manifestSigner interface { + sign(toolName string, args any) error +} + +// noopSigner is the default when destructive-op signing is disabled. +// Adds zero overhead to the hot path. +type noopSigner struct{} + +func (noopSigner) sign(string, any) error { return nil } + +// signedManifest is the JSON object written one-per-line to the signed +// audit log. The signature covers the canonical encoding of every other +// field, so any tampering with tool/args/timestamp/etc. invalidates the +// chain. Verifiers reconstruct the canonical form and check against the +// stored public key. +type signedManifest struct { + Tool string `json:"tool"` + ArgsRedacted json.RawMessage `json:"args_redacted"` + Target string `json:"target,omitempty"` + Timestamp string `json:"timestamp"` + Nonce string `json:"nonce"` + OperatorPubkey string `json:"operator_pubkey"` + Signature string `json:"signature,omitempty"` +} + +// canonicalForSigning returns the byte sequence the signature is computed +// over. We marshal a copy of the manifest with Signature cleared and rely +// on encoding/json's deterministic key order (alphabetical) for stable +// canonicalization. Verifiers do the same trick. +func (m signedManifest) canonicalForSigning() ([]byte, error) { + cp := m + cp.Signature = "" + return json.Marshal(cp) +} + +// ed25519Signer holds the key material for a profile and writes manifests +// to the signed audit log. Lazy keypair generation: the first sign call +// for a profile that has no signing key yet creates one, persists the +// private key in the keychain, and writes the pubkey into config. +type ed25519Signer struct { + mu sync.Mutex + profile string + logPath string + priv ed25519.PrivateKey // nil until lazy-loaded + pub ed25519.PublicKey // nil until lazy-loaded + pubB64 string // cached for the manifest field +} + +// newEd25519Signer constructs the signer for the named profile and the +// given audit log path. Construction does NOT touch the keychain — the +// keypair is materialized lazily on the first sign call so a server +// that runs read-only never generates one. +func newEd25519Signer(profile, logPath string) *ed25519Signer { + return &ed25519Signer{ + profile: profile, + logPath: logPath, + } +} + +func (s *ed25519Signer) sign(toolName string, args any) error { + s.mu.Lock() + defer s.mu.Unlock() + + if err := s.ensureKeyLoaded(); err != nil { + return fmt.Errorf("loading signing key: %w", err) + } + + nonce := make([]byte, 32) + if _, err := rand.Read(nonce); err != nil { + return fmt.Errorf("generating nonce: %w", err) + } + + rawArgs, err := json.Marshal(args) + if err != nil { + return fmt.Errorf("marshaling args: %w", err) + } + rawArgs = redactParams(rawArgs) + + m := signedManifest{ + Tool: toolName, + ArgsRedacted: rawArgs, + Target: destructiveTarget(args), + Timestamp: nowFunc().UTC().Format(time.RFC3339), + Nonce: base64.StdEncoding.EncodeToString(nonce), + OperatorPubkey: s.pubB64, + } + + canonical, err := m.canonicalForSigning() + if err != nil { + return fmt.Errorf("canonicalizing manifest: %w", err) + } + sig := ed25519.Sign(s.priv, canonical) + m.Signature = base64.StdEncoding.EncodeToString(sig) + + return s.appendManifest(m) +} + +// ensureKeyLoaded materializes the per-profile keypair from the keychain, +// generating + persisting a new one if none exists yet. Caller must hold s.mu. +func (s *ed25519Signer) ensureKeyLoaded() error { + if s.priv != nil { + return nil + } + + encoded, err := keychainGetSigningKey(s.profile) + if err == nil && encoded != "" { + raw, decErr := base64.StdEncoding.DecodeString(encoded) + if decErr != nil { + return fmt.Errorf("decoding stored signing key: %w", decErr) + } + if len(raw) != ed25519.PrivateKeySize { + return fmt.Errorf("stored signing key is %d bytes, want %d", len(raw), ed25519.PrivateKeySize) + } + s.priv = ed25519.PrivateKey(raw) + s.pub = s.priv.Public().(ed25519.PublicKey) + s.pubB64 = base64.StdEncoding.EncodeToString(s.pub) + return nil + } + + // Generate fresh keypair. + pub, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + return fmt.Errorf("generating keypair: %w", err) + } + encodedPriv := base64.StdEncoding.EncodeToString(priv) + if err := keychainSetSigningKey(s.profile, encodedPriv); err != nil { + return fmt.Errorf("storing signing key in keychain: %w", err) + } + pubB64 := base64.StdEncoding.EncodeToString(pub) + if err := configSetSigningPubkey(s.profile, pubB64); err != nil { + // Roll back the keychain write so the next attempt regenerates + // rather than leaving a key whose pubkey isn't recorded. + _ = keychainDeleteSigningKey(s.profile) + return fmt.Errorf("persisting public key to config: %w", err) + } + s.priv = priv + s.pub = pub + s.pubB64 = pubB64 + return nil +} + +// appendManifest writes one JSON line to the signed audit log. Caller +// must hold s.mu so concurrent destructive ops don't interleave bytes. +// File mode 0600 to match the existing audit log. +func (s *ed25519Signer) appendManifest(m signedManifest) error { + if err := os.MkdirAll(filepath.Dir(s.logPath), 0700); err != nil { + return fmt.Errorf("creating signed-audit-log dir: %w", err) + } + f, err := os.OpenFile(s.logPath, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0600) + if err != nil { + return fmt.Errorf("opening signed-audit-log: %w", err) + } + defer f.Close() + + enc := json.NewEncoder(f) + if err := enc.Encode(m); err != nil { + return fmt.Errorf("writing signed manifest: %w", err) + } + return nil +} + +// signedAuditLogPath returns the conventional location, mirroring the +// existing audit log next to it. +func signedAuditLogPath() string { + return filepath.Join(config.ConfigDir(), "mcp-audit-signed.log") +} + +// newSigner returns the manifestSigner a Server should use given the +// requested configuration. Disabled → noop (zero overhead path). +func newSigner(enabled bool, profile string) manifestSigner { + if !enabled { + return noopSigner{} + } + return newEd25519Signer(profile, signedAuditLogPath()) +} + +// --- Test seams (overridden by stepup_test.go siblings) --- +// Wrapping these calls in package-level vars lets tests inject in-memory +// keychain + config writes without touching the user's real OS keychain. + +var keychainGetSigningKey = func(profile string) (string, error) { + return keychain.GetSigningKey(profile) +} +var keychainSetSigningKey = func(profile, encoded string) error { + return keychain.SetSigningKey(profile, encoded) +} +var keychainDeleteSigningKey = func(profile string) error { + return keychain.DeleteSigningKey(profile) +} +var configSetSigningPubkey = func(profile, pubB64 string) error { + return config.SetSigningPubkey(profile, pubB64) +} + +// VerifyManifestStream reads JSON-encoded signedManifests from r and +// returns nil if every record's signature checks out against the +// supplied trusted public key. The first invalid record (or read error) +// short-circuits with an error naming the offending record's nonce so +// operators can grep the file. +// +// Exposed for the `jc audit verify` CLI command and tests. +func VerifyManifestStream(r io.Reader, trustedPubkey ed25519.PublicKey) (int, error) { + dec := json.NewDecoder(r) + verified := 0 + for dec.More() { + var m signedManifest + if err := dec.Decode(&m); err != nil { + return verified, fmt.Errorf("decoding manifest #%d: %w", verified+1, err) + } + canonical, err := m.canonicalForSigning() + if err != nil { + return verified, fmt.Errorf("canonicalizing manifest #%d (nonce=%s): %w", verified+1, m.Nonce, err) + } + sig, err := base64.StdEncoding.DecodeString(m.Signature) + if err != nil { + return verified, fmt.Errorf("decoding signature on manifest #%d (nonce=%s): %w", verified+1, m.Nonce, err) + } + if !ed25519.Verify(trustedPubkey, canonical, sig) { + return verified, fmt.Errorf("signature mismatch on manifest #%d (nonce=%s, tool=%s)", verified+1, m.Nonce, m.Tool) + } + verified++ + } + return verified, nil +} diff --git a/internal/mcp/sign_test.go b/internal/mcp/sign_test.go new file mode 100644 index 0000000..3e7692b --- /dev/null +++ b/internal/mcp/sign_test.go @@ -0,0 +1,338 @@ +package mcp + +import ( + "bytes" + "crypto/ed25519" + "crypto/rand" + "encoding/base64" + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "sync/atomic" + "testing" +) + +// signerFixture overrides the keychain/config seams with in-memory state +// so tests don't touch the user's real OS keychain or write to their +// config file. Returns a cleanup function. +func signerFixture(t *testing.T) (logPath string, cleanup func()) { + t.Helper() + + tmpDir := t.TempDir() + logPath = filepath.Join(tmpDir, "mcp-audit-signed.log") + + prevGet := keychainGetSigningKey + prevSet := keychainSetSigningKey + prevDel := keychainDeleteSigningKey + prevPub := configSetSigningPubkey + + store := map[string]string{} + pubkeys := map[string]string{} + + keychainGetSigningKey = func(profile string) (string, error) { + v, ok := store[profile] + if !ok { + return "", errors.New("not found") + } + return v, nil + } + keychainSetSigningKey = func(profile, encoded string) error { + store[profile] = encoded + return nil + } + keychainDeleteSigningKey = func(profile string) error { + delete(store, profile) + return nil + } + configSetSigningPubkey = func(profile, pubB64 string) error { + pubkeys[profile] = pubB64 + return nil + } + + cleanup = func() { + keychainGetSigningKey = prevGet + keychainSetSigningKey = prevSet + keychainDeleteSigningKey = prevDel + configSetSigningPubkey = prevPub + } + return logPath, cleanup +} + +func TestNoopSigner_AlwaysOK(t *testing.T) { + if err := (noopSigner{}).sign("users_delete", destructiveInput{Identifier: "alice", Execute: true}); err != nil { + t.Errorf("noopSigner.sign returned %v, want nil", err) + } +} + +func TestNewSigner_DisabledReturnsNoop(t *testing.T) { + if _, ok := newSigner(false, "default").(noopSigner); !ok { + t.Errorf("newSigner(false) = %T, want noopSigner", newSigner(false, "default")) + } +} + +func TestNewSigner_EnabledReturnsEd25519(t *testing.T) { + if _, ok := newSigner(true, "default").(*ed25519Signer); !ok { + t.Errorf("newSigner(true) = %T, want *ed25519Signer", newSigner(true, "default")) + } +} + +func TestEd25519Signer_RoundTrip(t *testing.T) { + logPath, cleanup := signerFixture(t) + defer cleanup() + + s := newEd25519Signer("default", logPath) + if err := s.sign("users_delete", destructiveInput{Identifier: "alice", Execute: true}); err != nil { + t.Fatalf("sign() error: %v", err) + } + + // Manifest written? + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("reading log: %v", err) + } + if !strings.Contains(string(data), `"tool":"users_delete"`) { + t.Errorf("log missing tool name: %s", data) + } + if !strings.Contains(string(data), `"target":"alice"`) { + t.Errorf("log missing target: %s", data) + } + + // Verify signature against the recorded pubkey. + var m signedManifest + if err := json.Unmarshal(bytes.TrimSpace(data), &m); err != nil { + t.Fatalf("decoding manifest: %v", err) + } + pubBytes, _ := base64.StdEncoding.DecodeString(m.OperatorPubkey) + canonical, _ := m.canonicalForSigning() + sig, _ := base64.StdEncoding.DecodeString(m.Signature) + if !ed25519.Verify(ed25519.PublicKey(pubBytes), canonical, sig) { + t.Error("signature did not verify against the manifest's own pubkey") + } +} + +func TestEd25519Signer_MultipleOpsReuseKey(t *testing.T) { + logPath, cleanup := signerFixture(t) + defer cleanup() + + s := newEd25519Signer("default", logPath) + for i := 0; i < 3; i++ { + if err := s.sign("users_delete", destructiveInput{Identifier: "alice", Execute: true}); err != nil { + t.Fatalf("sign() #%d error: %v", i, err) + } + } + + data, _ := os.ReadFile(logPath) + lines := bytes.Split(bytes.TrimSpace(data), []byte("\n")) + if len(lines) != 3 { + t.Fatalf("expected 3 lines, got %d", len(lines)) + } + + // All three manifests must share the same pubkey (we only generate + // once per signer's lifetime). + var pubkeys []string + for _, line := range lines { + var m signedManifest + if err := json.Unmarshal(line, &m); err != nil { + t.Fatalf("decode: %v", err) + } + pubkeys = append(pubkeys, m.OperatorPubkey) + } + if pubkeys[0] != pubkeys[1] || pubkeys[1] != pubkeys[2] { + t.Errorf("pubkeys differ across ops: %v", pubkeys) + } +} + +func TestEd25519Signer_NoncesAreUnique(t *testing.T) { + logPath, cleanup := signerFixture(t) + defer cleanup() + + s := newEd25519Signer("default", logPath) + for i := 0; i < 5; i++ { + if err := s.sign("users_delete", destructiveInput{Identifier: "alice", Execute: true}); err != nil { + t.Fatal(err) + } + } + + data, _ := os.ReadFile(logPath) + lines := bytes.Split(bytes.TrimSpace(data), []byte("\n")) + seen := map[string]bool{} + for _, line := range lines { + var m signedManifest + _ = json.Unmarshal(line, &m) + if seen[m.Nonce] { + t.Errorf("duplicate nonce %s", m.Nonce) + } + seen[m.Nonce] = true + } +} + +func TestVerifyManifestStream_HappyPath(t *testing.T) { + logPath, cleanup := signerFixture(t) + defer cleanup() + + s := newEd25519Signer("default", logPath) + for i := 0; i < 3; i++ { + if err := s.sign("users_delete", destructiveInput{Identifier: "alice", Execute: true}); err != nil { + t.Fatal(err) + } + } + + data, _ := os.ReadFile(logPath) + verified, err := VerifyManifestStream(bytes.NewReader(data), s.pub) + if err != nil { + t.Fatalf("verify: %v", err) + } + if verified != 3 { + t.Errorf("verified = %d, want 3", verified) + } +} + +func TestVerifyManifestStream_TamperDetected(t *testing.T) { + logPath, cleanup := signerFixture(t) + defer cleanup() + + s := newEd25519Signer("default", logPath) + if err := s.sign("users_delete", destructiveInput{Identifier: "alice", Execute: true}); err != nil { + t.Fatal(err) + } + + // Tamper with the on-disk manifest: swap the target. + data, _ := os.ReadFile(logPath) + tampered := bytes.Replace(data, []byte(`"target":"alice"`), []byte(`"target":"bob"`), 1) + + verified, err := VerifyManifestStream(bytes.NewReader(tampered), s.pub) + if err == nil { + t.Fatal("expected verification to fail after target swap, got nil") + } + if verified != 0 { + t.Errorf("verified should stop at first failure; got %d", verified) + } + if !strings.Contains(err.Error(), "signature mismatch") { + t.Errorf("error should mention signature mismatch, got: %v", err) + } +} + +func TestVerifyManifestStream_WrongPubkey(t *testing.T) { + logPath, cleanup := signerFixture(t) + defer cleanup() + + s := newEd25519Signer("default", logPath) + if err := s.sign("users_delete", destructiveInput{Identifier: "alice", Execute: true}); err != nil { + t.Fatal(err) + } + + // Fresh, unrelated pubkey. + otherPub, _, _ := ed25519.GenerateKey(rand.Reader) + + data, _ := os.ReadFile(logPath) + if _, err := VerifyManifestStream(bytes.NewReader(data), otherPub); err == nil { + t.Fatal("expected verification to fail with wrong pubkey, got nil") + } +} + +func TestEd25519Signer_RedactsSensitiveArgs(t *testing.T) { + logPath, cleanup := signerFixture(t) + defer cleanup() + + type fakeArgs struct { + Identifier string `json:"identifier"` + APIKey string `json:"api_key"` + Execute bool `json:"execute"` + } + + s := newEd25519Signer("default", logPath) + if err := s.sign("admins_create", fakeArgs{Identifier: "alice", APIKey: "supersecret123456", Execute: true}); err != nil { + t.Fatal(err) + } + + data, _ := os.ReadFile(logPath) + if bytes.Contains(data, []byte("supersecret123456")) { + t.Errorf("manifest leaked the API key in args_redacted: %s", data) + } + if !bytes.Contains(data, []byte("REDACTED")) { + t.Errorf("manifest should include REDACTED placeholder: %s", data) + } +} + +// Integration: confirm the chokepoint actually invokes the signer on a +// successful destructive op, and skips it when the signer is disabled. + +type recordingSigner struct { + calls atomic.Int64 + lastTool string +} + +func (r *recordingSigner) sign(toolName string, args any) error { + r.calls.Add(1) + r.lastTool = toolName + return nil +} + +func TestChokepoint_SignsSuccessfulDestructiveOps(t *testing.T) { + setupToolTest(t) + + users := []map[string]any{ + {"_id": "aabbccddee112233aabbcc01", "username": "alice"}, + } + ts := startV1Server(t, users, nil, nil) + overrideV1ClientForTest(t, ts.URL) + + rec := &recordingSigner{} + cs := connectToolTestServer(t, Options{signer: rec}) + + result := callTool(t, cs, "users_delete", map[string]any{ + "identifier": "aabbccddee112233aabbcc01", + "execute": true, + }) + if result.IsError { + t.Fatalf("unexpected error: %s", getResultText(t, result)) + } + if got := rec.calls.Load(); got != 1 { + t.Errorf("signer calls = %d, want 1", got) + } + if rec.lastTool != "users_delete" { + t.Errorf("signer tool = %q, want users_delete", rec.lastTool) + } +} + +func TestChokepoint_DoesNotSignFailedOps(t *testing.T) { + setupToolTest(t) + + // No upstream server → users_delete will fail to reach the API. + rec := &recordingSigner{} + cs := connectToolTestServer(t, Options{signer: rec}) + + result := callTool(t, cs, "users_delete", map[string]any{ + "identifier": "doesnotexist", + "execute": true, + }) + if !result.IsError { + t.Fatal("expected destructive call to fail without upstream API") + } + if got := rec.calls.Load(); got != 0 { + t.Errorf("signer calls on failed op = %d, want 0", got) + } +} + +func TestChokepoint_DoesNotSignPlanMode(t *testing.T) { + setupToolTest(t) + + rec := &recordingSigner{} + cs := connectToolTestServer(t, Options{signer: rec}) + + // Pre-resolved hex ID so the resolver doesn't hit the upstream API. + // Plan mode (no execute=true) should short-circuit before any API + // call and skip the signer entirely. + result := callTool(t, cs, "users_delete", map[string]any{ + "identifier": "aabbccddee112233aabbcc01", + // execute omitted → plan mode + }) + if result.IsError { + t.Fatalf("plan mode shouldn't error: %s", getResultText(t, result)) + } + if got := rec.calls.Load(); got != 0 { + t.Errorf("signer calls in plan mode = %d, want 0", got) + } +} diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index b2cebfb..643b95a 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "os" "strings" "github.com/klaassen-consulting/jc/internal/api" @@ -5180,6 +5181,7 @@ func addTypedTool[In any](s *Server, name, description string, handler func(ctx result, out, err := handler(ctx, req, args) // Audit log. + toolFailed := err != nil || (result != nil && result.IsError) if err != nil { s.auditLog.log(name, req.Params.Arguments, false, err.Error()) } else if result != nil && result.IsError { @@ -5194,6 +5196,18 @@ func addTypedTool[In any](s *Server, name, description string, handler func(ctx s.auditLog.log(name, req.Params.Arguments, true, "") } + // Op-envelope signing (KLA-411): on a successful destructive op, + // emit a signed manifest to the signed audit log. This is a + // post-success forensic record — failures don't sign because + // "the op didn't happen" is the truth we want recorded by the + // regular audit log instead. Signing errors are surfaced on + // stderr but do not roll back the op (the API call already fired). + if !toolFailed && isExecutingDestructive(args) { + if signErr := s.signer.sign(name, args); signErr != nil { + fmt.Fprintf(os.Stderr, "jc: warning: failed to sign destructive-op manifest for %s: %v\n", name, signErr) + } + } + return result, out, err } From fdd5d1c162b5bf80e5ee9ce21ad1564df895789b Mon Sep 17 00:00:00 2001 From: Juergen Klaassen Date: Tue, 28 Apr 2026 13:59:58 -0600 Subject: [PATCH 2/2] fix(mcp): only regenerate signing key on true not-found (PR #25) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Cursor Bugbot finding on internal/mcp/sign.go:142. Before: ensureKeyLoaded treated *any* error from keychainGetSigningKey as "key missing, regenerate." A transient non-not-found error (locked keychain, permission denied, corrupted entry) would have caused jc to generate a fresh keypair and overwrite the existing entry, permanently severing the verification chain for every previously signed manifest. After: switch on the error type. Genuine keychain.ErrNotFound (the expected first-run state) bootstraps. Anything else propagates and fail-closes the signing attempt — operator sees a clear error, existing key + audit chain are preserved. Mechanics: - internal/keychain/keychain.go: re-export keyring.ErrNotFound as keychain.ErrNotFound + IsNotFound helper, so call sites can distinguish without importing zalando/go-keyring directly. - internal/mcp/sign.go: rewrite the get-or-generate switch with explicit cases for "found", "empty value w/ no error" (treat as bootstrap), "ErrNotFound" (bootstrap), default (propagate). - internal/mcp/sign_test.go: regression test asserting that a transient keychain error neither regenerates nor writes the audit log; counterpart test confirming ErrNotFound still bootstraps and that the second op reuses the same keypair. Test-fixture seam updated to return keychain.ErrNotFound instead of errors.New("not found") so existing tests still exercise the bootstrap path correctly. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/keychain/keychain.go | 16 ++++++++ internal/mcp/sign.go | 21 ++++++++++- internal/mcp/sign_test.go | 70 ++++++++++++++++++++++++++++++++++- 3 files changed, 104 insertions(+), 3 deletions(-) diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go index 78c487c..c127a48 100644 --- a/internal/keychain/keychain.go +++ b/internal/keychain/keychain.go @@ -1,6 +1,7 @@ package keychain import ( + "errors" "fmt" "strings" @@ -15,6 +16,21 @@ const ( URIPrefix = "keychain://jc/" ) +// ErrNotFound is returned when an account doesn't exist in the keychain. +// Re-exported from go-keyring so callers can distinguish "this profile +// has no entry yet" (safe to bootstrap) from "the keychain is locked / +// permission denied / corrupted" (must NOT silently regenerate, since +// that would overwrite an existing entry the caller can't currently see). +var ErrNotFound = keyring.ErrNotFound + +// IsNotFound reports whether err is a not-found keychain error. Use this +// instead of errors.Is(err, ErrNotFound) when the wrapping chain may +// include fmt.Errorf with %w — both forms work, but IsNotFound makes the +// intent explicit at call sites. +func IsNotFound(err error) bool { + return errors.Is(err, ErrNotFound) +} + // Set stores an API key in the OS keychain for the given profile. func Set(profile, apiKey string) error { if err := keyring.Set(ServiceName, profile, apiKey); err != nil { diff --git a/internal/mcp/sign.go b/internal/mcp/sign.go index bdf70b6..a3363be 100644 --- a/internal/mcp/sign.go +++ b/internal/mcp/sign.go @@ -120,14 +120,22 @@ func (s *ed25519Signer) sign(toolName string, args any) error { } // ensureKeyLoaded materializes the per-profile keypair from the keychain, -// generating + persisting a new one if none exists yet. Caller must hold s.mu. +// generating + persisting a new one only if no entry exists yet. Caller +// must hold s.mu. +// +// Critical: any non-not-found error from the keychain (locked, permission +// denied, corrupted entry, transient I/O) propagates rather than falling +// through to keypair regeneration. Otherwise a transient keychain glitch +// would overwrite an existing key and permanently break verification of +// every previously signed manifest in the audit log. func (s *ed25519Signer) ensureKeyLoaded() error { if s.priv != nil { return nil } encoded, err := keychainGetSigningKey(s.profile) - if err == nil && encoded != "" { + switch { + case err == nil && encoded != "": raw, decErr := base64.StdEncoding.DecodeString(encoded) if decErr != nil { return fmt.Errorf("decoding stored signing key: %w", decErr) @@ -139,6 +147,15 @@ func (s *ed25519Signer) ensureKeyLoaded() error { s.pub = s.priv.Public().(ed25519.PublicKey) s.pubB64 = base64.StdEncoding.EncodeToString(s.pub) return nil + case err == nil && encoded == "": + // Empty value with no error — treat as not-found and bootstrap. + case keychain.IsNotFound(err): + // Genuine not-found — bootstrap. + default: + // Locked, permission denied, corrupted, transient I/O — fail + // closed. Generating a new key would overwrite the existing + // entry once the keychain comes back, severing the audit chain. + return fmt.Errorf("retrieving signing key: %w", err) } // Generate fresh keypair. diff --git a/internal/mcp/sign_test.go b/internal/mcp/sign_test.go index 3e7692b..07587c4 100644 --- a/internal/mcp/sign_test.go +++ b/internal/mcp/sign_test.go @@ -7,11 +7,14 @@ import ( "encoding/base64" "encoding/json" "errors" + "fmt" "os" "path/filepath" "strings" "sync/atomic" "testing" + + "github.com/klaassen-consulting/jc/internal/keychain" ) // signerFixture overrides the keychain/config seams with in-memory state @@ -34,7 +37,10 @@ func signerFixture(t *testing.T) (logPath string, cleanup func()) { keychainGetSigningKey = func(profile string) (string, error) { v, ok := store[profile] if !ok { - return "", errors.New("not found") + // Return the sentinel so ensureKeyLoaded distinguishes + // "no entry yet, bootstrap" from "transient keychain + // failure, fail closed". + return "", keychain.ErrNotFound } return v, nil } @@ -232,6 +238,68 @@ func TestVerifyManifestStream_WrongPubkey(t *testing.T) { } } +func TestEd25519Signer_TransientKeychainErrorDoesNotRegenerate(t *testing.T) { + // Regression for the Bugbot finding on PR #25: a transient, + // non-not-found keychain error (locked, permission denied, etc.) + // must NOT fall through to keypair regeneration. Doing so would + // overwrite an existing entry once the keychain comes back, breaking + // verification of every previously-signed manifest. + logPath, cleanup := signerFixture(t) + defer cleanup() + + // Override the seam to simulate a transient failure. + prevGet := keychainGetSigningKey + defer func() { keychainGetSigningKey = prevGet }() + + var setCalled atomic.Int64 + prevSet := keychainSetSigningKey + keychainSetSigningKey = func(profile, encoded string) error { + setCalled.Add(1) + return prevSet(profile, encoded) + } + defer func() { keychainSetSigningKey = prevSet }() + + keychainGetSigningKey = func(profile string) (string, error) { + return "", fmt.Errorf("keychain locked: %w", errors.New("user not authenticated")) + } + + s := newEd25519Signer("default", logPath) + err := s.sign("users_delete", destructiveInput{Identifier: "alice", Execute: true}) + if err == nil { + t.Fatal("expected sign() to fail on transient keychain error, got nil") + } + if !strings.Contains(err.Error(), "retrieving signing key") { + t.Errorf("error should name the retrieval step, got: %v", err) + } + if setCalled.Load() != 0 { + t.Errorf("keychain Set called %d times — must NOT regenerate on transient errors", setCalled.Load()) + } + // Manifest log must not have been written. + if _, err := os.Stat(logPath); !os.IsNotExist(err) { + t.Errorf("signed log should not exist after failed sign attempt; stat err = %v", err) + } +} + +func TestEd25519Signer_NotFoundErrorBootstraps(t *testing.T) { + // Counterpart to the transient-error test: a genuine ErrNotFound + // (the expected first-run state) DOES trigger bootstrap. + logPath, cleanup := signerFixture(t) + defer cleanup() + + s := newEd25519Signer("default", logPath) + if err := s.sign("users_delete", destructiveInput{Identifier: "alice", Execute: true}); err != nil { + t.Fatalf("first-run sign should bootstrap and succeed, got: %v", err) + } + // Second op should reuse, not regenerate. + pub1 := s.pubB64 + if err := s.sign("users_delete", destructiveInput{Identifier: "bob", Execute: true}); err != nil { + t.Fatalf("second sign: %v", err) + } + if s.pubB64 != pub1 { + t.Errorf("pubkey changed across ops: %q → %q", pub1, s.pubB64) + } +} + func TestEd25519Signer_RedactsSensitiveArgs(t *testing.T) { logPath, cleanup := signerFixture(t) defer cleanup()