Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Daniel Ruggeri]

I'm not sure if my mail client mangled the message or my email provider
did, but I couldn't read this except from lists.a.o so my reply may
appear as a new thread in your email client if it's following thread IDs.


Christophe JAILLET wrote:
> First of all, http://blog.haproxy.com/haproxy/proxy-protocol/ list 
> another module implementation for Apache:
> https://github.com/ggrandes/apache24-modules/blob/master/mod_myfixip.c
>
> If anyone wants to give it a look.
>
>
>
> Anyway, a few minor comments below.
>
> CJ
>
>
> Le 30/12/2016 à 15:20, drugg...@apache.org a écrit :
>> Author: druggeri
>> Date: Fri Dec 30 14:20:48 2016
>> New Revision: 1776575
>>
>> URL:http://svn.apache.org/viewvc?rev=1776575=rev
>> Log:
>> Merge new PROXY protocol code into mod_remoteip
>>
>> Modified:
>>  httpd/httpd/trunk/docs/log-message-tags/next-number
>>  httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
>>  httpd/httpd/trunk/modules/metadata/mod_remoteip.c
>>
>> Modified: httpd/httpd/trunk/docs/log-message-tags/next-number
>> URL:http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/next-number?rev=1776575=1776574=1776575=diff
>> ==
>> --- httpd/httpd/trunk/docs/log-message-tags/next-number (original)
>> +++ httpd/httpd/trunk/docs/log-message-tags/next-number Fri Dec 30 14:20:48 
>> 2016
>> @@ -1 +1 @@
>> -3491
>> +3514
>>
>> Modified: httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
>> URL:http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml?rev=1776575=1776574=1776575=diff
>> ==
>> --- httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml (original)
>> +++ httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml Fri Dec 30 14:20:48 
>> 2016
>> @@ -42,6 +42,12 @@ via the request headers.
>>   with the useragent IP address reported in the request header configured
>>   with the RemoteIPHeader 
>> directive.
>>   
>> +Additionally, this module implements the server side of
>> +HAProxy's
>> +http://blog.haproxy.com/haproxy/proxy-protocol/;>Proxy 
>> Protocol when
>> +using the > module="mod_remoteip">RemoteIPProxyProtocolEnable
>> +directive.
>> +
>>   Once replaced as instructed, this overridden useragent IP address is
>>   then used for the mod_authz_host
>>   Require 
>> ip
>> @@ -59,6 +65,7 @@ via the request headers.
>>   mod_authz_host
>>   mod_status
>>   mod_log_config
>> +> href="http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt;>Proxy 
>> Protocol Spec
>>   
>>   Remote IP Processing
>>   
>> @@ -214,6 +221,70 @@ RemoteIPProxiesHeader X-Forwarded-By
>>   
>>   
>>   
>> +
>> +RemoteIPProxyProtocol
> s/RemoteIPProxyProtocol/RemoteIPProxyProtocolEnable/

Right - this was addressed in a subsequent commit after discussing the
name. It's now RemoteIPProxyProtocol


>
>> +Enable, optionally enable or disable the proxy protocol 
>> handling
>> +ProxyProtocol On|Optional|Off
>> +server configvirtual host
>> +
> Compatibility note missing.

Added - thanks!


