An ask for eyes on proposal

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

An ask for eyes on proposal

Daniel Ruggeri
Hi, all;
With the proposal to T&R set for Monday, I wanted to draw attention to the PROXY protocol proposal in STATUS. Just hoping for a quick review. I know it appears to be a large change, but as I worked through the feedback, ten of the commits effectively got coded out. What we are left with is essentially just the donated code + safety around IPv6 + the ability to designate subnets that do not get PROXY processing.

This code has been around a while and I think it would be nice if we could incorporate the donated code in the first release since being donated.

Cheers
--
Daniel Ruggeri
Reply | Threaded
Open this post in threaded view
|

Re: An ask for eyes on proposal

William A Rowe Jr
FYI the one change I've been considering is to extend the

On Thu, Jun 8, 2017 at 12:30 PM, Daniel Ruggeri <[hidden email]> wrote:

> Hi, all;
> With the proposal to T&R set for Monday, I wanted to draw attention to the
> PROXY protocol proposal in STATUS. Just hoping for a quick review. I know it
> appears to be a large change, but as I worked through the feedback, ten of
> the commits effectively got coded out. What we are left with is essentially
> just the donated code + safety around IPv6 + the ability to designate
> subnets that do not get PROXY processing.
>
> This code has been around a while and I think it would be nice if we could
> incorporate the donated code in the first release since being donated.
>
> Cheers
> --
> Daniel Ruggeri
Reply | Threaded
Open this post in threaded view
|

Re: An ask for eyes on proposal

William A Rowe Jr
In reply to this post by Daniel Ruggeri
[Again, using all the words]

On Thu, Jun 8, 2017 at 12:30 PM, Daniel Ruggeri <[hidden email]> wrote:
> Hi, all;
> With the proposal to T&R set for Monday, I wanted to draw attention to the
> PROXY protocol proposal in STATUS. Just hoping for a quick review. I know it
> appears to be a large change, but as I worked through the feedback, ten of
> the commits effectively got coded out. What we are left with is essentially
> just the donated code + safety around IPv6 + the ability to designate
> subnets that do not get PROXY processing.

The one change I've been considering is to expand this syntax;

RemoteIPProxyProtocol On|Off

to

RemoteIPProxyProtocol [On|Off|host|range [host|range]...]

Rather than rely on RemoteIPProxyProtocolExceptions (which
was a great addition, thank you), I like configuring systems
with whitelists rather than blacklists, when available. Although
it's nebulous which is the whitelist and which is the blacklist,
in this case :)
Reply | Threaded
Open this post in threaded view
|

Re: An ask for eyes on proposal

Jim Jagielski
Is expansion of the syntax something that could be folded in
for 2.4.27?

> On Jun 8, 2017, at 2:51 PM, William A Rowe Jr <[hidden email]> wrote:
>
> [Again, using all the words]
>
> On Thu, Jun 8, 2017 at 12:30 PM, Daniel Ruggeri <[hidden email]> wrote:
>> Hi, all;
>> With the proposal to T&R set for Monday, I wanted to draw attention to the
>> PROXY protocol proposal in STATUS. Just hoping for a quick review. I know it
>> appears to be a large change, but as I worked through the feedback, ten of
>> the commits effectively got coded out. What we are left with is essentially
>> just the donated code + safety around IPv6 + the ability to designate
>> subnets that do not get PROXY processing.
>
> The one change I've been considering is to expand this syntax;
>
> RemoteIPProxyProtocol On|Off
>
> to
>
> RemoteIPProxyProtocol [On|Off|host|range [host|range]...]
>
> Rather than rely on RemoteIPProxyProtocolExceptions (which
> was a great addition, thank you), I like configuring systems
> with whitelists rather than blacklists, when available. Although
> it's nebulous which is the whitelist and which is the blacklist,
> in this case :)

Reply | Threaded
Open this post in threaded view
|

Re: An ask for eyes on proposal

William A Rowe Jr
AIUI, yes, since the absolute 1-arg on|off boolean syntax would be
preserved. Those would be disallowed for other patterns (e.g. any
IP-looking thing subsumes and precludes using the first pattern.)
'On' devolves to 0.0.0.0/32 (any match).

