diff --git a/server/cmd/main.go b/server/cmd/main.go index e2325ff..acb7991 100644 --- a/server/cmd/main.go +++ b/server/cmd/main.go @@ -360,9 +360,27 @@ func boolEnv(key string) bool { } // serverTouchingRoutes restricts AssertServerScopeMapComplete to routes whose -// pattern names server-derived data — "server", "console" or -// "workflows/:id/run" — rather than every routeScopes entry, so unrelated -// routes are never swept in and boot never fails for no reason. +// 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". +// +// 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 { var out []string for _, route := range r.Routes() { @@ -370,7 +388,9 @@ func serverTouchingRoutes(r *gin.Engine) []string { 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) } diff --git a/server/internal/api/handlers.go b/server/internal/api/handlers.go index 3e2f153..aebab55 100644 --- a/server/internal/api/handlers.go +++ b/server/internal/api/handlers.go @@ -573,7 +573,22 @@ func getKey(c *gin.Context) { return } - assignments, _ := services.GetAssignmentsWithServers(auth.InstanceID(c), id) + all, _ := services.GetAssignmentsWithServers(auth.InstanceID(c), id) + + // A tag-restricted token may legitimately hold a key that is also + // assigned to a server outside its restriction — the key itself is + // still returned above. Only the assignment list is filtered, and + // silently: an assignment whose Server is nil or out of scope is + // dropped rather than kept with the hostname redacted, so the response + // gives no signal — not even a count — of what was removed. + scope := auth.ServerScope(c) + assignments := make([]services.AssignmentWithServer, 0, len(all)) + for _, a := range all { + if a.Server != nil && !services.ServerInTokenScope(*a.Server, scope) { + continue + } + assignments = append(assignments, a) + } c.JSON(http.StatusOK, KeyDetailResponse{ Key: key, diff --git a/server/internal/api/serverscope.go b/server/internal/api/serverscope.go index d58885f..9daf29c 100644 --- a/server/internal/api/serverscope.go +++ b/server/internal/api/serverscope.go @@ -53,13 +53,25 @@ var serverScopedRoutes = map[string]bool{ // 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. This route's path carries - // neither "server" nor "console", so it is not swept in by - // serverTouchingRoutes and this entry is not boot-enforced; it is kept - // here anyway as the record of a considered decision, matching its - // sibling revoke route. + // 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, + // Creating a server has no server to filter yet. "POST /api/servers": false, // The agent's own enrolment routes authenticate as the agent, not as a @@ -77,10 +89,11 @@ var serverScopedRoutes = map[string]bool{ } // POST/GET /api/mcp is deliberately absent from this map. main.go's -// serverTouchingRoutes only feeds in routes whose path contains "server" or -// "console" (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: +// 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