Skip to content

feat(ai): enforce session ownership isolation - #1592

Merged
robocanic merged 3 commits into
apache:aifrom
Qiao-yq:feat/ai-1539-session-ownership
Nov 2, 2026
Merged

robocanic merged 3 commits into
apache:aifrom
Qiao-yq:feat/ai-1539-session-ownership

Conversation

@Qiao-yq

@Qiao-yq Qiao-yq commented Oct 10, 2026 •

Copy link
Copy Markdown

Summary

Implements the Session ownership and tenant/user isolation foundation for Issue #1539.

  • add server-owned Subject and explicit AccessContext
  • persist tenant, owner, version, and expiration on Sessions
  • enforce tenant/owner isolation in Memory and GORM stores
  • apply authorization across Session APIs, chat, history, and deletion
  • add explicit maintenance interfaces for expiration cleanup and legacy adoption
  • harden static authentication defaults and require an explicit Router authenticator
  • add shared Memory/GORM ownership and lifecycle contract tests

Scope

This PR implements the Session ownership portion of requirement 4 and establishes the authentication and policy interfaces used by later work.

Static authentication is intentionally limited to local development:

  • defaults to 127.0.0.1
  • CORS is disabled by default
  • non-loopback binding requires allow_insecure_static_auth: true
  • system maintenance access is rejected by the HTTP middleware

Legacy Session adoption is disabled by default. When explicitly enabled, it adopts only rows where both tenant and owner are absent. Partially owned rows remain isolated for manual repair.

Out of scope

This PR does not claim to fully complete Issue requirement 4.

  • production JWT or signed-gateway authentication remains a later Phase 1 G1 requirement
  • resource-level ACL resolution and AuthorizedResourceScope are not implemented; ResourceReference remains untrusted input
  • idempotency, quotas, retention configuration, deletion propagation, Sanitizer, Usage/Cost, and Context Compiler work remain in later PRs

Until production authentication and resource-level authorization are delivered, this change provides Session ownership isolation but must not be treated as complete production identity isolation.

Compatibility and migration

  • Store APIs now require AccessContext, so callers inside the AI module were updated together.
  • SessionStore and MessageStore expose business operations only; privileged cleanup and adoption use MaintenanceStore.
  • GORM ownership columns remain migration-compatible with existing rows.
  • Memory and GORM use the same Session validation, active-state, expiration, and ownership contract.

Validation

  • go test ./...
  • go vet ./...
  • go test -race ./security ./store/test ./store/memory ./store/gorm ./component/server/...
  • git diff --check

Related to #1539.

@Qiao-yq
Qiao-yq force-pushed the feat/ai-1539-session-ownership branch 2 times, most recently from a96b5e5 to 9ccbd17 Compare October 10, 2026 03:43
@Qiao-yq
Qiao-yq force-pushed the feat/ai-1539-session-ownership branch from 9ccbd17 to 532bdc5 Compare October 17, 2026 04:02
@robocanic

Copy link
Copy Markdown
Contributor

@larry-zy help review this PR.

@larry-zy larry-zy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reproduced two ownership-isolation issues against commit 532bdc5; the affected code and reproduction details are annotated inline.

