Bug 62220 - RemoteIPInternalProxyList does not work after PROXY implementation
Summary: RemoteIPInternalProxyList does not work after PROXY implementation
Status: RESOLVED FIXED
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_remoteip (show other bugs)
Version: 2.4.33
Hardware: All All
: P2 major (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-26 21:40 UTC by smtalk
Modified: 2018-08-04 10:34 UTC (History)
0 users



Attachments
Patch for test (785 bytes, patch)
2018-03-27 21:56 UTC, Christophe JAILLET
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description smtalk 2018-03-26 21:40:24 UTC
RemoteIPInternalProxyList does not work anymore after the following commit: https://github.com/apache/httpd/commit/b6855504e3273a92bde4a21fb573582faf16e381#diff-982de0fd2ba316e125a0dd58de12e74b

1 commit earlier mod_remote.c had no problems with it.
Comment 1 Christophe JAILLET 2018-03-27 19:13:46 UTC
Thanks for your report.

Could you please provide logs with:
   LogLevel remoteip:trace1
(with the unmodified 2.4.33)
Comment 2 smtalk 2018-03-27 20:44:19 UTC
Hm... Nothing seems to be there with it in error log.

So, it returns nothing RemoteIPInternalProxyList. If I use "RemoteIPInternalProxy IP" instead of "RemoteIPInternalProxyList file.lst", then remoteip:trace1 logs the expected info. No info with just RemoteIPInternalProxyList file.lst.
Comment 3 Christophe JAILLET 2018-03-27 21:25:19 UTC
Strange.
Basically 'RemoteIPInternalProxyList' is just a convenient way to call 'RemoteIPInternalProxy'.

I suppose that you have already checked 'file.lst' itself?
Comment 4 smtalk 2018-03-27 21:50:57 UTC
Yes, the file has the IP address. We got reports from many servers, they all got broken after update to 2.4.33. I compiled 2.4.33 with mod_remoteip.c from https://github.com/apache/httpd/commit/b6855504e3273a92bde4a21fb573582faf16e381#diff-982de0fd2ba316e125a0dd58de12e74b to make sure it's the one which broke everything, if I take mod_remoteip.c one commit back and compile 2.4.33 with it - everything works as expected. We clearly have a bug.
Comment 5 Christophe JAILLET 2018-03-27 21:56:51 UTC
Created attachment 35817 [details]
Patch for test

Could you please try the attached patch.
It is just for testing.

It could explain why you have an issue with 2.4.33, but would not explain why RemoteIPInternalProxyList does not work by RemoteIPInternalProxy does
Comment 6 Christophe JAILLET 2018-03-27 21:57:25 UTC
s/by/but/
Comment 7 smtalk 2018-03-27 22:16:43 UTC
1) patch enables RemoteIPProxyProtocol by default:
[Wed Mar 28 01:12:43.030981 2018] [remoteip:debug] [pid 14075] mod_remoteip.c(892): [client 185.38.149.80:40630] AH03503: RemoteIPProxyProtocol: enabled on connection to 185.38.149.80:8080

2) Setting it manually to off still skips the content of RemoteIPInternalProxyList.

3) Still nothing is logged with trace1.

And yes, RemoteIPInternalProxy has no issue, with IP specified in config, while RemoteIPInternalProxyList does not work.
Comment 8 smtalk 2018-04-03 11:48:55 UTC
Any news on this? (do you need any further details?)
Comment 9 Dave 2018-04-03 12:41:39 UTC
We experience problems with Apache 2.4.33 on all servers. The same problem as smtalk.

Do you have a solution? Can I test something?
Comment 10 Christophe JAILLET 2018-04-05 20:02:28 UTC
I can not reproduce the issue with my test configuration.
Moreover, RemoteIPInternalProxyList and RemoteIPInternalProxy are really similar. They should really behave the same.

Could you please provide a reduced conf file in order to try to reproduce the issue?
Comment 11 smtalk 2018-04-05 20:45:36 UTC
Sent to christophe.jaillet@. Thank you!
Comment 12 Dave 2018-04-18 11:27:06 UTC
Is there any news? Have you found the problem with smtalk?
Comment 13 Vincent Verloop 2018-05-30 12:00:46 UTC
Directadmin: Apache + Nginx reverse proxy configuration, has this issue.

