Repository navigation
Add TLS listener config and SPIFFE X.509 authority capability - #6760
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6760 +/- ##
==========================================
+ Coverage 79.62% 79.63% +0.01%
==========================================
Files 815 816 +1
Lines 82955 83168 +213
==========================================
+ Hits 66056 66234 +178
- Misses 16894 16929 +35
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
351e5b5 to
38b57a1
Compare
639e78a to
9a5e056
Compare
9a5e056 to
9420b4d
Compare
38b57a1 to
3ef4f21
Compare
3ef4f21 to
c703e55
Compare
9420b4d to
2080af6
Compare
ChrisJBurns
left a comment
There was a problem hiding this comment.
Multi-agent consensus review
The bundle-source wiring and lifecycle handling look solid overall. I found one behavioral issue and one documentation mismatch that should be addressed before this stack is treated as providing the described capability.
| Finding | Consensus | Severity |
|---|---|---|
| Do not accept a TLS listener configuration that is ignored | 9/10 | Medium |
| Update the SPIFFE architecture docs for the wiring added here | 10/10 | Low |
[Low] Update the SPIFFE architecture docs for the wiring added here (Consensus: 10/10)
docs/arch/18-spiffe-association-declarations.md still says that the file-bundle loader is isolated and that newServer does not wire a JWT bundle source. This PR now constructs the source and passes both the X.509 and JWT sources into the authorization server, so those statements are stale in the resulting stacked branch. The related summaries in docs/arch/09-operator-architecture.md and docs/arch/README.md make the same claim.
Please update the docs to distinguish the source wiring added here from what remains unavailable: startup validation still rejects SPIFFE configuration, and the TLS listener/X.509 verification path is not yet implemented.
Raised by: architecture/operator review
c703e55 to
c9a00f2
Compare
c9a00f2 to
0e4e08b
Compare
2080af6 to
a15864a
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Reviewed a15864a with a four-perspective panel covering authentication/security, lifecycle, operator/API compatibility, and SDK/test contracts. No remaining merge-blocking defect found in this gated groundwork change. The previous ignored-TLS-setting issue is fixed: every non-nil listener configuration now fails explicitly, and both SPIFFE startup gates remain intact. Source ownership, cleanup, nil-interface handling, and the optional authority capability look sound.
Two nonblocking suggestions are attached inline: regenerate the stale TLS documentation/SDK descriptions and strengthen boundary/lifecycle regression coverage. CI now confirms the documentation drift: Docs / Verify Swagger Documentation fails on the old TLSListener description. This is reported separately from the code approval; other checks are still running.
Separate enablement follow-up, not introduced by this PR: the existing file loader rejects valid empty revocation bundles and retains previous authorities. That loader is unchanged from the merge base and remains gated, but its revocation behavior should be corrected before SPIFFE is enabled.
No local tests or linters were run; verification used source inspection and published CI.
SPIFFE X.509-SVID client authentication needs the client certificate from a TLS handshake, which the auth server's routes cannot see over plain HTTP. Introduce the RunConfig shape for a dedicated auth server TLS listener (cert/key file paths) and give the server construction its SPIFFE bundle source, so a way to read the current SPIFFE X.509 trust anchors exists ahead of wiring the listener itself into the runner and operator. The bundle source is wired into server construction now (feeding the existing, currently-unpopulated SPIFFEX509BundleSource/ SPIFFEJWTBundleSource dispatch fields) so the listener has real authorities to hint at TLS handshake time. RunConfig.Validate still hard-rejects any non-empty spiffe_trust_domains (validateSPIFFENotYetEnforced): no code yet verifies a presented X.509-SVID against this bundle, so that gate stays in place until the verification strategy lands. HasSPIFFEX509ClientAuth also lets later validation and wiring code check, without duplicating the association scan, whether any configured association actually needs the listener.
a15864a to
3aa1109
Compare
Summary
RunConfigshape for a dedicated auth server TLS listener (cert and key file paths), and the SPIFFE bundle source wired into server construction.SPIFFEX509BundleSource/SPIFFEJWTBundleSourcedispatch fields, so the listener will have real trust anchors to hint at handshake time once it is wired in.HasSPIFFEX509ClientAuth, so later validation and wiring can check whether any association needs the listener without repeating the association scan.RunConfig.Validate()still hard-rejects any non-emptyspiffe_trust_domains(validateSPIFFENotYetEnforced). No code verifies a presented X.509-SVID against the bundle yet, so that gate stays until the verification strategy lands.Part of #6199. Related: #6202.
Stacked on #6759. The base is
spiffe-v2-01-bundle-file-source, so the diff shows only this change. Retarget tomainonce that PR merges.Type of change
Test plan
task test)task test-e2e)task lint-fix)Built and vetted this commit (
task build,go vetonpkg/authserverandcmd/thv-operator).task genandtask docsleave no diff. New config and server tests are in the diff and run in CI.Does this introduce a user-facing change?
No. Configuring SPIFFE trust domains is still rejected at startup.
Special notes for reviewers
🤖 Generated with Claude Code