chore: remove vault encryption hooks from certain GORM tables in favor of AES-only encryption - #4245
Conversation
📝 WalkthroughWalkthroughThis PR removes Vault-backed storage integration from the configstore encryption system. All credential tables (MCP, OAuth, sessions, temp tokens) now use only encrypt.IsEnabled() for encryption decisions, eliminating vault-first branching, vault secret resolution, and vault cleanup hooks. ChangesVault Integration Removal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BearTS
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
Confidence Score: 4/5Safe to merge only after verifying that all vault-backed rows have been migrated or that this is being deployed to an environment that never used vault storage; deploying without that migration will cause silent read failures on any existing vault-status rows. The vault removal and AES-only consolidation are clean across most tables, and the DeleteMCPClientConfig transaction improvement is a genuine correctness win. The gap is that every AfterFind hook now silently passes raw vault-path strings back to callers for any row whose encryption_status is still 'vault' — no error is surfaced, so OAuth tokens, connection strings, and session tokens become silently unusable for those rows. The required migration is explicitly called out in the PR description but is not present in the diff. framework/configstore/tables/oauth.go and the parallel AfterFind hooks in mcp.go, sessions.go, temp_token.go, and vectorstore.go all share the silent-pass-through behavior for vault-status rows; mcp_per_user_headers.go still has orphaned AfterDelete and DeleteVaultSecrets vault code that was removed from every other table. Important Files Changed
Reviews (2): Last reviewed commit: "chore: dont store temp tokens in vault" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/configstore/tables/mcp.go (1)
223-239:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRoot cause: legacy
EncryptionStatusVaultdata is no longer readable anywhere these hooks changed.Across these
AfterFindpaths, thevaultbranch was removed without a compatibility gate. Deployments with pre-existing Vault-backed rows will either fail immediately (JSON-bearing rows) or surface vault references as secrets, and any later save can permanently AES-encrypt that reference instead of the real value. Please block release on a migration-safe path: either keep legacy reads until the migration has completed, or make startup/read fail explicitly when anyvaultrow remains.🤖 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 `@framework/configstore/tables/mcp.go` around lines 223 - 239, The AfterFind hook on TableMCPClient currently only handles "encrypted" rows and drops support for legacy "vault" rows; restore compatibility by handling the legacy EncryptionStatusVault case in TableMCPClient.AfterFind (or fail fast if you choose the migration-enforce route). Specifically, update the AfterFind method to detect when c.EncryptionStatus equals the legacy vault value (e.g., EncryptionStatusVault or "vault") and either (A) perform the legacy Vault read/decrypt for HeadersJSON and ConnectionString (the same fields currently handled for "encrypted") before returning, or (B) return a clear, non-ambiguous error from AfterFind indicating a migration is required so startup/read will fail fast; ensure you reference and use the same fields (c.HeadersJSON and c.ConnectionString.Val / c.ConnectionString.IsFromEnv()) and preserve existing error wrapping behavior when decrypt/read fails.framework/configstore/rdb.go (1)
1721-1910:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock update/delete on unmigrated Vault-backed MCP rows.
The PR notes that
encryption_status='vault'rows must be migrated before rollout, but these paths now operate on them silently. InUpdateMCPClientConfig, Line 1854 can relabel a Vault-backed row as AES-encrypted even when the read-onlyconnection_stringwas never rewritten (for example, a normal UI edit with emptyConfigHash). InDeleteMCPClientConfig, Lines 1915-1960 now remove the DB row without the old Vault cleanup, which leaves the external secret orphaned. Please fail fast whenexistingClient.EncryptionStatus == "vault"so operators get a migration-required error instead of silent state corruption / secret retention.Suggested guard
func (s *RDBConfigStore) UpdateMCPClientConfig(ctx context.Context, id string, clientConfig *tables.TableMCPClient) error { return s.DB().Transaction(func(tx *gorm.DB) error { // Find existing client var existingClient tables.TableMCPClient if err := dbForUpdate(tx.WithContext(ctx)).Where("client_id = ?", id).First(&existingClient).Error; err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { return fmt.Errorf("MCP client with id '%s' not found", id) } return err } + if existingClient.EncryptionStatus == "vault" { + return fmt.Errorf("mcp client %q still uses vault-backed encryption; migrate it before updating", id) + } // Create a deep copy to avoid modifying the original clientConfigCopy, err := deepCopy(clientConfig) if err != nil { return err @@ func (s *RDBConfigStore) DeleteMCPClientConfig(ctx context.Context, id string) error { return s.DB().Transaction(func(tx *gorm.DB) error { // Find existing client var existingClient tables.TableMCPClient if err := dbForUpdate(tx.WithContext(ctx)).Where("client_id = ?", id).First(&existingClient).Error; err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { return fmt.Errorf("MCP client with id '%s' not found", id) } return err } + if existingClient.EncryptionStatus == "vault" { + return fmt.Errorf("mcp client %q still uses vault-backed encryption; migrate it before deleting", id) + }Also applies to: 1913-1960
🤖 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 `@framework/configstore/rdb.go` around lines 1721 - 1910, The code must fail fast for Vault-backed rows to avoid relabeling or orphaning secrets: in UpdateMCPClientConfig (inside the transaction after loading existingClient) check existingClient.EncryptionStatus == "vault" and return an explicit migration-required error before any serialization/encryption or before setting updates so the row is never relabeled; do the same early in DeleteMCPClientConfig (after loading existingClient) and abort the transaction with the same migration-required error instead of deleting; ensure these checks run before any call sites that mutate encryption_status, connection_string, or call encrypt.Encrypt so Vault rows are preserved for the migration path.
🤖 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.
Outside diff comments:
In `@framework/configstore/rdb.go`:
- Around line 1721-1910: The code must fail fast for Vault-backed rows to avoid
relabeling or orphaning secrets: in UpdateMCPClientConfig (inside the
transaction after loading existingClient) check existingClient.EncryptionStatus
== "vault" and return an explicit migration-required error before any
serialization/encryption or before setting updates so the row is never
relabeled; do the same early in DeleteMCPClientConfig (after loading
existingClient) and abort the transaction with the same migration-required error
instead of deleting; ensure these checks run before any call sites that mutate
encryption_status, connection_string, or call encrypt.Encrypt so Vault rows are
preserved for the migration path.
In `@framework/configstore/tables/mcp.go`:
- Around line 223-239: The AfterFind hook on TableMCPClient currently only
handles "encrypted" rows and drops support for legacy "vault" rows; restore
compatibility by handling the legacy EncryptionStatusVault case in
TableMCPClient.AfterFind (or fail fast if you choose the migration-enforce
route). Specifically, update the AfterFind method to detect when
c.EncryptionStatus equals the legacy vault value (e.g., EncryptionStatusVault or
"vault") and either (A) perform the legacy Vault read/decrypt for HeadersJSON
and ConnectionString (the same fields currently handled for "encrypted") before
returning, or (B) return a clear, non-ambiguous error from AfterFind indicating
a migration is required so startup/read will fail fast; ensure you reference and
use the same fields (c.HeadersJSON and c.ConnectionString.Val /
c.ConnectionString.IsFromEnv()) and preserve existing error wrapping behavior
when decrypt/read fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 030805cf-9f52-49bc-9cff-97331ed4bc29
⛔ Files ignored due to path filters (1)
plugins/modelcatalogresolver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
framework/configstore/rdb.goframework/configstore/tables/mcp.goframework/configstore/tables/mcp_per_user_headers.goframework/configstore/tables/oauth.goframework/configstore/tables/sessions.goframework/configstore/tables/temp_token.goframework/configstore/tables/vectorstore.goplugins/modelcatalogresolver/go.mod
4f89a48 to
cd6685d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
framework/configstore/tables/mcp.go (1)
223-239:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBackfill legacy
vaultrows before shipping this read-path change.Removing the
EncryptionStatusVaultbranch here makes existingconfig_mcp_clientsrows unreadable on rollout. For those rows,HeadersJSONnow stays as the vault locator, so the later unmarshal on Line 265 fails, and non-envConnectionStringnever resolves either. The PR description already calls out the required migration, but it is not part of this cohort; ship the backfill/re-encryption in the same release (or keep a temporary vault-read fallback) before merging. The same rollout blocker applies to the other table hooks in this PR.🤖 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 `@framework/configstore/tables/mcp.go` around lines 223 - 239, The AfterFind hook on TableMCPClient currently only handles EncryptionStatusEncrypted and will break existing rows that still have EncryptionStatusVault; restore a fallback branch for EncryptionStatusVault or perform an inline vault-read-and-reencrypt in AfterFind so legacy rows remain readable. Specifically, update AfterFind (the function name) to detect EncryptionStatusVault, resolve HeadersJSON and ConnectionString by reading the vault locator (using your vault client) and then re-encrypt or replace those fields with decrypted values before returning, or alternatively ensure a migration/backfill job runs in the same release to move rows from EncryptionStatusVault to EncryptionStatusEncrypted; touch the HeadersJSON, ConnectionString.Val, and switch the EncryptionStatus flag so later JSON unmarshal and ConnectionString.IsFromEnv/GetValue logic work as expected.framework/configstore/rdb.go (2)
1915-1959:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDeleting an MCP client still races concurrent session/token/header inserts.
This transaction deletes existing dependent rows by
mcp_client_id, but the create/upsert paths foroauth_user_tokens,oauth_user_sessions,mcp_per_user_header_credentials, andmcp_per_user_header_flowsinsert by that same string key without first locking or validating the client row. A concurrent auth flow can commit after theseDELETEs and leave fresh orphan rows for a client that was just removed.To close the gap, this needs a cross-file contract: either reference
config_mcp_clients.idwith FK/cascade, or add a tombstone/disabled state that the create paths check under the same client-row lock before inserting.As per coding guidelines, framework changes should preserve race-safe behavior and atomic cleanup.
🤖 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 `@framework/configstore/rdb.go` around lines 1915 - 1959, The delete transaction still races with concurrent inserts because dependent rows use the client string key and creators don't lock/validate the client row; fix by either (A) changing dependent tables to reference tables.TableMCPClient.ID with a proper FK+cascade (add migration, update models for TableOauthUserToken, TableOauthUserSession, TableMCPPerUserHeaderCredential, TableMCPPerUserHeaderFlow and TableVirtualKeyMCPConfig to use mcp_client_id uint FK) or (B) add a tombstone/disabled boolean on tables.TableMCPClient and make all create/upsert paths (the code paths that insert into TableOauthUserToken, TableOauthUserSession, TableMCPPerUserHeaderCredential, TableMCPPerUserHeaderFlow and TableVirtualKeyMCPConfig) perform a dbForUpdate(tx.WithContext(ctx)).Where("client_id = ?").First(&client) FOR UPDATE and check client.Disabled (or return error) before inserting; ensure the Delete code still locks the client row (use dbForUpdate as shown) and sets Disabled (or deletes) inside the same transaction so concurrent creators cannot commit orphan rows.Source: Coding guidelines
1831-1856:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't rewrite MCP secrets to plaintext while leaving
encryption_statusstale.Line 1846 and Line 1892 always persist
headers_json/connection_string, but Line 1854 only updatesencryption_statuswhenencrypt.IsEnabled(). If an already-encrypted client is edited while encryption is off, this path writes plaintext and preserves the old encrypted marker, so the nextAfterFinddecryption runs against plaintext and the row becomes unreadable.Suggested guard
- if encrypt.IsEnabled() { + if encrypt.IsEnabled() { updates["encryption_status"] = encryptionStatusEncrypted + } else if existingClient.EncryptionStatus == encryptionStatusEncrypted { + return fmt.Errorf("cannot update MCP client %q while encryption is disabled because stored secrets are still marked encrypted", id) }Based on learnings,
framework/configstore/rdb.goandframework/configstore/tables/mcp.gomust keepencryption_statusaligned with whether MCP fields are actually encrypted, andAfterFinduses that status to decide decryption.Also applies to: 1878-1893
🤖 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 `@framework/configstore/rdb.go` around lines 1831 - 1856, The code writes headers_json/connection_string but only sets updates["encryption_status"] when encrypt.IsEnabled(), which can leave the DB marker stale; change the updates map logic so encryption_status is explicitly set to encrypted when encrypt.IsEnabled() and explicitly set to the non-encrypted state when encrypt.IsEnabled() is false (e.g. updates["encryption_status"] = encrypt.IsEnabled() ? encryptionStatusEncrypted : <unencrypted-status-constant>), and apply the same change to the other block that updates "connection_string" so encryption_status always reflects whether the stored fields are actually encrypted (use the existing encrypt.IsEnabled(), the updates map, "headers_json"/"connection_string", and the encryptionStatusEncrypted constant as references).Source: Learnings
🤖 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.
Outside diff comments:
In `@framework/configstore/rdb.go`:
- Around line 1915-1959: The delete transaction still races with concurrent
inserts because dependent rows use the client string key and creators don't
lock/validate the client row; fix by either (A) changing dependent tables to
reference tables.TableMCPClient.ID with a proper FK+cascade (add migration,
update models for TableOauthUserToken, TableOauthUserSession,
TableMCPPerUserHeaderCredential, TableMCPPerUserHeaderFlow and
TableVirtualKeyMCPConfig to use mcp_client_id uint FK) or (B) add a
tombstone/disabled boolean on tables.TableMCPClient and make all create/upsert
paths (the code paths that insert into TableOauthUserToken,
TableOauthUserSession, TableMCPPerUserHeaderCredential,
TableMCPPerUserHeaderFlow and TableVirtualKeyMCPConfig) perform a
dbForUpdate(tx.WithContext(ctx)).Where("client_id = ?").First(&client) FOR
UPDATE and check client.Disabled (or return error) before inserting; ensure the
Delete code still locks the client row (use dbForUpdate as shown) and sets
Disabled (or deletes) inside the same transaction so concurrent creators cannot
commit orphan rows.
- Around line 1831-1856: The code writes headers_json/connection_string but only
sets updates["encryption_status"] when encrypt.IsEnabled(), which can leave the
DB marker stale; change the updates map logic so encryption_status is explicitly
set to encrypted when encrypt.IsEnabled() and explicitly set to the
non-encrypted state when encrypt.IsEnabled() is false (e.g.
updates["encryption_status"] = encrypt.IsEnabled() ? encryptionStatusEncrypted :
<unencrypted-status-constant>), and apply the same change to the other block
that updates "connection_string" so encryption_status always reflects whether
the stored fields are actually encrypted (use the existing encrypt.IsEnabled(),
the updates map, "headers_json"/"connection_string", and the
encryptionStatusEncrypted constant as references).
In `@framework/configstore/tables/mcp.go`:
- Around line 223-239: The AfterFind hook on TableMCPClient currently only
handles EncryptionStatusEncrypted and will break existing rows that still have
EncryptionStatusVault; restore a fallback branch for EncryptionStatusVault or
perform an inline vault-read-and-reencrypt in AfterFind so legacy rows remain
readable. Specifically, update AfterFind (the function name) to detect
EncryptionStatusVault, resolve HeadersJSON and ConnectionString by reading the
vault locator (using your vault client) and then re-encrypt or replace those
fields with decrypted values before returning, or alternatively ensure a
migration/backfill job runs in the same release to move rows from
EncryptionStatusVault to EncryptionStatusEncrypted; touch the HeadersJSON,
ConnectionString.Val, and switch the EncryptionStatus flag so later JSON
unmarshal and ConnectionString.IsFromEnv/GetValue logic work as expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc6cdb93-8fa5-4a8a-bf2a-92f841daf37a
⛔ Files ignored due to path filters (1)
plugins/modelcatalogresolver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
framework/configstore/rdb.goframework/configstore/tables/mcp.goframework/configstore/tables/mcp_per_user_headers.goframework/configstore/tables/oauth.goframework/configstore/tables/sessions.goframework/configstore/tables/temp_token.goframework/configstore/tables/vectorstore.goplugins/modelcatalogresolver/go.mod
BearTS
changed the title
BearTS
changed the title
Merge activity
|
…r of AES-only encryption (#4245) ## Summary Removes Vault-based secret storage from all sensitive-field GORM hooks and related database operations, leaving only the standard `encrypt` package path for at-rest encryption. This eliminates the dual-path complexity that existed for MCP client configs, OAuth tokens, sessions, temp tokens, and vector store configs. ## Changes - Removed all `VaultIsEnabled()` branches from `BeforeSave`, `AfterFind`, and `AfterDelete` hooks across `TableMCPClient`, `TableMCPPerUserHeaderCredential`, `TableOauthToken`, `TableOauthUserSession`, `TableOauthUserToken`, `SessionsTable`, `TempToken`, and `TableVectorStoreConfig`. - Removed `AfterDelete` vault cleanup hooks from all affected table types. - Removed `DeleteVaultSecrets` helper methods from `TableOauthUserToken`, `TableOauthUserSession`, and `TempToken`. - Removed pre/post-transaction vault compensation logic (`vaultStoredPaths`, `vaultRemovePaths`) from `UpdateMCPClientConfig` in `rdb.go`. - Removed the pre-transaction vault ID collection and post-transaction goroutine vault cleanup from `DeleteMCPClientConfig`, moving the record lookup inside the transaction instead. - Removed vault ID pre-collection and post-delete goroutine cleanup from `DeleteTempTokensByResourceID` and `DeleteExpiredTempTokens`. - Bumped `cloud.google.com/go/iam`, `aws-sdk-go-v2`, and `smithy-go` dependency versions in the `modelcatalogresolver` plugin. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/configstore/... go test ./framework/configstore/tables/... ``` Verify that MCP client configs, OAuth tokens, sessions, and temp tokens are correctly encrypted and decrypted using the `encrypt` package when `encrypt.IsEnabled()` is true, and that no vault-related paths are written or read. ## Breaking changes - [x] Yes - [ ] No Any deployments that previously relied on Vault-backed secret storage for these table types will no longer have secrets written to or read from Vault. Rows with `encryption_status = 'vault'` will not be decrypted correctly after this change. A migration to re-encrypt existing vault-backed rows using the standard encryption path is required before deploying. ## Security considerations Vault integration for field-level secret storage has been removed. All sensitive fields (OAuth tokens, MCP connection strings, headers, session tokens, temp tokens, vector store config) are now exclusively encrypted via the `encrypt` package. Ensure the `encrypt` package key material is properly secured in your deployment environment, as Vault is no longer available as an alternative secret backend. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
2 participants
Summary
Removes Vault-based secret storage from all sensitive-field GORM hooks and related database operations, leaving only the standard
encryptpackage path for at-rest encryption. This eliminates the dual-path complexity that existed for MCP client configs, OAuth tokens, sessions, temp tokens, and vector store configs.Changes
VaultIsEnabled()branches fromBeforeSave,AfterFind, andAfterDeletehooks acrossTableMCPClient,TableMCPPerUserHeaderCredential,TableOauthToken,TableOauthUserSession,TableOauthUserToken,SessionsTable,TempToken, andTableVectorStoreConfig.AfterDeletevault cleanup hooks from all affected table types.DeleteVaultSecretshelper methods fromTableOauthUserToken,TableOauthUserSession, andTempToken.vaultStoredPaths,vaultRemovePaths) fromUpdateMCPClientConfiginrdb.go.DeleteMCPClientConfig, moving the record lookup inside the transaction instead.DeleteTempTokensByResourceIDandDeleteExpiredTempTokens.cloud.google.com/go/iam,aws-sdk-go-v2, andsmithy-godependency versions in themodelcatalogresolverplugin.Type of change
Affected areas
How to test
Verify that MCP client configs, OAuth tokens, sessions, and temp tokens are correctly encrypted and decrypted using the
encryptpackage whenencrypt.IsEnabled()is true, and that no vault-related paths are written or read.Breaking changes
Any deployments that previously relied on Vault-backed secret storage for these table types will no longer have secrets written to or read from Vault. Rows with
encryption_status = 'vault'will not be decrypted correctly after this change. A migration to re-encrypt existing vault-backed rows using the standard encryption path is required before deploying.Security considerations
Vault integration for field-level secret storage has been removed. All sensitive fields (OAuth tokens, MCP connection strings, headers, session tokens, temp tokens, vector store config) are now exclusively encrypted via the
encryptpackage. Ensure theencryptpackage key material is properly secured in your deployment environment, as Vault is no longer available as an alternative secret backend.Checklist
docs/contributing/README.mdand followed the guidelines