Re: svn commit: r1783256 - /httpd/httpd/branches/2.4.x/STATUS

[Daniel Ruggeri]

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.
>>

Re: svn commit: r1783256 - /httpd/httpd/branches/2.4.x/STATUS

[William A Rowe Jr]

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

Re: svn commit: r1783256 - /httpd/httpd/branches/2.4.x/STATUS

[William A Rowe Jr]

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

[Daniel Ruggeri]

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 

Re: svn commit: r1783256 - /httpd/httpd/branches/2.4.x/STATUS

[William A Rowe Jr]

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

[Daniel Ruggeri]

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=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=1783255=1783256=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