feat: 凭证管理增强 --story=125449007#548
Conversation
There was a problem hiding this comment.
代码审查总结
本次 PR 引入了凭证管理系统,整体设计良好,测试覆盖充分。发现以下关键问题需要修复:
🚨 严重问题
- 异常捕获过于宽泛 -
create_credential.py:56和多处使用裸except Exception,可能隐藏关键错误 - SQL 注入风险 -
test_secret_json_field.py:52使用字符串拼接 SQL,应使用参数化查询 - 代码逻辑不完整 -
create_credential.py:56有未完成的代码片段if scope_l
⚡ 性能问题
- N+1 查询 - 凭证作用域检查可能导致多次数据库查询
⚠️ 逻辑问题
- 作用域验证逻辑 -
CredentialScopeLevel.NONE处理不明确 - 事务完整性 - 部分更新操作缺少事务保护
✨ 建议改进
- 添加凭证轮换机制
- 考虑凭证使用审计日志
- 增强密钥管理的安全性
审查评级: 需要修复严重问题后再合并
| :param request: HTTP 请求对象 | ||
| :param space_id: 空间ID | ||
| :return: 创建的凭证信息 | ||
| """ |
There was a problem hiding this comment.
🚨 语法错误: 此行代码不完整 if scope_l,缺少完整的条件判断,会导致 Python 语法错误
| default=CredentialScopeLevel.NONE.value, | ||
| choices=Credential.CREDENTIAL_SCOPE_LEVEL_CHOICES, | ||
| ) | ||
| scopes = serializers.ListField( |
There was a problem hiding this comment.
except Exception 会捕获所有异常,包括系统错误。建议改为具体异常类型如 except ValidationError
There was a problem hiding this comment.
✅ 此问题似乎已被最近的更改解决,现在使用具体类型捕获异常。
|
|
||
|
|
||
| class UpdateCredentialSerializer(serializers.Serializer): | ||
| name = serializers.CharField(help_text=_("凭证名称"), max_length=32, required=False) |
There was a problem hiding this comment.
There was a problem hiding this comment.
✅ 此问题似乎已被最近的更改解决,异常捕获已改为具体类型。
| :param space_id: 空间ID | ||
| :param credential_id: 凭证ID | ||
| :return: 更新后的凭证信息 | ||
| """ |
There was a problem hiding this comment.
with transaction.atomic() 包裹整个更新逻辑
There was a problem hiding this comment.
✅ 此问题似乎已被最近的更改解决,update_credential 视图中已添加 transaction.atomic() 包裹整个更新逻辑。
4bb7eab to
1053ba7
Compare
There was a problem hiding this comment.
代码审查总结
本次 PR 实现了凭证管理的完整重构,包括多类型凭证支持、作用域控制和 AES 加密存储,整体设计合理。以下是几个需要关注的问题:
🔒 安全问题
bkflow/utils/crypt.py — 固定 IV 导致语义安全性缺失
ROOT_IV = b"TencentBkApp--Iv" 在所有加密操作中被复用。在 AES-CFB 模式下,相同 key + 相同 IV 加密相同明文会得到相同密文,攻击者可通过密文相同推断明文相同,且若多段密文使用相同 key/IV 还存在 XOR 攻击风险。建议每次加密时随机生成 IV 并将其附在密文前(IV 无需保密)。
bkflow/utils/crypt.py — __parse_key 截断 key 存在风险
若 INSTANCE_KEY 长度不足24字节,self.INSTANCE_KEY[:24].encode() 后的 key 会更短,AES 不接受非16/24/32字节的 key,会在运行时抛 ValueError,且错误信息不够明确。建议显式校验 key 长度。
⚠️ 逻辑问题
bkflow/space/credential/basic_auth.py — display_value() 直接修改内部状态
调用 display_value() 后再调用 value() 返回的 self.data 中 password 已变为星号,导致数据丢失。bk_access_token.py 和 bk_app.py 也存在同样问题。建议返回副本:return {**self.data, "password": "*********"}。
bkflow/space/views.py — 切换 scope_level 时旧作用域未清理
update_scopes 方法只在 scope_level == PART 时删除旧作用域。当凭证从 PART 更新为 ALL 或 NONE 时,对应的 CredentialScope 记录不会被删除,产生孤儿数据,并导致 has_scope() 判断异常。建议在 update=True 时无条件先删除旧作用域。
bkflow/apigw/serializers/credential.py — if credential 分支永远不会触发
CredentialDispatcher 构造失败时会抛 CredentialTypeNotSupport 异常,不会返回 None,所以 else: data["content"] = {} 永远不会执行。建议改为 try/except CredentialTypeNotSupport 处理。
✨ 改进建议
bkflow/space/views.py:partial_update — content 更新绕过了类型校验
update_credential.py(apigw 视图)通过 credential.update_credential(value) 更新 content(含校验),但 views.py 的 partial_update 使用 setattr(instance, "content", value) 直接覆盖,会绕过类型校验逻辑,建议统一使用 instance.update_credential(value) 方法。
ef5ca8b to
95ad22f
Compare
* feat: 节点配置添加凭证选择 --story=125449007 # Reviewed, transaction id: 66075 * fix: 关闭label提示下划线 --story=125449007 # Reviewed, transaction id: 66078 * feat: 节点配置添加凭证选择 --story=125449007 # Reviewed, transaction id: 66104 * fix: 凭证选择校验优化 --story=125449007 # Reviewed, transaction id: 66163
95ad22f to
6a4917d
Compare
There was a problem hiding this comment.
代码审查总结
本次 PR 对凭证管理系统进行了重构,新增多类型凭证支持(BK_ACCESS_TOKEN、BASIC_AUTH、CUSTOM)、作用域控制和 AES 加密存储。整体方向正确,上次审查指出的问题已全部修复。✅
仍存在以下需要关注的问题:
🔒 安全问题
bkflow/utils/crypt.py — 固定 IV 导致加密安全性不足
ROOT_IV = b"TencentBkApp--Iv" 在所有加密中被复用。AES-CFB 模式下相同 key + IV 加密同一明文得到相同密文,攻击者可据此推断明文。建议每次加密时生成随机 IV 并存储在密文前。
bkflow/utils/crypt.py:63 — __parse_key key 截断风险
self.INSTANCE_KEY[:24].encode() 若 INSTANCE_KEY(即 PRIVATE_SECRET env)未配置(默认为空字符串),截断后 key 长度为 0,AES 会在运行时抛 ValueError,且报错信息不清晰。建议在 __init__ 中显式校验 key 长度 ≥ 16/24/32 字节。
⚠️ 逻辑问题
bkflow/apigw/serializers/credential.py:35 — if credential 永远为真,else 分支死代码
CredentialDispatcher.__init__ 在类型不支持时抛出 CredentialTypeNotSupport 异常,不会返回 None;因此 credential 对象始终非空,else: data["content"] = {} 永远不会执行。应改为 try/except CredentialTypeNotSupport。
bkflow/space/views.py — 切换 scope_level 时旧作用域未清理(孤儿数据)
update_scopes 方法仅在 scope_level == PART 时删除旧记录。当凭证从 PART 切换为 ALL 或 NONE 时,CredentialScope 表中的旧记录不会被删除,形成孤儿数据并可能导致 has_scope() 行为异常。建议在 update=True 时无条件先清理旧记录。
bkflow/apigw/views/update_credential.py:67 — update_credential 内部已调用 save(),外层再次 credential.save() 造成双重写入
Credential.update_credential(value) 在 models.py:353 末尾已执行 self.save();第 72 行再次 credential.save() 会造成重复写入,且可能以旧字段值覆盖刚写入的 content。应在 if attr == "content" 分支中跳过外层的 credential.save() 调用,或重构逻辑统一保存。
bkflow/space/credential/basic_auth.py:56 / bk_app.py / bk_access_token.py — display_value() 直接修改内部状态
self.data["password"] = "*********" 直接修改了实例的 data 字典,导致调用 display_value() 后再调用 value() 返回的也是脱敏值,原始凭证丢失。应返回副本:return {**self.data, "password": "*********"}。
✨ 改进建议
bkflow/utils/models.py:176 — 裸 except Exception 可能掩盖解密失败的真实原因
静默吞掉所有解密异常会使密钥错误、数据损坏等问题难以排查。建议至少记录 warning 日志,或仅捕获 (ValueError, UnicodeDecodeError) 等预期的解密失败类型。
| def to_representation(self, instance): | ||
| data = super().to_representation(instance) | ||
| credential = CredentialDispatcher(credential_type=instance.type, data=instance.content) | ||
| if credential: |
There was a problem hiding this comment.
if credential 死代码:CredentialDispatcher.__init__ 类型不支持时直接抛出异常,不会返回 None,所以 else: data["content"] = {} 永远不执行。应改为 try/except CredentialTypeNotSupport 明确捕获。
| space_id = serializer.validated_data.get("space_id") | ||
| queryset = queryset.filter(space_id=space_id, is_deleted=False) | ||
| return queryset | ||
|
|
There was a problem hiding this comment.
scope_level 时旧作用域未清理:从 PART 切换为 ALL 或 NONE 时,CredentialScope 旧记录不删除,形成孤儿数据并可能导致 has_scope() 返回错误结果。建议在 update=True 时无条件先执行 CredentialScope.objects.filter(credential_id=credential.id).delete()。
There was a problem hiding this comment.
代码审查总结
✅ 已解决的先前问题
相较于上次审查(commit 4bb7eab4b94baf3a77904488670bf23e29f7aac5),以下问题在当前版本已得到修复:
create_credential.py— 不完整代码片段if scope_l已消除,改用transaction.atomic()+CredentialScope.bulk_createupdate_credential.py— 作用域更新现在包裹在事务中serializers/credential.py—validate()已改为捕获具体异常CredentialTypeNotSupport
🔒 安全问题
bkflow/utils/crypt.py 第 39 行 — ROOT_IV = b"TencentBkApp--Iv" 固定 IV 复用
AES-CFB 模式下相同 key + IV 加密相同明文得到相同密文,存在语义安全性缺失与 XOR 攻击风险。建议每次加密随机生成 16 字节 IV,并拼接在密文前(解密时读取前 16 字节作为 IV)。
bkflow/utils/crypt.py 第 63 行 — self.INSTANCE_KEY[:24].encode() 密钥截断
若 INSTANCE_KEY 长度不足 24 字节,AES 会因 key 长度无效而在运行时抛 ValueError。建议在 __init__ 处显式断言或使用 PBKDF2/HKDF 派生固定长度密钥。
⚠️ 逻辑问题
bkflow/space/credential/basic_auth.py 第 46 行 — display_value() 直接修改 self.data
self.data["password"] = "*********" 会永久改写内部状态,之后再调用 value() 返回的 data 中密码已变为星号,数据丢失。bk_access_token.py 和 bk_app.py 存在同样问题。建议改为 return {**self.data, "password": "*********"} 返回副本。
bkflow/space/views.py — update_scopes 切换到 ALL/NONE 时旧作用域不清理
update_scopes 仅在 scope_level == PART 时删除旧记录。当凭证从 PART 更新为 ALL 或 NONE 时,CredentialScope 表中的旧记录不会被删除,产生孤儿数据,导致 can_use_in_scope() 在极端情况下行为异常。建议在 update=True 时无条件先删除该凭证的所有旧作用域。
bkflow/apigw/serializers/credential.py 第 26 行 — if credential: 判断永远为真
CredentialDispatcher() 构造失败时抛 CredentialTypeNotSupport 异常而不返回 None,因此 else: data["content"] = {} 永远不会执行。建议改为 try/except CredentialTypeNotSupport 来处理序列化失败场景。
bkflow/apigw/views/update_credential.py 第 281-286 行 — 重复 save()
credential.update_credential(value) 内部已调用 self.save()(见 models.py),循环结束后的 credential.save() 会再次触发一次无谓的数据库写入。建议在执行 update_credential() 后跳过后续 save(),或重构逻辑分支。
✨ 整体评价
本次 PR 完成了凭证管理的重要重构,事务保护、多类型凭证支持、AES 加密存储等主体实现合理,测试覆盖充分。主要需要关注加密安全性(固定 IV)和 display_value() 的状态修改问题,建议在合并前修复。
| specific language governing permissions and limitations under the License. | ||
|
|
||
| We undertake not to change the open source license (MIT license) applicable | ||
|
|
There was a problem hiding this comment.
🔒 固定 IV 导致语义安全性缺失:所有加密操作复用同一 IV,相同明文每次得到相同密文,CFB 模式下还存在 XOR 攻击风险。建议每次调用 encrypt() 时用 os.urandom(16) 生成随机 IV,并将 IV 前缀拼入密文(解密时先取前 16 字节作为 IV)。
| decrypt_key = self.__parse_key() | ||
| if isinstance(plaintext, str): | ||
| plaintext = plaintext.encode(encoding=self.encoding) | ||
| secret_txt = AES.new(decrypt_key, AES.MODE_CFB, self.ROOT_IV).encrypt(plaintext) |
There was a problem hiding this comment.
🔒 密钥截断存在运行时崩溃风险:若 INSTANCE_KEY 长度不足 24 字节,encode() 后 AES 收到非法 key 长度会抛 ValueError。建议在 init 中加断言校验,或改用 hashlib.sha256(key).digest()[:32] 派生固定 32 字节密钥。
There was a problem hiding this comment.
凭证管理增强 PR 审查总结
本次 PR 为凭证管理系统引入了重大增强,包括新增凭证类型(BK_ACCESS_TOKEN、BASIC_AUTH、CUSTOM)、凭证作用域控制、内容加密迁移以及相关 API 扩展。整体设计合理,测试覆盖较好。以下标注了几个需要关注的问题:
🚨 严重问题
bkflow/apigw/views/update_credential.py 双重 save 问题
当请求体同时包含 content 和其他字段(如 name、scope_level)时,update_credential(value) 内部会调用 self.save(),之后第 286 行又会再次调用 credential.save(),导致两次写库。当 content 更新已通过内部 save() 落库后,后续 credential.save() 会用循环中被 setattr 修改的其他字段覆盖写入,逻辑正确性依赖字段顺序,存在潜在数据不一致风险。建议将 content 字段处理与其他字段统一在一次 save 中完成。
🔒 安全问题
bkflow/space/credential/basic_auth.py 和 bk_access_token.py 的 display_value() 直接修改 self.data
self.data["password"] = "*********" 直接修改了原始数据引用,若同一实例被多次访问(如序列化后再调用 value()),将返回已脱敏的星号而非真实凭证值。CustomCredential.display_value() 已正确使用副本,建议这两个类同样改为 display_data = dict(self.data) 再操作。
⚠️ 逻辑问题
bkflow/space/credential/resolver.py 数字字符串被误判为 credential_id
str(value).isdigit() 会将纯数字密码(如 "123456")误判为凭证引用 ID,导致错误的凭证解析。建议明确区分凭证引用格式(如约定传 int 类型 ID,字符串始终视为直接值),或使用专用结构区分引用与直接值。
⚠️ 逻辑问题
UpdateCredentialSerializer 的 scope_level 默认值会在 PATCH 请求中意外降级凭证作用域
scope_level 设置了 default=CredentialScopeLevel.NONE.value,在 partial=True 的 PATCH 请求中若调用方未传该字段,序列化器会自动填充 NONE,导致已配置好的作用域被静默重置。建议在 UpdateCredentialSerializer 中去掉该默认值,改为纯 required=False。
✨ 改进建议
0009_encrypt_credential_content.py 加密幂等性判断依赖确定性加密假设
迁移注释说明了 encrypt(decrypt(x)) == x 的前提,但若底层加密使用随机 IV 则此假设不成立,可能导致已加密数据被重复加密损坏。建议在迁移入口处添加该假设的单元测试保障,并确保 rollback 路径在 CI 中经过验证。
| from bkflow.space.exceptions import CredentialTypeNotSupport | ||
| from bkflow.space.models import Credential, CredentialScopeLevel | ||
| from bkflow.space.serializers import CredentialScopeSerializer | ||
|
|
There was a problem hiding this comment.
| CredentialBaseQuerySerializer, | ||
| CredentialSerializer, | ||
| CredentialScopeSerializer, | ||
| SpaceConfigBaseQuerySerializer, |
There was a problem hiding this comment.
|
|
||
| for attr, value in credential_data.items(): | ||
| if attr == "content": | ||
| # 使用update_credential方法来更新content,会做验证 |
There was a problem hiding this comment.
⚡ 重复 save() 导致多余数据库写入:update_credential(value) 内部已调用 self.save()(见 models.py),循环结束后的 credential.save()(第 69 行)会再次触发一次额外写入。建议在 content 分支中跳过后续 save(),或将 update_credential() 重构为只更新字段不保存,统一由外层 save() 提交。
|
|
||
| # KEY 和 IV 的长度需等于16 | ||
| ROOT_KEY = b"TencentBkApp-Key" | ||
| ROOT_IV = b"TencentBkApp--Iv" |
There was a problem hiding this comment.
🔒 固定 IV 导致语义安全性缺失:ROOT_IV 在所有加密调用中复用,AES-CFB 模式下相同 key+IV 加密相同明文会得到相同密文,且存在 XOR 攻击风险。建议每次加密时随机生成 IV,并将其拼接在密文头部(IV 无需保密)。
| return plain.decode(encoding=self.encoding) | ||
|
|
||
| def __parse_key(self): | ||
| return self.INSTANCE_KEY[:24].encode() |
There was a problem hiding this comment.
🔒 key 截断存在静默失效风险:若 INSTANCE_KEY 长度不足 24 字节,[:24].encode() 会生成不足 24 字节的 bytes,AES 会抛 ValueError: AES key must be either 16, 24, or 32 bytes long,且错误信息不包含诊断信息。建议显式校验 len(self.INSTANCE_KEY) >= 24 并在不满足时提前抛出明确错误。
|
|
||
| :return: 脱敏后的凭证内容(password 替换为星号) | ||
| """ | ||
| self.data["password"] = "*********" |
There was a problem hiding this comment.
display_value() 直接修改 self.data 内部状态:调用后 self.data['password'] 永久变为星号,后续调用 value() 返回的数据已被污染。建议改为 return {**self.data, 'password': '*********'} 避免副作用。bk_access_token.py 和 bk_app.py 存在相同问题。
| def to_representation(self, instance): | ||
| data = super().to_representation(instance) | ||
| credential = CredentialDispatcher(credential_type=instance.type, data=instance.content) | ||
| if credential: |
There was a problem hiding this comment.
CredentialDispatcher.__init__ 在类型不支持时抛出 CredentialTypeNotSupport 异常,不会返回 None,因此 if credential 永远为真,else: data['content'] = {} 永不执行。建议改用 try/except CredentialTypeNotSupport 来处理不支持的类型。
| 更新凭证作用域 | ||
| """ | ||
| # 创建凭证作用域 | ||
| if scope_level == CredentialScopeLevel.PART.value: |
There was a problem hiding this comment.
PART 更新为 ALL 或 NONE 时,update_scopes 仅在 scope_level == PART 时删除旧记录,导致 CredentialScope 产生孤儿数据并使 can_use_in_scope() 判断异常。建议在 update=True 时无条件执行 CredentialScope.objects.filter(credential_id=credential.id).delete()。
There was a problem hiding this comment.
代码审查总结
本次 PR 对凭证管理系统进行了全面重构,新增多凭证类型支持、AES 加密存储、作用域控制及 API 更新接口。整体结构清晰,测试覆盖良好。
✅ 已解决问题
上一轮审查指出的 4 个问题(语法错误、异常捕获过宽、事务完整性)均已在此次提交中修复。
🔒 安全问题(需修复)
-
bkflow/utils/crypt.py:30— 固定 IV
ROOT_IV全局复用,AES-CFB 模式下相同明文每次产生相同密文,存在语义安全缺陷及多块 XOR 分析风险。请每次加密随机生成 IV 并附加在密文头。 -
bkflow/utils/crypt.py:63— key 截断无校验
self.INSTANCE_KEY[:24].encode()若原始 key 不足 24 字节会产生非法 AES key,运行时抛ValueError且诊断信息模糊。请提前显式校验 key 长度。
⚠️ 逻辑问题(需修复)
-
bkflow/space/credential/basic_auth.py:59(及 bk_app.py、bk_access_token.py)
display_value()直接修改self.data,后续调用value()返回已脱敏数据,导致凭证数据被污染。请改为返回副本。 -
bkflow/apigw/serializers/credential.py:35— 死代码分支
CredentialDispatcher()失败时抛异常而非返回None,else分支永不执行。请改用try/except CredentialTypeNotSupport处理。 -
bkflow/space/views.py:129— scope_level 切换时孤儿记录
从PART切换为ALL/NONE时旧CredentialScope记录不会被删除,导致数据残留及作用域判断异常。请在update=True时无条件清理旧作用域。
There was a problem hiding this comment.
代码审查总结
本次 PR 在上一轮基础上进行了大量修复,整体质量有明显提升。以下是当前状态:
✅ 已解决的先前问题
views.py partial_update双重save()— 已重构,content分支与其他字段各自走独立保存路径,不再双写update_credential.py事务保护 — 已使用transaction.atomic()包裹- 序列化器异常捕获过宽 — 已改为捕获具体类型
CredentialTypeNotSupport create_credential.py不完整代码 — 已完整实现
🔒 安全问题(仍未修复)
bkflow/utils/crypt.py:32 — ROOT_IV 固定值在所有加密操作中复用,AES-CFB 模式下相同 key+IV 加密相同明文得到相同密文,缺乏语义安全性。建议每次 encrypt() 调用时用 os.urandom(16) 生成随机 IV,并拼接到密文头部。
bkflow/utils/crypt.py:63 — self.INSTANCE_KEY[:24].encode() 若 PRIVATE_SECRET 为空或长度不足 24 字节,AES 初始化会抛 ValueError: AES key must be either 16, 24, or 32 bytes long,且错误信息无诊断信息。建议在 __init__ 中显式校验 key 长度。
⚠️ 逻辑问题(仍未修复)
bkflow/space/credential/basic_auth.py:56(及 bk_access_token.py、bk_app.py)— display_value() 直接执行 self.data["password"] = "*********",永久修改内部状态。调用后再调用 value() 返回的也是脱敏值,原始凭证数据丢失。建议改为 return {**self.data, "password": "*********"}。
bkflow/apigw/serializers/credential.py:35 — CredentialDispatcher() 构造失败时抛 CredentialTypeNotSupport 异常,不会返回 None,所以 else: data["content"] = {} 永远不执行,属于死代码。建议改为 try/except CredentialTypeNotSupport。
bkflow/space/views.py:109 — update_scopes() 仅在 scope_level == PART 时删除旧记录。从 PART 切换为 ALL 或 NONE 时,旧的 CredentialScope 记录不会被清除,形成孤儿数据并可能导致 can_use_in_scope() 行为异常。建议在 update=True 时无条件先删除旧作用域。
⚠️ 新发现问题
bkflow/apigw/serializers/credential.py:79 — UpdateCredentialSerializer 中 scope_level 设置了 default=CredentialScopeLevel.NONE.value。在 partial=True 的 PATCH 请求中,若调用方未传该字段,序列化器会自动填充 NONE,导致已配置好的作用域被静默重置为"不开放"。建议去掉该 default,仅保留 required=False。
bkflow/space/credential/resolver.py:58 — str(value).isdigit() 会将纯数字字符串(如 "123456")误判为凭证 ID,导致将直接密码值当作凭证引用去数据库查询。建议使用 isinstance(value, int) 区分凭证 ID 引用与字符串直接值。
整体评价:本次 PR 完成了凭证管理重构的主要功能,新增多类型支持、AES 加密存储、作用域控制,测试覆盖较充分。加密安全性(固定 IV)和 display_value() 状态污染是相对重要的问题,建议修复后合并。
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #548 +/- ##
=========================================
Coverage ? 81.32%
=========================================
Files ? 246
Lines ? 13910
Branches ? 0
=========================================
Hits ? 11312
Misses ? 2598
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.