[Vault] 凭证存库信封加密与本地 KEK 轮换 - #199
Merged
Merged
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds vault master-key configuration, AES-256-GCM envelope encryption, encrypted credential persistence, secret-aware API and UI flows, key rotation, and session-scoped MCP static bearer injection. ChangesVault security
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
qifanlili
changed the title
qifanlili
marked this pull request as ready for review
August 3, 2026 10:41
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
docs/design/be/vault-runtime.md (1)
57-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the
KeyProvidersketch and the key-version wording with the implementation.Two small drifts exist between this document and
internal/secrets/provider.go:
- The sketch omits
Name() string. The implemented interface has it, andService.Openuses it to reject envelopes sealed by another provider.- Line 41 states that the ciphertext header carries the key version. The implementation persists
key_versionas a separate envelope column, andaadBytesbinds only the format version.📝 Proposed doc fix
```go type KeyProvider interface { + Name() string Prepare(ctx) error WrapDEK(...) (wrappedDEK, error) UnwrapDEK(...) (dek, error) }</details> As per coding guidelines: "Synchronize implementation and design documentation when externally observable behavior or important architectural and compatibility contracts change". <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/design/be/vault-runtime.mdaround lines 57 - 63, Update the KeyProvider
interface sketch to include Name() string, matching internal/secrets/provider.go
and Service.Open provider validation. Revise the key-version description near
line 41 to state that key_version is stored as a separate envelope column, while
aadBytes binds only the format version; remove the claim that the ciphertext
header carries the key version.</details> <!-- cr-comment:v1:e5d30d5a111ed1842a719c24 --> _Source: Coding guidelines_ </blockquote></details> <details> <summary>internal/secrets/secrets_test.go (1)</summary><blockquote> `251-266`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Add cases for the remaining `decrypt_only` validation branches.** `TestNewLocalKeyProviderRejectsVersionCollision` covers only the current-version collision. Two validation branches in `NewLocalKeyProvider` stay untested: a `decrypt_only` version below 1, and a duplicate version within `decrypt_only` itself. <details> <summary>💚 Proposed additional cases</summary> ```diff if _, err := secrets.NewLocalKeyProvider( secrets.LocalKeyMaterial{Version: 2, KEK: kek}, []secrets.LocalKeyMaterial{{Version: 2, KEK: other}}, ); err == nil { t.Fatal("decrypt_only colliding with current version must fail") } + if _, err := secrets.NewLocalKeyProvider( + secrets.LocalKeyMaterial{Version: 2, KEK: kek}, + []secrets.LocalKeyMaterial{{Version: 0, KEK: other}}, + ); err == nil { + t.Fatal("decrypt_only version below 1 must fail") + } + if _, err := secrets.NewLocalKeyProvider( + secrets.LocalKeyMaterial{Version: 3, KEK: kek}, + []secrets.LocalKeyMaterial{{Version: 1, KEK: other}, {Version: 1, KEK: other}}, + ); err == nil { + t.Fatal("duplicated decrypt_only version must fail") + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/secrets/secrets_test.go` around lines 251 - 266, Extend TestNewLocalKeyProviderRejectsVersionCollision with cases covering NewLocalKeyProvider rejecting a decrypt_only LocalKeyMaterial whose Version is below 1 and rejecting duplicate versions among decrypt_only entries. Generate valid KEKs for each case, assert an error is returned, and keep the existing current-version collision assertion intact.internal/secrets/kek_test.go (1)
13-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth new secrets test files declare success scenarios before failure scenarios. The shared root cause is that the required ordering convention was not applied when the two test files were created.
internal/secrets/kek_test.go#L13-L73: moveTestResolveKEKErrorsaboveTestResolveKEKFromBase64andTestResolveKEKFromFile.internal/secrets/secrets_test.go#L34-L64: move the failure tests (TestOpenTamperFailsClosed,TestOpenAADBindingMismatchFails,TestOpenRejectsUnknownFormat,TestOpenRejectsProviderMismatch,TestOpenRejectsUnsupportedKeyVersion) aboveTestSealOpenRoundTripandTestSealAcrossBlockBoundary.As per coding guidelines: "Write failure scenarios before success scenarios".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/secrets/kek_test.go` around lines 13 - 73, Reorder the tests to place failure scenarios before success scenarios: in internal/secrets/kek_test.go (lines 13-73), move TestResolveKEKErrors above TestResolveKEKFromBase64 and TestResolveKEKFromFile; in internal/secrets/secrets_test.go (lines 34-64), move TestOpenTamperFailsClosed, TestOpenAADBindingMismatchFails, TestOpenRejectsUnknownFormat, TestOpenRejectsProviderMismatch, and TestOpenRejectsUnsupportedKeyVersion above TestSealOpenRoundTrip and TestSealAcrossBlockBoundary. Make no behavioral changes.Source: Coding guidelines
internal/db/vaults_sqlx_test.go (1)
88-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the nil-envelope branch.
setVaultCredentialEnvelopeArgumentsnow has two branches. This test covers only the populated envelope. The nil branch must still supply all six argument keys, otherwisebindNamedfails at runtime for archived or envelope-less credentials. Add the failure/edge case before the success case, as required by the test guidelines.💚 Proposed test addition
func TestVaultCredentialArgumentsWithoutEnvelope(t *testing.T) { arguments := vaultCredentialArguments(VaultCredential{ Metadata: []byte(`{}`), Auth: []byte(`{}`), }) for _, field := range []string{"ciphertext", "nonce", "wrapped_dek", "format_version", "key_provider", "key_version"} { value, ok := arguments[field] if !ok { t.Fatalf("%s argument is missing", field) } if value != nil { t.Fatalf("%s argument = %#v, want nil", field, value) } } }As per coding guidelines: "Write failure scenarios before success scenarios".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/vaults_sqlx_test.go` around lines 88 - 113, Add a nil-envelope test before TestVaultCredentialArgumentsPreserveJSONBoundaries, invoking vaultCredentialArguments with no SecretEnvelope and asserting ciphertext, nonce, wrapped_dek, format_version, key_provider, and key_version are all present with nil values. Keep the existing populated-envelope success test unchanged.Source: Coding guidelines
tests/vaults_api_test.go (1)
654-668: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that every envelope column is cleared.
vaultCredentialClearSecretsSQLnulls six columns:ciphertext,nonce,wrapped_dek,format_version,key_provider, andkey_version. This assertion checks onlyciphertext. A partial regression in the cleanup SQL would leavewrapped_dekornonceat rest and the test would still pass.💚 Proposed change
from vault_credentials where vault_external_id = $1 - and ciphertext is not null + and ( + ciphertext is not null + or nonce is not null + or wrapped_dek is not null + or format_version is not null + or key_provider is not null + or key_version is not null + ) `, vaultID).Scan(&count); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/vaults_api_test.go` around lines 654 - 668, Update assertVaultSecretsPurged to verify that all six envelope columns—ciphertext, nonce, wrapped_dek, format_version, key_provider, and key_version—are NULL for the vault’s credentials, rather than checking only ciphertext. Keep the existing failure behavior while making the query detect any remaining non-null envelope value.internal/vaults/handler.go (1)
783-788: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDistinguish an archived credential from a lost envelope.
ArchiveVaultCredentialclears the envelope columns. A validate request on an archivedmcp_oauthcredential therefore reachesOpenCredentialSecretwith a nil envelope and returns 400 "vault credential secret is missing; resubmit the secret". That message tells the client to resubmit a secret for a credential that is archived. Checkcredential.ArchivedAtfirst and return an archived-credential error.♻️ Proposed change
+ if credential.ArchivedAt != nil { + httpapi.WriteError(w, r, httpapi.NewError(http.StatusBadRequest, "invalid_request_error", "Credential is archived")) + return + } // Decrypt transiently to inspect whether a refresh token is present. The // plaintext is not persisted or logged. if err := OpenCredentialSecret(r.Context(), h.secretSvc, &credential); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/vaults/handler.go` around lines 783 - 788, In the validation flow before calling OpenCredentialSecret, check credential.ArchivedAt and return the established archived-credential error when the credential is archived. Keep OpenCredentialSecret and its existing lost-envelope error handling for non-archived credentials.tests/vaults_encryption_test.go (1)
18-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrder the failure tests before the success tests.
The file starts with the success round-trip test. The failure tests (
TestVaultCredentialUpdateMissingEnvelopeReturns400,TestVaultCredentialUpdateOpenFailureReturns5xx,TestVaultCredentialBackfillEndpointGone) appear later. The test guidelines require failure scenarios first.As per coding guidelines: "Write failure scenarios before success scenarios".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/vaults_encryption_test.go` around lines 18 - 30, Reorder the tests in tests/vaults_encryption_test.go so the failure scenarios TestVaultCredentialUpdateMissingEnvelopeReturns400, TestVaultCredentialUpdateOpenFailureReturns5xx, and TestVaultCredentialBackfillEndpointGone appear before the successful TestVaultCredentialEncryptedAtRest round-trip test, without changing their implementations.Source: Coding guidelines
internal/db/vaults.go (1)
264-276: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueForce
versionto 0 on insert.The insert passes
:versionfromcredential.SecretVersion. Callers set this field only through reads. A caller that reuses a credential struct read from the database inserts a non-zero starting version. The row version is an internal concurrency counter, so the database layer should own its initial value.♻️ Proposed change
- credential.VaultUUID = vaultUUID.String() + credential.VaultUUID = vaultUUID.String() + credential.SecretVersion = 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/vaults.go` around lines 264 - 276, Update the credential insert statement in the relevant vault persistence method to insert a literal initial version of 0 instead of binding :version from credential.SecretVersion. Keep version updates for existing records unchanged so the database layer owns the counter’s initial value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/configuration-reference.yaml`:
- Around line 106-115: Update the decrypt_only configuration example to be an
empty list by default and comment out or remove the placeholder entry, including
its usable all-zero kek and version 2 value. Preserve the surrounding guidance
that prior KEK versions are added only after rotation, ensuring no decrypt-only
version exceeds the current version: setting.
In `@internal/config/types.go`:
- Around line 33-46: Update the MasterKeyConfig comment to state that at most
one of Kek or KekFile may be set, while preserving the existing description of
KekFile and the dev fallback behavior. Do not change validateVaultMasterKey or
configuration logic.
In `@internal/db/vaults.go`:
- Around line 306-348: Update the no-rows fallback in UpdateVaultCredential
after GetVaultCredential succeeds: keep ErrNotFound for missing or archived
credentials, but return ErrVersionConflict for every active credential instead
of checking current.SecretVersion and falling through to ErrNotFound. Remove the
stale-version equality branch around current and preserve existing getErr
handling.
In `@internal/secrets/service.go`:
- Around line 59-97: Validate the binding fields before creating an envelope:
reject empty ExternalID or VaultExternalID in the SealCredentialSecret flow, or
enforce the same validation at Service.Seal before generating the DEK. Ensure
invalid VaultCredential bindings return an error and never proceed to envelope
creation, while preserving valid credential sealing behavior.
In `@internal/vaults/handler.go`:
- Around line 643-680: Update vault-runtime.md to document the POST credential
update response contracts for HTTP 409 version conflicts and metadata-only
updates when the credential secret envelope is absent, including the expected
behavior and response details. Preserve the existing 400 missing-envelope
resubmission documentation and align the new entries with the implemented update
flow around normalizeCredentialAuthForUpdate and SecretEnvelope handling.
In `@tests/vaults_encryption_test.go`:
- Around line 125-129: Update the stale-version update assertion in the CAS test
to verify that the returned error matches db.ErrVersionConflict using errors.Is,
rather than only checking for a non-nil error; add the errors and internal/db
imports required for this specific assertion.
---
Nitpick comments:
In `@docs/design/be/vault-runtime.md`:
- Around line 57-63: Update the KeyProvider interface sketch to include Name()
string, matching internal/secrets/provider.go and Service.Open provider
validation. Revise the key-version description near line 41 to state that
key_version is stored as a separate envelope column, while aadBytes binds only
the format version; remove the claim that the ciphertext header carries the key
version.
In `@internal/db/vaults_sqlx_test.go`:
- Around line 88-113: Add a nil-envelope test before
TestVaultCredentialArgumentsPreserveJSONBoundaries, invoking
vaultCredentialArguments with no SecretEnvelope and asserting ciphertext, nonce,
wrapped_dek, format_version, key_provider, and key_version are all present with
nil values. Keep the existing populated-envelope success test unchanged.
In `@internal/db/vaults.go`:
- Around line 264-276: Update the credential insert statement in the relevant
vault persistence method to insert a literal initial version of 0 instead of
binding :version from credential.SecretVersion. Keep version updates for
existing records unchanged so the database layer owns the counter’s initial
value.
In `@internal/secrets/kek_test.go`:
- Around line 13-73: Reorder the tests to place failure scenarios before success
scenarios: in internal/secrets/kek_test.go (lines 13-73), move
TestResolveKEKErrors above TestResolveKEKFromBase64 and TestResolveKEKFromFile;
in internal/secrets/secrets_test.go (lines 34-64), move
TestOpenTamperFailsClosed, TestOpenAADBindingMismatchFails,
TestOpenRejectsUnknownFormat, TestOpenRejectsProviderMismatch, and
TestOpenRejectsUnsupportedKeyVersion above TestSealOpenRoundTrip and
TestSealAcrossBlockBoundary. Make no behavioral changes.
In `@internal/secrets/secrets_test.go`:
- Around line 251-266: Extend TestNewLocalKeyProviderRejectsVersionCollision
with cases covering NewLocalKeyProvider rejecting a decrypt_only
LocalKeyMaterial whose Version is below 1 and rejecting duplicate versions among
decrypt_only entries. Generate valid KEKs for each case, assert an error is
returned, and keep the existing current-version collision assertion intact.
In `@internal/vaults/handler.go`:
- Around line 783-788: In the validation flow before calling
OpenCredentialSecret, check credential.ArchivedAt and return the established
archived-credential error when the credential is archived. Keep
OpenCredentialSecret and its existing lost-envelope error handling for
non-archived credentials.
In `@tests/vaults_api_test.go`:
- Around line 654-668: Update assertVaultSecretsPurged to verify that all six
envelope columns—ciphertext, nonce, wrapped_dek, format_version, key_provider,
and key_version—are NULL for the vault’s credentials, rather than checking only
ciphertext. Keep the existing failure behavior while making the query detect any
remaining non-null envelope value.
In `@tests/vaults_encryption_test.go`:
- Around line 18-30: Reorder the tests in tests/vaults_encryption_test.go so the
failure scenarios TestVaultCredentialUpdateMissingEnvelopeReturns400,
TestVaultCredentialUpdateOpenFailureReturns5xx, and
TestVaultCredentialBackfillEndpointGone appear before the successful
TestVaultCredentialEncryptedAtRest round-trip test, without changing their
implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 188ab2d3-bf4c-445b-a54e-fd2ad13a9dc9
📒 Files selected for processing (23)
docs/configuration-reference.yamldocs/design/be/vault-runtime.mdinternal/api/platform_mcp_vault_auth.gointernal/api/server.gointernal/config/config.gointernal/config/config_test.gointernal/config/types.gointernal/config/yaml.gointernal/config/yaml_types.gointernal/db/migrations/00047_add_vault_secret_envelope.sqlinternal/db/vaults.gointernal/db/vaults_sqlx_test.gointernal/secrets/kek.gointernal/secrets/kek_test.gointernal/secrets/provider.gointernal/secrets/secrets_test.gointernal/secrets/service.gointernal/vaults/handler.gomain.gotests/files_api_test.gotests/platform_console_backend_api_test.gotests/vaults_api_test.gotests/vaults_encryption_test.go
![duckpr[bot]](https://avatars.githubusercontent.com/in/4166808?s=60&v=4){kind=small}
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
Important
加密实现本身是正确且 fail-closed 的（篡改密文/nonce/wrapped_dek/AAD/key_version/key_provider 均验证会失败，无明文回退），多租户作用域与 OAuth callback 的租户来源也经独立视角确认无误。两个值得合并前处理的运维隐患：dev 临时 KEK 每次重启静默作废所有旧信封，且作废后 Open 返回 5xx 而非 400，不触发 resubmit 引导；"active credential 必须有信封"的不变量没有 DB 级或集中式应用级守卫，当前仅靠每条写路径恰好都提供 secret。
Reviewed changes — 本 PR 为 Vault 凭证引入 AES-256-GCM 信封加密（一次性 DEK + 本地 KEK，current/decrypt_only 轮换无 rewrap），并做 direct cutover migration 删除明文 secret_payload 列。
- 新增
internal/secrets—Service.Seal/Open（AAD 绑 organization/workspace/vault/credential + format_version）、LocalKeyProvider（按key_version在 current∪decrypt_only 选钥，未知版本 fail closed）、ResolveKEK/GenerateKEK 00047_add_vault_secret_envelope.sql— 加信封列（ciphertext/nonce/wrapped_dek/format_version/key_provider/key_version/version）并 dropsecret_payload；无 Expand/Backfillinternal/db/vaults.go— envelope 列映射、vaultCredentialClearSecretsSQL（archive 清空）、UpdateVaultCredentialCAS（version = version + 1 where version = :expected_version，ErrNotFound 后重查区分 archived vs version mismatch）internal/vaults/handler.go—SealCredentialSecret/OpenCredentialSecret、update 流程 Open→merge→reseal、缺信封 + 无替换 secret → 400、Open 失败 → 5xx；authUpdateProvidesSecretReplacement按 authType 判定main.gobuildVaultSecretsService— prod 必须配置 KEK；dev 回退进程内临时 KEKinternal/api/platform_mcp_vault_auth.go— OAuth callback 在 create 前 seal
⚠️ Direct cutover 的前向数据丢失缺少运维可见的升级指引
00047_add_vault_secret_envelope.sql 在同一次 migration 里加信封列并 drop secret_payload，既有明文随列丢弃、不 backfill。升级后所有既有 active credential 的信封列均为 NULL，update/validate 会返回 400 引导客户端重新提交 secret——这是设计意图，且修复路径（authUpdateProvidesSecretReplacement + 测试）已实现。
但升级须知目前只写在 docs/design/be/vault-runtime.md，运维真正会看的 migration header 和 docs/configuration-reference.yaml 的 vault: 块都没有"既有凭证需重新录入"的告警。Down migration 只回滚 schema、不回滚数据（无法回滚），这一点也未在 migration 注释里说明。建议至少在 migration header 与配置参考里补一句升级前的运维准备，避免升级后才在 400 响应里发现数据已丢。设计本身合理，仅是文档落点。
ℹ️ Nitpicks
internal/secrets/service.go:64,102的defer wipe(dek)在 Go 的 GC/escape 语义下是 best-effort，且LocalKeyProvider.keys里的 KEK 进程生命周期内不擦除。这是纯 Go 的已知限制，threat model 也把"打进 OMA 进程"列为接受缺口；仅提醒Servicedoc comment 里"materialize a DEK only for the duration of one operation and wipe it before returning"略高于实际保证。无需改动。
anthropic/glm-5.2 ｜ 𝕏
Comment on lines
+182
to
+188
| case cfg.Env == config.EnvironmentDev: | ||
| generated, err := secrets.GenerateKEK() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| kek = generated | ||
| logger.Warn("vault master key not configured; using process-scoped ephemeral KEK (sealed secrets will not survive a restart)") |
There was a problem hiding this comment.
OpenCredentialSecret 对“信封存在但 Open 失败”走 writeSecretOpenError 的 5xx 分支（handler.go writeSecretOpenError 只把 ErrMissingSecretEnvelope 映射成 400），所以即便客户端想修复，update/validate 也只返回 500 而不是引导重新提交 secret 的 400——resubmit 修复路径在这种场景下不会自动触发。
Technical details
# dev 临时 KEK 重启作废后无法自动恢复
## Affected sites
- main.go:182-188 — `GenerateKEK()` 每次启动产生新 KEK，旧 KEK 不进 `decrypt_only`
- internal/vaults/handler.go:184-191 — `writeSecretOpenError` 仅 `ErrMissingSecretEnvelope` → 400，其余 Open 错误 → 500
- internal/secrets/service.go:91-116 — Open 对不匹配 KEK 的 wrapped_dek 返回错误（非 ErrMissingSecretEnvelope）
## Required outcome
- dev 重启后，被作废的信封要么能在下次 update 时通过 400 引导客户端重新提交 secret 修复（与缺信封一致的体验），要么启动时显式失败/自检，而不是静默 Warn 后等到首次 Open 才 500。
## Suggested approach
- 选项 A：dev 临时 KEK 启动时如果库里已存在 `ciphertext is not null` 的行，做一次 Open 自检或直接 fail-fast，把静默作废变成显式信号。
- 选项 B：在 dev/staging 把“KEK 不匹配导致的 Open 失败”也映射成 400 + resubmit 引导（风险：可能掩盖真正需要 5xx 的篡改/损坏场景，需按 provider 错误类型区分）。
## Open questions for the human
- dev 场景是否预期允许这种“重启即丢密钥”？如果 dev 也要求跨重启可解，应该默认拒绝临时 KEK 回退或强制写入 `kek_file`。
Comment on lines
+146
to
+148
| if len(credential.SecretPayload) == 0 || isJSONNull(credential.SecretPayload) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
ℹ️ SealCredentialSecret 在 SecretPayload 为空或 JSON null 时是 no-op，意味着“active credential 必须有信封”这个不变量没有被集中式守卫。当前所有 create 路径（static_bearer/environment_variable/mcp_oauth 以及 OAuth callback）恰好都强制要求 secret，所以实践上是安全的——但设计文档明确选择不用 PostgreSQL CHECK，而这层应用保证只靠每条写路径各自记得提供 secret。
Technical details
# active credential 必须有信封——应用级不变量未集中化
## Affected sites
- internal/vaults/handler.go:146-148 — 空 payload 时直接 return nil，不报错
- internal/api/platform_mcp_vault_auth.go:421-431 — OAuth callback 走同一 seal 路径，若 buildPlatformMCPVaultOAuthCredentialPayloads 返回空 secret 会静默写入无信封行
- docs/design/be/vault-runtime.md:86 — 显式声明不使用 CHECK、由应用写路径强制
## Required outcome
- 任何把 active credential 写入 DB 的路径，在 secret 为空时应当 fail-closed（报错而非静默 no-op），让“缺信封”只可能由 direct cutover 的历史遗留行产生，而不是新写入路径无意中产生。
## Suggested approach
- 在 SealCredentialSecret 区分“合法的 archive/clear 场景”与“应当有 secret 的 create/update 场景”：create/update 调用点期望非空 secret 时，空 payload 应返回错误。或在 CreateVaultCredential 入口对 active 行做一次 envelope 非 nil 断言。
- 不要重新引入 PostgreSQL CHECK（设计已明确排除）。
qifanlili
force-pushed
the
feat/vault-secret-envelope-encryption
branch
from
August 5, 2026 07:49
639b4e6 to
c18a287
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
docs/design/be/vault-runtime.md (1)
164-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Mermaid diagram for the proposed injection flow.
The flow includes session authentication, credential selection, decryption, authorization-header replacement, and redirect behavior. Add a sequence diagram that identifies the Sandbox, proxy, Vault, and upstream MCP server.
As per coding guidelines,
docs/design/**/*.mdmust prioritize Mermaid for complex flows, state machines, component/service dependencies, timing interactions, and data flows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/be/vault-runtime.md` around lines 164 - 173, Add a Mermaid sequence diagram to the proposed outbound MCP request flow near the existing numbered procedure, showing interactions among Sandbox, proxy, Vault, and the upstream MCP server for session authentication, credential and path/host matching, decryption, authorization replacement or passthrough, and forwarding. Include the fail-closed/non-match outcomes and cross-origin redirect behavior, while keeping the existing prose and matching rules consistent.Source: Coding guidelines
cmd/vault-unwrap-dek/main_test.go (1)
56-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOrder the unwrap failure test before the success test.
Move
TestVerifyUnwrapFailsWithoutMatchingKEKbeforeTestVerifyUnwrapSucceedsWithDecryptOnlyKEK. This keeps failure scenarios before success scenarios.As per coding guidelines, “Test organization order should write failure scenarios first, then success scenarios.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/vault-unwrap-dek/main_test.go` around lines 56 - 130, Reorder the test functions so TestVerifyUnwrapFailsWithoutMatchingKEK appears before TestVerifyUnwrapSucceedsWithDecryptOnlyKEK, without changing either test’s implementation or behavior.Source: Coding guidelines
cmd/vault-unwrap-dek/main.go (1)
41-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCreate and inject the CLI logger at startup.
runwrites terminal errors directly, andloadCredentialEnvelopecreates a logger inside a helper. Create the root logger inmain, callslog.SetDefaultbefore startup work, derive a component logger, and inject it into the database path. Return terminal errors tomainso it reports them once.As per coding guidelines, “Logger and handler must be created through
internal/loggingat the executable program assembly layer and callslog.SetDefaultearly in startup.”Also applies to: 342-348
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/vault-unwrap-dek/main.go` around lines 41 - 72, Create the root logger in main using internal/logging, call slog.SetDefault before invoking run, and derive a component logger for the CLI. Pass that logger through run into the database/envelope-loading path, including loadCredentialEnvelope, instead of constructing a helper logger there. Refactor run to return terminal errors without writing them, leaving main responsible for reporting each error once.Source: Coding guidelines
internal/db/vaults_mapper_test.go (1)
135-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe sensitive-flag assertion can pass without checking anything.
The loop skips argument names that are not in
wantSensitive. If a parameter is renamed, for exampleparams.Ciphertexttoparams.CipherText, no entry matches and the test still passes. This test is the guard that secret columns stay redacted in statement logs, so make a missing name fail.💚 Proposed change
+ seen := make(map[string]bool, len(wantSensitive)) for _, arg := range bound.Args { want, ok := wantSensitive[arg.Name] if !ok { continue } + seen[arg.Name] = true if arg.Sensitive != want { t.Fatalf("argument %q sensitive = %t, want %t", arg.Name, arg.Sensitive, want) } } + for name := range wantSensitive { + if !seen[name] { + t.Fatalf("argument %q not present in bound args", name) + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/vaults_mapper_test.go` around lines 135 - 143, Update the sensitive-flag assertion loop around wantSensitive so an argument name missing from the expected map fails the test instead of being skipped. Preserve the existing comparisons for known names, ensuring renamed or unexpected parameters cannot make the redaction test pass without validating every argument.internal/db/vault_credentials_mapper.go (1)
98-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider returning a struct instead of six values.
vaultCredentialSecretColumnsreturns six positional values. Both call sites ininternal/db/vaults.go(insertVaultCredentialParamsFromandupdateVaultCredentialParamsFrom) destructure the same six values in the same order. A small struct removes the ordering risk and reduces duplication.♻️ Proposed refactor
-func vaultCredentialSecretColumns(envelope *secrets.Envelope) (ciphertext, nonce, wrappedDEK []byte, formatVersion *int32, keyProvider *string, keyVersion *int64) { - if envelope == nil { - return nil, nil, nil, nil, nil, nil - } - version := int32(envelope.FormatVersion) - provider := envelope.KeyProvider - keyVer := envelope.KeyVersion - return append([]byte(nil), envelope.Ciphertext...), - append([]byte(nil), envelope.Nonce...), - append([]byte(nil), envelope.WrappedDEK...), - &version, - &provider, - &keyVer +type vaultCredentialSecretValues struct { + Ciphertext []byte + Nonce []byte + WrappedDEK []byte + FormatVersion *int32 + KeyProvider *string + KeyVersion *int64 +} + +func vaultCredentialSecretColumns(envelope *secrets.Envelope) vaultCredentialSecretValues { + if envelope == nil { + return vaultCredentialSecretValues{} + } + version := int32(envelope.FormatVersion) + provider := envelope.KeyProvider + keyVer := envelope.KeyVersion + return vaultCredentialSecretValues{ + Ciphertext: append([]byte(nil), envelope.Ciphertext...), + Nonce: append([]byte(nil), envelope.Nonce...), + WrappedDEK: append([]byte(nil), envelope.WrappedDEK...), + FormatVersion: &version, + KeyProvider: &provider, + KeyVersion: &keyVer, + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/vault_credentials_mapper.go` around lines 98 - 111, Refactor vaultCredentialSecretColumns to return a named struct containing the six secret-column fields instead of positional values, preserving the nil-envelope behavior and defensive byte copies. Update insertVaultCredentialParamsFrom and updateVaultCredentialParamsFrom to consume the struct fields rather than destructuring six return values, and remove any duplicated ordering assumptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/be/vault-runtime.md`:
- Line 35: Update the key-rotation statement in the vault runtime design to
match the documented behavior: local rotation retains each envelope’s original
wrapped DEK rather than rewrapping it, so the prior KEK remains required until
all affected envelopes are replaced. Ensure the wording does not promise
automatic DEK rewrapping.
In `@internal/db/migrations/00047_add_vault_secret_envelope.sql`:
- Around line 22-34: Make the Down migration for vault_credentials explicitly
irreversible instead of recreating secret_payload and dropping the replacement
columns. Replace its body with the migration framework’s supported explicit
failure mechanism, or document and implement a genuine recovery procedure that
restores deleted secret values.
In `@web/src/features/managed-agents/resources/model.tsx`:
- Around line 653-658: Update the credential form around credentialAuthBody so
Secret name is disabled when editing an existing credential, preventing users
from changing a field that update requests do not submit. Keep secretName
editable in create mode and preserve the existing update behavior for
secret_value.
---
Nitpick comments:
In `@cmd/vault-unwrap-dek/main_test.go`:
- Around line 56-130: Reorder the test functions so
TestVerifyUnwrapFailsWithoutMatchingKEK appears before
TestVerifyUnwrapSucceedsWithDecryptOnlyKEK, without changing either test’s
implementation or behavior.
In `@cmd/vault-unwrap-dek/main.go`:
- Around line 41-72: Create the root logger in main using internal/logging, call
slog.SetDefault before invoking run, and derive a component logger for the CLI.
Pass that logger through run into the database/envelope-loading path, including
loadCredentialEnvelope, instead of constructing a helper logger there. Refactor
run to return terminal errors without writing them, leaving main responsible for
reporting each error once.
In `@docs/design/be/vault-runtime.md`:
- Around line 164-173: Add a Mermaid sequence diagram to the proposed outbound
MCP request flow near the existing numbered procedure, showing interactions
among Sandbox, proxy, Vault, and the upstream MCP server for session
authentication, credential and path/host matching, decryption, authorization
replacement or passthrough, and forwarding. Include the fail-closed/non-match
outcomes and cross-origin redirect behavior, while keeping the existing prose
and matching rules consistent.
In `@internal/db/vault_credentials_mapper.go`:
- Around line 98-111: Refactor vaultCredentialSecretColumns to return a named
struct containing the six secret-column fields instead of positional values,
preserving the nil-envelope behavior and defensive byte copies. Update
insertVaultCredentialParamsFrom and updateVaultCredentialParamsFrom to consume
the struct fields rather than destructuring six return values, and remove any
duplicated ordering assumptions.
In `@internal/db/vaults_mapper_test.go`:
- Around line 135-143: Update the sensitive-flag assertion loop around
wantSensitive so an argument name missing from the expected map fails the test
instead of being skipped. Preserve the existing comparisons for known names,
ensuring renamed or unexpected parameters cannot make the redaction test pass
without validating every argument.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 71048a3b-1123-416f-921e-74eb5c146b3e
📒 Files selected for processing (34)
cmd/vault-unwrap-dek/main.gocmd/vault-unwrap-dek/main_test.godocs/configuration-reference.yamldocs/design/be/vault-runtime.mdinternal/api/platform_mcp_vault_auth.gointernal/api/server.gointernal/config/config.gointernal/config/config_test.gointernal/config/types.gointernal/config/yaml.gointernal/config/yaml_types.gointernal/db/migrations/00047_add_vault_secret_envelope.sqlinternal/db/vault_credentials_mapper.gointernal/db/vault_credentials_mapper.xmlinternal/db/vaults.gointernal/db/vaults_mapper.gointernal/db/vaults_mapper.xmlinternal/db/vaults_mapper_postgres_test.gointernal/db/vaults_mapper_test.gointernal/db/vaults_sqlx_test.gointernal/secrets/kek.gointernal/secrets/kek_test.gointernal/secrets/provider.gointernal/secrets/secrets_test.gointernal/secrets/service.gointernal/vaults/handler.gomain.gotests/files_api_test.gotests/platform_console_backend_api_test.gotests/vaults_api_test.gotests/vaults_encryption_test.goweb/src/features/managed-agents/api.tsweb/src/features/managed-agents/resources/dialogs.tsxweb/src/features/managed-agents/resources/model.tsx
💤 Files with no reviewable changes (1)
- internal/db/vaults_sqlx_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- docs/configuration-reference.yaml
- internal/config/yaml_types.go
- internal/config/yaml.go
- internal/api/platform_mcp_vault_auth.go
- internal/config/types.go
- internal/secrets/kek_test.go
- tests/platform_console_backend_api_test.go
- internal/secrets/kek.go
- internal/config/config_test.go
- tests/files_api_test.go
- internal/api/server.go
- main.go
- internal/secrets/provider.go
- internal/secrets/secrets_test.go
- internal/config/config.go
- internal/vaults/handler.go
qifanlili
force-pushed
the
feat/vault-secret-envelope-encryption
branch
2 times, most recently
from
August 5, 2026 09:24
22197d4 to
dfd19b6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
cmd/vault-unwrap-dek/main.go (1)
324-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInject the database logger from
main.
loadCredentialEnvelopecreates a runtime logger inside a dependency helper. Create the logger inmainand pass it throughrun,resolveEnvelope, andloadCredentialEnvelopetodb.Open.As per coding guidelines, Go files must “inject loggers from executable assembly through dependencies.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/vault-unwrap-dek/main.go` around lines 324 - 326, Move logger creation from loadCredentialEnvelope into main, then thread the logger through run, resolveEnvelope, and loadCredentialEnvelope, using the injected logger in db.Open. Remove the helper’s locally constructed logger while preserving the existing logger configuration and call flow.Source: Coding guidelines
internal/db/vaults_mapper_test.go (2)
162-168: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that the expected version is bound.
This test passes when SQL increments
versionbut does not useparams.ExpectedVersion. A regression that removes the CAS predicate can then admit lost updates without failing this test.Assert that
bound.Argscontainsparams.ExpectedVersionand that the generated update has a version predicate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/vaults_mapper_test.go` around lines 162 - 168, Update the test around buildVaultCredentialMapperUpdateByExternalID to assert that bound.Args includes params.ExpectedVersion and that bound.SQL contains the version comparison predicate, in addition to the existing SQL fragments, so removal of the CAS condition fails the test.
135-143: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the complete envelope persistence contract.
These tests check selected envelope fields only. They can pass after a mapper regression omits an expected sensitive argument or leaves envelope metadata after archival.
internal/db/vaults_mapper_test.go#L135-L143: track each key inwantSensitive, then fail if any expected argument was not emitted.internal/db/vaults_mapper_test.go#L213-L215: assert that archive SQL clearsciphertext,nonce,wrapped_dek,format_version,key_provider, andkey_version.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/vaults_mapper_test.go` around lines 135 - 143, Update internal/db/vaults_mapper_test.go at lines 135-143 to track every key in wantSensitive as its argument is validated, then fail if any expected key was not emitted; update lines 213-215 to assert the archive SQL clears ciphertext, nonce, wrapped_dek, format_version, key_provider, and key_version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/db/migrations/00049_add_vault_secret_envelope.sql`:
- Around line 27-37: Make the down migration for the vault_credentials envelope
schema explicitly irreversible instead of adding secret_payload and dropping the
envelope columns. Replace the destructive rollback statements with a deliberate
failure guard that prevents downgrade success and communicates that restoration
requires unavailable KEK access.
- Around line 13-23: Update the migration after adding the envelope columns and
before dropping secret_payload to archive every currently active
vault_credentials row whose envelope is incomplete: require ciphertext, nonce,
wrapped_dek, format_version, key_provider, and key_version to all be non-NULL,
and set archived_at for rows failing that check. Preserve already archived rows
and leave complete active envelopes unchanged.
In `@internal/db/vault_credentials_mapper.go`:
- Around line 104-127: Update requireCompleteSecretEnvelope to reject
FormatVersion values above the PostgreSQL INT maximum (1<<31-1), before
vaultCredentialSecretColumns converts envelope.FormatVersion to int32. Preserve
the existing lower-bound and other envelope validations.
In `@internal/secrets/secrets_test.go`:
- Around line 34-47: Reorder the tests in the secrets test file so the failure
and rejection cases, including TestSealRejectsIncompleteBinding and tamper
tests, appear before the successful TestSealOpenRoundTrip case. Keep each test’s
implementation unchanged.
In `@internal/vaults/seal_test.go`:
- Around line 13-25: Update the “failure empty payload” and “failure json null
payload” subtests around SealCredentialSecret to assert the returned error
matches “vault credential secret payload is required to seal.” Keep the nil
service setup if desired, but verify the exact payload-validation error so the
tests cannot pass solely because requireSecretsService rejects the nil service.
---
Nitpick comments:
In `@cmd/vault-unwrap-dek/main.go`:
- Around line 324-326: Move logger creation from loadCredentialEnvelope into
main, then thread the logger through run, resolveEnvelope, and
loadCredentialEnvelope, using the injected logger in db.Open. Remove the
helper’s locally constructed logger while preserving the existing logger
configuration and call flow.
In `@internal/db/vaults_mapper_test.go`:
- Around line 162-168: Update the test around
buildVaultCredentialMapperUpdateByExternalID to assert that bound.Args includes
params.ExpectedVersion and that bound.SQL contains the version comparison
predicate, in addition to the existing SQL fragments, so removal of the CAS
condition fails the test.
- Around line 135-143: Update internal/db/vaults_mapper_test.go at lines 135-143
to track every key in wantSensitive as its argument is validated, then fail if
any expected key was not emitted; update lines 213-215 to assert the archive SQL
clears ciphertext, nonce, wrapped_dek, format_version, key_provider, and
key_version.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 38eb299a-9834-4f2d-b637-51ff0fbdc9b1
📒 Files selected for processing (44)
cmd/vault-unwrap-dek/main.gocmd/vault-unwrap-dek/main_test.goconfig/config.example.yamldeploy/docker-compose/oma-server.yamldocs/configuration-reference.yamldocs/design/be/vault-runtime.mdinternal/api/platform_mcp_vault_auth.gointernal/api/server.gointernal/config/config.gointernal/config/config_test.gointernal/config/reference_test.gointernal/config/types.gointernal/config/yaml.gointernal/config/yaml_types.gointernal/db/db.gointernal/db/migrations/00049_add_vault_secret_envelope.sqlinternal/db/vault_credentials_envelope_test.gointernal/db/vault_credentials_mapper.gointernal/db/vault_credentials_mapper.xmlinternal/db/vaults.gointernal/db/vaults_mapper.gointernal/db/vaults_mapper.xmlinternal/db/vaults_mapper_postgres_test.gointernal/db/vaults_mapper_test.gointernal/db/vaults_sqlx_test.gointernal/secrets/kek.gointernal/secrets/kek_test.gointernal/secrets/provider.gointernal/secrets/secrets_test.gointernal/secrets/service.gointernal/vaults/handler.gointernal/vaults/seal_test.gojustfilemain.goscripts/generate-vault-kek.shscripts/tests/generate-vault-kek_test.shtests/files_api_test.gotests/platform_console_backend_api_test.gotests/uuid_boundary_postgres_test.gotests/vaults_api_test.gotests/vaults_encryption_test.goweb/src/features/managed-agents/api.tsweb/src/features/managed-agents/resources/dialogs.tsxweb/src/features/managed-agents/resources/model.tsx
💤 Files with no reviewable changes (1)
- internal/db/vaults_sqlx_test.go
🚧 Files skipped from review as they are similar to previous changes (24)
- internal/config/yaml_types.go
- tests/vaults_api_test.go
- tests/platform_console_backend_api_test.go
- web/src/features/managed-agents/resources/dialogs.tsx
- web/src/features/managed-agents/resources/model.tsx
- internal/config/yaml.go
- internal/db/vaults_mapper.go
- tests/files_api_test.go
- internal/secrets/provider.go
- internal/db/vaults_mapper.xml
- web/src/features/managed-agents/api.ts
- internal/api/server.go
- internal/config/types.go
- internal/secrets/kek.go
- internal/db/vaults_mapper_postgres_test.go
- internal/db/vaults.go
- docs/configuration-reference.yaml
- cmd/vault-unwrap-dek/main_test.go
- internal/db/vault_credentials_mapper.xml
- internal/config/config.go
- internal/api/platform_mcp_vault_auth.go
- internal/vaults/handler.go
- tests/vaults_encryption_test.go
- internal/secrets/kek_test.go
Comment on lines
+13
to
+23
| alter table vault_credentials | ||
| add column if not exists ciphertext bytea, | ||
| add column if not exists nonce bytea, | ||
| add column if not exists wrapped_dek bytea, | ||
| add column if not exists format_version int, | ||
| add column if not exists key_provider text, | ||
| add column if not exists key_version bigint, | ||
| add column if not exists version bigint not null default 0; | ||
|
|
||
| alter table vault_credentials | ||
| drop column if exists secret_payload; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Archive active credentials that have no complete envelope.
Existing plaintext-only rows keep archived_at IS NULL after this migration, but all envelope fields become NULL. The active list path will still expose them as usable credentials. The update path must fail closed when it cannot decrypt them.
Archive every active row with an incomplete envelope during the Up migration. This keeps the lifecycle state consistent and lets users recreate the credential.
Proposed migration change
alter table vault_credentials
drop column if exists secret_payload;
+
+update vault_credentials
+set
+ archived_at = now(),
+ updated_at = now()
+where archived_at is null
+ and (
+ ciphertext is null
+ or nonce is null
+ or wrapped_dek is null
+ or format_version is null
+ or key_provider is null
+ or key_version is null
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| alter table vault_credentials | |
| add column if not exists ciphertext bytea, | |
| add column if not exists nonce bytea, | |
| add column if not exists wrapped_dek bytea, | |
| add column if not exists format_version int, | |
| add column if not exists key_provider text, | |
| add column if not exists key_version bigint, | |
| add column if not exists version bigint not null default 0; | |
| alter table vault_credentials | |
| drop column if exists secret_payload; | |
| alter table vault_credentials | |
| add column if not exists ciphertext bytea, | |
| add column if not exists nonce bytea, | |
| add column if not exists wrapped_dek bytea, | |
| add column if not exists format_version int, | |
| add column if not exists key_provider text, | |
| add column if not exists key_version bigint, | |
| add column if not exists version bigint not null default 0; | |
| alter table vault_credentials | |
| drop column if exists secret_payload; | |
| update vault_credentials | |
| set | |
| archived_at = now(), | |
| updated_at = now() | |
| where archived_at is null | |
| and ( | |
| ciphertext is null | |
| or nonce is null | |
| or wrapped_dek is null | |
| or format_version is null | |
| or key_provider is null | |
| or key_version is null | |
| ); |
🧰 Tools
🪛 Squawk (2.61.0)
[warning] 23-23: Dropping a column may break existing clients.
(ban-drop-column)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/db/migrations/00049_add_vault_secret_envelope.sql` around lines 13 -
23, Update the migration after adding the envelope columns and before dropping
secret_payload to archive every currently active vault_credentials row whose
envelope is incomplete: require ciphertext, nonce, wrapped_dek, format_version,
key_provider, and key_version to all be non-NULL, and set archived_at for rows
failing that check. Preserve already archived rows and leave complete active
envelopes unchanged.
Comment on lines
+27
to
+37
| alter table vault_credentials | ||
| add column if not exists secret_payload jsonb; | ||
|
|
||
| alter table vault_credentials | ||
| drop column if exists version, | ||
| drop column if exists key_version, | ||
| drop column if exists key_provider, | ||
| drop column if exists format_version, | ||
| drop column if exists wrapped_dek, | ||
| drop column if exists nonce, | ||
| drop column if exists ciphertext; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent a silent destructive rollback.
The Down migration creates an empty secret_payload column and then deletes every envelope field. A rollback can therefore report success after permanently deleting secrets created under this schema.
Make this migration explicitly irreversible. A downgrade cannot restore plaintext because the migration has no KEK access.
Proposed rollback guard
-- +goose Down
-
-alter table vault_credentials
- add column if not exists secret_payload jsonb;
-
-alter table vault_credentials
- drop column if exists version,
- drop column if exists key_version,
- drop column if exists key_provider,
- drop column if exists format_version,
- drop column if exists wrapped_dek,
- drop column if exists nonce,
- drop column if exists ciphertext;
+do $$
+begin
+ raise exception 'migration 00049 is irreversible: vault credential plaintext cannot be restored';
+end
+$$;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| alter table vault_credentials | |
| add column if not exists secret_payload jsonb; | |
| alter table vault_credentials | |
| drop column if exists version, | |
| drop column if exists key_version, | |
| drop column if exists key_provider, | |
| drop column if exists format_version, | |
| drop column if exists wrapped_dek, | |
| drop column if exists nonce, | |
| drop column if exists ciphertext; | |
| alter table vault_credentials | |
| add column if not exists secret_payload jsonb; | |
| -- +goose Down | |
| do $$ | |
| begin | |
| raise exception 'migration 00049 is irreversible: vault credential plaintext cannot be restored'; | |
| end | |
| $$; |
🧰 Tools
🪛 Squawk (2.61.0)
[warning] 31-31: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 32-32: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 33-33: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 34-34: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 35-35: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 36-36: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 37-37: Dropping a column may break existing clients.
(ban-drop-column)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/db/migrations/00049_add_vault_secret_envelope.sql` around lines 27 -
37, Make the down migration for the vault_credentials envelope schema explicitly
irreversible instead of adding secret_payload and dropping the envelope columns.
Replace the destructive rollback statements with a deliberate failure guard that
prevents downgrade success and communicates that restoration requires
unavailable KEK access.
qifanlili
force-pushed
the
feat/vault-secret-envelope-encryption
branch
2 times, most recently
from
August 6, 2026 03:26
80ac83f to
ce965cc
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/db/vault_credential_mapper_test.go (1)
29-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a builder contract assertion for
UpdateByExternalID.The insert path is covered. The update statement changed more: it binds six new envelope columns, increments
version, and adds theversion = #{params.ExpectedVersion}predicate. No builder test pins that argument order or the CAS binding. A reordered parameter invault_credential_mapper.xmlwould bind ciphertext intononceand still compile.Add a second
assertMapperBuilderContractcase forvaultCredentialMapperUpdateByExternalIDStatementwith the expected argument names and aversion = $SQL fragment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/vault_credential_mapper_test.go` around lines 29 - 54, Add a second builder contract assertion alongside the existing insert case for vaultCredentialMapperUpdateByExternalIDStatement, invoking the corresponding update builder with representative params. Pin the complete expected argument order, including the six envelope fields, ExpectedVersion, and version increment bindings, and include the version = $ SQL fragment so reordered XML parameters fail the test.internal/db/vault_credential_mapper.go (1)
103-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the range check into
vaultCredentialSecretColumns.Line 107 narrows
envelope.FormatVersion(anint) toint32without a local bound check. Today both callers validate first:vaultCredentialInsertParamscallsrequireCompleteSecretEnvelope, andDB.UpdateVaultCredentialvalidates before callingvaultCredentialUpdateParams. The safety therefore depends on caller ordering, not on this function. A future caller that skips validation will silently persist a wrappedformat_version.Return an error from this helper, or call
requireCompleteSecretEnvelopeinside it for non-nil envelopes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/vault_credential_mapper.go` around lines 103 - 116, The vaultCredentialSecretColumns helper currently narrows envelope.FormatVersion without validating its range. Add requireCompleteSecretEnvelope validation inside vaultCredentialSecretColumns for non-nil envelopes, or change the helper to return and propagate an error, ensuring out-of-range versions cannot be converted to int32 and persisted; update vaultCredentialInsertParams and vaultCredentialUpdateParams as needed.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/be/vault-runtime.md`:
- Line 80: Update the nonce schema description in the vault runtime design to
use bytea without a length modifier, and state that the application validates
the nonce as exactly 12 bytes. Keep the documentation aligned with the migration
and do not introduce database CHECK-constraint enforcement.
In `@internal/vaults/handler.go`:
- Around line 655-665: Update the envelope-less mcp_oauth repair flow in
authUpdateProvidesSecretReplacement and its caller to keep refresh state
consistent: when current.Auth contains a public refresh object, require
refresh.refresh_token in the replacement body, or remove the public refresh
object before normalization so the credential remains access-token-only. Ensure
normalizeCredentialAuthForUpdate cannot persist refresh metadata without a
sealed refresh token.
In `@tests/vaults_encryption_test.go`:
- Around line 20-128: Reorder the test functions so
TestVaultCredentialUpdateMissingEnvelopeBehavior,
TestVaultCredentialUpdateOpenFailureReturns5xx, and
TestVaultCredentialBackfillEndpointGone appear before the success tests,
including TestVaultCredentialEncryptedAtRest. Keep every test body unchanged.
---
Nitpick comments:
In `@internal/db/vault_credential_mapper_test.go`:
- Around line 29-54: Add a second builder contract assertion alongside the
existing insert case for vaultCredentialMapperUpdateByExternalIDStatement,
invoking the corresponding update builder with representative params. Pin the
complete expected argument order, including the six envelope fields,
ExpectedVersion, and version increment bindings, and include the version = $ SQL
fragment so reordered XML parameters fail the test.
In `@internal/db/vault_credential_mapper.go`:
- Around line 103-116: The vaultCredentialSecretColumns helper currently narrows
envelope.FormatVersion without validating its range. Add
requireCompleteSecretEnvelope validation inside vaultCredentialSecretColumns for
non-nil envelopes, or change the helper to return and propagate an error,
ensuring out-of-range versions cannot be converted to int32 and persisted;
update vaultCredentialInsertParams and vaultCredentialUpdateParams as needed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b22fc8d-a99e-44c0-845f-575447726e3b
📒 Files selected for processing (36)
config/config.example.yamldeploy/docker-compose/oma-server.yamldocs/configuration-reference.yamldocs/design/be/vault-runtime.mdinternal/api/platform_mcp_vault_auth.gointernal/api/server.gointernal/config/config.gointernal/config/config_test.gointernal/config/reference_test.gointernal/config/types.gointernal/config/yaml.gointernal/config/yaml_types.gointernal/db/db.gointernal/db/migrations/00049_add_vault_secret_envelope.sqlinternal/db/vault_credential_envelope_test.gointernal/db/vault_credential_mapper.gointernal/db/vault_credential_mapper.xmlinternal/db/vault_credential_mapper_test.gointernal/db/vaults.gointernal/secrets/kek.gointernal/secrets/kek_test.gointernal/secrets/provider.gointernal/secrets/secrets_test.gointernal/secrets/service.gointernal/vaults/handler.gointernal/vaults/seal_test.gojustfilemain.gotests/files_api_test.gotests/platform_console_backend_api_test.gotests/uuid_boundary_postgres_test.gotests/vaults_api_test.gotests/vaults_encryption_test.goweb/src/features/managed-agents/api.tsweb/src/features/managed-agents/resources/dialogs.tsxweb/src/features/managed-agents/resources/model.tsx
🚧 Files skipped from review as they are similar to previous changes (25)
- web/src/features/managed-agents/api.ts
- internal/config/yaml.go
- internal/api/server.go
- justfile
- internal/db/db.go
- internal/vaults/seal_test.go
- internal/secrets/kek_test.go
- config/config.example.yaml
- deploy/docker-compose/oma-server.yaml
- internal/secrets/kek.go
- docs/configuration-reference.yaml
- internal/config/yaml_types.go
- internal/config/config.go
- main.go
- tests/platform_console_backend_api_test.go
- internal/api/platform_mcp_vault_auth.go
- internal/config/reference_test.go
- web/src/features/managed-agents/resources/dialogs.tsx
- tests/files_api_test.go
- internal/config/types.go
- internal/secrets/provider.go
- web/src/features/managed-agents/resources/model.tsx
- tests/uuid_boundary_postgres_test.go
- tests/vaults_api_test.go
- internal/config/config_test.go
|
|
||
| | 列 | 类型 | 说明 | | ||
| |---|---|---| | ||
| | `ciphertext` | bytea | AES-GCM 密文（含 tag） | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the nonce schema description with the migration.
bytea(12) implies schema-level length enforcement. The migration declares nonce bytea, and the design explicitly excludes database CHECK constraints. Document this as bytea with application-side validation of the 12-byte GCM nonce.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/be/vault-runtime.md` at line 80, Update the nonce schema
description in the vault runtime design to use bytea without a length modifier,
and state that the application validates the nonce as exactly 12 bytes. Keep the
documentation aligned with the migration and do not introduce database
CHECK-constraint enforcement.
Comment on lines
+20
to
+128
| func TestVaultCredentialEncryptedAtRest(t *testing.T) { | ||
| app := newTestAppWithStore(t, nil, newFakeStore("vaults-encryption-bucket")) | ||
| defer app.close() | ||
| vault := createVault(t, app, `{"display_name":"vault encryption at rest"}`) | ||
| defer cleanupVaultRows(t, app, vault.ID) | ||
|
|
||
| assertVaultCredentialsHaveNoSecretPayloadColumn(t, app) | ||
|
|
||
| created := createVaultCredential(t, app, vault.ID, staticBearerBody("encrypted", "https://mcp.roundtrip.example/sse", "round-trip-secret")) | ||
| if strings.Contains(string(created.Auth), "round-trip-secret") { | ||
| t.Fatalf("API response leaked the secret: %s", created.Auth) | ||
| } | ||
|
|
||
| env, binding := readVaultCredentialEnvelope(t, app, created.ID) | ||
| if len(env.Ciphertext) == 0 || len(env.Nonce) == 0 || len(env.WrappedDEK) == 0 { | ||
| t.Fatalf("envelope columns not populated: %+v", env) | ||
| } | ||
| opened, err := app.vaultSecrets.Open(context.Background(), binding, env) | ||
| if err != nil || !strings.Contains(string(opened), "round-trip-secret") { | ||
| t.Fatalf("open sealed credential: %v plaintext=%q", err, opened) | ||
| } | ||
| } | ||
|
|
||
| func TestVaultCredentialArchiveClearsEnvelope(t *testing.T) { | ||
| app := newTestAppWithStore(t, nil, newFakeStore("vaults-archive-bucket")) | ||
| defer app.close() | ||
| vault := createVault(t, app, `{"display_name":"vault archive envelope"}`) | ||
| defer cleanupVaultRows(t, app, vault.ID) | ||
| created := createVaultCredential(t, app, vault.ID, staticBearerBody("archive envelope", "https://mcp.archive-env.example/sse", "archive-secret")) | ||
| archiveVaultCredential(t, app, vault.ID, created.ID) | ||
| if vaultCredentialHasEnvelope(t, app, created.ID) { | ||
| t.Fatal("archived credential still carries envelope columns") | ||
| } | ||
| } | ||
|
|
||
| func TestVaultCredentialUpdateSecretVersionCAS(t *testing.T) { | ||
| app := newTestAppWithStore(t, nil, newFakeStore("vaults-cas-bucket")) | ||
| defer app.close() | ||
| ctx := context.Background() | ||
| vault := createVault(t, app, `{"display_name":"vault cas"}`) | ||
| defer cleanupVaultRows(t, app, vault.ID) | ||
| created := createVaultCredential(t, app, vault.ID, staticBearerBody("cas credential", "https://mcp.cas.example/sse", "cas-secret")) | ||
|
|
||
| workspaceUUID := defaultWorkspaceUUID(t, app) | ||
| current, err := app.db.GetVaultCredential(ctx, workspaceUUID, vault.ID, created.ID) | ||
| if err != nil { | ||
| t.Fatalf("get credential: %v", err) | ||
| } | ||
| stale := current | ||
| stale.DisplayName = "cas credential updated" | ||
| updated, err := app.db.UpdateVaultCredential(ctx, workspaceUUID, vault.ID, created.ID, stale) | ||
| if err != nil || updated.SecretVersion != 1 { | ||
| t.Fatalf("first update = (%+v, %v)", updated, err) | ||
| } | ||
| stale.DisplayName = "cas credential stale" | ||
| if _, err := app.db.UpdateVaultCredential(ctx, workspaceUUID, vault.ID, created.ID, stale); !errors.Is(err, db.ErrVersionConflict) { | ||
| t.Fatalf("stale-version update error = %v, want ErrVersionConflict", err) | ||
| } | ||
| } | ||
|
|
||
| func TestVaultCredentialUpdateMissingEnvelopeBehavior(t *testing.T) { | ||
| app := newTestAppWithStore(t, nil, newFakeStore("vaults-missing-envelope-bucket")) | ||
| defer app.close() | ||
| vault := createVault(t, app, `{"display_name":"vault missing envelope"}`) | ||
| defer cleanupVaultRows(t, app, vault.ID) | ||
|
|
||
| credentialID := insertEnvelopeLessCredential(t, app, vault.ID, "missing-envelope-key") | ||
| omitBody, _ := json.Marshal(map[string]any{"auth": map[string]any{"type": "static_bearer"}}) | ||
| omitResp := doVaultRequest(t, app, http.MethodPost, "/v1/vaults/"+vault.ID+"/credentials/"+credentialID+"?beta=true", bytes.NewReader(omitBody), defaultTestKey, true) | ||
| defer omitResp.Body.Close() | ||
| if omitResp.StatusCode != http.StatusBadRequest { | ||
| t.Fatalf("missing-envelope update status = %d, want 400: %s", omitResp.StatusCode, readAll(t, omitResp.Body)) | ||
| } | ||
|
|
||
| resealBody, _ := json.Marshal(map[string]any{ | ||
| "auth": map[string]any{"type": "static_bearer", "token": "replacement-token"}, | ||
| }) | ||
| resealResp := doVaultRequest(t, app, http.MethodPost, "/v1/vaults/"+vault.ID+"/credentials/"+credentialID+"?beta=true", bytes.NewReader(resealBody), defaultTestKey, true) | ||
| defer resealResp.Body.Close() | ||
| if resealResp.StatusCode != http.StatusOK || !vaultCredentialHasEnvelope(t, app, credentialID) { | ||
| t.Fatalf("missing-envelope reseal status = %d hasEnvelope=%v", resealResp.StatusCode, vaultCredentialHasEnvelope(t, app, credentialID)) | ||
| } | ||
| } | ||
|
|
||
| func TestVaultCredentialUpdateOpenFailureReturns5xx(t *testing.T) { | ||
| app := newTestAppWithStore(t, nil, newFakeStore("vaults-tamper-open-bucket")) | ||
| defer app.close() | ||
| vault := createVault(t, app, `{"display_name":"vault tamper open"}`) | ||
| defer cleanupVaultRows(t, app, vault.ID) | ||
| created := createVaultCredential(t, app, vault.ID, staticBearerBody("tamper open", "https://mcp.tamper.example/sse", "tamper-secret")) | ||
| tamperVaultCredentialCiphertext(t, app, created.ID) | ||
|
|
||
| body, _ := json.Marshal(map[string]any{"auth": map[string]any{"type": "static_bearer"}}) | ||
| resp := doVaultRequest(t, app, http.MethodPost, "/v1/vaults/"+vault.ID+"/credentials/"+created.ID+"?beta=true", bytes.NewReader(body), defaultTestKey, true) | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode < 500 { | ||
| t.Fatalf("tampered-envelope update status = %d, want 5xx: %s", resp.StatusCode, readAll(t, resp.Body)) | ||
| } | ||
| } | ||
|
|
||
| func TestVaultCredentialBackfillEndpointGone(t *testing.T) { | ||
| app := newTestAppWithStore(t, nil, newFakeStore("vaults-backfill-gone-bucket")) | ||
| defer app.close() | ||
| resp := doVaultRequest(t, app, http.MethodPost, "/v1/vaults/backfill_secrets?beta=true", nil, defaultTestKey, true) | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode != http.StatusNotFound { | ||
| t.Fatalf("backfill endpoint status = %d, want 404: %s", resp.StatusCode, readAll(t, resp.Body)) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Order the failure tests before the success tests.
Lines 20 and 43 define success scenarios. The failure scenarios follow at Lines 80, 104, and 120. Move TestVaultCredentialUpdateMissingEnvelopeBehavior, TestVaultCredentialUpdateOpenFailureReturns5xx, and TestVaultCredentialBackfillEndpointGone above TestVaultCredentialEncryptedAtRest. Keep each test body unchanged.
As per coding guidelines, "Order tests with failure scenarios before success scenarios."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/vaults_encryption_test.go` around lines 20 - 128, Reorder the test
functions so TestVaultCredentialUpdateMissingEnvelopeBehavior,
TestVaultCredentialUpdateOpenFailureReturns5xx, and
TestVaultCredentialBackfillEndpointGone appear before the success tests,
including TestVaultCredentialEncryptedAtRest. Keep every test body unchanged.
Source: Coding guidelines
qifanlili
force-pushed
the
feat/vault-secret-envelope-encryption
branch
from
August 6, 2026 03:35
ce965cc to
b1872fa
Compare
jh0904
approved these changes
Aug 6, 2026
|
|
||
| "github.com/superduck-ai/yourbatis" | ||
| "github.com/superduck-ai/open-managed-agents/internal/secrets" | ||
| yoursql "github.com/superduck-ai/yourbatis" |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/vaults/inject_match_test.go (1)
18-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOrder failure scenarios before success scenarios.
internal/vaults/inject_match_test.go#L18-L47: move mismatch and invalid-input cases before exact and prefix-match cases.internal/vaults/inject_match_test.go#L68-L111: move rejection cases before injection and passthrough cases.internal/vaults/inject_test.go#L14-L109: move missing-envelope and unavailable-secret-service cases before successful seal/open and passthrough cases.As per coding guidelines, "
**/*_test.go: Order tests with failure scenarios before success scenarios."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/vaults/inject_match_test.go` around lines 18 - 47, Reorder the table-driven test cases so failure and rejection scenarios appear before success scenarios: in internal/vaults/inject_match_test.go ranges 18-47 and 68-111, and internal/vaults/inject_test.go range 14-109. Preserve each case and assertion unchanged; only adjust ordering, placing mismatches, invalid inputs, missing envelopes, and unavailable services before injection, passthrough, seal/open, and other successful cases.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/db/decode_vault_ids_test.go`:
- Around line 14-18: Reorder the table-driven cases in
internal/db/decode_vault_ids_test.go:14-18 so the invalid JSON failure case
appears before successful decode cases; no other changes are needed there. In
internal/codesessions/mcp_proxy_test.go:190-242, move
TestMCPProxyVaultInjectionRejectedReturns502 before the success-path MCP proxy
tests.
In `@internal/vaults/inject.go`:
- Around line 50-57: Update CodeSessionVaultInjectionContext and
GetCodeSessionVaultInjectionContext to retain OrganizationUUID alongside
WorkspaceUUID, then pass both tenant UUIDs from the injection flow into
ListActiveVaultCredentialsForVaultIDs and the underlying vault/credential reads.
Ensure the lookup path no longer relies on workspace-only tenant scoping.
- Around line 61-73: Update the injection flow around DecideInjection to reject
any request whose URL scheme is not HTTPS before opening the credential or
setting the Authorization header. Preserve passthrough behavior for non-matching
requests, return ErrInjectionRejected for matching HTTP targets, and add a
regression test covering an http:// request.
---
Nitpick comments:
In `@internal/vaults/inject_match_test.go`:
- Around line 18-47: Reorder the table-driven test cases so failure and
rejection scenarios appear before success scenarios: in
internal/vaults/inject_match_test.go ranges 18-47 and 68-111, and
internal/vaults/inject_test.go range 14-109. Preserve each case and assertion
unchanged; only adjust ordering, placing mismatches, invalid inputs, missing
envelopes, and unavailable services before injection, passthrough, seal/open,
and other successful cases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a316de82-88fc-4b94-9431-a12ce6c70ad3
📒 Files selected for processing (50)
CONTEXT.mdconfig/config.example.yamldeploy/docker-compose/oma-server.yamldocs/configuration-reference.yamldocs/design/be/managed-agent-claude-code-permission-bridge.mddocs/design/be/vault-runtime.mdinternal/api/platform_mcp_vault_auth.gointernal/api/server.gointernal/codesessions/handler.gointernal/codesessions/mcp_proxy.gointernal/codesessions/mcp_proxy_test.gointernal/codesessions/upstream_proxy_mitm.gointernal/config/config.gointernal/config/config_test.gointernal/config/reference_test.gointernal/config/types.gointernal/config/yaml.gointernal/config/yaml_types.gointernal/db/code_session_mapper.gointernal/db/code_session_mapper.xmlinternal/db/code_sessions.gointernal/db/db.gointernal/db/decode_vault_ids_test.gointernal/db/migrations/00049_add_vault_secret_envelope.sqlinternal/db/vault_credential_envelope_test.gointernal/db/vault_credential_mapper.gointernal/db/vault_credential_mapper.xmlinternal/db/vault_credential_mapper_test.gointernal/db/vaults.gointernal/secrets/kek.gointernal/secrets/kek_test.gointernal/secrets/provider.gointernal/secrets/secrets_test.gointernal/secrets/service.gointernal/vaults/handler.gointernal/vaults/inject.gointernal/vaults/inject_match.gointernal/vaults/inject_match_test.gointernal/vaults/inject_test.gointernal/vaults/seal_test.gojustfilemain.gotests/files_api_test.gotests/platform_console_backend_api_test.gotests/uuid_boundary_postgres_test.gotests/vaults_api_test.gotests/vaults_encryption_test.goweb/src/features/managed-agents/api.tsweb/src/features/managed-agents/resources/dialogs.tsxweb/src/features/managed-agents/resources/model.tsx
🚧 Files skipped from review as they are similar to previous changes (31)
- internal/db/db.go
- config/config.example.yaml
- internal/config/yaml.go
- web/src/features/managed-agents/resources/dialogs.tsx
- deploy/docker-compose/oma-server.yaml
- internal/config/reference_test.go
- internal/vaults/seal_test.go
- internal/secrets/kek_test.go
- tests/uuid_boundary_postgres_test.go
- internal/api/platform_mcp_vault_auth.go
- main.go
- tests/platform_console_backend_api_test.go
- tests/files_api_test.go
- internal/secrets/kek.go
- justfile
- internal/config/yaml_types.go
- web/src/features/managed-agents/api.ts
- web/src/features/managed-agents/resources/model.tsx
- internal/db/vault_credential_envelope_test.go
- internal/db/vault_credential_mapper_test.go
- internal/api/server.go
- docs/configuration-reference.yaml
- internal/config/config_test.go
- internal/secrets/provider.go
- internal/secrets/secrets_test.go
- tests/vaults_api_test.go
- internal/config/config.go
- internal/config/types.go
- internal/db/vault_credential_mapper.xml
- tests/vaults_encryption_test.go
- internal/vaults/handler.go
Comment on lines
+14
to
+18
| {name: "empty", raw: ``, want: nil}, | ||
| {name: "empty array", raw: `[]`, want: []string{}}, | ||
| {name: "ordered ids", raw: `["vlt_a","vlt_b"]`, want: []string{"vlt_a", "vlt_b"}}, | ||
| {name: "trim blanks", raw: `[" vlt_a ",""]`, want: []string{"vlt_a"}}, | ||
| {name: "invalid json", raw: `{`, wantErr: true}, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Order failure scenarios before success scenarios.
internal/db/decode_vault_ids_test.go#L14-L18: move the invalid-JSON case before successful decode cases.internal/codesessions/mcp_proxy_test.go#L190-L242: placeTestMCPProxyVaultInjectionRejectedReturns502before success-path MCP proxy tests.
As per coding guidelines: “Order tests with failure scenarios before success scenarios.”
📍 Affects 2 files
internal/db/decode_vault_ids_test.go#L14-L18(this comment)internal/codesessions/mcp_proxy_test.go#L190-L242
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/db/decode_vault_ids_test.go` around lines 14 - 18, Reorder the
table-driven cases in internal/db/decode_vault_ids_test.go:14-18 so the invalid
JSON failure case appears before successful decode cases; no other changes are
needed there. In internal/codesessions/mcp_proxy_test.go:190-242, move
TestMCPProxyVaultInjectionRejectedReturns502 before the success-path MCP proxy
tests.
Source: Coding guidelines
Comment on lines
+50
to
+57
| record, err := i.db.GetCodeSessionVaultInjectionContext(ctx, codeSessionExternalID, organizationUUID, workspaceUUID) | ||
| if err != nil { | ||
| return fmt.Errorf("%w: load context: %w", ErrInjectionRejected, err) | ||
| } | ||
| if len(record.VaultIDs) == 0 { | ||
| return nil | ||
| } | ||
| credentials, err := i.db.ListActiveVaultCredentialsForVaultIDs(ctx, record.WorkspaceUUID, record.VaultIDs) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)internal/vaults/inject\.go$|vault.*context|CodeSessionVaultInjectionContext|ListActiveVaultCredentialsForVaultIDs' || true
echo "== inject outline =="
ast-grep outline internal/vaults/inject.go || true
echo "== inject relevant =="
cat -n internal/vaults/inject.go | sed -n '1,120p'
echo "== search symbols =="
rg -n "CodeSessionVaultInjectionContext|ListActiveVaultCredentialsForVaultIDs|GetCodeSessionVaultInjectionContext|OrganizationUUID|WorkspaceUUID|record\.VaultIDs" -S .Repository: superduck-ai/open-managed-agents
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== db package files likely containing vault DB methods =="
rg -l "GetCodeSessionVaultInjectionContext|ListActiveVaultCredentialsForVaultIDs|CodeSessionVaultInjectionContext|CreateVault|VaultCredential|VaultIDs" internal -g '*.go' | sed -n '1,80p'
echo "== relevant DB method definitions =="
rg -n "func \(.*db\)|GetCodeSessionVaultInjectionContext|ListActiveVaultCredentialsForVaultIDs|CodeSessionVaultInjectionContext|ListActiveVault" internal -g '*.go' -A 45 | sed -n '1,260p'
echo "== vault injection tests =="
cat -n internal/vaults/inject_test.go | sed -n '1,220p'Repository: superduck-ai/open-managed-agents
Length of output: 19041
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== mapper FindVaultInjectionContext =="
rg -n "FindVaultInjectionContext|vaultInjectionContext|code_session_vault" internal/db internal -g '*.go' -A 70 -B 20 | sed -n '1,240p'
echo "== GetVault and GetVaultCredential definitions and tests =="
rg -n "func \(d \*DB\) GetVault|func \(d \*DB\) GetVaultCredential|func \(d \*DB\) ListVaultCredentialsPage|VaultCredential \{" internal/db/vault_credential_mapper.go internal/db/vaults.go internal/db/code_sessions.go -A 50 -B 10 | sed -n '1,260p'
echo "== deterministic model/signature scan =="
python3 - <<'PY'
from pathlib import Path
import re
files = {
"internal/db/code_sessions.go": ["CodeSessionVaultInjectionContext", "GetCodeSessionVaultInjectionContext", "FindVaultInjectionContext"],
"internal/db/vaults.go": ["ListActiveVaultCredentialsForVaultIDs", "GetVault", "ListVaultCredentialsPage"],
}
for f, symbols in files.items():
text = Path(f).read_text()
print(f"\n## {f}")
for s in symbols:
idx = text.find(s)
if idx == -1:
print(f"MISSING {s}")
continue
start = text.rfind("\n", 0, max(0, idx-240))
end = text.find("\n}\n", idx)
print(f"### {s}")
print(text[start:end+5] if end != -1 else text[start:idx+240])
PYRepository: superduck-ai/open-managed-agents
Length of output: 41710
Include the injected credential org/workspace context.
GetCodeSessionVaultInjectionContext uses both tenant UUIDs, but CodeSessionVaultInjectionContext returns only WorkspaceUUID and ListActiveVaultCredentialsForVaultIDs then calls the workspace-only vault/credential reads. Keep OrganizationUUID in the returned context and update the vault lookup path to require both tenant UUIDs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/vaults/inject.go` around lines 50 - 57, Update
CodeSessionVaultInjectionContext and GetCodeSessionVaultInjectionContext to
retain OrganizationUUID alongside WorkspaceUUID, then pass both tenant UUIDs
from the injection flow into ListActiveVaultCredentialsForVaultIDs and the
underlying vault/credential reads. Ensure the lookup path no longer relies on
workspace-only tenant scoping.
Source: Coding guidelines
Comment on lines
+61
to
+73
| decision := DecideInjection(requestURL, credentials) | ||
| switch decision.Kind { | ||
| case InjectionPassthrough: | ||
| return nil | ||
| case InjectionReject: | ||
| return ErrInjectionRejected | ||
| case InjectionInject: | ||
| token, err := openStaticBearerToken(ctx, i.secretSvc, decision.Credential) | ||
| if err != nil { | ||
| return fmt.Errorf("%w: open credential: %w", ErrInjectionRejected, err) | ||
| } | ||
| header.Del("Authorization") | ||
| header.Set("Authorization", "Bearer "+token) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)internal/(vaults|codesessions)/' | sed -n '1,120p'
echo "== inject.go =="
cat -n internal/vaults/inject.go
echo "== inject_match.go =="
cat -n internal/vaults/inject_match.go
echo "== inject tests =="
cat -l internal/vaults/inject_test.go 2>/dev/null || true
sed -n '1,260p' internal/vaults/inject_test.go | cat -n
echo "== handler calls =="
sed -n '60,95p' internal/codesessions/handler.go | cat -n
sed -n '200,245p' internal/codesessions/mcp_proxy_test.go | cat -n
echo "== url scheme/static search =="
rg -n "http://|https://|Scheme|Bearer |Authorization|Decrypt|SecretPayload|ListVault|VaultExternalID|MCPServerURL|static bearer|StaticBearer" internal -g '*.go' | sed -n '1,220p'Repository: superduck-ai/open-managed-agents
Length of output: 41029
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: External
Reachability path
● Entry
internal/api/server.go:88
NewServer
│
▼
● Hop
internal/codesessions/handler.go:73
WithVaultSecrets
│
▼
● Sink
internal/vaults/inject.go
Reject bearer injection when the request scheme is not HTTPS.
DecideInjection matches host and path but does not require requestURL.Scheme == "https", so a matching HTTP MCP target can reach header.Set("Authorization", "Bearer "+token). Reject injection for HTTP targets. Add a regression test with http://....
Proposed fix
case InjectionInject:
+ if requestURL == nil || !strings.EqualFold(requestURL.Scheme, "https") {
+ return ErrInjectionRejected
+ }
token, err := openStaticBearerToken(ctx, i.secretSvc, decision.Credential)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| decision := DecideInjection(requestURL, credentials) | |
| switch decision.Kind { | |
| case InjectionPassthrough: | |
| return nil | |
| case InjectionReject: | |
| return ErrInjectionRejected | |
| case InjectionInject: | |
| token, err := openStaticBearerToken(ctx, i.secretSvc, decision.Credential) | |
| if err != nil { | |
| return fmt.Errorf("%w: open credential: %w", ErrInjectionRejected, err) | |
| } | |
| header.Del("Authorization") | |
| header.Set("Authorization", "Bearer "+token) | |
| decision := DecideInjection(requestURL, credentials) | |
| switch decision.Kind { | |
| case InjectionPassthrough: | |
| return nil | |
| case InjectionReject: | |
| return ErrInjectionRejected | |
| case InjectionInject: | |
| if requestURL == nil || !strings.EqualFold(requestURL.Scheme, "https") { | |
| return ErrInjectionRejected | |
| } | |
| token, err := openStaticBearerToken(ctx, i.secretSvc, decision.Credential) | |
| if err != nil { | |
| return fmt.Errorf("%w: open credential: %w", ErrInjectionRejected, err) | |
| } | |
| header.Del("Authorization") | |
| header.Set("Authorization", "Bearer "+token) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/vaults/inject.go` around lines 61 - 73, Update the injection flow
around DecideInjection to reject any request whose URL scheme is not HTTPS
before opening the credential or setting the Authorization header. Preserve
passthrough behavior for non-matching requests, return ErrInjectionRejected for
matching HTTP targets, and add a regression test covering an http:// request.
- 在MCP HTTP代理路径中集成vaults.Injector进行运行时凭证注入 - 实现static_bearer类型凭证的路径前缀匹配和注入逻辑 - 添加对真实mcp_url的目标URL进行凭证匹配和授权验证 - 重构handler结构支持WithVaultSecrets方法注入凭证服务 - 实现注入决策逻辑包括passthrough、inject和reject三种状态 - 添加相关的数据库查询方法和单元测试验证注入行为
qifanlili
force-pushed
the
feat/vault-secret-envelope-encryption
branch
from
August 6, 2026 06:40
2ca5921 to
fb1dc20
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
总览
Vault 凭证以前会把明文秘密写进
secret_payload。本 PR 改成信封加密存库：每条秘密用一次性 DEK 加密，再用本地 KEK 包住 DEK；写路径 seal，读路径 open。主密钥走config.yaml（支持 inline 或_file），并支持 current +decrypt_only换钥，旧数据不用整库重加密。本 PR 只做存库加密。运行时 MITM / vault 注入不在本次范围，后续另开 PR。
提供的功能与改动
1. 信封加密与 Secret Service
internal/secrets：AES-256-GCM 信封，AAD 绑定 organization / workspace / vault / credential，搬走解不开。KeyProvider接口先落地本地 KEK；启动时 Prepare，业务路径不直接读配置里的原始密钥字节。2. 数据库 Direct cutover
00047_add_vault_secret_envelope.sql：加ciphertext/nonce/wrapped_dek/format_version/key_provider/key_version/version，并删除secret_payload。version做 CAS。3. 写路径强制加密
client_secret一类秘密当明文落库。4. 本地 KEK 轮换
version+decrypt_only旧钥列表。key_version在 current ∪ decrypt_only 里选钥。key_version，能解就行。关键设计与原因
配置关系
vault.master_key只管“怎么封/怎么解”；Vault API、OAuth 写路径、DB 信封列共用这一套。运行时注入仍依赖后续 MITM 能力，不在本配置里。验证与范围
建议验证：
internal/secrets、internal/config、vault / encryption 相关 Go 测试。vault.master_key后启服务，创建或更新 credential，确认库中是信封列而不是明文。decrypt_only，确认历史行仍可 Open，新写入打上新key_version。docs/design/be/vault-runtime.md。本 PR 不包含：运行时 MITM 注入、Shamir/KMS provider、批量 rewrap、把秘密写进沙箱环境变量。
总结
合入后，Vault 秘密按信封加密落库，本地 KEK 可轮换且旧数据仍可解；明文
secret_payload从 schema 移除。沙箱侧如何用上这些秘密，留给后续注入 PR。Summary by CodeRabbit
New Features
Bug Fixes
Documentation