Skip to content

Propagate MCPOIDCConfig CA bundle to vmcp OIDC config#4923

Open
ChrisJBurns wants to merge 2 commits intomainfrom
cburns/vmcp-oidc-cabundle
Open

Propagate MCPOIDCConfig CA bundle to vmcp OIDC config#4923
ChrisJBurns wants to merge 2 commits intomainfrom
cburns/vmcp-oidc-cabundle

Conversation

@ChrisJBurns
Copy link
Copy Markdown
Collaborator

Summary

VirtualMCPServer referencing an MCPOIDCConfig with a ConfigMap-backed caBundleRef mounted the CA into the pod but never told the vmcp binary where to find it, so OIDC discovery against a self-signed issuer failed with x509: certificate signed by unknown authority. This PR closes the plumbing gap by mirroring the earlier MCPServer fix (#3142, #3391): the vMCP OIDCConfig now carries the mounted CA path, and the incoming-OIDC middleware loads it into its HTTP trust store.

Closes #4918

Medium level
  • Type: Added CABundlePath to pkg/vmcp/config.OIDCConfig (consistent with StaticBackendConfig.CABundlePath) and validated it in the vMCP config validator (absolute path, no null bytes, no traversal).
  • Converter: Populated CABundlePath from resolved.ThvCABundlePath inside mapResolvedOIDCToVmcpConfigFromRef so both inline caBundleRef and the Kubernetes service-account CA default flow through.
  • Middleware: Wired oidcCfg.CABundlePath into auth.TokenValidatorConfig.CACertPath inside newOIDCAuthMiddleware; the downstream HTTP client builder already reads the field and appends it to the TLS trust store.
  • Tests: Unit tests at the config validator, converter, and auth factory layers, plus an envtest scenario that stands up a VirtualMCPServer with a ConfigMap-backed caBundleRef and asserts both the rendered vmcp config ConfigMap and the Deployment's CA volume mount.
Low level
File Change
pkg/vmcp/config/config.go Added optional CABundlePath string field to OIDCConfig with doc comment and JSON/YAML tag caBundlePath.
pkg/vmcp/config/validator.go Validate OIDCConfig.CABundlePath — reject null bytes, .., and non-absolute paths.
pkg/vmcp/config/validator_test.go 4 new table cases covering valid absolute, relative, traversal, and null-byte paths.
cmd/thv-operator/pkg/vmcpconfig/converter.go Pass resolved.ThvCABundlePath through to vmcpconfig.OIDCConfig.CABundlePath in mapResolvedOIDCToVmcpConfigFromRef.
cmd/thv-operator/pkg/vmcpconfig/converter_test.go 3 new converter cases (inline CA, K8s SA default CA, empty).
pkg/vmcp/auth/factory/incoming.go Forward oidcCfg.CABundlePath into auth.TokenValidatorConfig.CACertPath.
pkg/vmcp/auth/factory/incoming_cabundle_test.go New table test: empty / valid PEM / non-existent path (proves the field is actually consumed).
cmd/thv-operator/test-integration/mcp-oidc-config/mcpoidcconfig_virtualmcpserver_integration_test.go New envtest Context: asserts rendered vmcp config ConfigMap contains caBundlePath: /config/certs/<cm>/<key> and Deployment mounts the ConfigMap read-only.
deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml Regenerated — adds caBundlePath under spec.config.incomingAuth.oidc.
deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml Regenerated (Helm template wrapper).
docs/operator/crd-api.md Regenerated.

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring
  • Documentation
  • Other

Test plan

  • task lint-fix passes (0 issues)
  • task test passes
  • task build passes
  • New unit tests at validator, converter, and OIDC middleware factory layers
  • New envtest integration scenario exercises the full operator → vmcp config plumbing
  • Regenerated CRD manifests and CRD API docs are up to date (verified via re-run)

Special notes for reviewers

  • Field name: I chose CABundlePath (matching StaticBackendConfig.CABundlePath in the same package) over CACertPath (downstream auth.TokenValidatorConfig naming) or ThvCABundlePath (operator-resolver internal name) for consistency within pkg/vmcp/config.
  • Factory test validates wiring: The non-existent-path case asserts middleware construction fails — proving the field is actually consumed by the downstream HTTP client builder, not silently dropped.
  • Kubernetes service-account path: The resolver already sets ThvCABundlePath = defaultK8sCABundlePath in resolveFromK8sServiceAccountConfig when useClusterAuth is true, so that path now works automatically. Covered by a dedicated converter test case.
  • Pre-existing drift note: pkg/audit/zz_generated.deepcopy.go is stale relative to the DetectApplicationErrors field on pkg/audit.Config. Surfaced when running task operator-generate, but it's out of scope for this PR — would recommend a separate follow-up to regenerate.

Generated with Claude Code

ChrisJBurns and others added 2 commits April 18, 2026 00:04
VirtualMCPServer referencing an MCPOIDCConfig with a ConfigMap-backed
caBundleRef mounted the CA into the pod but never told the vmcp binary
where to find it. OIDC discovery against a self-signed issuer then
failed with "x509: certificate signed by unknown authority".

The vmcp OIDCConfig had no CA-bundle field, so the operator converter
silently dropped the resolver's ThvCABundlePath. This mirrors the
earlier MCPServer fix (#3142, #3391).

- Add CABundlePath to pkg/vmcp/config.OIDCConfig with absolute-path,
  no-traversal, no-null-byte validation consistent with
  StaticBackendConfig.CABundlePath.
- Populate CABundlePath from resolved.ThvCABundlePath in the operator's
  mapResolvedOIDCToVmcpConfigFromRef, covering both inline caBundleRef
  and the Kubernetes service-account CA default.
- Wire CABundlePath into auth.TokenValidatorConfig.CACertPath in
  newOIDCAuthMiddleware so the incoming-OIDC middleware loads the CA
  into its HTTP client for discovery, JWKS, and introspection.
- Add unit tests at each layer and a VirtualMCPServer envtest scenario
  asserting the rendered vmcp-config ConfigMap contains caBundlePath
  and the Deployment mounts the ConfigMap read-only at the expected
  path.

Closes #4918

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
task operator-manifests + task crdref-gen after adding
CABundlePath to pkg/vmcp/config.OIDCConfig.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label Apr 17, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.39%. Comparing base (e4bfbfe) to head (5f237db).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4923      +/-   ##
==========================================
+ Coverage   69.36%   69.39%   +0.02%     
==========================================
  Files         538      538              
  Lines       55462    55469       +7     
==========================================
+ Hits        38473    38491      +18     
+ Misses      14033    14026       -7     
+ Partials     2956     2952       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Small, focused fix that mirrors the established StaticBackendConfig.CABundlePath validation pattern and the earlier MCPServer CA-bundle propagation fixes (#3142, #3391).

Tests cover every layer:

  • Validator: valid/relative/../null-byte cases
  • Operator converter: inline, KSA default, and empty ThvCABundlePath cases
  • Auth middleware wiring: valid PEM, empty, and nonexistent-path (proves the field is consumed, not silently dropped)
  • Envtest: asserts the rendered vmcp-config ConfigMap contains the mounted CA path AND the Deployment mounts the ConfigMap read-only at /config/certs/<cm-name>

Non-blocking nit: pkg/vmcp/config/validator.go:121-129 and :169-177 are now two near-identical CA-bundle validation blocks. Extracting a validateCABundlePath(path, field string) error helper would prevent drift if a third caller ever appears — but with only two call sites, leaving it inline is defensible.

@ChrisJBurns
Copy link
Copy Markdown
Collaborator Author

ChrisJBurns commented Apr 21, 2026

@jerm-dro I think this is one of those changes that reveal the leakage onto CRDs. The caBundlePath exists on the MCPOIDCConfig already, I'm trying to wire it into the vMCP binary so it can use it but I think by adding it to the vmcp config struct it leaks onto the CRD unnecessarily. (I'll working on the RFC for the model-drift - I think there are multiple phases. shall put more info on the rfc)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VirtualMCPServer ignores MCPOIDCConfig.caBundleRef — CA is mounted but never read

3 participants