Re: [PATCH] Native FTP Relay

On 11/08/2014 11:30 a.m., Alex Rousskov wrote:
> On 08/10/2014 06:11 AM, Amos Jeffries wrote:
<snip>
>> * please use tok.prefix(CharacterSet::ALPHA) to parse the FTP command
>> instead of BlackSpace. Then explicit delimiter check to validate the input.
>> - RFC 959 is clear:
>> "The command codes themselves are alphabetic characters terminated
>> by the character <SP> (Space) if parameters follow and Telnet-EOL
>> otherwise."
>
> Not changed. I do not see a compelling reason for a general purpose
> relay to police traffic by default. Squid itself does not care if there
> is some other character present in some command name. Why increase the
> chances of breaking what was working without Squid by rejecting commands
> by default?
>
> Yes, it is possible that some command that Squid does care about would
> have invalid trailing characters in it, preventing Squid from
> recognizing it, but, with your approach, Squid would have to reject that
> command anyway, so we would not break something that would work in a
> policing Squid. In the worst case, we would just make triage more difficult.
>
> If you insist on Squid rejecting RFC-invalid command names, I will
> change the code, but I suggest leaving it permissive for now.
>

I disagree be should be quite so tolerant in the current Internet
without an explicit reason. But that is not a blocker on this patch, we
can fix it later.

RFC 959 is quite explicit. Section 5.3 documents what consists a valid
command (alphabet characters only, case insensitive, 4 or fewer).
Also, the basic FTP commands are a fairly well-known set. Everything
else must be listed in FEAT - which we can filter for offerings of
commands not fitting the supported syntax.

+1. Branch looks good enough for merge now. Please do, so I can re-work
the other concurrent submissions on the new
Tokenizer/CharacterSet/ConnStateData APIs.

Amos
Received on Mon Aug 11 2014 - 11:50:47 MDT