DO NOT REPLY [Bug 47330] New: proposal : port of mod_remoteip in Tomcat as RemoteIpValve

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330

           Summary: proposal : port of mod_remoteip in Tomcat as
                    RemoteIpValve
           Product: Tomcat 6
           Version: 6.0.20
          Platform: PC
        OS/Version: Windows XP
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Catalina
        AssignedTo: dev@...
        ReportedBy: cyrille.leclerc@...


Created an attachment (id=23772)
 --> (https://issues.apache.org/bugzilla/attachment.cgi?id=23772)
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 :
 <Valve
   className="org.apache.catalina.connector.RemoteIpValve"
   allowedInternalProxies="192\.168\.0\.10, 192\.168\.0\.11"
   remoteIPHeader="x-forwarded-for"
   remoteIPProxiesHeader="x-forwarded-by"
   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.

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330





--- Comment #1 from Cyrille Le Clerc <cyrille.leclerc@...>  2009-06-07 17:42:33 PST ---
Created an attachment (id=23773)
 --> (https://issues.apache.org/bugzilla/attachment.cgi?id=23773)
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

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330


Cyrille Le Clerc <cyrille.leclerc@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #23772|0                           |1
        is obsolete|                            |
  Attachment #23773|0                           |1
        is obsolete|                            |




--- Comment #2 from Cyrille Le Clerc <cyrille.leclerc@...>  2009-07-13 09:50:20 PST ---
Created an attachment (id=23970)
 --> (https://issues.apache.org/bugzilla/attachment.cgi?id=23970)
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

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330

--- Comment #3 from Cyrille Le Clerc <cyrille.leclerc@...> 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 :
http://code.google.com/p/xebia-france/wiki/XForwardedFilter
http://xebia-france.googlecode.com/svn/web/xebia-servlet-extras/tags/xebia-servlet-extras-1.0.0/src/main/java/fr/xebia/servlet/filter/XForwardedFilter.java

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

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330

--- Comment #4 from Mark Thomas <markt@...> 2009-11-01 18:32:58 GMT ---
Created an attachment (id=24455)
 --> (https://issues.apache.org/bugzilla/attachment.cgi?id=24455)
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.

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330

Cyrille Le Clerc <cyrille.leclerc@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24455|0                           |1
        is obsolete|                            |

--- Comment #5 from Cyrille Le Clerc <cyrille.leclerc@...> 2009-11-02 13:17:01 UTC ---
Created an attachment (id=24465)
 --> (https://issues.apache.org/bugzilla/attachment.cgi?id=24465)
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 :
http://xebia-france.googlecode.com/svn/web/xebia-servlet-extras/tags/xebia-servlet-extras-1.0.1/src/main/java/fr/xebia/servlet/filter/XForwardedFilter.java

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330

Mark Thomas <markt@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |NEEDINFO

--- Comment #6 from Mark Thomas <markt@...> 2009-11-03 17:25:13 GMT ---
The updated patch is missing a bunch of files including the valve and the test
case.

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330

Cyrille Le Clerc <cyrille.leclerc@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24465|0                           |1
        is obsolete|                            |

--- Comment #7 from Cyrille Le Clerc <cyrille.leclerc@...> 2009-11-04 01:04:03 UTC ---
Created an attachment (id=24471)
 --> (https://issues.apache.org/bugzilla/attachment.cgi?id=24471)
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

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330

Mark Thomas <markt@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEEDINFO                    |NEW

--- Comment #8 from Mark Thomas <markt@...> 2009-11-04 18:05:37 GMT ---
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

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330

--- Comment #9 from Mark Thomas <markt@...> 2009-11-05 12:35:50 GMT ---
I've applied the filter to trunk and proposed the valve for backport to 6.0.x
along with the required changes to Request

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330

Mark Thomas <markt@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #10 from Mark Thomas <markt@...> 2009-11-06 11:22:39 GMT ---
The valve has been applied to 6.0.x and will be included in 6.0.21 onwards.

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330

--- Comment #11 from Cyrille Le Clerc <cyrille.leclerc@...> 2009-11-08 15:50:42 UTC ---
Created an attachment (id=24505)
 --> (https://issues.apache.org/bugzilla/attachment.cgi?id=24505)
logging, naming and javadoc patch

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"

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330

Cyrille Le Clerc <cyrille.leclerc@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24505|logging, naming and javadoc |logging, naming and javadoc
        description|patch                       |patch of RemoteIpFilter

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

https://issues.apache.org/bugzilla/show_bug.cgi?id=47330

--- Comment #12 from Mark Thomas <markt@...> 2009-11-09 06:31:33 GMT ---
Patch applied. Many thanks.

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...