From d31d54730be536dffa8e8592badd03800c58831b Mon Sep 17 00:00:00 2001 From: Javier Marcos <1271349+javuto@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:11:52 +0200 Subject: [PATCH] Enable audit logs for MCP actions --- cmd/api/handlers/audit.go | 2 +- cmd/api/main.go | 2 +- cmd/api/mcp.go | 116 +++++++++++++++++- cmd/api/mcp_test.go | 85 ++++++++++++- frontend/src/api/audit.ts | 2 + .../src/features/audit/AuditPage.test.tsx | 16 ++- frontend/src/features/audit/AuditPage.tsx | 2 +- frontend/src/routes/_app/audit.tsx | 2 +- pkg/auditlog/audit.go | 37 ++++++ pkg/auditlog/utils.go | 3 + pkg/auditlog/utils_test.go | 1 + 11 files changed, 258 insertions(+), 10 deletions(-) diff --git a/cmd/api/handlers/audit.go b/cmd/api/handlers/audit.go index b418a1597..9a43fe782 100644 --- a/cmd/api/handlers/audit.go +++ b/cmd/api/handlers/audit.go @@ -18,7 +18,7 @@ import ( // // ?service=... exact match on service name // ?username=... case-insensitive partial match on username -// ?type=... log type integer (1..10), see pkg/auditlog.LogType* +// ?type=... log type integer (1..11), see pkg/auditlog.LogType* // ?env_uuid=... filter to one environment (resolved to internal ID) // ?since=RFC3339 created_at >= since // ?until=RFC3339 created_at <= until diff --git a/cmd/api/main.go b/cmd/api/main.go index 55b41792a..788de2540 100644 --- a/cmd/api/main.go +++ b/cmd/api/main.go @@ -1258,7 +1258,7 @@ func osctrlAPIService() { log.Info().Bool("writes", flagParams.MCP.AllowWrites).Msgf("MCP enabled — serving %s", _apiPath(apiMCPPath)) muxAPI.Handle( _apiPath(apiMCPPath), - handlerAuthCheck(mcpHandler(muxAPI, buildVersion, flagParams.MCP.AllowWrites), flagParams.Service.Auth, flagParams.JWT.JWTSecret)) + handlerAuthCheck(mcpHandler(muxAPI, buildVersion, flagParams.MCP.AllowWrites, auditLog), flagParams.Service.Auth, flagParams.JWT.JWTSecret)) } // Launch listeners for API server. The server runs in a goroutine so // the main goroutine can wait on the restart channel and trigger a diff --git a/cmd/api/mcp.go b/cmd/api/mcp.go index c0706b237..f277d928e 100644 --- a/cmd/api/mcp.go +++ b/cmd/api/mcp.go @@ -2,16 +2,35 @@ package main import ( "bytes" + "context" + "encoding/json" "fmt" "io" "net/http" + "strings" sdk "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/jmpsec/osctrl/cmd/api/handlers" "github.com/jmpsec/osctrl/pkg/apiclient" + "github.com/jmpsec/osctrl/pkg/auditlog" osctrlmcp "github.com/jmpsec/osctrl/pkg/mcp" ) +// maxAuditedArgsBytes bounds how much of a tool call's arguments the audit +// middleware records. Tool inputs are small scalars and short lists by +// construction (see pkg/mcp's tool schemas); this only guards against a +// pathological or malicious client sending an oversized arguments blob and +// bloating the audit_logs table. +const maxAuditedArgsBytes = 500 + +// mcpAuditLog is the one audit-log capability the MCP boundary needs. +// Narrowed to a single method — matching the Backend/WriteBackend pattern in +// pkg/mcp — so tests can fake it without a database. +type mcpAuditLog interface { + MCPToolCall(username, tool, args, ip string, envID uint, failed bool) +} + // mcpInternalHost is the host the in-process client addresses. Nothing // resolves it: loopbackTransport routes on method and path only, and none of // osctrl-api's mux patterns are host-scoped. It exists because http.Request @@ -111,8 +130,24 @@ func (r *responseRecorder) result(req *http.Request) *http.Response { // The returned handler must still be mounted behind handlerAuthCheck: that // rejects unauthenticated callers up front, so a bad token fails once at the // MCP boundary instead of once per tool call. -func mcpHandler(apiHandler http.Handler, version string, allowWrites bool) http.Handler { +// +// auditLog receives one entry per tool call, tagged with auditlog.LogTypeMCP +// so an operator can filter agent activity out of the far larger volume of +// SPA/CLI-driven entries, which carry no origin marker at all. This is only +// possible here, at the hosted boundary: osctrl-api itself is dispatching +// the call, so it has first-hand knowledge the request came from MCP. The +// standalone stdio binary (cmd/mcp) is, from the server's point of view, +// just another authenticated HTTP client — indistinguishable from +// osctrl-cli — and its calls are audited the same way any REST client's +// would be, with no special tagging. +func mcpHandler(apiHandler http.Handler, version string, allowWrites bool, auditLog mcpAuditLog) http.Handler { getServer := func(r *http.Request) *sdk.Server { + // Captured once per session (getServer runs at session creation, not + // per tool call — see mcp.NewStreamableHTTPHandler), same lifetime + // as the credentials loopbackTransport binds below. + username := mcpCallerUsername(r) + ip := strings.Split(r.RemoteAddr, ":")[0] + // One client per request, bound to that request's credentials. The // MCP server is cheap to build and holds no state, so per-request // construction keeps sessions from ever sharing an identity. @@ -129,10 +164,85 @@ func mcpHandler(apiHandler http.Handler, version string, allowWrites bool) http. // or a nil transport, neither of which is reachable here. return nil } + var srv *sdk.Server if allowWrites { - return osctrlmcp.NewServer(client, version, osctrlmcp.WithWrites(client)) + srv = osctrlmcp.NewServer(client, version, osctrlmcp.WithWrites(client)) + } else { + srv = osctrlmcp.NewServer(client, version) } - return osctrlmcp.NewServer(client, version) + srv.AddReceivingMiddleware(auditToolCalls(auditLog, username, ip)) + return srv } return sdk.NewStreamableHTTPHandler(getServer, nil) } + +// mcpCallerUsername reads the username handlerAuthCheck already resolved and +// stashed on the request context before mcpHandler ever runs. Re-deriving it +// here (rather than re-parsing the token) keeps this file agreeing with the +// rest of cmd/api about who the caller is, with no second source of truth. +func mcpCallerUsername(r *http.Request) string { + cv, ok := r.Context().Value(handlers.ContextKey(contextAPI)).(handlers.ContextValue) + if !ok { + return "" + } + return cv["user"] +} + +// auditToolCalls records one auditLog.MCPToolCall entry per "tools/call" +// request on this session — every read tool included, matching the existing +// convention of auditing GET-style views (see NodeAction's "viewed all +// nodes" and friends) rather than treating audit volume as a reason to skip +// reads. It never blocks or fails a tool call: audit failures only log a +// warning inside MCPToolCall itself. +func auditToolCalls(auditLog mcpAuditLog, username, ip string) sdk.Middleware { + return func(next sdk.MethodHandler) sdk.MethodHandler { + return func(ctx context.Context, method string, req sdk.Request) (sdk.Result, error) { + result, err := next(ctx, method, req) + if method != "tools/call" { + return result, err + } + + tool, args := toolCallDetails(req) + failed := err != nil + // A tool error is reported in-band (IsError on the result, no Go + // error) so the model can see and self-correct — see + // CallToolResult.IsError. That still counts as "failed" for the + // audit trail: a denied run_query is exactly what an operator + // wants to be able to find. + if ctr, ok := result.(*sdk.CallToolResult); ok && ctr.IsError { + failed = true + } + auditLog.MCPToolCall(username, tool, args, ip, auditlog.NoEnvironment, failed) + return result, err + } + } +} + +// toolCallDetails extracts the tool name and a size-bounded JSON rendering +// of its arguments from a tools/call request. The concrete parameter type +// varies by SDK dispatch phase (CallToolParamsRaw server-side, +// CallToolParams on the wire-shaped path); both are handled so this never +// depends on an SDK internal staying the same across versions any more than +// necessary. An unrecognized type degrades to an empty summary rather than +// panicking — a middleware must never be the reason a tool call fails. +func toolCallDetails(req sdk.Request) (tool, args string) { + switch p := req.GetParams().(type) { + case *sdk.CallToolParamsRaw: + return p.Name, truncate(string(p.Arguments), maxAuditedArgsBytes) + case *sdk.CallToolParams: + b, err := json.Marshal(p.Arguments) + if err != nil { + return p.Name, "" + } + return p.Name, truncate(string(b), maxAuditedArgsBytes) + default: + return "unknown", "" + } +} + +func truncate(s string, n int) string { + if len(s) <= n { + return s + } + return s[:n] + "...(truncated)" +} diff --git a/cmd/api/mcp_test.go b/cmd/api/mcp_test.go index 3dc35c900..bea167a53 100644 --- a/cmd/api/mcp_test.go +++ b/cmd/api/mcp_test.go @@ -42,11 +42,35 @@ func (a *recordingAPI) ServeHTTP(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte(a.body)) } +type fakeMCPAuditCall struct { + username string + tool string + args string + ip string + envID uint + failed bool +} + +type fakeAuditLog struct { + calls []fakeMCPAuditCall +} + +func (a *fakeAuditLog) MCPToolCall(username, tool, args, ip string, envID uint, failed bool) { + a.calls = append(a.calls, fakeMCPAuditCall{ + username: username, + tool: tool, + args: args, + ip: ip, + envID: envID, + failed: failed, + }) +} + // newMCPClient mounts mcpHandler over api and returns a connected MCP session. // creds are applied to the inbound HTTP request the way a real client would. func newMCPClient(t *testing.T, api http.Handler, apply func(*http.Request)) *sdk.ClientSession { t.Helper() - srv := httptest.NewServer(mcpHandler(api, "test", false)) + srv := httptest.NewServer(mcpHandler(api, "test", false, &fakeAuditLog{})) t.Cleanup(srv.Close) client := sdk.NewClient(&sdk.Implementation{Name: "test", Version: "test"}, nil) @@ -193,6 +217,63 @@ func TestPerRequestCredentialIsolation(t *testing.T) { } } +func TestAuditToolCallsRecordsToolCallOutcome(t *testing.T) { + for _, tc := range []struct { + name string + result sdk.Result + err error + wantFailed bool + }{ + { + name: "success", + result: &sdk.CallToolResult{}, + wantFailed: false, + }, + { + name: "tool error result", + result: &sdk.CallToolResult{IsError: true}, + wantFailed: true, + }, + { + name: "protocol error", + err: context.Canceled, + wantFailed: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + auditLog := &fakeAuditLog{} + handler := auditToolCalls(auditLog, "alice", "192.0.2.10")(func(context.Context, string, sdk.Request) (sdk.Result, error) { + return tc.result, tc.err + }) + req := &sdk.ServerRequest[*sdk.CallToolParamsRaw]{ + Params: &sdk.CallToolParamsRaw{ + Name: "get_node", + Arguments: json.RawMessage(`{"environment":"prod","identifier":"node-1"}`), + }, + } + + _, _ = handler(context.Background(), "tools/call", req) + + if len(auditLog.calls) != 1 { + t.Fatalf("audit calls = %d, want 1", len(auditLog.calls)) + } + call := auditLog.calls[0] + if call.username != "alice" || call.ip != "192.0.2.10" { + t.Fatalf("audit attribution = %+v, want alice/192.0.2.10", call) + } + if call.tool != "get_node" { + t.Fatalf("tool = %q, want get_node", call.tool) + } + if call.args != `{"environment":"prod","identifier":"node-1"}` { + t.Fatalf("args = %q", call.args) + } + if call.failed != tc.wantFailed { + t.Fatalf("failed = %v, want %v", call.failed, tc.wantFailed) + } + }) + } +} + // The hosted handler must honour the operator's write switch: mounting MCP // does not by itself put mutating tools on the wire. func TestHostedWriteToolsGated(t *testing.T) { @@ -207,7 +288,7 @@ func TestHostedWriteToolsGated(t *testing.T) { {"writes on", true, true}, } { t.Run(tc.name, func(t *testing.T) { - srv := httptest.NewServer(mcpHandler(api, "test", tc.allowWrites)) + srv := httptest.NewServer(mcpHandler(api, "test", tc.allowWrites, &fakeAuditLog{})) defer srv.Close() client := sdk.NewClient(&sdk.Implementation{Name: "test", Version: "test"}, nil) diff --git a/frontend/src/api/audit.ts b/frontend/src/api/audit.ts index 32cbd99f3..851762c02 100644 --- a/frontend/src/api/audit.ts +++ b/frontend/src/api/audit.ts @@ -61,6 +61,7 @@ export const LOG_TYPE = { Setting: 8, Visit: 9, User: 10, + MCP: 11, } as const; export const LOG_TYPE_LABELS: Record = { @@ -74,4 +75,5 @@ export const LOG_TYPE_LABELS: Record = { 8: 'setting', 9: 'visit', 10: 'user', + 11: 'mcp', }; diff --git a/frontend/src/features/audit/AuditPage.test.tsx b/frontend/src/features/audit/AuditPage.test.tsx index 012d6700c..6350414ec 100644 --- a/frontend/src/features/audit/AuditPage.test.tsx +++ b/frontend/src/features/audit/AuditPage.test.tsx @@ -63,7 +63,7 @@ vi.mock('$/api/client', () => ({ const auditSearchSchema = z.object({ service: z.string().optional(), username: z.string().optional(), - type: z.number().int().min(1).max(10).optional(), + type: z.number().int().min(1).max(11).optional(), env_uuid: z.string().optional(), since: z.string().optional(), until: z.string().optional(), @@ -176,6 +176,20 @@ describe('AuditPage', () => { expect(lastArgs?.type).toBe(1); }); + it('accepts the MCP audit type filter', async () => { + mockList.mockResolvedValue(makeResp([])); + renderWithProviders( + makeTestRouter('/_app/audit?type=11&page=1'), + ); + + await waitFor(() => { + expect(mockList).toHaveBeenCalled(); + }); + const calls = mockList.mock.calls; + const lastArgs = calls[calls.length - 1]?.[0]; + expect(lastArgs?.type).toBe(11); + }); + it('Apply filters button writes the username draft into the URL', async () => { const user = userEvent.setup(); mockList.mockResolvedValue(makeResp([])); diff --git a/frontend/src/features/audit/AuditPage.tsx b/frontend/src/features/audit/AuditPage.tsx index 302a8c80a..c87a16bdd 100644 --- a/frontend/src/features/audit/AuditPage.tsx +++ b/frontend/src/features/audit/AuditPage.tsx @@ -26,7 +26,7 @@ type Search = z.infer; // uses bare "tls"/"admin"/"api"). The two should not be unified — audit // readers compare to what was actually written to the column. const SERVICES = ['', 'osctrl-tls', 'osctrl-api', 'osctrl-cli'] as const; -const LOG_TYPE_KEYS = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10] as const; +const LOG_TYPE_KEYS = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11] as const; export function AuditPage() { usePageTitle('Audit'); diff --git a/frontend/src/routes/_app/audit.tsx b/frontend/src/routes/_app/audit.tsx index f8b60b515..8135416e0 100644 --- a/frontend/src/routes/_app/audit.tsx +++ b/frontend/src/routes/_app/audit.tsx @@ -6,7 +6,7 @@ import { AuditPage } from '$/features/audit/AuditPage'; export const auditSearchSchema = z.object({ service: z.string().optional(), username: z.string().optional(), - type: z.number().int().min(1).max(10).optional(), + type: z.number().int().min(1).max(11).optional(), env_uuid: z.string().optional(), since: z.string().optional(), until: z.string().optional(), diff --git a/pkg/auditlog/audit.go b/pkg/auditlog/audit.go index 451e8d3c9..9de463039 100644 --- a/pkg/auditlog/audit.go +++ b/pkg/auditlog/audit.go @@ -23,6 +23,7 @@ var LogTypes = map[uint]struct{}{ LogTypeSetting: {}, LogTypeVisit: {}, LogTypeUser: {}, + LogTypeMCP: {}, } // PageFilter describes the inputs accepted by GetPaged. @@ -121,6 +122,11 @@ const ( LogTypeSetting = 8 LogTypeVisit = 9 LogTypeUser = 10 + // LogTypeMCP marks audit entries generated by an MCP tool call. Kept + // distinct from LogTypeQuery/LogTypeNode/etc. so an operator can filter + // agent activity out of the (much larger) volume of SPA/CLI-driven + // entries, which carry no origin marker at all. + LogTypeMCP = 11 // Severities SeverityInfo = 1 SeverityWarning = 2 @@ -334,6 +340,37 @@ func (m *AuditLogManager) NewToken(username, ip string) { } } +// MCPToolCall records that an MCP client invoked a tool, tagged with the +// dedicated LogTypeMCP so an operator can filter agent activity out of the +// much larger volume of SPA/CLI-driven entries, none of which carry any +// origin marker. +// +// Recorded independently of whatever the dispatched REST handler itself +// audits for the same action (run_query still produces its own NewQuery +// entry, for example) — this is the complete record of what the agent +// asked for, including every read tool call, which never reaches a +// mutating handler at all. +// +// args is the tool's JSON-encoded input; callers are expected to have +// already bounded its size. failed marks a denied or errored call +// (permission refusal, bad input, a downstream 4xx/5xx) with +// SeverityWarning instead of SeverityInfo, so it stands out the same way +// FailedLogin does next to NewLogin. +func (m *AuditLogManager) MCPToolCall(username, tool, args, ip string, envID uint, failed bool) { + if !m.Enabled { + return + } + severity := uint(SeverityInfo) + line := fmt.Sprintf("agent called %s(%s)", tool, args) + if failed { + severity = SeverityWarning + line = fmt.Sprintf("agent call to %s(%s) was denied or failed", tool, args) + } + if err := m.CreateNew(username, line, ip, LogTypeMCP, severity, envID); err != nil { + log.Err(err).Msg("error creating MCP tool-call audit log") + } +} + // ConfAction - create new configuration action audit log entry func (m *AuditLogManager) ConfAction(username, action, ip string, envID uint) { if !m.Enabled { diff --git a/pkg/auditlog/utils.go b/pkg/auditlog/utils.go index 8705f2bd9..1ecca37ac 100644 --- a/pkg/auditlog/utils.go +++ b/pkg/auditlog/utils.go @@ -12,6 +12,7 @@ const ( LogTypeSettingStr = "Setting" LogTypeVisitStr = "Visit" LogTypeUserStr = "User" + LogTypeMCPStr = "MCP" LogTypeUnknown = "Unknown" // Severity strings SeverityInfoStr = "Info" @@ -43,6 +44,8 @@ func (m *AuditLogManager) LogTypeToString(logType uint) string { return LogTypeVisitStr case 10: return LogTypeUserStr + case 11: + return LogTypeMCPStr default: return LogTypeUnknown } diff --git a/pkg/auditlog/utils_test.go b/pkg/auditlog/utils_test.go index 088df550d..96ca9a8c9 100644 --- a/pkg/auditlog/utils_test.go +++ b/pkg/auditlog/utils_test.go @@ -17,6 +17,7 @@ func TestLogTypeToString(t *testing.T) { {8, "Setting"}, {9, "Visit"}, {10, "User"}, + {11, "MCP"}, {0, "Unknown"}, {99, "Unknown"}, }