Skip to content

Expose SPIFFE client-auth registration through the operator CRDs - #6500

Merged
jhrozek merged 1 commit into
mainfrom
spiffe-integration-split3-6
Sep 7, 2026
Merged

Expose SPIFFE client-auth registration through the operator CRDs#6500
jhrozek merged 1 commit into
mainfrom
spiffe-integration-split3-6

Conversation

@jhrozek

@jhrozek jhrozek commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

The SPIFFE client-authentication runtime model (trust domains + static workload-to-client associations) already exists in pkg/authserver, but was only reachable by hand-authoring an auth-server RunConfig file directly — there was no way to declare it through VirtualMCPServer/MCPExternalAuthConfig.

An earlier, never-reviewed attempt at this CRD exposure existed on an abandoned branch, but assumed a stale runtime shape (SPIFFE clients nested under inboundGrants.tokenExchange) that no longer matches the reviewed model (inboundGrants.spiffeClientAuth is now a sibling of tokenExchange/jwtBearer, with a larger field surface: a bundleSource discriminated union, resources, and a fixed grantTypes). This PR redoes the CRD exposure from scratch against the current model, reusing the old attempt's validation contract (its CEL test matrix) as an acceptance checklist, not its code.

  • Add spiffeTrustDomains and inboundGrants.spiffeClientAuth to the shared EmbeddedAuthServerConfig, with CEL admission validation for everything derivable from the object's own spec (paired configuration, no duplicate names/trust-domains/client-IDs/principal-patterns, full cross-referencing between domains and clients, method-subset enforcement, client-ID hygiene).
  • Wire the new fields through to authserver.RunConfig via the same converter pattern used for delegateClients/trustedIssuers, and extend reconcile-time revalidation to cover SPIFFE trust domains.
  • Deliberately do NOT expose grantTypes as a CRD field — the runtime only accepts exactly one value (urn:ietf:params:oauth:grant-type:token-exchange), so the converter sets it rather than exposing a field with one legal value.
  • Cross-field checks needing reconcile-time-derived values (allowedAudiences, scopesSupported — neither is CRD-exposed) are deliberately left to the existing reconcile-time revalidation path rather than a premature admission-time Go check — an earlier draft of this PR added such a check passing nil for both, which the runtime validator treats as "validate against nothing" rather than "skip," silently rejecting any resources entry or non-default scopes value. Caught and removed during adversarial review; see commit history for detail.

Refs #6199, #6200, #6205

Known, intentional consequence: a syntactically valid SPIFFE config is admitted by CEL but still fails reconciliation with a terminal error until RunConfig's "not yet enforced" placeholder gate (pkg/authserver/config.go) is lifted by a future PR — no live SVID verification exists yet, so this is expected, not a regression.

Type of change

  • New feature

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

31-case CEL admission test matrix (spiffe_cel_test.go) ported from the old attempt's test list and extended for the bundleSource discriminated union and resources; unit tests for the two new converter functions; a reconcile-level test proving a SPIFFE client with resources/custom scopes is accepted at the admission-equivalent Go layer but still correctly validated (accepted or rejected as appropriate) once real derived values reach BuildAuthServerRunConfig.

API Compatibility

  • This PR does not break the v1beta1 API — both new fields are optional and additive.

Changes

