feat(authority): enforce PostgreSQL tenant isolation - #3839
Conversation
huangruiteng
left a comment
There was a problem hiding this comment.
Request changes conclusion (author-owned PR; GitHub blocks formal self-review)
Findings
- [P1] Rebase onto current
mainand reconcile the authority RFC before merge. Exact heade52b3ab59825e6602972f7396ce0be3bc80c62ddis based atbd52b28aa0ce271859c2647ce87db874ee50d1db; currentmainhas since merged #3819 and #3833, which update the same authority provider profile and bilingual RFC. GitHub reportsCONFLICTING, and a merge-tree reproduces a content conflict indocs/architecture/rfcs/shared-goal-authority-state-provider-v0.zh-CN.md. The current exact diff would also omit newer NoKV/profile and Stage 4 RFC facts if resolved by taking this branch wholesale. Please rebase, preserve those newer mainline contracts, resolve both language versions as semantic mirrors, and request re-review on the new exact head. - [P2] Add fault injection for uncertain read cleanup / asynchronous release.
readInTenantTransactionnow promises that a failedROLLBACKcausesrelease(error)and pool eviction, whilePostgreSqlAuthorityConnection.releasenewly permits an async implementation. The real PostgreSQL matrix verifies successful rollback, RLS, and COMMIT response loss, but no test forcesROLLBACKor asyncreleasefailure and asserts the eviction signal. A small fake-connection test should cover this negative path so later pool-wrapper changes cannot silently return a dirty session.
动机
这个 PR 处理的是 PostgreSQL authority-store 从“按 SQL 条件约定 tenant scope”走向“数据库也强制 tenant 隔离”的关键安全阶段。此前 adapter 虽然把 (tenant_id, goal_id) 放入查询和主键,但数据库本身没有 RLS;一旦未来 service 查询遗漏 tenant predicate,或连接池带着错误 session 状态复用,数据库不会提供第二道隔离。当前改动希望让每个 tenant-scoped read/commit 都在 transaction-local loopx.tenant_id 下运行,并由 forced RLS 拒绝缺失 context 和跨 tenant 写入,同时继续保留 LoopX 对语义 transition、CAS、receipt 与 ambiguous COMMIT 的权威。这个目标与 Stage 2B 的 coverage-only 边界一致,也没有冒充尚未交付的 principal authentication 或 production promotion。
改动思路
实现分成三层。数据库层在四张 tenant-scoped 表上同时 ENABLE 和 FORCE ROW LEVEL SECURITY,每张表用 current_setting('loopx.tenant_id', TRUE) 做 USING / WITH CHECK;缺少 context 时表达式不为真,跨 tenant row 也无法通过写检查。adapter 层新增 beginTenantTransaction 和 readInTenantTransaction:先开启 read-only 或 read-write transaction,再用参数化 set_config(..., TRUE) 安装 tenant context,读路径返回前显式 rollback 清理 local GUC,清理不确定时把 error 交给 pool release 以驱逐连接。commit 路径继续只在 COMMIT 尝试前返回 failed,一旦 COMMIT 开始后出错则返回 ambiguous,要求按 operation receipt 回读。文档与 provider profile 则把 database RLS 和 service API authentication/tenant authorization 分开,避免把数据库隔离错误描述成完整身份认证。
具体改动
loopx/control_plane/coordination/postgresql_authority_store.ts扩展 connection release 契约,安装四表 RLS policy,新增 tenant transaction 与 rollback-error 处理,并把loadAuthority、readReceipt、scanCommitted统一收敛到 scoped read helper;commitAuthority同样在写 transaction 内设置 tenant context。loopx/control_plane/coordination/authority_store.ts更新 PostgreSQL provider profile:trust boundary 明确为 transaction-local tenant-scoped service database role,且仍保留 service API auth、role provisioning/audit、restore、failover/capacity、shadow/promotion 等 qualification holds。tests/control_plane_ts/postgresql_authority_store.integration.test.ts创建受限 runtime role,给予最小 schema/table 权限,在真实 PostgreSQL 上验证无 context 看不到行、正确 context 只看到本 tenant、跨 tenant insert 被 RLS 拒绝、runtime role 不能修改 store metadata;同时保留共享 conformance、rollback、CAS、receipt-recovery 测试。tests/control_plane_ts/authority_store.test.ts固化 provider profile 的新 trust/hold 语义。- 英文与中文 Shared Control-Plane Authority RFC 同步记录 RLS 行为、验证证据、coverage-only 边界和仍未交付的 service authentication / production route。
关键代码讲解
beginTenantTransaction是 tenant context 的唯一安装点。它使用 SQL 参数而不是拼接 tenant id,并校验set_config回读值;设置失败时先 rollback,避免带未知 context 继续执行。readInTenantTransaction是读路径生命周期 owner。成功读取后也 rollback,以清除 transaction-local GUC;任何 rollback uncertainty 都转成 release error,要求 pool 淘汰连接,而不是把它借给下一个 tenant。POSTGRESQL_AUTHORITY_STORE_SCHEMA_SQL用 forced RLS 约束表 owner,并在每张表同时设置 read filter 和 write check;查询仍保留显式 tenant predicate,因此 RLS 是 defense in depth,不替代正常索引范围与 service 授权。commitAuthority保持 provider-neutral failure taxonomy:transaction 内的协议/写入错误返回 typed failure;COMMIT 已开始后的异常保持 ambiguous,调用者只能通过相同operation_id的 receipt 查证,不能盲目重放。- restricted-role integration test 证明数据库边界真实生效,而不是只对 SQL 字符串做静态断言;这对 RLS、role privilege 和 transaction-local context 都是必要的 durable validation。
对主干的风险
安全实现本身的主要正向、负向路径都已通过真实 PostgreSQL 16 验证:同 tenant 的 CAS、不同 tenant 复用相同 goal/operation、transaction rollback、COMMIT response loss、无 context 读取、跨 context 写入与 metadata 权限均符合预期。typed state、domain neutrality、authority semantics 也清楚:tenant id 是显式 store scope,不靠文本分类;RLS 只是 service 内 defense in depth;production caller 和 authority-source promotion 仍然 default-off。当前不可合并风险首先是 stale base:authority RFC/profile 已被主干继续推进,中文版存在真实冲突,strict change-quality receipt 因此无效。其次是 cleanup uncertainty 尚缺故障注入;实现看起来保守,但这是连接池跨 tenant 隔离的负向关键路径,应在 rebase 时用 fake connection 固化 ROLLBACK failure -> release(error) 的行为。
我的整体评价
这是一个边界清晰、范围与风险相称的 Stage 2B 增量:它没有把 PostgreSQL 变成第二个语义 authority,也没有把 RLS 冒充 authentication;真实数据库验证显著强于只检查 schema 字符串。代码层面我没有发现 tenant 数据可跨 scope 暴露的 blocker。当前结论仍是 request changes,原因是 exact head 已与当前 authority RFC/profile 冲突,不能在旧 head 上给出有效 merge-ready 结论。请 rebase、合并主干最新双语契约,并补上连接清理故障注入;新 head 到达后可快速 re-review。
Validation
- Exact head reviewed:
e52b3ab59825e6602972f7396ce0be3bc80c62dd - PostgreSQL 16 integration matrix: 11/11 passed, including restricted-role two-tenant RLS and ambiguous-COMMIT receipt recovery
- TypeScript control-plane suite: 387 passed, 1 environment-gated PostgreSQL test skipped in the unconfigured aggregate run
- TypeScript typecheck: passed
- Exact PR diff hygiene and public/private boundary scan across all six changed files: passed
- Risk-based premerge: 13/13 automated checks passed, no manual holds; final gate blocked by exact-scope receipt
cqr_4d1e1be2ad49a6763564because current-main merge-tree is conflicting - Remote head and GitHub merge state re-read before publication
English verdict: REQUEST CHANGES for exact head e52b3ab59825e6602972f7396ce0be3bc80c62dd. The tenant-context/RLS design is sound, remains coverage-only, and passed an independently rerun PostgreSQL 16 matrix (11/11), but the branch now conflicts with merged authority RFC/profile work on main. Rebase, preserve the newer bilingual authority contracts, and add a fault-injection regression for rollback/release eviction before re-review.
Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
e52b3ab to
d48e17c
Compare
Final maintainer reviewFindingsNo remaining code or documentation blocker on exact head The branch is rebased directly on current Product and architecture judgmentThis remains a coverage-only Stage 2B provider slice. It strengthens tenant isolation inside the service database boundary without moving semantic authority into PostgreSQL or presenting RLS as principal authentication. Transaction ownership, ambiguous-COMMIT reconciliation, and provider-neutral receipts remain in the shared TypeScript AuthorityStore contract. Production service auth, tenant authorization, runtime promotion, restore rotation, capacity/failover, and shadow migration remain explicit holds. The related future-facing refactor is appropriately bounded: tenant read lifecycle and uncertain cleanup are centralized in one typed helper. Further extraction would add indirection without removing duplicated authority, so no additional safe-fix refactor was applied. Validation
Merge decisionThe cancelled pytest status is a workflow-timeout false negative with a complete passing test result. Per the owner's explicit authorization, admin merge is justified for this exact head; the bypass applies only to the stale review requirement and cancelled timeout status, not to any failed product validation. |
Summary
Trust boundary
The LoopX authority remains the semantic transaction owner. The PostgreSQL adapter receives an already authorized tenant/Goal scope and persists that scope; it does not authenticate principals or decide whether a principal may act for a tenant.
Every store read or commit starts a transaction, installs
loopx.tenant_idwith transaction-localset_config(..., TRUE), and relies on forced RLS for defense in depth. A role with direct table privileges sees no tenant rows without that context and cannot write a different tenant. Read-only paths explicitly roll back before returning; rollback/cleanup uncertainty evicts the connection instead of returning a dirty session to the pool.Once
COMMITstarts, an exception remainsambiguous, notfailed. Recovery uses the sameoperation_idthroughreadReceipt; the adapter does not blindly repeat a possibly committed effect.Rebase and review refinements
mainate8768343d749d4575a300af2d2101888b2072224ROLLBACKfailure, proves the same error is passed torelease(error), waits for asynchronous release before settling the read result, and attempts rollback only onceDeliberately not included
Those remain later reviewed stages of the RFC.
Validation
npm run typecheck:control-plane— passednpm run test:postgresql-authority-store— 12/12 passed against PostgreSQL 16.15npm run test:control-plane— 465/465 passed with PostgreSQL enabledgit diff --check origin/main...HEAD— passedloopx change-quality verify --goal-id loopx-meta --repo-path . --base-ref origin/main— exact-scope receiptcqr_41955688a67174026cecvalidloopx canary premerge --from-git-diff --goal-id loopx-meta— 13 selected checks passed; zero failures and zero manual holdsOne discarded local invocation selected the wrong Docker socket and therefore did not provide PostgreSQL credentials. The exact-head suites were rerun in the corrected environment with the passing results above.
Merge policy
This PR changes a database permission boundary. The owner explicitly authorized an admin merge after the rebase, bilingual RFC reconciliation, negative cleanup test, exact-head qualification, and remote CI verification.