Just pointing out I'm still not convinced it's entirely fleshed out, but
am certainly not going to object if others finish reviewing the patch
and feature at whatever level of progress it's at. I wouldn't vote for
it for 2.4.27 either, but that's simply our difference of opinion on
creeping featurism; I won't stand in the way, but won't participate.

Cheers,

Bill



On Thu, Jun 8, 2017 at 4:46 PM, Jim Jagielski <[hidden email]> wrote:

> Is expansion of the syntax something that could be folded in
> for 2.4.27?
>
>> On Jun 8, 2017, at 2:51 PM, William A Rowe Jr <[hidden email]> wrote:
>>
>> [Again, using all the words]
>>
>> On Thu, Jun 8, 2017 at 12:30 PM, Daniel Ruggeri <[hidden email]> wrote:
>>> Hi, all;
>>> With the proposal to T&R set for Monday, I wanted to draw attention to the
>>> PROXY protocol proposal in STATUS. Just hoping for a quick review. I know it
>>> appears to be a large change, but as I worked through the feedback, ten of
>>> the commits effectively got coded out. What we are left with is essentially
>>> just the donated code + safety around IPv6 + the ability to designate
>>> subnets that do not get PROXY processing.
>>
>> The one change I've been considering is to expand this syntax;
>>
>> RemoteIPProxyProtocol On|Off
>>
>> to
>>
>> RemoteIPProxyProtocol [On|Off|host|range [host|range]...]
>>
>> Rather than rely on RemoteIPProxyProtocolExceptions (which
>> was a great addition, thank you), I like configuring systems
>> with whitelists rather than blacklists, when available. Although
>> it's nebulous which is the whitelist and which is the blacklist,
>> in this case :)
>
Reply | Threaded
Open this post in threaded view
|

Re: An ask for eyes on proposal

Sander Hoentjen
In reply to this post by Daniel Ruggeri
On 06/08/2017 07:30 PM, Daniel Ruggeri wrote:

> Hi, all;
> With the proposal to T&R set for Monday, I wanted to draw attention to
> the PROXY protocol proposal in STATUS. Just hoping for a quick review.
> I know it appears to be a large change, but as I worked through the
> feedback, ten of the commits effectively got coded out. What we are
> left with is essentially just the donated code + safety around IPv6 +
> the ability to designate subnets that do not get PROXY processing.
>
> This code has been around a while and I think it would be nice if we
> could incorporate the donated code in the first release since being
> donated.
>
Hi,

While I know I don't have much say in this, since I never really
contributed much I still believe it would be better to specify enabling
Proxy Protocol on a server, not vhost level. Because well, once you
enable it in one vhost it gets enabled for all vhosts using that port/ip
combination.

Here is what I said before about it:

Right now the patch proposes RemoteIPProxyProtocol inside a vhost config, but wouldn't it be better (since it is connection-specific) to have something like a ProxyProtocolListen directive? Where you say instead of:
------
<VirtualHost 127.0.0.1:9001>
RemoteIPProxyProtocol On
</VirtualHost>
------
Something like:
------
ProxyProtocolListen 127.0.0.1:9001
or
ProxyProtocolEnable 127.0.0.1:9001
------

IMHO this is much cleaner than within a vhost (because that has side-effects on other vhosts as well)

What do you guys think?

Regards,
Sander

Reply | Threaded
Open this post in threaded view
|

Re: An ask for eyes on proposal

William A Rowe Jr
On Fri, Jun 9, 2017 at 4:17 AM, Sander Hoentjen <[hidden email]> wrote:

> On 06/08/2017 07:30 PM, Daniel Ruggeri wrote:
>> Hi, all;
>> With the proposal to T&R set for Monday, I wanted to draw attention to
>> the PROXY protocol proposal in STATUS. Just hoping for a quick review.
>> I know it appears to be a large change, but as I worked through the
>> feedback, ten of the commits effectively got coded out. What we are
>> left with is essentially just the donated code + safety around IPv6 +
>> the ability to designate subnets that do not get PROXY processing.
>
> [...] I still believe it would be better to specify enabling
> Proxy Protocol on a server, not vhost level. Because well, once you
> enable it in one vhost it gets enabled for all vhosts using that port/ip
> combination.
>
> Here is what I said before about it:
>
> Right now the patch proposes RemoteIPProxyProtocol inside a vhost config, but wouldn't it be better (since it is connection-specific) to have something like a ProxyProtocolListen directive? Where you say instead of:
> ------
> <VirtualHost 127.0.0.1:9001>
> RemoteIPProxyProtocol On
> </VirtualHost>
> ------
> Something like:
> ------
> ProxyProtocolListen 127.0.0.1:9001
> or
> ProxyProtocolEnable 127.0.0.1:9001
> ------
>
> IMHO this is much cleaner than within a vhost (because that has side-effects on other vhosts as well)

