fix(ai-proxy): fix bedrock Sigv4 mismatch #2402
Conversation
修复 AWS Bedrock Sigv4 签名路径编码问题并添加配置文档支持变更概述新功能
问题修复
安全增强
文档更新
变更统计
变更文件
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
![lingma-agents[bot]](https://avatars.githubusercontent.com/in/1198481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 0 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 1 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 0 | 次要问题，酬情优化。例如：代码格式不规范或注释缺失。 |
总计: 1 个问题
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
🔹 plugins/wasm-go/extensions/ai-proxy/provider/bedrock.go (1 💬)
- 修复modelId解码逻辑的潜在数据污染风险 (L763)
🚀 架构设计建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. AWS凭证配置缺少安全存储建议
在README中文和英文文档新增的Bedrock配置示例中，直接展示了明文AWS访问密钥和秘密密钥。项目未提供敏感凭证的存储安全建议（如环境变量、密钥管理服务等），可能导致用户将敏感信息以明文形式存储在配置文件中，存在安全风险。建议补充安全存储凭证的最佳实践说明。
📌 关键代码
| `awsAccessKey` | string | 必填 | - | AWS Access Key，用于身份认证 |
| `awsSecretKey` | string | 必填 | - | AWS Secret Access Key，用于身份认证 || `awsAccessKey` | string | Required | - | AWS Access Key used for authentication |
| `awsSecretKey` | string | Required | - | AWS Secret Access Key used for authentication |用户可能将敏感凭证以明文形式存储在配置文件或版本库中，导致AWS账户被非法访问。
🔍2. 路径编码逻辑存在架构不一致性
在bedrock.go中新增的encodeSigV4Path函数对路径分段进行URL编码，但其他云服务提供商（如Azure/OpenAI）的签名实现未采用统一的路径处理规范。需确认该编码逻辑是否符合AWS Sigv4规范要求，并评估是否需要在所有需要签名的服务中建立统一的路径编码标准。
📌 关键代码
func encodeSigV4Path(path string) string {
...
segments[i] = url.PathEscape(seg)
...}若编码方式与其他服务不一致，可能导致签名验证失败或出现安全漏洞。
🔍3. 模型ID解码未处理异常情况
在bedrock.go的buildChatCompletionResponse函数中，新增的modelId解码操作直接忽略错误返回值（_ := url.QueryUnescape），未处理解码失败场景。若模型ID包含无法解码的字符，可能导致空值或无效模型标识，引发后续接口调用异常。
📌 关键代码
modelId, _ := url.QueryUnescape(ctx.GetStringContext(ctxKeyFinalRequestModel, ""))模型ID解码失败时可能导致空指针异常或调用错误服务端点。
🔍4. 配置参数未实施校验机制
新增的AWS配置参数（awsRegion等）在配置加载时未添加有效性校验。例如未验证区域是否属于AWS有效区域列表，可能导致配置错误后服务无法正常工作。建议在配置加载阶段增加参数合法性校验。
📌 关键代码
| `awsRegion` | string | 必填 | - | AWS 区域，例如：us-east-1 |无效区域配置可能导致服务调用失败且错误提示不明确，增加运维排查难度。
审查详情
📒 文件清单 （3 个文件）
📝 变更: 3 个文件
📝 变更文件:
plugins/wasm-go/extensions/ai-proxy/README.mdplugins/wasm-go/extensions/ai-proxy/README_EN.mdplugins/wasm-go/extensions/ai-proxy/provider/bedrock.go
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
| }, | ||
| } | ||
| requestId := ctx.GetStringContext(requestIdHeader, "") | ||
| modelId, _ := url.QueryUnescape(ctx.GetStringContext(ctxKeyFinalRequestModel, "")) |
There was a problem hiding this comment.
修复modelId解码逻辑的潜在数据污染风险
🟡 Major | 🐞 Bugs
📋 问题详情
在buildChatCompletionResponse函数中，modelId使用url.QueryUnescape解码时忽略了错误返回值。如果原始值无法解码，可能导致不可预测的模型ID值，引发后续API调用失败。
💡 解决方案
需要处理解码失败情况，建议：
-modelId, _ := url.QueryUnescape(ctx.GetStringContext(ctxKeyFinalRequestModel, ""))
+modelId, err := url.QueryUnescape(ctx.GetStringContext(ctxKeyFinalRequestModel, ""))
+if err != nil {
+ modelId = ctx.GetStringContext(ctxKeyFinalRequestModel, "") // 使用原始值作为回退
+}确保在解码失败时使用原始字符串，避免空值或错误值导致模型调用失败。
您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2402 +/- ##
===========================================
+ Coverage 35.91% 46.05% +10.14%
===========================================
Files 69 81 +12
Lines 11576 13018 +1442
===========================================
+ Hits 4157 5995 +1838
+ Misses 7104 6678 -426
- Partials 315 345 +30 🚀 New features to boost your workflow:
|
3 participants
Ⅰ. Describe what this PR did
fix bedrock Sigv4 mismatch
ref: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_sigv-create-signed-request.html
URL Encoding Rules for Canonical String in AWS SigV4
Path Encoding (Canonical URI)
A-Z,a-z,0-9,-,_,.,~) and reserved forward slashes (/)./my path→/my%20path(space encoded as%20, not+)./) without encoding%2Finstead of%2f).Ⅱ. Does this pull request fix one issue?
fixes: #2396