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

[Alex]

14.02.2017, 19:06, "Alex Rousskov" <rousskov at measurement-factory.com>:

>>  + 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).

I suppose that two cases should be enough. AFAIR, forward/reverse configuration is handled by firewall redirection rules.
I have tested the patch in configuration like this, keeping forward proxy in mind:

[ FTP Client, 1.1.1.1] <-------> [ GW with Squid ] <------->  [ FTP Server, 5.5.5.5]


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

Yes, this is the case. I will adjust the comment to make it more clear.

>>  + 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.

Ok, I'll add corresponding notes there.