Re: svn commit: r1783256 - /httpd/httpd/branches/2.4.x/STATUS
On 3/13/2017 7:31 PM, William A Rowe Jr wrote: > On Sat, Mar 11, 2017 at 1:33 PM, Daniel Ruggeri wrote: >> On 2/20/2017 10:58 AM, William A Rowe Jr wrote: >>> On Sat, Feb 18, 2017 at 4:44 PM, Daniel Ruggeri >>> wrote: Hi, Bill; I've replied about the pre_connnection situation - hoping someone can give the proposed patch a test as I don't have a handy H2 testbed. >>> Yup! Will review that thread - it's the -1 half (as opposed to a general -0 >>> half >>> for a 'pause' request while I was trying to get to reviewing the >>> original commit.) >> No worries at all. Reviews are important. >> >> I am curious what you mean about -1 half vs -0 half, though. Is that >> -1.5 vs -.5? :-) > The entire -1 reservation was simply till a fix is in place... My -0 > reservation > (or 'half') was just a beg for time to review... if you applied it > literally it would > be -0.5 :) > On the other comment, can you help me understand what redundant code is happening per-request? When manipulating the request, there are only four things happening differently: 1. A check that we have data stored away from the connection filter 2. A check that the connection data has a client IP 3. The assignment of the data to the request_rec's structure and logging at TRACE1 4. If no data was found, a check to see if it was optional and a logging statement/return according to that result >>> AIUI; the directives are all configured per-Server, the PROXY protocol data >>> is fixed for the lifespan of the Connection. The PROXY protocol is >>> significantly >>> more binding that either x-f-f or even x-remoteip. I'm not even sure where >>> the >>> 'optional' scheme originated; if present when not allowed, that's a probable >>> abuse pattern, and when not present when honored, that too indicates some >>> malfunction and traffic shouldn't proceed IMO. I don't know that the >>> optional >>> list should be shipped, it's far too simple to create a completely insecure >>> setup that won't raise eyebrows. The PROXY protocol reference spec states >>> the connection (by origin or destination IP) follows the PROXY protocol, or >>> it does not. >> Sorry to mix threads. I just replied a moment ago with a bit of >> reasoning behind the Optional use case. While it's possible that a >> server admin could mistakenly enable something they don't intend to or >> open things up more than they should, that's applicable any time someone >> enables some sort of authnz. I'm happy to reinforce this point in the >> docs for the Optional case but I still think enough utility is there to >> include it. > Is OK - Read your replies to my questions in that thread and they were > very clear, thanks. > > Your objection in this thread centered around being able to connect both > as a test/monitoring and as a consumer of the site passing through HAProxy. > I'd expect that to be a binary decision based on the origin IP/netmask? > Not sure that is settled, we can dive deeper into that subject, but you are > not wrong that a given vhost needs to be monitored without PROXY protocol > and traffiked via PROXY+httpd. I'm hoping the by-immediate-peer IP is > sufficient to accomplish that as a binary decision, per the spec you refer > to below. At first blush, I don't see why not. Inspecting the client IP and determining if it is in a certain network range would be an easy/efficient toggle for whether the filter gets injected or not. I'm sure someone could dream up a use case, but since this filter is dealing with layer 4 information it seems reasonable for layer 4 to be the deciding point whether or not to use it. > >>> Beyond that concern, I'm wondering if we shouldn't be using the *original* >>> design of mod_remoteip, changing the conn_rec client_addr/client_ip (and >>> null out remote_host/logname) and never alter it between requests. >>> >>> We can leave a conn pool note behind for the per-req processing, to retrieve >>> the proxy IP into a req variable if desired doing the rest of the >>> remoteip request >>> phase, but the remaining per-req code and processing is near insignificant. >>> >>> Thoughts? >>> This should all be quite straight forward per request... In fact, it's a much shorter logical path and less work than having to parse the X-Forwarded-For header. >>> So I was unspooling how we would handle stacked variables. >>> >>> Any PROXY protocol is the nearest hop; if multiple PROXY protocol header >>> lines occurred, the closest would be transmitted first, etc. >> I'm not sure if multiple PROXY lines are permitted. Looking at section >> 4.1, I think the intent is that PROXY-aware servers would continue >> propagating the original client IP address in any PROXY headers it emits. >> For example, in the diagram in section 4.1, PX2 should emit a PROXY >> header to the backend server that has the client IP it received from the >> PROXY header in PX1. >> >> Ref: http://www.haproxy.org/download/1.8/
Re: svn commit: r1783256 - /httpd/httpd/branches/2.4.x/STATUS
On Sat, Mar 11, 2017 at 1:33 PM, Daniel Ruggeri wrote: > > On 2/20/2017 10:58 AM, William A Rowe Jr wrote: >> On Sat, Feb 18, 2017 at 4:44 PM, Daniel Ruggeri wrote: >>> Hi, Bill; >>>I've replied about the pre_connnection situation - hoping someone can >>> give the proposed patch a test as I don't have a handy H2 testbed. >> Yup! Will review that thread - it's the -1 half (as opposed to a general -0 >> half >> for a 'pause' request while I was trying to get to reviewing the >> original commit.) > > No worries at all. Reviews are important. > > I am curious what you mean about -1 half vs -0 half, though. Is that > -1.5 vs -.5? :-) The entire -1 reservation was simply till a fix is in place... My -0 reservation (or 'half') was just a beg for time to review... if you applied it literally it would be -0.5 :) >>> On the other comment, can you help me understand what redundant code is >>> happening per-request? When manipulating the request, there are only >>> four things happening differently: >>> 1. A check that we have data stored away from the connection filter >>> 2. A check that the connection data has a client IP >>> 3. The assignment of the data to the request_rec's structure and logging >>> at TRACE1 >>> 4. If no data was found, a check to see if it was optional and a logging >>> statement/return according to that result >> AIUI; the directives are all configured per-Server, the PROXY protocol data >> is fixed for the lifespan of the Connection. The PROXY protocol is >> significantly >> more binding that either x-f-f or even x-remoteip. I'm not even sure where >> the >> 'optional' scheme originated; if present when not allowed, that's a probable >> abuse pattern, and when not present when honored, that too indicates some >> malfunction and traffic shouldn't proceed IMO. I don't know that the optional >> list should be shipped, it's far too simple to create a completely insecure >> setup that won't raise eyebrows. The PROXY protocol reference spec states >> the connection (by origin or destination IP) follows the PROXY protocol, or >> it does not. > > Sorry to mix threads. I just replied a moment ago with a bit of > reasoning behind the Optional use case. While it's possible that a > server admin could mistakenly enable something they don't intend to or > open things up more than they should, that's applicable any time someone > enables some sort of authnz. I'm happy to reinforce this point in the > docs for the Optional case but I still think enough utility is there to > include it. Is OK - Read your replies to my questions in that thread and they were very clear, thanks. Your objection in this thread centered around being able to connect both as a test/monitoring and as a consumer of the site passing through HAProxy. I'd expect that to be a binary decision based on the origin IP/netmask? Not sure that is settled, we can dive deeper into that subject, but you are not wrong that a given vhost needs to be monitored without PROXY protocol and traffiked via PROXY+httpd. I'm hoping the by-immediate-peer IP is sufficient to accomplish that as a binary decision, per the spec you refer to below. >> Beyond that concern, I'm wondering if we shouldn't be using the *original* >> design of mod_remoteip, changing the conn_rec client_addr/client_ip (and >> null out remote_host/logname) and never alter it between requests. >> >> We can leave a conn pool note behind for the per-req processing, to retrieve >> the proxy IP into a req variable if desired doing the rest of the >> remoteip request >> phase, but the remaining per-req code and processing is near insignificant. >> >> Thoughts? >> >>> This should all be quite straight forward per request... In fact, it's a >>> much shorter logical path and less work than having to parse the >>> X-Forwarded-For header. >> So I was unspooling how we would handle stacked variables. >> >> Any PROXY protocol is the nearest hop; if multiple PROXY protocol header >> lines occurred, the closest would be transmitted first, etc. > > I'm not sure if multiple PROXY lines are permitted. Looking at section > 4.1, I think the intent is that PROXY-aware servers would continue > propagating the original client IP address in any PROXY headers it emits. > For example, in the diagram in section 4.1, PX2 should emit a PROXY > header to the backend server that has the client IP it received from the > PROXY header in PX1. > > Ref: http://www.haproxy.org/download/1.8/doc/proxy-protocol.txt So I read nothing that prohibits it... and you? >> All local x-remoteip style values would be the next most distant hop; very >> similar to the haproxy protocol, it indicates some absolutely trusted edge >> router/balancer. >> >> Any x-f-f that occurs would reflect all the next most distant hops. Finally, >> any 'Forwarded' header (rfc7239) are the most distant hops. I'm basing >> that conclusion on the fact that all 'Forwarded'-aware intermediaries which >> construct a 'Forwarded' header would not
Re: svn commit: r1783256 - /httpd/httpd/branches/2.4.x/STATUS
On Mon, Mar 13, 2017 at 7:31 PM, William A Rowe Jr wrote: > On Sat, Mar 11, 2017 at 1:33 PM, Daniel Ruggeri wrote: >> This is important for us on two fronts: >> * For mod_remoteip, we'd have to decide which to use. The current method >> is to prefer PROXY. >> * If we add PROXY support to mod_proxy, we have to decide which to propagate > > [...] > > We support X-F-F to some extent today, but not properly. But because we > are an HTTP server which can mangle HTTP request metadata, and our > proxy connections are not remote connection-bound, we should probably > apply the logic above to generate an RFC7239 Forwarded header. This > is where we probably collapse all Whoops, sorry... "Where we should probably collapse all" trusted proxy data into the alternate header, and relay all remaining untrusted X-F-F/Forwarded data on to the client as 'you deal with this'. Or add a flag to recombine it all and let the backend reprocess it all, but the entire point of putting httpd somewhere in the chain is to deduplicate and eliminate useless data and CPU time.
Re: svn commit: r1783256 - /httpd/httpd/branches/2.4.x/STATUS
On 2/20/2017 10:58 AM, William A Rowe Jr wrote: > On Sat, Feb 18, 2017 at 4:44 PM, Daniel Ruggeri wrote: >> Hi, Bill; >>I've replied about the pre_connnection situation - hoping someone can >> give the proposed patch a test as I don't have a handy H2 testbed. > Yup! Will review that thread - it's the -1 half (as opposed to a general -0 > half > for a 'pause' request while I was trying to get to reviewing the > original commit.) No worries at all. Reviews are important. I am curious what you mean about -1 half vs -0 half, though. Is that -1.5 vs -.5? :-) > >> On the other comment, can you help me understand what redundant code is >> happening per-request? When manipulating the request, there are only >> four things happening differently: >> 1. A check that we have data stored away from the connection filter >> 2. A check that the connection data has a client IP >> 3. The assignment of the data to the request_rec's structure and logging >> at TRACE1 >> 4. If no data was found, a check to see if it was optional and a logging >> statement/return according to that result > AIUI; the directives are all configured per-Server, the PROXY protocol data > is fixed for the lifespan of the Connection. The PROXY protocol is > significantly > more binding that either x-f-f or even x-remoteip. I'm not even sure where the > 'optional' scheme originated; if present when not allowed, that's a probable > abuse pattern, and when not present when honored, that too indicates some > malfunction and traffic shouldn't proceed IMO. I don't know that the optional > list should be shipped, it's far too simple to create a completely insecure > setup that won't raise eyebrows. The PROXY protocol reference spec states > the connection (by origin or destination IP) follows the PROXY protocol, or > it does not. Sorry to mix threads. I just replied a moment ago with a bit of reasoning behind the Optional use case. While it's possible that a server admin could mistakenly enable something they don't intend to or open things up more than they should, that's applicable any time someone enables some sort of authnz. I'm happy to reinforce this point in the docs for the Optional case but I still think enough utility is there to include it. > > Beyond that concern, I'm wondering if we shouldn't be using the *original* > design of mod_remoteip, changing the conn_rec client_addr/client_ip (and > null out remote_host/logname) and never alter it between requests. > > We can leave a conn pool note behind for the per-req processing, to retrieve > the proxy IP into a req variable if desired doing the rest of the > remoteip request > phase, but the remaining per-req code and processing is near insignificant. > > Thoughts? > >> This should all be quite straight forward per request... In fact, it's a >> much shorter logical path and less work than having to parse the >> X-Forwarded-For header. > So I was unspooling how we would handle stacked variables. > > Any PROXY protocol is the nearest hop; if multiple PROXY protocol header > lines occurred, the closest would be transmitted first, etc. I'm not sure if multiple PROXY lines are permitted. Looking at section 4.1, I think the intent is that PROXY-aware servers would continue propagating the original client IP address in any PROXY headers it emits. For example, in the diagram in section 4.1, PX2 should emit a PROXY header to the backend server that has the client IP it received from the PROXY header in PX1. Ref: http://www.haproxy.org/download/1.8/doc/proxy-protocol.txt > All local x-remoteip style values would be the next most distant hop; very > similar to the haproxy protocol, it indicates some absolutely trusted edge > router/balancer. > > Any x-f-f that occurs would reflect all the next most distant hops. Finally, > any 'Forwarded' header (rfc7239) are the most distant hops. I'm basing > that conclusion on the fact that all 'Forwarded'-aware intermediaries which > construct a 'Forwarded' header would not carry the x-f-f, but concatenate > these as closer than the nearest 'Forwarded'-aware hop. So the presence > of an x-f-f header indicates the presence of a 'Forwarded'-unaware agent > between this incoming connection and the closest 'Forwarded'-aware agent. Yep, I follow the thought process and agree. This assumes that the intermediary isn't being clever or dumb by... * Sending the traffic as it received it (so not technically complying with any of the methods of propagating client and intermediary info) * Sending an appropriate 7239 header, but blindly passing X-Forwarded-For * Rewriting both headers to contain the same data in their expected formats FWIW, I feel the struggle of unwrapping all of this, too. At $dayjob, because of the potential silliness of various intermediaries, we chose to create a custom header that is always written (dropped if it comes to us) when our edge devices receive a connection. > > I'm not suggesting these two enhancements, PROXY and RFC7239 are > intert
Re: svn commit: r1783256 - /httpd/httpd/branches/2.4.x/STATUS
On Sat, Feb 18, 2017 at 4:44 PM, Daniel Ruggeri wrote: > > Hi, Bill; >I've replied about the pre_connnection situation - hoping someone can > give the proposed patch a test as I don't have a handy H2 testbed. Yup! Will review that thread - it's the -1 half (as opposed to a general -0 half for a 'pause' request while I was trying to get to reviewing the original commit.) > On the other comment, can you help me understand what redundant code is > happening per-request? When manipulating the request, there are only > four things happening differently: > 1. A check that we have data stored away from the connection filter > 2. A check that the connection data has a client IP > 3. The assignment of the data to the request_rec's structure and logging > at TRACE1 > 4. If no data was found, a check to see if it was optional and a logging > statement/return according to that result AIUI; the directives are all configured per-Server, the PROXY protocol data is fixed for the lifespan of the Connection. The PROXY protocol is significantly more binding that either x-f-f or even x-remoteip. I'm not even sure where the 'optional' scheme originated; if present when not allowed, that's a probable abuse pattern, and when not present when honored, that too indicates some malfunction and traffic shouldn't proceed IMO. I don't know that the optional list should be shipped, it's far too simple to create a completely insecure setup that won't raise eyebrows. The PROXY protocol reference spec states the connection (by origin or destination IP) follows the PROXY protocol, or it does not. Beyond that concern, I'm wondering if we shouldn't be using the *original* design of mod_remoteip, changing the conn_rec client_addr/client_ip (and null out remote_host/logname) and never alter it between requests. We can leave a conn pool note behind for the per-req processing, to retrieve the proxy IP into a req variable if desired doing the rest of the remoteip request phase, but the remaining per-req code and processing is near insignificant. Thoughts? > This should all be quite straight forward per request... In fact, it's a > much shorter logical path and less work than having to parse the > X-Forwarded-For header. So I was unspooling how we would handle stacked variables. Any PROXY protocol is the nearest hop; if multiple PROXY protocol header lines occurred, the closest would be transmitted first, etc. All local x-remoteip style values would be the next most distant hop; very similar to the haproxy protocol, it indicates some absolutely trusted edge router/balancer. Any x-f-f that occurs would reflect all the next most distant hops. Finally, any 'Forwarded' header (rfc7239) are the most distant hops. I'm basing that conclusion on the fact that all 'Forwarded'-aware intermediaries which construct a 'Forwarded' header would not carry the x-f-f, but concatenate these as closer than the nearest 'Forwarded'-aware hop. So the presence of an x-f-f header indicates the presence of a 'Forwarded'-unaware agent between this incoming connection and the closest 'Forwarded'-aware agent. I'm not suggesting these two enhancements, PROXY and RFC7239 are intertwined, we can certainly ship them in different releases, but I was having problems working out X-F-F vs Forwarded until I was working through the PROXY logic and came to the conclusion above, and am looking for others to sanity-check my logic on this.
Re: svn commit: r1783256 - /httpd/httpd/branches/2.4.x/STATUS
On 2/16/2017 11:48 AM, wr...@apache.org wrote: > Author: wrowe > Date: Thu Feb 16 17:48:28 2017 > New Revision: 1783256 > > URL: http://svn.apache.org/viewvc?rev=1783256&view=rev > Log: > Slow two still-wobbly horses > > Modified: > httpd/httpd/branches/2.4.x/STATUS > > Modified: httpd/httpd/branches/2.4.x/STATUS > URL: > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1783256&r1=1783255&r2=1783256&view=diff > == > --- httpd/httpd/branches/2.4.x/STATUS (original) > +++ httpd/httpd/branches/2.4.x/STATUS Thu Feb 16 17:48:28 2017 > @@ -167,6 +167,9 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK: > 2.4 convenience patch (includes CHANGES): > > http://people.apache.org/~druggeri/patches/RemoteIPProxyProtocol.2.4.x.patch > +1: druggeri, jim > + -1: wrowe (as noted on list, not limiting to inbound primary pre_conn > +scope correctly; lots of redundant code happpening > per-request > +for a per-connection behavior. Investigating further.) > Hi, Bill; I've replied about the pre_connnection situation - hoping someone can give the proposed patch a test as I don't have a handy H2 testbed. On the other comment, can you help me understand what redundant code is happening per-request? When manipulating the request, there are only four things happening differently: 1. A check that we have data stored away from the connection filter 2. A check that the connection data has a client IP 3. The assignment of the data to the request_rec's structure and logging at TRACE1 4. If no data was found, a check to see if it was optional and a logging statement/return according to that result This should all be quite straight forward per request... In fact, it's a much shorter logical path and less work than having to parse the X-Forwarded-For header. >*) mod_brotli: Backport of mod_brotli filter > trunk patch: http://svn.apache.org/r1761714 > @@ -176,6 +179,7 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK: >http://svn.apache.org/r1779077 > 2.4.x patch: http://home.apache.org/~jim/patches/brotli-2.4.patch > +1: jim, jorton, > + -1: wrowe (Premature, waiting on github.com/google/brotli stable > release) > jailletc36: doc should also be back-ported (r1779091 + r1779699) > >*) mod_ssl: work around leaks on (graceful) restart. > > -- Daniel Ruggeri