>
>> 
>>
>> Modified: httpd/httpd/trunk/modules/metadata/mod_remoteip.c
>> URL:http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/metadata/mod_remoteip.c?rev=1776575=1776574=1776575=diff
>> ==
>> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
>> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 
>> 2016
>> @@ -12,15 +12,20 @@
>>* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>>* See the License for the specific language governing permissions and
>>* limitations under the License.
>> + *
>> + * The majority of the input filter code for PROXY protocol support is
>> + * Copyright 2014 Cloudzilla Inc.
>>*/
>>   
>>   #include "ap_config.h"
>>   #include "ap_mmn.h"
>> +#include "ap_listen.h"
>>   #include "httpd.h"
>>   #include "http_config.h"
>>   #include "http_connection.h"
>>   #include "http_protocol.h"
>>   #include "http_log.h"
>> +#include "http_main.h"
>>   #include "apr_strings.h"
>>   #include "apr_lib.h"
>>   #define APR_WANT_BYTEFUNC
>> @@ -36,6 +41,12 @@ typedef struct {
>>   void  *internal;
>>   } remoteip_proxymatch_t;
>>   
>> +typedef struct remoteip_addr_info {
>> +struct remoteip_addr_info *next;
>> +apr_sockaddr_t *addr;
>> +server_rec *source;
>> +} remoteip_addr_info;
>> +
>>   typedef struct {
>>   /** The header to retrieve a proxy-via IP list */
>>   const char *header_name;
>> @@ -48,6 +59,17 @@ typedef struct {
>>*  with the most commonly encountered listed first
>>*/
>>   apr_array_header_t *proxymatch_ip;
>> +
>> +remoteip_addr_info *proxy_protocol_enabled;
>> +remoteip_addr_info *proxy_protocol_optional;
>> +remoteip_addr_info *proxy_protocol_disabled;
>> +
>> +/** A flag 

Re: AW: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Daniel Ruggeri]

On 1/30/2017 4:45 AM, Ruediger Pluem wrote:
> Thinking of all the above it might be best if you read in mode 
> AP_MODE_SPECULATIVE on your own from upstream until
> you have MIN_HDR_LEN data. If the PROXY header is present, read MIN_HDR_LEN 
> in AP_MODE_READBYTES to finally consume the
> data and move on. If the PROXY header is not present, well then just forward 
> the original request and you are fine.
> This way you leave all the hassle to the upstream filters.

Yes, definitely. I was contemplating the same thing given the
permutations of modes it may be called in and the various cases to deal
with. That's the approach I've taken in the latest commit because the
hassle is definitely best left to upstream :-)

At a high level, it no longer stores the data to pass along. When
optional processing is enabled, the filter starts in speculative read
mode. Once MIN_HDR_LEN is read and we know if a header is there or not
we can discard ctx->bb, reinitialize ctx and move to READBYTES mode.

-- 
Daniel Ruggeri

Re: AW: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Ruediger Pluem]

On 01/28/2017 07:14 PM, Daniel Ruggeri wrote:
> 
> On 1/25/2017 9:02 PM, Daniel Ruggeri wrote:
>> On 1/25/2017 6:53 PM, Daniel Ruggeri wrote:
>>> I'd say that not returning until ctx->bb has enough information to
>>> determine if the header is present or not would be sufficient. Isn't
>>> this already done in the potentially repeated calls to ap_get_brigade on
>>> line no 1056 inside the loop verifying that ctx->done is not set? If
>>> we're not intentionally holding onto the data until we know it's safe,
>>> then I think there's a structural problem in this module that would
>>> require us to start doing so.
>> Sorry - I neglected to notice the obvious check that the brigade is not
>> empty JUST before this line. Nevermind this silly comment...
>>
>> Indeed, if the brigade runs dry before the minimum number of bytes
>> needed has been read, the data should not be deleted.
>> I'm thinking apr_bucket_setaside(b, f->c->pool) in place of
>> apr_bucket_delete(b) would be good here... I will also experiment with
>> having the filter ask for less data than it needs to verify these
>> multiple calls through the filter work as expected.
>>
> 
> Changes to set aside the bucket data are in r1780725. I'll await
> updating the 2.4 backport patch with this and the compiler warning fix
> when we're good on how to proceed regarding the other email I just sent.
> 
> To verify behavior, I forced the filter to receive only a few bytes at a
> time until it "built up" enough data to evaluate the header type and
> pass all data set aside in bb_out when RemoteIPProxyProtocol is set to
> Optional. Thanks for the pointers.
> 

I think there are still several edge cases open, especially in the optional
case. Let me try to list.

1. I guess you should step aside (for this particular call) if the filter is 
called with the following modes:

AP_MODE_INIT
AP_MODE_EATCRLF ???

2. All the real fun starts with the optional case if you do not find the PROXY 
header:

1. The original call to your filter requested less bytes than MIN_HDR_LEN. You 
can only
   return the amount of data requested and you need to keep the remaining stuff 
in your ctx->store_bb. For each
   following call you need to hand out your remaining data in ctx->store_bb 
first.
2. 1. gets even more fun if the the reader requested mode AP_MODE_SPECULATIVE. 
In this case you can proceed as in 1.,
   but you need to hand out copies of the data as you need to be able to return 
that data later again in case of a non
   speculative read.
3. In AP_MODE_GETLINE you need to ensure that you don't return data past a LF 
character. If there is no LF in your
   ctx->store_bb data you need to add the missing data by doing a 
ap_get_brigade on your own.
4. There might be intermittent calls of 1., 2. and 3.


Thinking of all the above it might be best if you read in mode 
AP_MODE_SPECULATIVE on your own from upstream until
you have MIN_HDR_LEN data. If the PROXY header is present, read MIN_HDR_LEN in 
AP_MODE_READBYTES to finally consume the
data and move on. If the PROXY header is not present, well then just forward 
the original request and you are fine.
This way you leave all the hassle to the upstream filters.



Regards

Rüdiger

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Ruediger Pluem]

On 01/28/2017 06:59 PM, Daniel Ruggeri wrote:
> Ruediger Pluem wrote:
>> +/* try to read a header's worth of data */
>> +while (!ctx->done) {
>> +if (APR_BRIGADE_EMPTY(ctx->bb)) {
>> +ret = ap_get_brigade(f->next, ctx->bb, ctx->mode, block,
>> + ctx->need - ctx->rcvd);
>> +if (ret != APR_SUCCESS) {
>> +return ret;
>> +}
>> +}
>> +if (APR_BRIGADE_EMPTY(ctx->bb)) {
> What about the case of an non blocking read where the upstream filter
> returns an empty brigade
> and APR_SUCCESS. This is equal to returning EAGAIN.
> 
>> +return APR_EOF;
>> +}
> 
> Coming back to this one after correcting the setaside stuff... Is this
> what you have in mind or should we actually return APR_EAGAIN?
> 
> return block == APR_NONBLOCK_READ ? APR_SUCCESS : APR_EOF;
> 

IMHO you will hit the APR_BRIGADE_EMPTY(ctx->bb) with APR_SUCCESS returned case 
only
in the non blocking case. So it would be sufficient to replace

return APR_EOF

with

return APR_SUCCESS.

You proposal is a little bit more fail safe and hence probably a good idea.
Furthermore I think you could move the second check for an empty brigade inside
the block checking it firstly (at the end).

Regards

Rüdiger

Re: AW: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Daniel Ruggeri]

On 1/25/2017 9:02 PM, Daniel Ruggeri wrote:
> On 1/25/2017 6:53 PM, Daniel Ruggeri wrote:
>> I'd say that not returning until ctx->bb has enough information to
>> determine if the header is present or not would be sufficient. Isn't
>> this already done in the potentially repeated calls to ap_get_brigade on
>> line no 1056 inside the loop verifying that ctx->done is not set? If
>> we're not intentionally holding onto the data until we know it's safe,
>> then I think there's a structural problem in this module that would
>> require us to start doing so.
> Sorry - I neglected to notice the obvious check that the brigade is not
> empty JUST before this line. Nevermind this silly comment...
>
> Indeed, if the brigade runs dry before the minimum number of bytes
> needed has been read, the data should not be deleted.
> I'm thinking apr_bucket_setaside(b, f->c->pool) in place of
> apr_bucket_delete(b) would be good here... I will also experiment with
> having the filter ask for less data than it needs to verify these
> multiple calls through the filter work as expected.
>

Changes to set aside the bucket data are in r1780725. I'll await
updating the 2.4 backport patch with this and the compiler warning fix
when we're good on how to proceed regarding the other email I just sent.

To verify behavior, I forced the filter to receive only a few bytes at a
time until it "built up" enough data to evaluate the header type and
pass all data set aside in bb_out when RemoteIPProxyProtocol is set to
Optional. Thanks for the pointers.

-- 
Daniel Ruggeri

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Daniel Ruggeri]

Ruediger Pluem wrote:
> +/* try to read a header's worth of data */
> +while (!ctx->done) {
> +if (APR_BRIGADE_EMPTY(ctx->bb)) {
> +ret = ap_get_brigade(f->next, ctx->bb, ctx->mode, block,
> + ctx->need - ctx->rcvd);
> +if (ret != APR_SUCCESS) {
> +return ret;
> +}
> +}
> +if (APR_BRIGADE_EMPTY(ctx->bb)) {
What about the case of an non blocking read where the upstream filter
returns an empty brigade
and APR_SUCCESS. This is equal to returning EAGAIN.

> +return APR_EOF;
> +}

Coming back to this one after correcting the setaside stuff... Is this
what you have in mind or should we actually return APR_EAGAIN?

return block == APR_NONBLOCK_READ ? APR_SUCCESS : APR_EOF;

-- 
Daniel Ruggeri

Re: AW: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Daniel Ruggeri]

On 1/25/2017 6:53 PM, Daniel Ruggeri wrote:
> I'd say that not returning until ctx->bb has enough information to
> determine if the header is present or not would be sufficient. Isn't
> this already done in the potentially repeated calls to ap_get_brigade on
> line no 1056 inside the loop verifying that ctx->done is not set? If
> we're not intentionally holding onto the data until we know it's safe,
> then I think there's a structural problem in this module that would
> require us to start doing so.
Sorry - I neglected to notice the obvious check that the brigade is not
empty JUST before this line. Nevermind this silly comment...

Indeed, if the brigade runs dry before the minimum number of bytes
needed has been read, the data should not be deleted.
I'm thinking apr_bucket_setaside(b, f->c->pool) in place of
apr_bucket_delete(b) would be good here... I will also experiment with
having the filter ask for less data than it needs to verify these
multiple calls through the filter work as expected.

-- 
Daniel Ruggeri

Re: AW: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Daniel Ruggeri]

First, my apologies, but it looks like line wrapping is going to exceed
the usual number of columns so formatting may get wonky in this reply.

