Trust private CAs for upstream auth servers

[jhrozek]

Summary

• Private-CA OAuth2 and OIDC upstreams could not complete discovery, token, user-info, or dynamic client registration requests, leaving in-cluster IdPs behind internal PKI unusable.
• Add per-upstream caBundleRef support, PEM validation, read-only ConfigMap projection, and additive system-root-plus-private-CA TLS trust for embedded auth-server clients.
• Roll workloads when a selected CA bundle changes or is removed, and surface invalid bundle content as a terminal status condition rather than repeatedly retrying reconciliation.

Fixes #6417

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (task lint-fix)
• Manual testing (controller coverage via task operator-test, including CA rotation/removal and terminal invalid-bundle status handling)

API Compatibility

• This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Changes

File(s)	Change
cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go, generated CRDs	Add upstream caBundleRef API documentation and generated schema updates.
cmd/thv-operator/pkg/controllerutil/{authserver,ca_bundle}.go	Validate, project, hash, and configure upstream CA bundles.
cmd/thv-operator/controllers/{mcpserver,mcpremoteproxy,virtualmcpserver}_*	Watch referenced ConfigMaps and roll workloads on CA changes.
pkg/networking/http_client.go	Add additive system-root-plus-bundle trust configuration.
pkg/authserver/**, pkg/auth/dcr/**	Pass per-upstream CA paths through OAuth2, OIDC, and DCR clients.
docs/arch/**, docs/operator/crd-api.md, example manifest	Document trust semantics and provide a private-CA configuration example.
Associated *_test.go files	Cover validation, client trust semantics, DCR propagation, watches, and rollout drift.

Does this introduce a user-facing change?

Yes. Operators can configure caBundleRef on embedded-auth OIDC and OAuth2 upstreams to trust an internal CA. The bundle augments system trust roots; it does not pin the upstream to that CA or disable publicly trusted certificates.

Special notes for reviewers

This is one vertical feature spanning CRD API, operator projection and rollout behavior, and runtime TLS clients. Please pay particular attention to the intentional additive trust semantics and to the checksum-driven rollout required because subPath ConfigMap mounts do not refresh in running pods.

[codecov]

Codecov Report

❌ Patch coverage is 78.45304% with 117 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.88%. Comparing base (9a9686c) to head (fbe8778).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...perator/controllers/virtualmcpserver_controller.go	60.81%	29 Missing ⚠️
...d/thv-operator/controllers/mcpserver_controller.go	78.21%	22 Missing ⚠️
...-operator/controllers/mcpremoteproxy_controller.go	85.71%	12 Missing ⚠️
cmd/thv-operator/app/app.go	0.00%	10 Missing ⚠️
...perator/api/v1beta1/mcpexternalauthconfig_types.go	61.90%	8 Missing ⚠️
cmd/thv-operator/pkg/controllerutil/ca_bundle.go	86.79%	7 Missing ⚠️
cmd/thv-operator/controllers/upstream_ca_bundle.go	77.77%	6 Missing ⚠️
cmd/thv-operator/pkg/controllerutil/authserver.go	91.89%	6 Missing ⚠️
...rs/mcpremoteproxy_authserver_cabundle_configmap.go	78.94%	4 Missing ⚠️
...rollers/mcpserver_authserver_cabundle_configmap.go	78.94%	4 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6428      +/-   ##
==========================================
+ Coverage   77.82%   77.88%   +0.05%     
==========================================
  Files         762      766       +4     
  Lines       73449    73959     +510     
==========================================
+ Hits        57165    57600     +435     
- Misses      16279    16354      +75     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 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.

[samuv]

Thanks for the thorough vertical implementation—the per-upstream mounts, additive system/private trust, DCR propagation, and checksum-driven rollouts are thoughtfully designed. The TLS propagation itself looks correct and retains hostname verification. However, several reconciliation and status-lifecycle paths need correction, and the Swagger documentation check currently fails.

Blockers

1. [BLOCKER] cmd/thv-operator/pkg/controllerutil/authserver.go:346 — CA validation occurs too late for authServerRef. handleAuthServerRef marks the reference valid before bundle validation; malformed bundles then fail during Deployment construction and repeatedly requeue while AuthServerRefValidated=True. Validate bundles in both handleAuthServerRef paths, record the correct terminal condition, and add MCPServer/MCPRemoteProxy regression tests. Rule: operator “Terminal vs transient errors.”

2. [BLOCKER] cmd/thv-operator/controllers/mcpserver_controller.go:2328 and mcpremoteproxy_controller.go:950 — repairing an invalid ConfigMap does not clear the failure condition. MCPServer never restores ExternalAuthConfigValidated=True; MCPRemoteProxy deliberately preserves InvalidConfig while the external-auth spec hash is unchanged, and ConfigMap content changes do not alter that hash. Add invalid→valid recovery handling and tests. Rule: operator “Status is reconstructable from observed state.”

3. [BLOCKER] cmd/thv-operator/controllers/virtualmcpserver_controller.go:486 — all CA-resolution errors are converted into terminal validation failures. Ordinary transient ConfigMap Get failures are swallowed through return false, so reconciliation can stop without backoff until another event happens. Only InvalidCABundleError should be terminal; return other errors to controller-runtime. Rule: operator “No log-and-swallow.”

4. [BLOCKER] pkg/authserver/config.go:551 — the new JSON/YAML-visible fields require regenerated Swagger artifacts, but docs/server/swagger.json, swagger.yaml, and docs.go are stale. The required “Verify Swagger Documentation” check currently fails. Run task docs and commit the generated changes.

Should-fix

1. [SHOULD-FIX] cmd/thv-operator/pkg/controllerutil/ca_bundle.go:47 — ValidateCABundleSource applies the legacy 48-character ConfigMap-name limit needed by oidc-ca-bundle-<name>. These new volumes are index-named, so otherwise valid longer ConfigMap names are rejected terminally. Use shape-only validation for embedded-auth bundles and test a longer legal name.

2. [SHOULD-FIX] examples/operator/external-auth/mcpexternalauthconfig_private_ca.yaml:34 — the example omits configMapRef.key and says it defaults to ca.crt, but the generated CRD requires key. Add key: ca.crt; otherwise the example is rejected by admission.

3. [SHOULD-FIX] PR commits — commits 0703597 and 0e43d3f lack the Signed-off-by trailers required by CONTRIBUTING.md.

Verification notes

• CRD compatibility, code generation, linting, unit tests, security scans, and Kubernetes lifecycle E2E checks pass.
• Private CA trust is consistently propagated through OIDC discovery/JWKS/token/userinfo and OAuth2 DCR.
• No existing review comments were present.

[samuv]

Follow-up on the earlier review: Swagger regeneration landed and the docs check is green. The three reconciliation blockers are still open on this head, and two of the should-fixes were not applied.

Still blocking

1. authServerRef still validates too late. ValidateEmbeddedAuthServerCABundles was added to handleExternalAuthConfig only. handleAuthServerRef still marks AuthServerRefValidated=True before any bundle check; malformed bundles fail during Deployment construction and requeue. handleInvalidUpstreamCABundle always writes ExternalAuthConfigValidated, including for authServerRef failures.

2. Repairing an invalid ConfigMap still does not restore a valid condition. MCPServer never sets ExternalAuthConfigValidated=True after a successful CA check. MCPRemoteProxy still preserves InvalidConfig while the external-auth spec hash is unchanged, and ConfigMap content does not change that hash. No invalid→valid recovery tests were added.

3. VirtualMCPServer still treats every CA-resolution error as terminal. validateAuthServerConfigCABundles stamps AuthServerConfigInvalid for all errors, and runAuthValidations returns false (no requeue). Only InvalidCABundleError should be terminal; ConfigMap Get failures must return to controller-runtime. The helper tests already classify this correctly; the controller does not use that distinction.

Still should-fix

1. 48-character ConfigMap name limit — ResolveCABundle still calls ValidateCABundleSource, which enforces the oidc-ca-bundle- volume-name cap. These volumes are index-named (authserver-upstream-ca-{index}).
2. Example still omits required key — the CRD requires configMapRef.key; examples/operator/external-auth/mcpexternalauthconfig_private_ca.yaml still has only a comment. Admission will reject it.
3. Signed-off-by — the original commits were rewritten with the trailer, but the three follow-up commits (b7a0f616, b3152f88, bdadc0b) still lack it.

Addressed

• Stale Swagger artifacts / failing “Verify Swagger Documentation” check.

Checklist

• Tests: CA helper classification and terminal-invalid coverage exist; authServerRef validation and invalid→valid recovery tests are still missing.
• Docs: Swagger and CRD reference regenerated. Example manifest is still invalid under the generated schema.
• Registry: no impact.
• Security: TLS trust path unchanged from the first review (hostname verification retained, additive system+bundle trust).
• Backwards compatibility: additive caBundleRef; no v1beta1 break.

[jhrozek]

Thanks for the second pass. That review landed against bdadc0b a few minutes before I pushed the fixes, so it's reviewing the older head — all three blockers and both code should-fixes are on 8e95962 now. Mapping them:

Blocker 1, late validation on authServerRef: 3e79675. Both handleAuthServerRef implementations now resolve the bundles where handleExternalAuthConfig already did. For the condition routing I went with a conditionType parameter on handleInvalidUpstreamCABundle rather than a Source field on InvalidCABundleError — the error is constructed down in ResolveCABundle, which has no idea which ref led there, whereas the call site always knows. MCPRemoteProxy needed a sentinel to signal the handled case up through validateAndHandleConfigs, since the unwrap at its Reconcile boundary can't tell the two ref paths apart.

Blocker 2, recovery: 30a7798 and 315a153. Root cause was CA failures sharing ConditionReasonInvalidConfig with the spec-derived failures, so both hash guards held them in place forever. They now use ConditionReasonInvalidCABundle, which is your "skip the preserve helper" option — the guard never sees a CA failure at all. I skipped the bundle-checksum-in-the-heal-key alternative because it needs a new status field on two CRDs and means carrying observed state forward as truth. Separately MCPServer had no ExternalAuthConfigValidated=True writer at all, so no amount of unblocking would have healed it; added one mirroring the MCPRemoteProxy twin. One wrinkle worth knowing about: splitting the reason meant mirrorInvalidOn* no longer recognised the CA condition as locally owned and started removing it a step before the CA check re-added it, restamping LastTransitionTime every reconcile. ownedByLocalValidation covers both reasons now and there's a test pinning it.

Blocker 3, vMCP: 45c2579. validateAuthServerConfigCABundles returns (terminal, error) and only stamps for InvalidCABundleError; runAuthValidations picked up the (bool, error) contract runValidations already documented. I also classified the CA checksum read on the deployment path — gated by the validation above today, but it returned terminal errors raw.

Should-fix 1: 06b5157, ValidateCABundleSourceShape without the cap, ResolveCABundle points at it, OIDC callers keep the length check.
Should-fix 2: 8e95962.

One thing your review didn't flag that I hit while writing the tests: the existing handleExternalAuthConfig CA check stamps the terminal reason for any error including a transient ConfigMap read, same defect as blocker 3. My first cut of the authServerRef block copied it and my own transient test caught it, so 3e79675 fixes all four sites rather than leaving two right and two wrong.

Tests: 8 authServerRef CA tests across the two controllers, 3 repair-then-reconcile tests, 2 vMCP classification tests, plus long-name coverage in the validation and ca_bundle packages. I checked each one fails without its production change — the first flap test I wrote passed either way because metav1.Time is second-resolution and a remove-then-re-add inside one second is invisible, so it seeds a timestamp an hour back instead.

The steady-state tests assert the condition is unchanged rather than the object's ResourceVersion. Status content is byte-identical across reconciles but the ResourceVersion still advances, because other writers in Reconcile issue unconditional no-op status patches. That's pre-existing and felt out of scope here.

No CRD schema change in any of this, condition reasons are plain string constants, so there's no missing generated output to look for.

On the sign-off trailer: the three follow-up commits are doc regeneration and still don't have it. Say the word if that blocks and I'll rewrite them.

[samuv]

Re-reviewed after the rebase. The earlier reconciliation blockers are addressed: authServerRef validates CA bundles before marking Valid, terminal CA failures use InvalidCABundle on the correct condition, ConfigMap repair restores Valid, and VirtualMCPServer requeues transient Get errors. The 48-character name cap no longer applies to embedded-auth bundles, and the example includes key: ca.crt.

Checklist

• Tests: terminal vs transient, authServerRef, and invalid→valid recovery coverage is in place.
• Docs: Swagger/CRD docs regenerated; example is admission-valid.
• Registry: no impact.
• Security: additive system-root-plus-bundle trust; hostname verification retained.
• Backwards compatibility: additive caBundleRef; no v1beta1 break.

[samuv]

🚀