Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6563 +/- ##
========================================
Coverage 78.90% 78.90%
========================================
Files 780 781 +1
Lines 77683 77823 +140
========================================
+ Hits 61298 61409 +111
- Misses 16380 16409 +29
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
79559fa to
08e84f1
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Fresh panel review of the incremental stack diff 5dee1fb7521e74127473258a63175a702a71f747...08e84f13ff636de2dc41c28544f9658150405616 against #6203.
Blocking — require no iss claim from a JWT-SVID
pkg/authserver/server/spiffe_client_auth.go:111-117 rejects an otherwise valid JWT-SVID unless it has an iss claim equal to the trust-domain string. iss is optional application-specific JWT-SVID metadata; #6203 requires a valid SPIFFE sub, valid expiry, exactly the AS issuer as audience, and validation against the associated trust-domain bundle—not an issuer claim. Once bundle wiring lands, conformant signed SVIDs without iss will be rejected. Remove this unconditional requirement, or make it an explicit documented/configured extension.
Blocking — the shared dispatcher changes unrelated authentication behavior
pkg/authserver/server/spiffe_client_auth.go:46-53 rejects duplicate client_assertion_type values before checking whether SPIFFE is configured or selected. This changes default/Fosite handling of entirely non-SPIFFE (including RFC 7523 private-key JWT) requests, contrary to #6203's explicit constraint that this arm must not modify the shared dispatcher. The SPIFFE path should reject ambiguous requests when any assertion type selects SPIFFE, while entirely non-SPIFFE requests must continue directly to defaultStrategy. Please move the shared dispatcher adjustment to #6562 along with its needed duplicate-SPIFFE detection, then rebase this PR.
The JWT bundle source is intentionally not connected to production configuration until dependency #6201; the PR documents that it remains unreachable, so that is not a finding. Standards, reuse, duplication, and DevEx panel members found no additional actionable issues; no assertion/claim logging was found.
JAORMX
left a comment
There was a problem hiding this comment.
Fresh review at 08e84f1, scoped strictly to this PR's stacked range from #6562 head 5dee1fb.\n\nApproved. The code-review, security, protocol, and architecture panel found no merge blockers. The JWT arm enforces assertion cardinality and size, verifies only through the configured JWT bundle source, requires a sole issuer audience, rejects mixed credentials, and fails closed without a bundle source or association. The documented absence of deployable bundle wiring is an explicit, safe deferral: validation cannot be reached from the production configuration path.\n\nNon-blocking: docs/arch/18-spiffe-association-declarations.md:3-5 still says no JWT-SVID is verified anywhere in the build, contradicting the implemented but non-deployable validation described at :81-116. Narrow it to no deployable construction path being wired to a trust bundle source.\n\nExact-head CI is green; that status is independent of this approval.
JAORMX
left a comment
There was a problem hiding this comment.
Superseding my approval: I rechecked the current-head JAORMX review (5154974136) against this PR's exact stacked range and its two blockers remain unresolved.\n\n### Blocking — do not require an application-specific iss claim\n\npkg/authserver/server/spiffe_client_auth.go:111-117 rejects an otherwise valid JWT-SVID unless it contains iss equal to the trust domain. #6203 requires the valid SPIFFE sub, expiry, sole AS-issuer audience, and bundle verification; iss is not among its JWT-SVID acceptance requirements and relying on undeclared claims harms interoperability. Remove the unconditional requirement, or make it a documented/configured extension.\n\n### Blocking — this JWT arm modifies unrelated default authentication behavior\n\npkg/authserver/server/spiffe_client_auth.go:46-53 rejects duplicate client_assertion_type before selecting SPIFFE. That changes fully non-SPIFFE/default Fosite requests (including RFC 7523 private-key JWT) and conflicts with #6203's scope: this arm must not modify the shared dispatcher. Put the required ambiguous-SPIFFE detection in #6562, and ensure entirely non-SPIFFE inputs continue to defaultStrategy.\n\nThe absent production bundle wiring remains an explicitly documented, safe fail-closed deferral. Exact-head CI is green and separate from these review blockers.
Validate SPIFFE JWT-SVID client assertions with go-spiffe jwtsvid.ParseAndValidate against the configured JWT bundle source, then resolve the verified SPIFFE ID through the shared SPIFFEClientResolver so X.509 and JWT credentials for the same association reach the same authorization outcome. The construction and wiring of a concrete JWT bundle source has no home in this branch: the SPIFFEBundleRegistry type it previously threaded through server_impl.go was removed by the SPIFFE trust/CRD restructuring this branch is rebased onto, so AuthorizationServerParams.SPIFFEJWTBundleSource is left unset (nil) pending that follow-up work.
Update the SPIFFE association-declarations doc and SPIFFEClient comments now that JWT-SVID client-authentication dispatch and validation logic exists, and record what remains pending: wiring a trust bundle source, X.509-SVID validation, replay protection, and grant/discovery integration.
Reject ambiguous assertion parameters and constrain JWT-SVID freshness and issuer claims. Allow client ID omission while preserving exact association ownership when a selector is supplied.
Rebasing onto main pulled in the merged base branch's slices.Contains duplicate-assertion-type detection, which now routes into authenticateSPIFFEJWTClient instead of the removed "not implemented" placeholder. That function rejects the duplicate itself with fosite.ErrInvalidRequest, so the existing test's expected hint was stale. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
08e84f1 to
4d09f2e
Compare
Drop the unconditional iss-claim requirement: RFC 7519 makes iss OPTIONAL and the SPIFFE JWT-SVID spec does not mandate it either, so a conformant SVID signed by a bundle-trusted key was being rejected for omitting application-specific metadata #6203 never required. Stop rejecting a duplicated client_assertion_type in the shared dispatcher before checking whether SPIFFE is even configured or selected — that changed default/Fosite handling of entirely non-SPIFFE requests (including RFC 7523 private-key JWT), which this arm must not touch. The SPIFFE JWT path still rejects an ambiguous assertion type once SPIFFE is actually selected, via authenticateSPIFFEJWTClient's existing exact-single-value check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Part of the SPIFFE client-authentication epic (#6199), issue #6203. Stacks on top of #6562 (the coordination dispatcher).
newSPIFFEClientAuthenticationStrategy: a serialized JWT-SVID client assertion (client_assertion_type/client_assertionform fields, capped at 16 KiB) is validated with go-spiffe'sjwtsvid.ParseAndValidateagainst the configured JWT bundle source, requiring the assertion's sole audience to be the configured authorization-server issuer.SPIFFEAssociationRegistry.Resolvepath (the same one Implement X.509-SVID OAuth client authentication (spiffe_x509) #6202's X.509 arm will use) to bind the configured immutable static OAuth client to the resolved principal.invalid_request; validation, association, and mixed-credential failures fail with a genericinvalid_client— deliberately generic so a probing attacker can't distinguish failure reasons. An HTTP Basic authorization header or anyclient_secretform field alongside a SPIFFE JWT assertion is rejected outright (SPIFFE auth must be the sole credential in the request).SPIFFEAssociationRegistry.Resolveis extended to accept an optional client ID: an empty one derives the configured client from the association, letting the JWT arm resolve without the caller needing to separately supplyclient_id.docs/arch/18-spiffe-association-declarations.md) covering the association model and this authentication path's behavior and known limitations (no application-level replay protection/jtipersistence/nonce yet; inherits go-jose's default one-minute claim leeway).Type of change
Test plan
task test)go test ./pkg/authserver/...and the touchedcmd/thv-operatorpackages pass.go build ./...andgo vet ./...are clean.API Compatibility
This PR does not touch operator API surface (no CRD/
v1beta1changes).Changes
pkg/authserver/server/spiffe_client_auth.gopkg/authserver/spiffe_association_registry.goResolveaccepts an optional (derivable) client IDpkg/authserver/server/provider.go,server_impl.gopkg/authserver/server/registration/spiffe_client.godocs/arch/18-spiffe-association-declarations.mdDoes this introduce a user-facing change?
No — SPIFFE JWT client authentication is not reachable yet; the embedded auth server still rejects any non-empty
spiffeTrustDomainsconfiguration until the trust-bundle loading work (#6201) lands.Special notes for reviewers
Second PR in the stack: #6562 (clientauth, base of this PR) → this PR (#6203, JWT) → #6202 (X.509, stacks on top of this one, not yet sent). Please review/merge in that order.
🤖 Generated with Claude Code