### Description
I would like to implement support for the Proxy Protocol as implemented by HAProxy and AWS ELBs. The main advantage this brings is better connection information, even behind multiple layers of NAT or LBs. This feature was already discussed a couple of years ago, and it appears the project was not against receiving a patch that adds support for this protocol: https://lists.kamailio.org/pipermail/sr-users/2016-June/093497.html
For example, at the moment, my Kamailio deployment is "blind" when it comes to the origin of inbound TCP connections, because it is only able to report the internal IP address of our ELBs, rather than the actual IP address of the client, and IP address of the LB the connection was received on. Dixa is offering to sponsor work (my own hours) to implement this, and we would also be a production user of this feature.
The Proxy Protocol is fairly well documented here: https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt
As far as I know, AWS ELBs only implement version 1 of the protocol, but I believe there is value in implementing both (while we're at it), and the second version of the protocol is a lot easier to parse, anyway.
I'm opening this feature request to a/ track progress, b/ discuss implementation details.
@miconda I would mainly love some pointers as to how/where to implement this.
My initial thought was to modify `handle_new_connect()` to change the connection state to `S_CONN_OK` instead of `S_CONN_ACCEPT`, and then peek the connection to see if we are getting a PROXY v1/v2 header, and the override the connection information structs. However, this obviously runs amuck of clean code, and obviously might wait a long time if the header isn't sent immediately.
Would it, instead, make more sense to attempt to parse it in a fashion not unlike the `tcp_read_hep3`/`tcp_header_headers` combination in `tcp_read.c` around line 1490:
```c if(unlikely(ksr_tcp_accept_hep3!=0)) { bytes=tcp_read_hep3(con, read_flags); if (bytes>=0) { if(!(con->req.flags & F_TCP_REQ_HEP3)) { /* not hep3, try to read headers */ bytes=tcp_read_headers(con, read_flags); } } } else { bytes=tcp_read_headers(con, read_flags); } ```
So for example:
```c if(unlikely(ksr_tcp_accept_hep3!=0)) { bytes=tcp_read_hep3(con, read_flags); if (bytes>=0) { if(!(con->req.flags & F_TCP_REQ_HEP3)) { /* not hep3, try to read headers */ goto read_headers } } } else { read_headers: if (unlikely(read_proxy_protocol!=0)) { tcp_read_proxy_protocol(con, read_flags); } bytes=tcp_read_headers(con, read_flags); } ```
Obviously, the above is just imaginary pseudocode. Thoughts?
Your idea sounds interesting, and I can see a benefit as well especially for the mentioned use cases (AWS and haproxy setups). Daniel has probably also some comments on this, but in my opinion your approach sounds reasonable.
* Add a core variable, e.g. ksr_tcp_accept_proxy * add a function to parse the proxy protocol(s), like tcp_read_proxy_protocol(..) * probably it makes sense to add a own flag for this as well to check the proper parsing later on (similar to the hep3 execution path) * call the tcp_read_proxy_protocol() in tcp_read_req(..) * add also a proxy_process_msg(..) function to actually use the data to set the internal connection information(s) * then check the flag in receive_tcp_msg(..) and call this function, after your special handling related to the proxy protocol call the normal receive_msg(..) function to process the payload
Just to show you one possible approach (obviously from a high level view).
@teotwaki - following the hep3 handling should be a good approach, thanks for considering to contribute and working on this feature!
@miconda & @henningw: Could I ask for some early input on my current progress?
https://github.com/kamailio/kamailio/compare/master...teotwaki:sl/implement-...
One thing that occurred to me, is that this parsing has to be done at a different moment than what we do with HEP3. From what I understand of the PROXY protocol, the parsing of the PROXY header has to be done as a first step after `accept()`ing a connection, even before TLS negotiation takes place.
@teotwaki - is the PROXY header sent in clear, before establishing the tls connection? If yes, then do it after accept.
Regarding the patch referenced above, I haven't looked extensively at the code parsing the proxy header, but adding the new parameter looks good.
I seem to have made some good progress:
``` 10(20) INFO: {1 1 INVITE 6f350846ab60b1d60295d17268df1605@0.0.0.0} <script>: $si:$sp==35.156.191.128:42759, $Ri:$Rp==100.118.0.3:5060, $RAi:$RAp==staging-sip.dixa.io:5060 ```
`$si` and `$sp` are correct, and refer to the client details outside of the ELB. However, for some reason, `$Ri` and `$Rp` still refers to the bind-address, even though I do override the `dst` parts of the code. There's probably something I'm missing.
I'll submit the PR for review in a bit. @miconda Do you prefer that I squash all commits into a single one, or just clean up the commits into logical chunks?
What would you want to have in `$Ri` and `$Rp`? They have to refer to a local socket, otherwise it may have unexpected side effects for other components.
It should be at least different commits for each major component it changes. You can make it with more commits, it can be squashed from PR web interface as well.
I was expecting to have the proxy's public address and port. I understand this might have side effects, and that's exactly what I wanted to test.
@teotwaki I will also look to your code today or latest tomorrow morning and give you feedback.
Thank you, has been merged into git master.
Closed #1757.
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Sebastian Lauwers