Viewing the patch as sent to the mailing list, points out unwanted tab
characters.
So I reattached a patch with these characters removed.
Thanks,
Mike Rumph
On 12/19/2013 10:46 AM, Mike Rumph wrote:
On 12/13/2013 5:03 PM, William A. Rowe Jr. wrote:
> There is nothing I see in the code that prevents a cycle with
internal proxy from following a cycle with external proxy.
It's been several years so I was going from memory, but you are right...
> So if your explanation is the way the code is intended, there may
exist some subtle error cases.
Yes, the 'fail case' illustrated simply prevented the trusted proxy
from presenting a private address.
I think this is an error, that it should have further prevented a a
trusted proxy from returning to the
internal proxy list, and patches are welcome.
I attached a patch to implement this idea with the following
interpretation:
Allow an internal proxy to present an external proxy, but do not allow
an external proxy to present an internal proxy.
In this case the presented internal proxy will be considered external.
The patch also documents the case where no RemoteIPInternalProxy,
RemoteIPInternalProxyList, RemoteIPTrustedProxy or
RemoteIPTrustedProxyList directives are configured at all.
So what do you think, Bill?
I also further endorse the patch in bug 54651 as essential.
- https://issues.apache.org/bugzilla/show_bug.cgi?id=54651
That patch should be committed to trunk and backported.
Without that patch, RemoteIPHeader headers containing a list of more
than one IP address are not processed correctly.
There's one other enhancement I've wanted to make, to accept an trust
as internal another
RemoteIPInternalHeader value from any routing appliances.
Said appliance would be responsible
for dropping any incoming value from the client, and replacing it
with an absolute header or list
of headers. Typical routing solutions use X-Remote-IP or similar
headers to convey this info.
So are you suggesting RemoteIPInternalHeader as a new directive?
I have some time to look into this as well.
So the processing order would be the values in the
RemoteIPInternalHeader list, followed by
the RemoteIPHeader values matching the RemoteIPInternalProxy
<http://httpd.apache.org/docs/current/mod/mod_remoteip.html#remoteipinternalproxylist> lists,
and then restricted to
extranet addresses presented by the RemoteIPTrustedProxy
<http://httpd.apache.org/docs/current/mod/mod_remoteip.html#remoteipinternalproxylist> list.
But in enforcing this change, some configurations are likely to be
broken where users have an
assorted collection of Internal and Trusted Proxy members which don't
actually follow this
design, so we might be a bit late in changing this before 2.6.
Index: modules/metadata/mod_remoteip.c
===================================================================
--- modules/metadata/mod_remoteip.c (revision 1552356)
+++ modules/metadata/mod_remoteip.c (working copy)
@@ -230,12 +230,25 @@
char *parse_remote;
char *eos;
unsigned char *addrbyte;
+
+ /* If no RemoteIPInternalProxy, RemoteIPInternalProxyList,
RemoteIPTrustedProxy
+ or RemoteIPTrustedProxyList directive is configured,
+ all proxies will be considered as external trusted proxies.
+ */
void *internal = NULL;
if (!config->header_name) {
return DECLINED;
}
+ if (config->proxymatch_ip) {
+ /* This indicates that a RemoteIPInternalProxy,
RemoteIPInternalProxyList, RemoteIPTrustedProxy
+ or RemoteIPTrustedProxyList directive is configured.
+ In this case, default to internal proxy.
+ */
+ internal = (void *) 1;
+ }
+
remote = (char *) apr_table_get(r->headers_in, config->header_name);
if (!remote) {
return OK;
@@ -254,7 +267,13 @@
match = (remoteip_proxymatch_t *)config->proxymatch_ip->elts;
for (i = 0; i < config->proxymatch_ip->nelts; ++i) {
if (apr_ipsubnet_test(match[i].ip, c->client_addr)) {
- internal = match[i].internal;
+ if (internal) {
+ /* Allow an internal proxy to present an external
proxy,
+ but do not allow an external proxy to present an
internal proxy.
+ In this case, the presented internal proxy will be
considered external.
+ */
+ internal = match[i].internal;
+ }
break;
}
}
