Re: [PATCH] Support PROXY protocol
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 19/08/2014 10:12 p.m., Amos Jeffries wrote: > Updated patch. I believe this covers everything so far, including > the 16-bit alignment and segmented TCP packet issues. > > Amos > If there are no objections I will apply this soon. Amos -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.22 (MingW32) iQEcBAEBAgAGBQJUAzXZAAoJELJo5wb/XPRjqDsH/j43Uf2L/esGDV+9XkZCC0ID 76h6XvhQusZc8jOqefb22dKM1kK9b2lQ5su94rjcsUTFkWwK9OJ1KYENvoZps58U ft+R7vd3fYGnfH6CJwDh33KRsyX+ZqCku53zumCNoG5TdP/FIE1HODyiV7u74lk1 rfcTssI5aG/f2x9gbOvTQURKbSemaiPSauWYIblRDteGp9knRiZSLcfbQFFMsioK R6010F+dH6VkmvxstW8yDQa/GXZbyw54LnQhRn697bfI5yOIj4jIQLWEz2dyVoyY ylOaTaM0AKV8CB61T9nV4PfBoveXaMvMqKNdCmoMUueEfPiPy1Exjbak9Rt7IOU= =1oHT -END PGP SIGNATURE-
Re: [PATCH] Support PROXY protocol
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 1/09/2014 2:48 a.m., Amos Jeffries wrote: > On 19/08/2014 10:12 p.m., Amos Jeffries wrote: >> Updated patch. I believe this covers everything so far, >> including the 16-bit alignment and segmented TCP packet issues. > >> Amos > > > If there are no objections I will apply this soon. > Merged to trunk as rev.1370 Amos -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.22 (MingW32) iQEcBAEBAgAGBQJUBcmJAAoJELJo5wb/XPRj37YH/ijvazrg7W1vEGx73+Iw6Qnw 5dhSajz2ZUOFYW6vDh2JO8fgtOsrNQ5gtCTCOHAB0Xq0AcPKoOjlP/Dv/+iPk9PV XVJNsDe/2T/4vXtnPGul6YLT3NxkNG30T/dWBfoc262vU0WcItlSR7K4aov/qLh6 6aib9PLO+yrkrAWfFUMI7rkzbJx91kukQMnMcA070PXrol8h0OJpDDjqHb1w293u nFWnX8fRMf5C0ngeWCLI4JxBFtHEc3Ka1mL0add6ncMpLFRirH/07tRxawxtKQLN BCu9JpNwASNnINDG0TmdzoTYdqrFUVHZUAb+yADDU8KBIpV1RiZh3gJDve0W1lQ= =r449 -END PGP SIGNATURE-
Re: [PATCH] Support PROXY protocol
I was not expecting this patch due to old emails about the proxy protocol implementation. I understand from the email that after this patch we can use STUNNEL and HAPROXY in-front of squid. right? +1 (for the idea and looked a bit at the code itself) Eliezer On 06/22/2014 08:15 AM, Amos Jeffries wrote: Support receiving PROXY protocol version 1 and 2. PROXY protocol has been developed by Willy Tarreau of HAProxy for communicating original src and dst IP:port details between proxies and load balancers in a protocol-agnostic way. stunnel, HAProxy and some other HTTP proxying software are already enabled and by adding support to Squid we can effectively chain these proxies without having to rely on X-Forwarded-For headers. This patch adds http(s)_port mode flag (proxy-surrogate) to signal the protocol is in use, parsing and processing logics for the PROXY protocol headers on new connections, and extends the follow_x_forwarded_for (renamed proxy_forwarded_access) access control to manage inbound connections. The indirect client security/trust model remains unchanged. As do all HTTP related logics on the connection once PROXY protocol header has been received. Furture Work: * support sending PROXY protocol to cache_peers * rework the PROXY parse logics as a Parser-NG child parser. Amos
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 fals
Re: [PATCH] Support PROXY protocol
On 26/06/2014 4:53 a.m., Eliezer Croitoru wrote: > I was not expecting this patch due to old emails about the proxy > protocol implementation. > I understand from the email that after this patch we can use STUNNEL and > HAPROXY in-front of squid. right? Right. stunnel, HAProxy and any other gateway software which supports sending the protocol. I was also not expecting it to happen for a version for two either, but Willy and I got talking about it the other day and when I looked closer the work already done on the parser and client-side cleanup happens to be enough to make it quite a relatively clean and simple addition. Amos > +1 (for the idea and looked a bit at the code itself) > > Eliezer > > On 06/22/2014 08:15 AM, Amos Jeffries wrote: >> Support receiving PROXY protocol version 1 and 2. >> >> PROXY protocol has been developed by Willy Tarreau of HAProxy for >> communicating original src and dst IP:port details between proxies and >> load balancers in a protocol-agnostic way. >> >> stunnel, HAProxy and some other HTTP proxying software are already >> enabled and by adding support to Squid we can effectively chain these >> proxies without having to rely on X-Forwarded-For headers. >> >> This patch adds http(s)_port mode flag (proxy-surrogate) to signal the >> protocol is in use, parsing and processing logics for the PROXY protocol >> headers on new connections, and extends the follow_x_forwarded_for >> (renamed proxy_forwarded_access) access control to manage inbound >> connections. >> The indirect client security/trust model remains unchanged. As do all >> HTTP related logics on the connection once PROXY protocol header has >> been received. >> >> >> Furture Work: >> * support sending PROXY protocol to cache_peers >> * rework the PROXY parse logics as a Parser-NG child parser. >> >> Amos >
Re: [PATCH] Support PROXY protocol
On 06/25/2014 01:41 PM, Alex Rousskov wrote: > On 06/21/2014 11:15 PM, Amos Jeffries wrote: >> > Support receiving PROXY protocol version 1 and 2. > 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 Per Amos request on IRC, here are some suggestions for renaming the new protocol itself (in case the protocol author, Willy Tarreau, might be open to suggestions): * On Behalf Of Protocol * True Client Information Disclosure Protocol * TCP Client Information Disclosure Protocol * The Right To Be Remembered Protocol * Remember The Right Client Protocol * From and To Protocol * Actually From and To Protocol * Letterhead Protocol * True Client Letterhead Protocol * TCP Client Letterhead Protocol * Top Line Protocol * Top Header Protocol * Top Shelf Protocol Most can be creatively "acronymized" for brevity, of course. I have not validated any of these suggestions against prior art and cultural clashes (although some puns were intentional). The protocol renaming can be combined with renaming the PROXY magic word in the top line as well, but they do not have to. HTH, Alex.
Re: [PATCH] Support PROXY protocol
cc'ing Willy so he can get in on this. On 11/07/2014 5:03 p.m., Alex Rousskov wrote: > On 06/25/2014 01:41 PM, Alex Rousskov wrote: >> On 06/21/2014 11:15 PM, Amos Jeffries wrote: Support receiving PROXY protocol version 1 and 2. > >> 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 > > > Per Amos request on IRC, here are some suggestions for renaming the new > protocol itself (in case the protocol author, Willy Tarreau, might be > open to suggestions): > > * On Behalf Of Protocol > > * True Client Information Disclosure Protocol - truth remains unknown despite ths protocol. > * TCP Client Information Disclosure Protocol - supports non-TCP protocols. > > * The Right To Be Remembered Protocol > * Remember The Right Client Protocol > > * From and To Protocol > * Actually From and To Protocol - security section says it could be full of lies. So the A is incorrect. > > * Letterhead Protocol > * True Client Letterhead Protocol - ditto on the truth issue. > * TCP Client Letterhead Protocol - ditto on the TCP issue. > > * Top Line Protocol > * Top Header Protocol > * Top Shelf Protocol - taken. I was thinking you had something funny along the lines of: * Traffic Envelope Annex protocol (TEA p'ot) We could also reply with HTTP 418 and close the connection on protocol failures. The semantics actually fits the dictionary definitions of the words too :-) Amos > > > Most can be creatively "acronymized" for brevity, of course. > > I have not validated any of these suggestions against prior art and > cultural clashes (although some puns were intentional). > > > The protocol renaming can be combined with renaming the PROXY magic word > in the top line as well, but they do not have to. > > > HTH, > > Alex. >
Re: [PATCH] Support PROXY protocol
> I was thinking you had something funny along the lines of: > > * Traffic Envelope Annex protocol (TEA p'ot) > > We could also reply with HTTP 418 and close the connection on protocol > failures. iLike. Willy, what do you think? Kinkie
Re: [PATCH] Support PROXY protocol
On 07/11/2014 02:27 AM, Amos Jeffries wrote: > - supports non-TCP protocols. > - security section says it could be full of lies. So the A[ctually] is > incorrect. IMHO, you are being too literate with the words in the protocol name while being very permissive with the protocol specs. Most of the ones you rejected can be used without harm IMHO. > I was thinking you had something funny along the lines of: > > * Traffic Envelope Annex protocol (TEA p'ot) I did not have anything like that in mind. Personally, I would not call it "envelop" because the protocol does not envelop the message, it only provides a prefix, header, or "top line". This is why I suggested "letterhead" rather than "envelop". Other related variants are: * Client Letterhead Protocol * Client Annex Protocol Pretty much anything would be better than PROXY though :-). Cheers, Alex.
Re: [PATCH] Support PROXY protocol
On 12/07/2014 3:04 a.m., Alex Rousskov wrote: > On 07/11/2014 02:27 AM, Amos Jeffries wrote: >> - supports non-TCP protocols. >> - security section says it could be full of lies. So the A[ctually] is >> incorrect. > > IMHO, you are being too literate with the words in the protocol name > while being very permissive with the protocol specs. Most of the ones > you rejected can be used without harm IMHO. > > >> I was thinking you had something funny along the lines of: >> >> * Traffic Envelope Annex protocol (TEA p'ot) > > I did not have anything like that in mind. Personally, I would not call > it "envelop" because the protocol does not envelop the message, it only > provides a prefix, header, or "top line". This is why I suggested > "letterhead" rather than "envelop". It is not message (Layer 3) related in the slightest. This is an Layer 2.1 envelope for the entire connection for delivery between softwares. Below even TLS (Layer 2.5). Amos > > Other related variants are: > > * Client Letterhead Protocol > * Client Annex Protocol > > Pretty much anything would be better than PROXY though :-). > > > Cheers, > > Alex. >
Re: [PATCH] Support PROXY protocol
Attaced patch contains all updated except the config option renaming,
which remains to be sorted out.
On 26/06/2014 7:41 a.m., Alex Rousskov wrote:
> 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?
We are a surrogate for the PROXY protocol. I know its a bit nasty.
> 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.
>
In all seriousness "haproxy-protocol" is probably the most correct
descriptive right now. But I am trying hard to avoid naming a competitor
in our most visible documentations.
"forwarded" would be wonderful, but I can forsee some confusion with
forward-proxy mode.
How about referred, referral, or proxy-referral ?
>
>> -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 ?
The XFF header has been deprecated by the Forwarded: header now, so a
name-changing was in the cards whatever this patch does.
>
>> +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.
>
Done. And calling mustStop() now.
>
>> +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. I get the point
though, splitting into three functions. The main one being
findProxyProtocolMagic().
>
>> +
>> +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.
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).
I have done as suggested and created the AsyncJob::Start() functionality
for it.
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.
>
>> +// 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?
>
No reason. Done.
>
>> +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.
>
Done.
>
>> +// 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(po
Re: [PATCH] Support PROXY protocol
> In all seriousness "haproxy-protocol" is probably the most correct > descriptive right now. But I am trying hard to avoid naming a competitor > in our most visible documentations. How so? We are not a commercial product; I have no issues with naming a competitor.
Re: [PATCH] Support PROXY protocol
On 07/13/2014 02:38 PM, Kinkie wrote: >> In all seriousness "haproxy-protocol" is probably the most correct >> descriptive right now. But I am trying hard to avoid naming a competitor >> in our most visible documentations. > > How so? We are not a commercial product; I have no issues with naming > a competitor. I do not know what you mean by "not a commercial product" exactly, but there is competition among all proxy products. Being an open source project does not position us above (or below) the competition with other open- and closed-source products. We may decide to "advertise" a competitor (of any kind), but using GPL does not really make the correct decision any easier IMO. We should name haproxy if naming haproxy brings Squid more good than harm. In the particular case of this configuration option, the harm I see comes not from competition with haproxy, but from implying that the protocol is somehow specific to haproxy and from having to rename the option later if the PROXY protocol is renamed or standardized (I hope IETF does not standardize the PROXY name for a protocol, but worst things have happened). If we have to pick between "proxy-protocol" and "haproxy-protocol", I would probably vote for "haproxy-protocol", but both options are pretty bad for various reasons. We can also use "willy-tarreau-proxy-protocol". Cheers, Alex.
Re: [PATCH] Support PROXY protocol
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.
> +// 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.
>>> -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.
>>> +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/TL
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
Re: [PATCH] Support PROXY protocol
On 22/06/2014 5:15 p.m., Amos Jeffries wrote: > Support receiving PROXY protocol version 1 and 2. > > PROXY protocol has been developed by Willy Tarreau of HAProxy for > communicating original src and dst IP:port details between proxies and > load balancers in a protocol-agnostic way. > > stunnel, HAProxy and some other HTTP proxying software are already > enabled and by adding support to Squid we can effectively chain these > proxies without having to rely on X-Forwarded-For headers. > > This patch adds http(s)_port mode flag (proxy-surrogate) to signal the > protocol is in use, parsing and processing logics for the PROXY protocol > headers on new connections, and extends the follow_x_forwarded_for > (renamed proxy_forwarded_access) access control to manage inbound > connections. > The indirect client security/trust model remains unchanged. As do all > HTTP related logics on the connection once PROXY protocol header has > been received. > > > Furture Work: > * support sending PROXY protocol to cache_peers > * rework the PROXY parse logics as a Parser-NG child parser. > > Amos > So on the table the question of the http_port option name (and derived from that the *_access control name). The contenders so far: proxy surrogate [1] proxy-surrogate [1] require-PROXY expect-PROXY [2] require-PROXY-header expect-PROXY-header [2] forwarded [3] proxy-forwarded [3] haproxy-protocol[4] indirect-client [1] potential naming confusion with Surrogate protocol HTTP extension. And Alex objects that it means "nothing" in this squid context. [2] potential naming confusion with "explicit proxy" terminology [3] potential naming confusion with "forward proxy" terminology [4] free advertising for the competition At this stage it looks like Alexs' "require-proxy-header" is front runner for relevance. Probably with "indirect_client" for the access control. Does anyone else have optin names or even just words to throw into the mix? Amos
Re: [PATCH] Support PROXY protocol
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 :-).
> +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).
>>> +Known Issue: Due to design issues HTTPS traffic is not yet
>>> accepted
>>> + over this protocol. So use of proxy-surrogate on
>>> https_port
>>> + 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 ConnS
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.
>> +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, the
Re: [PATCH] Support PROXY protocol
On 07/30/2014 09:02 AM, Amos Jeffries wrote: > +NAME: proxy_forwarded_access follow_x_forwarded_for > Requests may pass through a chain of several other proxies > + before reaching us. The original source details may by sent in: > + * HTTP message Forwarded header, or > + * HTTP message X-Forwarded-For header, or > + * PROXY protocol connection header. ... > + For proxy-surrogate ports an allow match is required for Squid to > + permit the corresponding TCP connection, before Squid even looks for > + 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. > + On each HTTP request Squid checks for X-Forwarded-For header fields. Given the "first evaluate proxy_forwarded_access for connection, then evaluate proxy_forwarded_access for each HTTP request header" functionality described above I wonder whether it is a good idea to merge both evaluations into one directive. Would not it be easier for admin to write correct ACL rules if we keep them separate? For example, let's assume an admin wants to trust a PROXY client device at (and only at) address A and XFF-setting child proxy at (and only at) address B. If we split the functionality into proxy_forwarded_access and follow_x_forwarded_for, then the admin can do: proxy_forwarded_access allow A #proxy_forwarded_access deny all follow_x_forwarded_for allow B #follow_x_forwarded_for deny B Right? What about the merged implementation proposed in the patch? How can the admin express the above logic? AFAICT, the following will _not_ work: proxy_forwarded_access allow A proxy_forwarded_access allow B #proxy_forwarded_access deny all because it will allow PROXY clients at B and XFF setting by A. Thank you, Alex.
Re: [PATCH] Support PROXY protocol
On 5/08/2014 2:47 a.m., Alex Rousskov wrote: > On 07/30/2014 09:02 AM, Amos Jeffries wrote: > >> +NAME: proxy_forwarded_access follow_x_forwarded_for > >> Requests may pass through a chain of several other proxies >> +before reaching us. The original source details may by sent in: >> +* HTTP message Forwarded header, or >> +* HTTP message X-Forwarded-For header, or >> +* PROXY protocol connection header. > > ... > >> +For proxy-surrogate ports an allow match is required for Squid to >> +permit the corresponding TCP connection, before Squid even looks for >> +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. > >> +On each HTTP request Squid checks for X-Forwarded-For header fields. > > > Given the "first evaluate proxy_forwarded_access for connection, then > evaluate proxy_forwarded_access for each HTTP request header" > functionality described above I wonder whether it is a good idea to > merge both evaluations into one directive. Would not it be easier for > admin to write correct ACL rules if we keep them separate? > > For example, let's assume an admin wants to trust a PROXY client device > at (and only at) address A and XFF-setting child proxy at (and only at) > address B. If we split the functionality into proxy_forwarded_access and > follow_x_forwarded_for, then the admin can do: > > proxy_forwarded_access allow A > #proxy_forwarded_access deny all > > follow_x_forwarded_for allow B > #follow_x_forwarded_for deny B > > Right? What about the merged implementation proposed in the patch? How > can the admin express the above logic? AFAICT, the following will _not_ > work: > > proxy_forwarded_access allow A > proxy_forwarded_access allow B > #proxy_forwarded_access deny all > > because it will allow PROXY clients at B and XFF setting by A. > Good point. I have been considering these lookups as stateless. However, XFF does offer the unusual property that regex can check for specific ordering of IPs. We do not have that with PROXY+XFF. I am adding proxy_protocol_access as the first access control, reverting follow_x_forwarded_for for the second. But retaining the new description texts. Also for lack of anything better in the last week I am using require-proxy-header for the port option. New patch attached. Amos === modified file 'doc/release-notes/release-3.5.sgml' --- doc/release-notes/release-3.5.sgml 2014-08-02 14:03:21 + +++ doc/release-notes/release-3.5.sgml 2014-08-05 15:23:59 + @@ -26,40 +26,41 @@ Known issues Although this release is deemed good enough for use in many setups, please note the existence of http://bugs.squid-cache.org/buglist.cgi?query_format=advanced&product=Squid&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&version=3.5"; name="open bugs against Squid-3.5">. Changes since earlier releases of Squid-3.5 The 3.5 change history can be http://www.squid-cache.org/Versions/v3/3.5/changesets/"; name="viewed here">. Major new features since Squid-3.4 Squid 3.5 represents a new feature release above 3.4. The most important of these new features are: Support libecap v1.0 Authentication helper query extensions Support named services Upgraded squidclient tool Helper support for concurrency channels + Receive PROXY protocol, Versions 1 & 2 Most user-facing changes are reflected in squid.conf (see below). Support libecap v1.0 Details at http://wiki.squid-cache.org/Features/BLAH";>. The new libecap version allows Squid to better check the version of the eCAP adapter being loaded as well as the version of the eCAP library being used. Squid-3.5 can support eCAP adapters built with libecap v1.0, but no longer supports adapters built with earlier libecap versions due to API changes. Authentication helper query extensions Details at http://www.squid-cache.org/Doc/config/auth_param/";>. @@ -146,71 +147,111 @@ The default is to use X.509 certificate encryption instead. When performing TLS/SSL server certificates are always verified, the results shown at debug level 3. The encrypted type is displayed at debug level 2 and the connection is used to send and receive the messages regardless of verification results. Helper support for concurrency channels Helper concurrency greatly reduces the communication lag between Squid and its helpers allowing faster transaction speeds even on sequential helpers. The Digest authentication, Store-ID, and URL-rewrite helpers packaged with Squid have been updated to support concurrency channels. They will auto-detect the channel-ID field and will produce the appropriate response format. With these helpers concurrency may now be set to 0 or any hi
Re: [PATCH] Support PROXY protocol
On 08/05/2014 08:31 PM, Amos Jeffries wrote:
> I am adding proxy_protocol_access as the first access control, reverting
> follow_x_forwarded_for for the second.
Great. I think this is a much simpler/cleaner design.
> +} else if (strcmp(token, "require-proxy-header") == 0) {
> +s->flags.proxySurrogate = true;
> +debugs(3, DBG_IMPORTANT, "Disabling TPROXY Spoofing on port " <<
> s->s << " (require-proxy-header enabled)");
> +
The IMPORTANT message should probably be printed only if TPROXY spoofing
on that port was actually configured/requested.
And, at that point, I would actually make the apparently illegal
combination a fatal misconfiguration error, but I suspect you do not
like that approach, and I will not insist on it.
> But retaining the new description texts.
> -DEFAULT_DOC: X-Forwarded-For header will be ignored.
> +DEFAULT_DOC: indirect client IP will not be accepted.
The old documentation line was much more clear IMO. If it was incorrect,
can you rephrase the correction please? Perhaps you meant to say
something like "Any client IP specified in X-Forwarded-For header will
be ignored"? If yes, the current version still sounds better to me.
Adding Forwarded header to those description is a good idea if correct,
of course.
> +DEFAULT_DOC: all TCP connections will be denied
I would clarify that with either
* "all TCP connections to ports with require-proxy-header will be denied" or
* "proxy_protocol_access deny all"
> + Any host for which we accept client IP details can place
> + incorrect information in the relevant header, and Squid
I would s/for/from/ but perhaps I misunderstand how this works.
> +tok.skip(Proxy1p0magic);
We already know the magic is there. If you want to optimize this, then
skip() in ConnStateData::parseProxyProtocolHeader() and pass the
Tokenizer object to the version-specific parsers. I do not insist on
this change, but you may want to add a corresponding TODO if you do not
want to optimize this.
> +if (!tok.prefix(tcpVersion, CharacterSet::ALPHA+CharacterSet::DIGIT))
That dynamic creation, addition, and destruction of a 256-element vector
is a significant performance penalty. Please create a static version of
that alphanumeric(?) CharacterSet instead of computing it at least once
for every PROXY connection.
> +const CharacterSet ipChars = CharacterSet("IP Address",".:") +
> CharacterSet::HEXDIG;
Same here, made worse by adding creation of an intermediate set.
Please check for other occurrences.
> +if (!tok.prefix(tcpVersion, CharacterSet::ALPHA+CharacterSet::DIGIT))
> +return proxyProtocolError(tok.atEnd()?"PROXY/1.0 error: invalid
> protocol family":NULL);
I recommend removing that code and using
if (tok.skip("UNKNOWN")) { ... your UNKNOWN code here ...}
else if (tok.skip("TCP")) { ... your TCP code here ...}
instead.
> +if (!tcpVersion.cmp("UNKNOWN")) {
> +} else if (!tcpVersion.cmp("TCP",3)) {
Please test for the more likely/common condition first.
> +if (!tcpVersion.cmp("UNKNOWN")) {
> +} else if (!tcpVersion.cmp("TCP",3)) {
Please use the length argument consistently if this code stays.
> +// skip to first LF (assumes it is part of CRLF)
> +const SBuf::size_type pos = in.buf.findFirstOf(CharacterSet::LF);
> +if (pos != SBuf::npos) {
> +if (in.buf[pos-1] != '\r')
It would be better to use Tokenizer for this IMO. If you insist on
manual parsing, consider at least skipping the magic header in the
findFirstOf() call.
> + return proxyProtocolError(tok.atEnd()?"PROXY/1.0 error: missing SP":NULL);
Please surround the ? and : parts of the operator with spaces for
readability, especially since the string itself contains ':'.
> +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'))
This code would be a lot clearer if you rewrite it in a positive way:
const bool parsedAll = tok.prefix() && tok.skip() && ...
if (!parsedAll)
...
I do not insist on this change.
> +// 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()?"PROXY/1.0 error: invalid
> syntax":NULL);
AFAICT, atEnd() may be false just because we have not received the whole
PROXY header yet, not because we received something invalid. For
example, int64() returns false not just on failures but on "insufficient
data" (as it should!).
[N.B. prefix() should behave similarly on insufficient data, but it is
currently broken; I can fix after the
Re: [PATCH] Support PROXY protocol
On 11/08/2014 4:32 p.m., Alex Rousskov wrote:
> On 08/05/2014 08:31 PM, Amos Jeffries wrote:
>
>> I am adding proxy_protocol_access as the first access control, reverting
>> follow_x_forwarded_for for the second.
>
> Great. I think this is a much simpler/cleaner design.
>
>
>> +} else if (strcmp(token, "require-proxy-header") == 0) {
>> +s->flags.proxySurrogate = true;
>> +debugs(3, DBG_IMPORTANT, "Disabling TPROXY Spoofing on port " <<
>> s->s << " (require-proxy-header enabled)");
>> +
>
> The IMPORTANT message should probably be printed only if TPROXY spoofing
> on that port was actually configured/requested.
>
> And, at that point, I would actually make the apparently illegal
> combination a fatal misconfiguration error, but I suspect you do not
> like that approach, and I will not insist on it.
>
Done.
>> But retaining the new description texts.
>
>> -DEFAULT_DOC: X-Forwarded-For header will be ignored.
>> +DEFAULT_DOC: indirect client IP will not be accepted.
>
> The old documentation line was much more clear IMO. If it was incorrect,
> can you rephrase the correction please? Perhaps you meant to say
> something like "Any client IP specified in X-Forwarded-For header will
> be ignored"? If yes, the current version still sounds better to me.
> Adding Forwarded header to those description is a good idea if correct,
> of course.
>
Done.
>> +DEFAULT_DOC: all TCP connections will be denied
>
> I would clarify that with either
>
> * "all TCP connections to ports with require-proxy-header will be denied" or
>
> * "proxy_protocol_access deny all"
>
Done
>> +Any host for which we accept client IP details can place
>> +incorrect information in the relevant header, and Squid
>
> I would s/for/from/ but perhaps I misunderstand how this works.
>
>
>> +tok.skip(Proxy1p0magic);
>
> We already know the magic is there. If you want to optimize this, then
> skip() in ConnStateData::parseProxyProtocolHeader() and pass the
> Tokenizer object to the version-specific parsers. I do not insist on
> this change, but you may want to add a corresponding TODO if you do not
> want to optimize this.
>
This is done with the parser method Tokenizer so that if we happen not
to receive the whole header in one read for any reason we can easily
undo and retry on the following read(2).
Only if the Tokenizer parses completely and fully is the I/O buffer
updated.
We could generate a temporary SBuf and setup the Tokenizer with that.
But it's simpler just to setup the Tokenizer with the existing buffer
and unconditionally skip the magic we already know exists.
>
>> +if (!tok.prefix(tcpVersion, CharacterSet::ALPHA+CharacterSet::DIGIT))
>
> That dynamic creation, addition, and destruction of a 256-element vector
> is a significant performance penalty. Please create a static version of
> that alphanumeric(?) CharacterSet instead of computing it at least once
> for every PROXY connection.
>
>
>> +const CharacterSet ipChars = CharacterSet("IP Address",".:") +
>> CharacterSet::HEXDIG;
>
> Same here, made worse by adding creation of an intermediate set.
>
> Please check for other occurrences.
Fixed, and checked.
>> +if (!tok.prefix(tcpVersion, CharacterSet::ALPHA+CharacterSet::DIGIT))
>> +return proxyProtocolError(tok.atEnd()?"PROXY/1.0 error: invalid
>> protocol family":NULL);
>
> I recommend removing that code and using
>
> if (tok.skip("UNKNOWN")) { ... your UNKNOWN code here ...}
> else if (tok.skip("TCP")) { ... your TCP code here ...}
>
> instead.
>
>
>> +if (!tcpVersion.cmp("UNKNOWN")) {
>> +} else if (!tcpVersion.cmp("TCP",3)) {
>
> Please test for the more likely/common condition first.
>
Agreed. Done.
>> +// skip to first LF (assumes it is part of CRLF)
>> +const SBuf::size_type pos = in.buf.findFirstOf(CharacterSet::LF);
>> +if (pos != SBuf::npos) {
>> +if (in.buf[pos-1] != '\r')
>
> It would be better to use Tokenizer for this IMO. If you insist on
> manual parsing, consider at least skipping the magic header in the
> findFirstOf() call.
>
Waiting on the FTP change that makes it easy to settle in trunk.
>
>> + return proxyProtocolError(tok.atEnd()?"PROXY/1.0 error: missing SP":NULL);
>
> Please surround the ? and : parts of the operator with spaces for
> readability, especially since the string itself contains ':'.
>
Done.
>
>> +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'))
>
> This code would be a lot clearer if you rewrite it in a positive way:
>
> const bool parsedAll = tok.prefix() && tok.skip() && ...
> if (!parsedAll)
> ...
>
> I do not insist on this change.
>
Makes some sense. Done.
>
>> +// parse src-IP SP dst-IP SP src-
Re: [PATCH] Support PROXY protocol
On 08/12/2014 10:17 AM, Amos Jeffries wrote:
> On 11/08/2014 4:32 p.m., Alex Rousskov wrote:
>> On 08/05/2014 08:31 PM, Amos Jeffries wrote:
>>> +tok.skip(Proxy1p0magic);
>>
>> We already know the magic is there. If you want to optimize this, then
>> skip() in ConnStateData::parseProxyProtocolHeader() and pass the
>> Tokenizer object to the version-specific parsers. I do not insist on
>> this change, but you may want to add a corresponding TODO if you do not
>> want to optimize this.
>>
>
> This is done with the parser method Tokenizer so that if we happen not
> to receive the whole header in one read for any reason we can easily
> undo and retry on the following read(2).
> Only if the Tokenizer parses completely and fully is the I/O buffer
> updated.
I know.
> We could generate a temporary SBuf and setup the Tokenizer with that.
> But it's simpler just to setup the Tokenizer with the existing buffer
> and unconditionally skip the magic we already know exists.
No temporary SBuf is needed AFAICT. You just _move_ the Tokenizer
creation into ConnStateData::parseProxyProtocolHeader() where you can
skip() the magic instead of doing startsWith() tests. Any buffer updates
happen when and where they happen today.
This is just an optional optimization, nothing more, of course. Not
required (although it would have helped avoid the missing skip() bug).
>>> +// 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()?"PROXY/1.0 error:
>>> invalid syntax":NULL);
>>
>> AFAICT, atEnd() may be false just because we have not received the whole
>> PROXY header yet, not because we received something invalid. For
>> example, int64() returns false not just on failures but on "insufficient
>> data" (as it should!).
>>
>
> I am not worried about those failure cases particularly, allowing for
> sender emitting one field per packet is a tolerance optimization to
> begin with. The entire header is designed to be sent in one packet and
> the spec explicitly requires it to be sent in a single write(2).
The PROXY RFC says "A receiver may reject an incomplete line which does
not contain the CRLF sequence in the first atomic read operation". If
this ever gets to the IETF review, I hope this requirement will be
stricken down. An "atomic read" is a bogus concept in this context; the
whole requirement violates the spirit of a TCP-compatible protocol and
will lead to interoperability problems.
It does not matter what the sender does because a lot of things can
happen to a packet after the write(2) call. IMO, we MUST accept any
valid PROXY header regardless of the packet and I/O segmentation at
lower levels. Fortunately, it is not very difficult to do in Squid.
>> [N.B. prefix() should behave similarly on insufficient data, but it is
>> currently broken; I can fix after the FTP Relay project is done.]
That Tokenizer::prefix() fix is also in trunk now.
>>> +if ((in.buf[0] & 0xF0) != 0x20) // version == 2 is mandatory
>>> +return proxyProtocolError("PROXY/2.0 error: invalid version");
>>> +
>>> +const char command = (in.buf[0] & 0x0F);
>>> +if ((command & 0xFE) != 0x00) // values other than 0x0-0x1 are invalid
>>> +return proxyProtocolError("PROXY/2.0 error: invalid command");
>>
>> These and similar v2 checks do not make sense to me because we know for
>> sure that in.buf starts with Proxy2p0magic. Why test parts of a
>> hard-coded constant? I am guessing you meant to skip magic before these
>> tests...
>>
>> Which forces me to ask whether the PROXY v2 path was tested recently?
>
> Willy reported it working before the audit,
Strange. I do not see how that code could have worked, but that is not
important, I guess. The RFC already claims Squid support! :-)
>>> +const char *clen = in.buf.rawContent() + Proxy2p0magic.length() + 2;
>>> +const uint16_t len = ntohs(*(reinterpret_cast>> *>(clen)));
>>
>> Could this cast violate integer alignment rules on some platforms? If
>> yes, we may have to memcpy those two bytes first. I do not think we can
>> guarantee any specific alignment for in.buf.rawContent() and, hence, we
>> cannot guarantee any specific alignment for clen.
>>
>
> Unsure. The uint16_t should be compiled as 8-bit aligned AFAIK. In
> theory we only encounter that type of problem with the more complex
> types (like pax below).
>
> I dont like casting anyway, so if you have any better way to do it
> without a double data copy I am interested. Note that ntohs() is
> effectively one data copy.
Your call, but I would memcpy() before interpreting that clen value to
be on the safe side. It is a very fast copy. If you do not memcpy(),
please add a comment. For example:
/
Re: [PATCH] Support PROXY protocol
On 08/12/2014 06:18 PM, Alex Rousskov wrote: > On 08/12/2014 10:17 AM, Amos Jeffries wrote: >> On 11/08/2014 4:32 p.m., Alex Rousskov wrote: >>> On 08/05/2014 08:31 PM, Amos Jeffries wrote: +const char *clen = in.buf.rawContent() + Proxy2p0magic.length() + 2; +const uint16_t len = ntohs(*(reinterpret_cast>>> *>(clen))); >>> Could this cast violate integer alignment rules on some platforms? If >>> yes, we may have to memcpy those two bytes first. I do not think we can >>> guarantee any specific alignment for in.buf.rawContent() and, hence, we >>> cannot guarantee any specific alignment for clen. >> Unsure. The uint16_t should be compiled as 8-bit aligned AFAIK. In >> theory we only encounter that type of problem with the more complex >> types (like pax below). > Your call, but I would memcpy() before interpreting that clen value to > be on the safe side. It is a very fast copy. If you do not memcpy(), > please add a comment. For example: > > // We hope uint16_t can start at any byte, w/o alignment problems. FWIW, the following page says "uint16_t* requires 2-byte alignment" at least for some ARM processors, but I have not dug deep enough to verify whether their claim is applicable to our code. http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka15414.html Given the above, I am even more pessimistic that we can safely get away with unaligned uint16_t access in all environments. HTH, Alex.
Re: [PATCH] Support PROXY protocol
Updated patch. I believe this covers everything so far, including the
16-bit alignment and segmented TCP packet issues.
Amos
=== modified file 'doc/release-notes/release-3.5.sgml'
--- doc/release-notes/release-3.5.sgml 2014-08-11 16:09:06 +
+++ doc/release-notes/release-3.5.sgml 2014-08-12 14:53:59 +
@@ -27,40 +27,41 @@
Although this release is deemed good enough for use in many setups, please
note the existence of
http://bugs.squid-cache.org/buglist.cgi?query_format=advanced&product=Squid&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&version=3.5";
name="open bugs against Squid-3.5">.
Changes since earlier releases of Squid-3.5
The 3.5 change history can be http://www.squid-cache.org/Versions/v3/3.5/changesets/"; name="viewed
here">.
Major new features since Squid-3.4
Squid 3.5 represents a new feature release above 3.4.
The most important of these new features are:
Support libecap v1.0
Authentication helper query extensions
Support named services
Upgraded squidclient tool
Helper support for concurrency channels
Native FTP Relay
+ Receive PROXY protocol, Versions 1 & 2
Most user-facing changes are reflected in squid.conf (see below).
Support libecap v1.0
Details at http://wiki.squid-cache.org/Features/BLAH";>.
The new libecap version allows Squid to better check the version of
the eCAP adapter being loaded as well as the version of the eCAP library
being used.
Squid-3.5 can support eCAP adapters built with libecap v1.0,
but no longer supports adapters built with earlier libecap versions
due to API changes.
Authentication helper query extensions
Details at http://www.squid-cache.org/Doc/config/auth_param/";>.
@@ -188,72 +189,111 @@
FTP Relay highlights:
Added ftp_port directive telling Squid to relay native FTP commands.
Active and passive FTP support on the user-facing side; require
passive connections to come from the control connection source IP
address.
IPv6 support (EPSV and, on the user-facing side, EPRT).
Intelligent adaptation of relayed FTP FEAT responses.
Relaying of multi-line FTP control responses using various formats.
Support relaying of FTP MLSD and MLST commands (RFC 3659).
Several Microsoft FTP server compatibility features.
ICAP/eCAP support (at individual FTP command/response level).
Optional "current FTP directory" tracking with the assistance of
injected (by Squid) PWD commands (cannot be 100% reliable due to
symbolic links and such, but is helpful in some common use cases).
No caching support -- no reliable Request URIs for that (see above).
+Receive PROXY protocol, Versions 1 & 2
+More info at http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt";>
+
+PROXY protocol provides a simple way for proxies and tunnels of any kind to
+ relay the original client source details without having to alter or
understand
+ the protocol being relayed on the connection.
+
+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.
+ HTTP traffic without the PROXY header is not accepted on such a port.
+
+The accel and intercept options are still used to
identify the
+ traffic syntax being delivered by the client proxy.
+
+Squid can be configured by adding an http_port
+ with the require-proxy-header mode flag. The
proxy_protocol_access
+ must also be configured with src ACLs to whitelist proxies which
are
+ trusted to send correct client details.
+
+Forward-proxy traffic from a client proxy:
+
+ http_port 3128 require-proxy-header
+ proxy_protocol_access allow localhost
+
+
+Intercepted traffic from a client proxy or tunnel:
+
+ http_port 3128 intercept require-proxy-header
+ proxy_protocol_access allow localhost
+
+
+Known Issue:
+ Use of require-proxy-header on https_port is not
supported.
+
Changes to squid.conf since Squid-3.4
There have been changes to Squid's configuration file since Squid-3.4.
Squid supports reading configuration option parameters from external
files using the syntax parameters("/path/filename"). For example:
acl whitelist dstdomain parameters("/etc/squid/whitelist.txt")
The squid.conf macro ${service_name} is added to provide the service name
of the process parsing the config.
There have also been changes to individual directives in the config file.
This section gives a thorough account of those changes in three categories:
New tags
collapsed_forwarding
Ported from Squid-2 with no configuration or visible behaviour
changes.
Collapsing of requests is performed across SMP workers.
+ proxy_protocol_ac
