fix: scope the assignment count leaked by the key list
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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user