From 8d065ba68267c1af12e53bb64c726c39713ffa0a Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 15:22:21 -0500 Subject: [PATCH 1/3] fix: redact bnk_config from the instance-wide global cluster list (#116) GET /api/k8s/clusters is instance-wide (require_viewer, no project scope) and returns every cluster to any authenticated viewer. That was already true for cluster metadata, but ADR-424 added bnk_config to the serialized shape, so the global list now also exposed each cluster's host_ids, dpu_ids, control_plane_host_id, and tmfifo_pool_cidr cross-project -- infrastructure membership one project should not see for another. No consumer needs it there: all nine frontend callers of the global list (useAllClusters) ignore bnk_config; the only surface that renders it, K8sClusterList, uses the project-scoped useProjectClusters. So redacting it on the global path removes the disclosure without losing a feature. serialize_cluster gains include_bnk_config (default True); list_all_clusters passes False. The project-scoped list and the per-cluster detail -- whose callers actually consume it, and which can be authorized per project -- keep it. Redacting also lets the global path drop the membership bulk-fetch those fields required. Test: a BNK cluster with a BnkClusterConfig is absent (None) from the global list but present on the project-scoped list. Verified non-vacuous -- against the unpatched service the global row leaks the full config. Note: the broader question of whether an instance-wide list of cluster METADATA should be visible to any viewer at all is a pre-existing tenancy-model design choice (predates ADR-424) and is out of scope here; this fixes the specific ADR-424 widening the issue flags. Fixes #116 Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- backend/routes/k8s/_shared.py | 14 +++++++++- .../services/cluster_management_service.py | 23 ++++++++-------- .../test_cluster_management_service.py | 27 +++++++++++++++++++ 3 files changed, 52 insertions(+), 12 deletions(-) diff --git a/backend/routes/k8s/_shared.py b/backend/routes/k8s/_shared.py index 0975d2c..6d81b93 100644 --- a/backend/routes/k8s/_shared.py +++ b/backend/routes/k8s/_shared.py @@ -26,6 +26,7 @@ def serialize_cluster( cluster: KubernetesCluster, include_project_id: bool = True, membership: "tuple[list[int], list[int]] | None" = None, + include_bnk_config: bool = True, ) -> dict: """ Serialize a KubernetesCluster to dict. @@ -34,6 +35,13 @@ def serialize_cluster( membership: pre-fetched (host_ids, dpu_ids) for BNK clusters in list contexts. When provided, _serialize_bnk_config uses it directly instead of issuing per-cluster queries (eliminates the 2N pattern; ADR-424 finding C). + + include_bnk_config: when False, bnk_config is redacted (None). The + ADR-424 bnk_config carries cross-project infrastructure membership + (host_ids, dpu_ids, control_plane_host_id, tmfifo_pool_cidr); the global + instance-wide list must not leak it to any viewer (#116). The + project-scoped list and the per-cluster detail keep it (their callers + are the surfaces that actually consume it). """ platform_context = PlatformContextService.serialize_cluster_context(cluster) @@ -68,7 +76,11 @@ def serialize_cluster( "deployable_release_id": cluster.deployable_release_id, "running_release_id": cluster.running_release_id, # BNK multi-host cluster configuration side-table (ADR-424) - "bnk_config": _serialize_bnk_config(cluster, membership=membership) if getattr(cluster, "bnk_config", None) else None, + "bnk_config": ( + _serialize_bnk_config(cluster, membership=membership) + if include_bnk_config and getattr(cluster, "bnk_config", None) + else None + ), } if include_project_id: result["project_id"] = cluster.project_id diff --git a/backend/services/cluster_management_service.py b/backend/services/cluster_management_service.py index 703e92d..573eef4 100644 --- a/backend/services/cluster_management_service.py +++ b/backend/services/cluster_management_service.py @@ -347,26 +347,27 @@ def create_cluster(self, project_id: int, cluster_data) -> dict[str, Any]: } def list_all_clusters(self) -> dict[str, Any]: - """List all Kubernetes clusters (global).""" + """List all Kubernetes clusters (global). + + This endpoint is instance-wide (require_viewer, not project-scoped), so + it must not expose the ADR-424 bnk_config -- host/DPU membership, + control-plane host, tmfifo pool CIDR -- cross-project to any viewer + (#116). bnk_config is redacted here; the project-scoped list and the + per-cluster detail keep it. No frontend consumer of this global list + reads bnk_config (only the project-scoped K8sClusterList does), so this + removes the disclosure without losing a feature -- and it also drops the + now-unnecessary membership bulk-fetch those fields required. + """ from sqlalchemy.orm import selectinload from routes.k8s._shared import serialize_cluster - from services.bnk_cluster_service import BnkClusterService clusters = ( self.db.query(KubernetesCluster) .options(selectinload(KubernetesCluster.bnk_config)) .all() ) - # Bulk-fetch membership for all BNK clusters in 2 queries (not 2N). - # selectinload(bnk_config) already avoids the config N+1; this bulk - # call eliminates the host+DPU membership N+1 in _serialize_bnk_config. - bnk_ids = [c.id for c in clusters if getattr(c, "bnk_config", None)] - membership_map = BnkClusterService(self.db).bulk_cluster_membership(bnk_ids) - result = [ - serialize_cluster(c, membership=membership_map.get(c.id)) - for c in clusters - ] + result = [serialize_cluster(c, include_bnk_config=False) for c in clusters] return {"clusters": result, "count": len(result)} def list_project_clusters(self, project_id: int) -> dict[str, Any]: diff --git a/backend/tests/component/test_cluster_management_service.py b/backend/tests/component/test_cluster_management_service.py index d451a3a..ba29fd0 100644 --- a/backend/tests/component/test_cluster_management_service.py +++ b/backend/tests/component/test_cluster_management_service.py @@ -235,6 +235,33 @@ def test_list_project_clusters_filters(self, db, make_project, make_k8s_cluster) assert result["count"] == 1 assert result["clusters"][0]["name"] == "c1" + def test_global_list_redacts_bnk_config_but_project_list_keeps_it( + self, db, make_project, make_k8s_cluster + ): + """#116: the instance-wide global list must not leak ADR-424 bnk_config + (host/DPU membership, control-plane host, tmfifo pool CIDR) cross-project + to any viewer. The project-scoped list -- whose caller actually renders + it -- must still include it.""" + from models.kubernetes import BnkClusterConfig + + p = make_project() + cluster = make_k8s_cluster(project=p, name="bnk-cluster") + db.add(BnkClusterConfig(cluster_id=cluster.id, tmfifo_pool_cidr="192.168.100.0/22")) + db.commit() + + svc = ClusterManagementService(db) + + global_row = next( + c for c in svc.list_all_clusters()["clusters"] if c["name"] == "bnk-cluster" + ) + assert global_row["bnk_config"] is None, "global list leaked bnk_config (#116)" + + project_row = next( + c for c in svc.list_project_clusters(p.id)["clusters"] if c["name"] == "bnk-cluster" + ) + assert project_row["bnk_config"] is not None, "project-scoped list must keep bnk_config" + assert project_row["bnk_config"]["tmfifo_pool_cidr"] == "192.168.100.0/22" + def test_list_project_clusters_nonexistent_project(self, db): svc = ClusterManagementService(db) with pytest.raises(NotFoundError): From 0f8d98f40b8c948733dcb8aa2418a6a64fcd09bf Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 15:41:24 -0500 Subject: [PATCH 2/3] review fix: retarget the global-list bnk_config test; fix stale docstring + guard mwiget is right: CI was red and my "no backend test asserts bnk_config on the global list" claim was wrong. test_serialize_cluster_with_bnk_config_and_members did exactly that via client.get("/api/k8s/clusters"), and I missed it because it goes through the HTTP client, not a serialize_cluster reference. Retargeted rather than deleted, as suggested: its real subject is _serialize_bnk_config's B6 cross-bucket guard (host_ids/dpu_ids don't contaminate, with two hosts + two DPUs chosen so SQLite's per-table autoincrement can't mask a leak) -- coverage worth keeping. It now fetches the PROJECT-scoped list (/api/projects/{id}/k8s/clusters), where bnk_config is still rendered, so the B6 guard survives, and adds a second assertion on the global list that bnk_config is None -- locking the redaction, which nothing did before. Also from the review: - Guard comment on get_cluster_details: GET /k8s/clusters/{id} is require_viewer with NO project scope (unlike the PUT/DELETE, which use require_cluster_owner), and it hand-builds its dict. A future change reusing serialize_cluster there would silently reintroduce #116 one request further along, with the id coming from the very global list this PR hardens. Noted in code as cheap insurance. - Dropped list_all_clusters from bulk_cluster_membership's 2N docstring -- it no longer serializes bnk_config, so it no longer calls this. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- backend/services/bnk_cluster_service.py | 3 +- .../services/cluster_management_service.py | 12 ++++++- .../test_bnk_cluster_member_assignment.py | 32 +++++++++++++++---- 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/backend/services/bnk_cluster_service.py b/backend/services/bnk_cluster_service.py index 2e23fcd..dd4dc4c 100644 --- a/backend/services/bnk_cluster_service.py +++ b/backend/services/bnk_cluster_service.py @@ -81,7 +81,8 @@ def bulk_cluster_membership( ) -> dict[int, tuple[list[int], list[int]]]: """Return {cluster_id: (host_ids, dpu_ids)} for multiple clusters in 2 queries. - Eliminates the 2N query pattern in list_all_clusters / list_project_clusters: + Eliminates the 2N query pattern in list_project_clusters (list_all_clusters + no longer serializes bnk_config, so it does not call this -- #116): instead of 2 queries per BNK cluster, run one grouped host query and one grouped DPU query, then bucket by cluster_id. Single-sources the query logic used by cluster_membership (ADR-424 finding C). diff --git a/backend/services/cluster_management_service.py b/backend/services/cluster_management_service.py index 573eef4..85ed52c 100644 --- a/backend/services/cluster_management_service.py +++ b/backend/services/cluster_management_service.py @@ -393,7 +393,17 @@ def list_project_clusters(self, project_id: int) -> dict[str, Any]: return {"clusters": result, "count": len(result)} def get_cluster_details(self, cluster_id: int) -> dict[str, Any]: - """Get cluster details.""" + """Get cluster details. + + NOTE (#116): GET /k8s/clusters/{cluster_id} is require_viewer with NO + project scope (unlike the PUT/DELETE on the same path, which use + require_cluster_owner). This handler hand-builds its dict and must NOT + gain a bnk_config key -- reusing serialize_cluster here (or adding + bnk_config by hand) would reintroduce the cross-project disclosure #116 + closes, one request further along, and the id needed comes straight from + the global list. If bnk_config is ever needed on detail, scope this route + to the project first. + """ cluster = self._get_cluster(cluster_id) context = PlatformContextService.serialize_cluster_context(cluster) return { diff --git a/backend/tests/component/test_bnk_cluster_member_assignment.py b/backend/tests/component/test_bnk_cluster_member_assignment.py index 9369b98..34573e8 100644 --- a/backend/tests/component/test_bnk_cluster_member_assignment.py +++ b/backend/tests/component/test_bnk_cluster_member_assignment.py @@ -628,11 +628,14 @@ def test_hosts_and_dpus_bucketed_by_cluster(self, db): def test_serialize_cluster_with_bnk_config_and_members( self, client, db, admin_headers, sample_user, make_project, make_k8s_cluster ): - """GET /api/k8s/clusters renders bnk_config.host_ids / dpu_ids correctly - for a cluster that has a BnkClusterConfig with members assigned. + """The project-scoped cluster list renders bnk_config.host_ids / dpu_ids + correctly; the instance-wide global list redacts bnk_config (#116). - Verifies _serialize_bnk_config branch (bnk_config present) and that - host IDs do not leak into the dpu_ids bucket (B6). + Verifies _serialize_bnk_config branch (bnk_config present on the + project-scoped list) and that host IDs do not leak into the dpu_ids + bucket (B6). The subject moved from the global list to the project list + because #116 redacts bnk_config on the global path -- so this also pins + the redaction: present when project-scoped, absent when global. Two hosts and two DPUs ensure that IDs are distinct across the tables (SQLite auto-increments per-table from 1, so IDs can coincide with a @@ -660,8 +663,11 @@ def test_serialize_cluster_with_bnk_config_and_members( ) assert r.status_code == 200, r.text - # Fetch the cluster list — exercises bulk_cluster_membership + serialize_cluster. - resp = client.get("/api/k8s/clusters", headers=admin_headers) + # Project-scoped list — exercises bulk_cluster_membership + serialize_cluster, + # and is the surface that still renders bnk_config after #116. + resp = client.get( + f"/api/projects/{project.id}/k8s/clusters", headers=admin_headers + ) assert resp.status_code == 200, resp.text clusters = resp.json()["clusters"] @@ -669,7 +675,7 @@ def test_serialize_cluster_with_bnk_config_and_members( assert target is not None, f"cluster {cluster.id} not in response" bnk = target.get("bnk_config") - assert bnk is not None, "bnk_config must be present for a BNK cluster" + assert bnk is not None, "bnk_config must be present on the project-scoped list" assert sorted(bnk["host_ids"]) == sorted([host1.id, host2.id]), ( f"host_ids must contain the two assigned hosts, got {bnk['host_ids']}" @@ -685,6 +691,18 @@ def test_serialize_cluster_with_bnk_config_and_members( f"host_ids must have exactly 2 entries (no DPU leakage), got {bnk['host_ids']}" ) + # #116: the instance-wide global list must NOT leak bnk_config to any + # viewer, even though the same cluster carries it on the project list. + global_resp = client.get("/api/k8s/clusters", headers=admin_headers) + assert global_resp.status_code == 200, global_resp.text + global_target = next( + (c for c in global_resp.json()["clusters"] if c["id"] == cluster.id), None + ) + assert global_target is not None + assert global_target.get("bnk_config") is None, ( + "global list leaked bnk_config cross-project (#116)" + ) + # --------------------------------------------------------------------------- # B6 — _require_cp_member=True BadRequestError path From 74a70c89b61e16da0c972aa546d8facf34b9e939 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 16:04:51 -0500 Subject: [PATCH 3/3] review: document that the project-scoped list is not membership-scoped (#116) mwiget traced the read path further: the disclosure is still reachable in two requests. The global list returns each cluster's project_id (include_project_id defaults True), and list_project_clusters serves full bnk_config for any project_id supplied -- its route is require_viewer (any authenticated user) with no membership/ownership check. So redacting bnk_config on the global list is a strict improvement but does NOT close #116. The real fix -- enforcing per-project membership on that route -- is the pre-existing tenancy-model decision this PR deliberately scoped out (require_viewer is role-based across the app; changing it is a real behaviour change for any caller relying on cross-project reads). Documented the residual at list_project_clusters, mirroring the get_cluster_details guard, so the open door is discoverable in code. The PR is reframed Relates to #116 (not Fixes) so merging does not auto-close an issue whose disclosure is still open. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- backend/services/cluster_management_service.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/backend/services/cluster_management_service.py b/backend/services/cluster_management_service.py index 85ed52c..20f94e5 100644 --- a/backend/services/cluster_management_service.py +++ b/backend/services/cluster_management_service.py @@ -371,7 +371,19 @@ def list_all_clusters(self) -> dict[str, Any]: return {"clusters": result, "count": len(result)} def list_project_clusters(self, project_id: int) -> dict[str, Any]: - """List all Kubernetes clusters for a project.""" + """List all Kubernetes clusters for a project. + + NOTE (#116): this list still renders bnk_config, and its route is + require_viewer (any authenticated user) with NO membership/ownership + check -- project_id is a path param anyone may supply. Combined with the + global list (which still returns each cluster's project_id), any viewer + can read any project's bnk_config in two requests. Redacting bnk_config + on the global list (this change) is a strict improvement but does NOT + fully close #116: the project list must enforce per-project membership + first, and that is a pre-existing tenancy-model decision (require_viewer + is role-based across the app) larger than this change. Until that lands, + bnk_config here is readable by any authenticated user. + """ from sqlalchemy.orm import selectinload from routes.k8s._shared import serialize_cluster