On 1/17/2017 3:48 AM, Plüm, Rüdiger, Vodafone Group wrote:
>
>> -Ursprüngliche Nachricht-
>> Von: Daniel Ruggeri [mailto:drugg...@primary.net]
>> Gesendet: Dienstag, 17. Januar 2017 00:57
>> An: dev@httpd.apache.org
>> Betreff: Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-
>> message-tags/next-number docs/manual/mod/mod_remoteip.xml
>> modules/metadata/mod_remoteip.c
>>
>> For the most part, yes except the portions that make the header presence
>> optional (the HDR_MISSING case). Those were added as it came into the
>> code base to handle a use case I was working on. I've added some
>> comments inline since I won't have time to poke around myself for a
>> while yet.
>>
>>
>> For convenience, here's a link to the original code
>>
>> https://github.com/roadrunner2/mod-proxy-
>> protocol/blob/master/mod_proxy_protocol.c
>>
>>
>> On 1/16/2017 10:14 AM, Jim Jagielski wrote:
>>> Let me take a look... afaict, this is a copy of what
>>> was donated, which has been working for numerous people.
>>> But that doesn't mean it can't have bugs ;)
>>>
>>>> On Jan 16, 2017, at 7:20 AM, Ruediger Pluem <rpl...@apache.org>
>> wrote:
>>>> Anyone?
>> Apologies for missing the original reply
>>
>>>> Regards
>>>>
>>>> Rüdiger
>>>>
>>>> On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
>>>>> On 12/30/2016 03:20 PM, drugg...@apache.org wrote:
>>>>>> Author: druggeri
>>>>>> Date: Fri Dec 30 14:20:48 2016
>>>>>> New Revision: 1776575
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=1776575=rev
>>>>>> Log:
>>>>>> Merge new PROXY protocol code into mod_remoteip
>>>>>>
>>>>>> Modified:
>>>>>>httpd/httpd/trunk/docs/log-message-tags/next-number
>>>>>>httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
>>>>>>httpd/httpd/trunk/modules/metadata/mod_remoteip.c
>>>>>>
>>>>>>
>> 
>> ==
>>>>>> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
>>>>>> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30
>> 14:20:48 2016
>>>>>> + */
>>>>>> +static int remoteip_hook_pre_connection(conn_rec *c, void *csd)
>>>>>> +{
>>>>>> +remoteip_config_t *conf;
>>>>>> +remoteip_conn_config_t *conn_conf;
>>>>>> +int optional;
>>>>>> +
>>>>>> +conf = ap_get_module_config(ap_server_conf->module_config,
>>>>>> +_module);
>>>>>> +
>>>>>> +/* Used twice - do the check only once */
>>>>>> +optional = remoteip_addr_in_list(conf-
>>> proxy_protocol_optional, c->local_addr);
>>>>>> +
>>>>>> +/* check if we're enabled for this connection */
>>>>>> +if ((!remoteip_addr_in_list(conf->proxy_protocol_enabled, c-
>>> local_addr)
>>>>>> +  && !optional )
>>>>>> +|| remoteip_addr_in_list(conf->proxy_protocol_disabled, c-
>>> local_addr)) {
>>>>>> +return DECLINED;
>>>>>> +}
>>>>>> +
>>>>>> +/* mod_proxy creates outgoing connections - we don't want
>> those */
>>>>>> +if (!remoteip_is_server_port(c->local_addr->port)) {
>>>>>> +return DECLINED;
>>>>>> +}
>>>>> Why is the c->local_addr->port set to a port we are listening on in
>> case of proxy connections?
>>
>> This is from the original code, but wouldn't this be the local port on
>> the outbound connection (some high number)? The remoteip_is_server_port
>> should therefore fail this check.
> My bad I missed the ! in the condition. So this should work.
>
>>>>>> +
>>>>>> +switch (psts) {
>>>>>> +case HDR_MISSING:
>>>>>> +if (conn_conf->proxy_protocol_optional) {
>>>>>> + 

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Daniel Ruggeri]

To clarify, the issues with handling of the buckets and the fact that
buckets are not being set aside came from the original code. The
question about what to do regarding f->next versus passing the data
along some other way is the only one that came from the code I added to
make the header optional.

Being honest, I don't fully grok all the details of the filter stack and
what kinds of buckets to expect emanating from core as the request for
data propagates up through the input filters, but what Rudiger has
pointed out seems like a structural problem with the module that could
bite users under certain circumstances. What I'm particularly unclear
about is what those circumstances would be.

I'll try to reply to the other thread to provide more clarity.

-- 
Daniel Ruggeri

On 1/24/2017 8:36 AM, Jim Jagielski wrote:
> ++1. I know that Daniel is out of pocket for a little bit so I'l
> give it a coupla more days before I "restore" to the original filter
> code...
>> On Jan 24, 2017, at 3:46 AM, Ruediger Pluem  wrote:
>>
>>
>>
>> On 01/17/2017 02:48 PM, Jim Jagielski wrote:
 On Jan 16, 2017, at 6:57 PM, Daniel Ruggeri  wrote:

 For the most part, yes except the portions that make the header presence
 optional (the HDR_MISSING case). Those were added as it came into the
 code base to handle a use case I was working on. I've added some
 comments inline since I won't have time to poke around myself for a
 while yet.


 For convenience, here's a link to the original code

 https://github.com/roadrunner2/mod-proxy-protocol/blob/master/mod_proxy_protocol.c

>>> Would it make sense to have the "stable" version available
>>> for backport, and keep in the WIP in trunk?
>>>
>> This would be an option, but apart from this I would like to see the WIP in 
>> trunk
>> somehow fixed. Otherwise it is a perfect candidate for falling through the 
>> cracks
>> and giving yet another surprise once we branch "whatever we name it" from 
>> trunk.
>> IMHO easier to fix that now or even revert that part for the time being 
>> while people
>> remember what is going on then later digging through it while hitting the 
>> issues.
>>
>> Regards
>>
>> Rüdiger

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Jim Jagielski]

++1. I know that Daniel is out of pocket for a little bit so I'l
give it a coupla more days before I "restore" to the original filter
code...
> On Jan 24, 2017, at 3:46 AM, Ruediger Pluem  wrote:
> 
> 
> 
> On 01/17/2017 02:48 PM, Jim Jagielski wrote:
>> 
>>> On Jan 16, 2017, at 6:57 PM, Daniel Ruggeri  wrote:
>>> 
>>> For the most part, yes except the portions that make the header presence
>>> optional (the HDR_MISSING case). Those were added as it came into the
>>> code base to handle a use case I was working on. I've added some
>>> comments inline since I won't have time to poke around myself for a
>>> while yet.
>>> 
>>> 
>>> For convenience, here's a link to the original code
>>> 
>>> https://github.com/roadrunner2/mod-proxy-protocol/blob/master/mod_proxy_protocol.c
>>> 
>> 
>> Would it make sense to have the "stable" version available
>> for backport, and keep in the WIP in trunk?
>> 
> 
> This would be an option, but apart from this I would like to see the WIP in 
> trunk
> somehow fixed. Otherwise it is a perfect candidate for falling through the 
> cracks
> and giving yet another surprise once we branch "whatever we name it" from 
> trunk.
> IMHO easier to fix that now or even revert that part for the time being while 
> people
> remember what is going on then later digging through it while hitting the 
> issues.
> 
> Regards
> 
> Rüdiger

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Ruediger Pluem]

On 01/17/2017 02:48 PM, Jim Jagielski wrote:
> 
>> On Jan 16, 2017, at 6:57 PM, Daniel Ruggeri  wrote:
>>
>> For the most part, yes except the portions that make the header presence
>> optional (the HDR_MISSING case). Those were added as it came into the
>> code base to handle a use case I was working on. I've added some
>> comments inline since I won't have time to poke around myself for a
>> while yet.
>>
>>
>> For convenience, here's a link to the original code
>>
>> https://github.com/roadrunner2/mod-proxy-protocol/blob/master/mod_proxy_protocol.c
>>
> 
> Would it make sense to have the "stable" version available
> for backport, and keep in the WIP in trunk?
> 

This would be an option, but apart from this I would like to see the WIP in 
trunk
somehow fixed. Otherwise it is a perfect candidate for falling through the 
cracks
and giving yet another surprise once we branch "whatever we name it" from trunk.
IMHO easier to fix that now or even revert that part for the time being while 
people
remember what is going on then later digging through it while hitting the 
issues.

Regards

Rüdiger

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Christophe JAILLET]

First of all, http://blog.haproxy.com/haproxy/proxy-protocol/ list 
another module implementation for Apache:

https://github.com/ggrandes/apache24-modules/blob/master/mod_myfixip.c

If anyone wants to give it a look.



Anyway, a few minor comments below.

CJ


Le 30/12/2016 à 15:20, drugg...@apache.org a écrit :

Author: druggeri
Date: Fri Dec 30 14:20:48 2016
New Revision: 1776575

URL:http://svn.apache.org/viewvc?rev=1776575=rev
Log:
Merge new PROXY protocol code into mod_remoteip

Modified:
 httpd/httpd/trunk/docs/log-message-tags/next-number
 httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
 httpd/httpd/trunk/modules/metadata/mod_remoteip.c

Modified: httpd/httpd/trunk/docs/log-message-tags/next-number
URL:http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/next-number?rev=1776575=1776574=1776575=diff
==
--- httpd/httpd/trunk/docs/log-message-tags/next-number (original)
+++ httpd/httpd/trunk/docs/log-message-tags/next-number Fri Dec 30 14:20:48 2016
@@ -1 +1 @@
-3491
+3514

Modified: httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
URL:http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml?rev=1776575=1776574=1776575=diff
==
--- httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml (original)
+++ httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml Fri Dec 30 14:20:48 2016
@@ -42,6 +42,12 @@ via the request headers.
  with the useragent IP address reported in the request header configured
  with the RemoteIPHeader 
directive.
  
+Additionally, this module implements the server side of

+HAProxy's
+http://blog.haproxy.com/haproxy/proxy-protocol/;>Proxy 
Protocol when
+using the RemoteIPProxyProtocolEnable
+directive.
+
  Once replaced as instructed, this overridden useragent IP address is
  then used for the mod_authz_host
  Require ip
@@ -59,6 +65,7 @@ via the request headers.
  mod_authz_host
  mod_status
  mod_log_config
+http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt;>Proxy 
Protocol Spec
  
  Remote IP Processing
  
@@ -214,6 +221,70 @@ RemoteIPProxiesHeader X-Forwarded-By

  
  
  
+

+RemoteIPProxyProtocol


s/RemoteIPProxyProtocol/RemoteIPProxyProtocolEnable/


+Enable, optionally enable or disable the proxy protocol 
handling
+ProxyProtocol On|Optional|Off
+server configvirtual host
+


Compatibility note missing.


