AlinsRan opened a new pull request, #2864: URL: https://github.com/apache/apisix-ingress-controller/pull/2864
### Type of change: - [x] Bugfix ### What this PR does / why we need it: An HTTPRoute that attaches only to an `HTTPS` listener, via `sectionName: https`, still answers plaintext requests for the same hostname and path. The TLS handshake succeeds on 443 and the same request succeeds unencrypted on 80, reaching the same backend. Nothing in the translation prevents it. Hostname matching cannot tell the two apart, since the hostname is the same. The only existing mechanism is the `server_port` route variable behind `listener_port_match_mode`, and it does not fit this job: - It is `off` by default, so out of the box there is no isolation at all. - Turning it on is not enough. `server_port` is evaluated against the port APISIX accepted the connection on, not the port the Gateway declares. A Service mapping 443 to a data plane listening on 9443 is the normal deployment, and there the injected `server_port == 443` matches nothing, so the route stops serving on **both** protocols. Getting isolation requires renumbering the Gateway listeners to the data plane's physical ports, which contradicts what `spec.listeners[].port` means. - `collectServerPortMatchPorts` skips listeners that set a `hostname`, so a route attached only to such a listener gets no predicate in any mode. That is deliberate and correct for its own purpose, but it means the most common HTTPS listener shape, one with a hostname bound to its certificate, has no isolation available at all. This PR pins a route that attached only to `HTTPS` listeners to the `https` scheme: ```json "vars": [["scheme", "==", "https"]] ``` `$scheme` reflects the connection APISIX accepted rather than a number the Gateway declared, so it holds whatever port mapping sits in front of the data plane, and it works with the default configuration. It is orthogonal to `listener_port_match_mode`, which keeps its own job of pinning a route to a listener port. The mechanism is the same one `addServerPortVars` already uses, so the data plane needs nothing new. A route that also attaches to an `HTTP` listener is left alone: it is meant to serve both. Applies to HTTPRoute and GRPCRoute. The L4 routes have no scheme. ### The case this does not fit TLS terminated in front of APISIX, where the connection APISIX accepts is plaintext even though the client used HTTPS. `$scheme` is then `http` and the route would not match. In that topology the Gateway is not terminating TLS, so the listener's `certificateRefs` are unused and the listener should be declared `HTTP`. The documentation change says so. It is worth a reviewer's attention because it is the one way this change can take traffic down rather than protect it, and I would rather have it called out than discovered. If that risk is judged too high for a default, gating this behind a configuration field is a small change and I am happy to make it. My reading is that it should be on by default: serving an HTTPS-only route over plaintext is a data exposure, and the configuration that breaks is already inconsistent. ### Pre-submission checklist: - [x] Did you explain what problem does this PR solve? Or what new features have been added? - [x] Have you added corresponding test cases? - [x] Have you modified the corresponding document? - [ ] Is this PR backward compatible? Not backward compatible, deliberately: a route attached only to HTTPS listeners stops answering plaintext requests. That is the fix. Tests: `internal/adc/translator/httproute_scheme_test.go` covers HTTPS-only with and without a listener hostname, the mixed case, plaintext-only, no listeners, and that the mode does not affect it. The three positive cases fail without the change. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
