Re: [PATCH] Support PROXY protocol

On 15/07/2014 4:25 a.m., Alex Rousskov wrote:
> On 07/12/2014 10:45 PM, Amos Jeffries wrote:
>
>> +bool
>> +ConnStateData::findProxyProtocolMagic()
>> +{
>> + // http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt
>> +
>> + // detect and parse PROXY protocol version 1 header
>> + if (in.buf.length() > Proxy10magic.length() && in.buf.startsWith(Proxy10magic)) {
>> + return parseProxy10();
>> +
>> + // detect and parse PROXY protocol version 2 header
>> + } else if (in.buf.length() > Proxy20magic.length() && in.buf.startsWith(Proxy20magic)) {
>> + return parseProxy20();
>> +
>> + // detect and terminate other protocols
>> + } else if (in.buf.length() >= Proxy20magic.length()) {
>> + // input other than the PROXY header is a protocol error
>> + return proxyProtocolError("PROXY protocol error: invalid header");
>> + }
>> +
>
> I know you disagree, but the above looks much clearer than the earlier
> code to me. Thank you.
>
> The "// detect and ..." comments are pretty much not needed now because
> the code is self-documenting! The "else"s are also not needed. Your call
> on whether to remove that redundancy.
>
> Consider adding
>
> // TODO: detect short non-magic prefixes earlier to avoid
> // waiting for more data which may never come
>
> but this is not a big deal because most non-malicious clients will not
> send valid non-PROXY requests that is 12 bytes or less, I guess.
>

Added.

>
>> + // XXX: should do this in start(), but SSL/TLS operations begin before start() is called
>
> I agree that this should be done in start(). Fortunately, the code this
> XXX refers to does not rely on job protection (AFAICT) so it is not a
> big deal. The XXX is really about the SSL code that makes the move
> difficult.
>

Where should socket MTU discovery be configured if not at the point
before where the connection actually starts being used?

The constructor where this code block exists is well after accept(), but
also well before socket usage (other than TLS).

>
>>>> -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 ?
>
> IMO, it is misleading/awkward. Follow_x_forwarded_for was pretty good.
> We can use something like follow_forwarded_client_info or
> trust_relayed_client_info, but let's wait for the primary naming issue
> to be resolved first. That resolution might help us here.
>
>
>>>> +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.
>
> Not really. In this context, "magic" is, roughly, a "rare" string
> constant typically used as a prefix to detect/identify the following
> structure or message. The PROXY protocol header contains both "magical"
> and "regular" parts.
>
>
>> +/**
>> + * Test the connection read buffer for PROXY protocol header.
>> + * Version 1 and 2 header currently supported.
>> + */
>> +bool
>> +ConnStateData::findProxyProtocolMagic()
>
> This method does not just "test". It actually parses. IMO,
> findProxyProtocolMagic() should be called parseProxyProtocolHeader().
>
> No, it does not matter that the actual non-magic parsing code is in
> other parsing methods. The method description and name should reflect
> what the method does from the caller point of view. The method internals
> are not that important when you are naming and describing the interface.
>

Okay, okay. Done.

>
>>>> + 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.
>
> It only looked that way, but sooner or later that code would break
> because it was essentially stopping the job inside the job's constructor.
>
>
>> 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).
>
> Yes, I know that ConnStateData is an AsyncJob that already violates a
> lot of job API principles. That is not your fault, of course. However,
> adding more violations into the job constructor makes things worse
> (somebody would have to rewrite that later).
>
>
>> I have done as suggested and created the AsyncJob::Start() functionality
>> for it.
>
> Thank you. Hopefully, you would be able to keep that change despite the
> extra work it requires.
>
>
>> 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.
>
> I agree that there is a problem with https_port support, but the "moving
> PROXY ACL check into start() brakes https_port support for PROXY"
> argument does not work for me: I agree that the SSL code in trunk does a
> lot of things between creating the ConnStateData job and calling
> readSomeData(). However, that was true for the original (mk1) patch as
> well! Moreover, the original (mk1) patch could close the connection in
> the ConnStateData constructor and then proceed with negotiating SSL for
> that connection. Thus, I do not think fixing ConnStateData constructor
> is the source of the htts_port support problems. That support was
> already broken in mk1.
>
> I see two correct ways to address the https_port problem:
>
> 1. Refuse PROXY support on https_port for now. As any support
> limitation, this is unfortunate, but I think it should be accepted. The
> configuration code should be adjusted to reject Squid configurations
> that use proxy-surrogate with an https_port.

Doing this one. I have no desire for re-writing all the SSL negotiation
logics right now.

