squid
squid copied to clipboard
squid-cache
PROXY protocol support for https_port
To enable PROXY protocol support on an https_port with SslBump:
https_port ... ssl-bump ... require-proxy-header
SslBump is currently required for receiving PROXY protocol on an https_port. The following https_port configurations lead to a fatal configuration error:
- require-proxy-header without the ssl-bump mode
- require-proxy-header with one of the following https_port modes: intercept, tproxy, and accel.
The implementation is based on a new proxySurrogateHttps flag (the already existing proxySurrogateHttp serves for http_port). To avoid complex conditions with TrafficMode flags combinations throughout the code (often duplicated) we had to supply TrafficMode with several methods covering all required modes and making the flags themselves private. The old flags should be accessed/initialized only from the configuration code.
It is unclear to me why this feature addition is tied to intercept+SSL-Bump usage. The PROXY protocol bytes occur before any TLS bytes and may be used with non-intercepted TLS connections, and non-TLS protocols over port 443.
The new feature is limited to one (popular) Squid configuration. Support for other configurations is far from trivial, but can be added later if somebody needs that support. This work makes any such future support easier to implement and review.
It is the PP2_CLIENT_SSL flag of PROXYv2 which tells use whether or not TLS is actually there at all.
This part of the discussion is outside this PR scope, but, no, the PP2_CLIENT_SSL flag is not equivalent to "expect TLS after the PROXY protocol message". That flag indicates that the client connected to haproxy (or equivalent) over TLS. It indicates haproxy configuration that is somewhat similar to Squid https_port without SslBump. AFAICT, the haproxy-Squid connection may contain TLS without that flag (e.g., when haproxy intercepts client TLS). And, if haproxy has or gains appropriate support, the haproxy-Squid connection may contain plain HTTP with that flag (e.g., when the client sends "GET https://" to haproxy over a TLS connection).
Please separate the flags refactoring from the interception refactoring from the PROXY protocol addition
I separated all the refactoring required for the new feature in a dedicated PR835.
PROXY protocol addition
I'll post another PR for this soon.
@eduard-bagdasaryan, see PR #873 for separate implementation receiving/parsing PROXY protocol. It will leave you with the ::Server::xaction->pp2Client object Pointer to access the needed extension values. You should not have to touch PROXY protocol parsing in this PR, just check the listening port flags and use pp2Client detail. Any over-read into TLS related bytes will be left in ::Server::inBuf when SSL-Bump starts - AFAIK this is already the case for CONNECT early-data.
Amos: see PR #873 for separate implementation receiving/parsing PROXY protocol. It will leave you with the ::Server::xaction->pp2Client object
::Server::xaction should not exist (as discussed in https://github.com/squid-cache/squid/pull/865#discussion_r674066436) so any PR relying on that data member is not a good example to follow. There are other serious design concerns about #873, but this is not the place to discuss them, and the situation may change while that draft PR is under active development. We just should not prematurely use that PR as an example to follow.
Eduard: I'll post another PR for [PROXY protocol addition] soon.
Just to avoid misunderstanding: AFAICT, the PROXY protocol addition code that Eduard is talking about above was in this PR from the very beginning. It may need updates based on the splinter PR #835 outcome, of course, but it is existing, working code rather than something that still needs to be designed and written. Eduard is simply following your review request to split this PR into several ones, each dedicated to a specific aspect of this large PR. @eduard-bagdasaryan, please correct me if I am wrong.
it is existing, working code rather than something that still needs to be designed and written
You are right, initially, this PR832 encompassed all the required changes. After discussing we decided to split the code into a couple of PRs, separating refactoring (PR835) from functionality changes. I can post the second part anytime, but think it would be better to let the dust settle in PR835 first.
Sorry, I wrote https://github.com/squid-cache/squid/pull/832#issuecomment-891452843 before re-reviewing this PR. I was under the impression that this change was making SSL-Bump use TLV values found in PROXYv2 headers (eg SNI, client cert). I see that is not happening now.
AIUI, the state of this PR right now is that we need to complete the issues in PR #835, rebase this on the resulting master, then see if this PR needs any further polishing to accept for merge.
the state of this PR right now is that we need to complete the issues in PR #835, rebase this on the resulting master, then see if this PR needs any further polishing to accept for merge.
Yes, that is the plan.
I have removed the S-waiting-for-author label: This PR is not waiting for its author. Since June 2021, this PR is waiting for PR #835 (which was promptly created upon @yadij request). That PR is also stalled, waiting for @yadij review for more than four months now. We do not have a label for this sad state.
Update: I have made a final call on the PR #835 discussion.
@eduard-bagdasaryan please update this PR to use the existing flags object instead of the wrapper API added by that PR.
Also, please be clear that the existing location and handling of the PROXY protocol for http_port was added as a hack to avoid Squid-3 issues that no longer exist in Squid-5+. I am willing to put up with the code duplication this PR adds but please keep objects storing the PROXY information somewhat cleanly isolated from existing ConnStateData members.