Re: [PATCH] Support PROXY protocol

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? 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.

> -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.

> +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.

> +bool
> +ConnStateData::parseProxyProtocolMagic()

This appears to parse a lot more than just the magic characters. Please
rename to parseProxyProtocolHeader() or similar.

> +
> + 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.

For example, while proxyProtocolValidateClient() may close the
connection, the code following ConnStateData creation will not notice
that and will call ConnStateData::readSomeData() which might even
assert. This is just an example; other things can go wrong.

It is OK to initialize data members and copy configuration values in the
constructor, like the current code does, but the actual work should
start in start(). In general, the more stuff you can do in or after
start(), the better!

[Besides lacking job protections, the constructor also cannot call
virtual functions, which makes making custom kid classes difficult.
Please keep the constructors as simple/limited and as possible.]

I have not studied every detail here, but I recommend adding
ConnStateData::start() and moving everything you can from the
ConnStateData constructor there. The new proxyProtocolValidateClient
call should be in start(), for example. At the end of start(), if things
are still OK, call readSomeData(). Remove that readSomeData() call from
where we create ConnStateData now, replacing it with an
AsyncJob::Start() call.

Please do not forget to call parent's start() method from kid's start()
method, despite the fact that the former may be empty.

> + // 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?

> + 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.

> + // 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.

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.

> + 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.

> + 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.

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

s/on this port/on this connection/ or s/on this port//.

* 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?

HTH,

Alex.
Received on Wed Jun 25 2014 - 19:41:39 MDT