>
> 2. Support PROXY on https_port. As you have mentioned, this probably
> requires moving SSL negotiations inside the ConnStateData job so that
> the job can be Start()ed when it is created instead of passing the job
> as a POD-like structure during SSL negotiations.
>
>
>> +<p><em>Known Issue:</em> Due to design issues HTTPS traffic is not yet accepted
>> + over this protocol. So use of <em>proxy-surrogate</em> on <em>https_port</em>
>> + is not supported.
>
> If you continue going with #1, please do not blame mysterious "design
> issues" (Squid design? PROXY protocol design? OpenSSL design? Internet
> design?) but simply say that PROXY for https_port is not yet supported.
> There is no design issue here AFAICT. Https_port support just needs more
> development work. This is just an implementation limitation.

They are Squid design issues:

 1) creating the ConnStateData Job before negotiating TLS/SSL using
old-style global functions. But only start()ing it after TLS negotiation.

 2) sharing a socket between ConnStateData read(2) and OpenSSL
openssl_read(2) operations.

Specifically #2 has issues with ConnStateData reading arbitrary amount
of bytes off the socket into its I/O buffer before identifying the end
of PROXY header. So even if we started the ConnStateData early and
paused for TLS/SSL negotiations later, due to AsyncCall delays between
accept() and ConnStateData::start() an unknown amount of TLS/SSL bytes
may be sucked in.

>
> BTW, when receiving on an https_port, will the PROXY header be
> encrypted? If yes, why not postpone the ACL check until the connection
> is decrypted? And where does the PROXY draft explain/define whether the
> PROXY header should be encrypted?

It is documented as a prefix on the TCP layer payload. So AIUI, the
header is an un-encrypted prefix before the TLS ClientHello.

>
>> +<p>Squid currently supports receiving HTTP via version 1 or 2 of the protocol.
> ...
>
> "receiving HTTP via [PROXY] protocol" sounds awkward to me. The PROXY
> protocol does not envelop or embed the HTTP protocol that follows the
> PROXY header IMO. It just starts a connection with a small header and
> does not have a notion of "PROXY message body".
>
> If "version 1 or 2" is the PROXY protocol version (rather than the HTTP
> version), then the above is inconsistent with the version 1.5 documented
> later:
>
>> +proxy-protocol.txt
>> + Documents Proxy Protocol 1.5, for communicating original client IP
>> + details between consenting proxies and servers even when
>> + transparent interception is taking place.
>
> I think the above ought to say "Documents PROXY Protocol versions 1 and
> 2", just like the draft title.
>

Fixed.

>
>> +<p>PROXY protocol provides a simple way for proxies and tunnels of any kind to
>> + relay the original client source details ...
>
>> +proxy-protocol.txt
>> + Documents Proxy Protocol 1.5, for communicating original client IP
>> + details ...
>
> AFAICT, the PROXY protocol supports a lot more than client IP details.
> It also communicates client source port, transport protocol, and the
> destination address details.

Fixed.

>
>
>> +<sect1>Support PROXY protocol
>
> We only support receiving PROXY protocol; Squid does not support being a
> PROXY protocol client, right? Can we be less ambitious in the above
> claim then? Something like "PROXY protocol support (receiving)" or
> "Support for receiving PROXY protocol header" would work better IMO.
>
>

Done.

>> + HTTP message Forwarded header, or
>> + HTTP message X-Forwarded-For header, or
>> + PROXY protocol connection header.
>
>> + Allowing or Denying the X-Forwarded-For or Forwarded headers to
>> + be followed to find the original source of a request. Or permitting
>> + a client proxy to connect using PROXY protocol.
>
> What happens when the incoming traffic has both an allowed PROXY header
> and an allowed HTTP X-Forwarded-For header? Which takes priority?

 PROXY is about the TCP connection and equivalent to NAT on the local
machine.

 XFF/Forwarded is about the single message where it resides.

When evaluating the security direct TCP is evaluated first, then PROXY,
then XFF entries.

TCP direct IP
  [ PROXY src-IP ]
  XFF last entry
  [ XFF 2nd to last entry ]
  [ ... ]

> What
> if X-Forwarded-For header changes client information in the middle of a
> connection? Please document these cases. I could not find that info in
> the PROXY protocol draft, but perhaps I missed it.

Same thing as if XFF was received on a non-PROXY connection. AFAIK that
changes only the HttpReuqest values for the particular message.

XFF does not change its behaviour in any way due to PROXY protocol
existence. The connection IP is checked for trust then the XFF entries
until one fails. It just happens that the connection IP now comes from
PROXY.

