Fix: Run the TLS bridge without SPIRE (need-driven SPIFFE provider; drop in-process trust self-check)#523
Conversation
…ss trust self-check
Two boot-path fixes so the TLS bridge runs without SPIRE.
1) Need-driven SPIFFE provider. The platform base config ships an empty
'spiffe: {}' for every agent, and main built the provider on mere presence of
the block — but NewProvider blocks until the SPIRE Workload API returns an
SVID. A proxy-sidecar agent that only runs the TLS bridge (cert-manager CA,
no SVID) on a SPIRE-less cluster would hang forever before binding any
listener. spiffeProviderNeeded now gates construction on actual consumers:
top-level mTLS, or a plugin with identity.type=spiffe (today only
token-exchange). An unused 'spiffe: {}' logs and is skipped.
2) Remove RunTrustSelfCheck. It inspected the sidecar process's own
SSL_CERT_FILE/NODE_EXTRA_CA_CERTS/REQUESTS_CA_BUNDLE, but the operator sets
those on the agent container, not the sidecar — so it logged a false
'agent will not trust minted leaves' WARN on every real deployment. An
in-process check can't validate another container's trust store.
Verified end-to-end: bridge boots with the SPIRE socket unmounted, forges a
leaf from the cert-manager CA, and the agent's HTTPS is decrypted.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoves the ChangesConditional SPIFFE Provider and Trust-Check Removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
authbridge/cmd/authbridge-proxy/main_test.go (1)
30-47: 💤 Low valueConsider adding a test case for malformed plugin config JSON.
The
pluginUsesSPIFFEIdentityfunction has an error-handling path at lines 128-131 inmain.gothat returnsfalsewhen JSON unmarshalling fails. Adding a test case with invalid JSON would verify this defensive path:{"malformed plugin config", outbound(config.PluginEntry{ Name: "bad-plugin", Config: json.RawMessage(`{not valid json`), }), false},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@authbridge/cmd/authbridge-proxy/main_test.go` around lines 30 - 47, The test suite for the pluginUsesSPIFFEIdentity function is missing a test case to verify the error-handling path when JSON unmarshalling fails. Add a new test case to the tests slice that includes a PluginEntry with malformed JSON in the Config field (such as invalid JSON syntax), and verify that the function returns false when it encounters this invalid JSON during unmarshalling, similar to how the existing test cases use the outbound helper function with identityConfig or other Config values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@authbridge/cmd/authbridge-proxy/main_test.go`:
- Around line 30-47: The test suite for the pluginUsesSPIFFEIdentity function is
missing a test case to verify the error-handling path when JSON unmarshalling
fails. Add a new test case to the tests slice that includes a PluginEntry with
malformed JSON in the Config field (such as invalid JSON syntax), and verify
that the function returns false when it encounters this invalid JSON during
unmarshalling, similar to how the existing test cases use the outbound helper
function with identityConfig or other Config values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cb06264a-471d-47b6-88f8-5f56596f4dd0
📒 Files selected for processing (3)
authbridge/authlib/tlsbridge/engine.goauthbridge/cmd/authbridge-proxy/main.goauthbridge/cmd/authbridge-proxy/main_test.go
💤 Files with no reviewable changes (1)
- authbridge/authlib/tlsbridge/engine.go
…d by the need predicate Review follow-up. spiffeProviderNeeded detects a plugin's need via identity.type=spiffe, while BuildWithSPIFFE injects based on the spiffe.ProviderConsumer interface; they agree only because token-exchange is the lone consumer today. Deriving need purely from the interface is not possible — token-exchange implements ProviderConsumer for BOTH identity types and injection happens before Configure, so 'is a consumer' != 'needs the provider' (gating on the interface would rebuild the provider for client-secret agents and reintroduce the SPIRE-less hang). Instead add plugins.SPIFFEConsumerPlugins() and a test that fails if a new ProviderConsumer appears that the predicate may not detect — making the hazard enforceable. Also note RunTrustSelfCheck's removal in the (historical) Phase 1 plan doc. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
clawgenti
left a comment
There was a problem hiding this comment.
Need-driven SPIFFE provider construction and removal of RunTrustSelfCheck are both well-motivated and cleanly implemented; the enforcement test (TestProviderConsumersCoveredByPredicate) is a nice addition that keeps the predicate/interface agreement honest going forward.
All checks pass. Ready for human review.
Reviewed by clawgenti using github:pr-review
| defer registryMu.RUnlock() | ||
| var names []string | ||
| for name, factory := range registry { | ||
| if _, ok := factory().(spiffe.ProviderConsumer); ok { |
There was a problem hiding this comment.
suggestion: factory() is called here purely to probe the interface — it instantiates a full plugin instance just for the type assertion. If any factory has observable side effects (spawning goroutines, opening connections, registering metrics), this enumerate call would silently trigger them. Consider adding a lighter probe path — e.g. a PluginCapabilities() or a separate RegisterConsumerTag registration step — so SPIFFEConsumerPlugins doesn't need to construct live instances. (Fine to defer until a factory with side effects actually appears.)
cwiklik
left a comment
There was a problem hiding this comment.
Clean, well-scoped fix. Gates SPIRE Provider construction on actual need (spiffeProviderNeeded: top-level mTLS, or a plugin with identity.type=spiffe) so a TLS-bridge-only proxy-sidecar boots on SPIRE-less clusters, and drops RunTrustSelfCheck (it inspected the sidecar's trust env, but the operator sets that on the agent container, so it false-WARNed on every deploy).
I verified the main risk — a consumer needing the Provider without matching the predicate — is not present: token-exchange's identity.type is required and validated, and SetSPIFFEProvider is ignored for non-spiffe identities, so a missing/other type fails fast rather than nil-derefs. TestProviderConsumersCoveredByPredicate is a nice tripwire for future consumers.
Areas reviewed: Go, Tests, Docs, Security
Commits: 2, both signed-off (DCO ✓)
CI: all green (Go CI, CodeQL, Trivy, Bandit, pre-commit)
Approving — two non-blocking notes inline.
Reviewed with Claude Code (/github:pr-review).
| // later with a precise error; don't force the provider on for it. | ||
| return false | ||
| } | ||
| return probe.Identity.Type == "spiffe" |
There was a problem hiding this comment.
nit — the literal "spiffe" here duplicates the plugin's SpiffeIdentity constant. If that constant's value ever changes, this probe silently drifts → the consumer receives a nil Provider, and TestProviderConsumersCoveredByPredicate wouldn't catch it (it checks the consumer name set, not the string value). Consider referencing a shared identity-type constant instead of the literal.
| // the full production set. | ||
| func TestProviderConsumersCoveredByPredicate(t *testing.T) { | ||
| // Plugins whose SPIFFE need spiffeProviderNeeded is known to detect. | ||
| covered := map[string]bool{"token-exchange": true} |
There was a problem hiding this comment.
suggestion — covered is a hand-maintained set, so this asserts the consumer name is listed but not that spiffeProviderNeeded actually fires for that plugin's spiffe config. A stronger invariant: for each SPIFFEConsumerPlugins() entry, build a config with identity.type=spiffe and assert the predicate returns true. The current form is a fine tripwire — just noting it's a reminder, not a functional proof.
- pluginUsesSPIFFEIdentity: reference tokenexchange.SpiffeIdentity instead of the duplicated "spiffe" literal, so the probe can't drift from the plugin's own identity-type constant. - TestSpiffeProviderNeeded: add a malformed-JSON case covering the unmarshal error path (returns false). - TestProviderConsumersCoveredByPredicate: in addition to the name tripwire, assert spiffeProviderNeeded actually fires for each consumer's identity.type=spiffe config (functional invariant, not just membership). - SPIFFEConsumerPlugins: document that probing via factory() is safe under the PluginFactory contract (cheap, side-effect-free constructor) and what to do if that ever changes. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Summary
Two boot-path fixes so the AuthBridge TLS bridge (proxy-sidecar) runs on clusters without SPIRE. Follow-up to #522.
1. Build the SPIFFE provider only when something consumes it
The platform base config ships an empty
spiffe: {}for every agent.cmd/authbridge-proxybuilt the SPIFFE provider on mere presence of that block — butspiffe.NewProvider→workloadapi.NewX509Sourceblocks until the SPIRE Workload API returns the first SVID, with no timeout. A proxy-sidecar agent that only runs the TLS bridge mints leaves from a cert-manager CA and never touches an SVID, so on a SPIRE-less cluster it would hang forever before binding any listener (podRunningbut every port closed, no error).spiffeProviderNeedednow gates construction on a real consumer:X509Source), oridentity.type=spiffe— today onlytoken-exchange).An unused
spiffe: {}logsskipping SPIRE providerand is skipped. mTLS without a spiffe block still fails loud via the existing guard.2. Remove
RunTrustSelfCheckIt read the sidecar's own
SSL_CERT_FILE/NODE_EXTRA_CA_CERTS/REQUESTS_CA_BUNDLE, but the operator sets those on the agent container — so it logged a falseagent will not trust minted leaves and egress will safely tunnelWARN on every real deployment (even when the agent trusts the CA fine). An in-process check can't validate another container's trust store.Test Plan
go build+go vet(authlib, cmd/authbridge-proxy)go test— addedspiffeProviderNeededunit coverage (mtls→true, spiffe-identity→true, client-secret→false, empty→false); tlsbridge package tests passskipping SPIRE provider+tls-bridge enabled, binds:8081, forges a leaf from the cert-manager CA (issuer=authbridge-tls-bridge-ca-<workload>), and the agent's HTTPS to example.com is decrypted (HTTP/2 404) — no hang, nopermission denied, notrust self-checkWARN, no SPIRE volume mounted. (Requires the companion operator fix that setsfsGroupfor the CA mount.)Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit