diff --git a/server/cmd/main.go b/server/cmd/main.go index acb7991..3acbbf8 100644 --- a/server/cmd/main.go +++ b/server/cmd/main.go @@ -293,7 +293,7 @@ func serve() { log.Fatalf("api scope map: %v", err) } - if err := api.AssertServerScopeMapComplete(serverTouchingRoutes(r)); err != nil { + if err := api.AssertServerScopeMapComplete(apiRoutes(r)); err != nil { log.Fatalf("api server scope map: %v", err) } @@ -359,41 +359,24 @@ func boolEnv(key string) bool { return false } -// serverTouchingRoutes restricts AssertServerScopeMapComplete to routes whose -// pattern names server-derived data — "server", ":serverId", "console" or -// "assign", or the exact "workflows/:id/run" — rather than every routeScopes -// entry, so unrelated routes are never swept in and boot never fails for no -// reason. ":serverId" and "assign" were added after POST /api/keys/:id/assign -// turned out to resolve a named server (in its request body, not its path) -// with no scope check and no entry in serverScopedRoutes at all — the -// original three-way filter never saw it because nothing in its path pattern -// said "server". +// apiRoutes lists every registered /api route as "METHOD /path", which is the +// whole input AssertServerScopeMapComplete now takes. // -// A substring filter is the weak part of this design: it only catches a route -// whose *path* names a server, and a route can act on a server named in its -// body, a query parameter, or an ID a handler derives some other way, with a -// path that says nothing about it — as the keys/assign route did. Each time -// that happens the fix is another substring added here, which finds this -// class of gap one instance late rather than by construction. The stronger -// design would invert the model: every /api route declares itself in -// serverScopedRoutes (or an adjacent map), with an explicit exemption list -// for the handful that genuinely act on nothing server-scoped, so a new route -// is checked by default rather than only if its path happens to match a -// pattern someone thought to add. That inversion is a larger change than -// this filter widening and is left as a follow-up, not done here. -func serverTouchingRoutes(r *gin.Engine) []string { +// It replaces a substring filter that fed in only routes whose path contained +// "server", ":serverId", "console" or "assign". That filter could only ever +// catch a route whose *path* named a server, and a route can act on one named +// in its body, in a query parameter, or derived by the handler — it caught one +// of the leaks found in the final review of the MCP feature, and none of the +// eleven found during implementation. Declaring every route is more typing +// once and no maintenance after: a new route fails boot until somebody answers +// "does this touch server data?" for it. +func apiRoutes(r *gin.Engine) []string { var out []string for _, route := range r.Routes() { if !strings.HasPrefix(route.Path, "/api/") { continue } - if strings.Contains(route.Path, "server") || - strings.Contains(route.Path, ":serverId") || - strings.Contains(route.Path, "console") || - strings.Contains(route.Path, "assign") || - route.Path == "/api/workflows/:id/run" { - out = append(out, route.Method+" "+route.Path) - } + out = append(out, route.Method+" "+route.Path) } return out } diff --git a/server/internal/api/serverscope.go b/server/internal/api/serverscope.go index f27a90a..268e883 100644 --- a/server/internal/api/serverscope.go +++ b/server/internal/api/serverscope.go @@ -2,168 +2,71 @@ package api import "fmt" -// serverScopedRoutes declares, for every route that returns or acts on -// server-derived data, whether it honours the acting token's tag restriction. +// scopeDecl is one route's declaration about server-derived data. +type scopeDecl int + +const ( + // scoped: the handler honours the acting token's tag restriction. + scoped scopeDecl = iota + // fleetWide: the route deliberately reaches the whole fleet. Every + // fleetWide entry carries a comment giving the reason. It must never mean + // "not scoped yet" — an unresolved gap belongs on a fix list, not here, + // because this value is read as a considered decision. + fleetWide + // exempt: the route touches no server-derived data at all. Every exempt + // entry carries a comment saying why, because "this reads no server data" + // is exactly the claim that turns out to be wrong when a handler later + // grows a server lookup. + exempt +) + +// serverScopedRoutes declares, for EVERY registered /api route, whether it +// honours the acting token's tag restriction. // -// It is a maintained map for the same reason routeScopes is: a route added -// tomorrow that reads server data without filtering would leak a restricted -// token's blind spot silently, and this turns that into a failure at boot. +// The declaration is inverted from what it used to be. It was once a partial +// map checked only against routes whose path contained "server", "console" or +// "assign"; that filter caught one of the routes found leaking in the final +// review of this feature, and none of the eleven found during implementation, +// because a route can act on a server named in its body, in a query parameter +// or derived by the handler, with a path saying nothing about it. Every route +// must now appear here with an explicit value and boot fails on an undeclared +// one, so the question "does this touch server data?" is asked once per route +// by construction rather than when someone thinks to widen a pattern. // -// false means the route is deliberately fleet-wide and requires a comment -// explaining the deliberate reason. It must never mean "not scoped yet" — -// an unresolved gap belongs on the fix list, not in this map, because a false -// entry here is read as a considered decision, not a placeholder. -var serverScopedRoutes = map[string]bool{ - "GET /api/servers": true, - "GET /api/servers/:id": true, - "DELETE /api/servers/:id": true, - "POST /api/servers/:id/apply-updates": true, - "POST /api/servers/:id/update-agent": true, - "PUT /api/servers/:id/tags": true, - "POST /api/servers/:id/generate-key": true, - "POST /api/console/connect": true, - "GET /api/console/tunnel": true, - "POST /api/workflows/:id/run": true, +// Two things this assertion cannot do, and one of them has already bitten: +// +// 1. It can only ever check that a DECLARATION EXISTS, never that the handler +// honours it. "POST /api/workflows/:id/run" was declared scoped here while +// services.TriggerWorkflow resolved its targets through the unscoped +// ResolveTargets — a true entry that lied, boot-enforced, for the whole +// life of the feature. A declaration is a claim a reviewer must verify, +// not a property this file establishes. +// +// 2. /api/mcp is exempt at route level, and that is the honest answer rather +// than an omission. One route serves roughly twenty tools of very +// different shapes — some read no server data at all, some resolve one +// host, some enumerate the fleet — so no single route-level value could +// be true of all of them. The decision genuinely lives per tool, where +// each tool that touches server data applies auth.ServerScope's selector +// itself, and the registry's own tests are where that is enforced. +var serverScopedRoutes = map[string]scopeDecl{ - // Workload routes all resolve the server through GetServerScoped before - // touching anything. - "GET /api/servers/:id/workloads": true, - "POST /api/servers/:id/workloads/refresh": true, - "POST /api/servers/:id/workloads/:wid/action": true, - "GET /api/servers/:id/workloads/:wid/logs": true, + // ---- servers ---- - // listServerVulnerabilities and getServerPackages now resolve the server - // through GetServerScoped before calling ListFindings/ListPackages, so an - // out-of-scope server ID reads as not-found before either function runs. - "GET /api/servers/:id/vulnerabilities": true, - "GET /api/servers/:id/packages": true, - - // getServerRunLog/streamServerRunLog resolve :serverId through - // GetServerScoped before reading anything from the log store, so a - // restricted token holding a valid runId still cannot read output from a - // server outside its scope. - "GET /api/runs/:runId/servers/:serverId/logs": true, - "GET /api/runs/:runId/servers/:serverId/logs/stream": true, - - // revokeAssignment resolves :serverId through GetServerScoped before - // calling RevokeAssignment, so a restricted token cannot mutate an - // assignment on a server outside its scope. - "DELETE /api/keys/:id/assign/:serverId": true, - - // assignKey resolves body.ServerID through GetServerScoped before calling - // services.AssignKey, which itself uses the unscoped GetServer — so a - // restricted token can no longer assign a key to a server outside its - // scope by naming it in the request body. serverTouchingRoutes in - // cmd/main.go now matches on "assign" as well as "server", so this entry - // is boot-enforced like its sibling revoke route. - "POST /api/keys/:id/assign": true, - - // getKey filters services.GetAssignmentsWithServers' result down to - // assignments whose server passes services.ServerInTokenScope(*, before - // returning it, so a restricted token cannot learn the hostname of an - // out-of-scope server through a key it happens to also hold there. The - // key document itself is still returned unfiltered — a token restricted - // to staging may legitimately hold a key that is also assigned in prod, - // and only the assignment list, not the key's existence, is the leak - // this closes. This route's path carries none of "server", ":serverId", - // "console" or "assign", so it is not swept in by serverTouchingRoutes - // and this entry is not boot-enforced — kept anyway as the record of a - // considered decision, same as the assign/revoke routes above before - // their substrings were added to the filter. - "GET /api/keys/:id": true, - - // listKeys' services.ListKeys narrows each key's AssignedCount to - // assignments on servers ServerInTokenScope admits, for the same reason - // as getKey above: a nonzero count on a key a restricted token sees - // nothing assigned to in its own scope is itself the leak — it tells the - // token an assignment exists on a host it must not know about, without - // naming the host. This route's path carries none of "server", - // ":serverId", "console" or "assign" either, so — like GET - // /api/keys/:id — it is not swept in by serverTouchingRoutes and this - // entry is not boot-enforced. Widening the filter to catch "keys" was - // deliberately not done: it would sweep in every other /keys route - // (create, delete, private-key) that has nothing to do with servers. - // The real fix for routes like this and GET /api/keys/:id is the - // declare-by-default inversion already recorded as a follow-up above. - "GET /api/keys": true, - - // listMonitors/getMonitor redact models.Monitor.Runner to - // models.RunnerRestricted via services.RedactMonitorRunner when it names - // a server outside the caller's scope — Runner is literally a server ID - // for an agent-pushed monitor, so left unfiltered it discloses one - // directly, worse than the key list's count. The monitor itself is still - // returned: a restricted operator may legitimately need to see that it - // exists and is up or down, so only the runner field goes neutral rather - // than the whole monitor disappearing from the list. Runner "server" - // (control-plane-run) is never touched — it names no server. Neither - // path carries any of serverTouchingRoutes' substrings, so like the two - // key routes above these entries are not boot-enforced. - "GET /api/monitors": true, - "GET /api/monitors/:id": true, - - // listWorkflows/getWorkflow narrow Workflow.TargetServerIDs to what the - // caller's scope admits via services.VisibleServerIDs + - // FilterVisibleServerIDs, wrapped in WorkflowResponse so the JSON field - // name is unchanged. TargetTags is left untouched — the tag vocabulary - // itself is ruled acceptable to expose, unlike a resolved server ID. - // TargetsRestricted is set (with no count) whenever at least one target - // was dropped, so a caller is told some targets are hidden rather than - // silently shown a shorter list that happens to match what - // run_workflow's all-or-nothing scope refusal then also acts on — the - // two cannot appear to disagree with each other. Neither path carries - // any of serverTouchingRoutes' substrings, so like the routes above - // these entries are not boot-enforced. - "GET /api/workflows": true, - "GET /api/workflows/:id": true, - - // createWorkflow/updateWorkflow validate target_server_ids through - // services.validateTargetServers, which resolves each named ID with - // GetServerScoped — so a restricted token can neither save a workflow - // targeting a host outside its scope (which the scheduler, firing as the - // system, would otherwise run there) nor learn which IDs exist by the - // difference between "target server not found" and a successful save. - "POST /api/workflows": true, - "PUT /api/workflows/:id": true, - - // createMonitor/updateMonitor validate the runner — which is a server ID - // for an agent-pushed monitor — through services.validateRunner, now - // resolving with GetServerScoped for the same two reasons. - "POST /api/monitors": true, - "PUT /api/monitors/:id": true, - - // getRun and listWorkflowRuns narrow WorkflowRun.ServerRuns — each entry - // of which carries a ServerID and a Hostname — to what the caller's scope - // admits, setting servers_restricted (a boolean, never a count) when any - // entry was dropped. These are the parents of the per-server log routes - // above: scoping the logs but not the document naming the hosts left the - // hostnames readable without the output. - "GET /api/runs/:runId": true, - "GET /api/workflows/:id/runs": true, - - // searchPackages passes the caller's selector into services.SearchPackages, - // which drops hits on servers outside it using one VisibleServerIDs - // membership set. The MCP search_fleet tool answers the same question and - // was already scoped; this makes the REST twin agree. - "GET /api/packages/search": true, - - // listWorkloads passes the caller's selector into services.SearchWorkloads, - // which drops hits on servers outside it. A WorkloadHit names a server ID, - // so the fleet-wide form enumerated hosts directly. - "GET /api/workloads": true, - - // listVulnerabilities passes the selector as FindingFilter.TokenScope, - // narrowing server_id in the same query the Tags selector already narrows, - // and vulnerabilitySummary passes it to CountOpenFindingsBySeverity so the - // summary tiles count only visible servers. - "GET /api/vulnerabilities": true, - "GET /api/vulnerabilities/summary": true, + "GET /api/servers": scoped, + "GET /api/servers/:id": scoped, + "DELETE /api/servers/:id": scoped, + "POST /api/servers/:id/apply-updates": scoped, + "POST /api/servers/:id/update-agent": scoped, + "PUT /api/servers/:id/tags": scoped, + "POST /api/servers/:id/generate-key": scoped, // Creating a server has no server to filter yet. - "POST /api/servers": false, + "POST /api/servers": fleetWide, // The agent's own enrolment routes authenticate as the agent, not as a // user token, so no session selector exists to apply. - "GET /api/servers/new": false, - "POST /api/servers/new": false, + "GET /api/servers/new": fleetWide, + "POST /api/servers/new": fleetWide, // KnownTags aggregates the tag *vocabulary* in use across the fleet — keys // and the values seen for them — never a server identifier or any other @@ -171,31 +74,319 @@ var serverScopedRoutes = map[string]bool{ // hosts exist. Filtering it would mean plumbing a selector through an // aggregation query for a leak that carries no server identity; ruled // acceptable to leave fleet-wide rather than take that on for this. - "GET /api/servers/tags": false, + "GET /api/servers/tags": fleetWide, + + // ---- console ---- + + "POST /api/console/connect": scoped, + "GET /api/console/tunnel": scoped, + + // ---- workloads ---- + + // Workload routes all resolve the server through GetServerScoped before + // touching anything. + "GET /api/servers/:id/workloads": scoped, + "POST /api/servers/:id/workloads/refresh": scoped, + "POST /api/servers/:id/workloads/:wid/action": scoped, + "GET /api/servers/:id/workloads/:wid/logs": scoped, + + // listWorkloads passes the caller's selector into services.SearchWorkloads, + // which drops hits on servers outside it. A WorkloadHit names a server ID, + // so the fleet-wide form enumerated hosts directly. + "GET /api/workloads": scoped, + + // ---- vulnerabilities and packages ---- + + // listServerVulnerabilities and getServerPackages resolve the server + // through GetServerScoped before calling ListFindings/ListPackages, so an + // out-of-scope server ID reads as not-found before either function runs. + "GET /api/servers/:id/vulnerabilities": scoped, + "GET /api/servers/:id/packages": scoped, + + // listVulnerabilities passes the selector as FindingFilter.TokenScope, + // narrowing server_id in the same query the Tags selector already narrows, + // and vulnerabilitySummary passes it to CountOpenFindingsBySeverity so the + // summary tiles count only visible servers. + "GET /api/vulnerabilities": scoped, + "GET /api/vulnerabilities/summary": scoped, + + // searchPackages passes the caller's selector into services.SearchPackages, + // which drops hits on servers outside it using one VisibleServerIDs + // membership set. The MCP search_fleet tool answers the same question and + // was already scoped; this makes the REST twin agree. + "GET /api/packages/search": scoped, + + // Rescan flags the whole fleet and returns a count of servers flagged, not + // their identities. Scanning is a control-plane background job with no + // caller-visible per-server effect, and a partial rescan would leave the + // findings a restricted token *can* see computed against a stale database. + // Owner|admin only in any case. + "POST /api/vulnerabilities/rescan": fleetWide, + + // Accepting or reopening a finding names the finding, not a server, but a + // finding does belong to one — so a restricted token can accept a finding + // on a host outside its scope if it learns the finding ID. It cannot learn + // one through this API any more (every listing is now scoped), so this is + // left fleet-wide rather than given a lookup of its own. Owner|admin only. + "POST /api/vulnerabilities/:id/accept": fleetWide, + "DELETE /api/vulnerabilities/:id/accept": fleetWide, + + // Vuln alert rules carry severities and tag selectors, never server IDs. + "GET /api/vuln-rules": exempt, + "POST /api/vuln-rules": exempt, + "PUT /api/vuln-rules/:id": exempt, + "DELETE /api/vuln-rules/:id": exempt, + + // ---- keys ---- + + // getKey filters services.GetAssignmentsWithServers' result down to + // assignments whose server passes services.ServerInTokenScope before + // returning it, so a restricted token cannot learn the hostname of an + // out-of-scope server through a key it happens to also hold there. The + // key document itself is still returned unfiltered — a token restricted + // to staging may legitimately hold a key that is also assigned in prod, + // and only the assignment list, not the key's existence, is the leak + // this closes. + "GET /api/keys/:id": scoped, + + // listKeys' services.ListKeys narrows each key's AssignedCount to + // assignments on servers ServerInTokenScope admits, for the same reason + // as getKey above: a nonzero count on a key a restricted token sees + // nothing assigned to in its own scope is itself the leak — it tells the + // token an assignment exists on a host it must not know about, without + // naming the host. + "GET /api/keys": scoped, + + // assignKey resolves body.ServerID through GetServerScoped before calling + // services.AssignKey, and revokeAssignment resolves :serverId the same way + // before calling RevokeAssignment. + "POST /api/keys/:id/assign": scoped, + "DELETE /api/keys/:id/assign/:serverId": scoped, + + // Uploading a key and reading its stored private half touch no server: + // a key exists in the library before it is assigned anywhere. + "POST /api/keys": exempt, + "GET /api/keys/:id/private-key": exempt, + + // Deleting a key removes it everywhere it is assigned, including on hosts + // outside a restricted token's scope — the delete is of the key, not of a + // server, and there is no partial delete that leaves a key half-revoked. + // Nothing about which hosts held it is disclosed by the call. + "DELETE /api/keys/:id": fleetWide, + + // ---- workflows, steps and runs ---- + + // listWorkflows/getWorkflow narrow Workflow.TargetServerIDs to what the + // caller's scope admits via services.VisibleServerIDs + + // FilterVisibleServerIDs, wrapped in WorkflowResponse so the JSON field + // name is unchanged. TargetTags is left untouched — the tag vocabulary + // itself is ruled acceptable to expose, unlike a resolved server ID. + // TargetsRestricted is set (with no count) whenever at least one target + // was dropped. + "GET /api/workflows": scoped, + "GET /api/workflows/:id": scoped, + + // createWorkflow/updateWorkflow validate target_server_ids through + // services.validateTargetServers, which resolves each named ID with + // GetServerScoped — so a restricted token can neither save a workflow + // targeting a host outside its scope (which the scheduler, firing as the + // system, would otherwise run there) nor learn which IDs exist by the + // difference between "target server not found" and a successful save. + "POST /api/workflows": scoped, + "PUT /api/workflows/:id": scoped, + + // runWorkflow passes auth.ServerScope into services.TriggerWorkflow, which + // resolves through ResolveTargetsScoped. Note the history: this entry read + // scoped for the whole life of the feature while TriggerWorkflow called + // the UNSCOPED ResolveTargets — see this file's header on what this + // assertion can and cannot prove. + "POST /api/workflows/:id/run": scoped, + + // getRun and listWorkflowRuns narrow WorkflowRun.ServerRuns — each entry + // of which carries a ServerID and a Hostname — to what the caller's scope + // admits, setting servers_restricted (a boolean, never a count) when any + // entry was dropped. + "GET /api/runs/:runId": scoped, + "GET /api/workflows/:id/runs": scoped, + + // getServerRunLog/streamServerRunLog resolve :serverId through + // GetServerScoped before reading anything from the log store, so a + // restricted token holding a valid runId still cannot read output from a + // server outside its scope. + "GET /api/runs/:runId/servers/:serverId/logs": scoped, + "GET /api/runs/:runId/servers/:serverId/logs/stream": scoped, + + // Deleting a workflow, cancelling a run and arming a schedule all act on a + // definition rather than on a server, and none of them returns server + // data. Each can nevertheless reach a definition whose targets a + // restricted token cannot see — a cancel stops work on out-of-scope hosts, + // a schedule arms it there. That reach is real but bounded: the caller + // learns nothing about which hosts are involved (both /workflows listings + // are scoped), and a scope-narrowed variant of "cancel this run" would + // have to either half-cancel a run or refuse one whose targets are mixed, + // neither of which is a better answer than the current one. Recorded as a + // deliberate choice, not an oversight. + "DELETE /api/workflows/:id": fleetWide, + "POST /api/runs/:runId/cancel": fleetWide, + "PUT /api/workflows/:id/schedule": fleetWide, + "GET /api/workflows/:id/schedule/preview": exempt, + + // A step is a script with declared inputs and outputs. It names no server + // and is not bound to one; targeting happens at the workflow level. + "GET /api/steps": exempt, + "POST /api/steps": exempt, + "PUT /api/steps/:id": exempt, + "DELETE /api/steps/:id": exempt, + "GET /api/steps/:id/export": exempt, + "POST /api/steps/import": exempt, + "POST /api/steps/parse": exempt, + "POST /api/steps/seed-defaults": exempt, + // StepUsageCounts counts workflows per step, never servers. + "GET /api/steps/usage": exempt, + + // ---- monitors ---- + + // listMonitors/getMonitor redact models.Monitor.Runner to + // models.RunnerRestricted via services.RedactMonitorRunner when it names + // a server outside the caller's scope — Runner is literally a server ID + // for an agent-pushed monitor, so left unfiltered it discloses one + // directly. The monitor itself is still returned: a restricted operator + // may legitimately need to see that it exists and is up or down, so only + // the runner field goes neutral. Runner "server" (control-plane-run) is + // never touched — it names no server. + "GET /api/monitors": scoped, + "GET /api/monitors/:id": scoped, + + // createMonitor/updateMonitor validate the runner — which is a server ID + // for an agent-pushed monitor — through services.validateRunner, resolving + // with GetServerScoped so a restricted token can neither point a check at + // an out-of-scope agent nor use the not-found answer as an oracle. + "POST /api/monitors": scoped, + "PUT /api/monitors/:id": scoped, + + // Deleting a monitor removes the check, not a server, and returns nothing + // about where it ran. The runner field it might have named is already + // redacted on every read path, so a restricted token cannot learn one to + // then act on. + "DELETE /api/monitors/:id": fleetWide, + + // A monitor's incidents, uptime rollups and recent samples are all about + // the monitored endpoint — status, latency, timestamps — and carry no + // server identifier at all; the runner is a field of the monitor + // document, which these do not return. + "GET /api/monitors/:id/incidents": exempt, + "GET /api/monitors/:id/uptime": exempt, + "GET /api/monitors/:id/samples": exempt, + + // ---- notification channels ---- + + // A channel is an outbound destination — a webhook URL, an SMTP account. + // Nothing about a server reaches these routes. + "GET /api/channels": exempt, + "POST /api/channels": exempt, + "PUT /api/channels/:id": exempt, + "DELETE /api/channels/:id": exempt, + "POST /api/channels/:id/test": exempt, + + // ---- secrets ---- + + // Vault secrets are key/value pairs grouped by name, consumed by workflow + // steps at execution time. No secret is bound to a server, and no server + // attribute is returned by any of these. + "GET /api/secrets": exempt, + "POST /api/secrets": exempt, + "GET /api/secrets/:group": exempt, + "PUT /api/secrets/:group": exempt, + "DELETE /api/secrets/:group": exempt, + "DELETE /api/secrets/:group/:key": exempt, + "POST /api/secrets/:group/reveal": exempt, + + // ---- status pages ---- + + // A status page pairs monitor IDs with per-page display names, and every + // public read goes through services.assembleSnapshot, which is the + // redaction boundary — its PublicComponent vocabulary has no field for a + // host, URL or runner. These authoring routes handle the page document + // itself and never a server. + "GET /api/status-pages": exempt, + "POST /api/status-pages": exempt, + "GET /api/status-pages/:pageId": exempt, + "PUT /api/status-pages/:pageId": exempt, + "DELETE /api/status-pages/:pageId": exempt, + "GET /api/status-pages/:pageId/incidents": exempt, + "POST /api/status-pages/:pageId/incidents": exempt, + "PUT /api/status-pages/:pageId/incidents/:incidentId": exempt, + "DELETE /api/status-pages/:pageId/incidents/:incidentId": exempt, + "POST /api/status-pages/:pageId/incidents/:incidentId/updates": exempt, + + // ---- audit ---- + + // Audit rows are a record of what people and tokens did, and a row's free + // text detail can name a host in passing ("run triggered", "key + // assigned to web-01"). Filtering the log by tag would mean parsing those + // strings, or dropping every row whose target this token cannot resolve — + // which would hide a restricted token's own actions from itself the + // moment a server is renamed or deleted. The log is left whole and + // deliberately so: an audit trail with holes in it is worth less than the + // disclosure is worth avoiding, and the route is settings:read. + "GET /api/audit": fleetWide, + + // ---- instance administration ---- + + // Members, roles, single sign-on, settings and the licence are all + // instance-level configuration. None reads the servers collection. + "GET /api/instance/users": exempt, + "POST /api/instance/users": exempt, + "PUT /api/instance/users/:id/role": exempt, + "DELETE /api/instance/users/:id": exempt, + "GET /api/auth/providers": exempt, + "POST /api/auth/providers": exempt, + "PUT /api/auth/providers/:id": exempt, + "DELETE /api/auth/providers/:id": exempt, + "POST /api/auth/providers/:id/test": exempt, + "POST /api/auth/providers/:id/ack-notice": exempt, + "GET /api/auth/presets": exempt, + "GET /api/settings": exempt, + "PUT /api/settings": exempt, + "POST /api/settings/secrets-token": exempt, + "GET /api/license": exempt, + "POST /api/license": exempt, + + // A token document carries a tag selector but no server: minting one + // checks the selector is no wider than the caller's own + // (services.SelectorNarrowerOrEqual), which reads the caller's session, + // not the fleet. + "GET /api/tokens": exempt, + "GET /api/tokens/scopes": exempt, + "POST /api/tokens": exempt, + "DELETE /api/tokens/:id": exempt, + + // Reference documentation and the agent version lookup are static or read + // from a release feed. + "GET /api/openapi.json": exempt, + "GET /api/docs": exempt, + "GET /api/docs/scalar.js": exempt, + "GET /api/agent/latest-version": exempt, + + // ---- MCP ---- + + // Exempt at route level, for the reason set out in this file's header: + // one route serves many tools, so the answer genuinely lives per tool. + // Each tool touching server data applies the caller's selector itself. + "POST /api/mcp": exempt, + "GET /api/mcp": exempt, } -// POST/GET /api/mcp is deliberately absent from this map. main.go's -// serverTouchingRoutes only feeds in routes whose path contains "server", -// ":serverId", "console" or "assign" (or the one named workflow-run -// exception), and /api/mcp matches none of those, so it is never presented to -// AssertServerScopeMapComplete — there is nothing to declare true or false -// here. That is the right outcome: -// the single MCP route fronts many tools of very different shapes, several of -// which touch no server data at all, so a route-level entry could not say -// anything meaningful about tag scoping. Each tool that does read or act on -// server data applies auth.ServerScope itself, the same selector the REST -// handlers for those resources already apply, which is where this kind of -// scoping decision belongs for a many-operations-per-route endpoint. - -// AssertServerScopeMapComplete refuses to boot when a route touching server -// data is missing from serverScopedRoutes. +// AssertServerScopeMapComplete refuses to boot when any registered /api route +// is missing from serverScopedRoutes. routes is every /api route the engine +// registered — not a filtered subset — which is the whole point of the +// inversion: a new route is checked by default rather than only when its path +// happens to match a pattern somebody remembered to add. func AssertServerScopeMapComplete(routes []string) error { for _, r := range routes { if _, ok := serverScopedRoutes[r]; !ok { - if _, guarded := routeScopes[r]; !guarded { - continue - } - return fmt.Errorf("route %q is not declared in serverScopedRoutes", r) + return fmt.Errorf("route %q is not declared in serverScopedRoutes "+ + "(declare it scoped, fleetWide with a reason, or exempt with a reason)", r) } } return nil diff --git a/server/internal/api/serverscope_test.go b/server/internal/api/serverscope_test.go new file mode 100644 index 0000000..35eb399 --- /dev/null +++ b/server/internal/api/serverscope_test.go @@ -0,0 +1,32 @@ +package api + +import "testing" + +// The two maps must name exactly the same routes. AssertScopeMapComplete +// already fails boot on an /api route missing from routeScopes, so making +// serverScopedRoutes agree with routeScopes is what makes the inverted +// server-scope assertion total without needing a running engine to check it. +func TestServerScopeMapCoversEveryScopedRoute(t *testing.T) { + for r := range routeScopes { + if _, ok := serverScopedRoutes[r]; !ok { + t.Errorf("route %q is in routeScopes but not declared in serverScopedRoutes", r) + } + } + for r := range serverScopedRoutes { + if _, ok := routeScopes[r]; !ok { + t.Errorf("route %q is declared in serverScopedRoutes but is not a registered route", r) + } + } +} + +// A route that vanished from the engine but stayed here would make the +// assertion pass while declaring nothing real, so the assertion itself is +// tested for the one thing it does promise: an undeclared route fails. +func TestAssertServerScopeMapCompleteRejectsUndeclaredRoute(t *testing.T) { + if err := AssertServerScopeMapComplete([]string{"GET /api/servers"}); err != nil { + t.Fatalf("declared route rejected: %v", err) + } + if err := AssertServerScopeMapComplete([]string{"GET /api/brand-new"}); err == nil { + t.Fatal("undeclared route accepted; boot would not fail on it") + } +}