BUG/MEDIUM: ingress: keep https frontend for REQ_DENY/REQ_CAPTURE when SSLPassthrough is cluster-wide - #830
Closed
KlausNie wants to merge 2 commits into
Conversation
…n SSLPassthrough is cluster-wide haproxy.SSLPassthrough is a controller-wide flag: it becomes true as soon as any single ingress in the cluster uses ssl-passthrough, not just the ingress currently being processed. addRules() used that flag to decide where an ingress's allow-list/deny-list (REQ_DENY) or capture (REQ_CAPTURE) rule gets attached, swapping FrontHTTPS for FrontSSL whenever the flag was true - for every ingress, including ones that never opted into ssl-passthrough themselves. But a non-passthrough ingress's TLS traffic still terminates on FrontHTTPS: HTTPS.toggleSSLPassthrough replaces FrontHTTPS's public binds with an internal PROXY-protocol socket once SSLPassthrough is active, and FrontSSL's default backend (ssl-backend) forwards non-passthrough SNIs there. Dropping FrontHTTPS from the rule's frontends left that traffic completely unchecked by its own allow-list/deny-list, independent of whatever intermittent frontend-selection bug affected REQ_DENY placement before it (see haproxytech#770 / the fix in 79d754a). Keep FrontHTTPS in the frontend list alongside FrontHTTP and FrontSSL so both traffic paths - genuine SNI-routed passthrough and internally forwarded TLS termination - get the rule. Adds an e2e test (allowlist_passthrough_test.go) reproducing the bug: an unrelated ssl-passthrough ingress coexisting with a plain allow-list ingress, asserting the latter's HTTPS traffic is still denied.
- Fix new test file's copyright year (2019 -> 2026, matches the project's year-of-addition convention elsewhere in the repo). - Match sibling test naming/doc-comment style in the https e2e package (Test_HTTPS_Redirect/Offload/Passthrough are terse; the new suite's wrapper comment and test method name were verbose outliers). - Remove the sidecar ingress's deferred Delete() cleanup call: Test.Delete has no way to target the test's dynamic namespace (unlike Apply), so it was silently no-op'ing against the wrong namespace on every run. Documented the reliance on TearDownSuite's whole-namespace teardown instead of pretending per-test cleanup works. - Drop the trailing period on the new inline comment in addRules() to match its immediate neighbor's style.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
haproxy.SSLPassthroughis a controller-wide flag: it becomes true as soon as any single ingress in the cluster usesssl-passthrough, not just the ingress currently being processed.addRules()used that flag to decide where an ingress's allow-list/deny-list (REQ_DENY) or capture (REQ_CAPTURE) rule gets attached, swappingFrontHTTPSforFrontSSLwhenever the flag was true — for every ingress, including ones that never opted into ssl-passthrough themselves.But a non-passthrough ingress's TLS traffic still terminates on
FrontHTTPS:HTTPS.toggleSSLPassthroughreplacesFrontHTTPS's public binds with an internal PROXY-protocol socket onceSSLPassthroughis active, andFrontSSL's default backend (ssl-backend) forwards non-passthrough SNIs there. DroppingFrontHTTPSfrom the rule's frontends left that traffic completely unchecked by its own allow-list/deny-list — independent of, and in addition to, the intermittent frontend-selection bug fixed in #770 / 79d754a. That earlier fix made frontend selection stable; it didn't change that the selection itself dropsFrontHTTPSwhenever any ingress anywhere uses passthrough.Observed live: an ingress with
haproxy.org/allow-listsharing a cluster with an unrelatedssl-passthroughingress let non-allow-listed source IPs reach the backend over HTTPS (TCP and QUIC/HTTP3) with no enforcement at all, while the same allow-list correctly blocked plain HTTP.Fix
Keep
FrontHTTPSin the frontend list alongsideFrontHTTPandFrontSSLso both traffic paths — genuine SNI-routed passthrough and internally-forwarded TLS termination — get the rule. Confirmed this is provably safe for the mixed-cluster case: a non-passthrough ingress's host is never written into the SNI map, so the extraFrontSSLplacement it already had is inert there; the missing piece was purelyFrontHTTPS.Test plan
go build ./...andgo vetpassdeploy/tests/e2e/https/allowlist_passthrough_test.go: deploys an unrelatedssl-passthroughsidecar ingress alongside a plainallow-listingress and asserts the latter's HTTPS traffic is still denied for a non-allow-listed clientgo build -tags e2e_https ./deploy/tests/e2e/...passes