Hi Willy.
On 2024-08-12 (Mo.) 10:01, Willy Tarreau wrote:
Hi Alex,
I finally found time to have a look into this!
Great :-)
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).
Well I thought the same but that's then difficult to separate between the proxy
server and the destination server. Maybe we need some kind of server type?
As you can see in the sequence diagram are both servers from HAProxy point of
view.
https://github.com/git001/tls-proxy-tunnel/blob/main/README.md#sequence-diagram
The "target" is the "destination server" which is from my point of view the
"server" in haproxy.
The "upstream-proxy-target" is the upstream proxy server. I'm also not happy
with that name and open for suggestions.
- 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.
Full Ack.
- 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 ;-)
:-)
What's the suggested alternative?
I thought "*TRACE*" but haven't seen any good documentation how to use it.
I'm happy to switch to any better debugging setup.
- 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.
I thought that's how I added it, looks like I have overseen something.
From my understanding is the call in "xprt_handshake_io_cb(...)" before the
TLS/SSL handshake, is this wrong?
- 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.
Okay. I'm happy to discuss a better name.
- 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.
Okay.
- 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).
In general, is the "struct proxy{ { ... }tcp_req; }" the right one to add this
feature?
Maybe it's better to add it into the l4 or l5 ruleset?
I haven't seen any good way to define a server type like "upstream", "origin",
"any-other-type" for the server line maybe this type keyword could help to get
the separation between an upstream proxy server and an origin/target/destination
server.
Thank you for your feedback.
Thanks!
Willy
Regards
Alex
