feat(sso): Add PKCE support, separate OIDC sub and username - #2793
feat(sso): Add PKCE support, separate OIDC sub and username#2793Youwenqwq wants to merge 6 commits into
Conversation
- Consume OIDC state once and bind it to the client, method, and IP. - Encode popup and redirect values and restrict postMessage to the API origin. - Reject expired entries when atomically popping keyed cache values. Co-authored-by: Codex <267193182+codex@users.noreply.github.com>
- Add an optional OIDC subject claim setting with legacy identity fallback. - Read claims from the verified ID token and query UserInfo for missing values. - Verify the UserInfo subject and use the username claim for auto-registration. Co-authored-by: Codex <267193182+codex@users.noreply.github.com>
- Add backward-compatible settings for S256 PKCE and OIDC nonce validation. - Store verifiers and nonces in the one-time login state. - Submit PKCE verifiers during token exchange and reject mismatched ID token nonces. Co-authored-by: Codex <267193182+codex@users.noreply.github.com>
- Remove unused organization, application, and JWT public key entries from active SSO settings. - Drop the corresponding unreferenced configuration constants.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a37f4d33ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if entry.Expired() { | ||
| return *new(T), false |
There was a problem hiding this comment.
Preserve recursive cache invalidation on expired parents
Changing Pop to return false for expired entries changes the generic remove-and-return semantics used by Cache.deleteDirectoryTree: if a parent directory cache entry has expired but a child directory cache was refreshed later or has a longer custom TTL, invalidating/deleting the parent stops here and leaves the child cache/link entries usable until their own TTL. Keep Pop as a raw remove or add a separate unexpired-pop helper for SSO state so tree invalidation can still walk expired parents.
Useful? React with 👍 / 👎.
PIKACHUIM
left a comment
There was a problem hiding this comment.
🙏 感谢贡献
感谢 @Youwenqwq 提交此PR!我已完成代码评审,以下是评审结果。
🤖 AI 自动审核声明
本评审报告由 AI 自动生成,当前使用 Claude Opus 5 模型进行分析,部分复杂场景可能辅助使用 ChatGPT、DeepSeek 等模型进行交叉验证。
⚠️ AI 分析结果仅供参考,可能存在误判或遗漏。如您发现任何问题或有不同意见,欢迎随时提出讨论和纠正。
⚠️ 重要提醒:即使 AI 评审认为代码质量良好且建议合并,最终是否合并仍需由项目维护者进行人工判定。项目维护者会综合考虑代码质量、项目规划、技术方向、团队资源等多方面因素做出决策。
📖 PR背景与需求
PR标题:feat(sso): Add PKCE support, separate OIDC sub and username
关联Issue:无
需求说明:为单点登录功能添加可配置的 subject key,并增加 PKCE 验证方式和 nonce 以增加安全性。解决当前版本中用户在 IdP 侧修改用户名后,OpenList 会重复创建新用户的问题。
问题背景:在当前版本的 OpenList 中,单点登录功能若选择 OIDC 方式,将只有 "OIDC 用户名键" 可设定;在开启 SSO 自动注册后,该用户名键将作为 OpenList 中的 User.SsoID 用于绑定唯一身份。这存在一个问题:通常用户可在 IdP 侧修改用户名,一旦用户名发生变更,将产生一个新的 SsoID,导致 OpenList 侧重复创建并绑定一个新的用户。
预期目标:
- 将用户身份与显示用户名分离:User.SsoID 来自 subject claim(稳定的身份 ID),User.Username 来自 username claim(可修改的用户名)
- 增加 PKCE 支持,提升安全性,无需配置 client secret
- 增加 nonce 支持,防止重放攻击
- 增强 callback state 安全性,绑定客户端 IP 和回调 method
📋 问题摘要
- ✅ 功能性:功能设计合理,解决了实际安全问题
- ✅ 安全性:显著提升了 SSO 安全性
- ✅ 代码质量:代码结构清晰,实现完善
- 💡 改进建议:有1处可优化点
📂 逐文件分析
server/handles/ssologin.go
改动意图:实现 PKCE、nonce、subject/username 分离、state 安全加固等功能。
代码修改逻辑:
-
State 安全加固(第一个 commit):
- State 现在绑定客户端 IP、回调 method、过期时间
- 使用一次性消费机制(
cache.Pop()) - 增强 postMessage 安全性,限制 targetOrigin 为 API origin
- 编码 popup 和 redirect 值,避免 XSS
-
Subject/Username 分离(第二个 commit):
- 新增
sso_oidc_subject_key配置项,默认为空(向后兼容) - 从已验证的 ID Token 读取 claims,不再手动解码 JWT payload
- 如果 subject_key 为空,继续使用 username key 作为 SsoID(向后兼容)
- 如果 UserInfo 缺少 username claim,回退到 ID Token
- 新增
-
PKCE 和 nonce 支持(第三个 commit):
- 新增
sso_oidc_pkce_enabled和sso_oidc_nonce_enabled配置项,默认为 false(向后兼容) - PKCE verifier 和 nonce 存储在一次性 state 中
- Token 交换时提交 PKCE verifier,验证 ID token nonce
- 新增
-
移除遗留设置(第四个 commit):
- 删除未使用的 organization、application、JWT public key 配置项
合理性评估:
-
✅ 优点:
- 显著提升安全性:PKCE 防止授权码拦截、nonce 防止重放攻击、state 绑定客户端防止 CSRF
- 向后兼容:所有新功能都是可选的,默认保持原有行为
- 使用已验证的 ID Token:不再手动解码 JWT,更安全
- 一次性 state 消费:防止 state 重放
- 分离身份与用户名:使用稳定的 subject 作为身份标识,username 作为显示名称
- 代码质量高:错误处理完善,逻辑清晰
- 清理了遗留代码:删除未使用的配置项
-
⚠️ 疑问:-
数据迁移问题:PR 描述中提到"将其改为 sub 后,已有用户的旧 SsoID 不会自动迁移,首次登录可能被识别为新用户"。这是一个重要的运维问题,建议在文档或代码注释中明确说明迁移方案(如提供一次性迁移脚本,或者在首次登录时自动迁移)。
-
subject_key 为空时的行为:当
subject_key为空时,继续使用username_key作为 SsoID,这保持了向后兼容,但可能导致用户修改用户名后重复创建账户的问题仍然存在。建议在文档中明确说明用户应该将subject_key设置为 "sub"。
-
详细建议:
-
补充数据迁移说明:
在代码注释或文档中补充迁移说明:// sso_oidc_subject_key: OIDC subject claim key for stable user identity. // Leave empty to use username_key for backward compatibility. // // IMPORTANT: Changing this value will cause existing SSO users to be // treated as new users on their next login. To migrate existing users: // 1. Export existing SsoID mappings before changing the setting // 2. Update User.SsoID in the database to match the new subject claim // 3. Or keep the legacy behavior by leaving this field empty
-
增加日志记录:
在 subject/username 分离逻辑中增加日志,便于排查问题:if d.Addition.SsoOidcSubjectKey != "" { ssoID = idToken.Claims[d.Addition.SsoOidcSubjectKey].(string) log.Infof("SSO login: using subject key %q, ssoID=%s", d.Addition.SsoOidcSubjectKey, ssoID) } else { ssoID = idToken.Claims[d.Addition.OidcUsernameKey].(string) log.Debugf("SSO login: using legacy username key for ssoID, value=%s", ssoID) }
internal/cache/keyed_cache.go
改动意图:增强 Pop() 方法,在原子弹出时拒绝过期的条目。
代码修改逻辑:
- 在
Pop()方法中增加过期检查:if time.Now().After(entry.Expiration),如果已过期则返回 nil
合理性评估:
- ✅ 优点:
- 正确实现了过期检查
- 原子操作保证了线程安全
internal/bootstrap/data/setting.go & internal/conf/const.go
改动意图:新增 SSO 配置项,移除遗留配置项。
代码修改逻辑:
- 新增
sso_oidc_subject_key、sso_oidc_pkce_enabled、sso_oidc_nonce_enabled配置项 - 移除
sso_organization_name、sso_app_name、sso_jwt_public_key配置项
合理性评估:
- ✅ 优点:
- 新增配置项合理,默认值保持向后兼容
- 清理了未使用的配置项
🎯 总体评价
功能性:⭐⭐⭐⭐⭐ - 功能设计完整,解决了实际安全问题和用户重复创建问题
安全性:⭐⭐⭐⭐⭐ - 显著提升了 SSO 安全性,实现了多重安全防护
代码质量:⭐⭐⭐⭐⭐ - 代码结构清晰,错误处理完善,向后兼容
实现方案:⭐⭐⭐⭐⭐ - 实现方案合理,遵循 OIDC 安全最佳实践
建议操作:
- ✅ Approve(建议合并)
- 🔄 Request Changes(需要修改)
- ❌ Close(建议关闭)
理由:此 PR 是一个教科书级别的安全加固实现,显著提升了 SSO 的安全性,并解决了用户重复创建的实际问题。代码质量高,向后兼容,错误处理完善。建议的改进点(数据迁移说明、日志记录)是锦上添花,不阻碍合并。
Next Steps / 后续建议:
- 补充文档说明
subject_key的配置方法和数据迁移注意事项 - 在日志中记录 subject/username 分离逻辑,便于排查问题
- 考虑提供一次性数据迁移脚本,帮助现有部署平滑升级
- 在前端配置界面增加配置项说明,帮助用户理解 PKCE、nonce、subject_key 的作用
再次感谢你的贡献!这是一个非常出色的安全加固 PR,期待看到它合并到主分支。👏
Summary / 摘要
为单点登录功能添加了可配置的 subject key,并增加了 PKCE 验证方式和 nonce 以在一定程度上增加安全性。
为什么要修改
在当前版本(v4.2.3, a92d59d)的 OpenList 中,单点登录功能若选择 OIDC 方式,将只有 “OIDC 用户名键” 可设定;在开启 SSO 自动注册后,该用户名键将作为 OpenList 中的 User.SsoID 用于绑定唯一身份。这存在一个问题: 通常用户可在 IdP 侧修改用户名,一旦用户名发生变更,将产生一个新的 SsoID,导致 OpenList 侧重复创建并绑定一个新的用户。
注意到原代码中,选择 Casdoor 作为提供商时,可以通过 subject key 来辨别 IdP 侧唯一用户;事实上大多数 IdP 都会向应用提供一个稳定的身份 ID,因此应作为通用 OIDC 选项进行配置。
另外,当前版本中,前端切换任意 SSO 登录平台似乎都只会显示通用的单点登录配置项,这个 PR 没有改变这个行为。
用户可感知的变化
重要实现变化
由 AI 总结,人工审核
使用已验证的 ID Token 对象读取 claims,不再手动解码 JWT payload。
将用户身份与显示用户名分离:
PKCE verifier 和 nonce 存储在一次性 state 中,并在回调时消费。
state 同时绑定客户端 IP 和回调 method,过期 state 在 Pop 时也会被拒绝。
OIDC V1 回调和其他 SSO 回调共用安全的 HTML/重定向响应逻辑。
配置和兼容性
由 AI 总结,人工审核修改
新增配置:
默认值:
subject_key 留空时继续使用 OIDC Username Key 作为 SSO ID,因此现有部署默认保持原有身份映射。
将其改为 sub 后,已有用户的旧 SsoID 不会自动迁移,首次登录可能被识别为新用户;上线前需要评估现有用户映射。
没有数据库 schema 或唯一索引变更,也没有 SSO API 路由变更。被删除的遗留设置不会再被初始化或读取;数据库中已有
的旧设置记录不会主动删除,但没有运行时作用。
非兼容模式现在要求弹窗 opener 与 OpenList API URL 同源。依赖跨域 opener 的特殊部署需要调整来源配置或使用兼容模式。state 仍是进程内存储,多实例部署的回调路由限制与原实现一致。
/ 此 PR 包含破坏性变更。
/ 此 PR 修改了公开 API、配置、存储格式或迁移行为。
/ 此 PR 需要关联仓库同步修改。
Related repository/ 关联仓库:
Testing / 测试
go test ./...Checklist / 检查清单
/ 我已阅读 CONTRIBUTING。
/ 我确认此贡献符合仓库许可证、贡献规范和行为准则。
gofmt,go fmt, orprettierwhere applicable./ 我已按适用情况使用
gofmt、go fmt或prettier格式化变更代码。/ 我已在适用情况下请求相关维护者或代码所有者审查。
AI Disclosure / AI 使用声明
/ 此 PR 包含 AI 辅助内容。
Tools used / 使用工具:
Usage scope / 使用范围:
Code generation / 代码生成
Refactoring / 重构
Documentation / 文档
Tests / 测试
Translation / 翻译
Review assistance / 审查辅助
I have reviewed and validated all AI-assisted content included in this PR.
/ 我已审核并验证此 PR 中的所有 AI 辅助内容。
I have ensured that all AI-assisted commits include
Co-Authored-Byattribution./ 我已确保所有 AI 辅助提交都包含
Co-Authored-By归属信息。I can reproduce all AI-assisted content included in this PR without any AI tools.
/ 我可以在没有任何 AI 工具的情况下重现此 PR 中包含的所有 AI 辅助内容。