Skip to content

Commit b70b8c9

Browse files
JAORMXclaude
andcommitted
Address review feedback on MCPServerEntry RFC
- Clarify groupRef is plain string for consistency with MCPServer/MCPRemoteProxy - Fix Alt 1 YAML example to use string form for groupRef - Change caBundleRef to reference ConfigMap (CA certs are public data) - Add SSRF rationale: CEL IP blocking omitted since internal servers are legitimate - Clarify auth resolution loads config only, token exchange deferred to request time - Specify CA bundle volume mount for static mode (PEM files, not env vars) - Document toolConfigRef migration path via aggregation.tools[].workload Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 0952cb2 commit b70b8c9

1 file changed

Lines changed: 48 additions & 19 deletions

File tree

rfcs/THV-0055-mcpserverentry-direct-remote-backends.md

Lines changed: 48 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,8 @@ spec:
236236
key: api-key
237237

238238
# OPTIONAL: Custom CA bundle for private remote servers using
239-
# internal/self-signed certificates.
239+
# internal/self-signed certificates. References a ConfigMap (not Secret)
240+
# because CA certificates are public data.
240241
caBundleRef:
241242
name: internal-ca-bundle
242243
key: ca.crt
@@ -323,10 +324,10 @@ URL, Transport, Group, Ready status, and Age.
323324
|-------|------|----------|-------------|
324325
| `remoteURL` | string | Yes | URL of the remote MCP server. Must match `^https?://`. HTTPS enforced unless `toolhive.stacklok.dev/allow-insecure` annotation is set. |
325326
| `transport` | enum | Yes | MCP transport protocol: `streamable-http` or `sse`. |
326-
| `groupRef` | string | Yes | Name of the MCPGroup this entry belongs to (min length: 1). |
327+
| `groupRef` | string | Yes | Name of the MCPGroup this entry belongs to (min length: 1). Uses a plain string (not `LocalObjectReference`) for consistency with `MCPServer.spec.groupRef` and `MCPRemoteProxy.spec.groupRef`. |
327328
| `externalAuthConfigRef` | object | No | Reference to an MCPExternalAuthConfig in the same namespace. Omit for unauthenticated endpoints. |
328329
| `headerForward` | object | No | Header forwarding configuration. Reuses existing `HeaderForwardConfig` type from MCPRemoteProxy. |
329-
| `caBundleRef` | object | No | Reference to a Secret containing a custom CA certificate bundle for TLS verification. |
330+
| `caBundleRef` | object | No | Reference to a ConfigMap containing a custom CA certificate bundle for TLS verification. ConfigMap is used rather than Secret because CA certificates are public data, consistent with the `kube-root-ca.crt` pattern. |
330331

331332
**Status fields:**
332333

@@ -463,8 +464,11 @@ resources. For each MCPServerEntry in the group, vMCP:
463464
1. Lists MCPServerEntry resources filtered by `spec.groupRef`.
464465
2. Converts each entry to an internal `Backend` struct using the entry's
465466
`remoteURL`, `transport`, and name.
466-
3. Resolves `externalAuthConfigRef` if set (using existing auth resolution
467-
logic).
467+
3. If `externalAuthConfigRef` is set, loads the referenced
468+
MCPExternalAuthConfig spec and stores the auth strategy (token exchange
469+
endpoint, client credentials reference, audience) in the `Backend`
470+
struct. Actual token exchange is deferred to request time because
471+
tokens are short-lived and may be per-user.
468472
4. Resolves `headerForward` configuration if set.
469473
5. Resolves `caBundleRef` if set (fetching the CA certificate from the
470474
referenced Secret).
@@ -517,7 +521,7 @@ external TLS support.
517521
|--------|-------------|------------|
518522
| Man-in-the-middle on remote connection | Attacker intercepts vMCP-to-remote traffic | HTTPS required by default; custom CA bundles for private CAs |
519523
| Credential exposure in CRD spec | Auth secrets visible in CRD manifest | Credentials stored in K8s Secrets, referenced via `externalAuthConfigRef` and `headerForward.addHeadersFromSecrets`; never inline in CRD spec |
520-
| SSRF via remoteURL | Operator configures URL pointing to internal services | Mitigated by RBAC (only authorized users create MCPServerEntry); annotation required for non-HTTPS; NetworkPolicy should restrict vMCP egress |
524+
| SSRF via remoteURL | Operator configures URL pointing to internal services | Mitigated by RBAC (only authorized users create MCPServerEntry); annotation required for non-HTTPS; NetworkPolicy should restrict vMCP egress. Note: CEL-based IP range blocking (e.g., RFC 1918) is intentionally not applied because MCPServerEntry legitimately targets internal/corporate MCP servers. RBAC is the appropriate control layer since resource creation is restricted to trusted operators. |
521525
| Auth config confusion (existing issue) | Dual-boundary auth leading to wrong tokens sent to wrong endpoints | Eliminated: MCPServerEntry has exactly one auth boundary with one purpose |
522526
| Operator probing external URLs | Controller making network requests to untrusted URLs | Eliminated: controller performs validation only, no network probing |
523527

