Define SPIFFE trust configuration - #6467
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6467 +/- ##
========================================
Coverage 78.09% 78.09%
========================================
Files 767 768 +1
Lines 74306 74722 +416
========================================
+ Hits 58026 58351 +325
- Misses 16275 16366 +91
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d640d3f to
44adbfd
Compare
github-actions
Bot
added
size/L
github-actions
Bot
added
size/L
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
I reviewed this against the stacked base and #6200. The validation itself is careful, especially around SPIFFE parsing and pattern overlap, but I found three model problems that I think we should resolve before building the authentication paths on top of it: the serialized config is currently accepted and ignored, resource and audience permissions are conflated, and the runtime model discards its validated trust-domain records. I left those inline, plus the malformed CIMD client ID case and two smaller standards issues.
A few review-wide notes that do not have a useful inline location:
- The issue requires a normalized authenticated principal carrying the exact SPIFFE ID, canonical trust domain, OAuth client ID, and selected method. This PR does not define that result yet. That can land with runtime integration, but we should keep the acceptance criterion open until both credential paths produce it.
- The PR adds 513 non-test, non-generated code lines, above the repository's 400-line guideline.
- Commit 44adbfd is missing the Signed-off-by trailer required by CONTRIBUTING.md.
- The exported parsing and validation helpers have no production callers yet. Keeping them private until runtime integration would avoid committing their semantics as public API too early.
CI is green. I also ran task lint successfully. My local task test run exceeded 15 minutes without reporting a failure; the GitHub Go test job passed.
| // See DelegateClientRunConfig for the per-client field reference. | ||
| DelegateClients []DelegateClientRunConfig `json:"delegate_clients,omitempty" yaml:"delegate_clients,omitempty"` | ||
|
|
||
| // SPIFFETrustDomains declares SPIFFE trust roots. Association policies are |
There was a problem hiding this comment.
This publishes spiffe_trust_domains in the serialized API and generated Swagger, but RunConfig.Validate does not validate it and the RunConfig-to-Config conversion does not consume it. So an invalid or valid-looking SPIFFE declaration starts successfully and has no effect. Even for a stacked change, I don't think we should expose a silent no-op configuration field. Could we either remove this field/schema until the association and conversion slice lands, or wire validation and conversion in this PR?
There was a problem hiding this comment.
Good catch, and thanks for checking the stack context rather than just this diff.
I traced the not-yet-PRed follow-on branches: this is genuinely wired two commits later (a "Normalize canonical inbound grants" commit adds InboundGrantsRunConfig with legacy/canonical normalization for delegate clients, trusted issuers, and SPIFFE clients together, calls ValidateSPIFFETrust from RunConfig.Validate(), and builds Config.SPIFFETrust in embeddedauthserver.go). It's a bigger mechanism than a standalone spiffe_client_auth field would be, which is why it didn't land in this commit.
I'll leave spiffe_trust_domains as-is here rather than pull that normalization work forward or strip the field, but I take the point that this commit alone doesn't make that obvious — happy to add a note to the PR description making the sequencing explicit if that helps review the rest of the stack.
|
|
||
| Methods []SPIFFEAuthenticationMethod `json:"methods" yaml:"methods"` | ||
|
|
||
| // Audiences are allowed token-exchange target identifiers. Both RFC 8693 |
There was a problem hiding this comment.
RFC 8693 audience and RFC 8707 resource are separate request dimensions, but this one list authorizes both. Since these entries are also checked against the server's URI-only allowed_audiences, the model cannot represent a non-URI logical audience, nor can an operator permit a resource without permitting the same value as an audience. #6200 asks us to configure resources and audiences per association. I think these need separate Resources and Audiences fields before this becomes public API.
There was a problem hiding this comment.
Agreed this is a real gap — checked whether any of the follow-on branches split it into separate Resources/Audiences fields, and none of them do yet (still a single Audiences list all the way through the stack as currently drafted). So this isn't settled anywhere downstream either.
Given it changes the on-disk schema (and the CRD surface once that lands), I'd rather resolve the RFC 8693 vs RFC 8707 shape deliberately in its own commit than bolt it on here under review pressure. Leaving this open for now — let me know if you'd rather block this PR on it.
|
|
||
| // SPIFFETrustConfig is the immutable normalized SPIFFE trust model used at | ||
| // runtime. It contains validated association policy; trust-domain declarations | ||
| // are consumed only for validation and are not retained here. |
There was a problem hiding this comment.
Discarding the validated trust-domain records leaves the future X.509 and JWT validators with only a string reference. They will need to retain and reinterpret the raw RunConfig separately to recover the canonical trust domain, enabled domain methods, and eventually the bundle source. That gives us two sources of truth instead of the one authoritative model #6200 calls for. Could SPIFFETrustConfig retain immutable normalized trust-domain records and provide lookup by declaration name?
There was a problem hiding this comment.
Fixed — SPIFFETrustConfig now retains the validated, canonicalized trust-domain records (canonical trust domain string + enabled methods) and exposes them via TrustDomain(name string) (SPIFFETrustDomain, bool). NewSPIFFETrustConfig builds this map from the same validation pass instead of discarding it. Added TestSPIFFETrustConfigTrustDomainLookup for coverage.
| if err := storage.ValidateRegisterableClientID(entry.ClientID); err != nil { | ||
| return "", fmt.Errorf("inbound_grants.spiffe_client_auth[%d]: client_id: %w", index, err) | ||
| } | ||
| if u, err := url.ParseRequestURI(entry.ClientID); err == nil && u.Scheme != "" && u.Host != "" { |
There was a problem hiding this comment.
This check and runtime CIMD routing disagree for malformed HTTPS values. For example, https://example.org/%zz fails ParseRequestURI, passes this validation, but oauthproto.IsClientIDMetadataDocumentURL reserves every https:// prefix and routes lookup through CIMD. The association then validates but cannot resolve as its intended registered client. Please use the same CIMD predicate that controls runtime routing and add a malformed-URL regression case.
There was a problem hiding this comment.
Good catch. Switched to oauthproto.IsClientIDMetadataDocumentURL (the same predicate the runtime CIMD router uses) instead of hand-parsing with url.ParseRequestURI. Added the https://example.org/%zz malformed-URL case as a regression test.
|
|
||
| // NewSPIFFETrustConfig validates and normalizes SPIFFE trust declarations into | ||
| // an immutable runtime model. It does not load trust bundles or authenticate | ||
| // credentials. Callers must use this constructor; a zero-value model is invalid. |
There was a problem hiding this comment.
The constructor-only invariant is not enforceable while this is an exported concrete type. External callers can construct SPIFFETrustConfig{}, and NewSPIFFETrustConfig(nil, nil, nil, nil) itself returns an equivalent non-nil empty model. Nothing rejects either value. Could we either make the zero value intentionally valid and document it, or add state/validation that actually distinguishes a constructed model?
There was a problem hiding this comment.
You're right that the zero value was constructible and I hadn't actually distinguished it. Went with documenting the zero value as intentionally valid rather than adding back state to distinguish it: SPIFFETrustConfig{} now behaves identically to what NewSPIFFETrustConfig returns for empty input (no trust domains, no associations, fails closed by construction), so there's no invariant left to violate. Updated the doc comments on both the type and the constructor to say so explicitly.
| scopesSupported []string, | ||
| allowedAudiences []string, | ||
| ) error { | ||
| effectiveScopes := scopesSupported |
There was a problem hiding this comment.
The Go style rule asks us to avoid mutable assignment across branches. Please make this an immutable IIFE assignment, returning registration.DefaultScopes when scopesSupported is empty and scopesSupported otherwise.
There was a problem hiding this comment.
Fixed — converted to the immutable IIFE form.
| } | ||
|
|
||
| // validateSPIFFEClientAssociationPermissions validates the audiences, scopes, | ||
| // and grant types an association is permitted to request. |
There was a problem hiding this comment.
This function does not receive or validate grant types. The comment should say audiences and scopes only, otherwise it promises an invariant the implementation does not enforce.
There was a problem hiding this comment.
Fixed the comment to say audiences and scopes only.
44adbfd to
e0320a0
Compare
github-actions
Bot
added
size/L
|
Replied inline on the three model issues and the two smaller ones — thanks for the thorough pass. Summary of what changed (pushed as an amended commit):
On the review-wide notes:
CI is green on the amended commit. |
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
I re-reviewed the amended head and the rest of the stack against #6200. The fixes for retaining normalized trust domains, aligning CIMD client-ID detection, documenting the zero value, immutable assignment, comments, helper visibility, and the DCO trailer all look good. CI is green.
I still think the public model needs changes before we build the credential paths on it:
-
SPIFFEClientAuthRunConfig.Audiencesauthorizes both RFC 8693audienceand RFC 8707resource(pkg/authserver/spiffe_trust.go:66). Those are independent request dimensions with different syntax and policy semantics. The issue explicitly requires resources and audiences per association; the current model cannot permit one without permitting the other and cannot represent a non-URI logical audience because everything is bounded by URI-onlyallowed_audiences. -
The association has no configurable grants or independent token-exchange permission.
SPIFFEGrantTypeTokenExchangemakes every association token-exchange-capable by construction. #6200 requires grants and token-exchange permission to be narrowed per association; client authentication should not implicitly confer a grant. -
The trust-domain declaration omits the bundle source required by #6200. Bundle loading can be deferred, but the source belongs in the authoritative serialized model before that model becomes public.
-
RunConfig.SPIFFETrustDomainsis still accepted and exposed in generated Swagger without being validated or consumed in this PR. #6473 wires it later, but this PR is not independently fail-closed: a non-empty configuration starts successfully and has no effect.
Please settle these schema commitments in the foundation PR. They become substantially harder to correct once released or consumed by the follow-on stack.
e0320a0 to
8abeeaa
Compare
github-actions
Bot
added
size/XL
|
Pushed a commit that resolves all four points — thanks for pushing on this, and for confirming the earlier round of fixes.
CI is green on the amended commit. |
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Thanks for the amendment. I re-reviewed e0320a0..8abeeaa. The new head fixes several earlier blockers: spiffe_client_auth is now independent of token exchange, grant permission is explicit, a bundle-source discriminator exists, validation runs from RunConfig.Validate, and the normalized model is built at the runner boundary.
One authorization-model blocker remains:
pkg/authserver/spiffe_trust.go:578-587applies the global boundary backwards.RunConfig.AllowedAudiencesis documented and validated as the server's RFC 8707 resource-URI allowlist. The newResourcesvalues receive only URI syntax validation, so an association can declare a resource the server does not globally allow. Meanwhile RFC 8693Audiencesare required to be members of that URI-only resource list, which still prevents valid logical/non-URI audience identifiers. Please validateResourcesas a subset of the existing global resource list and give RFC 8693 audiences an independent policy boundary. Add regression cases proving permission in either dimension does not imply permission in the other.
Two schema/lifecycle issues also need resolution before this becomes a public configuration surface:
- A SPIFFE federation bundle endpoint needs an endpoint profile (
https_weborhttps_spiffe) in addition to its URL. Without it, the future loader cannot know whether to authenticate through Web PKI or SPIFFE, so the just-added schema already requires another compatibility change. - The normalized
Config.SPIFFETrustis still not consumed or retained by the server. Valid non-empty SPIFFE configuration now validates and starts, but provides no authentication or policy enforcement. That is fail-closed for access, but remains an operationally inert security setting. Either reject non-empty configuration until an enforcement path exists or land the first consumer with this exposed surface.
The PR description and test-plan text still describe the pre-amendment model (no RunConfig.Validate wiring, combined audiences, and no bundle source); please update them so reviewers and generated release context match the new behavior.
The amended commit has the required DCO trailer. CI is still running; completed checks are green so far.
8abeeaa to
108bc9c
Compare
github-actions
Bot
added
size/XL
|
I checked the latest amendment (
Leaving the change request in place. CI is still running; completed checks are green so far. |
github-actions
Bot
added
size/XL
108bc9c to
e497cae
Compare
github-actions
Bot
added
size/XL
|
Pushed a commit that resolves the two schema findings, and updated the PR description.
CI is green on the amended commit. |
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
I re-reviewed 108bc9c..e497cae. The resource/audience boundary is now correct, the bundle endpoint profile is present and validated, regression coverage was added, and the PR description is current. Thanks for fixing those.
The remaining blocker is the enforcement lifecycle. The new comment confirms that no open or drafted part of this epic verifies either SVID type against the configured bundle. Building Config.SPIFFETrust is conversion, not enforcement: a valid non-empty security configuration still starts and has no runtime effect. I do want this hard-rejected until the first authentication consumer lands. That preserves an honest, fail-loud contract while still allowing all types and validation to merge; the follow-up that introduces verification can remove the temporary rejection. Documentation alone is not enough for a setting that appears to configure client authentication.
CI has one failed network-isolation E2E unrelated to this authserver diff (--allow-docker-gateway); it needs a clean rerun before approval. The DCO trailer is present.
Both SPIFFE credential methods (X.509-SVID and JWT-SVID) need one fail-closed identity and association model before authentication can produce equivalent authorization outcomes for either method. Without a shared model, adding live SVID verification later would force a choice between duplicating trust-domain/scope/audience checks per credential type or bolting authorization onto whichever method lands first. This commit defines that model as pure config validation, deliberately without loading trust bundles or authenticating credentials — those are separate, later steps on this stacked branch (refs #6200). It is, however, independently fail-closed: `RunConfig.Validate()` now validates `spiffe_trust_domains`/`inbound_grants.spiffe_client_auth` directly, and `Config.SPIFFETrust` is built in the embedded auth server constructor, so a malformed or half-configured declaration cannot start successfully and silently have no effect. `SPIFFETrustDomainRunConfig` declares a named trust domain, the credential methods it enables, and exactly one bundle source: a SPIFFE Bundle Endpoint (an HTTPS URL plus an `https_web`/`https_spiffe` authentication profile, so a future loader knows whether to trust the endpoint's TLS connection via Web PKI or a separately distributed X.509-SVID root) or the local Workload API. Both are validated for shape now so the field exists in the authoritative model before consumption is built, even though fetching a bundle is out of scope here. `SPIFFEClientAuthRunConfig` associates a SPIFFE principal pattern (a concrete ID or a terminal `/*` wildcard) within a declared trust domain with an explicit OAuth client_id, methods, and permissions — client_id is never derived from the SPIFFE ID, so an operator always states which OAuth identity a workload maps to. Permissions are three independent dimensions instead of one combined list: `resources` (RFC 8707, optional, must be an absolute HTTP(S) URI and a member of the server's `allowed_audiences` allowlist — the same list `DelegateClientRunConfig.Audiences` is validated against), `audiences` (RFC 8693, required, but deliberately *not* bounded by that allowlist since a token audience is a distinct request dimension from a resource and may be a non-URI logical identifier), and `grant_types` (required, must be exactly token-exchange for now) — so permitting a resource never implies permitting the same value as an audience, or vice versa, and client authentication never by itself confers a grant. `NewSPIFFETrustConfig` validates and normalizes these declarations into an immutable `SPIFFETrustConfig`. It retains the validated, canonicalized trust-domain records (not just the association policy), exposed via a lookup-by-name method, so a future X.509/JWT-SVID validator has one authoritative source for a trust domain's canonical form, enabled methods, and bundle source instead of re-parsing the raw RunConfig separately. Its zero value is also valid (equivalent to what the constructor returns for empty input), so external packages may construct it directly without going through the constructor. Validation fails closed on anything that could make authorization ambiguous or order-dependent: duplicate trust-domain names or canonical trust domains, a principal whose trust domain doesn't match its declared trust-domain reference, overlapping principal patterns across associations (segment-aware, so `/agent/*` doesn't collide with `/agent-x/*`), client IDs colliding with the reserved synthetic-client namespace or matching a client metadata document URL (reserved for CIMD-resolved clients, checked with the same predicate the runtime CIMD router uses), a bundle source whose declared type doesn't match its payload, and a bundle endpoint whose authentication profile is missing or unrecognized. A valid, non-empty configuration still has no runtime effect in this commit: nothing yet registers a client or verifies a credential against it. That first consumer (static client registration) lands in a later commit on this stack; live SVID/bundle verification is a further step beyond it. Since no part of this epic as currently drafted verifies an SVID against the configured bundle, `RunConfig.Validate()` hard-rejects a non-empty `spiffe_trust_domains` outright rather than accepting it silently: an operator must not be able to believe SPIFFE client authentication is active when no credential is ever checked. The follow-up that adds real verification removes this rejection. Refs #6200 Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
e497cae to
3413cc2
Compare
github-actions
Bot
added
size/XL
|
Pushed the hard-reject: This has real ripple: it makes it impossible to construct a full server with non-empty SPIFFE config via the normal path, which affects #6473 (fixed, unaffected) and #6474 (several integration-style tests that proved SPIFFE behavior end-to-end could no longer do so). I worked through that across both PRs — see #6474 for details, since that's where most of the affected tests live. CI is green on this commit (network-isolation passed clean this run). |
2 participants
Summary
Both SPIFFE credential methods (X.509-SVID and JWT-SVID) need one fail-closed identity and association model before authentication can produce equivalent authorization outcomes for either method. Without a shared model, adding live SVID verification later would force a choice between duplicating trust-domain/scope/audience checks per credential type or bolting authorization onto whichever method lands first.
This PR defines that model as validated, normalized config, wired into
RunConfig.Validate()so a malformed or ambiguous declaration fails server startup rather than degrading silently later. It deliberately does not perform any live SVID/bundle verification, and does not register any client with the auth server's storage — those are separate, later steps on this stacked branch (refs #6200) so each piece can be reviewed independently. Stacked on #6465.SPIFFETrustDomainRunConfig, declaring a named trust domain, the credential methods (spiffe_x509/spiffe_jwt) it enables, and a requiredBundleSource(bundle_endpointwith anhttps_web/https_spiffeauthentication profile, orworkload_api) — validated for shape only; fetching or loading a bundle is a later step.SPIFFEClientAuthRunConfig, associating a SPIFFE principal pattern (a concrete ID or a terminal/*wildcard) within a declared trust domain with an explicit OAuthclient_id, methods, scopes, resources, audiences, and grant types.client_idis never derived from the SPIFFE ID, and client authentication never implies a grant by itself —grant_typesmust be declared explicitly.Resources(RFC 8707) andAudiences(RFC 8693) are independent request dimensions: onlyResourcesis bounded by the server'sallowed_audiencesallowlist (the same listDelegateClientRunConfig.Audiencesvalidates against);Audiencesis not, and may hold non-URI logical identifiers. Permission in one dimension never implies permission in the other.NewSPIFFETrustConfig, which validates and normalizes these declarations into an immutableSPIFFETrustConfig(unconstructible from outside the package except through the constructor), andValidateSPIFFETrust, called directly fromRunConfig.Validate()./agent/*doesn't collide with/agent-x/*), client IDs colliding with the reserved synthetic-client namespace or shaped as absolute URLs (reserved for CIMD-resolved clients), and resources/scopes outside the server's global allowlists.Fixes #
Type of change
Test plan
task test)task test-e2e)task lint-fix)pkg/authserver/spiffe_trust_test.gocovers principal normalization/pattern matching (wildcard boundaries, unicode/port/userinfo/query/dot-segment rejection), trust-domain validation (duplicate names, duplicate canonical trust domains, bundle-source shape and endpoint profile), and association validation (unknown/wrong trust domain, duplicate/overlapping principals, reserved client-ID prefixes, absolute-URL client IDs, resource/scope allowlisting audience independence, grant-type restriction, method enablement). No client is ever registered with the auth server's storage in this PR, so no e2e coverage is needed here.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Special notes for reviewers
This is a "define the types" commit in a stacked SPIFFE client-auth epic.
RunConfig.SPIFFETrustDomainsandRunConfig.InboundGrants.SPIFFEClientAuthare validated byRunConfig.Validate()in this PR, but nothing in the auth server yet consumes the normalizedSPIFFETrustConfig— no client is registered, and no credential (X.509-SVID/JWT-SVID) is ever verified. A non-empty, valid configuration therefore currently has no runtime effect beyond passing validation. Static client registration lands in a follow-on commit on this stack (#6474); live SVID/bundle verification is a further step beyond this stack. Please review this PR purely as the declarative validation/model layer.