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

[Alex Rousskov]

On 02/14/2017 12:25 PM, Alex wrote:
> It seems that the patch doesn't make things worse (if I understood you
> correctly).

AFAICT, you are not testing the use case that prompted [trunk] revision
12742.1.41 changes. There are three possibilities:

  1. r12742.1.41 changes were bogus/useless.
  2. Your changes break use cases supported by r12742.1.41 changes.
  3. Your changes do not break use cases supported by r12742.1.41
     because all of them have a COMM_TRANSPARENT flag set.

To know which outcome is correct, we need a test case that:

  A. Does not work without r12742.1.41 logic.
  B. Works with the current (i.e., post-r12742.1.41) unpatched code.
  C. Does not have a COMM_TRANSPARENT flag set.

If such a test case exists, then #2 is correct. Unfortunately, I do not
have a ready-to-use instructions on how to construct such a use case,
but it probably has to involve:

* direct/non-intercepted control connection (hence, no COMM_TRANSPARENT
flag if you are testing patched Squid code);

* multiple IP addresses from which Squid can open FTP data connections
to the FTP client (i.e., the FTP client IP has to be reachable from the
Squid box from two different source IP addresses);

* TCP stack selecting the wrong default source IP address when
establishing a connection from Squid to the FTP client.

When all of the above bullets are satisfied, r12742.1.41 starts to
matter in the patched code, meeting conditions A-C above. AFAICT, your
current test cases do not satisfy the last two bullets.


In other words, you patch removes setting the source IP address of the
Squid data connection to the destination address of the control
connection when there is no COMM_TRANSPARENT flag. Squid r12742.1.41
claims that such setting is necessary for some use cases to work. It is
not clear whether r12742.1.41 is only relevant to COMM_TRANSPARENT
connections. If it is, then your patch does not break things. If it is
not, then your patch probably does.


Hope this clarifies,

Alex.