@@ -543,7 +547,8 @@ external TLS support.
543547
- **At rest**: No sensitive data stored in MCPServerEntry spec. Auth
544548
credentials are in K8s Secrets, referenced indirectly.
545549
- **CA bundles**: Custom CA certificates referenced via `caBundleRef`,
546-
stored in K8s Secrets/ConfigMaps with standard K8s encryption at rest.
550+
stored in K8s ConfigMaps. CA certificates are public data and do not
551+
require Secret-level protection.
547552

548553
### Input Validation
549554

@@ -563,6 +568,17 @@ external TLS support.
563568
- **Dynamic mode**: vMCP reads secrets at runtime via K8s API
564569
(namespace-scoped RBAC).
565570
- **Static mode**: Operator mounts secrets as environment variables.
571+
- **CA bundle propagation** differs from credential secrets because CA
572+
certificates are multi-line PEM data that must be loaded from the
573+
filesystem (Go's `crypto/tls` loads CA bundles via file reads, not
574+
environment variables):
575+
- **Dynamic mode**: vMCP reads the CA bundle data from the K8s API
576+
at runtime (from the ConfigMap referenced by `caBundleRef`).
577+
- **Static mode**: The operator mounts the ConfigMap referenced by
578+
`caBundleRef` as a **volume** into the vMCP pod at a well-known
579+
path (e.g., `/etc/toolhive/ca-bundles/<entry-name>/ca.crt`). The
580+
generated backend ConfigMap includes the mount path so vMCP can
581+
construct the `tls.Config` at startup.
566582
- Secret rotation follows existing patterns:
567583
- **Dynamic mode**: Watch-based propagation, no pod restart needed.
568584
- **Static mode**: Requires pod restart (Deployment rollout).
@@ -596,8 +612,7 @@ Embed remote server configuration directly in the VirtualMCPServer CRD.
596612
```yaml
597613
kind: VirtualMCPServer
598614
spec:
599-
groupRef:
600-
name: engineering-team
615+
groupRef: engineering-team
601616
remoteServerRefs:
602617
- name: context7
603618
remoteURL: https://mcp.context7.com/mcp
@@ -724,10 +739,14 @@ MCPServerEntry is a purely additive change:
724739
1. Update VirtualMCPServer controller to discover MCPServerEntry resources
725740
in the group
726741
2. Update ConfigMap generation to include entry-type backends
727-
3. Update vMCP static config parser to deserialize entry backends
728-
4. Add `BackendTypeEntry` to vMCP types
729-
5. Implement external TLS transport creation for entry backends
730-
6. Integration tests with envtest
742+
3. Mount CA bundle ConfigMaps as volumes into the vMCP pod for entries
743+
that specify `caBundleRef` (at a well-known path such as
744+
`/etc/toolhive/ca-bundles/<entry-name>/`)
745+
4. Update vMCP static config parser to deserialize entry backends
746+
5. Add `BackendTypeEntry` to vMCP types
747+
6. Implement external TLS transport creation for entry backends
748+
(loading CA bundles from mounted volume paths)
749+
7. Integration tests with envtest
731750

732751
### Phase 3: Dynamic Mode Integration
733752

@@ -819,8 +838,10 @@ MCPServerEntry is a purely additive change:
819838
allowing local development workflows.
820839
821840
2. **Should the CRD support custom CA bundles for private remote servers?**
822-
Recommendation: Yes, via `caBundleRef` field referencing a Secret or
823-
ConfigMap. This is essential for enterprises with internal CAs. The
841+
Recommendation: Yes, via `caBundleRef` field referencing a ConfigMap.
842+
CA certificates are public data and ConfigMap is the semantically
843+
appropriate resource type, consistent with the `kube-root-ca.crt`
844+
pattern. This is essential for enterprises with internal CAs. The
824845
current design includes this field.
825846

826847
3. **Should there be a `disabled` field for temporarily removing an entry
@@ -832,10 +853,18 @@ MCPServerEntry is a purely additive change:
832853
4. **Should MCPServerEntry support `toolConfigRef` for tool filtering?**
833854
MCPRemoteProxy supports tool filtering via `toolConfigRef`.
834855
VirtualMCPServer also has its own tool filtering/override configuration
835-
in `spec.aggregation.tools`. For MCPServerEntry, tool filtering should
836-
be configured at the VirtualMCPServer level (where it already exists)
837-
rather than duplicating it on the entry. Defer unless there is a clear
838-
use case for entry-level filtering.
856+
in `spec.aggregation.tools`, which supports per-backend filtering via
857+
the `workload` field (e.g., `tools: [{workload: "salesforce", filter: [...]}]`).
858+
For MCPServerEntry, tool filtering should be configured at the
859+
VirtualMCPServer level rather than duplicating it on the entry.
860+
**Migration note:** Users migrating from MCPRemoteProxy who rely on
861+
`toolConfigRef` for per-backend tool filtering should configure
862+
equivalent filtering in `VirtualMCPServer.spec.aggregation.tools`
863+
with the `workload` field set to the MCPServerEntry name. If
864+
post-implementation feedback reveals that `aggregation.tools` is
865+
insufficient for per-backend filtering use cases, `toolConfigRef`
866+
can be added to MCPServerEntry in a follow-up without breaking
867+
changes.
839868

840869
## References
841870

0 commit comments

Comments
 (0)