Expose canonical inbound grants in CRDs - #6499
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6499 +/- ##
==========================================
+ Coverage 78.43% 78.54% +0.11%
==========================================
Files 776 776
Lines 76094 76415 +321
==========================================
+ Hits 59685 60023 +338
+ Misses 16404 16387 -17
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JAORMX
left a comment
There was a problem hiding this comment.
The canonical model is wired through the CRDs, conversion, and generated artifacts, but two correctness gaps block approval:
-
VirtualMCPServerinline auth validation still recognizes only legacydelegateClients/trustedIssuers[*].jwtBearerGrantas satisfying token-only operation (cmd/thv-operator/controllers/virtualmcpserver_controller.go:670-685). A valid inline configuration using onlyspec.authServerConfig.inboundGrantsis admitted but markedAuthServerConfigValidated=Falseand never deploys. Please make this path recognize configured canonical grant families, consistent with the external-auth path. -
MCPExternalAuthConfigvalidation's confidential-client transport check only considers legacyDelegateClients(cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:961-967, called at2129-2133). A canonical delegate client for an invalid non-loopback HTTP issuer can therefore reportValid=True, while the normalized runtime configuration later rejects it. Please apply the same normalized/canonical-aware transport validation at the owning configuration resource and add coverage for the canonical path.
The remaining red Go Vulnerability Check is also present on the stacked bases and appears unrelated to this diff.
fbe4ff6 to
67f5da4
Compare
5834d9c to
6dbb23e
Compare
67f5da4 to
b4c8fed
Compare
6dbb23e to
e038ceb
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Re-review at e038ceb5d38e0961cb3b332c58359863bbf392f6: the two prior blockers are still unresolved.
VirtualMCPServeraccepts canonical grants in CRD admission but its inline runtime guard still ignorescfg.InboundGrants, rejecting valid token-only configurations (cmd/thv-operator/controllers/virtualmcpserver_controller.go:670-685).ValidateConfidentialClientTransportstill ignores canonicalinboundGrants.tokenExchange.delegateClients, allowing cleartext non-loopback HTTP issuers with a confidential client (cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:961-967).
Please address both with regression tests. CI is green; no local tests were run.
JAORMX
left a comment
There was a problem hiding this comment.
Changes requested:
cmd/thv-operator/controllers/virtualmcpserver_controller.go:670: inline validation ignores canonicalInboundGrants, so valid token-only canonical configurations are rejected and never reconcile. Include canonical grant families.cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:961: confidential-client transport validation considers only legacy delegate clients; include canonical token-exchange delegate clients.cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:2151: canonical issuer policies bypass equivalent trusted-issuer validation and can be publishedValid=Truedespite invalid policy values. Validate normalized canonical policies at this boundary.
JAORMX
left a comment
There was a problem hiding this comment.
One precision addendum to the latest review: canonical inboundGrants also skips MCPExternalAuthConfig's early trusted-issuer validation. At cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:2151-2155, tokenexchange.ValidateTrustedIssuers(...) only runs when cfg.InboundGrants == nil. Consequently an invalid issuer policy (for example, self-issuer collision or invalid issuer/JWKS URL) can leave the owning config reported Valid=True.
This is not a complete runtime bypass: BuildAuthServerRunConfig later calls RunConfig.Validate() and catches the invalid configuration before a consuming workload runs (cmd/thv-operator/pkg/controllerutil/authserver.go:952-954,1034-1052). Please apply the equivalent normalized trusted-issuer validation on the owning configuration path and test the Valid=False result for a canonical invalid issuer policy.
e038ceb to
ae26bc4
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Changes requested:
cmd/thv-operator/controllers/virtualmcpserver_controller.go:670-685: inline validation still ignores canonicalInboundGrants, so valid token-only canonical configurations are rejected and never deployed.cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:961-967: confidential-client transport validation examines only legacy delegate clients. Apply it to normalized canonical token-exchange delegate clients too.cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:2151-2155: canonical issuer policies bypass the owningMCPExternalAuthConfigtrusted-issuer validation and can be reportedValid=Truedespite invalid issuer/JWKS URLs or self-issuer collisions. Normalize and validate them at this boundary.
All current CI checks are green; no local tests were run.
ae26bc4 to
ce2447f
Compare
The SPIFFE client-auth epic needs a place to configure SPIFFE association policy without inventing a parallel trust/grant path next to the existing delegate-client and trusted-issuer configuration. As more inbound grant families (RFC 8693 token exchange, RFC 7523 JWT-bearer, SPIFFE) accumulate, they need one canonical surface to configure and reason about instead of three independent ones, without breaking deployments that already rely on the legacy fields. Add pkg/authserver/inbound_grants.go with NormalizeInboundGrants, which reconciles a new canonical RunConfig.InboundGrants surface (per-family token_exchange/jwt_bearer sub-configs whose issuer_policies reference a trusted_issuers entry by name) against the legacy top-level delegate_clients and the RFC 8693/7523 fields embedded directly on trusted_issuers. Legacy and canonical configuration for the same grant family are mutually exclusive and rejected at validation time; the two families are otherwise independent, and omitting inbound_grants entirely preserves released behavior. Thread the normalized result through RunConfig.Validate, the embedded-auth-server runner, and buildProvider/discovery, adding a DisableTokenExchange capability so RFC 8693 registration and discovery advertisement can be turned off together and can't drift out of sync. Add TrustedIssuer.Name so canonical issuer_policies can reference an issuer without duplicating its fields. SPIFFE client authentication (InboundGrants.SPIFFEClientAuth, defined in the previous commit) is deliberately kept a sibling of TokenExchange and JWTBearer here, not nested under either: SPIFFE authenticates a client, it does not by itself grant it anything, so making it subordinate to RFC 8693 enablement would mean disabling token exchange silently drops every SPIFFE association, and every SPIFFE-authenticated client would be implicitly token-exchange-capable. It is validated and wired directly from RunConfig.InboundGrants in RunConfig.Validate/embeddedauthserver.go, independent of this file's legacy/canonical projection, so authentication method and grant-family enablement stay separately configurable. Update docs/arch/17-token-exchange-delegation.md for the new inbound_grants shape and the now-conditional token-exchange discovery advertisement, and add a runner-level test proving the canonical delegate-client, SPIFFE-client, and jwt_bearer paths reach a running server (the existing tests only covered normalization in isolation). SPIFFE client-auth associations always require the token-exchange grant (the only grant type they may declare), independent of the legacy/canonical token-exchange projection above: NormalizeInboundGrants now sets Capabilities.TokenExchange true whenever InboundGrants.SPIFFEClientAuth is non-empty, so a SPIFFE-only configuration cannot leave it false and silently disable the RFC 8693 grant handler server-wide -- which would reject every SPIFFE client's own token requests before authentication is even checked. Guarded by a regression test in this package (not just the runner-level test above) since the equivalent fix was previously lost during a rebase when its only coverage lived one package away. DCR (RFC 7591 /oauth/register) now rejects a registration whose effective grant types include token-exchange when it is disabled server-wide, instead of accepting the client and only failing later, confusingly, at /oauth/token. The check runs on the post-defaulting grant types validateGrantTypes already computes (a private_key_jwt client with an empty grant_types is implicitly token-exchange-only), so it catches both the explicit and implicit cases the same way scope validation already gates DCR on ScopesSupported. Corrected two stale doc references caught in review: the SPIFFE client-policy field path (inbound_grants.spiffe_client_auth, not nested under token_exchange) and the JWT-bearer legacy/canonical conflict wording (family-wide across all issuers, not per-issuer). Refs #6200 Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
The operator needs the shared inbound-grant model before it can add SPIFFE associations without inventing a separate Kubernetes API. Add v1beta1 grant-family types, CEL constraints, runtime conversion, generated schemas, and compatibility coverage for existing grant fields. Refs #6200
Operators need a visible migration signal before legacy grant fields can be removed safely. Record deprecated field paths during normalization and surface them as status conditions without changing the effective authorization policy. Refs #6200
The SPIFFE client-auth epic needs a place to configure SPIFFE association policy without inventing a parallel trust/grant path next to the existing delegate-client and trusted-issuer configuration. As more inbound grant families (RFC 8693 token exchange, RFC 7523 JWT-bearer, SPIFFE) accumulate, they need one canonical surface to configure and reason about instead of three independent ones, without breaking deployments that already rely on the legacy fields. Add pkg/authserver/inbound_grants.go with NormalizeInboundGrants, which reconciles a new canonical RunConfig.InboundGrants surface (per-family token_exchange/jwt_bearer sub-configs whose issuer_policies reference a trusted_issuers entry by name) against the legacy top-level delegate_clients and the RFC 8693/7523 fields embedded directly on trusted_issuers. Legacy and canonical configuration for the same grant family are mutually exclusive and rejected at validation time; the two families are otherwise independent, and omitting inbound_grants entirely preserves released behavior. Thread the normalized result through RunConfig.Validate, the embedded-auth-server runner, and buildProvider/discovery, adding a DisableTokenExchange capability so RFC 8693 registration and discovery advertisement can be turned off together and can't drift out of sync. Add TrustedIssuer.Name so canonical issuer_policies can reference an issuer without duplicating its fields. SPIFFE client authentication (InboundGrants.SPIFFEClientAuth, defined in the previous commit) is deliberately kept a sibling of TokenExchange and JWTBearer here, not nested under either: SPIFFE authenticates a client, it does not by itself grant it anything, so making it subordinate to RFC 8693 enablement would mean disabling token exchange silently drops every SPIFFE association, and every SPIFFE-authenticated client would be implicitly token-exchange-capable. It is validated and wired directly from RunConfig.InboundGrants in RunConfig.Validate/embeddedauthserver.go, independent of this file's legacy/canonical projection, so authentication method and grant-family enablement stay separately configurable. Update docs/arch/17-token-exchange-delegation.md for the new inbound_grants shape and the now-conditional token-exchange discovery advertisement, and add a runner-level test proving the canonical delegate-client, SPIFFE-client, and jwt_bearer paths reach a running server (the existing tests only covered normalization in isolation). SPIFFE client-auth associations always require the token-exchange grant (the only grant type they may declare), independent of the legacy/canonical token-exchange projection above: NormalizeInboundGrants now sets Capabilities.TokenExchange true whenever InboundGrants.SPIFFEClientAuth is non-empty, so a SPIFFE-only configuration cannot leave it false and silently disable the RFC 8693 grant handler server-wide -- which would reject every SPIFFE client's own token requests before authentication is even checked. Guarded by a regression test in this package (not just the runner-level test above) since the equivalent fix was previously lost during a rebase when its only coverage lived one package away. DCR (RFC 7591 /oauth/register) now rejects a registration whose effective grant types include token-exchange when it is disabled server-wide, instead of accepting the client and only failing later, confusingly, at /oauth/token. The check runs on the post-defaulting grant types validateGrantTypes already computes (a private_key_jwt client with an empty grant_types is implicitly token-exchange-only), so it catches both the explicit and implicit cases the same way scope validation already gates DCR on ScopesSupported. Corrected two stale doc references caught in review: the SPIFFE client-policy field path (inbound_grants.spiffe_client_auth, not nested under token_exchange) and the JWT-bearer legacy/canonical conflict wording (family-wide across all issuers, not per-issuer). Refs #6200 Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
d634568 to
6123649
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Re-review at 6123649 (current #6499 head): prior status ownership and non-embedded deprecation-condition blockers are resolved. The following merge blockers remain.\n\n- cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:688-696: the final CEL block removed the existing HTTP delegate-client transport rule and the canonical/legacy same-family exclusion rules. The Go validator fails closed later, but Kubernetes now admits configurations the PR promises to reject; an accepted invalid update can leave the prior, potentially broader runtime policy active. Restore the transport rule for both legacy and canonical token-exchange delegates and the two mutual-exclusion CEL rules, then regenerate CRDs. Exact-head Operator Tests Integration confirms all five cases fail.\n- cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:1083-1101: canonical issuer policies are normalized, but validation calls tokenexchange.ValidateTrustedIssuers directly and omits the runtime validator's issuer/JWKS URL, HTTPS, and private-address safeguards (pkg/authserver/config.go:1332-1365). A bad canonical issuer can therefore be reported Valid=True on MCPExternalAuthConfig until a consumer fails later. Apply the complete trusted-issuer validation at the owning configuration boundary.\n- docs/operator/crd-api.md is not generated from this head: exact-head Generate CRD Docs fails and shows a non-empty diff. Run task crdref-gen and commit the output.\n\nExact-head CI is not green: Generate CRD Docs and Operator Tests Integration fail for the concrete issues above. The unrelated E2E lifecycle timeout should also be rerun/triaged before merge.
6123649 to
8481a2d
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Re-review at 8481a2d1393d08a1345658f6ebd359fa7cf82b4d (reviewed only 6123649..8481a2d, excluding stacked descendants): the generated CRD docs are now synchronized, but two correctness blockers remain.
-
Blocking:
cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:690-692still makes the CELinsecureAllowHTTPexclusion depend only onallowConfidentialClientRegistration. A legacy or canonical delegate client withinsecureAllowHTTP: trueand an HTTPS issuer is admitted, while the owning controller rejects it as a confidential-client configuration. Make the CEL constraint cover effective delegate clients in both shapes, not just DCR, and add admission coverage for both legacy and canonical inputs. -
Blocking: canonical trusted-issuer validation at the owning
MCPExternalAuthConfigboundary remains incomplete (cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:1101-1103,2298-2303). Folding intotokenexchange.ValidateTrustedIssuersvalidates policy structure but not issuer/JWKS URL schemes, userinfo, fragments, or private-address safeguards; the full validation only occurs later while a workload buildsRunConfig(pkg/authserver/config.go:1332-1365). Invalid shared configuration can therefore be publishedValid=Trueuntil a consumer fails. Reuse the complete validator at this boundary and addValid=Falseregressions for malformed issuer/JWKS URLs and private/loopback JWKS URLs.
These are independent correctness/security-boundary findings, not CI-based. The previous generated-CRD-doc blocker is resolved.
8481a2d to
74c96a8
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Re-review at 74c96a8f79da9788f55f01f3d9b87eab524ec7d7 (only 8481a2d..74c96a8, excluding stacked descendants): the confidential delegate-client CEL rule is now correctly applied to both legacy and canonical shapes. The owning trusted-issuer validation now covers scheme, fragments, userinfo, and literal private/loopback JWKS addresses.\n\nOne blocker remains:\n\n- Blocking: ValidateTrustedIssuerURL / ValidateJWKSURL test only parsed.Host != "" (pkg/authserver/server/tokenexchange/multi_issuer_validator.go:1194-1216,1233-1266). An authority such as https://:443 has Host == ":443" but an empty Hostname(), so either a legacy or canonical issuer/JWKS endpoint with that invalid authority reaches MCPExternalAuthConfig as Valid=True through ValidateInboundGrants (cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:1110-1125,2320-2325). It fails only later when used. Require a non-empty Hostname() and add legacy/canonical Valid=False regressions for issuer and JWKS URLs with empty-host authorities.\n\nThis is an independently established validity-boundary defect, not a CI finding. The exact-head Lint Go Code check is currently failing; that is author-owned status and is not a reason for this review decision.
74c96a8 to
d7abacf
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Re-review at d7abacf858c78cdb40410606648fdb682692ccb4 (only 74c96a8..d7abacf, excluding stacked descendants): the canonical/legacy trusted-issuer validation now flows through the owning MCPExternalAuthConfig boundary, and the prior scheme/userinfo/fragment/literal-private-IP validation work remains in place. One concrete blocker remains unresolved.
- Blocking:
ValidateTrustedIssuerURLandValidateJWKSURLtest onlyparsed.Host != ""(pkg/authserver/server/tokenexchange/multi_issuer_validator.go:1194-1216,1233-1266). The authorityhttps://:443hasHost == ":443"but an emptyHostname(), so both legacy and canonical configurations can passValidateInboundGrantsand be published asValid=TruebyMCPExternalAuthConfigbefore failing at consumption. Require a non-emptyHostname()(and align the equivalent config/server issuer validators), then add regression coverage for issuer and JWKS endpoints in the legacy and canonical config paths, including the owningValid=Falsestatus outcome.
This is an independently established configuration-validity/security-boundary defect, not a CI finding. Exact-head CI is still running; completed checks are green, including lint.
Canonical inbound grants bypassed trusted-issuer and cross-surface validation, while the runtime and operator could replace the same status conditions array. Fold both grant surfaces before validation, scope deprecation reporting to embedded auth, and give runtime observations a dedicated status snapshot that the operator projects into compatibility fields. Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
d7abacf to
1c698d7
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Re-review at 1c698d7da002fd9652a38446b0b0bfea2b50570e, scoped to d7abacf..1c698d7 so stacked/inherited changes were not reattributed.
The empty-host URL validation blocker is resolved. The relevant authorization-server, trusted-issuer, and JWKS validation boundaries now require URL.Hostname(); https://:443 is rejected before MCPExternalAuthConfig can report Valid=True. Legacy and canonical projections are folded through the same validation path, with regression coverage for issuer and JWKS cases.
Non-blocking follow-up: similar pre-existing operator URL validators outside this PRs scope still test URL.Host rather than Hostname() (cmd/thv-operator/pkg/validation/oidc_validation.go, cmd/thv-operator/pkg/validation/url_validation.go). Please address that separately rather than expanding this stacked CRD PR.
Exact-head CI is green; CI status is not part of this approval decision.
Summary
The operator's CRDs (
VirtualMCPServer,MCPExternalAuthConfig) only expose the legacy, per-field inbound-grant configuration (delegateClients,trustedIssuers[*]policy fields). The canonicalinboundGrantsgrant-family model already exists at the runtimeRunConfiglayer (this stack's earlier PRs), but the operator has no way to declare it — a prerequisite for adding SPIFFE associations without inventing a second, parallel Kubernetes API surface for the same underlying grants.inboundGrantsCRD types (TokenExchange/JWTBearergrant families, delegate clients, issuer policies) to the sharedEmbeddedAuthServerConfig, with CEL admission validation for legacy/canonical mutual exclusion.authserver.RunConfigvia the same converter pattern the legacy fields already use.ConditionTypeVirtualMCPServerDeprecatedInboundGrantConfiguration) during normalization, without changing the effective authorization policy — this gives operators a visible migration signal before legacy fields are removed.Refs #6200
Type of change
Test plan
task test)task lint-fix)New table-driven unit tests for the converter and CEL admission rules (
authserver_inbound_grants_test.go,inbound_grants_cel_test.go), plus a reconcile-level test asserting the deprecation condition transitions correctly and emits a one-shot warning event.API Compatibility
v1beta1API —inboundGrantsis a new optional field; existing legacy fields are unchanged and continue to work standalone.Changes
Large diff (~3,470 lines), dominated by generated CRD YAML (
deploy/charts/operator-crds/**, regenerated viatask operator-manifests/task operator-generate) anddocs/operator/crd-api.md(regenerated viatask crdref-gen) — roughly 2,600 of the changed lines are generated, not hand-written. Hand-written surface:mcpexternalauthconfig_types.go(new CRD types + CEL),controllerutil/authserver.go(converter wiring),virtualmcpserverstatus/collector.go(deprecation condition), and their tests.Does this introduce a user-facing change?
Yes — operators can now declare
inboundGrantsonVirtualMCPServer/MCPExternalAuthConfigdirectly, and get a status condition warning if they're still using the legacy per-field configuration it's meant to replace.Special notes for reviewers
This PR is a cherry-picked/rebuilt version of work originally done on an earlier, abandoned branch before this stack's review cycle reworked the underlying runtime model — content and tests were re-verified against the current
RunConfig/InboundGrantsRunConfigshape rather than merged as-is. Stacked on #6474.