fix: encrypt sensitive static config - #7699
Conversation
547be7a to
d0877ad
Compare
|
@claude review |
There was a problem hiding this comment.
🟡 Changes recommended
Credential precedence, rollback gaps, and plaintext exposure during drift status updates must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Encrypts sensitive static MCP configuration while preserving runtime use and redacted API/UI status.
Changes:
- Adds static-secret extraction, storage, migration, rehydration, and cleanup.
- Integrates encrypted configuration across APIs, controllers, catalog sync, and runtime paths.
- Adds
valueConfiguredUI handling and tests.
File summaries
| File | Description |
|---|---|
apiclient/types/mcpserver.go |
Adds configured-value status. |
pkg/storage/openapi/generated/openapi_generated.go |
Updates generated schemas. |
pkg/storage/apis/obot.obot.ai/v1/constants.go |
Adds system catalog finalizer. |
pkg/mcp/staticcredentials.go |
Manages static credentials. |
pkg/mcp/staticconfig.go |
Extracts, redacts, and rehydrates secrets. |
pkg/mcp/staticconfig_test.go |
Tests static configuration handling. |
pkg/mcp/types.go |
Rehydrates runtime manifests. |
pkg/mcp/action.go |
Merges runtime and static credentials. |
pkg/mcp/action_test.go |
Updates helper coverage. |
pkg/controller/routes.go |
Registers migration and cleanup handlers. |
pkg/controller/migrate.go |
Removes superseded migration helpers. |
pkg/controller/migrate_test.go |
Removes obsolete tests. |
pkg/controller/handlers/systemmcpserver/systemmcpserver.go |
Migrates and rehydrates system servers. |
pkg/controller/handlers/systemmcpserver/systemmcpserver_test.go |
Tests encrypted system configuration. |
pkg/controller/handlers/mcpservercatalogentry/mcpservercatalogentry.go |
Adds migration, drift, and cleanup logic. |
pkg/controller/handlers/mcpservercatalogentry/mcpservercatalogentry_test.go |
Adds gateway-backed test setup. |
pkg/controller/handlers/mcpserver/mcpserver.go |
Integrates secrets into server reconciliation. |
pkg/controller/handlers/mcpserver/mcpserver_test.go |
Updates drift tests. |
pkg/controller/handlers/mcpcatalog/mcpcatalog.go |
Credentializes synchronized catalogs. |
pkg/controller/handlers/mcpcatalog/staticconfig_test.go |
Tests sync rollback and composites. |
pkg/api/handlers/mcp.go |
Integrates encrypted server configuration. |
pkg/api/handlers/mcp_test.go |
Expands conversion and rollback tests. |
pkg/api/handlers/mcpcatalogs.go |
Secures catalog CRUD and previews. |
pkg/api/handlers/mcpcatalogs_test.go |
Tests component rehydration. |
pkg/api/handlers/systemmcpcatalogs.go |
Secures system catalog CRUD. |
pkg/api/handlers/systemmcpcatalogs_test.go |
Updates conversion tests. |
pkg/api/handlers/systemmcpserver.go |
Secures system server CRUD. |
pkg/api/handlers/systemmcpserver_test.go |
Tests encrypted conversion. |
pkg/api/handlers/mcpwebhookvalidation.go |
Secures webhook static configuration. |
pkg/api/handlers/mcpwebhookvalidation_test.go |
Tests webhook redaction. |
pkg/api/handlers/mcphelpers.go |
Adds credential helpers. |
pkg/api/handlers/registry/handler.go |
Merges registry credentials. |
pkg/api/handlers/poweruserworkspace.go |
Updates catalog conversion. |
pkg/api/handlers/gateway_test.go |
Adds shared gateway test client. |
ui/user/src/lib/services/user/types.ts |
Adds user configured-value status. |
ui/user/src/lib/services/admin/types.ts |
Adds admin configured-value status. |
ui/user/src/lib/components/mcp/CustomConfigurationFieldset.svelte |
Handles encrypted placeholders. |
ui/user/src/lib/components/mcp/CustomConfigurationFieldset.svelte.spec.ts |
Tests encrypted field behavior. |
ui/user/src/lib/components/mcp/RemoteRuntimeForm.svelte |
Handles encrypted remote headers. |
Review details
Suppressed comments (1)
pkg/api/handlers/mcp.go:4016
- The static credential has already been changed when shutdown runs. If shutdown fails, the handler returns while the Kubernetes manifest remains unchanged, so a failed update can still replace or remove the server's effective static secret. Restore
existingServerSecretson this error path before returning.
// Shutdown the server, even if there is no credential
if err := m.removeMCPServer(req.Context(), server); err != nil {
return err
}
- Files reviewed: 38/39 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d4bdc6d to
9adaa7d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Runtime hydration, webhook propagation, synchronization rollback, and creation races can lose or misapply static configuration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 38/39 changed files
- Comments generated: 5
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Migration ordering, credential cleanup, runtime hydration, component restarts, and synchronization consistency contain unresolved defects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
pkg/controller/handlers/mcpcatalog/mcpcatalog.go:623
- Credential rollback only covers extraction/storage failures. Both sync paths call this method before
app.Apply; if applying objects then fails, the committed credentials remain paired with old or partially updated manifests, so changed/renamed static fields can break either version. Coordinate credential commits with apply (for example, stage changes and reconcile each credential against the object version that actually persisted) and recover correctly from partial applies.
// credentializeCatalogObjects extracts sensitive static configuration from catalog manifests
// into encrypted credentials before the objects are persisted. If credentialization fails,
// credentials changed earlier in the batch are restored; object application is handled separately.
- Files reviewed: 41/42 changed files
- Comments generated: 5
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Credential consistency races, unordered precedence, and env/header key collisions can lose or misapply secrets.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/controller/handlers/mcpcatalog/mcpcatalog.go:618
- System catalog credentials are also changed before
app.Applywith no rollback if object application fails. That can leave the persisted system entry referring to values that were replaced or deleted by a failed sync. Retain the prior credential state and restore it whenever Apply returns an error.
pkg/controller/handlers/mcpcatalog/mcpcatalog.go:205
- The static credential is committed before
app.Apply, but it is not restored if applying the catalog objects fails. For example, a sync that removes a secret can delete it here, then leave the old catalog entry (which still requires it) in storage when Apply errors. Keep the previous credentials until Apply succeeds and roll them back on an Apply failure.
if err := h.credentializeCatalogObjects(req.Ctx, toAdd); err != nil {
return fmt.Errorf("failed to credentialize synced catalog configuration: %w", err)
}
return app.Apply(req.Ctx, mcpCatalog, toAdd...)
pkg/controller/handlers/mcpservercatalogentry/mcpservercatalogentry.go:72
- The system catalog migration has the same lost-update race: it writes the credential before the versioned object update, but does not restore
existingon conflict. A concurrent API update can therefore succeed while its new secret is silently replaced by the stale plaintext being migrated.
if err := mcp.StoreStaticCredentialSecrets(req.Ctx, h.gatewayClient, mcp.SystemCatalogEntryStaticCredentialContext(entry.Name), entry.Name, secrets); err != nil {
return fmt.Errorf("failed to store static configuration before migration: %w", err)
}
return req.Client.Update(req.Ctx, entry)
- Files reviewed: 42/43 changed files
- Comments generated: 5
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Secret-key collisions, nondeterministic precedence, and incomplete rollback paths can corrupt or expose incorrect runtime configuration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/api/handlers/mcpwebhookvalidation.go:175
- The static credential is updated before the webhook credential, but the later upsert error returns without restoring
existingStaticSecrets. The old manifest then remains persisted while its static values have already changed. Restore the static credential before returning that error.
pkg/mcp/staticconfig.go:198
- Environment variables and remote headers use the same secret path, so valid fields with the same key overwrite each other. The new test demonstrates this by extracting
env=envandheader=headerinto oneTOKENentry and then hydrating both asheader; configurations that previously supplied distinct static values will silently change behavior. Namespace paths by field kind (for example,env/...andheader/...) and account for already-written credentials during migration.
pkg/controller/handlers/mcpcatalog/mcpcatalog.go:205 - Credential updates are committed before
app.Apply, but an apply failure does not invoke the rollback captured bycredentializeCatalogObjects. A transient Kubernetes conflict can therefore leave the persisted catalog entry unchanged while its encrypted static values have already been replaced or deleted. Roll back the batch when apply fails, or stage/apply these changes so the object and credential cannot diverge.
if err := h.credentializeCatalogObjects(req.Ctx, toAdd); err != nil {
return fmt.Errorf("failed to credentialize synced catalog configuration: %w", err)
}
return app.Apply(req.Ctx, mcpCatalog, toAdd...)
pkg/api/handlers/mcpgateway/handler.go:456
- This hot path still lists every credential in the system-server context and copies them in unspecified database order. Once static configuration is stored separately, a user credential containing the same key can overwrite the protected static value (or vice versa), unlike
RuntimeCredentialSecrets, which explicitly gives static values precedence. Load the user and static credentials separately and merge them with deterministic static precedence.
if !needsCredentials && systemServer.Spec.Manifest.RemoteConfig != nil {
for _, header := range systemServer.Spec.Manifest.RemoteConfig.Headers {
if header.Value == "" {
needsCredentials = true
break
}
}
- Files reviewed: 42/43 changed files
- Comments generated: 2
- Review effort level: Balanced
3342b19 to
067bd27
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Static-value collisions, incomplete automatic-connect propagation, and non-atomic synchronization can lose or misapply credentials.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
pkg/mcp/staticconfig.go:199
- Environment variables and remote headers use the same credential path, so a manifest containing both with the same key loses one value during extraction. The new test demonstrates this by storing only the header value and hydrating that value into both fields, which changes existing inline configurations during migration. Namespace the path by field kind so the two values remain distinct.
pkg/controller/handlers/mcpcatalog/mcpcatalog.go:205 - Credentials are committed before
app.Apply, but a later apply failure does not restore them. Existing catalog objects can therefore keep their old manifest while immediately using new, deleted, or differently keyed static secrets; in particular, removing a field from the desired source deletes its credential even if the object update fails. Keep rollback state through the apply and reconcile credential/object updates so failures cannot leave mismatched runtime configuration.
if err := h.credentializeCatalogObjects(req.Ctx, toAdd); err != nil {
return fmt.Errorf("failed to credentialize synced catalog configuration: %w", err)
}
return app.Apply(req.Ctx, mcpCatalog, toAdd...)
pkg/controller/handlers/mcpcatalog/mcpcatalog.go:618
- The system-catalog sync likewise commits all static credential changes before
app.Applyand has no rollback when apply fails. A failed or partially failed apply can leave existing system catalog manifests paired with credentials from the unapplied desired version, changing or breaking deployed servers despite the sync error. Coordinate per-object credential updates with apply outcomes or retain enough state to reconcile partial success safely.
if err := h.credentializeCatalogObjects(req.Ctx, toAdd); err != nil {
return fmt.Errorf("failed to credentialize synced catalog configuration: %w", err)
}
return app.Apply(req.Ctx, systemCatalog, toAdd...)
- Files reviewed: 52/53 changed files
- Comments generated: 4
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
OAuth creation, composite readiness, credential key collisions, and sync failure handling contain unresolved correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
pkg/api/handlers/mcp.go:1051
- This OAuth-consent creation path still converts the redacted catalog manifest directly and never copies its static credential. A server created here therefore retains
valueConfigured: truebut has no server-scoped static secret; the later connect path finds the existing server and will not repair it, so required static values are unavailable at runtime. Mirror the hydration/extract/store flow added toSessionManager.serverOrInstanceFromConnectURL, including cleanup if credential storage fails.
mcp.AddExtractedEnvVarsToCatalogEntry(&entry)
pkg/mcp/staticconfig.go:199
- Environment variables and headers use the identical credential path, so an env and header sharing a key overwrite each other; hydration then injects the surviving value into both fields. Root keys are also left unescaped and can collide with generated component paths. Namespace paths by field kind (for example
env/...andheader/...) and encode root keys, with migration compatibility for existing credentials.
pkg/controller/handlers/mcpcatalog/mcpcatalog.go:205 - Static credentials are committed before the catalog objects, but an
app.Applyfailure returns without restoring them. Existing manifests can then run with newly changed or deleted secrets that were never applied to the catalog resource. Make credentialization return a compensation step (or otherwise roll back the batch) when object application fails.
if err := h.credentializeCatalogObjects(req.Ctx, toAdd); err != nil {
return fmt.Errorf("failed to credentialize synced catalog configuration: %w", err)
}
return app.Apply(req.Ctx, mcpCatalog, toAdd...)
pkg/controller/handlers/mcpcatalog/mcpcatalog.go:618
- The system-catalog sync has the same non-atomic failure path: credentials are replaced before
app.Apply, and an apply error leaves old resources paired with new/deleted secrets. Roll back credential changes when application fails so runtime configuration cannot diverge from the persisted catalog.
if err := h.credentializeCatalogObjects(req.Ctx, toAdd); err != nil {
return fmt.Errorf("failed to credentialize synced catalog configuration: %w", err)
}
return app.Apply(req.Ctx, systemCatalog, toAdd...)
- Files reviewed: 52/53 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Static key collisions, an incomplete OAuth connect path, and non-atomic catalog synchronization can lose or misapply secrets.
Review details
Suppressed comments (4)
pkg/mcp/staticconfig.go:198
- Environment variables and headers share the same credential key here. If both define the same key with different static values, the header overwrites the environment value and hydration restores that one value into both fields (the added
TestStaticConfigurationEnvAndHeaderShareUserStyleKeycurrently demonstrates this corruption). Include the field kind in both extraction and hydration paths so these values remain collision-safe.
pkg/api/handlers/mcp.go:1051 - This connect/OAuth path still converts and creates a server directly from the redacted catalog manifest. Unlike the parallel
SessionManager.serverOrInstanceFromConnectURLpath, it never reveals the catalog static credential or copies it to the new server, so a first OAuth consent connection creates a server whosevalueConfiguredfields have no backing values and startup fails. Apply the same hydrate-before-conversion and extract/store-after-create flow here.
mcp.AddExtractedEnvVarsToCatalogEntry(&entry)
pkg/controller/handlers/mcpcatalog/mcpcatalog.go:205
- Credentials are committed before
app.Apply, but they are not restored if applying the catalog objects fails. For an existing entry, a failed sync can therefore make the old persisted manifest run with the new static secret even though the sync reports failure. Keep the previous credentials until apply succeeds, or expose and invoke a rollback when apply fails.
if err := h.credentializeCatalogObjects(req.Ctx, toAdd); err != nil {
return fmt.Errorf("failed to credentialize synced catalog configuration: %w", err)
}
return app.Apply(req.Ctx, mcpCatalog, toAdd...)
pkg/controller/handlers/mcpcatalog/mcpcatalog.go:618
- The system-catalog path also commits every static credential before
app.Applyand does not restore them when apply fails. This can leave existing system catalog manifests paired with secrets from a sync that never completed. Coordinate credential commit/rollback with the apply result.
if err := h.credentializeCatalogObjects(req.Ctx, toAdd); err != nil {
return fmt.Errorf("failed to credentialize synced catalog configuration: %w", err)
}
return app.Apply(req.Ctx, systemCatalog, toAdd...)
- Files reviewed: 54/55 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Summary
Details
Upgrade behavior
Existing inline static values are extracted into encrypted credentials by the migration included in this PR.