feat: add vault backend (aws-secrets-manager, gcp-secret-manager, hashicorp-vault) as alternative to AES encryption for sensitive config fields - #4157
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds VaultHooks and helpers, wires transport-level vault_store config/schema and init stub, and updates many configstore table hooks and rdb delete flows to store, resolve, and best-effort remove secrets from an external vault when enabled. ChangesVault-backed secret storage for configstore tables
Sequence DiagramsequenceDiagram
participant Client as App/API
participant GORM as GORM Hooks
participant VaultHooks as VaultHooks
participant Vault as Vault (external)
participant DB as Database
Client->>GORM: Save model (BeforeSave)
GORM->>VaultHooks: StoreString(ctx, path, &value)
VaultHooks->>Vault: write secret at path
Vault->>VaultHooks: ack
GORM->>DB: persist row with vault reference
Client->>GORM: Load model (AfterFind)
GORM->>VaultHooks: ResolveString(ctx, &ref)
VaultHooks->>Vault: read secret at path
Vault->>VaultHooks: secret
VaultHooks->>GORM: populate field
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 unit tests (beta)
Comment |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BearTS
changed the title
aws-secrets-manager, gcp-secret-manager, hashicorp-vault) as alternative to AES encryption for sensitive config fields
BearTS
force-pushed
the
06-08-feat_vault_support
branch
from
June 8, 2026 11:43
d3cfd70 to
5033424
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
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/oauth.go (2)
93-110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
ResolveStringforCodeVerifierto matchBeforeSavelogic.In
BeforeSave, vault storage forCodeVerifieris guarded byc.CodeVerifier != ""(line 59), butAfterFindcallsVaultHooks.ResolveStringunconditionally. If the row was saved with an emptyCodeVerifier, no vault entry exists, andResolveStringwill attempt to resolve an empty/missing vault reference—potentially returning an error or corrupting the field.🐛 Proposed fix
case EncryptionStatusVault: if err := resolveVaultEnvVar(tx.Statement.Context, c.ClientSecret); err != nil { return fmt.Errorf("failed to resolve vault oauth client secret: %w", err) } - if err := VaultHooks.ResolveString(tx.Statement.Context, &c.CodeVerifier); err != nil { - return fmt.Errorf("failed to resolve vault oauth code verifier: %w", err) + if c.CodeVerifier != "" { + if err := VaultHooks.ResolveString(tx.Statement.Context, &c.CodeVerifier); err != nil { + return fmt.Errorf("failed to resolve vault oauth code verifier: %w", err) + } }🤖 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/oauth.go` around lines 93 - 110, AfterFind currently calls VaultHooks.ResolveString unconditionally for c.CodeVerifier under EncryptionStatusVault; mirror the BeforeSave guard by only attempting VaultHooks.ResolveString(tx.Statement.Context, &c.CodeVerifier) when c.CodeVerifier != "" to avoid resolving non-existent vault references. Update the EncryptionStatusVault branch in AfterFind to check c.CodeVerifier != "" before calling VaultHooks.ResolveString (keep the same context and pointer usage).
162-179:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMissing vault support for
TableOauthToken.
TableOauthToken.BeforeSaveencryptsAccessTokenandRefreshTokenbut lacks the vault branch added toTableOauthConfig. These tokens are sensitive credentials that should follow the same vault-or-encrypt pattern for consistency. Similarly,TableOauthUserSessionandTableOauthUserTokenbelow appear to be missing the vault integration.If this is intentional scoping for this PR, consider adding a TODO or tracking issue. Otherwise, this creates an inconsistency where some OAuth secrets can be vaulted and others cannot.
🤖 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/oauth.go` around lines 162 - 179, The BeforeSave in TableOauthToken currently only encrypts AccessToken/RefreshToken; add the same vault-or-encrypt branch used in TableOauthConfig so tokens can be stored in the vault when vault.IsEnabled() (and fall back to encrypting and setting EncryptionStatusEncrypted otherwise), and mirror this change for TableOauthUserSession.BeforeSave and TableOauthUserToken.BeforeSave (or add a clear TODO/tracking-note if omission is intentional); update use of EncryptionStatus to include a vaulted state and ensure fields are cleared/marked appropriately when vaulted.transports/bifrost-http/lib/config.go (1)
349-365:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
vault_storeis never deserialized in the custom unmarshal path.
ConfigData.UnmarshalJSONdrops thevault_storeblock becauseTempConfigDatahas no matching field andcd.VaultStoreConfigis never assigned. That makesinitVault(&configData)effectively unreachable from file config.Suggested fix
type TempConfigData struct { @@ FeatureFlags *FeatureFlagsFileConfig `json:"feature_flags,omitempty"` + VaultStoreConfig *VaultStoreConfig `json:"vault_store,omitempty"` } @@ cd.FeatureFlags = temp.FeatureFlags + cd.VaultStoreConfig = temp.VaultStoreConfigAs per coding guidelines,
transports/config.schema.jsonis the source of truth for config fields andvault_storemust be wired through parsing.Also applies to: 372-384
🤖 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 `@transports/bifrost-http/lib/config.go` around lines 349 - 365, TempConfigData used in ConfigData.UnmarshalJSON lacks a VaultStoreConfig field so the "vault_store" JSON is dropped and cd.VaultStoreConfig never gets set, preventing initVault(&configData) from running; update TempConfigData to include a VaultStoreConfig json.RawMessage (or the appropriate type used elsewhere) with the `vault_store` json tag, assign that parsed value to cd.VaultStoreConfig inside ConfigData.UnmarshalJSON after unmarshalling (mirroring how other RawMessage fields like VectorStoreConfig are handled), and ensure transports/config.schema.json includes the vault_store entry so the field remains authoritative.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 `@framework/configstore/tables/provider.go`:
- Around line 121-128: The vault branch in provider.go unconditionally stores
p.ProxyConfigJSON; update the guard to match plugin.go by only calling
VaultHooks.StoreString (and setting p.EncryptionStatus = EncryptionStatusVault)
when vaultIsEnabled() is true AND p.ProxyConfigJSON != "" AND p.ProxyConfigJSON
!= "{}", so avoid unnecessary Vault writes for empty/trivial proxy configs;
locate the branch around vaultIsEnabled(), the VaultHooks.StoreString call, and
the EncryptionStatusVault assignment to apply this conditional.
In `@framework/configstore/tables/sessions.go`:
- Around line 30-35: The vault storage block should skip storing an empty
session token; update the logic around VaultHooks.StoreString in the session
save/update flow (the code path using s.TableName(), s.Token and setting
s.EncryptionStatus = EncryptionStatusVault) to first check that s.Token != ""
before calling VaultHooks.StoreString and toggling EncryptionStatusVault,
mirroring the guard used in the oauth flow (e.g., the if c.CodeVerifier != ""
pattern) so you avoid unnecessary vault API calls and empty references.
In `@framework/configstore/tables/temp_token.go`:
- Around line 40-45: The vault storage block in the TempToken model currently
calls VaultHooks.StoreString unconditionally; add a guard to skip vault storage
when t.Token is empty by checking t.Token != "" before calling
VaultHooks.StoreString (the same pattern used in the fallback branch). Update
the code around vaultIsEnabled(), VaultHooks.StoreString(tx.Statement.Context,
path, &t.Token), and setting t.EncryptionStatus = EncryptionStatusVault so the
vault call and status change only occur when t.Token is non-empty.
In `@framework/configstore/tables/vectorstore.go`:
- Around line 48-52: The vault branch for vs.EncryptionStatus
(EncryptionStatusVault) is missing a nil check for vs.Config before calling
VaultHooks.ResolveString; add the same guard used in the encrypted branch (check
if vs.Config == nil and return a descriptive error) and only call
VaultHooks.ResolveString(tx.Statement.Context, vs.Config) if vs.Config is
non-nil, keeping the error wrapping consistent with other branches.
In `@framework/configstore/tables/virtualkey.go`:
- Around line 292-296: In AfterFind, guard the call to VaultHooks.ResolveString
the same way AfterDelete guards VaultHooks.Remove: check that VaultHooks is
non-nil and VaultHooks.ResolveString is non-nil before invoking it when
vk.EncryptionStatus == EncryptionStatusVault (the block that calls
VaultHooks.ResolveString for vk.Value); mirror the defensive pattern used around
VaultHooks.Remove in AfterDelete so reading rows won't panic if the hooks aren't
initialized.
In `@transports/bifrost-http/lib/config.go`:
- Around line 239-245: VaultStoreConfig is missing the enabled field so runtime
structs diverge from the schema; add an Enabled bool (json:"enabled") to the
VaultStoreConfig struct(s) to match transports/config.schema.json's
vault_store.enabled/type, update any constructors/parsers that build
VaultStoreConfig and any gating logic (e.g., initVault) to read the new Enabled
field, and ensure any other occurrences of VaultStoreConfig in the repo are
updated the same way so enable/disable semantics are consistent at boot.
In `@transports/config.schema.json`:
- Around line 4942-4973: Add a dependentRequired constraint to the "hashicorp"
object schema so AppRole credentials are validated as a pair: require
"secret_id" whenever "role_id" is present and vice versa. Modify the "hashicorp"
schema (the object with properties "address", "token", "namespace",
"mount_path", "role_id", "secret_id") to include dependentRequired or an
equivalent JSON Schema construct that enforces the mutual dependency between
"role_id" and "secret_id" and keep additionalProperties as false.
- Around line 4896-4926: Add JSON Schema dependency rules to the "aws" object so
credentials must be provided together: use dependentRequired on the aws
properties to require "secret_access_key" when "access_key_id" is present and
vice versa, and require both "access_key_id" and "secret_access_key" when
"session_token" is present; update the "aws" schema (the object with properties
region, access_key_id, secret_access_key, session_token, role_arn, kms_key_id)
to include these dependentRequired entries matching the S3 pattern elsewhere.
---
Outside diff comments:
In `@framework/configstore/tables/oauth.go`:
- Around line 93-110: AfterFind currently calls VaultHooks.ResolveString
unconditionally for c.CodeVerifier under EncryptionStatusVault; mirror the
BeforeSave guard by only attempting
VaultHooks.ResolveString(tx.Statement.Context, &c.CodeVerifier) when
c.CodeVerifier != "" to avoid resolving non-existent vault references. Update
the EncryptionStatusVault branch in AfterFind to check c.CodeVerifier != ""
before calling VaultHooks.ResolveString (keep the same context and pointer
usage).
- Around line 162-179: The BeforeSave in TableOauthToken currently only encrypts
AccessToken/RefreshToken; add the same vault-or-encrypt branch used in
TableOauthConfig so tokens can be stored in the vault when vault.IsEnabled()
(and fall back to encrypting and setting EncryptionStatusEncrypted otherwise),
and mirror this change for TableOauthUserSession.BeforeSave and
TableOauthUserToken.BeforeSave (or add a clear TODO/tracking-note if omission is
intentional); update use of EncryptionStatus to include a vaulted state and
ensure fields are cleared/marked appropriately when vaulted.
In `@transports/bifrost-http/lib/config.go`:
- Around line 349-365: TempConfigData used in ConfigData.UnmarshalJSON lacks a
VaultStoreConfig field so the "vault_store" JSON is dropped and
cd.VaultStoreConfig never gets set, preventing initVault(&configData) from
running; update TempConfigData to include a VaultStoreConfig json.RawMessage (or
the appropriate type used elsewhere) with the `vault_store` json tag, assign
that parsed value to cd.VaultStoreConfig inside ConfigData.UnmarshalJSON after
unmarshalling (mirroring how other RawMessage fields like VectorStoreConfig are
handled), and ensure transports/config.schema.json includes the vault_store
entry so the field remains authoritative.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52a61f67-75af-4dce-a437-236d6e12194c
📒 Files selected for processing (12)
framework/configstore/tables/encryption.goframework/configstore/tables/key.goframework/configstore/tables/mcp.goframework/configstore/tables/oauth.goframework/configstore/tables/plugin.goframework/configstore/tables/provider.goframework/configstore/tables/sessions.goframework/configstore/tables/temp_token.goframework/configstore/tables/vectorstore.goframework/configstore/tables/virtualkey.gotransports/bifrost-http/lib/config.gotransports/config.schema.json
BearTS
force-pushed
the
06-08-feat_vault_support
branch
from
June 8, 2026 16:28
5033424 to
3de0f48
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/configstore/tables/plugin.go (1)
56-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat
"{}"as "no config".
BeforeSaveserializesnilconfig to"{}", then both vault and AES branches skip it. On an update of an already vaulted/encrypted row, that leaves the oldEncryptionStatusin place while persisting literal JSON, so a laterAfterFindtries to resolve/decrypt"{}"as if it were a vault reference or ciphertext.Based on learnings: "The skip condition should only occur when
p.ConfigJSONis an empty string, not when it equals{}."🤖 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/plugin.go` around lines 56 - 69, The guard that treats "{}" as "no config" is wrong: in the BeforeSave/Save logic in plugin.go you must only skip vault/AES handling when p.ConfigJSON is an empty string, not when it equals "{}". Update the two branches that check p.ConfigJSON (the vault branch using vaultIsEnabled() + VaultHooks.StoreString and the AES branch using encrypt.IsEnabled() + encrypt.Encrypt) to remove the p.ConfigJSON != "{}" check so they only act when p.ConfigJSON != ""; keep setting p.EncryptionStatus to EncryptionStatusVault / EncryptionStatusEncrypted and ensure AfterFind continues to expect vault references/ciphertext only when EncryptionStatus indicates so.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.
Inline comments:
In `@framework/configstore/tables/encryption.go`:
- Around line 35-44: The default prefix literal "bifrost" in vaultPrefix()
should be extracted into a named constant (e.g., defaultVaultPrefix) to improve
maintainability and ensure consistency with config.schema.json; update the file
to declare the constant near the top, replace the hardcoded string in
vaultPrefix() with that constant, and ensure any other uses of the same default
(if present) reference this constant instead of repeating the literal; keep
VaultHooks.Prefix() invocation and vaultIsEnabled() unchanged.
In `@framework/configstore/tables/key.go`:
- Around line 487-555: The AfterFind implementation dereferences
tx.Statement.Context which panics when AfterFind is called with tx == nil (the
manual dbKey.AfterFind(nil) path); update AfterFind to safely derive a context
before calling resolveVaultEnvVar/resolveVaultString by doing something like:
set ctx := context.Background() (or context.TODO()) and if tx != nil &&
tx.Statement != nil && tx.Statement.Context != nil then override ctx =
tx.Statement.Context, then pass that ctx into
resolveVaultEnvVar/resolveVaultString (refer to AfterFind, resolveVaultEnvVar,
resolveVaultString and the current use of tx.Statement.Context).
In `@framework/configstore/tables/plugin.go`:
- Around line 78-83: AfterFind should guard using VaultHooks before calling
ResolveString: in the switch on p.EncryptionStatus (case EncryptionStatusVault)
check VaultHooks != nil and VaultHooks.ResolveString != nil before invoking
VaultHooks.ResolveString with tx.Statement.Context and &p.ConfigJSON, and return
a clear error if the hooks are unavailable; update the AfterFind method (the
switch handling EncryptionStatusVault and the use of p.ConfigJSON) to mirror the
nil-check already present in AfterDelete (which checks VaultHooks.Remove).
In `@framework/configstore/tables/provider.go`:
- Around line 166-169: In AfterFind, guard the call to VaultHooks.ResolveString
so it doesn't panic when the stubbed vault init is missing: before calling
VaultHooks.ResolveString with (tx.Statement.Context, &p.ProxyConfigJSON), check
that p.EncryptionStatus == EncryptionStatusVault && p.ProxyConfigJSON != "" and
that VaultHooks is non-nil and VaultHooks.ResolveString (or the equivalent
method) is callable; if the hook is missing, treat it as a no-op or return a
clear error instead of calling into a nil function. Ensure you reference
p.EncryptionStatus, EncryptionStatusVault, p.ProxyConfigJSON and
VaultHooks.ResolveString when making the change.
In `@framework/configstore/tables/sessions.go`:
- Around line 61-67: The AfterDelete hook on SessionsTable can't access
TokenHash/EncryptionStatus when rdb.go deletes via
Delete(&tables.SessionsTable{}, ...), so VaultHooks.Remove is never called and
secrets leak; fix by moving vault cleanup into the store layer where you can
load the session row before deletion (e.g., in the store method in
framework/configstore/rdb.go) or change the delete call to delete a hydrated
SessionsTable instance so AfterDelete sees the fields; implement: first load the
SessionsTable by ID/criteria, call VaultHooks.Remove(tx.Statement.Context, path)
(using the same tx) if EncryptionStatus==EncryptionStatusVault, then delete the
loaded SessionsTable within the same transaction, ensuring VaultHooks.Remove/DB
delete occur atomically.
In `@framework/configstore/tables/temp_token.go`:
- Around line 71-77: TempToken.AfterDelete currently assumes t.ID and
t.EncryptionStatus are present, but batch deletes using
Delete(&tables.TempToken{}) bypass per-row hooks and will orphan vaulted
secrets; update the store logic that performs deletions (where
Delete(&tables.TempToken{}) is used) to first Select the affected TempToken rows
(including ID and EncryptionStatus) and then either (a) delete hydrated
TempToken instances inside the same transaction so AfterDelete can run safely,
or (b) after selecting the rows call VaultHooks.Remove(tx.Statement.Context,
path) for each vaulted token yourself using the same
vaultPrefix()/TableName()/ID path construction, ensuring VaultHooks.Remove is
invoked only for items with EncryptionStatus == EncryptionStatusVault and done
within the transaction/cleanup flow.
In `@framework/configstore/tables/vectorstore.go`:
- Around line 31-33: The vault path currently uses
vaultPrefix()/ConfigTable/config which is shared across rows; change the path
construction used in VaultHooks.StoreString (and the corresponding load/delete
calls at the other location) to include a stable per-row identifier (e.g., the
row primary key or a stable UUID field) so each vector-store row gets its own
vault path. Locate the code building path via
tx.Statement.DB.NamingStrategy.ColumnName and vs.TableName() and append a unique
component such as vs.ID or a persisted vs.UUID (or derive from
tx.Statement.Schema.PrimaryField) to the fmt.Sprintf path before calling
VaultHooks.StoreString, and make the same change to the code that reads/removes
the secret at the other referenced lines so operations target the per-row vault
path.
- Around line 30-37: The vault branch calls VaultHooks.StoreString with
vs.Config even when vs.Config is nil or empty, causing downstream nil-pointer
issues; update the conditional in the function containing vaultIsEnabled() so
that before calling VaultHooks.StoreString (and before setting
vs.EncryptionStatus = EncryptionStatusVault) you first check that vs.Config !=
nil and *vs.Config != "" (the same guard used in the encrypt.IsEnabled branch),
and only then build the path and call VaultHooks.StoreString; ensure you
reference the existing symbols vaultIsEnabled, VaultHooks.StoreString, vs.Config
and EncryptionStatusVault when making this change.
In `@transports/bifrost-http/lib/config.go`:
- Line 174: Config unmarshaling currently ignores the new VaultStoreConfig
because TempConfigData used in ConfigData.UnmarshalJSON doesn't include the
vault_store field and the subsequent copy block never assigns
cd.VaultStoreConfig; update TempConfigData (used in ConfigData.UnmarshalJSON) to
include VaultStoreConfig and add an assignment to set cd.VaultStoreConfig from
the parsed temp struct in the copy block so that VaultStoreConfig is hydrated
when config.json contains vault_store (refer to VaultStoreConfig,
TempConfigData, ConfigData.UnmarshalJSON and cd.VaultStoreConfig).
In `@transports/config.schema.json`:
- Around line 4877-4985: vault_config currently only requires "enabled" and
"type" so backend-specific blocks can be omitted; add JSON Schema conditionals
to enforce presence of the correct backend object depending on
vault_config.type. Modify the vault_config schema (root object symbol:
"vault_config") to include an allOf/oneOf of three conditional subschemas using
"if": {"properties":{"type":{"const":"aws-secrets-manager"}}} "then":
{"required":["aws"]} (and similarly for "gcp-secret-manager" requiring "gcp",
and "hashicorp-vault" requiring "hashicorp"}), ensuring each conditional also
constrains that the corresponding backend object exists and leverages the
existing "aws","gcp","hashicorp" property definitions and their
dependentRequired/additionalProperties settings.
---
Outside diff comments:
In `@framework/configstore/tables/plugin.go`:
- Around line 56-69: The guard that treats "{}" as "no config" is wrong: in the
BeforeSave/Save logic in plugin.go you must only skip vault/AES handling when
p.ConfigJSON is an empty string, not when it equals "{}". Update the two
branches that check p.ConfigJSON (the vault branch using vaultIsEnabled() +
VaultHooks.StoreString and the AES branch using encrypt.IsEnabled() +
encrypt.Encrypt) to remove the p.ConfigJSON != "{}" check so they only act when
p.ConfigJSON != ""; keep setting p.EncryptionStatus to EncryptionStatusVault /
EncryptionStatusEncrypted and ensure AfterFind continues to expect vault
references/ciphertext only when EncryptionStatus indicates so.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 252ca480-27d0-4ddc-96a0-fc203a142536
📒 Files selected for processing (12)
framework/configstore/tables/encryption.goframework/configstore/tables/key.goframework/configstore/tables/mcp.goframework/configstore/tables/oauth.goframework/configstore/tables/plugin.goframework/configstore/tables/provider.goframework/configstore/tables/sessions.goframework/configstore/tables/temp_token.goframework/configstore/tables/vectorstore.goframework/configstore/tables/virtualkey.gotransports/bifrost-http/lib/config.gotransports/config.schema.json
BearTS
force-pushed
the
06-08-feat_vault_support
branch
from
June 8, 2026 19:25
3de0f48 to
74ec0e7
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/configstore/tables/virtualkey.go (1)
270-282:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve vault-backed rows when vault is later disabled.
AfterFindresolves vaulted values back to plaintext, butBeforeSavedecides storage solely from the current runtime flags. That means updating an existingEncryptionStatusVaultrow aftervaultIsEnabled()flips to false will either write plaintext back into the DB while the status still says"vault"(when AES is also off), or migrate the row to AES and orphan the old vault secret because nothing removes it. This breaks the advertisedenabled=falsefallback behavior for existing data.Also applies to: 292-297
♻️ Duplicate comments (2)
framework/configstore/tables/temp_token.go (1)
71-77:⚠️ Potential issue | 🟠 MajorBatch temp-token deletes bypass the data this hook needs.
framework/configstore/rdb.goremoves temp tokens withDelete(&tables.TempToken{})in bothDeleteTempTokensByResourceIDandDeleteExpiredTempTokens, soAfterDeletedoes not have the deleted row’sIDorEncryptionStatus. Line 75 then builds the wrong path, and vaulted temp-token secrets remain orphaned. Load the affected rows first and perform vault removal in the store layer before deleting the hydrated records.framework/configstore/tables/sessions.go (1)
61-67:⚠️ Potential issue | 🟠 MajorThis cleanup hook cannot remove vaulted session secrets on the current delete paths.
framework/configstore/rdb.godeletes sessions withDelete(&tables.SessionsTable{}, ...)and flushes withDelete(&tables.SessionsTable{}), so this hook never gets a hydratedTokenHashorEncryptionStatus. Line 65 therefore cannot derive the real vault path, and vaulted session secrets are left orphaned. Move the vault removal into the store layer: select the affected rows first, remove their vault entries in the same transaction, then delete those hydrated rows.🤖 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/sessions.go` around lines 61 - 67, The AfterDelete hook on SessionsTable (SessionsTable.AfterDelete) cannot reliably remove vaulted session secrets because rdb deletion paths use Delete(&tables.SessionsTable{}, ...) and Delete(&tables.SessionsTable{}) which pass un-hydrated rows (no TokenHash/EncryptionStatus) to the hook; as a result VaultHooks.Remove is called with an incorrect path and secrets are orphaned. Fix by moving vault cleanup out of SessionsTable.AfterDelete into the store layer (where rdb.go issues deletions): before issuing Delete(&tables.SessionsTable{}, ...) or Delete(&tables.SessionsTable{}), perform a SELECT to load the affected SessionsTable rows within the same transaction, iterate those hydrated rows to call VaultHooks.Remove(tx.Statement.Context, vaultPrefix()+"/"+row.TableName()+"/"+row.TokenHash) for rows with EncryptionStatus == EncryptionStatusVault, and only after successful vault removals issue the DELETE for those rows in the same tx; remove or no-op the current logic in SessionsTable.AfterDelete to avoid double/failed attempts.
🤖 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 `@framework/configstore/tables/encryption.go`:
- Around line 37-39: vaultIsEnabled() currently only checks VaultHooks.IsEnabled
which allows starting in "vault mode" without StoreString/ResolveString and
causes silent plaintext persistence; change vaultIsEnabled to return true only
when VaultHooks.IsEnabled is non-nil and returns true AND VaultHooks.StoreString
and VaultHooks.ResolveString are non-nil; additionally, where initialization
sets EncryptionStatusVault (e.g., code paths like the key table init that call
vaultIsEnabled and later set EncryptionStatusVault), add an explicit hard-fail
(error return or process exit) if IsEnabled exists and returns true but the
required StoreString/ResolveString hooks are missing so startup fails closed
instead of no-op.
In `@framework/configstore/tables/key.go`:
- Around line 333-404: The BeforeSave block (in key.go around the vault-enabled
branch) currently calls vaultEnvVar/vaultString for many fields but never
removes stale secrets when those fields become nil/empty/env-backed; add
best-effort removal calls (e.g., call the vault removal helper used elsewhere —
like vaultRemove or equivalent) for each field you no longer vault (Value,
AzureEndpoint, AzureClientID, AzureClientSecret, AzureTenantID, VertexProjectID,
VertexProjectNumber, VertexRegion, VertexAuthCredentials, BedrockAccessKey,
BedrockSecretKey, BedrockSessionToken, BedrockRegion, BedrockARN,
BedrockRoleARN, BedrockExternalID, BedrockRoleSessionName,
BedrockBatchS3ConfigJSON, AliasesJSON, VLLMUrl, OllamaUrl, SGLUrl) when their
new value is empty/nil or env-backed before skipping vaulting; keep using the
same base path construction (base := fmt.Sprintf("%s/%s/%s", vaultPrefix(),
k.TableName(), k.KeyID) and col := ...) and mark as best-effort (ignore/remove
errors only after logging) so retired secrets don’t accumulate while preserving
existing vaultEnvVar/vaultString behavior and the k.EncryptionStatus flow.
In `@framework/configstore/tables/mcp.go`:
- Around line 197-214: HeadersJSON is being stored directly via
VaultHooks.StoreString while ConnectionString uses the shared helper pair
(vaultEnvVar/vaultString); change HeadersJSON to use the same helper functions
so the nil-hook guard in framework/configstore/tables/encryption.go is honored.
Locate the block in mcp.go where vaultIsEnabled() is true and replace the
VaultHooks.StoreString usage for c.HeadersJSON with the shared vault helper
(call vaultString when saving and resolveVaultString when reading), keeping the
same vault path construction (vaultPrefix(), c.TableName(), c.ClientID,
ColumnName "", "HeadersJSON") and still set c.EncryptionStatus =
EncryptionStatusVault; mirror the same pattern used for ConnectionString (use
the cs local copy approach if needed) and apply the same change at the second
occurrence referenced in the comment (lines ~243-247).
In `@framework/configstore/tables/oauth.go`:
- Around line 49-69: The CodeVerifier branch is directly calling
VaultHooks.StoreString, bypassing the shared vault helpers
(vaultString/resolveVaultString) and losing empty-value no-op semantics and the
nil-hook guard; change the CodeVerifier handling in the vaultIsEnabled block so
it uses the same helper used for ClientSecret (i.e., call vaultString with the
same path and &c.CodeVerifier instead of VaultHooks.StoreString), keep setting
vaulted = true and c.EncryptionStatus = EncryptionStatusVault when appropriate,
and make the equivalent change in the second occurrence (the block around lines
93-100) so both store+read paths use the shared helpers.
In `@framework/configstore/tables/plugin.go`:
- Around line 56-63: The code currently skips vault/encryption when p.ConfigJSON
== "{}", but initialization elsewhere normalizes nil configs to "{}", so change
the guard to only skip when p.ConfigJSON == "" by removing the p.ConfigJSON !=
"{}" checks; update the conditional in the block using vaultIsEnabled() (the if
that calls VaultHooks.StoreString and sets p.EncryptionStatus =
EncryptionStatusVault) and the subsequent else-if that checks
encrypt.IsEnabled() to only test p.ConfigJSON != "" so that
VaultHooks.StoreString and the encryption path run for "{}" as intended.
In `@framework/configstore/tables/vectorstore.go`:
- Around line 50-52: The EncryptionStatusVault branch in vectorstore.go
currently calls VaultHooks.ResolveString directly which can panic if enterprise
hooks aren't installed; replace that call with the validated vault resolver
helper (i.e., the helper that safely resolves vault-backed strings and returns a
normal error when hooks are absent) instead of VaultHooks.ResolveString;
specifically update the EncryptionStatusVault case to call the validated
resolver helper with tx.Statement.Context and vs.Config and propagate/return the
error so AfterFind returns an error instead of panicking.
In `@transports/config.schema.json`:
- Around line 4985-4998: The conditional blocks in the "allOf" array only
require the matching backend object but don't forbid the others; update each
conditional (the ones matching "type": "aws-secrets-manager",
"gcp-secret-manager", and "hashicorp-vault") so their "then" not only requires
the correct backend property ("aws", "gcp", or "hashicorp") but also forbids the
other backend properties (e.g. when type is "aws-secrets-manager" the "then"
must forbid "gcp" and "hashicorp"). Modify the three conditionals in
transports/config.schema.json to add a negation (e.g. a "not" clause that
disallows the unwanted required properties) so backend objects are mutually
exclusive with respect to vault_store.type.
---
Duplicate comments:
In `@framework/configstore/tables/sessions.go`:
- Around line 61-67: The AfterDelete hook on SessionsTable
(SessionsTable.AfterDelete) cannot reliably remove vaulted session secrets
because rdb deletion paths use Delete(&tables.SessionsTable{}, ...) and
Delete(&tables.SessionsTable{}) which pass un-hydrated rows (no
TokenHash/EncryptionStatus) to the hook; as a result VaultHooks.Remove is called
with an incorrect path and secrets are orphaned. Fix by moving vault cleanup out
of SessionsTable.AfterDelete into the store layer (where rdb.go issues
deletions): before issuing Delete(&tables.SessionsTable{}, ...) or
Delete(&tables.SessionsTable{}), perform a SELECT to load the affected
SessionsTable rows within the same transaction, iterate those hydrated rows to
call VaultHooks.Remove(tx.Statement.Context,
vaultPrefix()+"/"+row.TableName()+"/"+row.TokenHash) for rows with
EncryptionStatus == EncryptionStatusVault, and only after successful vault
removals issue the DELETE for those rows in the same tx; remove or no-op the
current logic in SessionsTable.AfterDelete to avoid double/failed attempts.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a5d430a-0c04-47da-b76a-0e20c2c41781
📒 Files selected for processing (12)
framework/configstore/tables/encryption.goframework/configstore/tables/key.goframework/configstore/tables/mcp.goframework/configstore/tables/oauth.goframework/configstore/tables/plugin.goframework/configstore/tables/provider.goframework/configstore/tables/sessions.goframework/configstore/tables/temp_token.goframework/configstore/tables/vectorstore.goframework/configstore/tables/virtualkey.gotransports/bifrost-http/lib/config.gotransports/config.schema.json
BearTS
force-pushed
the
06-08-feat_vault_support
branch
3 times, most recently
from
June 8, 2026 21:03
6a204af to
8008b02
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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/vectorstore.go (1)
30-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClearing the vector-store config leaves the vaulted secret behind.
Lines 30-41 only handle the non-empty case. If
vs.Configwas previously vaulted and is later cleared, the row skips both branches, keeps its oldEncryptionStatus, and the secret atprefix/config_vector_store/configsurvives until the row is deleted.AfterDeletedoes not help for this update-to-empty transition.Treat the empty-string transition as a first-class path in
BeforeSave: remove the vault entry and reset the row back to plain text when config is cleared.Also applies to: 66-72
framework/configstore/tables/provider.go (1)
86-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClearing
ProxyConfigcurrently re-saves the old secret.Lines 86-92 only update
p.ProxyConfigJSONwhenp.ProxyConfig != nil. On an existing row, settingp.ProxyConfig = nilleaves the previous decrypted payload inp.ProxyConfigJSON, and Lines 120-135 vault/encrypt that stale value again. In practice, proxy config removal never takes effect, and the old vault entry is never cleaned up unless the whole row is deleted.Add an explicit nil/empty branch that clears
ProxyConfigJSON, resetsEncryptionStatus, and removes the current vault secret when the provider transitions to “no proxy config.”Also applies to: 120-135, 198-205
🤖 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/provider.go` around lines 86 - 92, The code only updates ProxyConfigJSON when p.ProxyConfig != nil, so clearing p.ProxyConfig leaves the old decrypted payload and re-encrypts it later; fix by adding an explicit else branch wherever ProxyConfig is marshaled (the shown block manipulating p.ProxyConfig, p.ProxyConfigJSON and p.EncryptionStatus) to: set p.ProxyConfigJSON = "" (or nil-equivalent), reset p.EncryptionStatus to the “no encryption” state, and call the vault-secret removal routine for this provider (use the existing vault client helper used elsewhere in this file) to delete the current secret; apply the same change to the other analogous blocks referenced (lines 120-135 and 198-205) so transitions to “no proxy config” actually clear stored JSON and vault entries.framework/configstore/tables/plugin.go (1)
51-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset vault state when a plugin config is cleared.
Line 52 normalizes a cleared config to
"{}", but Lines 56-69 skip both persistence branches without resettingEncryptionStatus. If this row was previously vault-backed, the save writesConfigJSON="{}"withEncryptionStatus="vault", and Lines 78-83 will try to resolve"{}"as a vault reference on the next read. The old secret also stays in vault because cleanup only exists inAfterDelete.Handle the empty/
"{}"transition explicitly inBeforeSave: remove the current vault entry (when applicable) and reset the row back to plain text. Based on learnings:"{}"inframework/configstore/tables/plugin.gois the intentional “no config” sentinel and should bypass vault/encryption.Also applies to: 78-83, 105-113
🤖 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/plugin.go` around lines 51 - 69, Detect the "no config" sentinel (p.ConfigJSON == "" or p.ConfigJSON == "{}") in the BeforeSave hook and, if the current row is marked as vault-backed (p.EncryptionStatus == EncryptionStatusVault), compute the same vault path used earlier (using vaultPrefix(), p.TableName(), p.Name and the field name via tx.Statement.DB.NamingStrategy.ColumnName("", "ConfigJSON")) and delete the secret from Vault (use your existing Vault helper used for vaultString or add a vault-delete helper), then set p.EncryptionStatus back to the plain state (e.g., EncryptionStatusPlain) and leave p.ConfigJSON as "{}"; also apply the same sentinel-shortcircuit to the other code paths mentioned (the vault/encrypt resolution logic around lines 78-83 and 105-113) so they skip any Vault/encryption resolution when ConfigJSON is "{}".Source: Learnings
♻️ Duplicate comments (4)
framework/configstore/tables/encryption.go (1)
38-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed when vault is configured but not actually wired.
vaultIsEnabled()now avoids the partial-hook path, but it still turns “vault selected in config, hooks missing at runtime” into a silent fallback. In this PR,vault_store.enabled=trueis accepted while OSSinitVaultremains a no-op stub, so returningfalsehere can send new secrets down the AES/plaintext path instead of aborting startup. Split “vault configured” from “vault ready” and make config load fail whenIsEnabled()is true butStoreString/ResolveStringare unset.As per coding guidelines,
vault_store.enabled=truemeans sensitive fields are expected to be stored in the external vault backend, and the PR objective notesinitVaultis currently a no-op stub in OSS.🤖 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/encryption.go` around lines 38 - 40, The current vaultIsEnabled() mixes "vault configured" with "vault hooks wired" and silently falls back when IsEnabled() is true but StoreString/ResolveString are nil; change vaultIsEnabled() to only return true when the vault is fully ready (VaultHooks.IsEnabled != nil && VaultHooks.IsEnabled() && VaultHooks.StoreString != nil && VaultHooks.ResolveString != nil), add a new vaultConfigured() helper that returns true when VaultHooks.IsEnabled != nil && VaultHooks.IsEnabled(), and update the configuration-load/startup code to fail hard (return an error or abort startup) when vaultConfigured() is true but vaultIsEnabled() is false so that a configured-but-unwired vault causes config load to fail rather than silently using AES/plaintext.Source: Coding guidelines
framework/configstore/tables/temp_token.go (1)
71-77:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBatch temp-token deletes can bypass the data this hook needs.
Line 71 assumes GORM is deleting a populated
TempToken, but directDelete(&tables.TempToken{}, ...)paths only give this hook a zero-value model. In that caseEncryptionStatusis empty andIDis unusable, so vault-backed temp tokens are orphaned on cleanup/expiry. The vault removal needs to happen in the store delete flow, or the store must delete hydratedTempTokenrows.#!/bin/bash set -euo pipefail echo "Temp token delete call sites:" rg -n -C3 --type=go 'Delete\(&tables\.TempToken\{\}|Delete\(&TempToken\{\}' framework echo echo "Relevant temp-token delete helpers in configstore:" rg -n -C5 --type=go 'TempToken|delete temp token|DeleteTempToken|cleanup expired temp' framework/configstore/rdb.go framework/configstoreExpected result: delete flows should either select affected
TempTokenrows before deleting, or explicitly remove vault secrets in the same transaction. Any batch delete againstTempToken{}leaves this hook without the row data required to build the vault path.🤖 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/temp_token.go` around lines 71 - 77, The AfterDelete hook on TempToken (func (t *TempToken) AfterDelete) cannot rely on model fields during batch deletes because GORM may pass a zero-value model; change the delete flow so vault secrets are removed with real row data: either (A) modify the store delete helpers (e.g., the TempToken cleanup/delete functions in configstore/rdb.go) to SELECT the affected TempToken rows first and then delete them within the same transaction while calling VaultHooks.Remove(tx.Statement.Context, path) for each token using the populated ID and EncryptionStatus, or (B) move the vault-removal logic out of AfterDelete and into those store delete functions so they explicitly build the vault path and call VaultHooks.Remove before or as part of the SQL delete; ensure you preserve transactional context (use tx) and check EncryptionStatus == EncryptionStatusVault before calling VaultHooks.Remove.framework/configstore/tables/sessions.go (1)
61-67:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
AfterDeleteonly works if the caller deletes a hydrated session row.Line 61 can only clean up the vault entry when GORM invokes this hook with a populated
SessionsTable. If the store layer still deletes sessions withDelete(&tables.SessionsTable{}, ...),EncryptionStatusandTokenHashare zero values here, so vaulted secrets are left behind. Please move the vault removal into the delete transaction in the store layer, or delete a loadedSessionsTableinstance instead.#!/bin/bash set -euo pipefail echo "Session delete call sites:" rg -n -C3 --type=go 'Delete\(&tables\.SessionsTable\{\}|Delete\(&SessionsTable\{\}' framework echo echo "Relevant session delete helpers in configstore:" rg -n -C5 --type=go 'SessionsTable|delete session|DeleteSession' framework/configstore/rdb.go framework/configstoreExpected result: session deletes should first load a
SessionsTablerow (or explicitly remove the vault secret in the store layer) before deleting. Any directDelete(&tables.SessionsTable{}, ...)path leaves this hook without theTokenHashit needs.🤖 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/sessions.go` around lines 61 - 67, The AfterDelete hook on SessionsTable (method AfterDelete) relies on populated fields (EncryptionStatus, TokenHash) so it only removes vault secrets when GORM calls it with a hydrated SessionsTable; callers that call Delete(&tables.SessionsTable{}, ...) pass zero-valued fields and leave vault entries behind. Fix by changing session deletion in the store layer: either (A) load the SessionsTable row first (using the store's Get/Find by ID or token) and then Delete(theLoadedSession) inside the same transaction so AfterDelete has TokenHash/EncryptionStatus, or (B) move the vault-removal logic into the store-layer delete function (invoke VaultHooks.Remove(tx.Statement.Context, path) with the correct path computed from the loaded row) so that VaultHooks.Remove is always called within the delete transaction before issuing Delete(&SessionsTable{}). Ensure the chosen change references SessionsTable, AfterDelete, EncryptionStatus, TokenHash and VaultHooks.Remove so vault secrets are reliably cleaned up.framework/configstore/tables/key.go (1)
333-404:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove vaulted key secrets when a field is cleared or switched away from vault storage.
vaultEnvVar/vaultStringskip nil, empty, and env-backed inputs, and this file only removes secrets on row deletion. Updating an existing vaulted key field tonil,"", orenv.*leaves the old secret at the deterministic vault path indefinitely, so rotated credentials keep lingering in the vault after the database row no longer references them.Also applies to: 775-796
🤖 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/key.go` around lines 333 - 404, The vault code only writes secrets via vaultEnvVar/vaultString and sets EncryptionStatusVault but never deletes old vault entries when a field is cleared or switched off vault storage; update the save/update logic (the block using base := fmt.Sprintf... with col(...) and the same logic around EncryptionStatusVault) to detect fields that were previously stored in vault and now are nil/empty or env.* (or EncryptionStatus changed away from EncryptionStatusVault) and call the vault deletion helper for that deterministic path (use the same col("FieldName") naming) for each affected field (Value, AzureEndpoint, AzureClientID, AzureClientSecret, AzureTenantID, Vertex*, Bedrock*, VLLMUrl, OllamaUrl, SGLUrl, AliasesJSON/BedrockBatchS3ConfigJSON), ensuring deletions occur before writing new values and when toggling EncryptionStatus.
🤖 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 `@framework/configstore/tables/virtualkey.go`:
- Around line 270-277: The code currently calls vaultString(...) and sets
vk.EncryptionStatus = EncryptionStatusVault before the DB transaction commits
(in the block in question), which can leave vault entries orphaned if the DB
rollbacks; remove the pre-commit vault write and instead perform the vault write
only after the DB commit using a transactional commit callback (e.g., GORM
AfterCommit/transaction commit hook) or a two-phase approach (write to a staged
path then atomically promote it on commit). Concretely: stop calling
vaultString(tx.Statement.Context, path, &vk.Value) and setting
EncryptionStatusVault inside the current pre-commit hook; compute the same path
(using vk.TableName(), vk.ID and the NamingStrategy column for "Value") but
register a post-commit action that performs vaultString(context, path,
&vk.Value) and on success sets/updates the row’s EncryptionStatus to
EncryptionStatusVault (via a safe follow-up update) or promotes the staged path
— ensure the post-commit action runs only after tx commit and handles
errors/retries.
- Around line 321-327: The AfterDelete hook is rebuilding the vault path from
current config which can differ from the original persisted reference; update
the TableVirtualKey model and hooks so the raw vault reference is preserved at
read time (e.g. add a field like RawVaultRef or VaultOriginalPath and set it
inside AfterFind when vk.Value is resolved) and then use that preserved field in
AfterDelete (in TableVirtualKey.AfterDelete) to call
VaultHooks.Remove(tx.Statement.Context, preservedPath) instead of recomputing
with vaultPrefix()/vk.TableName()/vk.ID/…; ensure nil/empty checks for the
preserved field and fall back safely if not set.
---
Outside diff comments:
In `@framework/configstore/tables/plugin.go`:
- Around line 51-69: Detect the "no config" sentinel (p.ConfigJSON == "" or
p.ConfigJSON == "{}") in the BeforeSave hook and, if the current row is marked
as vault-backed (p.EncryptionStatus == EncryptionStatusVault), compute the same
vault path used earlier (using vaultPrefix(), p.TableName(), p.Name and the
field name via tx.Statement.DB.NamingStrategy.ColumnName("", "ConfigJSON")) and
delete the secret from Vault (use your existing Vault helper used for
vaultString or add a vault-delete helper), then set p.EncryptionStatus back to
the plain state (e.g., EncryptionStatusPlain) and leave p.ConfigJSON as "{}";
also apply the same sentinel-shortcircuit to the other code paths mentioned (the
vault/encrypt resolution logic around lines 78-83 and 105-113) so they skip any
Vault/encryption resolution when ConfigJSON is "{}".
In `@framework/configstore/tables/provider.go`:
- Around line 86-92: The code only updates ProxyConfigJSON when p.ProxyConfig !=
nil, so clearing p.ProxyConfig leaves the old decrypted payload and re-encrypts
it later; fix by adding an explicit else branch wherever ProxyConfig is
marshaled (the shown block manipulating p.ProxyConfig, p.ProxyConfigJSON and
p.EncryptionStatus) to: set p.ProxyConfigJSON = "" (or nil-equivalent), reset
p.EncryptionStatus to the “no encryption” state, and call the vault-secret
removal routine for this provider (use the existing vault client helper used
elsewhere in this file) to delete the current secret; apply the same change to
the other analogous blocks referenced (lines 120-135 and 198-205) so transitions
to “no proxy config” actually clear stored JSON and vault entries.
---
Duplicate comments:
In `@framework/configstore/tables/encryption.go`:
- Around line 38-40: The current vaultIsEnabled() mixes "vault configured" with
"vault hooks wired" and silently falls back when IsEnabled() is true but
StoreString/ResolveString are nil; change vaultIsEnabled() to only return true
when the vault is fully ready (VaultHooks.IsEnabled != nil &&
VaultHooks.IsEnabled() && VaultHooks.StoreString != nil &&
VaultHooks.ResolveString != nil), add a new vaultConfigured() helper that
returns true when VaultHooks.IsEnabled != nil && VaultHooks.IsEnabled(), and
update the configuration-load/startup code to fail hard (return an error or
abort startup) when vaultConfigured() is true but vaultIsEnabled() is false so
that a configured-but-unwired vault causes config load to fail rather than
silently using AES/plaintext.
In `@framework/configstore/tables/key.go`:
- Around line 333-404: The vault code only writes secrets via
vaultEnvVar/vaultString and sets EncryptionStatusVault but never deletes old
vault entries when a field is cleared or switched off vault storage; update the
save/update logic (the block using base := fmt.Sprintf... with col(...) and the
same logic around EncryptionStatusVault) to detect fields that were previously
stored in vault and now are nil/empty or env.* (or EncryptionStatus changed away
from EncryptionStatusVault) and call the vault deletion helper for that
deterministic path (use the same col("FieldName") naming) for each affected
field (Value, AzureEndpoint, AzureClientID, AzureClientSecret, AzureTenantID,
Vertex*, Bedrock*, VLLMUrl, OllamaUrl, SGLUrl,
AliasesJSON/BedrockBatchS3ConfigJSON), ensuring deletions occur before writing
new values and when toggling EncryptionStatus.
In `@framework/configstore/tables/sessions.go`:
- Around line 61-67: The AfterDelete hook on SessionsTable (method AfterDelete)
relies on populated fields (EncryptionStatus, TokenHash) so it only removes
vault secrets when GORM calls it with a hydrated SessionsTable; callers that
call Delete(&tables.SessionsTable{}, ...) pass zero-valued fields and leave
vault entries behind. Fix by changing session deletion in the store layer:
either (A) load the SessionsTable row first (using the store's Get/Find by ID or
token) and then Delete(theLoadedSession) inside the same transaction so
AfterDelete has TokenHash/EncryptionStatus, or (B) move the vault-removal logic
into the store-layer delete function (invoke
VaultHooks.Remove(tx.Statement.Context, path) with the correct path computed
from the loaded row) so that VaultHooks.Remove is always called within the
delete transaction before issuing Delete(&SessionsTable{}). Ensure the chosen
change references SessionsTable, AfterDelete, EncryptionStatus, TokenHash and
VaultHooks.Remove so vault secrets are reliably cleaned up.
In `@framework/configstore/tables/temp_token.go`:
- Around line 71-77: The AfterDelete hook on TempToken (func (t *TempToken)
AfterDelete) cannot rely on model fields during batch deletes because GORM may
pass a zero-value model; change the delete flow so vault secrets are removed
with real row data: either (A) modify the store delete helpers (e.g., the
TempToken cleanup/delete functions in configstore/rdb.go) to SELECT the affected
TempToken rows first and then delete them within the same transaction while
calling VaultHooks.Remove(tx.Statement.Context, path) for each token using the
populated ID and EncryptionStatus, or (B) move the vault-removal logic out of
AfterDelete and into those store delete functions so they explicitly build the
vault path and call VaultHooks.Remove before or as part of the SQL delete;
ensure you preserve transactional context (use tx) and check EncryptionStatus ==
EncryptionStatusVault before calling VaultHooks.Remove.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f92812e-e072-4b7e-bde8-ca37cf5d9c28
📒 Files selected for processing (12)
framework/configstore/tables/encryption.goframework/configstore/tables/key.goframework/configstore/tables/mcp.goframework/configstore/tables/oauth.goframework/configstore/tables/plugin.goframework/configstore/tables/provider.goframework/configstore/tables/sessions.goframework/configstore/tables/temp_token.goframework/configstore/tables/vectorstore.goframework/configstore/tables/virtualkey.gotransports/bifrost-http/lib/config.gotransports/config.schema.json
BearTS
force-pushed
the
06-08-feat_vault_support
branch
6 times, most recently
from
June 9, 2026 08:28
6bab51f to
cd08b1c
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
BearTS
marked this pull request as ready for review
June 9, 2026 08:33
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
transports/go.mod (1)
134-134:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove unresolved merge-conflict marker in
go.mod(build blocker).Line 134 contains
<<<<<<< HEAD, which makestransports/go.modinvalid and will break module resolution/tooling (go mod tidy,go list, builds). Resolve the conflict and keep only the valid dependency entry.🤖 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 `@transports/go.mod` at line 134, Remove the unresolved merge marker "<<<<<<< HEAD" from the transports/go.mod dependency line so the file contains only the valid dependency entry for github.com/mattn/go-sqlite3 v1.14.32 (keeping the "// indirect" comment if appropriate); ensure there are no other conflict markers (e.g., >>>>>> or ======) left in go.mod and run go mod tidy to verify module resolution succeeds.framework/configstore/rdb.go (2)
5053-5072:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't fire vault cleanup from a caller-owned transaction.
When
txis supplied here, Lines 5070-5072 can run before the outer transaction commits. If that transaction later rolls back, the temp-token rows come back but their vault secrets are already gone. This cleanup needs to be triggered by the owner of the transaction after a successful commit, not from inside this helper.🤖 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 5053 - 5072, DeleteTempTokensByResourceID currently spawns DeleteVaultSecrets asynchronously inside the helper, which can run before an outer transaction commits; change the behavior so vault secret deletion is not triggered from inside this function when a caller-supplied tx is present: collect vaultIDs as you already do, but if len(tx) > 0 do not call tables.TempToken{}.DeleteVaultSecrets; instead return the vaultIDs (e.g. change DeleteTempTokensByResourceID to return ([]string, int64, error) or add an out parameter) so the transaction owner can call tables.TempToken{}.DeleteVaultSecrets after a successful commit; keep the existing immediate cleanup behavior only when no caller tx is provided.
1924-1988:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCapture and delete the same vault-backed rows.
These paths collect vault IDs in one statement and delete rows in a later statement. Under concurrent inserts/updates, the delete can remove rows that were never in the ID snapshot, so their vault secrets are never cleaned up. Delete by the collected primary keys (or use a delete-returning path) inside one transaction, and fail if the ID snapshot query fails.
Suggested pattern
- if err := db.WithContext(ctx).Model(&tables.TempToken{}). - Where("scope = ? AND resource_id = ? AND encryption_status = ?", scope, resourceID, tables.EncryptionStatusVault). - Pluck("id", &vaultIDs); err != nil { - // ignored - } - res := db.WithContext(ctx). - Where("scope = ? AND resource_id = ?", scope, resourceID). - Delete(&tables.TempToken{}) + err := db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { + if err := tx.Model(&tables.TempToken{}). + Where("scope = ? AND resource_id = ? AND encryption_status = ?", scope, resourceID, tables.EncryptionStatusVault). + Pluck("id", &vaultIDs).Error; err != nil { + return err + } + if len(vaultIDs) == 0 { + return nil + } + return tx.Where("id IN ?", vaultIDs).Delete(&tables.TempToken{}).Error + })Also applies to: 5058-5066, 5079-5085
🤖 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 1924 - 1988, The code currently snapshots vault-backed IDs outside the transaction then deletes rows later, which can miss newly-inserted rows and leave vault secrets uncleared; move the ID collection into the same transaction and fail on any snapshot error, then delete rows by those collected primary keys inside the transaction. Concretely: inside the Transaction(func(tx *gorm.DB) error { ... }) before deleting TableOauthUserToken/TableOauthUserSession/TableMCPPerUserHeaderCredential, run tx.WithContext(ctx).Select("id").Where("mcp_client_id = ? AND encryption_status = ?", existingClient.ClientID, tables.EncryptionStatusVault).Find(&tokens).Error (and equivalent for sessions/creds) and return the error if non-nil, populate vaultTokenIDs/vaultSessionIDs/vaultCredIDs from those results, then perform deletes using "id IN ?" against tx (e.g., tx.Where("id IN ?", vaultTokenIDs).Delete(&tables.TableOauthUserToken{})) instead of deleting by mcp_client_id; keep the post-transaction vault cleanup using the collected ID slices.framework/configstore/tables/plugin.go (1)
56-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset encryption status when persisting plaintext config.
When Line 56-Line 70 skip both vault and AES branches (
""/"{}"or protection disabled),p.EncryptionStatuskeeps its previous value. That can routeAfterFindinto vault/decrypt logic for plaintext JSON on later reads.Suggested fix
} else { p.ConfigJSON = "{}" } + p.EncryptionStatus = "plain_text" // Encrypt config after serialization if vaultIsEnabled() && p.ConfigJSON != "" && p.ConfigJSON != "{}" {🤖 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/plugin.go` around lines 56 - 70, When neither the vault branch nor the AES branch runs, p.EncryptionStatus is left unchanged causing stale values to trigger decryption on later reads; update the logic in the plugin persistence code (the block that checks vaultIsEnabled(), encrypt.IsEnabled(), and p.ConfigJSON) to explicitly set p.EncryptionStatus = EncryptionStatusPlain in the fallback case (i.e., when you skip both vault and encrypt branches or when ConfigJSON is "" or "{}"). Ensure you set this after deciding not to vault or encrypt so the AfterFind/decrypt logic won't run erroneously; keep references to p.ConfigJSON, p.EncryptionStatus, EncryptionStatusVault and EncryptionStatusEncrypted to locate the code.
🤖 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 `@framework/configstore/rdb.go`:
- Around line 2573-2580: The current DeletePlugin logic swallows a not-found
record by returning nil; change it to return the canonical ErrNotFound so the
transport layer can map to HTTP 404. In the function that loads and deletes the
plugin (using tables.TablePlugin and txDB.WithContext), keep the existing
"First(&plugin)" check and hydrated delete, but when errors.Is(err,
gorm.ErrRecordNotFound) is true return ErrNotFound instead of nil; leave other
error returns and the final txDB.WithContext(...).Delete(&plugin).Error
unchanged.
In `@framework/configstore/tables/encryption.go`:
- Around line 50-59: Add a companion sync helper to vaultEnvVar (e.g.,
vaultSyncEnvVar) that handles both storing and removing vault entries: if field
is nil, env-backed, or empty call VaultHooks.DeleteString (no-op if nil) to
remove the deterministic path; otherwise call VaultHooks.StoreString as
vaultEnvVar currently does; then replace BeforeSave callers that currently use
vaultEnvVar with this new vaultSyncEnvVar so cleared/rotated secrets remove
their vault entry (also apply same change for the other similar block around
lines 74-83).
In `@framework/configstore/tables/key.go`:
- Around line 333-336: The vault path is using the auto-increment k.ID which is
unset during BeforeSave, causing new keys to write to .../0/...; change the path
construction in the vault-related code (where vaultIsEnabled(), vaultPrefix(),
and col are used) to use k.KeyID instead of k.ID, and make the same replacement
in the other vault handling block (the code referenced around the cleanup logic
that currently uses k.ID) so all vault writes/reads use the stable KeyID
identifier.
In `@framework/configstore/tables/plugin.go`:
- Around line 56-59: The vault path is unstable and incorrectly formatted:
BeforeSave may run with p.ID == 0 (so create-time path differs from delete-time
path) and the code formats p.ID with "%s" which is wrong for uint; change vault
writes to occur after the DB has a stable primary key (move vaultString call
from BeforeSave to AfterCreate and perform cleanup in AfterDelete) and format
the numeric ID correctly (use "%d" or strconv.FormatUint(uint64(p.ID), 10) when
building path). Update the functions that build the path (where vaultPrefix(),
p.TableName(), p.Name, p.ID, and fieldName are used) to use the corrected
numeric formatting and run writes in AfterCreate so the same ID is used on
delete in AfterDelete.
In `@framework/configstore/tables/provider.go`:
- Line 123: The fmt.Sprintf call building path uses %s for p.ID which is a uint,
causing formatted output like %!s(uint=5); update the format to use the correct
specifier (e.g., %d or %v) or explicitly convert p.ID to a string
(strconv.FormatUint/FormatUint(uint64(p.ID), 10)) in the path construction so
that vaultPrefix(), p.TableName(), p.Name, p.ID and fieldName produce a valid
string path.
- Line 204: The path formatting in AfterDelete uses fmt.Sprintf with "%s" for
p.ID which is a uint and causes malformed output; update the format to use a
numeric verb (e.g., "%d") or use "%v" and/or explicitly cast p.ID to an
int/uint64 so the constructed path is valid. Locate the line in AfterDelete
where path := fmt.Sprintf("%s/%s/%s/%s/%s", vaultPrefix(), p.TableName(),
p.Name, p.ID, fieldName) and replace the format specifier for p.ID accordingly
while keeping vaultPrefix(), p.TableName(), p.Name and fieldName as strings.
In `@framework/configstore/tables/vectorstore.go`:
- Around line 65-73: The AfterDelete hook on TableVectorStoreConfig is
insufficient because UpdateVectorStoreConfig uses
Delete(&tables.TableVectorStoreConfig{}) (batch/global delete) which won't
hydrate rows and thus skips VaultHooks.Remove; modify the store-layer delete
path (the UpdateVectorStoreConfig flow) to first select/fetch the affected
TableVectorStoreConfig rows (or otherwise load hydrated instances) and then
delete them individually (or explicitly call VaultHooks.Remove per-row using the
same path construction logic) so vault cleanup always runs deterministically;
ensure changes reference TableVectorStoreConfig, AfterDelete,
UpdateVectorStoreConfig, and VaultHooks.Remove so secret cleanup semantics are
preserved across persistence flows.
- Around line 30-33: The BeforeSave hook builds a vault path using vs.ID which
is zero for new rows, causing stored secrets under "/.../0/..." and later
cleanup (e.g., in AfterDelete) to miss them; change the vault path construction
in BeforeSave (and any other places like the AfterDelete/cleanup code around
vaultDelete usage) to use a stable table-scoped key instead of vs.ID — for
example build path using vaultPrefix(), vs.TableName(), and the fieldName (from
tx.Statement.DB.NamingStrategy.ColumnName("", "Config")) so vaultString and
vaultDelete operate on the same stable path; update all occurrences where vs.ID
is included (BeforeSave and the code around lines handling vaultDelete) to
remove vs.ID and use the table-scoped path.
---
Outside diff comments:
In `@framework/configstore/rdb.go`:
- Around line 5053-5072: DeleteTempTokensByResourceID currently spawns
DeleteVaultSecrets asynchronously inside the helper, which can run before an
outer transaction commits; change the behavior so vault secret deletion is not
triggered from inside this function when a caller-supplied tx is present:
collect vaultIDs as you already do, but if len(tx) > 0 do not call
tables.TempToken{}.DeleteVaultSecrets; instead return the vaultIDs (e.g. change
DeleteTempTokensByResourceID to return ([]string, int64, error) or add an out
parameter) so the transaction owner can call
tables.TempToken{}.DeleteVaultSecrets after a successful commit; keep the
existing immediate cleanup behavior only when no caller tx is provided.
- Around line 1924-1988: The code currently snapshots vault-backed IDs outside
the transaction then deletes rows later, which can miss newly-inserted rows and
leave vault secrets uncleared; move the ID collection into the same transaction
and fail on any snapshot error, then delete rows by those collected primary keys
inside the transaction. Concretely: inside the Transaction(func(tx *gorm.DB)
error { ... }) before deleting
TableOauthUserToken/TableOauthUserSession/TableMCPPerUserHeaderCredential, run
tx.WithContext(ctx).Select("id").Where("mcp_client_id = ? AND encryption_status
= ?", existingClient.ClientID, tables.EncryptionStatusVault).Find(&tokens).Error
(and equivalent for sessions/creds) and return the error if non-nil, populate
vaultTokenIDs/vaultSessionIDs/vaultCredIDs from those results, then perform
deletes using "id IN ?" against tx (e.g., tx.Where("id IN ?",
vaultTokenIDs).Delete(&tables.TableOauthUserToken{})) instead of deleting by
mcp_client_id; keep the post-transaction vault cleanup using the collected ID
slices.
In `@framework/configstore/tables/plugin.go`:
- Around line 56-70: When neither the vault branch nor the AES branch runs,
p.EncryptionStatus is left unchanged causing stale values to trigger decryption
on later reads; update the logic in the plugin persistence code (the block that
checks vaultIsEnabled(), encrypt.IsEnabled(), and p.ConfigJSON) to explicitly
set p.EncryptionStatus = EncryptionStatusPlain in the fallback case (i.e., when
you skip both vault and encrypt branches or when ConfigJSON is "" or "{}").
Ensure you set this after deciding not to vault or encrypt so the
AfterFind/decrypt logic won't run erroneously; keep references to p.ConfigJSON,
p.EncryptionStatus, EncryptionStatusVault and EncryptionStatusEncrypted to
locate the code.
In `@transports/go.mod`:
- Line 134: Remove the unresolved merge marker "<<<<<<< HEAD" from the
transports/go.mod dependency line so the file contains only the valid dependency
entry for github.com/mattn/go-sqlite3 v1.14.32 (keeping the "// indirect"
comment if appropriate); ensure there are no other conflict markers (e.g.,
>>>>>> or ======) left in go.mod and run go mod tidy to verify module resolution
succeeds.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce84bb86-b084-47bb-9c26-b8ad379a6e86
⛔ Files ignored due to path filters (14)
core/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/compat/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/prompts/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sumui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
core/go.modframework/configstore/rdb.goframework/configstore/tables/encryption.goframework/configstore/tables/key.goframework/configstore/tables/mcp.goframework/configstore/tables/mcp_per_user_headers.goframework/configstore/tables/oauth.goframework/configstore/tables/plugin.goframework/configstore/tables/provider.goframework/configstore/tables/sessions.goframework/configstore/tables/temp_token.goframework/configstore/tables/vectorstore.goframework/configstore/tables/virtualkey.goframework/go.modplugins/compat/go.modplugins/governance/go.modplugins/jsonparser/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/prompts/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtransports/bifrost-http/lib/config.gotransports/config.schema.jsontransports/go.mod
BearTS
force-pushed
the
06-08-feat_vault_support
branch
from
June 9, 2026 08:51
cd08b1c to
43c3d9d
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
BearTS
force-pushed
the
06-08-feat_vault_support
branch
from
June 9, 2026 09:05
43c3d9d to
13b2174
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
transports/go.mod (1)
134-134:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve leftover merge-conflict marker in
go.mod.Line 134 includes
<<<<<<< HEAD, which makestransports/go.modsyntactically invalid and will break Go module tooling.Suggested fix
- github.com/mattn/go-sqlite3 v1.14.32 // indirect; indirect<<<<<<< HEAD + github.com/mattn/go-sqlite3 v1.14.32 // indirect🤖 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 `@transports/go.mod` at line 134, Remove the leftover merge-conflict marker '<<<<<<< HEAD' that appears on the dependency line for github.com/mattn/go-sqlite3 v1.14.32 // indirect; ensure the line contains only the valid module requirement 'github.com/mattn/go-sqlite3 v1.14.32 // indirect' (no conflict markers or extra text) and then run go mod tidy / verify the module file is syntactically valid to restore proper Go tooling behavior.framework/configstore/tables/mcp_per_user_headers.go (1)
107-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClearing per-user headers leaves the old vaulted values behind.
If
HeadersJSONbecomes{}or the row is re-saved after vault mode is disabled, this hook stops callingvaultStringbut never removes the prior secret at the same path.AfterDeleteonly handles full-row cleanup, so routine per-user header rotations keep stale credentials in the vault.🤖 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_per_user_headers.go` around lines 107 - 119, When HeadersJSON is cleared or vault mode is turned off we must remove the stale secret from the vault; modify the hook that currently checks VaultIsEnabled() and c.HeadersJSON to also detect the case where previously c.EncryptionStatus == EncryptionStatusVault but now either c.HeadersJSON == "{}" or VaultIsEnabled() is false, and in that branch call the vault delete operation for the same path you build with VaultPrefix()/c.TableName()/c.ID/NamingStrategy.ColumnName("", "HeadersJSON") (using tx.Statement.Context) and set c.EncryptionStatus to the non‑vault state; ensure symmetric behavior when switching from vault to encrypt mode (remove old vault secret) and when clearing headers so vaultString (and the vault delete function) are invoked appropriately instead of leaving secrets behind.
♻️ Duplicate comments (1)
framework/configstore/tables/oauth.go (1)
50-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRetired OAuth secrets are still left behind in vault on update.
These branches only store when the new value is still vault-worthy. If
ClientSecret,CodeVerifier, orRefreshTokenis cleared, switched to env-backed storage, or re-saved after vault mode is disabled, the old secret stays at the same deterministic vault path because there is no matching best-effortRemoveon the skip/fallback path. The newAfterDeletehooks only cover full row deletion, so normal rotations still accumulate stale OAuth material.Also applies to: 167-177, 271-278, 368-377
🤖 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/oauth.go` around lines 50 - 70, The branches under VaultIsEnabled() (in functions using vaultEnvVar and vaultString and setting EncryptionStatusVault) only write new secrets but do not remove old ones; modify the update/save logic to perform a best-effort vault removal (call the vault remove routine for the same deterministic path) whenever a secret is no longer being vaulted — e.g., when ClientSecret is nil or FromEnv is true or Val is empty, when CodeVerifier becomes empty, or when RefreshToken is no longer vaulted — before skipping the vault write, and do the same in the mirrored update branches mentioned (the other blocks using vaultEnvVar/vaultString and EncryptionStatusVault and the AfterDelete hooks) so stale entries are removed from the vault on rotations/disablement. Ensure you build the same path using VaultPrefix(), TableName(), c.ID and the column name via tx.Statement.DB.NamingStrategy.ColumnName("", "<FieldName>") and call the vault remove helper in those skip/fallback paths.
🤖 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 `@framework/configstore/rdb.go`:
- Around line 1831-1847: The code is mutating external vault state
(VaultHooks.StoreString/Remove) before the DB transaction commits (before
calling Updates(updates)), which can leave DB and vault inconsistent; change the
flow so vault mutations for headersPath and connPath are executed only after the
SQL transaction successfully commits (or implement compensating rollback that
reverts vault changes if Updates fails), ensure you call
VaultHooks.StoreString/Remove after the successful return of the function that
performs Updates(updates) (or register commit/rollback callbacks around the
transaction), and add explicit error handling for VaultHooks.StoreString,
VaultHooks.Remove and any rollback logic; apply the same fix to the other
occurrence around the encrypt.IsEnabled() branch (lines ~1896-1906) and ensure
vault hooks are not called pre-commit.
- Around line 5087-5101: The code in DeleteTempTokensByResourceID currently
launches a goroutine to call tables.TempToken{}.DeleteVaultSecrets with vaultIDs
while still inside the caller-owned transaction; instead, stop performing async
vault cleanup here — collect and return the vaultIDs (or expose them via a
dedicated return value) so the caller can invoke
tables.TempToken{}.DeleteVaultSecrets only after committing the outer
transaction, and ensure that call is performed with explicit error handling (no
silent goroutine). Update the function signature/return to include the vaultIDs
(or a post-commit cleanup callback) and remove the immediate go
tables.TempToken{}.DeleteVaultSecrets(...) invocation inside this function.
- Around line 5407-5416: The delete call is passing a value (existing) which
prevents GORM from invoking TableOauthToken's pointer receiver hook AfterDelete
(so VaultHooks.Remove may not run); change the deletion to pass a pointer to the
hydrated struct (use &existing) and propagate/check the returned result error
(i.e., call s.DB().WithContext(ctx).Delete(&existing) and handle result.Error)
so AfterDelete executes and vault cleanup runs.
In `@framework/configstore/tables/key.go`:
- Around line 333-428: When VaultIsEnabled() is false and you take the
AES/plaintext branch (the else if encrypt.IsEnabled() path), add a best-effort
cleanup that removes any existing vault entries if the row's prior encryption
status was EncryptionStatusVault; detect the prior status (the pre-save/original
value of k.EncryptionStatus from the GORM statement/context) and call the same
removal helpers used above (removeVaultEnvVar/removeVaultString for the same
columns: Value, AzureEndpoint, AzureClientID, AzureClientSecret, AzureTenantID,
VertexProjectID, VertexProjectNumber, VertexRegion, VertexAuthCredentials,
BedrockAccessKey, BedrockSecretKey, BedrockSessionToken, BedrockRegion,
BedrockARN, BedrockRoleARN, BedrockExternalID, BedrockRoleSessionName,
BedrockBatchS3ConfigJSON, AliasesJSON, VLLMUrl, OllamaUrl, SGLUrl) before
writing plaintext/AES values so old vault paths are deleted when switching out
of vault mode.
In `@framework/configstore/tables/mcp.go`:
- Around line 197-221: When VaultIsEnabled() has flipped false we must remove
any existing vault entries for this MCP before falling back to DB storage; add
logic (before entering the encrypt.IsEnabled() branch) that checks for a prior
vault state (e.g., c.EncryptionStatus == EncryptionStatusVault or the row’s
stored status) and constructs the same connPath and headersPath used above and
calls removeVaultEnvVar(tx.Statement.Context, connPath, ...) and
removeVaultString(tx.Statement.Context, headersPath, ...) to delete stale
secrets regardless of current c.ConnectionString or c.HeadersJSON values; keep
using tx.Statement.Context and the same NamingStrategy.ColumnName calls so paths
match the ones created when Vault was enabled.
In `@framework/configstore/tables/vectorstore.go`:
- Around line 54-59: Redundant nil/empty check: inside the
EncryptionStatusEncrypted case remove the inner guard and directly call
decryptString on vs.Config (i.e., replace the block that checks "if vs.Config !=
nil && *vs.Config != \"\"" with a direct call), keeping the existing error
handling (return fmt.Errorf("failed to decrypt vector store config: %w", err))
so that decryptString(vs.Config) is invoked when the switch reaches
EncryptionStatusEncrypted.
---
Outside diff comments:
In `@framework/configstore/tables/mcp_per_user_headers.go`:
- Around line 107-119: When HeadersJSON is cleared or vault mode is turned off
we must remove the stale secret from the vault; modify the hook that currently
checks VaultIsEnabled() and c.HeadersJSON to also detect the case where
previously c.EncryptionStatus == EncryptionStatusVault but now either
c.HeadersJSON == "{}" or VaultIsEnabled() is false, and in that branch call the
vault delete operation for the same path you build with
VaultPrefix()/c.TableName()/c.ID/NamingStrategy.ColumnName("", "HeadersJSON")
(using tx.Statement.Context) and set c.EncryptionStatus to the non‑vault state;
ensure symmetric behavior when switching from vault to encrypt mode (remove old
vault secret) and when clearing headers so vaultString (and the vault delete
function) are invoked appropriately instead of leaving secrets behind.
In `@transports/go.mod`:
- Line 134: Remove the leftover merge-conflict marker '<<<<<<< HEAD' that
appears on the dependency line for github.com/mattn/go-sqlite3 v1.14.32 //
indirect; ensure the line contains only the valid module requirement
'github.com/mattn/go-sqlite3 v1.14.32 // indirect' (no conflict markers or extra
text) and then run go mod tidy / verify the module file is syntactically valid
to restore proper Go tooling behavior.
---
Duplicate comments:
In `@framework/configstore/tables/oauth.go`:
- Around line 50-70: The branches under VaultIsEnabled() (in functions using
vaultEnvVar and vaultString and setting EncryptionStatusVault) only write new
secrets but do not remove old ones; modify the update/save logic to perform a
best-effort vault removal (call the vault remove routine for the same
deterministic path) whenever a secret is no longer being vaulted — e.g., when
ClientSecret is nil or FromEnv is true or Val is empty, when CodeVerifier
becomes empty, or when RefreshToken is no longer vaulted — before skipping the
vault write, and do the same in the mirrored update branches mentioned (the
other blocks using vaultEnvVar/vaultString and EncryptionStatusVault and the
AfterDelete hooks) so stale entries are removed from the vault on
rotations/disablement. Ensure you build the same path using VaultPrefix(),
TableName(), c.ID and the column name via
tx.Statement.DB.NamingStrategy.ColumnName("", "<FieldName>") and call the vault
remove helper in those skip/fallback paths.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2324707e-674d-41f4-9279-dc3148fc24db
⛔ Files ignored due to path filters (14)
core/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/compat/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/prompts/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sumui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
core/go.modframework/configstore/rdb.goframework/configstore/tables/encryption.goframework/configstore/tables/key.goframework/configstore/tables/mcp.goframework/configstore/tables/mcp_per_user_headers.goframework/configstore/tables/oauth.goframework/configstore/tables/plugin.goframework/configstore/tables/provider.goframework/configstore/tables/sessions.goframework/configstore/tables/temp_token.goframework/configstore/tables/vectorstore.goframework/configstore/tables/virtualkey.goframework/go.modplugins/compat/go.modplugins/governance/go.modplugins/jsonparser/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/prompts/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtransports/bifrost-http/lib/config.gotransports/config.schema.jsontransports/go.mod
BearTS
force-pushed
the
06-08-feat_vault_support
branch
2 times, most recently
from
June 9, 2026 09:46
8fe8020 to
cc73d6d
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
akshaydeo
reviewed
Jun 9, 2026
akshaydeo
reviewed
Jun 9, 2026
BearTS
force-pushed
the
06-08-feat_vault_support
branch
from
June 9, 2026 10:03
cc73d6d to
e110fd1
Compare
BearTS
force-pushed
the
06-08-feat_vault_support
branch
from
June 9, 2026 11:31
e110fd1 to
4221dea
Compare
BearTS
force-pushed
the
06-08-feat_vault_support
branch
from
June 9, 2026 14:28
4221dea to
4fbbb09
Compare
Contributor
Merge activity
|
Merged
18 tasks
akshaydeo
pushed a commit
that referenced
this pull request
Jun 12, 2026
… `hashicorp-vault`) as alternative to AES encryption for sensitive config fields (#4157) ## Summary Adds a vault backend integration layer to the configstore tables, enabling sensitive fields (API keys, credentials, tokens) to be stored in an external secret manager instead of AES-encrypted in the database. This introduces a new `vault` encryption status alongside the existing `plain_text` and `encrypted` statuses, with the vault implementation itself deferred to the enterprise layer via function pointer hooks. ## Changes - Introduced `EncryptionStatusVault = "vault"` as a third encryption status constant. - Added a `VaultHooks` struct in `encryption.go` with pluggable function pointers (`IsEnabled`, `Prefix`, `StoreString`, `ResolveString`, `Remove`) that the enterprise layer populates at startup. - Added helper functions (`vaultEnvVar`, `resolveVaultEnvVar`, `vaultString`, `resolveVaultString`) that guard against nil/empty values and missing hooks before delegating to vault operations. - Updated `BeforeSave` hooks across all configstore tables (`key`, `mcp`, `oauth`, `plugin`, `provider`, `sessions`, `temp_token`, `vectorstore`, `virtualkey`) to check vault first, fall through to AES encryption, and set the appropriate `EncryptionStatus`. - Updated `AfterFind` hooks to switch on `EncryptionStatus`, resolving vault references when `vault` and decrypting when `encrypted`, preserving backward compatibility with existing AES-encrypted rows. - Added `AfterDelete` hooks to all affected tables for best-effort vault secret cleanup when a row is deleted. - Added `VaultStoreConfig` struct to `config.go` and a `vault_store` key to `ConfigData`, allowing the config file to carry vault backend settings that the enterprise layer consumes. - Added an `initVault` no-op stub in `config.go` that logs when vault config is present but defers actual initialization to the enterprise layer. - Extended `config.schema.json` with a `vault_config` definition covering `aws-secrets-manager`, `gcp-secret-manager`, and `hashicorp-vault` backends, including all relevant credential and configuration fields. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/configstore/... go test ./transports/bifrost-http/... go build ./... ``` To validate vault path construction and hook dispatch, wire up stub implementations of `VaultHooks` in a test and perform create/read/delete operations on any configstore table. Confirm that: - With `VaultHooks.IsEnabled` returning `true`, `EncryptionStatus` is set to `vault` on save and secrets are passed to `StoreString`. - On `AfterFind`, `ResolveString` is called for rows with `EncryptionStatus = vault`. - On `AfterDelete`, `Remove` is called for each vaulted field path. - Rows previously saved with `EncryptionStatus = encrypted` continue to decrypt correctly via the AES path. **New config key:** ```json { "vault_store": { "enabled": true, "type": "aws-secrets-manager", "prefix": "bifrost", "aws": { "region": "us-east-1", "role_arn": "arn:aws:iam::123456789012:role/bifrost-vault" } } } ``` Supported types: `aws-secrets-manager`, `gcp-secret-manager`, `hashicorp-vault`. ## Breaking changes - [ ] Yes - [x] No Existing AES-encrypted rows are unaffected. Vault is only activated when `VaultHooks.IsEnabled` returns `true`, which requires the enterprise layer to populate the hooks. ## Security considerations - Sensitive fields (API keys, OAuth secrets, session tokens, credentials) are no longer written to the database in AES-encrypted form when vault is active; only vault reference strings are persisted. - Vault hook function pointers are package-level globals populated at startup; callers must ensure they are set before any database operations occur when vault is enabled. - `AfterDelete` cleanup is best-effort — vault removal errors are intentionally swallowed to avoid blocking row deletion. Operators should audit vault paths independently if hard deletion guarantees are required. - The `vault_store` config block may contain credential fields; these should be supplied via `env.VAR_NAME` references rather than inline values. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Optional external vault support for storing sensitive config with AWS/GCP/HashiCorp backends; runtime gate with graceful fallback to existing encryption. * Per-row vault-backed storage and retrieval for keys, providers, plugins, MCP, OAuth, sessions, tokens, vector stores, virtual keys, and per-user headers. * Best-effort automatic cleanup of vault secrets on row deletion and batch deletes. * **Documentation** * Config schema and loader updated with a top-level vault_store section and backend-specific options (optional prefix). * **Chores** * Bumped several indirect Go dependency versions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Jun 15, 2026
akhsaul
pushed a commit
to akhsaul/bifrost
that referenced
this pull request
Aug 27, 2026
… `hashicorp-vault`) as alternative to AES encryption for sensitive config fields (maximhq#4157) ## Summary Adds a vault backend integration layer to the configstore tables, enabling sensitive fields (API keys, credentials, tokens) to be stored in an external secret manager instead of AES-encrypted in the database. This introduces a new `vault` encryption status alongside the existing `plain_text` and `encrypted` statuses, with the vault implementation itself deferred to the enterprise layer via function pointer hooks. ## Changes - Introduced `EncryptionStatusVault = "vault"` as a third encryption status constant. - Added a `VaultHooks` struct in `encryption.go` with pluggable function pointers (`IsEnabled`, `Prefix`, `StoreString`, `ResolveString`, `Remove`) that the enterprise layer populates at startup. - Added helper functions (`vaultEnvVar`, `resolveVaultEnvVar`, `vaultString`, `resolveVaultString`) that guard against nil/empty values and missing hooks before delegating to vault operations. - Updated `BeforeSave` hooks across all configstore tables (`key`, `mcp`, `oauth`, `plugin`, `provider`, `sessions`, `temp_token`, `vectorstore`, `virtualkey`) to check vault first, fall through to AES encryption, and set the appropriate `EncryptionStatus`. - Updated `AfterFind` hooks to switch on `EncryptionStatus`, resolving vault references when `vault` and decrypting when `encrypted`, preserving backward compatibility with existing AES-encrypted rows. - Added `AfterDelete` hooks to all affected tables for best-effort vault secret cleanup when a row is deleted. - Added `VaultStoreConfig` struct to `config.go` and a `vault_store` key to `ConfigData`, allowing the config file to carry vault backend settings that the enterprise layer consumes. - Added an `initVault` no-op stub in `config.go` that logs when vault config is present but defers actual initialization to the enterprise layer. - Extended `config.schema.json` with a `vault_config` definition covering `aws-secrets-manager`, `gcp-secret-manager`, and `hashicorp-vault` backends, including all relevant credential and configuration fields. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/configstore/... go test ./transports/bifrost-http/... go build ./... ``` To validate vault path construction and hook dispatch, wire up stub implementations of `VaultHooks` in a test and perform create/read/delete operations on any configstore table. Confirm that: - With `VaultHooks.IsEnabled` returning `true`, `EncryptionStatus` is set to `vault` on save and secrets are passed to `StoreString`. - On `AfterFind`, `ResolveString` is called for rows with `EncryptionStatus = vault`. - On `AfterDelete`, `Remove` is called for each vaulted field path. - Rows previously saved with `EncryptionStatus = encrypted` continue to decrypt correctly via the AES path. **New config key:** ```json { "vault_store": { "enabled": true, "type": "aws-secrets-manager", "prefix": "bifrost", "aws": { "region": "us-east-1", "role_arn": "arn:aws:iam::123456789012:role/bifrost-vault" } } } ``` Supported types: `aws-secrets-manager`, `gcp-secret-manager`, `hashicorp-vault`. ## Breaking changes - [ ] Yes - [x] No Existing AES-encrypted rows are unaffected. Vault is only activated when `VaultHooks.IsEnabled` returns `true`, which requires the enterprise layer to populate the hooks. ## Security considerations - Sensitive fields (API keys, OAuth secrets, session tokens, credentials) are no longer written to the database in AES-encrypted form when vault is active; only vault reference strings are persisted. - Vault hook function pointers are package-level globals populated at startup; callers must ensure they are set before any database operations occur when vault is enabled. - `AfterDelete` cleanup is best-effort — vault removal errors are intentionally swallowed to avoid blocking row deletion. Operators should audit vault paths independently if hard deletion guarantees are required. - The `vault_store` config block may contain credential fields; these should be supplied via `env.VAR_NAME` references rather than inline values. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Optional external vault support for storing sensitive config with AWS/GCP/HashiCorp backends; runtime gate with graceful fallback to existing encryption. * Per-row vault-backed storage and retrieval for keys, providers, plugins, MCP, OAuth, sessions, tokens, vector stores, virtual keys, and per-user headers. * Best-effort automatic cleanup of vault secrets on row deletion and batch deletes. * **Documentation** * Config schema and loader updated with a top-level vault_store section and backend-specific options (optional prefix). * **Chores** * Bumped several indirect Go dependency versions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
occcat
pushed a commit
to occcat/bifrost
that referenced
this pull request
Sep 2, 2026
… `hashicorp-vault`) as alternative to AES encryption for sensitive config fields (maximhq#4157) ## Summary Adds a vault backend integration layer to the configstore tables, enabling sensitive fields (API keys, credentials, tokens) to be stored in an external secret manager instead of AES-encrypted in the database. This introduces a new `vault` encryption status alongside the existing `plain_text` and `encrypted` statuses, with the vault implementation itself deferred to the enterprise layer via function pointer hooks. ## Changes - Introduced `EncryptionStatusVault = "vault"` as a third encryption status constant. - Added a `VaultHooks` struct in `encryption.go` with pluggable function pointers (`IsEnabled`, `Prefix`, `StoreString`, `ResolveString`, `Remove`) that the enterprise layer populates at startup. - Added helper functions (`vaultEnvVar`, `resolveVaultEnvVar`, `vaultString`, `resolveVaultString`) that guard against nil/empty values and missing hooks before delegating to vault operations. - Updated `BeforeSave` hooks across all configstore tables (`key`, `mcp`, `oauth`, `plugin`, `provider`, `sessions`, `temp_token`, `vectorstore`, `virtualkey`) to check vault first, fall through to AES encryption, and set the appropriate `EncryptionStatus`. - Updated `AfterFind` hooks to switch on `EncryptionStatus`, resolving vault references when `vault` and decrypting when `encrypted`, preserving backward compatibility with existing AES-encrypted rows. - Added `AfterDelete` hooks to all affected tables for best-effort vault secret cleanup when a row is deleted. - Added `VaultStoreConfig` struct to `config.go` and a `vault_store` key to `ConfigData`, allowing the config file to carry vault backend settings that the enterprise layer consumes. - Added an `initVault` no-op stub in `config.go` that logs when vault config is present but defers actual initialization to the enterprise layer. - Extended `config.schema.json` with a `vault_config` definition covering `aws-secrets-manager`, `gcp-secret-manager`, and `hashicorp-vault` backends, including all relevant credential and configuration fields. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/configstore/... go test ./transports/bifrost-http/... go build ./... ``` To validate vault path construction and hook dispatch, wire up stub implementations of `VaultHooks` in a test and perform create/read/delete operations on any configstore table. Confirm that: - With `VaultHooks.IsEnabled` returning `true`, `EncryptionStatus` is set to `vault` on save and secrets are passed to `StoreString`. - On `AfterFind`, `ResolveString` is called for rows with `EncryptionStatus = vault`. - On `AfterDelete`, `Remove` is called for each vaulted field path. - Rows previously saved with `EncryptionStatus = encrypted` continue to decrypt correctly via the AES path. **New config key:** ```json { "vault_store": { "enabled": true, "type": "aws-secrets-manager", "prefix": "bifrost", "aws": { "region": "us-east-1", "role_arn": "arn:aws:iam::123456789012:role/bifrost-vault" } } } ``` Supported types: `aws-secrets-manager`, `gcp-secret-manager`, `hashicorp-vault`. ## Breaking changes - [ ] Yes - [x] No Existing AES-encrypted rows are unaffected. Vault is only activated when `VaultHooks.IsEnabled` returns `true`, which requires the enterprise layer to populate the hooks. ## Security considerations - Sensitive fields (API keys, OAuth secrets, session tokens, credentials) are no longer written to the database in AES-encrypted form when vault is active; only vault reference strings are persisted. - Vault hook function pointers are package-level globals populated at startup; callers must ensure they are set before any database operations occur when vault is enabled. - `AfterDelete` cleanup is best-effort — vault removal errors are intentionally swallowed to avoid blocking row deletion. Operators should audit vault paths independently if hard deletion guarantees are required. - The `vault_store` config block may contain credential fields; these should be supplied via `env.VAR_NAME` references rather than inline values. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Optional external vault support for storing sensitive config with AWS/GCP/HashiCorp backends; runtime gate with graceful fallback to existing encryption. * Per-row vault-backed storage and retrieval for keys, providers, plugins, MCP, OAuth, sessions, tokens, vector stores, virtual keys, and per-user headers. * Best-effort automatic cleanup of vault secrets on row deletion and batch deletes. * **Documentation** * Config schema and loader updated with a top-level vault_store section and backend-specific options (optional prefix). * **Chores** * Bumped several indirect Go dependency versions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
2 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.
Summary
Adds a vault backend integration layer to the configstore tables, enabling sensitive fields (API keys, credentials, tokens) to be stored in an external secret manager instead of AES-encrypted in the database. This introduces a new
vaultencryption status alongside the existingplain_textandencryptedstatuses, with the vault implementation itself deferred to the enterprise layer via function pointer hooks.Changes
EncryptionStatusVault = "vault"as a third encryption status constant.VaultHooksstruct inencryption.gowith pluggable function pointers (IsEnabled,Prefix,StoreString,ResolveString,Remove) that the enterprise layer populates at startup.vaultEnvVar,resolveVaultEnvVar,vaultString,resolveVaultString) that guard against nil/empty values and missing hooks before delegating to vault operations.BeforeSavehooks across all configstore tables (key,mcp,oauth,plugin,provider,sessions,temp_token,vectorstore,virtualkey) to check vault first, fall through to AES encryption, and set the appropriateEncryptionStatus.AfterFindhooks to switch onEncryptionStatus, resolving vault references whenvaultand decrypting whenencrypted, preserving backward compatibility with existing AES-encrypted rows.AfterDeletehooks to all affected tables for best-effort vault secret cleanup when a row is deleted.VaultStoreConfigstruct toconfig.goand avault_storekey toConfigData, allowing the config file to carry vault backend settings that the enterprise layer consumes.initVaultno-op stub inconfig.gothat logs when vault config is present but defers actual initialization to the enterprise layer.config.schema.jsonwith avault_configdefinition coveringaws-secrets-manager,gcp-secret-manager, andhashicorp-vaultbackends, including all relevant credential and configuration fields.Type of change
Affected areas
How to test
To validate vault path construction and hook dispatch, wire up stub implementations of
VaultHooksin a test and perform create/read/delete operations on any configstore table. Confirm that:VaultHooks.IsEnabledreturningtrue,EncryptionStatusis set tovaulton save and secrets are passed toStoreString.AfterFind,ResolveStringis called for rows withEncryptionStatus = vault.AfterDelete,Removeis called for each vaulted field path.EncryptionStatus = encryptedcontinue to decrypt correctly via the AES path.New config key:
{ "vault_store": { "enabled": true, "type": "aws-secrets-manager", "prefix": "bifrost", "aws": { "region": "us-east-1", "role_arn": "arn:aws:iam::123456789012:role/bifrost-vault" } } }Supported types:
aws-secrets-manager,gcp-secret-manager,hashicorp-vault.Breaking changes
Existing AES-encrypted rows are unaffected. Vault is only activated when
VaultHooks.IsEnabledreturnstrue, which requires the enterprise layer to populate the hooks.Security considerations
AfterDeletecleanup is best-effort — vault removal errors are intentionally swallowed to avoid blocking row deletion. Operators should audit vault paths independently if hard deletion guarantees are required.vault_storeconfig block may contain credential fields; these should be supplied viaenv.VAR_NAMEreferences rather than inline values.Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
New Features
Documentation
Chores