Re: RemoteIP valve and multiple X-Forwarded-For headers

2011-01-11 Thread Brett Delle Grazie 
Hi,

On 10 December 2010 14:42, Jim Riggs apache-li...@riggs.me wrote:
 On Dec 10, 2010, at 7:59 AM, Mark Thomas wrote:

 Looks like a bug,

 Please add it to bugzilla, as Mark suggested.

 BTW, I think that the following change can fix it:
 (for current tc6.0.x, not tested!)

 I don't think so. I think the problem is further up on line 558:
 String[] remoteIpHeaderValue =
 commaDelimitedListToStringArray(request.getHeader(remoteIpHeader));

 I'm pretty sure that needs to be using request.getHeaders() but I
 haven't tested it either ;)

 It does indeed need to be using the Enumeration from request.getHeaders() 
 instead of the single value from getHeader().  I have started work on a 
 patch, unless someone beats me to it...


I've manually compiled 6.0.29 with the patch and applied it to our
test and then production systems (we couldn't wait for 6.0.30).
This works perfectly.

Thank you very much to all who helped.

-- 
Best Regards,

Brett Delle Grazie

-
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org

RemoteIP valve and multiple X-Forwarded-For headers

2010-12-10 Thread Brett Delle Grazie 
Hi,

We're using:
RHEL5 (fully up to date)
Tomcat 6.0.29 (from apache.org)
JVM 1.6.0_22

We use HAproxy (1.4.8) as a front end to Tomcat, HAproxy uses the 'option
forwardfor' which adds an additional X-Forwarded-For header
to the request.

Everything works fine except if the client has an X-Forwarded-For header
_already_ in the request (perhaps due to Squid in forward proxy on client
side).

Thus offending request looks like:

Headers (fake IP addresses used):
X-Forwarded-For: 192.168.0.4  (client side added)
... (some other headers) ...
X-Forwarded-For: 224.212.128.2 (added by HAproxy - this is the actual IP of
the client's squid proxy).
... (some other headers) ...

Now Tomcat's RemoteIP valve doesn't appear to handle this situation
correctly - it returns 192.168.0.4 instead of the expected 224.212.128.2

Should HAproxy be extending the existing header to:
e.g. X-Forwarded-For: 192.168.0.4, 224.212.128.2

Or should Tomcat's RemoteIP valve handle this situation?

I'm also not sure which situation is 'correct' according to standards
anyway...

Any ideas?

Thanks,

-- 
Best Regards,

Brett Delle Grazie

Re: RemoteIP valve and multiple X-Forwarded-For headers

2010-12-10 Thread Mark Thomas 
On 10/12/2010 13:03, Brett Delle Grazie wrote:
 Hi,
 
 We're using:
 RHEL5 (fully up to date)
 Tomcat 6.0.29 (from apache.org)
 JVM 1.6.0_22
 
 We use HAproxy (1.4.8) as a front end to Tomcat, HAproxy uses the 'option
 forwardfor' which adds an additional X-Forwarded-For header
 to the request.
 
 Everything works fine except if the client has an X-Forwarded-For header
 _already_ in the request (perhaps due to Squid in forward proxy on client
 side).
 
 Thus offending request looks like:
 
 Headers (fake IP addresses used):
 X-Forwarded-For: 192.168.0.4  (client side added)
 ... (some other headers) ...
 X-Forwarded-For: 224.212.128.2 (added by HAproxy - this is the actual IP of
 the client's squid proxy).
 ... (some other headers) ...
 
 Now Tomcat's RemoteIP valve doesn't appear to handle this situation
 correctly - it returns 192.168.0.4 instead of the expected 224.212.128.2
 
 Should HAproxy be extending the existing header to:
 e.g. X-Forwarded-For: 192.168.0.4, 224.212.128.2
 
 Or should Tomcat's RemoteIP valve handle this situation?

It isn't absolutely clear since I can't find a precise enough definition
of the X-Forwarded-For header but the implication is that Tomcat should
handle this. Currently it doesn't.

 I'm also not sure which situation is 'correct' according to standards
 anyway...

The problem is Tomcat is only looking at the first X-Forwarded-For
header when it should probably be looking at both. Please create a
bugzilla entry for this and someone will hopefully take a look.

Mark

 
 Any ideas?
 
 Thanks,
 


-
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org

Re: RemoteIP valve and multiple X-Forwarded-For headers

2010-12-10 Thread Konstantin Kolinko 
2010/12/10 Brett Delle Grazie brett.dellegra...@gmail.com:
(...)

 Everything works fine except if the client has an X-Forwarded-For header
 _already_ in the request (perhaps due to Squid in forward proxy on client
 side).

 Thus offending request looks like:

 Headers (fake IP addresses used):
 X-Forwarded-For: 192.168.0.4  (client side added)
 ... (some other headers) ...
 X-Forwarded-For: 224.212.128.2 (added by HAproxy - this is the actual IP of
 the client's squid proxy).
 ... (some other headers) ...

 Now Tomcat's RemoteIP valve doesn't appear to handle this situation
 correctly - it returns 192.168.0.4 instead of the expected 224.212.128.2


Looks like a bug,

Please add it to bugzilla, as Mark suggested.

BTW, I think that the following change can fix it:
(for current tc6.0.x, not tested!)

Index: java/org/apache/catalina/valves/RemoteIpValve.java
===
--- java/org/apache/catalina/valves/RemoteIpValve.java  (revision 1044342)
+++ java/org/apache/catalina/valves/RemoteIpValve.java  (working copy)
@@ -564,12 +564,12 @@
 // loop on remoteIpHeaderValue to find the first trusted
remote ip and to build the proxies chain
 for (idx = remoteIpHeaderValue.length - 1; idx = 0; idx--) {
 String currentRemoteIp = remoteIpHeaderValue[idx];
-remoteIp = currentRemoteIp;
 if (matchesOne(currentRemoteIp, internalProxies)) {
 // do nothing, internalProxies IPs are not appended to the
 } else if (matchesOne(currentRemoteIp, trustedProxies)) {
 proxiesHeaderValue.addFirst(currentRemoteIp);
 } else {
+remoteIp = currentRemoteIp;
 idx--; // decrement idx because break statement
doesn't do it
 break;
 }

Best regards,
Konstantin Kolinko

-
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org

Re: RemoteIP valve and multiple X-Forwarded-For headers

2010-12-10 Thread Mark Thomas 
On 10/12/2010 13:54, Konstantin Kolinko wrote:
 2010/12/10 Brett Delle Grazie brett.dellegra...@gmail.com:
 (...)

 Everything works fine except if the client has an X-Forwarded-For header
 _already_ in the request (perhaps due to Squid in forward proxy on client
 side).

 Thus offending request looks like:

 Headers (fake IP addresses used):
 X-Forwarded-For: 192.168.0.4  (client side added)
 ... (some other headers) ...
 X-Forwarded-For: 224.212.128.2 (added by HAproxy - this is the actual IP of
 the client's squid proxy).
 ... (some other headers) ...

 Now Tomcat's RemoteIP valve doesn't appear to handle this situation
 correctly - it returns 192.168.0.4 instead of the expected 224.212.128.2

 
 Looks like a bug,
 
 Please add it to bugzilla, as Mark suggested.
 
 BTW, I think that the following change can fix it:
 (for current tc6.0.x, not tested!)

I don't think so. I think the problem is further up on line 558:
String[] remoteIpHeaderValue =
commaDelimitedListToStringArray(request.getHeader(remoteIpHeader));

I'm pretty sure that needs to be using request.getHeaders() but I
haven't tested it either ;)

Fortunately, the contribution of this valve and the matching filter came
with an extensive set of unit tests. It should be easy to extend the
tests to cover multiple headers.

Mark

 
 Index: java/org/apache/catalina/valves/RemoteIpValve.java
 ===
 --- java/org/apache/catalina/valves/RemoteIpValve.java(revision 
 1044342)
 +++ java/org/apache/catalina/valves/RemoteIpValve.java(working copy)
 @@ -564,12 +564,12 @@
  // loop on remoteIpHeaderValue to find the first trusted
 remote ip and to build the proxies chain
  for (idx = remoteIpHeaderValue.length - 1; idx = 0; idx--) {
  String currentRemoteIp = remoteIpHeaderValue[idx];
 -remoteIp = currentRemoteIp;
  if (matchesOne(currentRemoteIp, internalProxies)) {
  // do nothing, internalProxies IPs are not appended to 
 the
  } else if (matchesOne(currentRemoteIp, trustedProxies)) {
  proxiesHeaderValue.addFirst(currentRemoteIp);
  } else {
 +remoteIp = currentRemoteIp;
  idx--; // decrement idx because break statement
 doesn't do it
  break;
  }
 
 Best regards,
 Konstantin Kolinko
 
 -
 To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
 For additional commands, e-mail: users-h...@tomcat.apache.org
 


-
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org

Re: RemoteIP valve and multiple X-Forwarded-For headers

2010-12-10 Thread Brett Delle Grazie 
On 10 December 2010 13:59, Mark Thomas ma...@apache.org wrote:

 On 10/12/2010 13:54, Konstantin Kolinko wrote:
  2010/12/10 Brett Delle Grazie brett.dellegra...@gmail.com:
  (...)
 
  Everything works fine except if the client has an X-Forwarded-For header
  _already_ in the request (perhaps due to Squid in forward proxy on
 client
  side).
 
  Thus offending request looks like:
 
  Headers (fake IP addresses used):
  X-Forwarded-For: 192.168.0.4  (client side added)
  ... (some other headers) ...
  X-Forwarded-For: 224.212.128.2 (added by HAproxy - this is the actual IP
 of
  the client's squid proxy).
  ... (some other headers) ...
 
  Now Tomcat's RemoteIP valve doesn't appear to handle this situation
  correctly - it returns 192.168.0.4 instead of the expected 224.212.128.2
 
 
  Looks like a bug,
 
  Please add it to bugzilla, as Mark suggested.


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


 
  BTW, I think that the following change can fix it:
  (for current tc6.0.x, not tested!)

 I don't think so. I think the problem is further up on line 558:
 String[] remoteIpHeaderValue =
 commaDelimitedListToStringArray(request.getHeader(remoteIpHeader));

 I'm pretty sure that needs to be using request.getHeaders() but I
 haven't tested it either ;)

 Fortunately, the contribution of this valve and the matching filter came
 with an extensive set of unit tests. It should be easy to extend the
 tests to cover multiple headers.

 Mark

 
  Index: java/org/apache/catalina/valves/RemoteIpValve.java
  ===
  --- java/org/apache/catalina/valves/RemoteIpValve.java(revision
 1044342)
  +++ java/org/apache/catalina/valves/RemoteIpValve.java(working
 copy)
  @@ -564,12 +564,12 @@
   // loop on remoteIpHeaderValue to find the first trusted
  remote ip and to build the proxies chain
   for (idx = remoteIpHeaderValue.length - 1; idx = 0; idx--)
 {
   String currentRemoteIp = remoteIpHeaderValue[idx];
  -remoteIp = currentRemoteIp;
   if (matchesOne(currentRemoteIp, internalProxies)) {
   // do nothing, internalProxies IPs are not appended
 to the
   } else if (matchesOne(currentRemoteIp, trustedProxies))
 {
   proxiesHeaderValue.addFirst(currentRemoteIp);
   } else {
  +remoteIp = currentRemoteIp;
   idx--; // decrement idx because break statement
  doesn't do it
   break;
   }
 
  Best regards,
  Konstantin Kolinko
 
  -
  To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
  For additional commands, e-mail: users-h...@tomcat.apache.org
 


 -
 To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
 For additional commands, e-mail: users-h...@tomcat.apache.org




-- 
Best Regards,

Brett Delle Grazie

Re: RemoteIP valve and multiple X-Forwarded-For headers

2010-12-10 Thread Jim Riggs 
On Dec 10, 2010, at 7:59 AM, Mark Thomas wrote:

 Looks like a bug,
 
 Please add it to bugzilla, as Mark suggested.
 
 BTW, I think that the following change can fix it:
 (for current tc6.0.x, not tested!)
 
 I don't think so. I think the problem is further up on line 558:
 String[] remoteIpHeaderValue =
 commaDelimitedListToStringArray(request.getHeader(remoteIpHeader));
 
 I'm pretty sure that needs to be using request.getHeaders() but I
 haven't tested it either ;)

It does indeed need to be using the Enumeration from request.getHeaders() 
instead of the single value from getHeader().  I have started work on a patch, 
unless someone beats me to it...


-
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org