Re: [PATCH] Support PROXY protocol

On 27/07/2014 6:18 a.m., Alex Rousskov wrote:
> On 07/25/2014 08:27 PM, Amos Jeffries wrote:
>
>> + // 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");
>> + }
>
> AFAICT, the above code declares an error on a valid PROXY header if
> Squid happens to read exactly Proxy20magic bytes first. I recommend
> removing the "optimization" from the first two if conditions, engaging
> the parser as soon as we get enough info to determine the protocol:
>
> {
> if (in.buf.startsWith(Proxy20magic))
> return parseProxy20();
>
> if (in.buf.startsWith(Proxy10magic))
> return parseProxy10();
>
> if (in.buf.length() >= Proxy20magic.length()) {
> // 1.0 magic is shorter, so we know that
> // the input does not start with any PROXY magic
> return proxyProtocolError("PROXY protocol error:...");
> }
>
> // need more data
> }
>
> The above avoids checking length twice (in common cases) and also places
> more common(?) case of a 2.0 protocol version first, so it should work
> faster than the patch code, on average :-).

Done.

>> +<p>Squid currently supports receiving HTTP traffic from a client proxy using this protocol.
>> + An http_port which has been configured to receive this protocol may only be used to
>> + receive traffic from client software sending in this protocol.
>> + Regular forward-proxy HTTP traffic is not accepted.
>
> I suggest rephrasing the last sentence to avoid a misinterpretation that
> the port cannot be used for forward-proxy traffic at all. Something
> along these lines:
>
> ...
> + receive traffic from clients sending the PROXY protocol header.
> + HTTP traffic without the PROXY header is not accepted on such a port.
>

Done.

>> +/// parse the PROXY/2.0 protocol header from the connection read buffer
>> +bool
>> +ConnStateData::parseProxy20()
>
> Consider using parseProxy2p0() and similar names in case the PROXY
> protocol goes to version ten :-)
>

Shrug. Done. The '0' is an addition by me to match standard protocols
version ABNF.

>
>> On 15/07/2014 4:25 a.m., Alex Rousskov wrote:
>>> On 07/12/2014 10:45 PM, Amos Jeffries wrote:
>>>>>> -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.
>
>
> Any news on the renaming front? Does it make sense for us to wait for a
> better protocol name or is it hopeless?

Still might happen at RFC creation time, but that is not in sight. So
hopeless to wait.

So, I've branched that discussion in the other half of this thread.

>
>>> 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.
>
>> + debugs(3,DBG_CRITICAL, "FATAL: https_port: proxy-surrogate option cannot be used on HTTPS ports.");
>
> Please s/cannot be used/is not supported/ to emphasize that we just lack
> the code necessary to make it possible (i.e., this is not some kind of
> fundamental protocol-level incompatibility or prohibition).
>

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?
>
>> When evaluating the security direct TCP is evaluated first, then PROXY,
>> then XFF entries.
>
>> 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.
>
> Sounds good. I think it is important to document this because XFF and
> the PROXY protocol relay similar information and the protocol does not
> document whether PROXY info is more or less trusted than XFF info. If I
> interpret your explanation correctly, trusted XFF info overwrites
> trusted PROXY info (but Squid will not even consider/see XFF if Squid
> does not trust the PROXY info first).
>

Well, PROXY overwrites the client TCP details (same way a NAT lookup
does). But XFF sets indirect_client variables which are kept separate
from TCP details in current Squid design so particular components can
have "foo_uses_indirect_client on/off" directives.
  Otherwise yes.

>>> Please adjust the documentation to make it clear whether the features
>>> are exclusive (either/or) or can co-exist (and/or).
>
> I do not think this has been done yet. Here is a specific suggestion
> which needs your validation and polishing and, depending on the final
> version, may prompt more changes:
>
> Replace
>
>> + The original source details can be relayed in:
>> + 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.
>
> with something along these lines:
>
> -------------
> The original source details may come from:
> * PROXY protocol connection header,
> * HTTP message Forwarded header, and
> * HTTP message X-Forwarded-For header.
>
> Depending on configuration and traffic, Squid may evaluate this
> directive right after accepting a TCP connection and/or when handling an
> HTTP request. Both cases are detailed below.
>
> For proxy-surrogate ports: An allow match is required for Squid to
> permit the corresponding HTTP connection, before Squid even looks at the
> HTTP request headers. If there is an allow match, Squid starts using
> PROXY header information to determine the source address of the
> connection for all future ACL checks. A deny match results in an HTTP
> 400 error response followed by TCP connection closure. Evaluation
> described in this paragraph does not happen on non proxy-surrogate ports.
>
> For HTTP headers containing Forwarded or X-Forwarded-For fields: An
> allow match is required for Squid to follow HTTP Forwarded or
> X-Forwarded-For values to find the original client information; found
> information overwrites the PROXY-supplied information, if any. If a
> Forwarded-For header is present, any X-Forwarded-For header is ignored.
> A deny match instructs Squid to ignore any Forwarded-For and
> X-Forwarded-For headers. Please note that for proxy-surrogate ports,
> Squid uses PROXY header information for the source address of the
> connection during the ACL evaluation described in this paragraph.
> -------------
>

Added that with some re-wording: Squid does not return 400 error on
PROXY protocol faailure (straight TCP FIN/RST is mandated), does not
follow the Forwarded header (yet), a deny match simply means stop
iterating and use the last values so far trusted, and XFF details are
stored on the HttpRequest not the Comm::Connection.

>>>> + 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)?
>
>> we use the [trusted] PROXY protocol header as per PROXY protocol.
>
> Does the PROXY protocol specify how the [trusted] destination
> information in the PROXY header is to be used? I did not see such
> information in the specs, but I could have easily missed them, so I have
> to ask.

It says to use it as the client IP for the connection. We do that by
replacing the connection-level IPs in Comm::Connection same as we do for
NAT/TPROXY.

>
>> Do we really need to enumerate whole chapters of the
>> protocol spec in the documentation of its config option?
>
> No. We need to document
>
> * what we do beyond what the protocol specifies (if anything) and
> * what the protocol specifies but we do not implement.
>

There is nothing beyond the protocol specs happening on http_port
traffic AFAIK and the option is omitted from the https_port
documentation where it is not supported.

>
>> 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.
>
> Agreed. For now though, the initial question is whether any additional
> description is needed. Just like with XFF, there is a potential conflict
> here. For example, PROXY says that the intercepted client was trying to
> get to address X. The HTTP header says we should go to domain Y. Does
> Squid validate that Y resolves to X? Does Squid try to connect to X?
> Similar questions have been asked about TPROXY (which has similar
> potential conflicts). If the PROXY specs do not clarify this, we should
> (the location of such clarifications is a different matter and may
> depend on what those clarifications are).

I dont think so for the http_port option. The must-know details specific
to Squid are for the access control directive documentation which is
referenced from there already.

>
>>> 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.
>
> Since this is a misconfiguration that (a) is likely to render a Squid
> port completely unusable and (b) cannot be detected without analyzing
> individual transactions, I think we should use debugging level 1,
> especially while PROXY is an experimental protocol/feature. Noise is
> better than silence in this case (though we are obviously selecting
> between two evils here -- we need to finish the "report occasionally" API).
>

Fine, making it level 1 by default with a build option to quieten the
output to every 1/32nd if needed.

Amos