You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bu...@apache.org on 2018/12/05 12:52:41 UTC
[Bug 62978] New: RemoteIpValve: Multiple forwards in
X-Forwarded-Proto header not supported
https://bz.apache.org/bugzilla/show_bug.cgi?id=62978
Bug ID: 62978
Summary: RemoteIpValve: Multiple forwards in X-Forwarded-Proto
header not supported
Product: Tomcat 9
Version: unspecified
Hardware: All
OS: All
Status: NEW
Keywords: PatchAvailable
Severity: normal
Priority: P2
Component: Catalina
Assignee: dev@tomcat.apache.org
Reporter: t-groot@outlook.com
Target Milestone: -----
When a request gets forwarded through multiple proxies, certain proxy
implementations seem to apply the same logic as it is common with the
X-Forwarded-For header, and likewise append the incoming protocol to the
existing value of the X-Forwarded-Proto header in a comma separated values
fashion.
Should such a request reach tomcat, it can never be considered secure, due to
its implementation in the RemoteIpValve.
The appended patch modifies the RemoteIpValve to consider also multiple
forwards to be secure, if it exclusively holds forwards of secure protocols.
I tried to stay in line with the existing code formatting and avoided any
exotics so it could be backported, if desired. For the new test cases i opted
to avoid the duplication of the repetitive assertion code as seen at existing
test cases, and hope that it is fine.
Background:
In our setup we have a loadbalancer enforcing the https protocol with web
clients and setting the X-Forwarded-* headers on the requests it forwards to
our API gateway (spring-cloud-netflix Zuul). Zuul processes these X-Forwarded-*
for headers and adds itself to the chain of proxies, and in its implementation
it performs the CSV based appending also to the X-Forwarded-Proto header.
Therefore the request reaching our services running on tomcat exhibit the
header like that:
'X-Forwarded-Proto: https,https'
Which obviously doesn't match ignoreCase 'https' and thus is considered
insecure. Configuring the protocolHeaderHttpsValue to be 'https,https' is not
an acceptable solution.
Although not completely certain, we have the suspicion that this is the reason
why something strips the "Secure" flags from our session cookies, which is
rather bad. Regardless of whether this is related or not, the support for
handling "new" behaviour of standard proxy solutions correctly and backwards
compatibly should be beneficial.
How to reproduce:
curl -kI https://localhost:8443/some-application/setting-secure-cookies -H
'X-Forwarded-For: 111.111.111.111,222.222.222.222' -H 'X-Forwarded-Proto:
https'
HTTP/1.1 200 OK
Server: Apache-Coyote/1.1
Set-Cookie: JSESSIONID=A1B6F171F11844A0FE8E857DBB8A0AA1;
Path=/some-application/; Secure; HttpOnly
vs
curl -kI https://localhost:8443/some-application/setting-secure-cookies -H
'X-Forwarded-For: 111.111.111.111,222.222.222.222' -H 'X-Forwarded-Proto:
https,https'
HTTP/1.1 200 OK
Server: Apache-Coyote/1.1
Set-Cookie: JSESSIONID=A5B7933C3AE78ACA2AD7A1767A4163B1;
Path=/some-application/; HttpOnly
I failed to find a profound resource specifying the value of this non-standard
header. However i have come to the opinion that Zuul's way of processing this
header is reasonable it is not the only kind of proxy doing it this way.
References:
Zuul's source code:
https://github.com/spring-cloud/spring-cloud-netflix/blob/master/spring-cloud-netflix-zuul/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/PreDecorationFilter.java#L221
Evidence/indication that this behaviour is, or has become recommended:
https://github.com/aspnet/BasicMiddleware/issues/18
Thanks for considering and kind regards,
tom
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62978] RemoteIpValve: Multiple forwards in X-Forwarded-Proto
header not supported
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62978
--- Comment #3 from Tom Groot <t-...@outlook.com> ---
Created attachment 36314
--> https://bz.apache.org/bugzilla/attachment.cgi?id=36314&action=edit
Fixes typo / copy paste mistake in test method names.
Hi Mark,
Thanks a lot for the quick response and also for taking care of the filter i
didn't know of.
I have to apologize, i noticed now that when copy-pasting these endlessly long
names of the test methods i failed to adjust some of them properly, hence this
small follow-up patch. In cases where the mock request is prepared as http, the
test method name said it was https. It should not matter too much, it would
just be to avoid confusion if someone wanted to make sense of the tests.
Sorry,
tom
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62978] RemoteIpValve: Multiple forwards in X-Forwarded-Proto
header not supported
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62978
--- Comment #4 from Mark Thomas <ma...@apache.org> ---
Patch applied. Thanks for the attention to detail. It is much appreciated.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62978] RemoteIpValve: Multiple forwards in X-Forwarded-Proto
header not supported
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62978
Tom Groot <t-...@outlook.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |t-groot@outlook.com
--- Comment #1 from Tom Groot <t-...@outlook.com> ---
Created attachment 36291
--> https://bz.apache.org/bugzilla/attachment.cgi?id=36291&action=edit
Patch incl. test cases to support multiple forwarded protocols in
X-Forwarded-Proto header value.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62978] RemoteIpValve: Multiple forwards in X-Forwarded-Proto
header not supported
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62978
Mark Thomas <ma...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--- Comment #2 from Mark Thomas <ma...@apache.org> ---
Thanks for the report, test cases and patch. As bug reports go, this one was
ideal.
Fixed in:
- trunk for 9.0.14 onwards
- 8.5.x for 8.5.36 onwards
- 7.0.x for 7.0.93 onwards
I also applied a very similar patch to the RemoteIpFilter that uses almost
identical code.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org