diff --git a/server/internal/api/handlers.go b/server/internal/api/handlers.go index 18d5ac2..3d67a2d 100644 --- a/server/internal/api/handlers.go +++ b/server/internal/api/handlers.go @@ -637,8 +637,14 @@ func assignKey(c *gin.Context) { func revokeAssignment(c *gin.Context) { keyID := c.Param("id") serverID := c.Param("serverId") + instanceID := auth.InstanceID(c) - if err := services.RevokeAssignment(auth.InstanceID(c), keyID, serverID); err != nil { + if _, err := services.GetServerScoped(instanceID, serverID, auth.ServerScope(c)); err != nil { + c.JSON(http.StatusNotFound, gin.H{"error": "server not found"}) + return + } + + if err := services.RevokeAssignment(instanceID, keyID, serverID); err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } diff --git a/server/internal/api/serverscope.go b/server/internal/api/serverscope.go index e67a582..96ab319 100644 --- a/server/internal/api/serverscope.go +++ b/server/internal/api/serverscope.go @@ -9,7 +9,10 @@ import "fmt" // 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. // -// false means "deliberately fleet-wide" and requires a comment saying why. +// 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, @@ -29,6 +32,24 @@ var serverScopedRoutes = map[string]bool{ "POST /api/servers/:id/workloads/:wid/action": true, "GET /api/servers/:id/workloads/:wid/logs": true, + // 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, + // 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 @@ -36,42 +57,13 @@ var serverScopedRoutes = map[string]bool{ "GET /api/servers/new": false, "POST /api/servers/new": false, - // KnownTags aggregates the tag *vocabulary* in use across the fleet — - // keys and the values seen for them — never a server identifier or any - // other server attribute, so it does not let a restricted token enumerate - // which hosts exist. Scoping it would need ListServersFiltered-style - // plumbing through KnownTags for a leak this narrow; left fleet-wide for - // now. + // KnownTags aggregates the tag *vocabulary* in use across the fleet — keys + // and the values seen for them — never a server identifier or any other + // server attribute, so it does not let a restricted token enumerate which + // 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, - - // listServerVulnerabilities and getServerPackages read findings/package - // data keyed by server ID without checking the token's tag restriction — - // this task's interfaces (GetServerScoped, ResolveTargetsScoped) only - // wrap services.GetServer/ListServers/ResolveTargets, and these two - // handlers call services.ListFindings/ListPackages directly, so they are - // out of this task's chokepoints. This is a known, unresolved gap: a - // restricted token can currently read vulnerability/package data for a - // server outside its scope by ID. Flagged for a follow-up task rather - // than silently left off this map. - "GET /api/servers/:id/vulnerabilities": false, - "GET /api/servers/:id/packages": false, - - // getServerRunLog/streamServerRunLog read a run's per-server log by - // (runId, serverId) via services.HasServerRunLog/ReadServerRunLog, not - // through GetServer/ListServers/ResolveTargets, so — same as the - // vulnerabilities/packages routes above — they are outside this task's - // chokepoints. Known gap: a restricted token that already knows a runId - // can currently read log output for a serverId outside its scope. - // Flagged for a follow-up task. - "GET /api/runs/:runId/servers/:serverId/logs": false, - "GET /api/runs/:runId/servers/:serverId/logs/stream": false, - - // revokeAssignment calls services.RevokeAssignment(instanceID, keyID, - // serverID) directly and never resolves the server through GetServer, so - // it is also outside this task's chokepoints. Known gap: a restricted - // token could revoke a key assignment naming a serverId outside its - // scope. Flagged for a follow-up task. - "DELETE /api/keys/:id/assign/:serverId": false, } // AssertServerScopeMapComplete refuses to boot when a route touching server diff --git a/server/internal/api/vulnerabilities.go b/server/internal/api/vulnerabilities.go index 5937c2a..ffa5e15 100644 --- a/server/internal/api/vulnerabilities.go +++ b/server/internal/api/vulnerabilities.go @@ -290,7 +290,14 @@ func writeFindingError(c *gin.Context, err error) { // @Security bearerAuth // @Router /servers/{id}/vulnerabilities [get] func listServerVulnerabilities(c *gin.Context) { - findings, err := services.ListFindings(c.Request.Context(), auth.InstanceID(c), c.Param("id")) + instanceID := auth.InstanceID(c) + id := c.Param("id") + if _, err := services.GetServerScoped(instanceID, id, auth.ServerScope(c)); err != nil { + c.JSON(http.StatusNotFound, gin.H{"error": "server not found"}) + return + } + + findings, err := services.ListFindings(c.Request.Context(), instanceID, id) if err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return @@ -314,7 +321,14 @@ func listServerVulnerabilities(c *gin.Context) { // @Security bearerAuth // @Router /servers/{id}/packages [get] func getServerPackages(c *gin.Context) { - sp, err := services.ListPackages(auth.InstanceID(c), c.Param("id")) + instanceID := auth.InstanceID(c) + id := c.Param("id") + if _, err := services.GetServerScoped(instanceID, id, auth.ServerScope(c)); err != nil { + c.JSON(http.StatusNotFound, gin.H{"error": "server not found"}) + return + } + + sp, err := services.ListPackages(instanceID, id) if err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return diff --git a/server/internal/api/workflows.go b/server/internal/api/workflows.go index 4ea8edb..e90e42e 100644 --- a/server/internal/api/workflows.go +++ b/server/internal/api/workflows.go @@ -67,6 +67,10 @@ func getServerRunLog(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": "invalid id"}) return } + if _, err := services.GetServerScoped(auth.InstanceID(c), serverID, auth.ServerScope(c)); err != nil { + c.JSON(http.StatusNotFound, gin.H{"error": "server not found"}) + return + } if !services.HasServerRunLog(runID, serverID) { c.JSON(http.StatusNotFound, gin.H{"error": "no logs"}) return @@ -117,6 +121,10 @@ func streamServerRunLog(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": "invalid id"}) return } + if _, err := services.GetServerScoped(auth.InstanceID(c), serverID, auth.ServerScope(c)); err != nil { + c.JSON(http.StatusNotFound, gin.H{"error": "server not found"}) + return + } c.Writer.Header().Set("Content-Type", "text/event-stream") c.Writer.Header().Set("Cache-Control", "no-cache") c.Writer.Header().Set("Connection", "keep-alive")