Bug 47330

Summary: proposal : port of mod_remoteip in Tomcat as RemoteIpValve
Product: Tomcat 6 Reporter: Cyrille Le Clerc <cyrille.leclerc>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Severity: normal CC: bfg
Priority: P2    
Version: 6.0.20   
Target Milestone: default   
Hardware: PC   
OS: Windows XP   
Attachments: First version of the proposed patch
Javadoc of RemoteIpValve
Second version of the proposed patch
Tomcat 7 patch with completed TODOs and failing test case
Fixed Tomcat 7 patch with successful test case
Fixed Tomcat 7 patch with successful test case
logging, naming and javadoc patch of RemoteIpFilter

Description Cyrille Le Clerc 2009-06-07 17:38:02 UTC
Created attachment 23772 [details]
First version of the proposed patch

Here is a proposal to port Apache Server mod_remoteip module as a Tomcat Valve to have the actual end user remote ip in ServletRequest.getRemoteAddr() and ServletRequest.getRemoteHost() methods even if reverse proxies (e.g. Apache Http Server mod_proxy_http + mod_proxy_balancer) and/or hardware load balancer (e.g. F5 Big IP, etc).

This feature will benefit security and audit frameworks like SpringSecurity which use ServletRequest.getRemoteAddr() in its eventing mechanism to track web user ip.

This proposal is composed of :
* RemoteIpValve.java : the proposed standalone code
* RemoteIpValveTest.java : 12 test cases to validate the behavior
Note : no existing Tomcat code is modified by this proposed Valve

Documentation for mod_remoteip : http://httpd.apache.org/docs/trunk/mod/mod_remoteip.html

Main differences between mod_remoteip and RemoteIpValve :
* RemoteIpValve uses regular expressions to express network subnets when mod_remoteip uses ip address blocks because:
** request filters valves (RemoteAddrValve and RemoteHostValve) already use regular expressions for this
** there are no ip address blocks library available in Tomcat similar to apr_ipsubnet_test that is used in httpd.
** The directives RemoteIPInternalProxyList and RemoteIPTrustedProxyList are not ported: configuration is server.xml oriented and limited to the java equivalents of  RemoteIPInternalProxy  and RemoteIPTrustedProxy

Sample of configuration : 
   allowedInternalProxies="192\.168\.0\.10, 192\.168\.0\.11"
   trustedProxies="proxy1, proxy2"

Configuration parameters :

| REMOTEIPVALVE PROPERTY  | EQUIVALENT MOD_REMOTEIP DIRECTIVE | FORMAT                               | DEFAULT VALUE                                 |
| remoteIPHeader          | RemoteIPHeader                    | Compliant http header string         | x-forwarded-for                               |
| internalProxies         | RemoteIPInternalProxy             | Comma delimited list of regular      | 10\.\d{1,3}\.\d{1,3}\.\d{1,3},                |
|                         |                                   | expressions (in the syntax supported | 192\.168\.\d{1,3}\.\d{1,3},                   |
|                         |                                   | by the Pattern library)              | 169\.254\.\d{1,3}\.\d{1,3},                   |
|                         |                                   |                                      | 127\.\d{1,3}\.\d{1,3}\.\d{1,3}                |
|                         |                                   |                                      |                                               |
|                         |                                   |                                      | By default, 10/8, 192.168/16, 169.254/16      |
|                         |                                   |                                      | and 127/8 are allowed ; 172.16/12 has not     |
|                         |                                   |                                      | been enabled by default because it is         |
|                         |                                   |                                      | complex to describe with regular expressions  |
| proxiesHeader           | RemoteIPProxiesHeader             | Compliant http header String         | x-forwarded-by                                |
| trustedProxies          | RemoteIPTrustedProxy              | Comma delimited list of regular      |                                               |
|                         |                                   | expressions (in the syntax supported |                                               |
|                         |                                   | by the Pattern library)              |                                               |

Reason why RemoteIpValve is declared in the o.a.catalina.connector package instead of o.a.catalina.valves :
Because Request.setRemoteAddr(String) and Request.setRemoteHost(String) methods are currently no-op and RemoteIpValve use package visibility to directly modify Request.remoteAddr and Request.remoteHost fields.
RemoteIpValve could be moved to o.a.catalina.valves if the Request.setRemoteAddr(String) and Request.setRemoteHost(String) methods to no longer be no-op but to actually modify the underlying fields.
Comment 1 Cyrille Le Clerc 2009-06-07 17:42:33 UTC
Created attachment 23773 [details]
Javadoc of RemoteIpValve

The plain text table of configuration parameters I wrote in the initial description has unfortunately been damaged. This javadoc contains the HTML version of the description of RemoteIpValve
Comment 2 Cyrille Le Clerc 2009-07-13 09:50:20 UTC
Created attachment 23970 [details]
Second version of the proposed patch

Add support for x-forwarded-proto / x-forwarded-ssl / front-end-https styles http header for https request : sets request.secure = true, request.scheme = https, request.serverPort = 443