As this lives in mod_remoteip (for better or worse) let's look at what
context mod_remoteip is configured in; we set up a list of those local
or global *client* IP's which we trust to provide legit x-f-f (or remote-ip
or otherly named) true IP address header fields.

in the PROXY protocol case, we configure which *client* IP's which
we *require* to submit a PROXY protocol line. Right now, we do that
as a RemoteIPProxyProtocolExceptions list of those which we do *not*
allow to submit a PROXY protocol line. I proposed we make the config
simpler, in theory, by listing those we will trust.

To your example, the *global* config line;

RemoteIPProxyProtocol 127.0.0.1  [or 127.0.0.0/24]

would configure all locally routed *client* requests, irrespective of
which by-IP vhost, to require the PROXY protocol line. Requests
from other hosts would be denied.

I think that's sufficient. But if we wanted to implement your basic
idea, we would still have the complication that we need to infer
whether 9001 is a http, https, or h2 listener following the PROXY
line processing. Your proposed syntax didn't really touch on that.

It is still possible to override behavior by-vhost (ip-based, we are
unprepared to read the TLS SNI or Host header at that point)
but I don't see any application to do so. A given client is either
an haproxy or similar, or it is not.
Reply | Threaded
Open this post in threaded view
|

Re: An ask for eyes on proposal

William A Rowe Jr
On Fri, Jun 9, 2017 at 8:29 AM, William A Rowe Jr <[hidden email]> wrote:
>
> To your example, the *global* config line;
>
> RemoteIPProxyProtocol 127.0.0.1  [or 127.0.0.0/24]
>
> would configure all locally routed *client* requests, irrespective of
> which by-IP vhost, to require the PROXY protocol line. Requests
> from other hosts would be denied.

... would not process the globally routed request for a PROXY line, I meant.
Reply | Threaded
Open this post in threaded view
|

Re: An ask for eyes on proposal

Sander Hoentjen
In reply to this post by William A Rowe Jr


On 06/09/2017 03:29 PM, William A Rowe Jr wrote:

> On Fri, Jun 9, 2017 at 4:17 AM, Sander Hoentjen <[hidden email]> wrote:
>> On 06/08/2017 07:30 PM, Daniel Ruggeri wrote:
>>> Hi, all;
>>> With the proposal to T&R set for Monday, I wanted to draw attention to
>>> the PROXY protocol proposal in STATUS. Just hoping for a quick review.
>>> I know it appears to be a large change, but as I worked through the
>>> feedback, ten of the commits effectively got coded out. What we are
>>> left with is essentially just the donated code + safety around IPv6 +
>>> the ability to designate subnets that do not get PROXY processing.
>> [...] I still believe it would be better to specify enabling
>> Proxy Protocol on a server, not vhost level. Because well, once you
>> enable it in one vhost it gets enabled for all vhosts using that port/ip
>> combination.
>>
>> Here is what I said before about it:
>>
>> Right now the patch proposes RemoteIPProxyProtocol inside a vhost config, but wouldn't it be better (since it is connection-specific) to have something like a ProxyProtocolListen directive? Where you say instead of:
>> ------
>> <VirtualHost 127.0.0.1:9001>
>> RemoteIPProxyProtocol On
>> </VirtualHost>
>> ------
>> Something like:
>> ------
>> ProxyProtocolListen 127.0.0.1:9001
>> or
>> ProxyProtocolEnable 127.0.0.1:9001
>> ------
>>
>> IMHO this is much cleaner than within a vhost (because that has side-effects on other vhosts as well)
> As this lives in mod_remoteip (for better or worse) let's look at what
> context mod_remoteip is configured in; we set up a list of those local
> or global *client* IP's which we trust to provide legit x-f-f (or remote-ip
> or otherly named) true IP address header fields.
>
> in the PROXY protocol case, we configure which *client* IP's which
> we *require* to submit a PROXY protocol line. Right now, we do that
> as a RemoteIPProxyProtocolExceptions list of those which we do *not*
> allow to submit a PROXY protocol line. I proposed we make the config
> simpler, in theory, by listing those we will trust.
>
> To your example, the *global* config line;
>
> RemoteIPProxyProtocol 127.0.0.1  [or 127.0.0.0/24]
>
> would configure all locally routed *client* requests, irrespective of
> which by-IP vhost, to require the PROXY protocol line. Requests
> from other hosts would not process the globally routed request for a PROXY line.
Now I'm not sure if I understand you. I agree with you that a whitelist
is better than a blacklist, so adding

