Author: markt Date: Mon Jun 4 18:57:59 2018 New Revision: 1832882 URL: http://svn.apache.org/viewvc?rev=1832882&view=rev Log: Correctly handle the case when the request passes through one or more trustedProxies but no internalProxies. Based on a patch by zhanhb This closes #45
Modified: tomcat/trunk/java/org/apache/catalina/filters/RemoteIpFilter.java tomcat/trunk/java/org/apache/catalina/valves/RemoteIpValve.java tomcat/trunk/test/org/apache/catalina/filters/TestRemoteIpFilter.java tomcat/trunk/test/org/apache/catalina/valves/TestRemoteIpValve.java tomcat/trunk/webapps/docs/changelog.xml Modified: tomcat/trunk/java/org/apache/catalina/filters/RemoteIpFilter.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/filters/RemoteIpFilter.java?rev=1832882&r1=1832881&r2=1832882&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/catalina/filters/RemoteIpFilter.java (original) +++ tomcat/trunk/java/org/apache/catalina/filters/RemoteIpFilter.java Mon Jun 4 18:57:59 2018 @@ -67,7 +67,8 @@ import org.apache.juli.logging.LogFactor * This servlet filter proceeds as follows: * </p> * <p> - * If the incoming <code>request.getRemoteAddr()</code> matches the servlet filter's list of internal proxies : + * If the incoming <code>request.getRemoteAddr()</code> matches the servlet + * filter's list of internal or trusted proxies: * </p> * <ul> * <li>Loop on the comma delimited list of IPs and hostnames passed by the preceding load balancer or proxy in the given request's Http @@ -761,8 +762,11 @@ public class RemoteIpFilter extends Gene public void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws IOException, ServletException { - if (internalProxies != null && - internalProxies.matcher(request.getRemoteAddr()).matches()) { + boolean isInternal = internalProxies != null && + internalProxies.matcher(request.getRemoteAddr()).matches(); + + if (isInternal || (trustedProxies != null && + trustedProxies.matcher(request.getRemoteAddr()).matches())) { String remoteIp = null; // In java 6, proxiesHeaderValue should be declared as a java.util.Deque LinkedList<String> proxiesHeaderValue = new LinkedList<>(); @@ -778,11 +782,14 @@ public class RemoteIpFilter extends Gene String[] remoteIpHeaderValue = commaDelimitedListToStringArray(concatRemoteIpHeaderValue.toString()); int idx; + if (!isInternal) { + proxiesHeaderValue.addFirst(request.getRemoteAddr()); + } // 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 (internalProxies.matcher(currentRemoteIp).matches()) { + if (internalProxies !=null && internalProxies.matcher(currentRemoteIp).matches()) { // do nothing, internalProxies IPs are not appended to the } else if (trustedProxies != null && trustedProxies.matcher(currentRemoteIp).matches()) { Modified: tomcat/trunk/java/org/apache/catalina/valves/RemoteIpValve.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/valves/RemoteIpValve.java?rev=1832882&r1=1832881&r2=1832882&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/catalina/valves/RemoteIpValve.java (original) +++ tomcat/trunk/java/org/apache/catalina/valves/RemoteIpValve.java Mon Jun 4 18:57:59 2018 @@ -47,7 +47,8 @@ import org.apache.tomcat.util.http.MimeH * This valve proceeds as follows: * </p> * <p> - * If the incoming <code>request.getRemoteAddr()</code> matches the valve's list of internal proxies : + * If the incoming <code>request.getRemoteAddr()</code> matches the valve's list + * of internal or trusted proxies: * </p> * <ul> * <li>Loop on the comma delimited list of IPs and hostnames passed by the preceding load balancer or proxy in the given request's Http @@ -572,9 +573,11 @@ public class RemoteIpValve extends Valve final int originalServerPort = request.getServerPort(); final String originalProxiesHeader = request.getHeader(proxiesHeader); final String originalRemoteIpHeader = request.getHeader(remoteIpHeader); + boolean isInternal = internalProxies != null && + internalProxies.matcher(originalRemoteAddr).matches(); - if (internalProxies !=null && - internalProxies.matcher(originalRemoteAddr).matches()) { + if (isInternal || (trustedProxies != null && + trustedProxies.matcher(originalRemoteAddr).matches())) { String remoteIp = null; // In java 6, proxiesHeaderValue should be declared as a java.util.Deque LinkedList<String> proxiesHeaderValue = new LinkedList<>(); @@ -590,11 +593,14 @@ public class RemoteIpValve extends Valve String[] remoteIpHeaderValue = commaDelimitedListToStringArray(concatRemoteIpHeaderValue.toString()); int idx; + if (!isInternal) { + proxiesHeaderValue.addFirst(originalRemoteAddr); + } // 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 (internalProxies.matcher(currentRemoteIp).matches()) { + if (internalProxies !=null && internalProxies.matcher(currentRemoteIp).matches()) { // do nothing, internalProxies IPs are not appended to the } else if (trustedProxies != null && trustedProxies.matcher(currentRemoteIp).matches()) { Modified: tomcat/trunk/test/org/apache/catalina/filters/TestRemoteIpFilter.java URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/filters/TestRemoteIpFilter.java?rev=1832882&r1=1832881&r2=1832882&view=diff ============================================================================== --- tomcat/trunk/test/org/apache/catalina/filters/TestRemoteIpFilter.java (original) +++ tomcat/trunk/test/org/apache/catalina/filters/TestRemoteIpFilter.java Mon Jun 4 18:57:59 2018 @@ -348,6 +348,75 @@ public class TestRemoteIpFilter extends } @Test + public void testInvokeAllProxiesAreTrustedEmptyInternal() throws Exception { + + // PREPARE + RemoteIpFilter remoteIpFilter = new RemoteIpFilter(); + FilterDef filterDef = new FilterDef(); + filterDef.addInitParameter("internalProxies", ""); + filterDef.addInitParameter("trustedProxies", "proxy1|proxy2|proxy3"); + filterDef.addInitParameter("remoteIpHeader", "x-forwarded-for"); + filterDef.addInitParameter("proxiesHeader", "x-forwarded-by"); + + filterDef.setFilter(remoteIpFilter); + MockHttpServletRequest request = new MockHttpServletRequest(); + + request.setRemoteAddr("proxy3"); + request.setRemoteHost("remote-host-original-value"); + request.setHeader("x-forwarded-for", "140.211.11.130, proxy1, proxy2"); + + // TEST + HttpServletRequest actualRequest = testRemoteIpFilter(filterDef, request).getRequest(); + + // VERIFY + String actualXForwardedFor = actualRequest.getHeader("x-forwarded-for"); + Assert.assertNull("all proxies are trusted, x-forwarded-for must be null", actualXForwardedFor); + + String actualXForwardedBy = actualRequest.getHeader("x-forwarded-by"); + Assert.assertEquals("all proxies are trusted, they must appear in x-forwarded-by", "proxy1, proxy2, proxy3", actualXForwardedBy); + + String actualRemoteAddr = actualRequest.getRemoteAddr(); + Assert.assertEquals("remoteAddr", "140.211.11.130", actualRemoteAddr); + + String actualRemoteHost = actualRequest.getRemoteHost(); + Assert.assertEquals("remoteHost", "140.211.11.130", actualRemoteHost); + } + + @Test + public void testInvokeAllProxiesAreTrustedUnusedInternal() throws Exception { + + // PREPARE + RemoteIpFilter remoteIpFilter = new RemoteIpFilter(); + FilterDef filterDef = new FilterDef(); + filterDef.addInitParameter("trustedProxies", "proxy1|proxy2|proxy3"); + filterDef.addInitParameter("remoteIpHeader", "x-forwarded-for"); + filterDef.addInitParameter("proxiesHeader", "x-forwarded-by"); + + filterDef.setFilter(remoteIpFilter); + MockHttpServletRequest request = new MockHttpServletRequest(); + + request.setRemoteAddr("proxy3"); + request.setRemoteHost("remote-host-original-value"); + request.setHeader("x-forwarded-for", "140.211.11.130, proxy1, proxy2"); + + // TEST + HttpServletRequest actualRequest = testRemoteIpFilter(filterDef, request).getRequest(); + + // VERIFY + String actualXForwardedFor = actualRequest.getHeader("x-forwarded-for"); + Assert.assertNull("all proxies are trusted, x-forwarded-for must be null", actualXForwardedFor); + + String actualXForwardedBy = actualRequest.getHeader("x-forwarded-by"); + Assert.assertEquals("all proxies are trusted, they must appear in x-forwarded-by", "proxy1, proxy2, proxy3", actualXForwardedBy); + + String actualRemoteAddr = actualRequest.getRemoteAddr(); + Assert.assertEquals("remoteAddr", "140.211.11.130", actualRemoteAddr); + + String actualRemoteHost = actualRequest.getRemoteHost(); + Assert.assertEquals("remoteHost", "140.211.11.130", actualRemoteHost); + } + + @Test public void testInvokeAllProxiesAreTrustedAndRemoteAddrMatchRegexp() throws Exception { // PREPARE Modified: tomcat/trunk/test/org/apache/catalina/valves/TestRemoteIpValve.java URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/valves/TestRemoteIpValve.java?rev=1832882&r1=1832881&r2=1832882&view=diff ============================================================================== --- tomcat/trunk/test/org/apache/catalina/valves/TestRemoteIpValve.java (original) +++ tomcat/trunk/test/org/apache/catalina/valves/TestRemoteIpValve.java Mon Jun 4 18:57:59 2018 @@ -203,6 +203,87 @@ public class TestRemoteIpValve { } @Test + public void testInvokeAllProxiesAreTrustedEmptyInternal() throws Exception { + + // PREPARE + RemoteIpValve remoteIpValve = new RemoteIpValve(); + remoteIpValve.setInternalProxies(""); + remoteIpValve.setTrustedProxies("proxy1|proxy2|proxy3"); + remoteIpValve.setRemoteIpHeader("x-forwarded-for"); + remoteIpValve.setProxiesHeader("x-forwarded-by"); + RemoteAddrAndHostTrackerValve remoteAddrAndHostTrackerValve = new RemoteAddrAndHostTrackerValve(); + remoteIpValve.setNext(remoteAddrAndHostTrackerValve); + + Request request = new MockRequest(); + request.setCoyoteRequest(new org.apache.coyote.Request()); + request.setRemoteAddr("proxy3"); + request.setRemoteHost("remote-host-original-value"); + request.getCoyoteRequest().getMimeHeaders().addValue("x-forwarded-for").setString("140.211.11.130, proxy1, proxy2"); + + // TEST + remoteIpValve.invoke(request, null); + + // VERIFY + String actualXForwardedFor = remoteAddrAndHostTrackerValve.getForwardedFor(); + Assert.assertNull("all proxies are trusted, x-forwarded-for must be null", actualXForwardedFor); + + String actualXForwardedBy = remoteAddrAndHostTrackerValve.getForwardedBy(); + Assert.assertEquals("all proxies are trusted, they must appear in x-forwarded-by", "proxy1, proxy2, proxy3", actualXForwardedBy); + + String actualRemoteAddr = remoteAddrAndHostTrackerValve.getRemoteAddr(); + Assert.assertEquals("remoteAddr", "140.211.11.130", actualRemoteAddr); + + String actualRemoteHost = remoteAddrAndHostTrackerValve.getRemoteHost(); + Assert.assertEquals("remoteHost", "140.211.11.130", actualRemoteHost); + + String actualPostInvokeRemoteAddr = request.getRemoteAddr(); + Assert.assertEquals("postInvoke remoteAddr", "proxy3", actualPostInvokeRemoteAddr); + + String actualPostInvokeRemoteHost = request.getRemoteHost(); + Assert.assertEquals("postInvoke remoteAddr", "remote-host-original-value", actualPostInvokeRemoteHost); + } + + @Test + public void testInvokeAllProxiesAreTrustedUnusedInternal() throws Exception { + + // PREPARE + RemoteIpValve remoteIpValve = new RemoteIpValve(); + remoteIpValve.setTrustedProxies("proxy1|proxy2|proxy3"); + remoteIpValve.setRemoteIpHeader("x-forwarded-for"); + remoteIpValve.setProxiesHeader("x-forwarded-by"); + RemoteAddrAndHostTrackerValve remoteAddrAndHostTrackerValve = new RemoteAddrAndHostTrackerValve(); + remoteIpValve.setNext(remoteAddrAndHostTrackerValve); + + Request request = new MockRequest(); + request.setCoyoteRequest(new org.apache.coyote.Request()); + request.setRemoteAddr("proxy3"); + request.setRemoteHost("remote-host-original-value"); + request.getCoyoteRequest().getMimeHeaders().addValue("x-forwarded-for").setString("140.211.11.130, proxy1, proxy2"); + + // TEST + remoteIpValve.invoke(request, null); + + // VERIFY + String actualXForwardedFor = remoteAddrAndHostTrackerValve.getForwardedFor(); + Assert.assertNull("all proxies are trusted, x-forwarded-for must be null", actualXForwardedFor); + + String actualXForwardedBy = remoteAddrAndHostTrackerValve.getForwardedBy(); + Assert.assertEquals("all proxies are trusted, they must appear in x-forwarded-by", "proxy1, proxy2, proxy3", actualXForwardedBy); + + String actualRemoteAddr = remoteAddrAndHostTrackerValve.getRemoteAddr(); + Assert.assertEquals("remoteAddr", "140.211.11.130", actualRemoteAddr); + + String actualRemoteHost = remoteAddrAndHostTrackerValve.getRemoteHost(); + Assert.assertEquals("remoteHost", "140.211.11.130", actualRemoteHost); + + String actualPostInvokeRemoteAddr = request.getRemoteAddr(); + Assert.assertEquals("postInvoke remoteAddr", "proxy3", actualPostInvokeRemoteAddr); + + String actualPostInvokeRemoteHost = request.getRemoteHost(); + Assert.assertEquals("postInvoke remoteAddr", "remote-host-original-value", actualPostInvokeRemoteHost); + } + + @Test public void testInvokeAllProxiesAreTrustedOrInternal() throws Exception { // PREPARE Modified: tomcat/trunk/webapps/docs/changelog.xml URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1832882&r1=1832881&r2=1832882&view=diff ============================================================================== --- tomcat/trunk/webapps/docs/changelog.xml (original) +++ tomcat/trunk/webapps/docs/changelog.xml Mon Jun 4 18:57:59 2018 @@ -137,6 +137,12 @@ <code>internalProxies</code> regular expression. Patch by Craig Andrews. (markt) </add> + <fix> + In the <code>RemoteIpValve</code> and <code>RemoteIpFilter</code>, + correctly handle the case when the request passes through one or more + <code>trustedProxies</code> but no <code>internalProxies</code>. Based + on a patch by zhanhb. (markt) + </fix> </changelog> </subsection> <subsection name="Coyote"> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org