Add support for Proxy Protocol between dnsdist and the recursor #8874
Conversation
|
I realized that the current logic of trying to reuse TCP backend connections is completely flawed when the proxy protocol is enabled. I'm working on a fix. |
|
It's in fact more complicated than that because of the custom values. Currently they can be set per-query which works for UDP but doesn't match the fact that they can only be sent once per backend connection over TCP. We could disable connection reuse when the proxy protocol is enabled, but that would be quite bad for performances. On the other hand, if we keep it, we at least need to ensure that the same backend connection can only be used for the same incoming client connection which is a bit better for performances but cumbersome. |
I've been wondering if the protocol should come with some framing, but that's another rabbit hole. |
|
Fixed by disabling TCP connection reuse in dnsdist when a backend has the proxy protocol feature enabled. We could try to still reuse the connection for the same client, as long as the TLV values are empty, but it's error-prone since in that case we should not add the proxy protocol payload for subsequent queries, except if the connection breaks at some point and we need to open a new one. |
Yes, we are planning on adding support for incoming proxy protocol payload! Unfortunately our time is finite so we had to make some choices. This PR will not extend beyond outgoing proxy protocol, and I'm not sure yet whether we will be able to implement incoming support before 1.5.0, but it will definitely be there in 1.6.0 at the latest. |
1fb427a
to
8e18166
Compare
|
Rebased on master to fix a conflict with #8851. |
|
@omoerbeek I requested a review from you mostly for the recursor-related bits, although your review on the rest of the code would be nice to have as well :-) |
There was a problem hiding this comment.
- It should be documented how the proxy protocol and
allow-frominteract. AFAKS, if a request is proxied, the from address in the proxy header is used to check againstallow-from. - I don't know if the recursor packer parsing is subject to being fuzzed, but it would be nice, especially with these additions.
- A check for interoperability with another implementation would really be nice.
More comments inline.
|
I have fixed most of the reported shortcomings.
Done!
I'm planning on adding a fuzzing target for the proxy protocol parser itself, but if you are thinking of the TCP handling code it's a bit complicated because it's event-based, with a state machine.
Agreed, I'll try to do that. |
Co-Authored-By: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Co-Authored-By: Otto Moerbeek <otto.moerbeek@open-xchange.com>
82dc348
to
5d802d1
Compare
Did you manage to do a TCP test? That is at least something... |
I did, see #8874 (comment) |
Great! I missed that comment earlier. LGTM! |
Short description
This PR implements:
It allows dnsdist to pass the initial source and destination addresses and ports to the recursor, along with custom values.
Compared to the (ab)use of EDNS Client Subnet, this doesn't prevent preserving an existing ECS value (zeo scope, notably) and provides the ports, along with custom values.
Compared to ECS and XPF, this prevents having to parse the existing payload and inject either an EDNS option or a new record, preserving TSIG-signatures.
Based on a rebased version of #8448.
Could use a good review :)
Checklist
I have: