Re: [PATCH] Support PROXY protocol

On 08/12/2014 10:17 AM, Amos Jeffries wrote:
> On 11/08/2014 4:32 p.m., Alex Rousskov wrote:
>> On 08/05/2014 08:31 PM, Amos Jeffries wrote:
>>> + tok.skip(Proxy1p0magic);
>>
>> We already know the magic is there. If you want to optimize this, then
>> skip() in ConnStateData::parseProxyProtocolHeader() and pass the
>> Tokenizer object to the version-specific parsers. I do not insist on
>> this change, but you may want to add a corresponding TODO if you do not
>> want to optimize this.
>>
>
> This is done with the parser method Tokenizer so that if we happen not
> to receive the whole header in one read for any reason we can easily
> undo and retry on the following read(2).
> Only if the Tokenizer parses completely and fully is the I/O buffer
> updated.

I know.

> We could generate a temporary SBuf and setup the Tokenizer with that.
> But it's simpler just to setup the Tokenizer with the existing buffer
> and unconditionally skip the magic we already know exists.

No temporary SBuf is needed AFAICT. You just _move_ the Tokenizer
creation into ConnStateData::parseProxyProtocolHeader() where you can
skip() the magic instead of doing startsWith() tests. Any buffer updates
happen when and where they happen today.

This is just an optional optimization, nothing more, of course. Not
required (although it would have helped avoid the missing skip() bug).

>>> + // parse src-IP SP dst-IP SP src-port SP dst-port CRLF
>>> + if (!tok.prefix(ipa, ipChars) || !tok.skip(' ') ||
>>> + !tok.prefix(ipb, ipChars) || !tok.skip(' ') ||
>>> + !tok.int64(porta) || !tok.skip(' ') ||
>>> + !tok.int64(portb) || !tok.skip('\r') || !tok.skip('\n'))
>>> + return proxyProtocolError(!tok.atEnd()?"PROXY/1.0 error: invalid syntax":NULL);
>>
>> AFAICT, atEnd() may be false just because we have not received the whole
>> PROXY header yet, not because we received something invalid. For
>> example, int64() returns false not just on failures but on "insufficient
>> data" (as it should!).
>>
>
> I am not worried about those failure cases particularly, allowing for
> sender emitting one field per packet is a tolerance optimization to
> begin with. The entire header is designed to be sent in one packet and
> the spec explicitly requires it to be sent in a single write(2).

The PROXY RFC says "A receiver may reject an incomplete line which does
not contain the CRLF sequence in the first atomic read operation". If
this ever gets to the IETF review, I hope this requirement will be
stricken down. An "atomic read" is a bogus concept in this context; the
whole requirement violates the spirit of a TCP-compatible protocol and
will lead to interoperability problems.

It does not matter what the sender does because a lot of things can
happen to a packet after the write(2) call. IMO, we MUST accept any
valid PROXY header regardless of the packet and I/O segmentation at
lower levels. Fortunately, it is not very difficult to do in Squid.

>> [N.B. prefix() should behave similarly on insufficient data, but it is
>> currently broken; I can fix after the FTP Relay project is done.]

That Tokenizer::prefix() fix is also in trunk now.

>>> + if ((in.buf[0] & 0xF0) != 0x20) // version == 2 is mandatory
>>> + return proxyProtocolError("PROXY/2.0 error: invalid version");
>>> +
>>> + const char command = (in.buf[0] & 0x0F);
>>> + if ((command & 0xFE) != 0x00) // values other than 0x0-0x1 are invalid
>>> + return proxyProtocolError("PROXY/2.0 error: invalid command");
>>
>> These and similar v2 checks do not make sense to me because we know for
>> sure that in.buf starts with Proxy2p0magic. Why test parts of a
>> hard-coded constant? I am guessing you meant to skip magic before these
>> tests...
>>
>> Which forces me to ask whether the PROXY v2 path was tested recently?
>
> Willy reported it working before the audit,

Strange. I do not see how that code could have worked, but that is not
important, I guess. The RFC already claims Squid support! :-)

>>> + const char *clen = in.buf.rawContent() + Proxy2p0magic.length() + 2;
>>> + const uint16_t len = ntohs(*(reinterpret_cast<const uint16_t *>(clen)));
>>
>> Could this cast violate integer alignment rules on some platforms? If
>> yes, we may have to memcpy those two bytes first. I do not think we can
>> guarantee any specific alignment for in.buf.rawContent() and, hence, we
>> cannot guarantee any specific alignment for clen.
>>
>
> Unsure. The uint16_t should be compiled as 8-bit aligned AFAIK. In
> theory we only encounter that type of problem with the more complex
> types (like pax below).
>
> I dont like casting anyway, so if you have any better way to do it
> without a double data copy I am interested. Note that ntohs() is
> effectively one data copy.

Your call, but I would memcpy() before interpreting that clen value to
be on the safe side. It is a very fast copy. If you do not memcpy(),
please add a comment. For example:

// We hope uint16_t can start at any byte, w/o alignment problems.

Thank you,

Alex.
Received on Wed Aug 13 2014 - 00:18:59 MDT