[squid-dev] [PATCH] Native FTP relay for active FTP

[Alex Rousskov]

On 02/14/2017 03:44 AM, Alex wrote:

> The attached patch allows FTP relay to work in NAT interception mode
> and also fixes IP address binding in TPROXY mode.

Thank you for working on this bug. NAT/TPROXY address manipulation is
not my area of expertise, but I have one higher level concern and a few
less important clarification requests:


> +    if (clientConnection->flags & COMM_TRANSPARENT) {
> +        conn->setAddrs(clientConnection->local, cltAddr);
> +        conn->flags |= COMM_TRANSPARENT;
> +    } else {
> +        // In case of NAT interception ...

Are there really just two cases here (tproxy and NAT)? IIRC, a "forward
FTP proxy" mode without any TCP/IP level redirection and address
rewriting tricks used to work fine, but your email and the new code may
be interpreted to imply otherwise. If there are indeed three supported
cases, then the new if-statement condition may need to be adjusted (at
least).

> +        // In case of NAT interception squid's local address
> +        // will be used for outgoing connection.
> +        conn->local.setAnyAddr();
> +        conn->remote = cltAddr;

In this context, the phrase "squid's local address" can mean a lot of
different things. Is there a more precise term we can use here? For
_example_, the existing comment uses "local IP address of the control
connection" which is a lot more specific.

Also, does the comment talk about the source or the destination address
for the "outgoing connection"? For example, the existing comment says
"as the source address of the active data connection" which is a lot
more specific.

If "outgoing connection" means "data connection", then I recommend using
the latter terminology because it is more precise (there are several
connections in this context that somebody might consider "outgoing",
depending on how they view their proxy role or network "position").


Finally, it is not clear to me whether the new comment means something
like this:

* If we set conn->local to any IP address (with the right version), then
the TCP stack will pick the correct source address for the data
connection because we are using NAT.

or something like this:

* The exact conn->local value does not matter because the TCP stack will
automatically pick the correct source address for the data connection
when we are using NAT. Just make sure the IP version is correct.

Or something else entirely (e.g., perhaps it is not the TCP stack but
some other Squid code that selects the right source address). The
passive voice in the new comment may be obscuring the intended meaning.



> +        if (conn->remote.isIPv4())
> +            conn->local.setIPv4();


I know that Squid uses the same code elsewhere, and I assume this
"works" today, but it looks misleading to me. Do we want the local
address to have the same IP version as the remote address has? If yes,
the above code does not say that and, ideally, should be adjusted.
Again, I am not saying that this code does not work.


Thank you,

Alex.