+
+
+The RemoteIPProxyProtocolEnable enables or
+disables the reading and handling of the proxy protocol connection header.
+If enabled with the On flag, the upstream client must
+send the header every time it opens a connection or the connection will
+be aborted. If enabled with the Optional flag, the upstream
+client may send the header.
+
+While this directive may be specified in any virtual host, it is
+important to understand that because the proxy protocol is connection
+based and protocol agnostic, the enabling and disabling is actually based
+on ip-address and port. This means that if you have multiple name-based
+virtual hosts for the same host and port, and you enable it any one of
+them, then it is enabled for all them (with that host and port). It also
+means that if you attempt to enable the proxy protocol in one and disable
+in the other, that won't work; in such a case the last one wins and a
+notice will be logged indicating which setting was being overridden.
+
+When multiple virtual hosts on the same IP and port are
+configured with a combination of On and Optional
+flags, connections will not be aborted if the header is not sent.
+Instead, enforcement will happen after the request is read so virtual
+hosts configured with On will return a 400 Bad Request.
+Virtual hosts configured with Optional will continue as
+usual but without replacing the client IP information
+
+
+Listen 80
+VirtualHost *:80
+ServerNamewww.example.com
+RemoteIPProxyProtocolEnable Optional
+
+#Requests to this virtual host may optionally not have
+# a proxy protocol header provided
+/VirtualHost
+
+VirtualHost *:80
+ServerNamewww.example.com
+RemoteIPProxyProtocolEnable On
+
+#Requests to this virtual host must have a proxy protocol
+# header provided. If it is missing, a 400 will result
+/VirtualHost
+
+Listen 8080
+VirtualHost *:8080
+ServerNamewww.example.com
+RemoteIPProxyProtocolEnable On
+
+#Requests to this virtual host must have a proxy protocol
+# header provided. If it is missing, the connection will
+# be aborted
+/VirtualHost
+
+
+
+
  
  RemoteIPTrustedProxy
  Restrict client IP addresses trusted to present the RemoteIPHeader 
value

Modified: httpd/httpd/trunk/modules/metadata/mod_remoteip.c

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Jim Jagielski]

> On Jan 16, 2017, at 6:57 PM, Daniel Ruggeri  wrote:
> 
> For the most part, yes except the portions that make the header presence
> optional (the HDR_MISSING case). Those were added as it came into the
> code base to handle a use case I was working on. I've added some
> comments inline since I won't have time to poke around myself for a
> while yet.
> 
> 
> For convenience, here's a link to the original code
> 
> https://github.com/roadrunner2/mod-proxy-protocol/blob/master/mod_proxy_protocol.c
> 

Would it make sense to have the "stable" version available
for backport, and keep in the WIP in trunk?

AW: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Plüm , Rüdiger , Vodafone Group]

> -Ursprüngliche Nachricht-
> Von: Daniel Ruggeri [mailto:drugg...@primary.net]
> Gesendet: Dienstag, 17. Januar 2017 00:57
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-
> message-tags/next-number docs/manual/mod/mod_remoteip.xml
> modules/metadata/mod_remoteip.c
> 
> For the most part, yes except the portions that make the header presence
> optional (the HDR_MISSING case). Those were added as it came into the
> code base to handle a use case I was working on. I've added some
> comments inline since I won't have time to poke around myself for a
> while yet.
> 
> 
> For convenience, here's a link to the original code
> 
> https://github.com/roadrunner2/mod-proxy-
> protocol/blob/master/mod_proxy_protocol.c
> 
> 
> On 1/16/2017 10:14 AM, Jim Jagielski wrote:
> > Let me take a look... afaict, this is a copy of what
> > was donated, which has been working for numerous people.
> > But that doesn't mean it can't have bugs ;)
> >
> >> On Jan 16, 2017, at 7:20 AM, Ruediger Pluem <rpl...@apache.org>
> wrote:
> >>
> >> Anyone?
> 
> Apologies for missing the original reply
> 
> >> Regards
> >>
> >> Rüdiger
> >>
> >> On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
> >>>
> >>> On 12/30/2016 03:20 PM, drugg...@apache.org wrote:
> >>>> Author: druggeri
> >>>> Date: Fri Dec 30 14:20:48 2016
> >>>> New Revision: 1776575
> >>>>
> >>>> URL: http://svn.apache.org/viewvc?rev=1776575=rev
> >>>> Log:
> >>>> Merge new PROXY protocol code into mod_remoteip
> >>>>
> >>>> Modified:
> >>>>httpd/httpd/trunk/docs/log-message-tags/next-number
> >>>>httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
> >>>>httpd/httpd/trunk/modules/metadata/mod_remoteip.c
> >>>>
> >>>>
> 
> ==
> >>>> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
> >>>> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30
> 14:20:48 2016
> >>>> + */
> >>>> +static int remoteip_hook_pre_connection(conn_rec *c, void *csd)
> >>>> +{
> >>>> +remoteip_config_t *conf;
> >>>> +remoteip_conn_config_t *conn_conf;
> >>>> +int optional;
> >>>> +
> >>>> +conf = ap_get_module_config(ap_server_conf->module_config,
> >>>> +_module);
> >>>> +
> >>>> +/* Used twice - do the check only once */
> >>>> +optional = remoteip_addr_in_list(conf-
> >proxy_protocol_optional, c->local_addr);
> >>>> +
> >>>> +/* check if we're enabled for this connection */
> >>>> +if ((!remoteip_addr_in_list(conf->proxy_protocol_enabled, c-
> >local_addr)
> >>>> +  && !optional )
> >>>> +|| remoteip_addr_in_list(conf->proxy_protocol_disabled, c-
> >local_addr)) {
> >>>> +return DECLINED;
> >>>> +}
> >>>> +
> >>>> +/* mod_proxy creates outgoing connections - we don't want
> those */
> >>>> +if (!remoteip_is_server_port(c->local_addr->port)) {
> >>>> +return DECLINED;
> >>>> +}
> >>> Why is the c->local_addr->port set to a port we are listening on in
> case of proxy connections?
> 
> This is from the original code, but wouldn't this be the local port on
> the outbound connection (some high number)? The remoteip_is_server_port
> should therefore fail this check.

My bad I missed the ! in the condition. So this should work.

> >>>> +
> >>>> +switch (psts) {
> >>>> +case HDR_MISSING:
> >>>> +if (conn_conf->proxy_protocol_optional) {
> >>>> +/* Same as DONE, but don't delete the
> bucket. Rather, put it
> >>>> +   back into the brigade and move the
> request along the stack */
> >>>> +ctx->done = 1;
> >>>> +APR_BRIGADE_INSERT_HEAD(bb_out, b);
> >>> See below. We need to restore all buckets. What if the original read
> was speculative?
> >>>
> >

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Daniel Ruggeri]

For the most part, yes except the portions that make the header presence
optional (the HDR_MISSING case). Those were added as it came into the
code base to handle a use case I was working on. I've added some
comments inline since I won't have time to poke around myself for a
while yet.


For convenience, here's a link to the original code

https://github.com/roadrunner2/mod-proxy-protocol/blob/master/mod_proxy_protocol.c


On 1/16/2017 10:14 AM, Jim Jagielski wrote:
> Let me take a look... afaict, this is a copy of what
> was donated, which has been working for numerous people.
> But that doesn't mean it can't have bugs ;)
>
>> On Jan 16, 2017, at 7:20 AM, Ruediger Pluem  wrote:
>>
>> Anyone?

Apologies for missing the original reply

>> Regards
>>
>> Rüdiger
>>
>> On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
>>>
>>> On 12/30/2016 03:20 PM, drugg...@apache.org wrote:
 Author: druggeri
 Date: Fri Dec 30 14:20:48 2016
 New Revision: 1776575

 URL: http://svn.apache.org/viewvc?rev=1776575=rev
 Log:
 Merge new PROXY protocol code into mod_remoteip

 Modified:
httpd/httpd/trunk/docs/log-message-tags/next-number
httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
httpd/httpd/trunk/modules/metadata/mod_remoteip.c

 ==
 --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
 +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 
 2016
 @@ -427,6 +730,464 @@ static int remoteip_modify_request(reque
 return OK;
 }

 +static int remoteip_is_server_port(apr_port_t port)
 +{
 +ap_listen_rec *lr;
 +
 +for (lr = ap_listeners; lr; lr = lr->next) {
 +if (lr->bind_addr && lr->bind_addr->port == port) {
 +return 1;
 +}
 +}
 +
 +return 0;
 +}
 +
 +/*
 + * Human readable format:
 + * PROXY {TCP4|TCP6|UNKNOWN}   
  
 + */
 +static remoteip_parse_status_t remoteip_process_v1_header(conn_rec *c,
 +  
 remoteip_conn_config_t *conn_conf,
 +  proxy_header 
 *hdr, apr_size_t len,
 +  apr_size_t 
 *hdr_len)
 +{
 +char *end, *word, *host, *valid_addr_chars, *saveptr;
 +char buf[sizeof(hdr->v1.line)];
 +apr_port_t port;
 +apr_status_t ret;
 +apr_int32_t family;
 +
 +#define GET_NEXT_WORD(field) \
 +word = apr_strtok(NULL, " ", ); \
 +if (!word) { \
 +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03497) \
 +  "RemoteIPProxyProtocol: no " field " found in 
 header '%s'", \
 +  hdr->v1.line); \
 +return HDR_ERROR; \
 +}
 +
 +end = memchr(hdr->v1.line, '\r', len - 1);
 +if (!end || end[1] != '\n') {
 +return HDR_NEED_MORE; /* partial or invalid header */
 +}
 +
 +*end = '\0';
 +*hdr_len = end + 2 - hdr->v1.line; /* skip header + CRLF */
 +
 +/* parse in separate buffer so have the original for error messages */
 +strcpy(buf, hdr->v1.line);
 +
 +apr_strtok(buf, " ", );
 +
 +/* parse family */
 +GET_NEXT_WORD("family")
 +if (strcmp(word, "UNKNOWN") == 0) {
 +conn_conf->client_addr = c->client_addr;
 +conn_conf->client_ip = c->client_ip;
 +return HDR_DONE;
 +}
 +else if (strcmp(word, "TCP4") == 0) {
 +family = APR_INET;
 +valid_addr_chars = "0123456789.";
 +}
 +else if (strcmp(word, "TCP6") == 0) {
 +#if APR_HAVE_IPV6
 +family = APR_INET6;
 +valid_addr_chars = "0123456789abcdefABCDEF:";
 +#else
 +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03498)
 +  "RemoteIPProxyProtocol: Unable to parse v6 address 
 - APR is not compiled with IPv6 support",
 +  word, hdr->v1.line);
 +return HDR_ERROR;
 +#endif
 +}
 +else {
 +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03499)
 +  "RemoteIPProxyProtocol: unknown family '%s' in 
 header '%s'",
 +  word, hdr->v1.line);
 +return HDR_ERROR;
 +}
 +
 +/* parse client-addr */
 +GET_NEXT_WORD("client-address")
 +
 +if (strspn(word, valid_addr_chars) != strlen(word)) {
 +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03500)
 +  "RemoteIPProxyProtocol: invalid client-address '%s' 
 found in "
 +  "header 

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Jim Jagielski]