Detailed documentation at : http://code.google.com/p/xebia-france/wiki/RemoteIpValve
Comment 3 Cyrille Le Clerc 2009-10-11 03:46:39 UTC
As Tomcat valves are currently being converted into Servlet Filter (1), here is a Servlet Filter implementation of mod_remoteip we called XForwardedFilter : 

Licensing and copyright

The XforwardedFilter code is still licensed ASL 2 but the copyright is for "Xebia and the original author or authors" but we can change it to the usual Apache Software Foundation license and copyright with great pleasure.

Implementation decisions

This filter has been developped before knowing that the Tomcat project would move from Valve to Filter and is intended to be servlet container agnostic.

We decided to rely on SLF4J for logging to be as portable as possible. Integrating this Filter in Tomcat project may require to migrate to JULI.

(1) Google Summer Of Code 2009, project "Convert current Tomcat valves to Servlet Filters" : http://wiki.apache.org/tomcat/SummerOfCode2009
Comment 4 Mark Thomas 2009-11-01 18:32:58 UTC
Created attachment 24455 [details]
Tomcat 7 patch with completed TODOs and failing test case

Many thanks for these patches. I'd like to add the valve and filter to Tomcat 7 and the valve to Tomcat 6. I have made the necessary changes to Tomcat 7 the valve can sit in the right package.

When I applied the patch, I completed the TODOs and added some documentation. However, one of the test cases failed and my initial investigation suggests that the patch needs further work.

I have attached my final Tomcat 7 patch. Please update as necessary to correct the failing test case and then I'll apply it. Once the valve is applied, I'll look a the filter which will probably be generated by porting the final version of the valve.
Comment 5 Cyrille Le Clerc 2009-11-02 13:17:01 UTC
Created attachment 24465 [details]
Fixed Tomcat 7 patch with successful test case

Here is the fixed patch. A small mistake seems to have been introduced in the test case during the refactoring.
Unfortunately, my "svn diff" command did not order the changed files in the same order as the "24455: Tomcat 7 patch with completed TODOs and failing test case" patch did.

For the Servlet API Filter implementation of this feature, there is some work to port it from a valve because the HttpServletRequest interface does not ease adding and removing http headers. 

To do this, I completely reimplemented the headers mechanisms with a case insensitive list of headers rebuilt in a request wrapper. 

Another apporach could have been to decorate the getXxxHeader() and getHeaderNames() methods. The getHeaderNames() method would be more complex because we need to add headers (like x-forwarded-server) but also to remove some (like x-forwarded-for). If this was an Iterator instead of an Enumeration, I would use a predicate to exclude the "removed" headers, a FilterEnumeration to apply the predicate on the Enumeration and a ChainIterator for the "added" headers. This would imply reinventing a subset of Apache Commons Collection applied to the old ages Enumeration.

Another point on the Filter port of the RemoteIpValve feature is the logging implementation. Using Tomcat JULI would make reusing this Filter outside of Tomcat more complex.

The version 1.0.0 of XForwardedFilter.java I linked on a previous comment of this defect had a bug in the debug statement, here is a fixed version :
Comment 6 Mark Thomas 2009-11-03 17:25:13 UTC
The updated patch is missing a bunch of files including the valve and the test case.
Comment 7 Cyrille Le Clerc 2009-11-04 01:04:03 UTC
Created attachment 24471 [details]
Fixed Tomcat 7 patch with successful test case

Sorry for the time you lost. Here is the patch that includes the following modified/added files : 
- valve.xml
- RemoteIpValveTest.java
- RemoteIpValve.java
- LocalStrings.properties
- mbeans-descriptors.xml
Comment 8 Mark Thomas 2009-11-04 18:05:37 UTC
Thanks for the updated patch.

I have applied the valve patch to trunk. The filter patch will follow along with a backport proposal for the valve to 6.0.x
Comment 9 Mark Thomas 2009-11-05 12:35:50 UTC
I've applied the filter to trunk and proposed the valve for backport to 6.0.x along with the required changes to Request
Comment 10 Mark Thomas 2009-11-06 11:22:39 UTC
The valve has been applied to 6.0.x and will be included in 6.0.21 onwards.
Comment 11 Cyrille Le Clerc 2009-11-08 15:50:42 UTC
Created attachment 24505 [details]
logging, naming and javadoc patch of RemoteIpFilter

Here is a patch of RemoteIpFilter with the following :
* fix NPE in log statement if protocolHeader has not been defined and the servlet container does not support request.getHeader(null)
* fix mismatch between javadoc  and code for filter parameter name "allowedInternalProxies" -> "internalProxies"
* finish javadoc refactoring "XForwardedFilter" -> "RemoteIpFilter"
Comment 12 Mark Thomas 2009-11-09 06:31:33 UTC
Patch applied. Many thanks.
Comment 13 Mark Thomas 2009-12-11 08:46:28 UTC
*** Bug 48378 has been marked as a duplicate of this bug. ***