feat: add OAuth2 AS discovery endpoints, signing key management, and MCPServerAuthMode config#4505
Open
Pratham-Mishra04 wants to merge 12 commits into
Open
feat: add OAuth2 AS discovery endpoints, signing key management, and MCPServerAuthMode config#4505Pratham-Mishra04 wants to merge 12 commits into
MCPServerAuthMode config#4505Pratham-Mishra04 wants to merge 12 commits into
Conversation
|
|
Collaborator
Author
This was referenced Jun 17, 2026
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds MCP inbound OAuth2 authentication infrastructure: new ChangesMCP OAuth2 Discovery Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuth2DiscoveryHandler
participant ClientConfig
participant RDBConfigStore
participant GovernanceConfigDB as GovernanceConfig DB
Client->>OAuth2DiscoveryHandler: GET /.well-known/jwks.json
OAuth2DiscoveryHandler->>ClientConfig: IsMCPOAuthDiscoveryEnabled()
alt OAuth discovery disabled
OAuth2DiscoveryHandler-->>Client: 404 Not Found
else OAuth discovery enabled
OAuth2DiscoveryHandler->>RDBConfigStore: GetOAuth2SigningKey(ctx)
RDBConfigStore->>GovernanceConfigDB: SELECT by GovernanceConfigKeyOAuth2SigningKey
alt key exists
GovernanceConfigDB-->>RDBConfigStore: stored JSON
RDBConfigStore->>RDBConfigStore: unmarshal + Decrypt()
else key missing
RDBConfigStore->>RDBConfigStore: rsa.GenerateKey RSA-2048
RDBConfigStore->>RDBConfigStore: Encrypt() private key
RDBConfigStore->>GovernanceConfigDB: INSERT ON CONFLICT DO NOTHING
RDBConfigStore->>RDBConfigStore: use plaintext private PEM
end
RDBConfigStore-->>OAuth2DiscoveryHandler: OAuth2SigningKey
OAuth2DiscoveryHandler->>OAuth2DiscoveryHandler: parseRSAPublicKeyPEM
OAuth2DiscoveryHandler->>OAuth2DiscoveryHandler: rsaPublicKeyToJWK
OAuth2DiscoveryHandler-->>Client: 200 {"keys":[...]}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
Confidence Score: 4/5Safe to merge for the discovery and keypair infrastructure; one config handler path lets zero-second TTLs be stored, which will cause immediately-expired tokens when the token endpoint is built on top of this. The signing-key generation, at-rest encryption, concurrency handling, and discovery endpoints are all well-implemented. The one concrete defect is in the config update handler: when a caller sends oauth2_server_config with only issuer_url and omits auth_code_ttl/access_token_ttl, both fields unmarshal to 0 and are stored without any server-side minimum check. The oauth2ServerCfg fallback only substitutes defaults for a nil config, not a zero-valued one, so future token issuance will read 0-second lifetimes and issue codes that expire on creation. transports/bifrost-http/handlers/config.go — the OAuth2ServerConfig update block should validate or default AuthCodeTTL and AccessTokenTTL before persisting. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant AuthMiddleware
participant OAuth2DiscoveryHandler
participant ConfigStore
participant DB
Client->>AuthMiddleware: "GET /.well-known/*"
AuthMiddleware->>AuthMiddleware: /.well-known/ prefix skip auth
AuthMiddleware->>OAuth2DiscoveryHandler: forward request
OAuth2DiscoveryHandler->>OAuth2DiscoveryHandler: discoveryEnabled()?
alt "MCPServerAuthMode == headers"
OAuth2DiscoveryHandler-->>Client: 404 Not Found
else "MCPServerAuthMode == both or oauth"
alt GET /.well-known/jwks.json
OAuth2DiscoveryHandler->>ConfigStore: GetOAuth2SigningKey(ctx)
ConfigStore->>DB: "SELECT governance_config WHERE key=oauth2_signing_key"
alt key exists
DB-->>ConfigStore: row
ConfigStore->>ConfigStore: Decrypt PrivateKeyPEM
ConfigStore-->>OAuth2DiscoveryHandler: OAuth2SigningKey
else key missing
ConfigStore->>ConfigStore: rsa.GenerateKey(2048)
ConfigStore->>ConfigStore: Encrypt PrivateKeyPEM
ConfigStore->>DB: INSERT ON CONFLICT DO NOTHING
alt won insert
ConfigStore-->>OAuth2DiscoveryHandler: new key plaintext
else lost race
ConfigStore->>DB: re-read + decrypt
ConfigStore-->>OAuth2DiscoveryHandler: winning key
end
end
OAuth2DiscoveryHandler-->>Client: 200 JWKS JSON
else GET /.well-known/oauth-authorization-server
OAuth2DiscoveryHandler-->>Client: 200 AS metadata JSON
else GET /.well-known/oauth-protected-resource
OAuth2DiscoveryHandler-->>Client: 200 PRM JSON
end
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client
participant AuthMiddleware
participant OAuth2DiscoveryHandler
participant ConfigStore
participant DB
Client->>AuthMiddleware: "GET /.well-known/*"
AuthMiddleware->>AuthMiddleware: /.well-known/ prefix skip auth
AuthMiddleware->>OAuth2DiscoveryHandler: forward request
OAuth2DiscoveryHandler->>OAuth2DiscoveryHandler: discoveryEnabled()?
alt "MCPServerAuthMode == headers"
OAuth2DiscoveryHandler-->>Client: 404 Not Found
else "MCPServerAuthMode == both or oauth"
alt GET /.well-known/jwks.json
OAuth2DiscoveryHandler->>ConfigStore: GetOAuth2SigningKey(ctx)
ConfigStore->>DB: "SELECT governance_config WHERE key=oauth2_signing_key"
alt key exists
DB-->>ConfigStore: row
ConfigStore->>ConfigStore: Decrypt PrivateKeyPEM
ConfigStore-->>OAuth2DiscoveryHandler: OAuth2SigningKey
else key missing
ConfigStore->>ConfigStore: rsa.GenerateKey(2048)
ConfigStore->>ConfigStore: Encrypt PrivateKeyPEM
ConfigStore->>DB: INSERT ON CONFLICT DO NOTHING
alt won insert
ConfigStore-->>OAuth2DiscoveryHandler: new key plaintext
else lost race
ConfigStore->>DB: re-read + decrypt
ConfigStore-->>OAuth2DiscoveryHandler: winning key
end
end
OAuth2DiscoveryHandler-->>Client: 200 JWKS JSON
else GET /.well-known/oauth-authorization-server
OAuth2DiscoveryHandler-->>Client: 200 AS metadata JSON
else GET /.well-known/oauth-protected-resource
OAuth2DiscoveryHandler-->>Client: 200 PRM JSON
end
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
transports/config.schema.json (1)
276-280: ⚡ Quick winDeclare the default for
mcp_server_auth_modein schema.The description documents
'headers'as the default, but the schema does not set"default": "headers". Add it so schema-driven tooling and generated config stay aligned with runtime expectations.As per coding guidelines, `transports/config.schema.json` is the source of truth for config fields.Suggested fix
"mcp_server_auth_mode": { "type": "string", "enum": ["headers", "both", "oauth"], + "default": "headers", "description": "How /mcp authenticates inbound MCP clients. 'headers' (default): VK/api-key/session headers only, discovery disabled. 'both': accepts header credentials and Bifrost-issued JWTs, discovery enabled. 'oauth': Bifrost JWTs only — WARNING: disables VK/header MCP access." },🤖 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/config.schema.json` around lines 276 - 280, The mcp_server_auth_mode property in the schema documents 'headers' as the default in its description, but the schema object itself is missing the "default" field declaration. Add "default": "headers" to the mcp_server_auth_mode object definition to ensure schema-driven tooling and generated configurations are aligned with the documented runtime expectations.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/clientconfig.go`:
- Around line 101-102: The GenerateClientConfigHash function does not include
the newly added MCPServerAuthMode and OAuth2ServerConfig fields in its hash
calculation, which means configuration changes to OAuth2 authentication settings
will not trigger reconciliation. Update the GenerateClientConfigHash function to
include both the MCPServerAuthMode field and the OAuth2ServerConfig field in the
hash computation to ensure config drift is properly detected.
In `@framework/configstore/migrations.go`:
- Around line 10905-10924: The migration with ID migrationName only defines the
Migrate function but lacks explicit rollback behavior. Add a Rollback function
to the migration object that removes the two columns (MCPServerAuthMode and
OAuth2ServerConfigJSON) from tables.TableClientConfig using the migrator's
DropColumn method, or if the migration is non-rollbackable, explicitly return an
error indicating that downgrade is not supported so operators have clear
expectations.
In `@framework/configstore/rdb.go`:
- Around line 6701-6707: The read path decrypts PrivateKeyPEM whenever
encrypt.IsEnabled() is true, but keys created before encryption was enabled are
stored as plaintext and will fail decryption, breaking OAuth operations. To fix
this, add an encryption status marker or version field to track whether each key
was encrypted when stored, then in the decrypt logic (around the
encrypt.IsEnabled() check in the code block starting at line 6701), only attempt
decryption if the encryption marker indicates the key was actually encrypted, or
detect plaintext keys and re-encrypt them on first read when encryption becomes
enabled. Apply the same logic to all similar decrypt paths mentioned in the
comment (also applies to lines 6731-6743).
- Around line 6688-6710: The current lazy signing-key creation has a race
condition where multiple concurrent callers can both miss the row, each
generating and saving different RSA keys, leading to inconsistency between
signed tokens and advertised keys. Fix this by making the creation atomic in the
createOAuth2SigningKey method: use an insert-on-conflict clause with DoNothing
set to true and Columns set to the key field, then check the RowsAffected count.
If RowsAffected is 0 (indicating a conflict where another process inserted
first), reload and decrypt the existing row before returning instead of
returning the newly generated key. Apply this fix to both the
createOAuth2SigningKey call path within GetOAuth2SigningKey and the other
similar signing-key creation location mentioned in the comment range.
In `@framework/configstore/tables/oauth2_server.go`:
- Around line 53-55: The comment for AccessTokenTTL in the OAuth2ServerConfig
incorrectly states the default value as 900 seconds, but the actual
DefaultAccessTokenTTL constant and schema both define it as 600 seconds. Update
the comment for AccessTokenTTL to correct the default value from 900 to 600
seconds to match the actual implementation and prevent confusion during
operational troubleshooting.
In `@transports/bifrost-http/handlers/config.go`:
- Around line 503-504: The assignments to updatedConfig.MCPServerAuthMode and
updatedConfig.OAuth2ServerConfig always overwrite existing values with zero
values when those fields are omitted from the payload during partial updates.
Instead of unconditionally assigning these fields, add conditional checks to
only update updatedConfig.MCPServerAuthMode and updatedConfig.OAuth2ServerConfig
when the corresponding payload.ClientConfig fields are actually provided (not
empty or zero values). This preserves existing runtime and database state when
these OAuth settings are not included in the update request.
- Around line 503-504: The MCPServerAuthMode and OAuth2ServerConfig fields in
the config handler are being assigned directly from the payload without
validating against the schema constraints defined in
transports/config.schema.json. Add validation logic before the assignments to
ensure MCPServerAuthMode conforms to the allowed enum values (headers, both,
oauth) and that OAuth2ServerConfig has the correct shape and applicability
according to the schema. If validation fails, return an appropriate error
response instead of persisting invalid configuration.
In `@transports/bifrost-http/handlers/plugins.go`:
- Around line 589-591: The isEnvVarObject function at line 589 is too permissive
in detecting EnvVar objects, treating any map containing the keys value,
env_var, and from_env as an EnvVar placeholder, even if additional fields are
present. This causes the function to incorrectly return the existing
configuration at line 591, discarding user updates. Tighten the isEnvVarObject
detection logic to only return true for objects that are exclusively EnvVar
objects with only the expected fields and no additional properties. Apply this
same fix to the similar check at lines 633-637 to prevent the same config
rollback issue from occurring in both locations.
In `@transports/bifrost-http/server/server.go`:
- Line 1397: The OAuth2 discovery routes registered by NewOAuth2DiscoveryHandler
are being blocked by APIMiddleware because the `/.well-known/` endpoint prefixes
are not whitelisted. Requests to these endpoints are rejected before reaching
the handler's discoveryEnabled() check. Add "/.well-known/" as a new entry to
the whitelistedPrefixes list in handlers/middlewares.go to allow these endpoints
through the APIMiddleware while letting the individual discovery handlers
continue to control access through their own discoveryEnabled() validation
logic.
In `@transports/config.schema.json`:
- Around line 299-306: The schema definitions for auth_code_ttl and
access_token_ttl fields currently only specify type as integer without enforcing
positive values, allowing zero or negative TTL values that would create invalid
credentials. Add a minimum constraint property to both auth_code_ttl and
access_token_ttl field definitions in the JSON schema to enforce that values
must be greater than zero, ensuring only valid positive integer values are
accepted.
---
Nitpick comments:
In `@transports/config.schema.json`:
- Around line 276-280: The mcp_server_auth_mode property in the schema documents
'headers' as the default in its description, but the schema object itself is
missing the "default" field declaration. Add "default": "headers" to the
mcp_server_auth_mode object definition to ensure schema-driven tooling and
generated configurations are aligned with the documented runtime expectations.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 0b0d9090-5ec7-407d-ace5-a4ae23b1050b
📒 Files selected for processing (14)
framework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/clientconfig.goframework/configstore/tables/oauth2_server.goframework/temptoken/scope.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/oauth2_discovery.gotransports/bifrost-http/handlers/oauth2_utils.gotransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/handlers/plugins_test.gotransports/bifrost-http/server/server.gotransports/config.schema.json
Pratham-Mishra04
force-pushed
the
06-16-feat_adds_mcp_server_oauth_tables
branch
2 times, most recently
from
226ccda to
bd05880
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 6709-6710: In the createOAuth2SigningKey function, before the
final reload attempt (the call to loadOAuth2SigningKey after handling insert
conflicts), add logic to repair any existing empty rows in the database. When an
insert conflict occurs, check if an empty row already exists (where Value is an
empty string or NULL), and if so, update that row with the newly generated key
data before reloading. This repair step should be applied in both locations
mentioned in the issue (around line 6709-6710 in the loadOAuth2SigningKey check
and also around line 6775), preventing the infinite retry loop that occurs when
loadOAuth2SigningKey returns ErrNotFound for empty rows.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 110527b2-c815-4a08-8d76-630f8379c0ad
📒 Files selected for processing (14)
framework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/clientconfig.goframework/configstore/tables/oauth2_server.goframework/temptoken/scope.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/oauth2_discovery.gotransports/bifrost-http/handlers/oauth2_utils.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (7)
- transports/bifrost-http/handlers/oauth2_utils.go
- transports/bifrost-http/server/server.go
- transports/config.schema.json
- framework/configstore/store.go
- framework/temptoken/scope.go
- transports/bifrost-http/handlers/oauth2_discovery.go
- framework/configstore/tables/clientconfig.go
Pratham-Mishra04
force-pushed
the
06-16-feat_adds_mcp_server_oauth_tables
branch
2 times, most recently
from
18d1803 to
67e3649
Compare
This was referenced Jun 18, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@transports/bifrost-http/handlers/oauth2_utils.go`:
- Around line 27-29: The oauth2MCPResourceURL function concatenates the issuer
URL with "/mcp" without normalizing trailing slashes, which results in "//mcp"
when the issuer URL is configured with a trailing slash. This causes resource
URL mismatches that break token validation. Fix this by trimming any trailing
slash from the result of oauth2IssuerURL(ctx, store) before appending "/mcp" to
ensure consistent, canonical resource URLs across discovery, authorization, and
verification 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: CHILL
Plan: Pro Plus
Run ID: f8dbb913-0854-4528-8915-13626b1d3311
📒 Files selected for processing (14)
framework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/clientconfig.goframework/configstore/tables/oauth2_server.goframework/temptoken/scope.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/oauth2_discovery.gotransports/bifrost-http/handlers/oauth2_utils.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (13)
- framework/temptoken/scope.go
- transports/bifrost-http/handlers/middlewares.go
- framework/configstore/clientconfig.go
- transports/bifrost-http/server/server.go
- framework/configstore/migrations.go
- transports/config.schema.json
- framework/configstore/store.go
- transports/bifrost-http/lib/config_test.go
- transports/bifrost-http/handlers/config.go
- framework/configstore/tables/clientconfig.go
- framework/configstore/tables/oauth2_server.go
- framework/configstore/rdb.go
- transports/bifrost-http/handlers/oauth2_discovery.go
Pratham-Mishra04
force-pushed
the
06-16-feat_adds_mcp_server_oauth_tables
branch
from
67e3649 to
46280b1
Compare
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The merge-base changed after approval.
akshaydeo
force-pushed
the
dev
branch
2 times, most recently
from
fa15f50 to
ca190fc
Compare
Sort client and tool names lexicographically in GetToolPerClient and GetAvailableTools so the tool list injected into LLM prompts is stable across requests. Also sort keys in OrderedMapFromMap so tool schema properties don't shuffle. Closes #2347
…i-agent items (#4609) ResponsesMessage had no Author or Recipient fields, and ResponsesMessageContentBlock had no EncryptedContent field. Go's JSON decoder silently drops unknown fields during decode, so these values were stripped out before bifrost re-serialized the request upstream. OpenAI requires all three on collab_tool_call items used by Codex multi_agent_v2 (>=0.141.0): - author: identifies the sending agent on collab_tool_call input items - recipient: identifies the target agent on collab_tool_call input items - encrypted_content: opaque reasoning token on content blocks that must be echoed verbatim on history replay author and recipient use json.RawMessage to survive future schema changes without coupling bifrost to OpenAI's object shape. encrypted_content is a *string (opaque base64 blob). Verified end-to-end: Codex multi_agent_v2 subagent spawn completes successfully through bifrost with all /v1/responses returning 200. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
## Summary Adds a new `/workspace/scim/oauth-discover-callback` route to handle the OAuth discovery callback flow within the SCIM user provisioning section. The parent SCIM layout is updated to render child routes via an `Outlet` when nested routes are active, rather than always rendering the SCIM page directly. ## Changes - The SCIM layout now uses `useChildMatches` to conditionally render either the `SCIMPage` or an `Outlet`, enabling nested child routes to render within the SCIM section. - A new `oauth-discover-callback` route is introduced under `/workspace/scim`, with its own layout enforcing the same `UserProvisioning` RBAC permission check. - The new page renders the `DiscoverCallbackView` component from the enterprise SCIM wizard. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test Navigate to the SCIM configuration page and initiate an OAuth discovery flow. Upon redirect, the app should land on `/workspace/scim/oauth-discover-callback` and render the `DiscoverCallbackView` without losing the RBAC permission guard. ```sh cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations The new route enforces the same `RbacResource.UserProvisioning` + `RbacOperation.View` permission check as the parent SCIM layout, ensuring unauthorized users cannot access the OAuth callback page. ## 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
Pratham-Mishra04
force-pushed
the
06-16-feat_adds_mcp_server_oauth_tables
branch
from
46280b1 to
9f1fff1
Compare
…resolve/remove hooks and UI badge (#4398) ## Summary This PR extends the `EnvVar` type to support external vault backends (AWS Secrets Manager, GCP Secret Manager, HashiCorp Vault, etc.) as a first-class secret source alongside the existing environment variable and plaintext forms. A `vault.path/to/secret` reference prefix is now recognized everywhere `env.VAR` references are handled. ## Changes - Added `VaultRef` and `FromVault` fields to `EnvVar`, with JSON tags `vault_var` and `from_vault`. All existing methods (`IsSet`, `IsRedacted`, `Equals`, `Redacted`, `FullyRedacted`, `ShouldPreserveStored`, `GetValue`, `Scan`, `UnmarshalJSON`, `Value`) are updated to handle vault references consistently with env references. - Introduced `core/schemas/vault.go` with hook function pointers (`VaultResolveHook`, `VaultRemoveHook`, `VaultStoreHook`, `VaultPrefixHook`) that are wired at enterprise startup and no-op in OSS deployments. Helpers `LookupVault`, `StoreVaultEnvVar`, `StoreOwnedVaultEnvVars`, `RemoveOwnedVaultEnvVars`, and `VaultBasePath` provide reflection-based bulk store/remove operations over struct fields. - Removed the `VaultHooks` struct and all per-field vault store/resolve/remove logic from `tables/encryption.go`, `key.go`, `oauth.go`, `plugin.go`, `provider.go`, `virtualkey.go`, and `mcp_per_user_headers.go`. `BeforeSave` hooks now delegate to `schemas.StoreOwnedVaultEnvVars` and `AfterDelete` hooks delegate to `schemas.RemoveOwnedVaultEnvVars`, eliminating the large per-field repetition. `AfterFind` vault resolution is now handled lazily via `GetValue()` and `LookupVault` rather than eagerly in GORM hooks. - AES encryption helpers (`encryptEnvVar`, `decryptEnvVar`) now skip vault-backed fields to prevent double-processing. - Hash generation in `clientconfig.go` and `rdb.go` includes a `vault:` prefix for vault-backed values to prevent collisions with env and literal values. - `EnvVarAsString` in `utils.go` returns `VaultRef` for vault-backed fields. - The HTTP config handler exposes vault-backed admin passwords with their reference intact rather than a generic redacted placeholder. - UI `EnvVar` schema and form utilities (`envVarForm.ts`, `envVarInput.tsx`, `schemas.ts`) add `vault_var` and `from_vault` fields. The `EnvVarInput` component auto-detects `vault.` prefixes and displays a yellow warning badge for vault references, distinct from the green badge used for env references. - A `warning` badge variant is added to the UI badge component. - Tests cover `Scan`, `UnmarshalJSON`, `Value`, `Redacted`, `IsSet`, `NewEnvVar`, `StoreVaultEnvVar`, `StoreOwnedVaultEnvVars` for vault paths, including no-op cases and hook-absent scenarios. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go test ./core/schemas/... ./framework/configstore/... # UI cd ui pnpm i pnpm build ``` To exercise vault resolution end-to-end, wire `schemas.VaultResolveHook` and `schemas.VaultStoreHook` to a vault backend and set a field value to `vault.your/path`. On save the reference is stored; on load `GetValue()` resolves it via the hook. ## Breaking changes - [x] Yes - [ ] No The `EnvVar` JSON representation gains two new optional fields (`vault_var`, `from_vault`). Existing serialized values without these fields deserialize correctly with zero values. The `VaultHooks` struct in `tables/encryption.go` is removed; any enterprise code wiring `VaultHooks` directly must be updated to set the `schemas.Vault*Hook` function pointers instead. ## Security considerations Vault references are treated as redacted in all API responses — the same policy applied to env var references. The resolved secret value is never serialized back to the database; only the `vault.path` reference is persisted. `FullyRedacted` and `Redacted` preserve `VaultRef` so round-trip update merges can match via `Equals` without exposing the secret. ## Checklist - [x] I added/updated tests where appropriate - [x] I verified builds succeed (Go and UI)
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes #123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## 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
… and `map[string]EnvVar` support, replacing per-model `BeforeSave`/`AfterDelete` vault hooks (#4404) ## Summary This PR centralises vault secret management into a single pair of global GORM callbacks (`bifrost:vault_store` and `bifrost:vault_remove`), removing the duplicated per-model `BeforeSave`/`AfterDelete` vault logic from `TableKey`, `TableMCPClient`, and `TableOauthConfig`. Models opt in by implementing the new `VaultPathKeyer` interface. It also extends vault support to `map[string]EnvVar` fields (e.g. MCP `Headers`), which previously were not walked by the store/remove helpers. ## Changes - **`VaultPathKeyer` interface** added to `core/schemas/vault.go`. Models that implement `VaultPathKey() string` are automatically handled by the global callbacks without any per-model wiring. - **`VaultStoreEnabled()` renamed to `VaultStoreWriteEnabled()`** and now requires both `VaultStoreHook` and `VaultRemoveHook` to be non-nil before write operations are attempted. - **`map[string]EnvVar` support** added to both `StoreOwnedVaultEnvVars` and `RemoveOwnedVaultEnvVars`. Each map entry is stored at `basePath/<column>/<mapKey>`. Fragment refs (`#key`) pointing at externally-managed shared secrets are never auto-deleted. - **`removeOwnedVaultEnvVar`** extracted as a private helper to deduplicate the single-field removal logic used by both the struct-field and map-entry paths. - **`RegisterVaultCallbacks(db)`** introduced in `framework/configstore/vault_callbacks.go`. It registers `vaultStoreCallback` (before create/update) and `vaultRemoveCallback` (after delete) on any `*gorm.DB`. Called from `openPostresConnection` so every pool gets the callbacks automatically. - **Per-model `BeforeSave` vault blocks and `AfterDelete` hooks removed** from `TableKey`, `TableMCPClient`, and `TableOauthConfig`. Each model now only implements `VaultPathKey()`. - **`AddProviderKey` / `UpdateProviderKey`** in `transports/bifrost-http/lib/config.go` re-read the stored key after a DB write so the in-memory copy reflects the vault reference rather than the original plaintext. - **Postgres helper functions** (`buildPostgresDSN`, `openPostresConnection`, `closeDbConn`, `applyPostgresPoolTuning`) extracted into `framework/configstore/postgres.go` to reduce duplication in the two-pool lifecycle. - **Helm chart and JSON schemas** updated to expose `vaultStore` configuration under `storage.configStore`, including `type`, `prefix`, `accessMode`, and backend-specific blocks for AWS, GCP, and HashiCorp Vault. ## Type of change - [ ] Bug fix - [x] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/schemas/... ./framework/configstore/... ``` - `TestStoreOwnedVaultEnvVars_WalksMap` — verifies that `map[string]EnvVar` entries are stored individually and converted to vault refs. - `TestRemoveOwnedVaultEnvVars_WalksMap` — verifies that only owned (non-fragment) map entries are removed. - `TestVaultCallbacks_AutoStoreAndRemove` — end-to-end test using an in-memory SQLite DB: creates a `TableMCPClient` with a plaintext `Authorization` header, asserts the vault store callback fires and the persisted `HeadersJSON` holds the vault ref, then deletes the row and asserts the remove callback fires. - `TestVaultCallbacks_NoOpWhenDisabled` — asserts no vault refs appear in the DB when hooks are not installed. To exercise vault configuration via Helm, set `storage.configStore.vaultStore.enabled: true` with the appropriate `type` and backend block. ## Breaking changes - [x] Yes - [ ] No `VaultStoreEnabled()` has been renamed to `VaultStoreWriteEnabled()`. Any enterprise or external code calling `VaultStoreEnabled()` must be updated to use `VaultStoreWriteEnabled()`. The semantics also changed slightly: write operations now require both `VaultStoreHook` and `VaultRemoveHook` to be wired. ## Related issues N/A ## Security considerations - Plaintext secrets are pushed to the vault backend before the DB row is written; the DB row stores only the `vault.<path>` reference. - Fragment refs (`vault.<path>#<key>`) pointing at externally-managed shared secrets are explicitly excluded from auto-deletion to prevent accidental removal of secrets owned by other systems. - The `read_only` access mode (resolvable via config schema) prevents auto-store and auto-delete when only secret resolution is needed. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
… support env/vault references (#4504) ## Summary Virtual key values can now be sourced from environment variables or vault references (`env.X` / `vault.X`) rather than only from literal strings. The resolved plaintext is stored in the existing `Value` column (preserving hash-based lookup and auth), while a new `value_source_ref` column persists the original reference so the `SecretVar` shape survives a DB round-trip. ## Changes - **`TableVirtualKey` struct** — `Value` is now `json:"-"` (excluded from direct marshalling). Two new fields are added: `ValueSourceRef` (persisted, encrypted) stores the `env.X`/`vault.X` reference; `ValueSource` (transient, `gorm:"-"`) holds the runtime `SecretVar`. Custom `MarshalJSON`/`UnmarshalJSON` handle both bare strings and `SecretVar` objects for the `value` JSON field, redacting resolved secrets for env/vault sources. - **`BeforeSave` / `AfterFind`** — `ValueSourceRef` is encrypted alongside `Value` on write and decrypted on read; `AfterFind` reconstructs `ValueSource` from the stored plaintext and reference without re-resolving the live environment, keeping it consistent with the hashed value. - **Database migration** — `add_virtual_key_value_source_ref_column` adds the `value_source_ref` column to `governance_virtual_keys`. Existing rows default to `NULL`/`""` (literal value); no backfill is needed. - **`UpdateVirtualKey`** — `value_source_ref` is included in the explicit `Select` column list so updates persist the reference. - **`GenerateVirtualKeyHash`** — `ValueSourceRef` is now included in the hash so a changed env/vault reference triggers a config resync. - **`mergeGovernanceConfig`** — Removed the inline `env.` prefix resolution logic; env/vault references are now resolved during `TableVirtualKey.UnmarshalJSON`, so `Value` already holds the plaintext by the time the merge runs. `ValueSourceRef` and `ValueSource` are propagated from the existing DB record when the value is carried forward. - **`CreateVirtualKeyRequest`** — Accepts an optional `value` field (literal, reference string, or `SecretVar` object). When omitted, a value is generated server-side as before. - **Config schema** — `value` under `virtual_keys` is updated from `type: string` to `anyOf: [string, object]` to accept `SecretVar` objects. - **SQLite store** — `RegisterVaultCallbacks` is now called on the SQLite DB at init, consistent with the Postgres path. - **`schemasync` ignore list** — The `value` property is added to the ignore list with an explanation of the custom marshalling. - **UI (`governance.ts`)** — `VirtualKey.value` is typed as `string | SecretVar`. A `resolveVirtualKeyValue` helper extracts the usable string from either form. All UI call sites (virtual keys table, MCP usage guide, prompt settings panel, API key selector) use this helper instead of accessing `.value` directly. - **Tests** — `virtualkey_secretvar_test.go` covers env-sourced, vault-sourced, and literal round-trips; hash stability across sources; `MarshalJSON` redaction; and `UnmarshalJSON` accepting bare strings, reference strings, and `SecretVar` objects. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go test ./framework/configstore/... ./framework/configstore/tables/... ./transports/bifrost-http/... # UI cd ui pnpm i pnpm build ``` To exercise env-sourced virtual keys end-to-end: 1. Set an environment variable, e.g. `export MY_VK=sk-bf-test-abc123`. 2. Create a virtual key via the API with `"value": "env.MY_VK"` or `"value": {"from_env": true, "env_var": "env.MY_VK"}`. 3. Confirm the response shows `"value": {"from_env": true, "env_var": "env.MY_VK"}` with the resolved secret redacted. 4. Confirm the `x-bf-vk` auth path still works with the resolved plaintext value. 5. Restart the service and confirm the virtual key is reconstructed correctly from the DB without re-reading the environment. ## Breaking changes - [x] Yes - [ ] No The `value` field on `VirtualKey` in the API response changes from a bare string to a `SecretVar` object for env/vault-sourced keys. Consumers that assumed `value` is always a string will need to handle the object form. Literal-value keys continue to emit a plain string in `value.value` with no other fields set, so the impact is limited to env/vault-sourced keys. The UI is updated accordingly. ## Security considerations - Resolved plaintext virtual key values are never emitted in API responses for env/vault-sourced keys; only the reference and source flags are returned. - `ValueSourceRef` is encrypted at rest alongside `Value` using the same encryption path. - `AfterFind` does not re-resolve the live environment on read, preventing a class of TOCTOU issues where the env var changes after the key is created. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
## Summary
Introduces a pause/resume/end gate for streaming responses, allowing plugins to pause chunk delivery to the client, buffer chunks in-flight, and resume or terminate the stream on demand. Streams that never engage the gate pay no extra cost — they continue to use a direct channel send with `ctx.Done()` guard, identical to the previous behavior.
## Changes
- Added `PauseStream`, `ResumeStream`, and `EndStream` methods to `BifrostContext`, which delegate to the `Tracer` interface and set `BifrostContextKeyStreamGated` to engage the gate.
- Added `PauseStream`, `ResumeStream`, `EndStream`, and `GateSend` to the `Tracer` interface, with no-op implementations on `NoOpTracer`.
- Introduced `GateSendChunk` in `providerUtils` as a drop-in replacement for the inline `select { case responseChan <- chunk: ... case <-ctx.Done(): }` pattern. When `BifrostContextKeyStreamGated` is set, it routes through `Tracer.GateSend`; otherwise it falls through to the same direct send as before.
- Replaced all direct channel send sites in `bifrost.go` and `utils.go` with `GateSendChunk`.
- Added `framework/streaming/gate.go` implementing the per-`StreamAccumulator` gate state machine (`Active → Paused → Active | Ended`), including a lazily-started flusher goroutine that drains the replay buffer when the gate resumes or ends.
- Extended `StreamAccumulator` with gate fields (`gateState`, `gateReplayBuf`, `gateCond`, `gateFlusherCh`, etc.) and added `BifrostContextKeyStreamGated` to the context key registry.
- Wired `PauseStream`, `ResumeStream`, `EndStream`, and `GateSend` through `framework/tracing/tracer.go` to the accumulator layer.
- `cleanupStreamAccumulator` now force-ends the gate and broadcasts to unblock any running flusher goroutine before cleanup.
The key design decision is the fast-path check on `BifrostContextKeyStreamGated`: streams that never call `Pause/Resume/End` skip all gate logic entirely, keeping the hot path allocation-free and lock-free.
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
```sh
go test ./...
```
To validate gate behavior manually:
1. Implement a plugin that calls `ctx.PauseStream()` in `PostLLMHook`, waits a short duration, then calls `ctx.ResumeStream()`. Verify that chunks are buffered during the pause and flushed in order on resume.
2. Call `ctx.EndStream(err)` from a plugin hook and verify that subsequent provider chunks are dropped and the supplied error is delivered as the terminal chunk.
3. Run a streaming request without any plugin engaging the gate and confirm behavior is identical to before this change.
## Breaking changes
- [x] Yes
- [ ] No
The `Tracer` interface has four new required methods: `PauseStream`, `ResumeStream`, `EndStream`, and `GateSend`. Any custom `Tracer` implementation outside this repository must add these methods. The `NoOpTracer` embedded struct can be used to satisfy them with no-ops.
## Related issues
## Security considerations
Buffered chunks are held in memory for the duration of a pause. Plugins that pause indefinitely or on high-throughput streams could cause unbounded memory growth. Callers should ensure `ResumeStream` or `EndStream` is always eventually called.
## 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
## Release Notes
* **New Features**
* Added streaming pause/resume capability allowing consumers to control response flow
* Enabled explicit stream termination with error handling
* **Bug Fixes**
* Improved stream closure and cleanup across all provider integrations (OpenAI, Gemini, Anthropic, Bedrock, Cohere, Azure, and others)
* Enhanced stream state management and buffering during pause operations
* **Tests**
* Comprehensive test coverage for streaming pause/resume/end lifecycle and state transitions
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
… fields, replacing `FromEnv`, `EnvVar`, `FromVault`, `VaultRef` (#4595) ## Summary Consolidates the `SecretVar` type's dual-source tracking fields (`FromEnv`/`EnvVar` and `FromVault`/`VaultRef`) into a unified `SecretType` enum with a single `ref` string field. Both environment variable references (`env.VAR`) and vault references (`vault.path`) are now represented identically, with the reference string stored in `ref` and the source type in `SecretType`. Backward compatibility is preserved for the old `env_var`/`from_env` JSON format. ## Changes - `SecretVar` struct fields `FromEnv`, `EnvVar`, `FromVault`, and `VaultRef` replaced with a `SecretType` enum (`plain_text`, `env`, `vault`) and a single unexported `ref` string. - `IsFromEnv()` and `IsFromVault()` methods supplemented with a unified `IsFromSecret()` method; `GetRawSecretRef()`, `GetRef()`, `EnvKey()`, `VaultPath()`, and `Type()` accessors added. - `MarshalJSON` added to emit `ref` and `type` fields; `UnmarshalJSON`, `Scan`, `Value`, `Redacted`, `FullyRedacted`, `Equals`, `IsSet`, and `ShouldPreserveStored` updated to use the unified fields. - Old `env_var`/`from_env` JSON payloads are transparently migrated to `ref`/`type` on deserialization. - `StoreVaultSecretVar` now sets `ref` to `"vault." + path` and `SecretType` to `SecretTypeVault` instead of populating the old `VaultRef` field. - `removeOwnedVaultSecretVar` updated to use `GetRef()` instead of manually stripping the `vault.` prefix from `VaultRef`. - All call sites across proxy configuration, auth config, encryption hooks, MCP table hooks, config store, plugins, and HTTP handlers updated to use `IsFromSecret()` and `GetRawSecretRef()`. - UI TypeScript `SecretVar` type updated: `env_var`, `from_env`, `vault_var`, and `from_vault` fields replaced with `ref` and `from_secret`; Zod schemas, form utilities, default constants, and all component logic updated accordingly. - Error and warning messages updated from "environment variable" to "external reference" to be source-agnostic. - `config.schema.json` updated to include `ref` and `type` fields alongside the legacy `env_var`/`from_env` fields for backward compatibility. - Tests updated to reflect the new field names and accessors; a dedicated backward-compatibility test added for the old `env_var`/`from_env` JSON format. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh go test ./core/schemas/... go test ./core/providers/utils/... go test ./framework/configstore/... go test ./transports/bifrost-http/... ``` Verify that existing `env.VAR` and `vault.path` references in stored configs continue to resolve correctly after the migration, including configs written in the old `env_var`/`from_env` format. ## Breaking changes - [x] Yes - [ ] No The `SecretVar` JSON representation changes from `env_var`/`from_env`/`vault_var`/`from_vault` to `ref`/`type`. Deserialization of the old format remains supported, but any client or config consuming API responses will receive the new format. The UI has been updated accordingly; external API consumers producing the old format will continue to work on ingest. ## Security considerations Secret source metadata previously exposed separate env var names and vault paths in distinct fields. These are now unified under `ref`, which is preserved in redacted responses so references remain visible to operators without leaking resolved values. The `Scan` method no longer strips quotes before checking for `vault.` prefixes, preventing a quoted literal string such as `"vault.x"` from being misidentified as a vault reference. ## 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
Pratham-Mishra04
force-pushed
the
06-16-feat_adds_mcp_server_oauth_tables
branch
from
9f1fff1 to
0cbfee6
Compare
…pe"]` (#4618) ## Summary Tightens the type of the `type` field in `toOptionalSecretVarPayload` to use `SecretVar["type"]` instead of the loose `string` type, ensuring it aligns with the actual `SecretVar` union/type definition. ## Changes - Replaced `type?: string` with `type?: SecretVar["type"]` in the `toOptionalSecretVarPayload` function parameter type, enforcing that only valid `SecretVar` type values are accepted rather than any arbitrary string. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations No security implications. This is a type-level change only. ## 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
Pratham-Mishra04
force-pushed
the
06-16-feat_adds_mcp_server_oauth_tables
branch
from
0cbfee6 to
d9c0026
Compare
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The merge-base changed after approval.
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
7 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
This PR introduces the foundational OAuth 2.1 authorization server infrastructure for Bifrost's
/mcpendpoint. It adds a configurablemcp_server_auth_modethat controls how inbound MCP clients are authenticated, enabling Bifrost to act as a spec-compliant OAuth 2.1 AS with RFC-mandated discovery endpoints, a JWKS endpoint, and a persistent RS256 signing keypair.Changes
MCPServerAuthMode— newvarcharcolumn onTableClientConfigwith three modes:headers(default): existing VK/api-key/session header auth only; discovery endpoints return 404.both: accepts both header credentials and Bifrost-issued JWTs; discovery endpoints are live.oauth: Bifrost JWTs only; header credentials are rejected on/mcp. Breaking for existing VK-based MCP integrations.OAuth2ServerConfig— new JSON blob column onTableClientConfigholding AS-specific settings (IssuerURL,AuthCodeTTL,AccessTokenTTL). Serialized viaBeforeSave/AfterFindhooks. Only meaningful when mode isbothoroauth.OAuth2SigningKey— RS2048 keypair generated on first use and persisted ingovernance_configunderoauth2_signing_key. The private key PEM is encrypted at rest viaframework/encryptwhen encryption is enabled.GetOAuth2SigningKey— newConfigStoreinterface method that lazily generates and persists the signing keypair on first call, always returning a usable key.OAuth2DiscoveryHandler— serves the three well-known discovery endpoints:GET /.well-known/oauth-protected-resource[/{path}](RFC 9728)GET /.well-known/oauth-authorization-server[/{path}](RFC 8414)GET /.well-known/jwks.json(RFC 7517)All three return 404 when
MCPServerAuthModeisheaders. Routes are always registered; the mode flag is the feature toggle.oauth2IssuerURL/oauth2ServerCfg— utility helpers that resolve the effective issuer URL (configuredIssuerURLor request-derived fallback) and AS config defaults.OAuth2ConsentScopeName— new temp-token scope for binding browser sessions to pending authorization requests on the public consent page.add_oauth2_server_tables— addsmcp_server_auth_modeandoauth2_server_config_jsoncolumns toconfig_client.mcp_server_auth_modeandoauth2_server_configadded toconfig.schema.jsonwith full descriptions and validation.IsMCPOAuthDiscoveryEnabled— helper onClientConfigthat returns true when mode isbothoroauth.Type of change
Affected areas
How to test
go test ./framework/configstore/... ./transports/bifrost-http/...mcp_server_auth_modeunset (or"headers"). ConfirmGET /.well-known/oauth-authorization-serverreturns 404.mcp_server_auth_modeto"both"and restart. Confirm:GET /.well-known/oauth-authorization-serverreturns a valid JSON document withissuer,authorization_endpoint,token_endpoint, etc.GET /.well-known/oauth-protected-resourcereturns a document pointing to/mcp.GET /.well-known/jwks.jsonreturns a JWKS with one RS256 key entry.governance_configand survives a restart (samekidreturned).mcp_server_auth_modeto"oauth"and confirm header-credential MCP requests are rejected.New config fields:
mcp_server_auth_mode"headers"|"both"|"oauth""headers"oauth2_server_config.issuer_urloauth2_server_config.auth_code_ttl600oauth2_server_config.access_token_ttl600Breaking changes
Setting
mcp_server_auth_modeto"oauth"disables VK/api-key/session header authentication on/mcp. Existing virtual-key MCP integrations will stop working. Use"both"for a non-breaking migration path that accepts both credential types simultaneously.Security considerations
framework/encryptwhen encryption is enabled. The plaintext key is only held in memory during the signing operation.OAuth2ConsentScopeNametemp token is the sole credential binding a browser session to a pending authorization request on the public (unauthenticated) consent page — it must be treated as a short-lived secret.issuer_url; omitting it causes token verification failures when theHostheader differs across nodes.Checklist
docs/contributing/README.mdand followed the guidelines