Re: [PATCH] Support PROXY protocol

Attaced patch contains all updated except the config option renaming,
which remains to be sorted out.

On 26/06/2014 7:41 a.m., Alex Rousskov wrote:
> On 06/21/2014 11:15 PM, Amos Jeffries wrote:
>> Support receiving PROXY protocol version 1 and 2.
>
>
>> + proxy-surrogate
>> + Support for PROXY protocol version 1 or 2 connections.
>> + The proxy_forwarded_access is required to whitelist
>> + downstream proxies which can be trusted.
>
>
> Could you suggest some alternative names to "proxy-surrogate", which
> sounds like nothing-on-top-of-nothing to me in Squid context?

We are a surrogate for the PROXY protocol. I know its a bit nasty.

> The
> terrible name for the PROXY protocol itself is clearly not your fault,
> but perhaps we can find a more intuitive way to call this new option?
> Here are some suggestions:
>
> require-PROXY
> expect-PROXY
> require-PROXY-header
> expect-PROXY-header
>
>
> All-CAPS in option names are unfortunate as it goes against Squid style,
> but the poor choice the protocol name essentially forces us to do that.
>

In all seriousness "haproxy-protocol" is probably the most correct
descriptive right now. But I am trying hard to avoid naming a competitor
in our most visible documentations.

"forwarded" would be wonderful, but I can forsee some confusion with
forward-proxy mode.

How about referred, referral, or proxy-referral ?

>
>> -NAME: follow_x_forwarded_for
>> +NAME: proxy_forwarded_access follow_x_forwarded_for
>
> The new name sounds worse than the old one. Hopefully this can be left
> as is or renamed to something better after the "proxy-surrogate" issue
> is resolved.
>

Is "forwarded_access" obectionable ?
The XFF header has been deprecated by the Forwarded: header now, so a
name-changing was in the cards whatever this patch does.

>
>> +bool
>> +ConnStateData::proxyProtocolError(bool fatalError)
>> +{
>> + if (fatalError) {
>> + // terminate the connection. invalid input.
>> + stopReceiving("PROXY protocol error");
>> + stopSending("PROXY protocol error");
>> + }
>> + return false;
>> +}
>
> I recommend using a "const char *" argument type so that you can log a
> meaningful fatalError. Helps with caller code self-documentation too.
> Nil argument would mean non-fatal error, of course.
>

Done. And calling mustStop() now.

>
>> +bool
>> +ConnStateData::parseProxyProtocolMagic()
>
> This appears to parse a lot more than just the magic characters. Please
> rename to parseProxyProtocolHeader() or similar.
>

The entire PROXY protocol is "magic" connection header. I get the point
though, splitting into three functions. The main one being
findProxyProtocolMagic().

>
>> +
>> + needProxyProtocolHeader_ = xact->squidPort->flags.proxySurrogate;
>> + if (needProxyProtocolHeader_)
>> + proxyProtocolValidateClient(); // will close the connection on failure
>
> Please do not place things that require job protection in a class
> constructor. Calling things like stopSending() and stopReceiving() does
> not (or will not) work well when we are just constructing a
> ConnStateData object. Nobody may notice (at the right time) that you
> "stopped" something because nothing has been started yet.

Did not require job protection until converting
stopSending()/stopReceiving() into mustStop() calls.

Also, note that ConnStateData is not a true AsyncJob. It never started
with AsynJob::Start() and cleanup is equally nasty as this setup
constructor (prior to any changes I am adding).

I have done as suggested and created the AsyncJob::Start() functionality
for it.

However, this means that the PROXY protocol is no longer capable of
being used on https_port. The PROXY protocol header comes before TLS
negotiation on the wire, but ConnStateData::start() is only called by
our code after SSL/TLS negotiation and SSL-bump operations complete.

>
>> + // TODO: we should also pass the port details for myportname here.
>
> Is there a good reason not to pass the port details now? Are they not
> available to ConnStateData?
>

No reason. Done.

>
>> + if (ch.fastCheck() != ACCESS_ALLOWED) {
>> + // terminate the connection. invalid input.
>> + stopReceiving("PROXY client not permitted by ACLs");
>> + stopSending("PROXY client not permitted by ACLs");
>> + }
>
> The copied(?) comment is wrong in this context. It is not the input that
> is invalid in this case. However, I think you should call
> proxyProtocolError() here instead of duplicating that code. The char*
> parameter discussed above will make it more suitable for all kinds of
> PROXY errors, not just the input parsing errors.
>

Done.

>
>> + // 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());
>> +
>> + // XXX parse IP and port strings
>> + Ip::Address originalClient, originalDest;
>> +
>> + if (!originalClient.GetHostByName(ipa.c_str()))
>> + return proxyProtocolError(true);
>> +
>> + if (!originalDest.GetHostByName(ipb.c_str()))
>> + return false;
>> +
>> + if (porta > 0 && porta <= 0xFFFF) // max uint16_t
>> + originalClient.port(static_cast<uint16_t>(porta));
>> + else
>> + return proxyProtocolError(true);
>> +
>> + if (portb > 0 && portb <= 0xFFFF) // max uint16_t
>> + originalDest.port(static_cast<uint16_t>(portb));
>> + else
>> + return proxyProtocolError(true);
>> +
>> + in.buf = tok.remaining(); // sync buffers
>> + needProxyProtocolHeader_ = false; // found successfully
>
> The last two lines appear too late to me. You found the header at the
> beginning of the code quoted above. The rest was parsing/validating
> various header components, which is a different matter. The v2 parsing
> code does not have this problem.
>

Done.

> Please move most of the parseProxyProtocolMagic() body into methods
> dedicated to each protocol version. Leave just the version detection
> code. That method is too long/deep for no good reason IMO.
>

I disagree on the function style. But done.

>
>> + debugs(33, 5, "PROXY protocol on connection " << clientConnection);
>> + clientConnection->local = originalDest;
>> + clientConnection->remote = originalClient;
>> + debugs(33, 5, "PROXY upgrade: " << clientConnection);
>
> We use this kind of address resetting code in many places, right? Please
> encapsulate it (together with the debugging) into a
> Connection::resetAddrs() or a similar method.

Two. PROXY/1.0 and PROXY/2.0 parsers. There are similar patterns
elsewhere, but setting one or other address individually.

>
>> + stopReceiving("PROXY protocol error");
>> + stopSending("PROXY protocol error");
>
> The two methods are designed for situations where you actually want to
> stop doing _one_ thing, not both. When you want to stop both, it is
> better to call mustStop(). The swanSong() method will close the
> connection for you in that case.
>
> You may argue that we might reach the new mustStop() call outside job
> protections, but I hope that is not possible because we should always be
> reading new data from an async reading handler.

Okay. Using mustStop().

>
>
>> + /// whether PROXY protocol header is still expected on this port
>> + bool needProxyProtocolHeader_;
>
> s/on this port/on this connection/ or s/on this port//.
>

Fixed.

>
> * When, in a misconfigured setup, somebody sends a PROXY header to a
> regular Squid HTTP port, does the Squid error look obvious/clear enough?
> Or will the admin have a hard time understanding why things do not work
> in that case?
>

Trunk will die with a 400 error quoting the PROXY header as the
"URL" or buffer content. After Parser-NG merge Squid will 400 error
(which quotes the input buffer contents IIRC). It seems clear enough to
me not to need new code for that type of config error case.

Amos