The relation XFF and PROXY have is that the XFF trustworthiness depends
on PROXY being trusted if present. The ACL trust assignment semantics is
identical when handled as diagrammed above. PROXY being a sub-step up
from direct TCP details which also needs to be verified to retain any
trust link between direct TCP client IP and the indirect client IP in XFF.

BTW, this confusion you seem to have between the two is exactly why I am
trying to rename follow_x_forwarded_for - since it does not necessarily
relate to XFF header when evaluating trust of PROXY protocol. And
certainly won't when we upgrade to only supporting Forwarded: header.

>
> Please adjust the documentation to make it clear whether the features
> are exclusive (either/or) or can co-exist (and/or).
>
>
>> + 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.
>> +
>
> What happens to the destination information in the PROXY header? Is it
> used in any way? Should we mention how it is used (or the fact that it
> is not used)?

If we can trust the source then we use the PROXY protocol header as per
PROXY protocol. Do we really need to enumerate whole chapters of the
protocol spec in the documentation of its config option?

For example, I dont see anything from RFC 2616 or 7230 documenting
"accel" or anything documenting TLS handshakes for ssl-bump.

If we need detailed long descriptions we have the wiki and/or the PROXY
spec document itself.

>
> Consider s/downstream/client/.
>
>
>> + /** marks ports receiving PROXY protocol traffic
>> + *
>> + * Indicating the following are required:
>> + * - PROXY protocol magic header
>> + * - src/dst IP retrieved from magic PROXY header
>> + * - reverse-proxy traffic prohibited
>> + * - intercepted traffic prohibited
>> + */
>> + bool proxySurrogate;
>
> Why is reverse-proxy traffic prohibited? The PROXY protocol draft
> mentions reverse proxies being used, but it is not clear to me whether
> they are only supported as PROXY protocol clients. I do not know all the
> PROXY details, but it seems to me that reverse proxies should be allowed
> as servers (AFAICT, the explicit configuration requirement is the key
> here, not the reverse/forward role of the proxy!).

Hmm. Good point.
I'm removing those prohibitions and replacing with just an implicit
no-spoofing for TPROXY.

>
>>>> + 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.
>
>
> I found more, including:
>
>> ./log/TcpLogger.cc: futureConn->remote = remote;
>> ./log/TcpLogger.cc- futureConn->local.setAnyAddr();

The local setup contains a setAnyAddr() and setIPv4() conditional
optimizations. Using a copy requires adding an otherwise needless local
variable allocation.

>
>> ./ftp.cc- conn->local = ftpState->ctrl.conn->local;
>> ./ftp.cc- conn->local.port(0);
>> ./ftp.cc: conn->remote = ftpState->ctrl.conn->remote;
>> ./ftp.cc- conn->remote.port(port);
>
>> ./ftp.cc- conn->local = ftpState->ctrl.conn->local;
>> ./ftp.cc- conn->local.port(0);
>> ./ftp.cc: conn->remote = ipaddr;
>> ./ftp.cc- conn->remote.port(port);
>
>> ./dns_internal.cc- conn->local = Config.Addrs.udp_incoming;
>> ./dns_internal.cc-
>> ./dns_internal.cc: conn->remote = nameservers[nsv].S;
>
>

Okay. Adding setAddrs(local, remote) to trunk since it is pure scope creep.

>
>>> * 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.
>
> By "die", you mean respond with an HTTP 400 error, right?
>

Er, yes. And close TCP connection.

>
>> It seems clear enough to
>> me not to need new code for that type of config error case.
>
> I disagree because admins do not normally see 400 errors (until users
> start complaining). Most see log messages though. Please note that in a
> multi-port setup, it may not be obvious that something is misconfigured
> because there may be few or no 400 errors when Squid starts serving
> requests.
>
> There are two kinds of likely misconfigurations:
>
> 1) Client sends PROXY. Squid is not configured to receive one. We could
> handle this better, but I do not insist on adding code to detect and
> warn about such cases because the required code would be relatively
> complex, and because it would make it even easier to spam cache.log with
> error messages when there is no misconfiguration at all (unless we add
> even more code).
>
> 2) Client does not send PROXY. Squid is configured to require one. I
> think we should announce such cases in cache.log. There is already code
> to detect this problem. AFAICT, we just need to a logging line.
>

Added a 33,2 level display of the error message and connection details.
I think higher levels would potentially be too noisy. Terminating
connections on error is a routine part of PROXY.

If we need more complex debugging of this we should probably allocate a
debug section to it. But for now 33 seems fine.

Amos