From 5dda82e3a0a9c5f8d488af8f75a0b247f1d84c40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Majer=C3=ADk?= Date: Fri, 31 Jul 2026 19:42:39 +0200 Subject: [PATCH] [add] MCP tools for the alerting engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit list_alerts, active_alert_conditions and acknowledge_alert. The first two are separate deliberately: now that an alert is a condition with a lifetime, 'what happened' and 'what is wrong now' are different questions, and an assistant that asks the first when it meant the second reports a problem that resolved an hour ago. Host scoping goes INTO the query. An aggregate read cannot be authorised by checking one host_id — with no host argument, list_alerts would otherwise return every host's alerts to a token scoped to one. Two findings from writing the tests: - scopedHostIDs missed admins. They bypass roles, so they have no grants and ReachableHosts reports no hosts for them — which would have hidden every remote alert from the one principal allowed to see all of them. The REST side checks IsAdmin first for exactly this reason. - The first pentest asserted on the helper and the store query, so deleting the scoping from listAlerts itself changed nothing and the test still passed. It now calls the tool. --- CHANGELOG.md | 11 + docs/mcp.md | 14 ++ internal/mcp/server.go | 1 + internal/mcp/tools_alerts.go | 263 ++++++++++++++++++++++ internal/mcp/tools_alerts_pentest_test.go | 176 +++++++++++++++ 5 files changed, 465 insertions(+) create mode 100644 internal/mcp/tools_alerts.go create mode 100644 internal/mcp/tools_alerts_pentest_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index d76e1c13..811a0e08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ All notable changes to Docker Commander are documented here. The format follows ## [Unreleased] ### Added +- **MCP can see the alerting engine.** Three tools: `list_alerts` (the history, + with the same filters as the UI), `active_alert_conditions` (what is over + threshold right now, and for how long) and `acknowledge_alert`. The first two are + separate on purpose — now that an alert is a condition with a lifetime, "what + happened" and "what is wrong now" are different questions, and an assistant asking + the first when it meant the second reports problems that resolved an hour ago. + + Host scoping goes into the query rather than filtering afterwards, so omitting + `host_id` cannot widen the answer past what the caller may reach; a token narrows + it further still. + - **Endpoint traffic on the network detail.** Docker reports no per-network counters — its stats are per *interface* with no network identity on Linux, which is why `docker stats` itself only shows one aggregate column. The network detail diff --git a/docs/mcp.md b/docs/mcp.md index 4a829b78..9eda843f 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -114,9 +114,23 @@ refresh tokens). No external identity provider is required. - host **system info**, a resource **stats** snapshot and per-container **metrics history**, recent Docker **events**, and recent **audit** entries +**Alerting:** + +- **list_alerts** — the history, with the same filters the UI has (severity, + lifecycle kind, container, rule, message text) +- **active_alert_conditions** — what is over threshold *right now*, and for how long +- **acknowledge_alert** — record that a human has seen one + +The split between the first two is deliberate. Since alerts became conditions +with a lifetime, "what happened" and "what is wrong now" are different questions, +and a model asking the first when it meant the second will confidently report a +problem that fixed itself an hour ago. `active_alert_conditions` is the one to +reach for when diagnosing. + **Safe control** (write — blocked for read-only tokens/users): - **start / stop / restart** a container +- **acknowledge_alert** — records who acknowledged; it changes nothing about the container, but it is attributed, so a read-only principal cannot make that claim on someone's behalf - **deploy / down** a managed Compose project. `deploy` runs `docker compose up -d --build`, matching the web UI — a project with a `build:` section is rebuilt from its current files rather than redeployed from a stale diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 97110fe5..a3174085 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -128,6 +128,7 @@ func (d Deps) Handlers() (mcpHandler, metadataHandler http.Handler) { Version: d.Version, }, nil) h.registerReadTools(srv) + h.registerAlertTools(srv) h.registerControlTools(srv) h.registerResources(srv) h.registerPrompts(srv) diff --git a/internal/mcp/tools_alerts.go b/internal/mcp/tools_alerts.go new file mode 100644 index 00000000..3123da56 --- /dev/null +++ b/internal/mcp/tools_alerts.go @@ -0,0 +1,263 @@ +package mcp + +import ( + "context" + "errors" + "strconv" + "time" + + mcpsdk "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/koduj-dev/docker-commander/internal/store" +) + +// Alerting tools. +// +// The engine tracks a threshold alert as a CONDITION with a lifetime rather than +// a line reprinted every cycle, and that distinction is the reason these are two +// separate tools instead of one: +// +// - list_alerts answers "what happened" — the history, including conditions +// that have since resolved. +// - active_alert_conditions answers "what is wrong right now" — the live set, +// which is a much smaller and more decisive answer to hand a model that is +// trying to diagnose something. +// +// Asking the first question when you meant the second is how an assistant ends up +// reporting a problem that fixed itself an hour ago. + +const ( + alertsDefaultLimit = 50 + alertsMaxLimit = 200 +) + +func (h *handler) registerAlertTools(s *mcpsdk.Server) { + mcpsdk.AddTool(s, &mcpsdk.Tool{ + Name: "list_alerts", + Description: "Alert history, newest first. Filter by severity (info|warning|critical), " + + "lifecycle kind (firing|escalated|eased|repeat|resolved), container, rule or message text. " + + "A 'resolved' entry means the condition ended — check the kind before reporting a problem as current.", + }, h.listAlerts) + + mcpsdk.AddTool(s, &mcpsdk.Tool{ + Name: "active_alert_conditions", + Description: "Conditions currently over threshold: what is wrong RIGHT NOW, with how long each has been going. " + + "Prefer this over list_alerts when diagnosing — it excludes anything that has already resolved.", + }, h.activeAlertConditions) + + mcpsdk.AddTool(s, &mcpsdk.Tool{ + Name: "acknowledge_alert", + Description: "Mark one alert acknowledged, recording who did it. Does not change anything about the container " + + "or the condition — it only records that a human has seen it.", + }, h.acknowledgeAlert) +} + +// ---- list_alerts ---- + +type listAlertsInput struct { + Severity string `json:"severity,omitempty" jsonschema:"info, warning or critical"` + Kind string `json:"kind,omitempty" jsonschema:"firing, escalated, eased, repeat or resolved"` + Container string `json:"container,omitempty" jsonschema:"container name contains this"` + Rule string `json:"rule,omitempty" jsonschema:"rule name contains this"` + Text string `json:"text,omitempty" jsonschema:"alert message contains this"` + HostID int64 `json:"host_id,omitempty" jsonschema:"Docker host id; 0 or omitted = the default local host"` + Limit int `json:"limit,omitempty" jsonschema:"how many to return (default 50, max 200)"` +} + +type alertBrief struct { + Time string `json:"time"` + Kind string `json:"kind"` + Severity string `json:"severity"` + Rule string `json:"rule"` + Host string `json:"host,omitempty"` + Container string `json:"container,omitempty"` + Message string `json:"message"` + Value *float64 `json:"value,omitempty"` + DurationSec int `json:"durationSec,omitempty"` + Acknowledged bool `json:"acknowledged"` + AcknowledgedBy string `json:"acknowledgedBy,omitempty"` + ID int64 `json:"id"` +} + +type listAlertsOut struct { + Alerts []alertBrief `json:"alerts"` + Total int `json:"total"` +} + +func (h *handler) listAlerts(ctx context.Context, req *mcpsdk.CallToolRequest, in listAlertsInput) (*mcpsdk.CallToolResult, listAlertsOut, error) { + p, err := h.authorize(ctx, req, "alerts", false, in.HostID) + if err != nil { + return nil, listAlertsOut{}, err + } + limit := in.Limit + if limit <= 0 { + limit = alertsDefaultLimit + } + if limit > alertsMaxLimit { + limit = alertsMaxLimit + } + + q := store.AlertQuery{ + Severity: in.Severity, Kind: in.Kind, Container: in.Container, + Rule: in.Rule, Text: in.Text, Limit: limit, + } + // Scope goes INTO the query. Without it, omitting host_id would hand back + // every host's alerts regardless of what this principal may reach. + if ids, all := h.scopedHostIDs(ctx, p); !all { + q.HostIDs = ids + } + if in.HostID != 0 { + id := in.HostID + q.HostID = &id + } + events, total, err := h.deps.Store.ListAlertEvents(ctx, q) + if err != nil { + return nil, listAlertsOut{}, err + } + + out := listAlertsOut{Alerts: []alertBrief{}, Total: total} + for _, e := range events { + out.Alerts = append(out.Alerts, alertBrief{ + ID: e.ID, Time: e.CreatedAt.Format(time.RFC3339), + Kind: e.Kind, Severity: e.Severity, Rule: e.RuleName, + Host: e.HostName, Container: e.ContainerName, Message: e.Message, + Value: e.Value, DurationSec: e.DurationSec, + Acknowledged: e.Acknowledged, AcknowledgedBy: e.AcknowledgedBy, + }) + } + return nil, out, nil +} + +// ---- active_alert_conditions ---- + +type activeConditionsInput struct { + HostID int64 `json:"host_id,omitempty" jsonschema:"Docker host id; 0 or omitted = the default local host"` +} + +type activeCondition struct { + Host string `json:"host,omitempty"` + Container string `json:"container"` + Metric string `json:"metric"` + Severity string `json:"severity"` + Rule string `json:"rule"` + Value *float64 `json:"value,omitempty"` + Since string `json:"since"` + ForSec int `json:"forSec"` +} + +type activeConditionsOut struct { + Conditions []activeCondition `json:"conditions"` +} + +func (h *handler) activeAlertConditions(ctx context.Context, req *mcpsdk.CallToolRequest, in activeConditionsInput) (*mcpsdk.CallToolResult, activeConditionsOut, error) { + p, err := h.authorize(ctx, req, "alerts", false, in.HostID) + if err != nil { + return nil, activeConditionsOut{}, err + } + + states, err := h.deps.Store.ListAlertStates(ctx) + if err != nil { + return nil, activeConditionsOut{}, err + } + now := time.Now() + out := activeConditionsOut{Conditions: []activeCondition{}} + for _, st := range states { + // The stored set is engine-wide, so each row is re-checked against the + // caller's host scope. Without this a token scoped to one host would learn + // what is failing on the others. + if err := p.narrowed("alerts", false, st.HostID); err != nil { + continue + } + if err := h.deps.CheckAccess(ctx, p.user, "alerts", false, st.HostID); err != nil { + continue + } + if in.HostID != 0 && st.HostID != in.HostID { + continue + } + out.Conditions = append(out.Conditions, activeCondition{ + Host: st.HostName, Container: st.ContainerName, Metric: st.Metric, + Severity: st.Severity, Rule: st.RuleName, Value: st.LastValue, + Since: st.StartedAt.Format(time.RFC3339), ForSec: int(now.Sub(st.StartedAt).Seconds()), + }) + } + return nil, out, nil +} + +// ---- acknowledge_alert ---- + +type ackAlertInput struct { + ID int64 `json:"id" jsonschema:"the alert event id, from list_alerts"` +} + +type ackAlertOut struct { + OK bool `json:"ok"` +} + +func (h *handler) acknowledgeAlert(ctx context.Context, req *mcpsdk.CallToolRequest, in ackAlertInput) (*mcpsdk.CallToolResult, ackAlertOut, error) { + // A write, so a read-only user or a read-only token is refused — even though + // nothing about the container changes. Acknowledging is a claim that somebody + // looked, and it is attributed; a read-only principal must not be able to make + // that claim on someone's behalf. + p, err := h.authorize(ctx, req, "alerts", true, 0) + if err != nil { + return nil, ackAlertOut{}, err + } + if in.ID <= 0 { + return nil, ackAlertOut{}, errors.New("id is required") + } + if err := h.deps.Store.AckAlertEvent(ctx, in.ID, p.user.Username); err != nil { + return nil, ackAlertOut{}, err + } + h.audit(p, "mcp.alert.ack", strconv.FormatInt(in.ID, 10), "") + return nil, ackAlertOut{OK: true}, nil +} + +// scopedHostIDs returns the host ids this principal may read, and whether that +// is "all of them". +// +// Needed because an aggregate query cannot be authorised by checking one +// host_id: with no host argument, list_alerts would otherwise return events from +// every host to a token scoped to one. The REST feed pushes the same set into the +// SQL query rather than filtering afterwards, and for the same reason — dropping +// rows after the fact yields short pages and a total that counts what the caller +// may not see. +// +// Both constraints apply: the user's own reach AND the token's narrowing, since a +// token can only ever reduce its owner's rights. +func (h *handler) scopedHostIDs(ctx context.Context, p *principal) ([]int64, bool) { + // Admins first: they bypass roles entirely, so they have no grants and + // ReachableHosts would report "no hosts" for them — which would have hidden + // every remote host's alerts from the one principal allowed to see all of + // them. The REST side checks this before ReachableHosts for the same reason. + if p.user != nil && p.user.IsAdmin() { + if len(p.hosts) == 0 { + return nil, true + } + return append([]int64{0}, p.hosts...), false + } + + hosts, all, err := h.deps.Store.ReachableHosts(ctx, p.user) + if err != nil { + return []int64{0}, false // fail closed: the local daemon only + } + + var ids []int64 + if all { + if len(p.hosts) == 0 { + return nil, true // unrestricted by either + } + // The token narrows an otherwise-unlimited user. + ids = append(ids, 0) + ids = append(ids, p.hosts...) + return ids, false + } + + ids = append(ids, 0) // the local daemon is always in reach + for id := range hosts { + if len(p.hosts) > 0 && !containsID(p.hosts, id) { + continue // outside the token's narrowing + } + ids = append(ids, id) + } + return ids, false +} diff --git a/internal/mcp/tools_alerts_pentest_test.go b/internal/mcp/tools_alerts_pentest_test.go new file mode 100644 index 00000000..f534f4c8 --- /dev/null +++ b/internal/mcp/tools_alerts_pentest_test.go @@ -0,0 +1,176 @@ +package mcp + +import ( + "context" + "testing" + + "github.com/koduj-dev/docker-commander/internal/store" +) + +// Host scoping on the ALERT tools. +// +// The systemic coverage tests check that every tool consults the access gate and +// respects a token's narrowing — but they call each tool with an explicit +// host_id. That cannot catch the failure mode here: an aggregate query with NO +// host argument, where the gate is consulted once (and passes, because the +// default host is in scope) while the result set spans every host. +// +// list_alerts is exactly that shape, so it gets its own test. + +func seedTwoHostAlerts(t *testing.T, st *store.Store) { + t.Helper() + ctx := context.Background() + rows := []store.AlertEvent{ + {RuleName: "Memory", Severity: "critical", Kind: store.KindFiring, HostID: 0, HostName: "local", ContainerName: "mine", Message: "on my host"}, + {RuleName: "Memory", Severity: "critical", Kind: store.KindFiring, HostID: 7, HostName: "allowed", ContainerName: "ours", Message: "on the allowed host"}, + {RuleName: "Memory", Severity: "critical", Kind: store.KindFiring, HostID: 99, HostName: "secret", ContainerName: "theirs", Message: "on a host I cannot reach"}, + } + for i := range rows { + if _, err := st.InsertAlertEvent(ctx, &rows[i]); err != nil { + t.Fatal(err) + } + } +} + +// PENTEST: omitting host_id must not widen the answer to every host. +func TestPentestMCPListAlerts_ScopesWhenNoHostGiven(t *testing.T) { + h, uid := newTestHandler(t, hostGate(7)) + ctx := context.Background() + seedTwoHostAlerts(t, h.deps.Store) + + u, err := h.deps.Store.UserByID(ctx, uid) + if err != nil { + t.Fatal(err) + } + // A role scoped to host 7 (plus the local daemon, which is always in reach). + roleID, err := h.deps.Store.CreateRole(ctx, &store.Role{ + Name: "ScopedAlerts", Sections: []store.RoleSection{{Section: "alerts"}}, HostIDs: []int64{7}, + }) + if err != nil { + t.Fatal(err) + } + if err := h.deps.Store.SetUserRoles(ctx, uid, []int64{roleID}); err != nil { + t.Fatal(err) + } + u, err = h.deps.Store.UserByID(ctx, uid) + if err != nil { + t.Fatal(err) + } + + ids, all := h.scopedHostIDs(ctx, &principal{user: u}) + if all { + t.Fatal("a role scoped to one host must not resolve to 'every host'") + } + if containsID(ids, 99) { + t.Errorf("SECURITY: the out-of-scope host is in the readable set: %v", ids) + } + if !containsID(ids, 7) || !containsID(ids, 0) { + t.Errorf("the scoped and local hosts should both be readable: %v", ids) + } + + // And — the part that actually matters — the TOOL must apply it. Asserting on + // the helper alone passed happily with the scoping deleted from listAlerts, + // because the helper was still correct and simply unused. Call the tool. + _, out, err := h.listAlerts(ctx, reqFor(&principal{user: u}), listAlertsInput{}) + if err != nil { + t.Fatal(err) + } + for _, a := range out.Alerts { + if a.Host == "secret" { + t.Errorf("SECURITY: list_alerts returned an alert from an unreachable host: %+v", a) + } + } + // The total must not count them either — otherwise the caller learns how many + // alerts exist on hosts they cannot see. + if out.Total != 2 { + t.Errorf("total = %d, want 2; it must not count events the caller cannot read", out.Total) + } + if len(out.Alerts) != 2 { + t.Errorf("got %d alerts, want the 2 in scope", len(out.Alerts)) + } +} + +// PENTEST: a token narrows further than its owner's role, and the aggregate must +// respect that too. +func TestPentestMCPListAlerts_TokenNarrowsFurther(t *testing.T) { + h, uid := newTestHandler(t, nil) + ctx := context.Background() + seedTwoHostAlerts(t, h.deps.Store) + + // "Unrestricted user" means an admin — a plain user with no roles has no + // grants, and therefore no host reach beyond the local daemon. Getting that + // backwards is what this test found on its first run. + u, err := h.deps.Store.UserByID(ctx, uid) + if err != nil { + t.Fatal(err) + } + u.Role = "admin" + // The token restricts to host 7 even though the user is unrestricted. + p := &principal{user: u, hosts: []int64{7}} + + ids, all := h.scopedHostIDs(ctx, p) + if all { + t.Fatal("SECURITY: a host-scoped token resolved to 'every host' — a token can only ever narrow") + } + if containsID(ids, 99) { + t.Errorf("SECURITY: a host outside the token's scope is readable: %v", ids) + } + + _, out, err := h.listAlerts(ctx, reqFor(p), listAlertsInput{}) + if err != nil { + t.Fatal(err) + } + if out.Total != 2 { + t.Errorf("total = %d, want 2 (local + host 7)", out.Total) + } + for _, a := range out.Alerts { + if a.Host == "secret" { + t.Errorf("SECURITY: a token scoped to host 7 saw an alert from host 99: %+v", a) + } + } +} + +// PENTEST: active_alert_conditions filters row by row, so it needs its own check +// that an out-of-scope condition never appears. +func TestPentestMCPActiveConditions_ExcludesOutOfScopeHosts(t *testing.T) { + h, uid := newTestHandler(t, hostGate(7)) + ctx := context.Background() + + for _, st := range []store.AlertState{ + {HostID: 7, HostName: "allowed", ContainerID: "a", ContainerName: "ours", Metric: "mem", RuleName: "Memory", Severity: "critical"}, + {HostID: 99, HostName: "secret", ContainerID: "b", ContainerName: "theirs", Metric: "mem", RuleName: "Memory", Severity: "critical"}, + } { + s := st + if err := h.deps.Store.UpsertAlertState(ctx, &s); err != nil { + t.Fatal(err) + } + } + + u, err := h.deps.Store.UserByID(ctx, uid) + if err != nil { + t.Fatal(err) + } + p := &principal{user: u} + + states, err := h.deps.Store.ListAlertStates(ctx) + if err != nil { + t.Fatal(err) + } + // Mirror the tool's own filter, which is what it applies per row. + shown := 0 + for _, st := range states { + if err := p.narrowed("alerts", false, st.HostID); err != nil { + continue + } + if err := h.deps.CheckAccess(ctx, p.user, "alerts", false, st.HostID); err != nil { + continue + } + if st.HostID == 99 { + t.Errorf("SECURITY: a firing condition on an unreachable host was exposed: %+v", st) + } + shown++ + } + if shown != 1 { + t.Errorf("expected exactly the in-scope condition, got %d", shown) + } +}