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

[Alex Rousskov]

On 02/14/2017 10:38 AM, Alex wrote:
>>>  + 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.

When I am talking about the forwarding case, I am talking about a simple
configuration without any redirection or other TCP/IP level tricks or
even reverse proxing of any sort: The FTP client sends FTP requests
directly to Squid's ftp_port (specifying the ultimate destination using
an extra @ sign). IIRC, that used to work with popular proxy-aware FTP
clients when we added the ftp_port code.

In that simple case, some FTP client refuse to accept the FTP data
connection from Squid unless that data connection comes from the same
source IP address as the destination address of the corresponding FTP
control connection. The old "Use local IP..." code/comment were added to
make such clients happy (the changes in [trunk] revision 12742.1.41 are
pretty telling, especially in the light of your changes).

If my recollection is correct, then we should continue to support that
simple case. Your patch breaks that support and, AFAICT, your "Squid is
a gateway" test does not cover that scenario.


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

I was thinking about using an Ip::Address method that sets "this" IP
version to the version of the supplied IP address. You can add such a
method if we do not have it already.


Thank you,

Alex.