Re: [PATCH] Native FTP Relay

On 08/11/2014 05:50 AM, Amos Jeffries wrote:
> 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.

I view this from interoperability point of view: From that point of
view, by default, traffic on the wire is more important than an RFC
text, especially when dealing with an old protocol (with weird
implementations that cannot be changed) and an old RFC (with vague
requirements that can be easily misinterpreted or interpreted
differently). In that context, "Do no harm" may overrule "Obey RFC".

I say this with no disrespect to IETF and without any implication that
RFCs themselves are unimportant. Needless to say, it is often developer
inability to follow an RFC that causes interoperability problems in the
first place (and may force us, proxy developers, to accommodate buggy
implementations).

Furthermore, I understand that being tolerant leads to problems on its
own. If we learn that we have to police FTP command names for some
specific good reason, I would not hesitate supporting the corresponding
changes.

> +1. Branch looks good enough for merge now.

Merged as trunk r13528. I also updated release notes in hope to minimize
your release work, but I cannot test their rendering, and you probably
still want to audit them for style (at least).

I plan to fix Tokenizer::prefix() return value for empty prefixes next.

Thank you,

Alex.
Received on Mon Aug 11 2014 - 16:29:15 MDT