`RemoteIPProxyProtocol` as a list of *clients* that are allowed to use the *server/port* that is specified in `ProxyProtocolEnable` would make sense to me.

>
> I think that's sufficient. But if we wanted to implement your basic
> idea, we would still have the complication that we need to infer
> whether 9001 is a http, https, or h2 listener following the PROXY
> line processing. Your proposed syntax didn't really touch on that.
No because it doesn't need to. Because proxy protocol is prepended all
that needs to be done is strip it, and store the information for later
use. So you just configure that as normal.
>
> It is still possible to override behavior by-vhost (ip-based, we are
> unprepared to read the TLS SNI or Host header at that point)
> but I don't see any application to do so. A given client is either
> an haproxy or similar, or it is not.
>
Well yes, I guess my main point is that the behaviour is set for a
server_ip:port combination, so it doesn't work for name-based virtual
hosts. That led me to the

ProxyProtocolEnable idea.

Reply | Threaded
Open this post in threaded view
|

Re: An ask for eyes on proposal

William A Rowe Jr
In reply to this post by William A Rowe Jr
Based on the fact that Jim's advanced this for consideration for 2.4.28,
any further feedback on the following proposal to make RemoteIPProxyProtocol
directive into a whitelist (to compliment the current blacklist directive)? E.g.
in the spirit of the protocol, assign specific remote ip consumers to the list
of those where we accept PROXY protocol wrapping?


On Fri, Jun 9, 2017 at 8:29 AM, William A Rowe Jr <[hidden email]> wrote:

> On Fri, Jun 9, 2017 at 4:17 AM, Sander Hoentjen <[hidden email]> wrote:
>> On 06/08/2017 07:30 PM, Daniel Ruggeri wrote:
>>> Hi, all;
>>> With the proposal to T&R set for Monday, I wanted to draw attention to
>>> the PROXY protocol proposal in STATUS. Just hoping for a quick review.
>>> I know it appears to be a large change, but as I worked through the
>>> feedback, ten of the commits effectively got coded out. What we are
>>> left with is essentially just the donated code + safety around IPv6 +
>>> the ability to designate subnets that do not get PROXY processing.
>>
>> [...] I still believe it would be better to specify enabling
>> Proxy Protocol on a server, not vhost level. Because well, once you
>> enable it in one vhost it gets enabled for all vhosts using that port/ip
>> combination.
>>
>> Here is what I said before about it:
>>
>> Right now the patch proposes RemoteIPProxyProtocol inside a vhost config, but wouldn't it be better (since it is connection-specific) to have something like a ProxyProtocolListen directive? Where you say instead of:
>> ------
>> <VirtualHost 127.0.0.1:9001>
>> RemoteIPProxyProtocol On
>> </VirtualHost>
>> ------
>> Something like:
>> ------
>> ProxyProtocolListen 127.0.0.1:9001
>> or
>> ProxyProtocolEnable 127.0.0.1:9001
>> ------
>>
>> IMHO this is much cleaner than within a vhost (because that has side-effects on other vhosts as well)
>
> As this lives in mod_remoteip (for better or worse) let's look at what
> context mod_remoteip is configured in; we set up a list of those local
> or global *client* IP's which we trust to provide legit x-f-f (or remote-ip
> or otherly named) true IP address header fields.
>
> in the PROXY protocol case, we configure which *client* IP's which
> we *require* to submit a PROXY protocol line. Right now, we do that
> as a RemoteIPProxyProtocolExceptions list of those which we do *not*
> allow to submit a PROXY protocol line. I proposed we make the config
> simpler, in theory, by listing those we will trust.
>
> To your example, the *global* config line;
>
> RemoteIPProxyProtocol 127.0.0.1  [or 127.0.0.0/24]
>
> would configure all locally routed *client* requests, irrespective of
> which by-IP vhost, to require the PROXY protocol line. Requests
> from other hosts would be denied.
>
> I think that's sufficient. But if we wanted to implement your basic
> idea, we would still have the complication that we need to infer
> whether 9001 is a http, https, or h2 listener following the PROXY
> line processing. Your proposed syntax didn't really touch on that.
>
> It is still possible to override behavior by-vhost (ip-based, we are
> unprepared to read the TLS SNI or Host header at that point)
> but I don't see any application to do so. A given client is either
> an haproxy or similar, or it is not.
Reply | Threaded
Open this post in threaded view
|