Let me take a look... afaict, this is a copy of what
was donated, which has been working for numerous people.
But that doesn't mean it can't have bugs ;)

> On Jan 16, 2017, at 7:20 AM, Ruediger Pluem  wrote:
> 
> Anyone?
> 
> Regards
> 
> Rüdiger
> 
> On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
>> 
>> 
>> On 12/30/2016 03:20 PM, drugg...@apache.org wrote:
>>> Author: druggeri
>>> Date: Fri Dec 30 14:20:48 2016
>>> New Revision: 1776575
>>> 
>>> URL: http://svn.apache.org/viewvc?rev=1776575=rev
>>> Log:
>>> Merge new PROXY protocol code into mod_remoteip
>>> 
>>> Modified:
>>>httpd/httpd/trunk/docs/log-message-tags/next-number
>>>httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
>>>httpd/httpd/trunk/modules/metadata/mod_remoteip.c
>>> 
>> 
>>> ==
>>> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
>>> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 
>>> 2016
>> 
>>> @@ -427,6 +730,464 @@ static int remoteip_modify_request(reque
>>> return OK;
>>> }
>>> 
>>> +static int remoteip_is_server_port(apr_port_t port)
>>> +{
>>> +ap_listen_rec *lr;
>>> +
>>> +for (lr = ap_listeners; lr; lr = lr->next) {
>>> +if (lr->bind_addr && lr->bind_addr->port == port) {
>>> +return 1;
>>> +}
>>> +}
>>> +
>>> +return 0;
>>> +}
>>> +
>>> +/*
>>> + * Human readable format:
>>> + * PROXY {TCP4|TCP6|UNKNOWN}
>>> 
>>> + */
>>> +static remoteip_parse_status_t remoteip_process_v1_header(conn_rec *c,
>>> +  
>>> remoteip_conn_config_t *conn_conf,
>>> +  proxy_header 
>>> *hdr, apr_size_t len,
>>> +  apr_size_t 
>>> *hdr_len)
>>> +{
>>> +char *end, *word, *host, *valid_addr_chars, *saveptr;
>>> +char buf[sizeof(hdr->v1.line)];
>>> +apr_port_t port;
>>> +apr_status_t ret;
>>> +apr_int32_t family;
>>> +
>>> +#define GET_NEXT_WORD(field) \
>>> +word = apr_strtok(NULL, " ", ); \
>>> +if (!word) { \
>>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03497) \
>>> +  "RemoteIPProxyProtocol: no " field " found in header 
>>> '%s'", \
>>> +  hdr->v1.line); \
>>> +return HDR_ERROR; \
>>> +}
>>> +
>>> +end = memchr(hdr->v1.line, '\r', len - 1);
>>> +if (!end || end[1] != '\n') {
>>> +return HDR_NEED_MORE; /* partial or invalid header */
>>> +}
>>> +
>>> +*end = '\0';
>>> +*hdr_len = end + 2 - hdr->v1.line; /* skip header + CRLF */
>>> +
>>> +/* parse in separate buffer so have the original for error messages */
>>> +strcpy(buf, hdr->v1.line);
>>> +
>>> +apr_strtok(buf, " ", );
>>> +
>>> +/* parse family */
>>> +GET_NEXT_WORD("family")
>>> +if (strcmp(word, "UNKNOWN") == 0) {
>>> +conn_conf->client_addr = c->client_addr;
>>> +conn_conf->client_ip = c->client_ip;
>>> +return HDR_DONE;
>>> +}
>>> +else if (strcmp(word, "TCP4") == 0) {
>>> +family = APR_INET;
>>> +valid_addr_chars = "0123456789.";
>>> +}
>>> +else if (strcmp(word, "TCP6") == 0) {
>>> +#if APR_HAVE_IPV6
>>> +family = APR_INET6;
>>> +valid_addr_chars = "0123456789abcdefABCDEF:";
>>> +#else
>>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03498)
>>> +  "RemoteIPProxyProtocol: Unable to parse v6 address - 
>>> APR is not compiled with IPv6 support",
>>> +  word, hdr->v1.line);
>>> +return HDR_ERROR;
>>> +#endif
>>> +}
>>> +else {
>>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03499)
>>> +  "RemoteIPProxyProtocol: unknown family '%s' in 
>>> header '%s'",
>>> +  word, hdr->v1.line);
>>> +return HDR_ERROR;
>>> +}
>>> +
>>> +/* parse client-addr */
>>> +GET_NEXT_WORD("client-address")
>>> +
>>> +if (strspn(word, valid_addr_chars) != strlen(word)) {
>>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03500)
>>> +  "RemoteIPProxyProtocol: invalid client-address '%s' 
>>> found in "
>>> +  "header '%s'", word, hdr->v1.line);
>>> +return HDR_ERROR;
>>> +}
>>> +
>>> +host = word;
>>> +
>>> +/* parse dest-addr */
>>> +GET_NEXT_WORD("destination-address")
>>> +
>>> +/* parse client-port */
>>> +GET_NEXT_WORD("client-port")
>>> +if (sscanf(word, "%hu", ) != 1) {
>>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03501)
>>> +  "RemoteIPProxyProtocol: error parsing port '%s' in 
>>> header '%s'",
>>> +  word, hdr->v1.line);
>>> +return HDR_ERROR;
>>> +}
>>> +
>>> +/* parse dest-port */
>>> +/* 

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Ruediger Pluem]

Anyone?

Regards

Rüdiger

On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
> 
> 
> On 12/30/2016 03:20 PM, drugg...@apache.org wrote:
>> Author: druggeri
>> Date: Fri Dec 30 14:20:48 2016
>> New Revision: 1776575
>>
>> URL: http://svn.apache.org/viewvc?rev=1776575=rev
>> Log:
>> Merge new PROXY protocol code into mod_remoteip
>>
>> Modified:
>> httpd/httpd/trunk/docs/log-message-tags/next-number
>> httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
>> httpd/httpd/trunk/modules/metadata/mod_remoteip.c
>>
> 
>> ==
>> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
>> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 
>> 2016
> 
>> @@ -427,6 +730,464 @@ static int remoteip_modify_request(reque
>>  return OK;
>>  }
>>  
>> +static int remoteip_is_server_port(apr_port_t port)
>> +{
>> +ap_listen_rec *lr;
>> +
>> +for (lr = ap_listeners; lr; lr = lr->next) {
>> +if (lr->bind_addr && lr->bind_addr->port == port) {
>> +return 1;
>> +}
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +/*
>> + * Human readable format:
>> + * PROXY {TCP4|TCP6|UNKNOWN}
>> 
>> + */
>> +static remoteip_parse_status_t remoteip_process_v1_header(conn_rec *c,
>> +  
>> remoteip_conn_config_t *conn_conf,
>> +  proxy_header 
>> *hdr, apr_size_t len,
>> +  apr_size_t 
>> *hdr_len)
>> +{
>> +char *end, *word, *host, *valid_addr_chars, *saveptr;
>> +char buf[sizeof(hdr->v1.line)];
>> +apr_port_t port;
>> +apr_status_t ret;
>> +apr_int32_t family;
>> +
>> +#define GET_NEXT_WORD(field) \
>> +word = apr_strtok(NULL, " ", ); \
>> +if (!word) { \
>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03497) \
>> +  "RemoteIPProxyProtocol: no " field " found in header 
>> '%s'", \
>> +  hdr->v1.line); \
>> +return HDR_ERROR; \
>> +}
>> +
>> +end = memchr(hdr->v1.line, '\r', len - 1);
>> +if (!end || end[1] != '\n') {
>> +return HDR_NEED_MORE; /* partial or invalid header */
>> +}
>> +
>> +*end = '\0';
>> +*hdr_len = end + 2 - hdr->v1.line; /* skip header + CRLF */
>> +
>> +/* parse in separate buffer so have the original for error messages */
>> +strcpy(buf, hdr->v1.line);
>> +
>> +apr_strtok(buf, " ", );
>> +
>> +/* parse family */
>> +GET_NEXT_WORD("family")
>> +if (strcmp(word, "UNKNOWN") == 0) {
>> +conn_conf->client_addr = c->client_addr;
>> +conn_conf->client_ip = c->client_ip;
>> +return HDR_DONE;
>> +}
>> +else if (strcmp(word, "TCP4") == 0) {
>> +family = APR_INET;
>> +valid_addr_chars = "0123456789.";
>> +}
>> +else if (strcmp(word, "TCP6") == 0) {
>> +#if APR_HAVE_IPV6
>> +family = APR_INET6;
>> +valid_addr_chars = "0123456789abcdefABCDEF:";
>> +#else
>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03498)
>> +  "RemoteIPProxyProtocol: Unable to parse v6 address - 
>> APR is not compiled with IPv6 support",
>> +  word, hdr->v1.line);
>> +return HDR_ERROR;
>> +#endif
>> +}
>> +else {
>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03499)
>> +  "RemoteIPProxyProtocol: unknown family '%s' in header 
>> '%s'",
>> +  word, hdr->v1.line);
>> +return HDR_ERROR;
>> +}
>> +
>> +/* parse client-addr */
>> +GET_NEXT_WORD("client-address")
>> +
>> +if (strspn(word, valid_addr_chars) != strlen(word)) {
>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03500)
>> +  "RemoteIPProxyProtocol: invalid client-address '%s' 
>> found in "
>> +  "header '%s'", word, hdr->v1.line);
>> +return HDR_ERROR;
>> +}
>> +
>> +host = word;
>> +
>> +/* parse dest-addr */
>> +GET_NEXT_WORD("destination-address")
>> +
>> +/* parse client-port */
>> +GET_NEXT_WORD("client-port")
>> +if (sscanf(word, "%hu", ) != 1) {
>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03501)
>> +  "RemoteIPProxyProtocol: error parsing port '%s' in 
>> header '%s'",
>> +  word, hdr->v1.line);
>> +return HDR_ERROR;
>> +}
>> +
>> +/* parse dest-port */
>> +/* GET_NEXT_WORD("destination-port") - no-op since we don't care about 
>> it */
>> +
>> +/* create a socketaddr from the info */
>> +ret = apr_sockaddr_info_get(_conf->client_addr, host, family, 
>> port, 0,
>> +c->pool);
>> +if (ret != APR_SUCCESS) {
>> +conn_conf->client_addr = NULL;
>> +ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, 

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Ruediger Pluem]