http://forum.directadmin.com/showthread.php?t=56159&p=287781#post287781
Comment 14 William A. Rowe Jr. 2018-05-31 15:23:44 UTC
[I'll note that in the discussion above "it doesn't work" wasn't particularly insightful - is this a crash-bug, or what specifically does not work?]

Not a solution, but explaining what might be going on so that the underlying
defect is fixed. First off, the docs are wrong;

Syntax:	RemoteIPInternalProxyList filename
Syntax:	RemoteIPTrustedProxyList filename
Context: server config, virtual host

Because these two directives run on exec (after preconfig, prior to other directives), the scope is actually *server config*. When I authored this
module, the expectation was that RemoteIPTrustedProxyList would be some
monster list, e.g.

  https://meta.wikimedia.org/wiki/XFF_project#Trusted_XFF_list

so it was never envisioned that a specific machine would trust anything
other than its physical traffic config (Internal) or some list that delays
startup for a minute or more (unless pre-piped through logresolve for dns
resolution).

I suspect everyone reporting a defect has their list directive within some
virtual host and expected that to be honored for the specific host. As it
is, all of the named lists are cumulative to the global server config. Specific internal+proxy trust in a specific vhost config overrides global config - it doesn't supplement it.

That could arguably be changed, given multi-tenant needs today. It could
also be changed to merge a global list with the per-server list during the
config merge, which makes far more sense than simply changing this behavior to ignore the global lists, unannounced.

Now... going back to the reports above, the comment is the directive "does not work". We need to know if the lists directive causes a crash? Or the IP's listed in those list directives are ignored?

If this is simply ignoring global trusted/internal List, note that every PROXY related directive now causes a virtual host config to come into existence. I have no explanation yet how the pre/post configs introduced to this module have impacted the creation of vhost configs and altered the behavior of the List directives, but that would be the starting point. Someone hitting such a behavior should be sharing a simple config example of how they encountered this, with relevant vhosts/remoteip directives.

If this were a crash; this means that every affected server has a global config with perhaps nothing more than one or multiple trusted/internal lists, and (I am guessing) further config that affects only intended virtual hosts, but are undefined for the global host. Note the global config values are all initialized to 0/NULL, so any exception begins there. Someone hitting such a crash needs to share the backtrace, please;
https://httpd.apache.org/dev/debugging.html#crashes

Note that no flags were merged for PROXY protocol handling in the initial merge_remoteip_server_config(), leading to some likely confusion. Also note that with the introduction of the PROXY filter, during early processing the server config loaded is the global config (ap_server_conf) and not the applicable physical vhost. This may or may not be relevant. This code also introduces some interesting pre/post config side effects of by replacing default behavior.
Comment 15 Yann Ylavic 2018-05-31 15:41:08 UTC
(In reply to William A. Rowe Jr. from comment #14)
> 
> Because these two directives run on exec (after preconfig, prior to other
> directives), the scope is actually *server config*.

Agreed on the scope, however EXEC_ON_READ happens *before* preconfig (from ap_read_config() in main.c, so before ap_run_pre_config() is called).

This is why I removed the pre_config hook in trunk (and proposed as backport), it simply "cleared" all the EXEC_ON_READ work.
Comment 16 William A. Rowe Jr. 2018-05-31 15:47:46 UTC
Reviewing the proposed fix;

https://svn.apache.org/viewvc/httpd/httpd/trunk/modules/metadata/mod_remoteip.c?r1=1832580&r2=1832579&pathrev=1832580

begs the question, the logic described in C14 above,

"Also note that with the introduction of the PROXY filter, during early processing the server config loaded is the global config (ap_server_conf) and not the applicable physical vhost."

... does this ensure that filter still gets the global server config, and this does not introduce a crash-bug when PROXY handling is introduced for a specific vhost? I suspect a second half of that patch is needed during post-config to ensure the global is configured?
Comment 17 Yann Ylavic 2018-05-31 15:55:18 UTC
Looks like the main server config is created on LoadModule, so we should be safe here.
Comment 18 William A. Rowe Jr. 2018-05-31 16:00:04 UTC
So long as the ap_set_module_config(ap_server_conf->module_config, &remoteip_module, config) bit was a no-op, we can proceed with this fix.
Comment 19 Christophe JAILLET 2018-08-04 10:34:19 UTC
This has been backported in 2.4.x in r1833070

This is part of 2.4.34