Trust private CAs for RFC 8693 trusted issuers - #6437
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6437 +/- ##
==========================================
- Coverage 78.00% 77.94% -0.07%
==========================================
Files 766 767 +1
Lines 74069 74147 +78
==========================================
+ Hits 57780 57796 +16
- Misses 16284 16346 +62
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Panel review — 3-axis: Spec / Standards / Domain (security + operator + reuse + duplication)Fixed point: Spec (vs #6429)
Standards (
|
The new CAFilePath fields on the authserver upstream run-configs are part of the generated API surface, so the committed spec no longer matched what swag produces and the docgen check failed. Regenerating also drops the package qualification from 21 schema keys (authserver, tokenexchange, ratelimit/types, audit, operator v1beta1). swag qualifies a key only when it sees the same package name twice, and the added fields shift which packages it double-counts. No API change -- the renames and their $ref updates account for nearly all of the diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The dedupe-enums workaround only recognized an exactly doubled enum array, but swag's repeat count varies by machine -- this branch's docs were generated on one that tripled them, so the arrays survived untouched and the docgen check still failed against CI's deduped output. Match any whole-number repeat instead of only 2x, and collapse the three affected arrays in the generated spec. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
buildTrustedIssuerConfigs omitted ActorMatcher while its controllerutil twin copied it, so the CRD-time validator always compiled an empty matcher. A malformed CEL expression therefore passed admission and only surfaced when the auth server failed to start, as a CrashLoopBackOff rather than a status condition.
The negative CA bundle test asserted the error contained "x509", but jwx's httprc layer does not propagate that cause: the registration fails as "resource registered but not ready" once the fetch times out. Assert on the stage that failed instead, which together with the success case still pins the bundle as the load-bearing difference.
The rebase resolved the generated OpenAPI conflict by taking the base, so the trusted issuer ca_file_path had to be regenerated back in.
Both generateCABundleVolumes calls exceeded the 130-character line limit.
jhrozek
left a comment
There was a problem hiding this comment.
Review: private CA trust for RFC 8693 trusted issuers
Reviewed across four axes (Kubernetes operator, security, Go quality + tests, docs). 0 HIGH, 4 MEDIUM, 6 LOW.
The core is sound, and I verified the high-risk properties rather than assuming them:
- Trust is genuinely per-issuer -
x509.SystemCertPool()returns a copy on Go 1.26,Build()makes a freshTransport+tls.Configper call,newExternalIssuerConfigruns once per issuer. No shared pool, no default-transport mutation. The doc comment's "scoped to that issuer's client only" is accurate as written. - Fails closed - invalid bundle at startup errors out of
NewMultiIssuerTokenValidator; invalid after admission gives a terminal condition and the operator refuses the Deployment update, so the running pod keeps its old valid bundle. No silent fallback to system roots. - SSRF defenses untouched - the bundle only sets
TLSClientConfig.RootCAs;protectedDialerControl,ValidatingTransport,DisableKeepAlives,SameHostRedirectPolicyall preserved and unreordered. - No spurious rollout on upgrade - the
embeddedAuthServerCABundleValueto inlinedResolveCABundlerefactor is byte-for-byte behaviour-preserving. The droppedif value == nil { continue }was reachable only forref == nil, now filtered before the call, since a non-nil ref with a nilConfigMapReferrors inValidateCABundleSourceShape(validation/oidc_validation.go:47). - Index drift is impossible -
buildTrustedIssuerRunConfigs,generateTrustedIssuerCABundleVolumes, andtrustedIssuerCABundleFilePathall index the same unfiltered slice;generateCABundleVolumes'continueskips emission without shiftingindex. Volume names cap at 23 chars, distinct prefix and mount base from upstream. - Watches fully wired - the shared field index (consumed by both MCPServer and MCPRemoteProxy) and the vmcp inline check were both extended. No half-wire.
ActorMatcherfix is complete - a field-by-field audit of both builders against the 12-field CRD struct found no other dropped field.- The renames are docs-safe - zero hits for
invalid upstream CA bundleoutside Go source.
Two things I'd want addressed before merge
Both are inline below: the condition message no longer identifies which reference failed (M1), and the new example authorizes nothing (M2). M1 is mechanical (~10 lines: 2 signatures, 4 call sites, 1 test seed). M2 needs your intent on four values, so I've diagnosed rather than suggested.
Not anchorable inline (files/lines not in the diff)
MEDIUM - docs/arch/17-token-exchange-delegation.md is untouched, and one bullet is now wrong. That doc owns trustedIssuers - both the Go field comment (mcpexternalauthconfig_types.go:805) and docs/arch/README.md:138 route there. Its "Operational notes" documents every other per-issuer transport knob (discovery redirects, JWKS caching, insecureAllowHTTP, allowPrivateIPs) and says nothing about caBundleRef - an operator whose private-CA issuer fails TLS reads exactly that list and finds no answer. Worse, line 800 says:
- Misconfiguration surfaces as a pod crash, not an operator condition - check pod logs, not
kubectl describe.
A malformed caBundleRef now does surface as ConditionReasonInvalidCABundle. Suggest one bullet in Operational notes, narrowing that pod-crash bullet, and adding caBundleRef to the trustedIssuers YAML example at ~line 154.
LOW - two stale doc lines left behind by their own hunks. docs/arch/03-transport-architecture.md:788 still says "(for embedded auth-server upstreams)" - line 307 was correctly widened, 788 wasn't. docs/arch/09-operator-architecture.md:227 still says "the same upstream configuration" - line 225 was updated, 227 wasn't. Both just need "upstream" dropped.
LOW - example header comment. Line 1 still reads # Embedded auth server using a private CA for its OIDC upstream. It now also demonstrates a trusted issuer.
INFO - stale test seed. mcpserver_externalauth_test.go:755 seeds Message: "invalid upstream CA bundle: already recorded", a message the operator can no longer emit. SetStatusCondition doesn't reset LastTransitionTime on a message-only change so the test still passes, but the seed now undercuts its own "already recorded" premise.
INFO - release note. The ActorMatcher propagation fix is a user-visible admission behaviour change: a malformed CEL actorMatcher that previously passed admission and only failed at auth-server startup is now rejected up front. Worth calling out.
INFO - the 10s subtest is a defensible trade-off given jwx doesn't propagate the x509 cause and the fetch context is detached, but making httpTimeout injectable would let it run in milliseconds.
Filed as COMMENT rather than REQUEST_CHANGES - M1 and M2 are the two I'd treat as blocking, your call on the rest.
2142ea1 to
28e0125
Compare
handleInvalidCABundle formatted the unwrapped *InvalidCABundleError instead of the full error returned by ValidateEmbeddedAuthServerCABundles, so the "trustedIssuers[%d] (%q) caBundleRef:" / "upstreamProviders[%d] (%q) caBundleRef:" prefix never reached the status condition message, leaving operators unable to tell which reference failed when two refs point at the same ConfigMap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The example's trusted issuer authorized nothing: no allowedActors or actorMatcher, and it reused the upstream OIDC clientId as a dangling allowedDelegateClients entry with no matching delegateClients declaration. Add allowedActors, a properly declared reporting-delegate client, and a placeholder secret for it. Also cover caBundleRef in the docs that describe trusted issuers (arch/09, arch/17, and the CRD field comment) and drop stale "upstream"-only wording left over from adding the issuer-level bundle. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the pass. Traced the two central claims against the actual code and they don't hold up on the current branch:
The stated fixed point ( |
|
Went through this one line by line and verified both flagged MEDIUMs by tracing the actual code — both confirmed, both fixed:
Also picked up the LOWs that were pure documentation/mechanical fixes: the stale "upstream"-only wording in arch/03 and arch/09, the example header comment, the RBAC-privilege sentence on Left three open as judgment calls rather than folding them into this pass: the domain-separator hash change (you flagged the rollout-cost tradeoff yourself), collapsing the duplicated CRD→runtime converters, and the two test-coverage gaps. Replied on each inline. Pushed as |
Generate CRDs CI caught a stale diff after the CABundleRef comment was expanded in an earlier commit.
Summary
TrustedIssuerConfig(RFC 8693trustedIssuers) had no way to trust aprivate CA when fetching an external issuer's OIDC discovery document and
JWKS. Against an in-cluster IdP served over HTTPS with a non-public CA
(e.g. Keycloak behind a cert-manager certificate), the fetch fails with
x509: certificate signed by unknown authority, and the only workaroundwas a process-wide
SSL_CERT_FILEoverride — exactly whatcaBundleRefon
upstreamProviders(VirtualMCPServer.authServerConfig.upstreamProviders has no way to trust a private CA for the upstream token/authorization endpoints #6417/Trust private CAs for upstream auth servers #6428) was meant to make unnecessary. Thiscloses the same gap on the other leg: token-exchange validation against
an external issuer, not login against an upstream IdP.
caBundleReftoTrustedIssuerConfig, mirroring theCABundleSource/configMapRefshape already used byupstreamProviders.Trust is additive and scoped to that issuer's client only.
→ CA bundle ConfigMap indexing/watch → checksum-driven pod rollout →
projected volume/mount on the auth-server pod → runtime
CAFilePath→MultiIssuerTokenValidator's per-issuer HTTP client(
WithSystemRootsPlusCABundle).buildTrustedIssuerConfigs(admission-time validation) dropped
ActorMatcher, so a syntacticallyinvalid CEL expression in
trustedIssuers[].actorMatcherpassed admissionand only surfaced as a CrashLoopBackOff when the auth server started,
instead of a status condition.
Fixes #6429
Type of change
Test plan
task test)task test-e2e)task lint-fix)API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.caBundleRefis a new optional field onTrustedIssuerConfig— additive, noexisting field changes.
Does this introduce a user-facing change?
Yes:
trustedIssuers[]entries can now setcaBundleRefto trust a privateCA when fetching that issuer's OIDC discovery document and JWKS, matching the
existing
upstreamProviders[].oidcConfig.caBundleRef/oauth2Config.caBundleRefbehavior.Special notes for reviewers
ActorMatcherpropagation fix (cccc7a64f) is kept as its own commit soit can be reviewed/reverted independently of the CA bundle feature.
upstream-provider and trusted-issuer CA bytes separately (with a marker),
so
upstream=[X], issuers=[]can't hash identically toupstream=[], issuers=[X].Generated with Claude Code