Hi Alex, I finally found time to have a look into this!
On Thu, Jun 13, 2024 at 03:00:59AM +0200, Aleksandar Lazic wrote: > > The final idea is something like this. > > > > ``` > > tcp-request content upstream-proxy-header Host %[req.ssl_sni,lower] > > tcp-request content upstream-proxy-header "$AUTH" "$TOKEN" > > tcp-request content upstream-proxy-header Proxy-Connection Keep-Alive > > tcp-request content upstream-proxy-target %[req.ssl_sni,lower] > > > > server stream 0.0.0.0:0 upstream-proxy-tunnel > > %[req.ssl_sni,lower,map_str(targets.map)] > > ``` > > > > The targets.map should have something like this. > > #dest proxy > > sni01 proxy01 > > sni02 proxy02 > > > > I hope the background of upstream-proxy-target is now more clear. > > > > > - In `server https_Via_Proxy1 0.0.0.0:0 upstream-proxy-tunnel > > > 127.0.0.1:3128` > > > from your config, what is 0.0.0.0:0 used for here? This binds to all > > > IPv4 > > > but on a random free port? > > > > This is required when the destination should be dynamic, it's documented > > here. > > http://docs.haproxy.org/3.0/configuration.html#4.4-do-resolve > > > > > A+ > > > Dave > > > > Regards > > Alex Overall I find that there are plenty of good points in this patch and the discussion around it. But it also raises questions: - IMHO the server's address should definitely be the proxy's address. This will permit to use all the standard mechanisms we have such as DNS resolution and health checks and continues to work as a regular handshake. I'm not sure this is the case in the current state of the patch since I'm seeing an extra "upstream-proxy-tunnel" parameter (actually I'm confused by what you call a "target" here, as used in upstream-proxy-target). - for the remote server's name, typically the one we'd extract from a Host header usually or from SNI in some cases (maybe that's what you called the "target" ?), we'd definitely need to support an expression sent as text (usually ip:port), though we could as well find it convenient to set the host and the port separately. In this case it's likely that the sole presence of this expression could be sufficient to enable the feature. - the Host field should default to the one in the URI if there's none explicitly added. - I like the fact that you implemented support for headers upfront, as we all know that this will be a prerequisite for many users. - you should definitely get rid of allthese DPRINTF() debug traces. The simple fact that I no longer know how to enable them should ring a bell about their relevance ;-) - we need to make sure this handshake is sent before SSL and not after. This should permit to do what users normally expect, i.e., encrypt a TCP connection and encapsulate it into a CONNECT request. - please don't realign the srv_kw_list at the end of the patch, it makes it impossible to spot the addition(s) there. We can tolerate a few unaligned entries (even though often it's a sign that a shorter name would be welcome), and if you really need to realign, please do that in a separate patch. - I think the headers addition should also be done into a separate patch since it's a significant part of the patch. It will also enlighten the default behavior since the code must work without extra header rules. - and BTW there's no need for this "upt" entry in the proxy struct, if a string is needed you can just use an ist or even char* which are simpler. But I do think that it should be extracted from the request (something we need to figure how by default). Thanks! Willy
