Document SPIFFE association declarations by jhrozek · Pull Request #6501 · stacklok/toolhive

jhrozek · 2026-09-03T08:47:11Z

Summary

A configuration-only feature (SPIFFE trust domains + static workload-to-client associations, no live SVID verification yet) could easily be mistaken for a completed authentication feature if it isn't clearly scoped in the architecture docs.

Add docs/arch/18-spiffe-association-declarations.md, documenting the trust/static-client flow, validation boundaries, startup collision handling, restart behavior, and an explicit list of what this feature does NOT do (bundle fetching, live SVID verification, token issuance through SPIFFE).
Cross-reference it from 09-operator-architecture.md, 11-auth-server-storage.md, and the arch docs index.
Add missing item-length CRD validation markers (MinLength/MaxLength) on SPIFFEClientConfig.Audiences/.Scopes, matching the markers already present on sibling fields on the same type — caught while writing the doc's schema description and cross-checking it against the actual CRD markers.

Refs #6200, #6205

Type of change

Documentation

Test plan

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

task operator-manifests/task crdref-gen re-run to confirm the CRD marker addition produces the expected schema/docs diff and nothing else drifts.

API Compatibility

This PR does not break the v1beta1 API — the marker addition only narrows validation on a field with no existing users (this feature isn't released yet).

Does this introduce a user-facing change?

Documentation only, plus slightly stricter (but backward-compatible) validation on two SPIFFE CRD fields.

Special notes for reviewers

This adapts a doc originally written on an earlier, abandoned branch to the current schema — every factual claim (field paths, bundleSource/resources behavior, collision-handling semantics) was independently re-verified against current source rather than carried over from the old draft; several were corrected in the process (stale field path, an outdated "fails closed on any collision" claim that's now more nuanced after #6474's hardening work, and the resources field not yet flowing into the runtime OAuth client). Stacked on #6500.

codecov · 2026-09-03T08:55:53Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.45%. Comparing base (66edc12) to head (65adece).

Additional details and impacted files

@@                       Coverage Diff                       @@
##           spiffe-integration-split3-6    #6501      +/-   ##
===============================================================
+ Coverage                        78.37%   78.45%   +0.07%     
===============================================================
  Files                              776      776              
  Lines                            76225    76225              
===============================================================
+ Hits                             59744    59803      +59     
+ Misses                           16476    16417      -59     
  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

The new architecture documentation accurately explains the intended model, but currently presents it as operational when it is not reachable.

The example and related text say that SPIFFE declarations create static OAuth clients, install the storage overlay, and reserve client IDs at startup (docs/arch/18-spiffe-association-declarations.md:3,34-78; docs/arch/11-auth-server-storage.md:435-445; docs/arch/09-operator-architecture.md:241). However, the current runtime hard-rejects every non-empty spiffe_trust_domains configuration before that setup can run (pkg/authserver/config.go:317-345). Applying the documented CRD example therefore prevents the embedded auth server from starting; the described static-client, collision, restart, and reservation behavior cannot yet occur.

Please describe these as declarations that are currently rejected pending live SVID verification, or defer the operational-runtime claims until that gate is lifted.

JAORMX

Changes requested:

docs/arch/18-spiffe-association-declarations.md:3,66-78, docs/arch/11-auth-server-storage.md:437-445, and docs/arch/09-operator-architecture.md:241: the documentation describes static-client registration as operational, but RunConfig.Validate rejects every non-empty spiffe_trust_domains configuration before auth-server initialization. State that declarations are currently rejected pending SVID verification, or defer the operational claims.

JAORMX

Changes requested:

docs/arch/18-spiffe-association-declarations.md:3,23,34,62,66-78,82-94, docs/arch/11-auth-server-storage.md:437-445, docs/arch/09-operator-architecture.md:241, and docs/operator/crd-api.md:4251-4260,4272-4275 describe static-client registration, durable reservations, decorators, restart behavior, and policy enforcement as operational. RunConfig.Validate rejects every non-empty SPIFFE trust-domain configuration before auth-server initialization (pkg/authserver/config.go:317-345), so those effects cannot occur. Describe the fields as declarations that are currently rejected pending live SVID verification, and defer the operational claims until that gate is lifted.

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

JAORMX

Re-review at c31b0a15e88195c3f3c95bfec7c1e6235aafea83:

RunConfig.Validate() still rejects every non-empty spiffe_trust_domains configuration before auth-server initialization (pkg/authserver/config.go:317-344). The new document and cross-references nevertheless describe static-client registration, storage overlays, durable reservations, restart behavior, and policy enforcement as operational (docs/arch/18-spiffe-association-declarations.md:3,23,34-78,82-94; docs/arch/09-operator-architecture.md:241; docs/arch/11-auth-server-storage.md:437-445; docs/arch/README.md:144-147). Applying the example prevents startup, so none of those effects occur. State prominently that declarations are currently rejected pending live SVID verification, and recast/defer operational claims.

JAORMX

Deep re-review at e6806000ac3f7bdf1a61bb6f1f2d56ab4989351f confirms the primary documentation blocker and found a second inaccurate explanation:

P1: every non-empty SPIFFE declaration is rejected during RunConfig validation before ConfigMap/runner creation (pkg/authserver/config.go:317-344; cmd/thv-operator/pkg/controllerutil/authserver.go:1022-1026). The example and claims of registry/overlay/reservation/restart/policy behavior in docs/arch/18-spiffe-association-declarations.md:3,23,34-60,62,66-78,82-94, docs/arch/11-auth-server-storage.md:437-445, docs/arch/09-operator-architecture.md:241, and docs/arch/README.md:144-147 are therefore non-operational. State that structural validation exists but any non-empty declaration results in terminal failure pending SVID verification; do not present the YAML as supported.
P2: docs/arch/11-auth-server-storage.md:441 and docs/arch/18-spiffe-association-declarations.md:66 claim CIMD cannot collide because it only caches and does not call RegisterClient. CIMD cache population is a durable UpsertDCRIssuedClient write (pkg/authserver/storage/cimd_decorator.go:295-302), protected by a corresponding SPIFFE decorator guard (spiffe_decorator.go:197-210). The actual non-collision constraint is SPIFFE client-ID validation rejecting CIMD metadata-document URLs (pkg/authserver/spiffe_trust.go:599-607). Correct the explanation.

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>

A configuration-only foundation could otherwise be mistaken for completed SVID authentication and bundle management. Document the trust and static-client flow, validation boundaries, restart behavior, and the explicit absence of live credential verification. Also add missing item-length CRD validation markers on SPIFFEClientConfig's Audiences/Scopes fields, matching the sibling fields on the same type. Refs #6200 Refs #6205 Signed-off-by: Jakub Hrozek <jakub@stacklok.com>

jhrozek requested review from ChrisJBurns, JAORMX, amirejaz, blkt, jerm-dro, rdimitrov, reyortiz3 and tgrunnagle as code owners September 3, 2026 08:47

github-actions Bot added the size/S Small PR: 100-299 lines changed label Sep 3, 2026

jhrozek force-pushed the spiffe-integration-split3-6 branch from b758757 to f57d37e Compare September 3, 2026 10:06

jhrozek force-pushed the spiffe-integration-split3-7 branch from 5525559 to a0921d9 Compare September 3, 2026 10:08