From e06f9d5670e091cf875803a793695d2af0973fb0 Mon Sep 17 00:00:00 2001 From: mrhid6 Date: Wed, 9 Sep 2026 07:54:42 +0000 Subject: [PATCH] fix: scope the assignment count leaked by the key list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /api/keys returned each key's AssignedCount as a raw CountDocuments over every non-revoked assignment, with no scope filter — a tag-restricted token reading the list saw a nonzero count for a key it can see nothing assigned to in its own scope, which is enough to tell it an assignment exists on a host it must not know about. Same class of leak getKey's assignment-list filter closed on the detail route, surviving on the list route through a count instead of a server object. services.ListKeys now takes the caller's tokenScope. An unrestricted caller (empty scope) takes the original unfiltered per-key CountDocuments with no extra work, so the common case is not slower. A restricted caller resolves the visible fleet once via ListServers before the per-key loop, then counts each key's assignments with an added server_id $in filter — one extra query total, not one per key. ListKeys had exactly one caller (listKeys), so the parameter went there rather than adding a second entry point. Recorded GET /api/keys in serverScopedRoutes as true; its path, like GET /api/keys/:id, matches none of serverTouchingRoutes' substrings, so the entry is not boot-enforced. Deliberately did not widen the filter to catch "keys" — that would sweep in create/delete/private-key routes with no server data at all. The real fix for this shape of gap is the declare-by-default inversion already recorded as a follow-up. --- server/internal/api/handlers.go | 2 +- server/internal/api/serverscope.go | 15 +++++++++++ server/internal/services/keys.go | 42 +++++++++++++++++++++++++++--- 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/server/internal/api/handlers.go b/server/internal/api/handlers.go index aebab55..142c131 100644 --- a/server/internal/api/handlers.go +++ b/server/internal/api/handlers.go @@ -489,7 +489,7 @@ func generateKey(c *gin.Context) { // @Security bearerAuth // @Router /keys [get] func listKeys(c *gin.Context) { - keys, err := services.ListKeys(auth.InstanceID(c)) + keys, err := services.ListKeys(auth.InstanceID(c), auth.ServerScope(c)) if 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 9daf29c..11ea229 100644 --- a/server/internal/api/serverscope.go +++ b/server/internal/api/serverscope.go @@ -72,6 +72,21 @@ var serverScopedRoutes = map[string]bool{ // 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, + // 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 diff --git a/server/internal/services/keys.go b/server/internal/services/keys.go index 1c7b36b..0ffda76 100644 --- a/server/internal/services/keys.go +++ b/server/internal/services/keys.go @@ -118,7 +118,20 @@ type KeyWithCount struct { AssignedCount int `bson:"-" json:"assigned_count"` } -func ListKeys(instanceID string) ([]KeyWithCount, error) { +// ListKeys returns every key for instanceID together with its assignment +// count, narrowed by tokenScope: AssignedCount only counts assignments on +// servers ServerInTokenScope admits. Without this, a restricted token reading +// the key list would see a nonzero count for a key it cannot see a single +// assignment of in its own scope — the same hostname-existence leak +// getKey's scope filter closes on the detail route, reachable here through a +// count instead of a server object. +// +// An empty tokenScope (an unrestricted token, or a cookie session) means "see +// everything", matching ServerInTokenScope elsewhere, and takes the original, +// unfiltered per-key CountDocuments query with no extra work: the visible- +// fleet resolution below is skipped entirely, so the common unrestricted case +// costs exactly what it cost before this scope filter existed. +func ListKeys(instanceID string, tokenScope map[string]string) ([]KeyWithCount, error) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() @@ -133,14 +146,37 @@ func ListKeys(instanceID string) ([]KeyWithCount, error) { return nil, err } + // Resolve the visible fleet once, outside the per-key loop, so a + // restricted token's count costs one extra query total rather than one + // per key — the same reasoning ResolveTargetsScoped already applies to + // target resolution. + scoped := len(tokenScope) > 0 + var visibleIDs []string + if scoped { + servers, err := ListServers(instanceID) + if err != nil { + return nil, err + } + visibleIDs = make([]string, 0, len(servers)) + for _, s := range servers { + if ServerInTokenScope(s, tokenScope) { + visibleIDs = append(visibleIDs, s.ServerID) + } + } + } + result := make([]KeyWithCount, 0, len(keys)) for _, k := range keys { setKeyMeta(&k) - count, _ := db.Col("assignments").CountDocuments(ctx, bson.M{ + filter := bson.M{ "instance_id": instanceID, "key_id": k.KeyID, "revoked_at": nil, - }) + } + if scoped { + filter["server_id"] = bson.M{"$in": visibleIDs} + } + count, _ := db.Col("assignments").CountDocuments(ctx, filter) result = append(result, KeyWithCount{Key: k, AssignedCount: int(count)}) } return result, nil