Re: An ask for eyes on proposal

Jim Jagielski
+1 for Whitelisting...

> On Jul 10, 2017, at 12:02 PM, William A Rowe Jr <[hidden email]> wrote:
>
> Based on the fact that Jim's advanced this for consideration for 2.4.28,
> any further feedback on the following proposal to make RemoteIPProxyProtocol
> directive into a whitelist (to compliment the current blacklist directive)? E.g.
> in the spirit of the protocol, assign specific remote ip consumers to the list
> of those where we accept PROXY protocol wrapping?
>
>
> On Fri, Jun 9, 2017 at 8:29 AM, William A Rowe Jr <[hidden email]> wrote:
>> On Fri, Jun 9, 2017 at 4:17 AM, Sander Hoentjen <[hidden email]> wrote:
>>> On 06/08/2017 07:30 PM, Daniel Ruggeri wrote:
>>>> Hi, all;
>>>> With the proposal to T&R set for Monday, I wanted to draw attention to
>>>> the PROXY protocol proposal in STATUS. Just hoping for a quick review.
>>>> I know it appears to be a large change, but as I worked through the
>>>> feedback, ten of the commits effectively got coded out. What we are
>>>> left with is essentially just the donated code + safety around IPv6 +
>>>> the ability to designate subnets that do not get PROXY processing.
>>>
>>> [...] I still believe it would be better to specify enabling
>>> Proxy Protocol on a server, not vhost level. Because well, once you
>>> enable it in one vhost it gets enabled for all vhosts using that port/ip
>>> combination.
>>>
>>> Here is what I said before about it:
>>>
>>> Right now the patch proposes RemoteIPProxyProtocol inside a vhost config, but wouldn't it be better (since it is connection-specific) to have something like a ProxyProtocolListen directive? Where you say instead of:
>>> ------
>>> <VirtualHost 127.0.0.1:9001>
>>> RemoteIPProxyProtocol On
>>> </VirtualHost>
>>> ------
>>> Something like:
>>> ------
>>> ProxyProtocolListen 127.0.0.1:9001
>>> or
>>> ProxyProtocolEnable 127.0.0.1:9001
>>> ------
>>>
>>> IMHO this is much cleaner than within a vhost (because that has side-effects on other vhosts as well)
>>
>> As this lives in mod_remoteip (for better or worse) let's look at what
>> context mod_remoteip is configured in; we set up a list of those local
>> or global *client* IP's which we trust to provide legit x-f-f (or remote-ip
>> or otherly named) true IP address header fields.
>>
>> in the PROXY protocol case, we configure which *client* IP's which
>> we *require* to submit a PROXY protocol line. Right now, we do that
>> as a RemoteIPProxyProtocolExceptions list of those which we do *not*
>> allow to submit a PROXY protocol line. I proposed we make the config
>> simpler, in theory, by listing those we will trust.
>>
>> To your example, the *global* config line;
>>
>> RemoteIPProxyProtocol 127.0.0.1  [or 127.0.0.0/24]
>>
>> would configure all locally routed *client* requests, irrespective of
>> which by-IP vhost, to require the PROXY protocol line. Requests
>> from other hosts would be denied.
>>
>> I think that's sufficient. But if we wanted to implement your basic
>> idea, we would still have the complication that we need to infer
>> whether 9001 is a http, https, or h2 listener following the PROXY
>> line processing. Your proposed syntax didn't really touch on that.
>>
>> It is still possible to override behavior by-vhost (ip-based, we are
>> unprepared to read the TLS SNI or Host header at that point)
>> but I don't see any application to do so. A given client is either
>> an haproxy or similar, or it is not.