On 12/30/2016 03:20 PM, drugg...@apache.org wrote:
> Author: druggeri
> Date: Fri Dec 30 14:20:48 2016
> New Revision: 1776575
> 
> URL: http://svn.apache.org/viewvc?rev=1776575=rev
> Log:
> Merge new PROXY protocol code into mod_remoteip
> 
> Modified:
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
> httpd/httpd/trunk/modules/metadata/mod_remoteip.c
> 

> ==
> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 2016

> @@ -427,6 +730,464 @@ static int remoteip_modify_request(reque
>  return OK;
>  }
>  
> +static int remoteip_is_server_port(apr_port_t port)
> +{
> +ap_listen_rec *lr;
> +
> +for (lr = ap_listeners; lr; lr = lr->next) {
> +if (lr->bind_addr && lr->bind_addr->port == port) {
> +return 1;
> +}
> +}
> +
> +return 0;
> +}
> +
> +/*
> + * Human readable format:
> + * PROXY {TCP4|TCP6|UNKNOWN}
> 
> + */
> +static remoteip_parse_status_t remoteip_process_v1_header(conn_rec *c,
> +  
> remoteip_conn_config_t *conn_conf,
> +  proxy_header *hdr, 
> apr_size_t len,
> +  apr_size_t 
> *hdr_len)
> +{
> +char *end, *word, *host, *valid_addr_chars, *saveptr;
> +char buf[sizeof(hdr->v1.line)];
> +apr_port_t port;
> +apr_status_t ret;
> +apr_int32_t family;
> +
> +#define GET_NEXT_WORD(field) \
> +word = apr_strtok(NULL, " ", ); \
> +if (!word) { \
> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03497) \
> +  "RemoteIPProxyProtocol: no " field " found in header 
> '%s'", \
> +  hdr->v1.line); \
> +return HDR_ERROR; \
> +}
> +
> +end = memchr(hdr->v1.line, '\r', len - 1);
> +if (!end || end[1] != '\n') {
> +return HDR_NEED_MORE; /* partial or invalid header */
> +}
> +
> +*end = '\0';
> +*hdr_len = end + 2 - hdr->v1.line; /* skip header + CRLF */
> +
> +/* parse in separate buffer so have the original for error messages */
> +strcpy(buf, hdr->v1.line);
> +
> +apr_strtok(buf, " ", );
> +
> +/* parse family */
> +GET_NEXT_WORD("family")
> +if (strcmp(word, "UNKNOWN") == 0) {
> +conn_conf->client_addr = c->client_addr;
> +conn_conf->client_ip = c->client_ip;
> +return HDR_DONE;
> +}
> +else if (strcmp(word, "TCP4") == 0) {
> +family = APR_INET;
> +valid_addr_chars = "0123456789.";
> +}
> +else if (strcmp(word, "TCP6") == 0) {
> +#if APR_HAVE_IPV6
> +family = APR_INET6;
> +valid_addr_chars = "0123456789abcdefABCDEF:";
> +#else
> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03498)
> +  "RemoteIPProxyProtocol: Unable to parse v6 address - 
> APR is not compiled with IPv6 support",
> +  word, hdr->v1.line);
> +return HDR_ERROR;
> +#endif
> +}
> +else {
> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03499)
> +  "RemoteIPProxyProtocol: unknown family '%s' in header 
> '%s'",
> +  word, hdr->v1.line);
> +return HDR_ERROR;
> +}
> +
> +/* parse client-addr */
> +GET_NEXT_WORD("client-address")
> +
> +if (strspn(word, valid_addr_chars) != strlen(word)) {
> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03500)
> +  "RemoteIPProxyProtocol: invalid client-address '%s' 
> found in "
> +  "header '%s'", word, hdr->v1.line);
> +return HDR_ERROR;
> +}
> +
> +host = word;
> +
> +/* parse dest-addr */
> +GET_NEXT_WORD("destination-address")
> +
> +/* parse client-port */
> +GET_NEXT_WORD("client-port")
> +if (sscanf(word, "%hu", ) != 1) {
> +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03501)
> +  "RemoteIPProxyProtocol: error parsing port '%s' in 
> header '%s'",
> +  word, hdr->v1.line);
> +return HDR_ERROR;
> +}
> +
> +/* parse dest-port */
> +/* GET_NEXT_WORD("destination-port") - no-op since we don't care about 
> it */
> +
> +/* create a socketaddr from the info */
> +ret = apr_sockaddr_info_get(_conf->client_addr, host, family, port, 
> 0,
> +c->pool);
> +if (ret != APR_SUCCESS) {
> +conn_conf->client_addr = NULL;
> +ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03502)
> +  "RemoteIPProxyProtocol: error converting family '%d', 
> host '%s',"
> +  " and port '%hu' to sockaddr; header was '%s'",
> +  family, host, port, 

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Daniel Ruggeri]

On 12/30/2016 8:37 PM, William A Rowe Jr wrote:
> Simply address the issue that caused the -1... 'Code mostly
> copyright...' needs to be 'Portions copyright'... A statement which is
> unlikely to become entirely false.

OK, got it. It wasn't entirely clear to me that was the hangup.


On 12/30/2016 8:34 PM, Eric Covener wrote:
> Hi Daniel, not to pull you in too many directions, but I would suggest
> to revert r1776674 and go back to erring on the side of attribution
> until we can arrive at a consensus.The license requires that
> copyright notices are preserved in derivative works.

Understood. Since it was a lapse in my initial commit that I didn't add
to CHANGES already, I'd like to keep r1776674 to cover that mistake, but
adjust mod_remoteip.c as Bill mentions.

I will do that later this evening or in the morning in the absence of
any disagreement.

-- 
Daniel Ruggeri

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[William A Rowe Jr]

Simply address the issue that caused the -1... 'Code mostly copyright...'
needs to be 'Portions copyright'... A statement which is unlikely to become
entirely false.


On Dec 30, 2016 18:28, "Daniel Ruggeri"  wrote:

> Aye - I suspected this would raise eyebrows so I did bring it up a few
> times [1][2]. I'm sure we're in agreement that attribution is important
> in the Open Source world so I'd like to be sure it's done appropriately.
> I'm happy to fix.
>
> Currently, though, I'm not sure how best to handle this veto... what
> would be the preferred path forward? As a first step, I've remove the
> three lines mentioned here and added to CHANGES in r1776674. The 2.4
> backport has also been modified (simply by removing the lines since
> CHANGES already has attribution to the original authors which seems to
> be preferred per your message).
>
> Does that work or did you have another approach in mind?
>
> [1]
> https://lists.apache.org/thread.html/95173df808992097f565bc7bcd06a0
> 1b2129415bff0aae6c608b82fe@%3Cdev.httpd.apache.org%3E
> [2]
> https://lists.apache.org/thread.html/28e660f38d945216d9d0bb4cba3e1b
> 4336a4c5051a46f17c8f99a0f0@%3Cdev.httpd.apache.org%3E
>
>
> --
> Daniel Ruggeri
>
> On 12/30/2016 8:00 PM, William A Rowe Jr wrote:
> > -1 (yes, veto.)
> >
> > In general, as the original author of this particular module, you
> > might notice I don't claim attribution. We rely on svn provenance and
> > Changes to describe code legacy, so these make me very uncomfortable,
> > especially when injected into our sources. That isn't the key issue.
> >
> > The statement itself may be presently true. The statement a number of
> > years from now might be radically false. The presence of this
> > statement makes it impossible for the future svn hacker to know when
> > to modify the statement or determine when the sources have changed
> > such that it becomes untrue.
> >
> > It is a relativistic value judgement, and these aren't useful as
> > legalistic elements, so the conventional 'portions copyright...' sort
> > of language is used instead.
>
>

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Eric Covener]

On Fri, Dec 30, 2016 at 9:27 PM, Daniel Ruggeri  wrote:
> Aye - I suspected this would raise eyebrows so I did bring it up a few
> times [1][2]. I'm sure we're in agreement that attribution is important
> in the Open Source world so I'd like to be sure it's done appropriately.
> I'm happy to fix.
>
> Currently, though, I'm not sure how best to handle this veto... what
> would be the preferred path forward? As a first step, I've remove the
> three lines mentioned here and added to CHANGES in r1776674. The 2.4
> backport has also been modified (simply by removing the lines since
> CHANGES already has attribution to the original authors which seems to
> be preferred per your message).
>
> Does that work or did you have another approach in mind?

Hi Daniel, not to pull you in too many directions, but I would suggest
to revert r1776674 and go back to erring on the side of attribution
until we can arrive at a consensus.The license requires that
copyright notices are preserved in derivative works.

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Daniel Ruggeri]

Aye - I suspected this would raise eyebrows so I did bring it up a few
times [1][2]. I'm sure we're in agreement that attribution is important
in the Open Source world so I'd like to be sure it's done appropriately.
I'm happy to fix.

Currently, though, I'm not sure how best to handle this veto... what
would be the preferred path forward? As a first step, I've remove the
three lines mentioned here and added to CHANGES in r1776674. The 2.4
backport has also been modified (simply by removing the lines since
CHANGES already has attribution to the original authors which seems to
be preferred per your message).

Does that work or did you have another approach in mind?

[1]
https://lists.apache.org/thread.html/95173df808992097f565bc7bcd06a01b2129415bff0aae6c608b82fe@%3Cdev.httpd.apache.org%3E
[2]
https://lists.apache.org/thread.html/28e660f38d945216d9d0bb4cba3e1b4336a4c5051a46f17c8f99a0f0@%3Cdev.httpd.apache.org%3E


-- 
Daniel Ruggeri

On 12/30/2016 8:00 PM, William A Rowe Jr wrote:
> -1 (yes, veto.)
>
> In general, as the original author of this particular module, you
> might notice I don't claim attribution. We rely on svn provenance and
> Changes to describe code legacy, so these make me very uncomfortable,
> especially when injected into our sources. That isn't the key issue.
>
> The statement itself may be presently true. The statement a number of
> years from now might be radically false. The presence of this
> statement makes it impossible for the future svn hacker to know when
> to modify the statement or determine when the sources have changed
> such that it becomes untrue.
>
> It is a relativistic value judgement, and these aren't useful as
> legalistic elements, so the conventional 'portions copyright...' sort
> of language is used instead.

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Eric Covener]

On Fri, Dec 30, 2016 at 9:00 PM, William A Rowe Jr  wrote:
> On Dec 30, 2016 06:20,  wrote:
>
> Author: druggeri
> Date: Fri Dec 30 14:20:48 2016
> New Revision: 1776575
>
> URL: http://svn.apache.org/viewvc?rev=1776575=rev
> Log:
> Merge new PROXY protocol code into mod_remoteip
>
> Modified:
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
> httpd/httpd/trunk/modules/metadata/mod_remoteip.c
>
>
> ==
> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48
> 2016
> @@ -12,15 +12,20 @@
>   * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>   * See the License for the specific language governing permissions and
>   * limitations under the License.
> + *
> + * The majority of the input filter code for PROXY protocol support is
> + * Copyright 2014 Cloudzilla Inc.
>   */
>
>
> -1 (yes, veto.)
>
> In general, as the original author of this particular module, you might
> notice I don't claim attribution. We rely on svn provenance and Changes to
> describe code legacy, so these make me very uncomfortable, especially when
> injected into our sources. That isn't the key issue.
>
> The statement itself may be presently true. The statement a number of years
> from now might be radically false. The presence of this statement makes it
> impossible for the future svn hacker to know when to modify the statement or
> determine when the sources have changed such that it becomes untrue.
>
> It is a relativistic value judgement, and these aren't useful as legalistic
> elements, so the conventional 'portions copyright...' sort of language is
> used instead.

Maybe something like this (for a start?)I think we have to keep
something here.

Index: modules/metadata/mod_remoteip.c
===
--- modules/metadata/mod_remoteip.c (revision 1776673)
+++ modules/metadata/mod_remoteip.c (working copy)
@@ -13,8 +13,9 @@
 * See the License for the specific language governing permissions and
 * limitations under the License.
 *
- * The majority of the input filter code for PROXY protocol support is
- * Copyright 2014 Cloudzilla Inc.
+ * Portions Copyright 2014 Cloudzilla Inc.:
+ *   remoteip_input_filter() based on mod_proxy_protocol
+ *   https://github.com/roadrunner2/mod-proxy-protocol 62d2df6
 */

#include "ap_config.h"


-- 
Eric Covener
cove...@gmail.com

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[William A Rowe Jr]

On Dec 30, 2016 06:20,  wrote:

Author: druggeri
Date: Fri Dec 30 14:20:48 2016
New Revision: 1776575

URL: http://svn.apache.org/viewvc?rev=1776575=rev
Log:
Merge new PROXY protocol code into mod_remoteip

Modified:
httpd/httpd/trunk/docs/log-message-tags/next-number
httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
httpd/httpd/trunk/modules/metadata/mod_remoteip.c



==
--- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
+++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48
2016
@@ -12,15 +12,20 @@
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
+ *
+ * The majority of the input filter code for PROXY protocol support is
+ * Copyright 2014 Cloudzilla Inc.
  */


-1 (yes, veto.)

In general, as the original author of this particular module, you might
notice I don't claim attribution. We rely on svn provenance and Changes to
describe code legacy, so these make me very uncomfortable, especially when
injected into our sources. That isn't the key issue.

The statement itself may be presently true. The statement a number of years
from now might be radically false. The presence of this statement makes it
impossible for the future svn hacker to know when to modify the statement
or determine when the sources have changed such that it becomes untrue.

It is a relativistic value judgement, and these aren't useful as legalistic
elements, so the conventional 'portions copyright...' sort of language is
used instead.

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Eric Covener]

On Fri, Dec 30, 2016 at 10:22 AM, Daniel Ruggeri  wrote:
> I do like the idea of being nice to end users... but this may be a dumb
> followup question showing a lack of understanding. How would we trigger
> behavior in two different modules for the same directive unless the
> directive were added to core?


It's rarely used, but config processor will let multiple modules share
a directive. They both get called when it's read and then manage it in
their own conf from there independently.

-- 
Eric Covener
cove...@gmail.com

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Daniel Ruggeri]

On 12/30/2016 9:04 AM, Jim Jagielski wrote:
> I was thinking something like
>
>   ProxyProtocolEnable Incoming | Outgoing | Both | Optional | Off
>
> so that we have 1 command for all possible PROXY PROTOCOL usages.
> This, I think, is clearer for end-users.
I do like the idea of being nice to end users... but this may be a dumb
followup question showing a lack of understanding. How would we trigger
behavior in two different modules for the same directive unless the
directive were added to core?

> However, I also see the value in having it as a ProxySet
> parameter for outgoing, so I'm fine whichever way ;)
Actually, I didn't even consider this as an option but *really* like the
idea. It aligns with how we handle flags for outbound behavior already
plus aligns with how HAProxy handles it (on your server declaration you
simply add the "send-proxy" flag). Not that we have to work the same as
another server out there, but as mentioned above, it's nice to be
friendly to the operator by being consistent with expectations.

Between the options of an all-in-one directive and a directive for
incoming with a flag for outgoing, I definitely like the latter option.

-- 
Daniel Ruggeri

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Jim Jagielski]

Yeah, the "namespace" stuff makes sense, but really, when
you think of it, the PROXY support is, theoretically at least,
independent of RemoteIP, even though that module is the one
that implements it. It makes it seem like it is "different"
in some way to PROXY...

I was thinking something like

  ProxyProtocolEnable Incoming | Outgoing | Both | Optional | Off

so that we have 1 command for all possible PROXY PROTOCOL usages.
This, I think, is clearer for end-users.

However, I also see the value in having it as a ProxySet
parameter for outgoing, so I'm fine whichever way ;)

On Dec 30, 2016, at 9:41 AM, Daniel Ruggeri  wrote:
> 
> 
> On 12/30/2016 8:28 AM, Jim Jagielski wrote:
>> Kewl beans!
> Indeed - the best beans to have!
> 
>> Any issue if we rename the directive to just ProxyProtocolEnable?
>> The "RemoteIP" prefix just seems weird :)
> I assume we try to keep a "namespace" for the more optional modules like
> this. It does seem weird and is unnecessarily long but starting with
> "Proxy" would lead me to think this directive _belongs_ to one of the
> mod_proxy modules.
> 
>> Also, just as a head's up, I'm looking on adding PROXY support
>> to the proxy module itself (that is, we *send* the PROXY line
>> to backends) as a configurable option. So when that
>> happens, we may wish to rethink the command again.
> Yes, I planned the same (not cookie licking! It's all yours if you want
> it!) which kinda makes me like the RemoteIP prefix on this new directive
> to further differentiate it from the client-side work of mod_proxy_*. In
> the case of mod_proxy sending the header, a directive like
> "ProxySendProxyProtocolHeader On" seems like a viable option (again,
> long... and kinda redundant... but "clear" in my mind who's doing what).
> 
> Pending further responses to the followup I sent a moment ago, I'll do
> another commit for final cleanup and to remove the new module now ported
> to remoteip and will then work on a backport for 2.4
> 
> -- 
> Daniel Ruggeri
> 

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Daniel Ruggeri]

On 12/30/2016 8:28 AM, Jim Jagielski wrote:
> Kewl beans!
Indeed - the best beans to have!

> Any issue if we rename the directive to just ProxyProtocolEnable?
> The "RemoteIP" prefix just seems weird :)
I assume we try to keep a "namespace" for the more optional modules like
this. It does seem weird and is unnecessarily long but starting with
"Proxy" would lead me to think this directive _belongs_ to one of the
mod_proxy modules.

> Also, just as a head's up, I'm looking on adding PROXY support
> to the proxy module itself (that is, we *send* the PROXY line
> to backends) as a configurable option. So when that
> happens, we may wish to rethink the command again.
Yes, I planned the same (not cookie licking! It's all yours if you want
it!) which kinda makes me like the RemoteIP prefix on this new directive
to further differentiate it from the client-side work of mod_proxy_*. In
the case of mod_proxy sending the header, a directive like
"ProxySendProxyProtocolHeader On" seems like a viable option (again,
long... and kinda redundant... but "clear" in my mind who's doing what).

Pending further responses to the followup I sent a moment ago, I'll do
another commit for final cleanup and to remove the new module now ported
to remoteip and will then work on a backport for 2.4

-- 
Daniel Ruggeri

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Daniel Ruggeri]

I went ahead and ported this code to mod_remoteip. I also taught it how
to be optional (process PROXY headers when available, but don't fail
otherwise), checks for IPv6 in APR as well as log numbers. A small bit
of refactoring was included.
A few questions came up during initial code review and the porting...
opinions would be appreciated.

On 12/30/2016 8:20 AM, drugg...@apache.org wrote:
> Author: druggeri
> Date: Fri Dec 30 14:20:48 2016
> New Revision: 1776575
>
> URL: http://svn.apache.org/viewvc?rev=1776575=rev
> Log:
> Merge new PROXY protocol code into mod_remoteip
>
> Modified:
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
> httpd/httpd/trunk/modules/metadata/mod_remoteip.c
. . .

> Modified: httpd/httpd/trunk/modules/metadata/mod_remoteip.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/metadata/mod_remoteip.c?rev=1776575=1776574=1776575=diff
> ==
> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 2016
> @@ -12,15 +12,20 @@
>   * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>   * See the License for the specific language governing permissions and
>   * limitations under the License.
> + *
> + * The majority of the input filter code for PROXY protocol support is
> + * Copyright 2014 Cloudzilla Inc.
>   */
Is this sufficient attribution?


> +/* XXX: Unsure if this is needed if v6 support is not available on
> +   this platform */
> +#ifndef INET6_ADDRSTRLEN
> +#define INET6_ADDRSTRLEN 46
> +#endif
I don't have a non-IPv6 capable platform so I'm not sure if this will be
defined anyway. Is this needed?


> +
> +/* add our filter */
> +if (!ap_add_input_filter(remoteip_filter_name, NULL, NULL, c)) {
> +/* XXX: Shouldn't this WARN in log? */
> +return DECLINED;
> +}
This came from the original code... but I assume that if the admin
desired for this to be configured, but we failed to enable it... we
should at least complain. Maybe more?

-- 
Daniel Ruggeri

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Jim Jagielski]

Kewl beans!

Any issue if we rename the directive to just ProxyProtocolEnable?
The "RemoteIP" prefix just seems weird :)

Also, just as a head's up, I'm looking on adding PROXY support
to the proxy module itself (that is, we *send* the PROXY line
to backends) as a configurable option. So when that
happens, we may wish to rethink the command again.