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 :-).
> +<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.
> +/// 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 :-)
> 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?
>> 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).
>>> +<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.
I do not classify the above issues as "design" issues in this context,
but I suspect this classification is not worth arguing about and thank
you for making the release notes change.
> 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.
Yes, if you are right that the PROXY header is sent unencrypted (and
that does make a lot of sense to me given the simple "devices" PROXY
targets!), then the code will need to be changed to preserve and give
unused bytes to the decryption layer. This is similar to CONNECT
bumping, I guess.
>>> + 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).
> BTW, this confusion you seem to have between the two
AFAICT, there has been no confusion, just missing information. Not
knowing something and being confused about something one knows are two
different things. We can solve the former by documenting how things work
(unless the PROXY protocol documentation does that for us). Hopefully,
our documentation will not cause more confusion than the lack of
information :-).
> 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.
I agree that either renaming or adding a new option is desirable. For
the renaming path, we are just having trouble finding a good new name.
>> 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.
-------------
>>> + 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.
> 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.
> 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).
>> 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).
Thank you,
Alex.