Large diff (~3,384 lines), dominated by generated CRD YAML/docs (deploy/charts/operator-crds/**, docs/operator/crd-api.md — roughly 2,200 of the changed lines) and the new CEL test file. Hand-written surface: mcpexternalauthconfig_types.go (new CRD types + 11 CEL rules), controllerutil/authserver.go (converter wiring), and their tests.

Does this introduce a user-facing change?

Yes — operators can now declare spiffeTrustDomains/inboundGrants.spiffeClientAuth on VirtualMCPServer/MCPExternalAuthConfig. Reconciliation of a non-empty config will fail with a clear terminal error until live SVID verification lands (see "Known, intentional consequence" above).

Implementation plan

Approved implementation plan

Expose SPIFFE client-auth registration through the operator CRDs

Context

The SPIFFE client-auth epic (#6199, refs #6200/#6205) already landed a full
runtime model in pkg/authserver (SPIFFETrustDomainRunConfig,
SPIFFEClientAuthRunConfig, ValidateSPIFFETrust, the static-client
registry/storage overlay, etc.), but that model is only reachable by hand-
authoring an auth-server RunConfig file — there is no way to declare it
through VirtualMCPServer/MCPExternalAuthConfig CRDs.

An old, never-reviewed branch (spiffe-integration-split3-5/6/7, abandoned
mid-epic when the stack was reworked through review) had already built this
CRD-exposure feature once, but its commit (0c921b295, "Expose SPIFFE
registration in CRDs") assumed a runtime shape that no longer matches: it
nested SPIFFE clients under InboundGrants.TokenExchange.SPIFFEClients and
never modeled BundleSource/Resources/GrantTypes. The current, reviewed
runtime model puts SPIFFEClientAuth as a top-level sibling of
TokenExchange on InboundGrantsRunConfig (deliberately independent of the
token-exchange capability toggle — see the doc comment on
InboundGrantsRunConfig in pkg/authserver/spiffe_trust.go), and has a
materially larger SPIFFEClientAuthRunConfig/SPIFFETrustDomainRunConfig
surface than the old commit ever covered. Rather than merge that stale
commit, we're redoing the CRD-exposure work from scratch against the current
model, reusing the old commit's validation contract (its CEL test matrix)
as an acceptance checklist, not its code.

Known, intentional consequence: RunConfig.Validate() currently
hard-rejects any non-empty SPIFFETrustDomains via
validateSPIFFENotYetEnforced — a deliberate placeholder until real SVID
verification lands (see pkg/authserver/config.go:328-345). Wiring CRD
fields into RunConfig.SPIFFETrustDomains means a syntactically valid CRD
spec will be admitted by CEL/webhook validation but will still fail
reconciliation with a terminal InvalidEmbeddedAuthServerConfigError until
that gate is lifted by a future PR. This is expected, not a bug — call it
out explicitly in the PR description.

Scope

In scope: two new CRD types (SPIFFETrustDomainConfig,
SPIFFEClientConfig) plus a bundle-source discriminated union, added to the
shared EmbeddedAuthServerConfig (used by both MCPExternalAuthConfig and
VirtualMCPServer — no VMCP-specific code needed, same as the old commit
found). CEL admission validation for everything expressible from the
object's own spec. Go-level bridging into the existing
authserver.ValidateSPIFFETrust/NewSPIFFETrustConfig for everything that
needs derived values (AllowedAudiences, ScopesSupported — neither is
CRD-exposed; both are derived at reconcile time, confirmed via
cmd/thv-operator/pkg/controllerutil/authserver.go:883-895) or the full
SPIFFE-ID/URL grammar. Reconcile-time revalidation via the existing
validateDelegateClientsAndTrustedIssuers pattern.

Out of scope: bundle fetching/loading, live SVID verification, lifting
the validateSPIFFENotYetEnforced gate (all still placeholder by design),
and the legacy-field-removal refactor that was tangled into the old commit
(0c921b295) — that already landed separately via 0501c3fea /
split3-5.

CRD types — cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go

Mirror the runtime shape exactly (field-for-field with
SPIFFETrustDomainRunConfig/SPIFFEClientAuthRunConfig in
pkg/authserver/spiffe_trust.go), using the same validation-marker idioms as
neighboring types (DelegateClientConfig at line 361, TrustedIssuerConfig
at line 404) — +kubebuilder:validation:MinLength/MaxLength,
MinItems/MaxItems, +listType=atomic/set, +optional on every pointer
field, //nolint:lll on types carrying long CEL rules.

  1. SPIFFEAuthenticationMethod enum (spiffe_x509, spiffe_jwt),
    SPIFFEBundleSourceType enum (bundle_endpoint, workload_api),
    SPIFFEBundleEndpointProfile enum (https_web, https_spiffe) —
    straight string-const mirrors of the runtime enums.

  2. SPIFFEBundleSourceConfig (discriminated union, required field on
    SPIFFETrustDomainConfig — the runtime field has no omitempty):

    • Type SPIFFEBundleSourceType
    • Endpoint *SPIFFEBundleEndpointSourceConfig (URL string,
      Profile SPIFFEBundleEndpointProfile)
    • WorkloadAPI *SPIFFEWorkloadAPIBundleSourceConfig (empty struct)
    • Two XValidation rules reusing the exact pattern already used for
      MCPExternalAuthConfig's own type-discriminated union (line 67-75):
      self.type == 'bundle_endpoint' ? has(self.endpoint) : !has(self.endpoint)
      and the workload_api/workloadAPI mirror.
  3. SPIFFETrustDomainConfig: Name, TrustDomain (pattern-validated,
    copy the old commit's regex — it's a reasonable approximation and the
    type doc comment should say runtime parsing via spiffeid.TrustDomainFromString
    remains authoritative, matching the existing "CEL is best-effort" convention
    already used elsewhere in this file), Methods (MinItems=1,MaxItems=2,
    enum), BundleSource SPIFFEBundleSourceConfig (required, no +optional).

  4. SPIFFEClientConfig (lives in a new SPIFFEClientAuth []SPIFFEClientConfig field on InboundGrantsConfig — a sibling
    of TokenExchange/JWTBearer, not nested under TokenExchange, matching
    InboundGrantsRunConfig's shape):

    • TrustDomainRef, PrincipalPattern (pattern-validated + the old
      commit's path-traversal CEL rule
      self.principalPattern.split('/').all(segment, segment != '.' && segment != '..')),
      ClientID, Methods (MinItems=1,MaxItems=2), Audiences
      (MinItems=1,MaxItems=50), Scopes (MinItems=1,MaxItems=50),
      Resources (+optional, MaxItems=50 — new vs. the old commit; RFC
      8707 resources, Go-validated against AllowedAudiences since that's
      derived, not CRD-known).
    • Do not expose GrantTypes as a CRD field. The runtime only accepts
      exactly ["urn:ietf:params:oauth:grant-type:token-exchange"]
      (validateSPIFFEGrants, spiffe_trust.go:758-766) — a field with one
      legal value is a field the converter should just set, not a knob users
      configure. Set it in the converter (GrantTypes: []string{authserver.SPIFFEGrantTypeTokenExchange}).
  5. SPIFFETrustDomains []SPIFFETrustDomainConfig stays on
    EmbeddedAuthServerConfig directly (top-level, matching
    RunConfig.SPIFFETrustDomains) — MinItems=1,MaxItems=50, +listType=atomic, +optional, same placement the old commit used.

  6. CEL rules on EmbeddedAuthServerConfig (add alongside its existing
    XValidation block): reuse the old commit's semantic set, adjusted for
    the new field path (self.inboundGrants.spiffeClientAuth, not
    self.inboundGrants.tokenExchange.spiffeClients):

    • paired configuration (spiffeTrustDomains and
      inboundGrants.spiffeClientAuth empty/non-empty together)
    • no duplicate trust-domain name
    • no duplicate trust-domain trustDomain value
    • every declared trust domain referenced by ≥1 client
    • every client's trustDomainRef resolves to a declared domain
    • client methods ⊆ referenced domain's methods
    • principalPattern's trust-domain segment matches the referenced
      domain's trustDomain
    • no duplicate client clientId
    • no duplicate client principalPattern
    • clientId must not start with synthetic: (reserved) or look like an
      absolute URL (reserved for CIMD) — reuse
      storage.ValidateRegisterableClientID's intent as the CEL approximation,
      Go-side stays authoritative

Go bridging — same file, new validateSPIFFETrustConfig helper

Alongside the existing validateEmbeddedAuthServer(): convert
cfg.SPIFFETrustDomains[]authserver.SPIFFETrustDomainRunConfig and
cfg.InboundGrants.SPIFFEClientAuth[]authserver.SPIFFEClientAuthRunConfig
(setting GrantTypes to the fixed value here), call
authserver.ValidateSPIFFETrust(trustDomains, inboundGrants, nil, nil) for a
config-shape-only check at admission time (no AllowedAudiences/
ScopesSupported available yet — same limitation already accepted for
DelegateClients/TrustedIssuers at this layer).

Controller wiring — cmd/thv-operator/pkg/controllerutil/authserver.go

  • New buildSPIFFETrustDomainRunConfigs([]mcpv1beta1.SPIFFETrustDomainConfig) []authserver.SPIFFETrustDomainRunConfig
    and buildSPIFFEClientAuthRunConfigs([]mcpv1beta1.SPIFFEClientConfig) []authserver.SPIFFEClientAuthRunConfig
    (pure field copy + slice clone, no error — same shape as
    buildTrustedIssuerRunConfigs at line 269, since nothing here needs secret
    resolution).
  • buildInboundGrantsRunConfig (line 824): set
    grants.SPIFFEClientAuth = buildSPIFFEClientAuthRunConfigs(config.SPIFFEClientAuth)
    as a sibling assignment alongside the existing TokenExchange/JWTBearer
    blocks.
  • BuildAuthServerRunConfig (line 889): set
    config.SPIFFETrustDomains = buildSPIFFETrustDomainRunConfigs(authConfig.SPIFFETrustDomains)
    next to the existing config.InboundGrants = inboundGrants assignment.
  • validateDelegateClientsAndTrustedIssuers (line 1034): extend the early-
    return guard to also check len(config.SPIFFETrustDomains) == 0, and add
    SPIFFETrustDomains: config.SPIFFETrustDomains to validationConfig so
    RunConfig.Validate() catches SPIFFE misconfiguration (including the
    "not yet enforced" gate) as a reconcile error, not a pod crash loop —
    consistent with why this function exists at all.

Tests

  • Port the old commit's CEL test matrix
    (spiffe_cel_test.go, ~30 cases: paired-config, duplicate name/trustDomain/
    clientId/principalPattern, unreferenced/unknown trust-domain-ref,
    methods-not-a-subset, principal/trust-domain-segment mismatch, reserved
    clientId prefixes, trust-domain string edge cases, principal path-segment
    edge cases) rewritten against the new field path
    (inboundGrants.spiffeClientAuth, not .tokenExchange.spiffeClients) and
    extended for BundleSource's discriminated union and the new Resources
    field.
  • One reconcile-level test asserting a valid, non-empty SPIFFE CRD config is
    admitted by CEL but produces a terminal InvalidEmbeddedAuthServerConfigError
    reconcile failure (proving the "not yet enforced" gate is reachable end-to-
    end through the CRD path, not just the RunConfig-file path already tested
    in pkg/authserver).
  • Unit tests for the two new converter functions and
    validateSPIFFETrustConfig, following this file's existing table-driven
    convention.

Verification

  1. task operator-manifests then task operator-generate — confirm CRD
    YAML and deepcopy regenerate cleanly (watch for the same missing-JSON-tag
    class of controller-gen failure hit earlier in this session).
  2. task lint-fix — 0 issues.
  3. task test — full suite green, including the new CEL/reconcile tests
    (these need envtest/a real apiserver — same suite that already runs
    cmd/thv-operator/test-integration/mcp-external-auth/*_cel_test.go).
  4. Manually apply a VirtualMCPServer with valid SPIFFE fields against a
    local kind cluster (or envtest) and confirm: CRD admission succeeds, then
    the reconciler surfaces the expected "not yet enforced" terminal
    condition — not a crash loop, not silent success.

Delegation

Hand off implementation to a fresh agent (this is CRD/operator work spanning
Go API types, CEL, and controller wiring — kubernetes-expert or a fresh
go-expert-developer), then run an adversarial Opus review pass before
committing, matching this session's established mechanism for anything
security/protocol-relevant. Land as spiffe-integration-split3-6 on top of
the already-verified spiffe-integration-split3-5.

Special notes for reviewers

This PR was implemented by a fresh agent against the approved plan above, then went through one full adversarial review round: the reviewer found two real bugs (an admission-time Go check that unconditionally rejected any resources/custom-scopes entry due to a bad nil-handling assumption in the plan itself, and missing task crdref-gen output) plus an untested design deviation — all fixed and re-verified before this PR was opened. Stacked on #6499.

@github-actions github-actions Bot added the size/XL Extra large PR: 1000+ lines changed label Sep 3, 2026
@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.91837% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.51%. Comparing base (500f478) to head (a9921a6).

Files with missing lines Patch % Lines
...perator/api/v1beta1/mcpexternalauthconfig_types.go 94.73% 2 Missing ⚠️
pkg/authserver/spiffe_trust.go 88.23% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6500      +/-   ##
==========================================
+ Coverage   78.49%   78.51%   +0.02%     
==========================================
  Files         776      776              
  Lines       76415    76494      +79     
==========================================
+ Hits        59983    60063      +80     
+ Misses      16427    16426       -1     
  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.

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The CRD conversion and generated artifacts are largely aligned, but the panel found material validation/lifecycle gaps:

  1. principalPattern rejects ~ in path segments (cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:731-738), although it is valid in SPIFFE URI path segments. This makes the served CRD schema narrower than the authoritative runtime parser. Please accept the full permitted segment grammar (including ~) and add concrete/wildcard admission coverage.

  2. Context-independent SPIFFE errors can still lead MCPExternalAuthConfig to report Valid=True: bundle endpoint URLs are not structurally validated and overlapping principal policies are only rejected later by runtime validation (cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:647-653, 890-891, 2366-2373). Validate these at admission or in the config controller; defer only checks that genuinely require workload-derived allowlists.

  3. The multi-issuer validator owns background jwk.Cache workers but neither it nor the auth server closes them; constructor error paths also leave earlier caches running (pkg/authserver/server/tokenexchange/multi_issuer_validator.go:415-445,549; pkg/authserver/server_impl.go:396-400). Add lifecycle cleanup on server close and on partial construction failure, with tests.

The red Go Vulnerability Check is inherited from the unchanged dependency in the stacked base and appears unrelated.

@jhrozek
jhrozek force-pushed the spiffe-integration-split3-5 branch from 5834d9c to 6dbb23e Compare September 3, 2026 10:04
@jhrozek
jhrozek force-pushed the spiffe-integration-split3-6 branch from b758757 to f57d37e Compare September 3, 2026 10:06
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 3, 2026
@jhrozek
jhrozek force-pushed the spiffe-integration-split3-5 branch from 6dbb23e to e038ceb Compare September 3, 2026 13:03
@jhrozek
jhrozek force-pushed the spiffe-integration-split3-6 branch from f57d37e to c5e6f6e Compare September 3, 2026 13:05
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 3, 2026

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-review at c5e6f6e6654575c784b132321caf72c543d9dd3a: none of the three prior blockers are resolved, and the same inline-configuration parity gap exists here.

  • CRD principalPattern still rejects valid SPIFFE ~ path segments (cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:731-738).
  • Malformed bundle endpoints and overlapping principal patterns still bypass validation on the owning config CRD and can be reported Valid=True (mcpexternalauthconfig_types.go:647-653,883-893; controllers/mcpexternalauthconfig_controller.go:106-176).
  • JWK cache workers still have no cleanup on server shutdown or partial validator construction (pkg/authserver/server/tokenexchange/multi_issuer_validator.go:433-470,557-578; pkg/authserver/server_impl.go:375-400).
  • VirtualMCPServer also still rejects advertised SPIFFE-only inline configuration because its guard ignores inboundGrants.spiffeClientAuth (cmd/thv-operator/controllers/virtualmcpserver_controller.go:670-685).

CI is green; no local tests were run.

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changes requested:

  • cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:737: principalPattern excludes ~, which the authoritative SPIFFE parser permits in path segments. Permit it and add admission coverage.
  • cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:649-653,2366-2373 and cmd/thv-operator/pkg/controllerutil/authserver.go:1022-1024: malformed bundle endpoints and overlapping principal patterns are published Valid=True until a consuming workload fails. Validate context-independent invariants at the owning config boundary.
  • cmd/thv-operator/controllers/virtualmcpserver_controller.go:670: include inboundGrants.spiffeClientAuth in inline auth-server validation; the shared CRD currently admits a SPIFFE-only shape that this guard rejects.

@jhrozek
jhrozek force-pushed the spiffe-integration-split3-5 branch from e038ceb to ae26bc4 Compare September 3, 2026 14:14
@jhrozek
jhrozek force-pushed the spiffe-integration-split3-6 branch from c5e6f6e to ff2fc59 Compare September 3, 2026 14:15
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 3, 2026

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changes requested:

  • cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:737: principalPattern still rejects ~, which the authoritative SPIFFE parser permits in path segments. Permit it and add concrete and wildcard admission coverage.
  • cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:649-653,890-891 and cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go:106-176: malformed bundle endpoints and overlapping principal patterns can be published Valid=True at the owning config boundary and only fail when a consuming workload validates the runtime configuration. Validate these context-independent invariants at admission or in the config controller.
  • cmd/thv-operator/controllers/virtualmcpserver_controller.go:670-685: include inboundGrants.spiffeClientAuth in inline auth-server validation; a SPIFFE-only inline shape admitted by the shared CRD is still rejected here.

All current CI checks are green; no local tests were run.

@jhrozek
jhrozek force-pushed the spiffe-integration-split3-5 branch from ae26bc4 to ce2447f Compare September 3, 2026 16:16
@jhrozek
jhrozek force-pushed the spiffe-integration-split3-6 branch from ff2fc59 to 3b10dc8 Compare September 3, 2026 16:24
@jhrozek
jhrozek force-pushed the spiffe-integration-split3-5 branch from d634568 to 6123649 Compare September 6, 2026 07:19

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Correction after rechecking the deployed go-spiffe parser and your pushback: you are right about ~. ToolHive pins go-spiffe/v2 v2.7.0 without the spiffeid_charset_backcompat build tag, so the runtime accepts [A-Za-z0-9._-] and rejects ~. The CRD regex correctly matches that runtime. I withdraw that finding.

One independently established correctness blocker remains at this head:

  • cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:756-780,2368-2374: the owning MCPExternalAuthConfig can report Valid=True for independently invalid SPIFFE permission values. Audience and scope entries may be empty; resource entries may be empty or malformed instead of absolute HTTP(S) URIs without fragments. Runtime validation already separates these shape checks from the derived scope-subset/resource-allowlist checks (pkg/authserver/spiffe_trust.go:625-649,710-761), but it only runs later when a workload consumes the config.

Please validate the context-independent shapes at admission or in MCPExternalAuthConfig.Validate(), while keeping checks that require derived ScopesSupported and AllowedAudiences at reconciliation. This is based on the owning resource publishing a false validity result, not on CI; exact-head CI status is separate.

@jhrozek
jhrozek force-pushed the spiffe-integration-split3-6 branch 2 times, most recently from 7f4d0b8 to 4d3eb12 Compare September 6, 2026 12:17
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 6, 2026

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-review at 4d3eb12a637bdf7693e1ec7bfd4bb5df34fbad4f (only 6123649..4d3eb12, excluding the stacked base):

The prior validity-boundary blocker is partially resolved: empty audience and scope entries are now rejected, but resource validation is still too weak.

  • Blocking: cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:756-770,2542-2556 accepts a non-empty resource matching ^https?://[^[:space:]]+$, including https://backend.example/#fragment and userinfo-bearing forms. The authoritative runtime validator rejects these independently invalid resource indicators in pkg/authserver/spiffe_trust.go:710-744, but it only runs when a workload consumes the config. Consequently, the owning MCPExternalAuthConfig may publish Valid=True for an invalid spec. Validate the context-independent absolute HTTP(S) URI shape at admission/config validation; retain the derived allowlist-membership check at reconciliation. Add fragment, userinfo, and missing-authority regression coverage, including the Valid=False status path.

All other previously requested items remain resolved. This request is based on the independently established correctness/status-boundary defect, not CI.

@jhrozek
jhrozek force-pushed the spiffe-integration-split3-5 branch 3 times, most recently from 74c96a8 to d7abacf Compare September 6, 2026 20:40
@jhrozek
jhrozek force-pushed the spiffe-integration-split3-6 branch from 4d3eb12 to 0cd94a9 Compare September 6, 2026 21:03
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 6, 2026

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-review at 0cd94a97f350855fd56579cc01a98366cb79da4a (only d7abacf..0cd94a9, excluding the stacked base): the previously requested resource validation is still incomplete.\n\n- Blocking: cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:756-770 accepts resource values matching ^https?://[^[:space:]]+$, including https://user@example.com, https://example.com/path#fragment, and https:///missing-authority. MCPExternalAuthConfig.validateEmbeddedAuthServer deliberately defers permission-shaped validation (:2563-2581), so the owning controller can publish Valid=True before a consumer reaches the stricter runtime parser (pkg/authserver/spiffe_trust.go:730-744). These URI-shape checks do not require derived allowlists: reject userinfo, fragments, and empty/missing authorities at the owning configuration boundary (with CEL where expressible plus Go validation as appropriate), and add Valid=False regressions for all three cases.\n\nThis is an independently established configuration-validity correctness/security-boundary defect (CWE-20; RFC 8707 §2), not a CI-based decision. The other previously requested SPIFFE validation work is verified in this scoped layer.

@jhrozek
jhrozek force-pushed the spiffe-integration-split3-5 branch from d7abacf to 1c698d7 Compare September 7, 2026 07:12
Base automatically changed from spiffe-integration-split3-5 to main September 7, 2026 08:01
@jhrozek
jhrozek force-pushed the spiffe-integration-split3-6 branch from 0cd94a9 to bcbe083 Compare September 7, 2026 08:27
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 7, 2026

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-review at bcbe08381052c669191a1cb10ab1089186d71072, scoped to this PR layer against merged base 500f478 (and treating the rebased, non-ancestor 0cd94a9 head as prior review context only):\n\n- Blocking — configuration-validity boundary: SPIFFEClientConfig.Resources uses only ^https?://[^[:space:]]+$ (cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:756-770), while MCPExternalAuthConfig.validateEmbeddedAuthServer explicitly defers resource validation (:2561-2581). Consequently the owning configuration can report Valid=True for independently invalid RFC 8707 resource indicators, including userinfo (https://user@example.com), fragments (https://example.com/#fragment), empty-host authorities (https://:443), and malformed/missing authorities. Shape validation does not require the workload-derived AllowedAudiences or ScopesSupported; keep those membership checks deferred, but reject invalid HTTP(S) URI shape at this owning boundary (including a non-empty URL.Hostname()), then add Valid=False regression coverage for these forms. The runtime parser currently checks URL.Host, not Hostname() (pkg/authserver/spiffe_trust.go:737-742), so it does not close the empty-host case either.\n\nThis is an independently established correctness/security-boundary issue (CWE-20; RFC 8707 §2), not a CI-based decision. Exact-head CI is currently 46 successful, 2 in progress, with no failures; that status is author-owned and does not affect this review decision.

@jhrozek
jhrozek force-pushed the spiffe-integration-split3-6 branch from bcbe083 to 9dd0362 Compare September 7, 2026 09:53
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 7, 2026
The runtime already supports declaring SPIFFE trust domains and
associating SPIFFE principals with OAuth clients, but only through a
hand-authored auth-server RunConfig file; there was no way to declare
it through the MCPExternalAuthConfig/VirtualMCPServer CRDs.

Add SPIFFETrustDomainConfig and SPIFFEClientConfig CRD types (mirroring
the runtime shape field-for-field), CEL admission validation for the
paired-configuration, duplicate-name, and trust-domain/principal
cross-reference invariants, and Go-level bridging into
authserver.ValidateSPIFFETrust for the checks CEL cannot express.
Audiences, scopes, and resource-indicator items now carry length and
shape constraints at admission time, matching the convention already
used for AllowedResources and TrustedIssuerConfig elsewhere in this
file, instead of deferring all of it to reconcile time.

Refs #6199

Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
@jhrozek
jhrozek force-pushed the spiffe-integration-split3-6 branch from 9dd0362 to a9921a6 Compare September 7, 2026 10:51
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 7, 2026

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-review at a9921a6891ced2f88b8b30b8d0a1778f8431f4e1, scoped to the new delta from prior reviewed head 9dd036248a5d90721eff25980b0cb32f679c3c39.

Approved. This refactor folds resource-shape validation into the existing per-entry SPIFFE validation loop to bring validateEmbeddedAuthServer below the lint complexity threshold. It preserves the prior admission-time validation and indexed error paths: each resource still passes ValidateResourceIndicators before pairwise pattern overlap checks (cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:2639-2656). Fresh code-review and Kubernetes-operator panel checks found no concrete blockers in this delta.

CI remains author-owned and separate from this approval decision.

@jhrozek
jhrozek merged commit 2714d76 into main Sep 7, 2026
58 of 59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants