diff --git a/framework/configstore/clientconfig.go b/framework/configstore/clientconfig.go index d81efda797..b4c890623e 100644 --- a/framework/configstore/clientconfig.go +++ b/framework/configstore/clientconfig.go @@ -577,6 +577,7 @@ type VirtualKeyProviderConfigHashInput struct { Provider string Weight *float64 AllowedModels []string + AllowAllKeys bool // Distinguishes deny-all (false, no KeyIDs) from allow-all (true, no KeyIDs) BudgetID *string RateLimitID *string KeyIDs []string // Only key IDs, not full key objects @@ -670,6 +671,7 @@ func GenerateVirtualKeyHash(vk tables.TableVirtualKey) (string, error) { Provider: pc.Provider, Weight: pc.Weight, AllowedModels: sortedAllowedModels, + AllowAllKeys: pc.AllowAllKeys, BudgetID: pc.BudgetID, RateLimitID: pc.RateLimitID, KeyIDs: keyIDs, diff --git a/framework/configstore/tables/encryption_test.go b/framework/configstore/tables/encryption_test.go index 8807570e14..6b295a89b5 100644 --- a/framework/configstore/tables/encryption_test.go +++ b/framework/configstore/tables/encryption_test.go @@ -12,7 +12,7 @@ import ( "gorm.io/driver/postgres" "gorm.io/driver/sqlite" "gorm.io/gorm" - "gorm.io/gorm/logger" + gormLogger "gorm.io/gorm/logger" ) const testEncryptionKey = "test-encryption-key-for-testing-32bytes" @@ -25,7 +25,7 @@ func init() { func setupTestDB(t *testing.T) *gorm.DB { t.Helper() db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{ - Logger: logger.Default.LogMode(logger.Silent), + Logger: gormLogger.Default.LogMode(gormLogger.Silent), }) require.NoError(t, err) @@ -1268,9 +1268,9 @@ func TestTableMCPClient_EncryptionDisabled_StoresPlaintext(t *testing.T) { db := setupTestDB(t) client := &TableMCPClient{ - ClientID: "mcp-dis-1", - Name: "disabled-mcp", - ConnectionType: "sse", + ClientID: "mcp-dis-1", + Name: "disabled-mcp", + ConnectionType: "sse", ConnectionString: schemas.NewEnvVar("https://mcp.example.com"), Headers: map[string]schemas.EnvVar{ "Authorization": *schemas.NewEnvVar("Bearer secret-token"), @@ -1504,7 +1504,7 @@ func createTestProvider(t *testing.T, db *gorm.DB, name string) uint { func trySetupPostgresDB(t *testing.T) *gorm.DB { t.Helper() db, err := gorm.Open(postgres.Open(postgresDSN), &gorm.Config{ - Logger: logger.Default.LogMode(logger.Silent), + Logger: gormLogger.Default.LogMode(gormLogger.Silent), }) if err != nil { return nil diff --git a/framework/configstore/tables/init.go b/framework/configstore/tables/init.go new file mode 100644 index 0000000000..e4c39c17c2 --- /dev/null +++ b/framework/configstore/tables/init.go @@ -0,0 +1,10 @@ +package tables + +import "github.com/maximhq/bifrost/core/schemas" + +var logger schemas.Logger + +// SetLogger sets the logger for the tables package. +func SetLogger(l schemas.Logger) { + logger = l +} diff --git a/framework/configstore/tables/virtualkey.go b/framework/configstore/tables/virtualkey.go index b8fee6ba14..edacc8fd02 100644 --- a/framework/configstore/tables/virtualkey.go +++ b/framework/configstore/tables/virtualkey.go @@ -44,13 +44,27 @@ func (TableVirtualKeyProviderConfig) TableName() string { return "governance_virtual_key_provider_configs" } -// UnmarshalJSON custom unmarshaller to handle both "keys" ([]TableKey) and "allowed_keys" ([]string) formats +// UnmarshalJSON custom unmarshaller to handle both "keys" ([]TableKey) and "allowed_keys" ([]string) formats. +// +// Two formats are supported: +// - Config file format: "allowed_keys" field ([]string or absent) +// - DB/API format: "allow_all_keys" bool + "keys" []TableKey +// +// Absent vs empty semantics for config file format: +// - allowed_keys absent → AllowAllKeys = true (allow all by default) +// - allowed_keys: [] → AllowAllKeys = false (deny all — explicit empty) +// - allowed_keys: ["*"] → AllowAllKeys = true (explicit allow-all wildcard) +// - allowed_keys: ["k1","k2"] → specific keys, AllowAllKeys = false func (pc *TableVirtualKeyProviderConfig) UnmarshalJSON(data []byte) error { - // Temporary struct to capture all fields including allowed_keys + // Temporary struct to capture all fields. + // - AllowedKeys uses *[]string so we can distinguish absent (nil) from empty ([]). + // - DBAllowAllKeys uses *bool to detect whether "allow_all_keys" is present in JSON + // (DB/API format), shadowing the bool in Alias so we can tell config vs DB format. type Alias TableVirtualKeyProviderConfig type TempProviderConfig struct { Alias - AllowedKeys []string `json:"allowed_keys"` // Config file format: array of key names + AllowedKeys *[]string `json:"allowed_keys"` // Config file format; nil = absent + DBAllowAllKeys *bool `json:"allow_all_keys"` // DB/API format; nil = absent (shadows Alias field) } var temp TempProviderConfig @@ -58,19 +72,37 @@ func (pc *TableVirtualKeyProviderConfig) UnmarshalJSON(data []byte) error { return err } - // Copy all standard fields + // Copy all standard fields (AllowAllKeys in Alias will be false because DBAllowAllKeys shadows + // the "allow_all_keys" JSON key; we restore the correct value below). *pc = TableVirtualKeyProviderConfig(temp.Alias) - // If allowed_keys is provided (config file format), convert to Keys or set AllowAllKeys - // This takes precedence if Keys is empty but allowed_keys has values - if len(temp.AllowedKeys) > 0 && len(pc.Keys) == 0 { - // Check for wildcard — ["*"] means allow all keys - if len(temp.AllowedKeys) == 1 && temp.AllowedKeys[0] == "*" { + if temp.DBAllowAllKeys != nil { + // DB/API format: "allow_all_keys" was explicitly present — use it directly. + pc.AllowAllKeys = *temp.DBAllowAllKeys + } else if len(pc.Keys) == 0 { + // Config file format (no "allow_all_keys" JSON field). + // Apply absent-vs-empty semantics for allowed_keys. + if temp.AllowedKeys == nil { + // absent → allow all by default. + // DEPRECATED (next major version): absent allowed_keys will mean deny-all, same as []. + // Add allowed_keys: ["*"] or list specific keys explicitly in your config.json. + logger.Warn("[DEPRECATED] virtual key provider config for provider %q has no allowed_keys in config.json — defaulting to allow all keys. This implicit allow-all will be removed in the next major version. Use allowed_keys: [\"*\"] to allow all keys explicitly, or list specific key names.", + pc.Provider) pc.AllowAllKeys = true } else { - pc.Keys = make([]TableKey, len(temp.AllowedKeys)) - for i, keyName := range temp.AllowedKeys { - pc.Keys[i] = TableKey{Name: keyName} + keys := *temp.AllowedKeys + if len(keys) == 0 { + // explicit empty [] → deny all (AllowAllKeys = false, Keys = []) + pc.AllowAllKeys = false + } else if len(keys) == 1 && keys[0] == "*" { + // wildcard ["*"] → allow all + pc.AllowAllKeys = true + } else { + // specific key names + pc.Keys = make([]TableKey, len(keys)) + for i, keyName := range keys { + pc.Keys[i] = TableKey{Name: keyName} + } } } } @@ -166,12 +198,21 @@ func (TableVirtualKeyMCPConfig) TableName() string { // UnmarshalJSON custom unmarshaller to handle both "mcp_client_id" (database format) // and "mcp_client_name" (config file format) for MCP client references. +// +// Absent vs empty semantics for tools_to_execute in config file format: +// - tools_to_execute absent → ["*"] (allow all tools for this client by default) +// - tools_to_execute: [] → [] (deny all tools for this client — explicit empty) +// - tools_to_execute: ["*"] → ["*"] (explicit allow-all wildcard) +// - tools_to_execute: ["t"] → ["t"] (specific tools only) func (mc *TableVirtualKeyMCPConfig) UnmarshalJSON(data []byte) error { - // Temporary struct to capture all fields including mcp_client_name + // Temporary struct to capture all fields. + // ToolsToExecute uses *[]string to distinguish absent (nil) from empty ([]). + // The outer *[]string shadows the []string on Alias for JSON unmarshalling. type Alias TableVirtualKeyMCPConfig type TempMCPConfig struct { Alias - MCPClientName string `json:"mcp_client_name"` // Config file format: MCP client name + MCPClientName string `json:"mcp_client_name"` // Config file format: MCP client name + ToolsToExecute *[]string `json:"tools_to_execute"` // pointer: nil = absent, non-nil = present } var temp TempMCPConfig @@ -179,9 +220,21 @@ func (mc *TableVirtualKeyMCPConfig) UnmarshalJSON(data []byte) error { return err } - // Copy all standard fields + // Copy all standard fields (Alias.ToolsToExecute is nil because the outer *[]string shadows it) *mc = TableVirtualKeyMCPConfig(temp.Alias) + // Apply absent-vs-empty semantics for tools_to_execute. + // DEPRECATED (next major version): absent tools_to_execute will mean deny-all, same as []. + // Add tools_to_execute: ["*"] to allow all tools explicitly, or list specific tool names. + if temp.ToolsToExecute == nil { + // absent → allow all tools for this client by default. + logger.Warn("[DEPRECATED] virtual key MCP config for client %q has no tools_to_execute in config.json — defaulting to allow all tools. This implicit allow-all will be removed in the next major version. Use tools_to_execute: [\"*\"] to allow all tools explicitly, or list specific tool names.", + temp.MCPClientName) + mc.ToolsToExecute = []string{"*"} + } else { + mc.ToolsToExecute = *temp.ToolsToExecute + } + // Capture mcp_client_name for later resolution to MCPClientID if temp.MCPClientName != "" { mc.MCPClientName = temp.MCPClientName @@ -198,7 +251,7 @@ type TableVirtualKey struct { Value string `gorm:"uniqueIndex:idx_virtual_key_value;type:text;not null" json:"value"` // The virtual key value IsActive bool `gorm:"default:true" json:"is_active"` ProviderConfigs []TableVirtualKeyProviderConfig `gorm:"foreignKey:VirtualKeyID;constraint:OnDelete:CASCADE" json:"provider_configs"` // Empty means no providers allowed (deny-by-default) - MCPConfigs []TableVirtualKeyMCPConfig `gorm:"foreignKey:VirtualKeyID;constraint:OnDelete:CASCADE" json:"mcp_configs"` + MCPConfigs []TableVirtualKeyMCPConfig `gorm:"foreignKey:VirtualKeyID;constraint:OnDelete:CASCADE" json:"mcp_configs"` // Empty means no MCP clients allowed (deny-by-default) // Foreign key relationships (mutually exclusive: either TeamID or CustomerID, not both) TeamID *string `gorm:"type:varchar(255);index" json:"team_id,omitempty"` diff --git a/transports/bifrost-http/lib/config.go b/transports/bifrost-http/lib/config.go index df66c81b7e..7ebe98fa8b 100644 --- a/transports/bifrost-http/lib/config.go +++ b/transports/bifrost-http/lib/config.go @@ -961,11 +961,13 @@ func loadGovernanceConfigFromFile(ctx context.Context, config *Config, configDat config.GovernanceConfig = governanceConfig // Merge with config file if present if configData.Governance != nil { + preprocessGovernanceVirtualKeys(ctx, config, configData) mergeGovernanceConfig(ctx, config, configData, governanceConfig) } } else if configData.Governance != nil { // No governance config in store, use config file logger.Debug("no governance config found in store, processing from config file") + preprocessGovernanceVirtualKeys(ctx, config, configData) config.GovernanceConfig = configData.Governance createGovernanceConfigInStore(ctx, config) } else { @@ -973,6 +975,70 @@ func loadGovernanceConfigFromFile(ctx context.Context, config *Config, configDat } } +// preprocessGovernanceVirtualKeys expands absent provider_configs / mcp_configs on each VK +// in configData to all currently configured providers / MCP clients. +// +// Go's JSON decoder sets a slice field to nil when the key is absent, and to a non-nil +// empty slice when the key is present but empty ([]). We use this to distinguish: +// - absent → allow all (expand to every provider / MCP client present at init time) +// - explicit [] → deny all (leave as empty slice; resolver enforces deny-by-default) +// +// DEPRECATED (next major version): relying on an absent key for allow-all behaviour will be +// removed. Always include the field explicitly in your config.json: +// - to allow all providers: list them explicitly in provider_configs +// - to deny all providers: set provider_configs: [] +// - to allow all MCP clients: list them explicitly in mcp_configs +// - to deny all MCP clients: set mcp_configs: [] +// In the next major version both absent and [] will mean deny-all (deny-by-default). +func preprocessGovernanceVirtualKeys(ctx context.Context, config *Config, configData *ConfigData) { + for i := range configData.Governance.VirtualKeys { + vk := &configData.Governance.VirtualKeys[i] + + // Absent provider_configs → allow all providers configured right now. + // DEPRECATED: explicitly list provider_configs in config.json instead of relying on absence. + if vk.ProviderConfigs == nil { + logger.Warn( + "virtual key %q has no provider_configs in config.json — expanding to all %d configured provider(s). "+ + "This implicit allow-all behaviour is DEPRECATED and will be removed in the next major version. "+ + "Please add an explicit provider_configs list to your config.json.", + vk.ID, len(config.Providers), + ) + expanded := make([]configstoreTables.TableVirtualKeyProviderConfig, 0, len(config.Providers)) + for providerName := range config.Providers { + expanded = append(expanded, configstoreTables.TableVirtualKeyProviderConfig{ + Provider: string(providerName), + AllowAllKeys: true, + }) + } + vk.ProviderConfigs = expanded + } + + // Absent mcp_configs → allow all MCP clients configured right now. + // DEPRECATED: explicitly list mcp_configs in config.json instead of relying on absence. + if vk.MCPConfigs == nil && config.MCPConfig != nil && len(config.MCPConfig.ClientConfigs) > 0 { + logger.Warn( + "virtual key %q has no mcp_configs in config.json — expanding to all %d configured MCP client(s). "+ + "This implicit allow-all behaviour is DEPRECATED and will be removed in the next major version. "+ + "Please add an explicit mcp_configs list to your config.json.", + vk.ID, len(config.MCPConfig.ClientConfigs), + ) + vk.MCPConfigs = make([]configstoreTables.TableVirtualKeyMCPConfig, 0, len(config.MCPConfig.ClientConfigs)) + for _, client := range config.MCPConfig.ClientConfigs { + vk.MCPConfigs = append(vk.MCPConfigs, configstoreTables.TableVirtualKeyMCPConfig{ + MCPClientName: client.Name, + ToolsToExecute: []string{"*"}, + }) + } + } + + // Resolve client names → DB IDs for all MCPConfigs (including explicitly-listed ones) + // so that MCPClientID is populated before GenerateVirtualKeyHash is called. + if len(vk.MCPConfigs) > 0 { + vk.MCPConfigs = resolveMCPConfigClientIDs(ctx, config.ConfigStore, vk.MCPConfigs, vk.ID) + } + } +} + // mergeGovernanceConfig merges governance config from file with store func mergeGovernanceConfig(ctx context.Context, config *Config, configData *ConfigData, governanceConfig *configstore.GovernanceConfig) { logger.Debug("merging governance config from config file with store") @@ -1139,9 +1205,6 @@ func mergeGovernanceConfig(ctx context.Context, config *Config, configData *Conf } configData.Governance.VirtualKeys[i].Value = governance.GenerateVirtualKey() } - // Resolve MCP client names to IDs for config file mcp_configs - configData.Governance.VirtualKeys[i].MCPConfigs = resolveMCPConfigClientIDs( - ctx, config.ConfigStore, configData.Governance.VirtualKeys[i].MCPConfigs, newVirtualKey.ID) virtualKeysToUpdate = append(virtualKeysToUpdate, configData.Governance.VirtualKeys[i]) governanceConfig.VirtualKeys[j] = configData.Governance.VirtualKeys[i] } else { @@ -1169,9 +1232,6 @@ func mergeGovernanceConfig(ctx context.Context, config *Config, configData *Conf } configData.Governance.VirtualKeys[i].Value = governance.GenerateVirtualKey() } - // Resolve MCP client names to IDs for config file mcp_configs - configData.Governance.VirtualKeys[i].MCPConfigs = resolveMCPConfigClientIDs( - ctx, config.ConfigStore, configData.Governance.VirtualKeys[i].MCPConfigs, newVirtualKey.ID) virtualKeysToAdd = append(virtualKeysToAdd, configData.Governance.VirtualKeys[i]) } } @@ -2386,6 +2446,7 @@ func reconcileVirtualKeyAssociations( // Update existing provider config from file existing.Weight = newPC.Weight existing.AllowedModels = newPC.AllowedModels + existing.AllowAllKeys = newPC.AllowAllKeys existing.BudgetID = newPC.BudgetID existing.RateLimitID = newPC.RateLimitID existing.Keys = newPC.Keys diff --git a/transports/bifrost-http/main.go b/transports/bifrost-http/main.go index 1fd2b7b2e6..5aeb489da0 100644 --- a/transports/bifrost-http/main.go +++ b/transports/bifrost-http/main.go @@ -63,6 +63,7 @@ import ( bifrost "github.com/maximhq/bifrost/core" schemas "github.com/maximhq/bifrost/core/schemas" + "github.com/maximhq/bifrost/framework/configstore/tables" "github.com/maximhq/bifrost/transports/bifrost-http/handlers" "github.com/maximhq/bifrost/transports/bifrost-http/lib" bifrostServer "github.com/maximhq/bifrost/transports/bifrost-http/server" @@ -143,6 +144,7 @@ func main() { lib.SetLogger(logger) bifrostServer.SetLogger(logger) handlers.SetLogger(logger) + tables.SetLogger(logger) ctx := context.Background() err := server.Bootstrap(ctx) diff --git a/transports/changelog.md b/transports/changelog.md index 4a02442a62..cc79dd4ef0 100644 --- a/transports/changelog.md +++ b/transports/changelog.md @@ -1,3 +1,30 @@ - feat: VK provider config key_ids now supports ["*"] wildcard to allow all keys; empty key_ids denies all; handler resolves wildcard to AllowAllKeys flag without DB key lookups - feat: add option to disable automatic MCP tool injection per request - feat: virtual key MCP configs now act as an execution-time allow-list — tools not permitted by the VK are blocked at inference and MCP tool execution + +## BREAKING CHANGES — explicit empty arrays now mean deny-all; absent inner fields now mean allow-all + +The following fields in `governance.virtual_keys[*]` in config.json have changed semantics: + +| Field | Old behaviour | New behaviour | Type | +|---|---|---|---| +| `provider_configs: []` | allow all providers | **deny all providers** | breaking | +| `provider_configs` absent | allow all providers | allow all providers — deprecated, see below | deprecated | +| `provider_configs[*].allowed_keys: []` | allow all keys | **deny all keys** | breaking | +| `provider_configs[*].allowed_keys` absent | deny all keys (bug) | **allow all keys** — deprecated, see below | breaking + deprecated | +| `mcp_configs: []` | allow all MCP clients | **deny all MCP clients** | breaking | +| `mcp_configs` absent | allow all MCP clients | allow all MCP clients — deprecated, see below | deprecated | +| `mcp_configs[*].tools_to_execute: []` | deny all tools | deny all tools | unchanged | +| `mcp_configs[*].tools_to_execute` absent | deny all tools (bug) | **allow all tools** — deprecated, see below | breaking + deprecated | + +### Migration guide + +**`provider_configs: []` / `mcp_configs: []`** — previously allowed all; now deny all. To keep allow-all behaviour, list entries explicitly instead of using an empty array. + +**`provider_configs[*].allowed_keys` absent** — previously (buggy) denied all keys; now allows all keys and emits a deprecation warning. If you intended deny-all, add `allowed_keys: []` explicitly. If you intended allow-all, add `allowed_keys: ["*"]` to silence the warning. + +**`mcp_configs[*].tools_to_execute` absent** — previously (buggy) denied all tools for that client; now allows all tools and emits a deprecation warning. If you intended deny-all, add `tools_to_execute: []` explicitly. If you intended allow-all, add `tools_to_execute: ["*"]` to silence the warning. + +### Deprecation notice + +Omitting `provider_configs`, `mcp_configs`, `allowed_keys`, or `tools_to_execute` entirely (absent key) currently defaults to allow-all and emits a startup warning. For `provider_configs` and `mcp_configs`, the implicit allow-all expands to whichever providers / MCP clients are present at startup — it is a boot-time snapshot, not a live wildcard. **This implicit allow-all will be removed in the next major version** — absent and `[]` will both mean deny-all (deny-by-default). Migrate by always specifying these fields explicitly in config.json. diff --git a/ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx b/ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx index 2644c4cd1e..9c731d664a 100644 --- a/ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx +++ b/ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx @@ -1030,7 +1030,7 @@ export default function VirtualKeySheet({ virtualKey, teams, customers, onSave, { label: "Allow All Tools", value: "*", - description: "Allow all current and future tools (including dynamically fetched ones)", + description: "Allow all current and future tools", }, ...[...availableTools, ...enabledToolsByConfig] .filter((tool, index, arr) => arr.findIndex((t) => t.name === tool.name) === index)