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.