Comment thread ai/store/gorm/store.go Outdated
Comment on lines +703 to +705
query = query.Where("tenant_id = ?", access.Subject.TenantID)
if !access.Subject.HasScope(security.ScopeSessionAdmin) {
query = query.Where("owner_id = ?", access.Subject.UserID)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Use exact identity comparison for Store ownership checks

These predicates inherit the column collation, while AccessContext.CanAccessOwner uses exact Go string equality. With case-insensitive identity columns, the GORM-backed memory tool can therefore read another tenant's or user's history when the identity differs only in case: memory_all_by_session_id -> AllMemory -> findActiveSession -> scopeSessions has no policy recheck. I reproduced both tenant-only and owner-only variants through the real memory tool using SQLite NOCASE; the case-sensitive controls were denied. Please align Store ownership comparisons with CanAccessOwner. Direct HTTP reads and deletes remain blocked by Manager's secondary authorization; no MySQL instance was tested.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. Fixed in 4c6bbad: GORM now rechecks tenant and owner identities using exact Go string comparisons after querying, including history access. List results are filtered using the same policy.

Comment thread ai/security/security.go
if a.Subject.TenantID != tenantID {
return false
}
return a.Subject.UserID == ownerID || a.Subject.HasScope(ScopeSessionAdmin)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Keep partially owned sessions isolated from business access

This admin branch grants access even when ownerID is empty, and GORM's admin query also omits the owner predicate. Without running legacy adoption, I reproduced HTTP GET, LIST, and DELETE returning 200 for same-tenant legacy sessions with owner_id = '' or NULL, while an ordinary user's GET returned 404. This contradicts the ADR's stated policy that partially owned rows remain isolated for explicit repair. Please reject incomplete ownership in both the policy and GORM queries before applying the same-tenant admin override.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4c6bbad. Business access now rejects sessions with an empty tenant or owner, including access by same-tenant admins. Legacy adoption still only claims sessions where both fields are absent.

@robocanic robocanic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

审查意见

Session 所有权隔离的整体设计清晰:身份只来自服务端 Authenticator,Get/List/Touch/Delete/BeginTurn/... 都通过 AccessContext 做租户 / 属主过滤,越权与不存在统一返回同一类错误,Memory 与 GORM 共用契约测试。我在 PR head 上跑了 go test ./security/... ./store/... ./component/...(在 ai/ 下),均通过。以下是几点需要关注的问题:

1. “system” 能力没有被限制在 MaintenanceStore 内(安全加固,中等)

  • ai/security/security.go:98、:115:SystemAccessContext() 是导出函数,CanAccessOwner 对 system 直接返回 true;ai/store/gorm/store.go:699-701 的 scopeSessions 在 access.IsSystem() 时不加任何租户 / 属主过滤,Memory 的 validateSessionLocked(ai/store/memory/store.go:532)也走同一个 CanAccessOwner。
  • 因此业务接口 SessionStore / MessageStore(Get/List/Delete/BeginTurn/AllMemory/...)在传入 system context 时可以跨租户读写任意 Session 和历史;PR 描述里“业务接口只暴露业务操作,特权清理 / 迁移走 MaintenanceStore”在接口类型上成立,但在运行时语义上并不成立。目前仅靠 HTTP 中间件 authenticationMiddleware 拒绝 IsSystem(),而 security.AccessContextFromContext(security.go:148)会原样接受放进 context 的 system context,进程内任意代码(例如某个 tool)只要调用 WithAccessContext(ctx, SystemAccessContext()) 就能绕过隔离。
  • 建议:业务方法(非 DeleteExpired / AdoptLegacySessions)在 access.IsSystem() 时返回 ErrResourceNotAccessible,或让 scopeSessions / CanAccessOwner 不再对 system 放行;AccessContextFromContext 也应拒绝 system。并在 store/test/contract.go 里补一个“system context 不能经由业务接口访问他人 Session”的用例。

2. 带方括号的 IPv6 host 会通过校验但启动失败(回归,低)

  • ai/component/server/component.go:141:isLoopbackHost 特意 strings.Trim(host, "[]"),说明 host: "[::1]" 被当作合法的回环地址;但 :236 改成了 net.JoinHostPort(s.host, ...),JoinHostPort 会对含 : 的 host 再加一层方括号,得到 [[::1]]:8888,Listen 报 missing port in address。我在本地验证了:[::1] → [[::1]]:8888 监听失败,而未加括号的 ::1 可以。改动前的 fmt.Sprintf("%s:%d") 对 [::1] 是可用的,所以这是一个回归。
  • 建议:在 Validate / NewServerComponent 里统一去掉方括号(或只接受不带括号的写法并在校验中报错),并补一个 IPv6 回环地址的测试。

3. adopt_legacy_sessions 的接线没有测试

  • ai/component/server/component.go:211-220:Start() 中根据 adopt_legacy_sessions 调用 AdoptLegacySessions 的逻辑(含失败时中止启动)没有任何测试覆盖;store 层有测试,但配置开关 → 调用 → 仅认领 tenant/owner 同时为空的行 这条链路没有端到端验证,而该开关会把历史数据全部归属到静态身份,建议至少补一个组件级测试(开关关闭时不认领、开启时认领)。

@Qiao-yq
Qiao-yq force-pushed the feat/ai-1539-session-ownership branch from 532bdc5 to 4c6bbad Compare October 18, 2026 13:14
@sonarqubecloud

Copy link
Copy Markdown

@Qiao-yq

Qiao-yq commented Oct 21, 2026

Copy link
Copy Markdown
Author

@robocanic Regarding point 1 (system capability): addressed in 4c6bbad by adding ValidateBusiness(). Business Store operations now reject system contexts, while maintenance operations retain system access. Shared contract tests cover rejection through Get and List.

Reference: #1592 (review)

@Qiao-yq

Qiao-yq commented Oct 21, 2026

Copy link
Copy Markdown
Author

@robocanic Regarding point 2 (IPv6 host): fixed in 4c6bbad. The constructor now normalizes bracketed IPv6 hosts before passing them to net.JoinHostPort. Added tests for IPv6 host normalization and loopback recognition.

Reference: #1592 (review)

@Qiao-yq

Qiao-yq commented Oct 21, 2026

Copy link
Copy Markdown
Author

@robocanic Regarding point 3 (legacy adoption tests): added wiring tests in 4c6bbad covering enabled and disabled adoption, the configured tenant/owner, system capability, and error propagation. These complement the existing Store tests for adopting only fully unowned sessions.

Reference: #1592 (review)

@AlexStocks AlexStocks left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

审查结论

方向正确,可以合并。 这是 ai 分支上 7 层堆叠 PR 的第 1 层(#1592 → #1594 → #1596 → #1608 → #1609 → #1610 → #1611),把「Session 只靠 ID 访问」改成显式 AccessContext,改动面覆盖 Memory/GORM 两个后端和 HTTP 边界,是本轮 AI Phase 1 里质量最高的一块。没有发现可导致越权读写的缺陷;下面列的三条都是可维护性与未来风险,不阻塞合并。

本次审查的基准

PR 作者声明本 PR 需先于 #1594/#1596 合并,属于严格堆叠。因此我按 merge-base(git merge-base origin/ai pr/1592)取证,而不是直接对 ai 读 diff:

  • 相对 ai 的累计 diff:+2015 / −440,39 个文件
  • 相对上一层的增量:+1895 / −439,39 个文件(本 PR 即链条底层,两者一致)
  • 与当前 ai(97d2db47,含 #1567/#1598)做真实三方合并:git merge-tree --write-tree origin/ai pr/1592 返回 0 冲突

已核实的实现细节

所有权判定是收敛到单点的。 ai/security/security.go:123 的 CanAccessOwner 是唯一判定入口,access.Validate() 失败、跨租户、tenantID/ownerID 为空一律返回 false,只有同租户且(UserID == ownerID 或持有 ai:sessions:admin)才放行。PolicyEnforcer / OwnershipPolicy 只是对它的包装,Store 侧又在 ai/store/memory/store.go:90,114,137,157,185,537 和 ai/store/gorm/store.go:110,152,690 逐点复核,属于「API 层判一次 + 存储层再判一次」的双层防御,不是单点信任。

身份来源不可伪造。 AccessContext.systemCapability 是 unexported 字段,外部包无法构造 AccessContext{} 通过 Validate();SystemAccessContext() 是唯一出口,且 ValidateBusiness() 会主动拒绝 system 上下文,保证系统能力只走 MaintenanceStore。ADR 0001 结尾也明确写了 MaintenanceStore 与私有标记「不是同进程内的能力边界」,这个自我限定是诚实的,不是掩饰。

错误码不回显存在性。 未授权与不存在都返回 ErrResourceNotAccessible,不会因为「404 vs 403」泄露 Session 是否存在。

静态身份的默认值有安全兜底。 默认只听 127.0.0.1、无跨域、tenant default、user local-user;非 loopback 监听必须显式打开 allow_insecure_static_auth。ADR 里也写清了「static 模式不得被描述为生产级用户隔离」。

建议(不阻塞)

P2 — SystemAccessContext() 是包级公开函数,等于给全模块发了万能钥匙。 ADR 已经承认这一点,但在 ai/ 内部的任何包都可以一行拿到 system 上下文,后续 PR(#1611 的摘要清理、#1596 的 retention)会不断新增维护路径,误用面只会变大。建议把 system 能力的铸造收进 bootstrap:只暴露 MaintenanceStore 接口 + 一个由启动期注入的私有 token,业务包即便拿到接口也拿不到 AccessContext。至少要加 grep 级别的 CI 检查,限制 SystemAccessContext() 的调用点白名单。

P3 — CanAccessOwner 在 IsSystem() 分支提前返回 true,跳过了空值检查。 ai/security/security.go:123-136 的顺序是「system → 直接 true」,所以 CanAccessOwner("", "") 对 system 上下文返回 true。这是 legacy adoption(只接管 tenant/owner 都为空的行)需要的语义,但当前没有注释说明这个顺序是刻意的,后面很容易被人「顺手修正」成先判空、直接打断 adoption。建议在该分支上加一行注释,说明空值放行是 adoption 的依赖。

P3 — Subject.UserID == ownerID 是裸字符串比较。 没有归一化(大小写、空白)。如果后续 JWT/SSO 适配器(#1600/#1609)对 sub 的来源不一致,同一个用户会出现两个身份。建议在 NewAccessContext 里对 TenantID/UserID 做一次 TrimSpace 并明确大小写敏感策略。

合并前置

  • 无阻塞项。
  • 注意本 PR 是 #1594/#1596/#1608/#1609/#1610/#1611 的底层,它是这 7 个 PR 里唯一必须单独保证质量的一层,合并顺序不能跳。

@larry-zy

Copy link
Copy Markdown

LGTM

@Qiao-yq

Qiao-yq commented Oct 25, 2026

Copy link
Copy Markdown
Author

Thanks for the thorough review. I agree with the remaining P2/P3 observations.

For this PR, I will keep the current design. MaintenanceStore and SystemAccessContext are intentional boundaries for trusted in-process code, and ADR 0001 documents that they are not a strict capability-security boundary. The IsSystem() branch in CanAccessOwner is retained for maintenance and legacy-adoption semantics, while business operations reject system contexts through ValidateBusiness().

Stricter system-capability issuance and identity canonicalization can be addressed in follow-up authentication and maintenance hardening work. No further changes are planned for this PR. Thanks for confirming that there are no blocking issues.

@robocanic
robocanic merged commit e6cef77 into apache:ai Nov 2, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants