KNOX-1375 - Dispatch whitelist validation should decode URLs before testing
authorPhil Zampino <pzampino@apache.org>
Tue, 3 Jul 2018 21:19:47 +0000 (17:19 -0400)
committerPhil Zampino <pzampino@apache.org>
Tue, 3 Jul 2018 21:19:47 +0000 (17:19 -0400)
gateway-service-knoxsso/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOResource.java
gateway-service-knoxsso/src/test/java/org/apache/knox/gateway/service/knoxsso/WebSSOResourceTest.java
gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/GatewayDispatchFilter.java
gateway-spi/src/main/java/org/apache/knox/gateway/util/WhitelistUtils.java
gateway-spi/src/test/java/org/apache/knox/gateway/dispatch/GatewayDispatchFilterTest.java

index f207432..cda3d48 100644 (file)
 package org.apache.knox.gateway.service.knoxsso;
 
 import java.io.IOException;
+import java.io.UnsupportedEncodingException;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.net.URLDecoder;
 import java.security.Principal;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -168,8 +170,8 @@ public class WebSSOResource {
   }
 
   private Response getAuthenticationToken(int statusCode) {
-    GatewayServices services = (GatewayServices) request.getServletContext()
-            .getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE);
+    GatewayServices services =
+                (GatewayServices) request.getServletContext().getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE);
     boolean removeOriginalUrlCookie = true;
     String original = getCookieValue((HttpServletRequest) request, ORIGINAL_URL_COOKIE_NAME);
     if (original == null) {
@@ -182,11 +184,25 @@ public class WebSSOResource {
         throw new WebApplicationException("Original URL not found in the request.", Response.Status.BAD_REQUEST);
       }
 
-      boolean validRedirect = (whitelist == null) || RegExUtils.checkWhitelist(whitelist, original);
+      boolean validRedirect = true;
+
+      // If there is a whitelist defined, then the original URL must be validated against it.
+      // If there is no whitelist, then everything is valid.
+      if (whitelist != null) {
+        String decodedOriginal = null;
+        try {
+          decodedOriginal = URLDecoder.decode(original, "UTF-8");
+        } catch (UnsupportedEncodingException e) {
+          //
+        }
+
+        validRedirect = RegExUtils.checkWhitelist(whitelist, (decodedOriginal != null ? decodedOriginal : original));
+      }
+
       if (!validRedirect) {
         log.whiteListMatchFail(original, whitelist);
         throw new WebApplicationException("Original URL not valid according to the configured whitelist.",
-                Response.Status.BAD_REQUEST);
+                                          Response.Status.BAD_REQUEST);
       }
     }
 
index 58877e4..19a8f03 100644 (file)
@@ -17,6 +17,7 @@
  */
 package org.apache.knox.gateway.service.knoxsso;
 
+import org.apache.http.HttpStatus;
 import org.apache.knox.gateway.config.GatewayConfig;
 import org.apache.knox.gateway.util.RegExUtils;
 import static org.junit.Assert.assertEquals;
@@ -25,6 +26,7 @@ import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
 import java.lang.reflect.Field;
+import java.net.URLEncoder;
 import java.security.KeyPair;
 import java.security.KeyPairGenerator;
 import java.security.NoSuchAlgorithmException;
@@ -46,6 +48,7 @@ import javax.servlet.http.Cookie;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpServletResponseWrapper;
+import javax.ws.rs.WebApplicationException;
 
 import org.apache.knox.gateway.services.GatewayServices;
 import org.apache.knox.gateway.services.security.token.JWTokenAuthority;
@@ -645,6 +648,63 @@ public class WebSSOResourceTest {
   }
 
   @Test
+  public void testWhitelistValidationWithEncodedOriginalURL() throws Exception {
+    GatewayConfig config = EasyMock.createNiceMock(GatewayConfig.class);
+    EasyMock.expect(config.getDispatchWhitelistServices()).andReturn(Collections.emptyList()).anyTimes();
+    EasyMock.expect(config.getDispatchWhitelist()).andReturn(null).anyTimes();
+    EasyMock.replay(config);
+
+    ServletContext context = EasyMock.createNiceMock(ServletContext.class);
+    EasyMock.expect(context.getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE)).andReturn(config).anyTimes();
+    EasyMock.expect(context.getInitParameter("knoxsso.cookie.name")).andReturn(null);
+    EasyMock.expect(context.getInitParameter("knoxsso.cookie.secure.only")).andReturn(null);
+    EasyMock.expect(context.getInitParameter("knoxsso.cookie.max.age")).andReturn(null);
+    EasyMock.expect(context.getInitParameter("knoxsso.cookie.domain.suffix")).andReturn(null);
+    EasyMock.expect(context.getInitParameter("knoxsso.redirect.whitelist.regex")).andReturn(null);
+    EasyMock.expect(context.getInitParameter("knoxsso.token.audiences")).andReturn(null);
+    EasyMock.expect(context.getInitParameter("knoxsso.token.ttl")).andReturn(null);
+    EasyMock.expect(context.getInitParameter("knoxsso.enable.session")).andReturn(null);
+
+    HttpServletRequest request = EasyMock.createNiceMock(HttpServletRequest.class);
+    EasyMock.expect(request.getParameter("originalUrl")).andReturn(URLEncoder.encode("http://disallowedhost:9080/service",
+                                                                                     "UTF-8"));
+    EasyMock.expect(request.getAttribute("targetServiceRole")).andReturn("KNOXSSO").anyTimes();
+    EasyMock.expect(request.getParameterMap()).andReturn(Collections.<String,String[]>emptyMap());
+    EasyMock.expect(request.getServletContext()).andReturn(context).anyTimes();
+
+    Principal principal = EasyMock.createNiceMock(Principal.class);
+    EasyMock.expect(principal.getName()).andReturn("alice").anyTimes();
+    EasyMock.expect(request.getUserPrincipal()).andReturn(principal).anyTimes();
+
+    GatewayServices services = EasyMock.createNiceMock(GatewayServices.class);
+    EasyMock.expect(context.getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE)).andReturn(services);
+
+    JWTokenAuthority authority = new TestJWTokenAuthority(publicKey, privateKey);
+    EasyMock.expect(services.getService(GatewayServices.TOKEN_SERVICE)).andReturn(authority);
+
+    HttpServletResponse response = EasyMock.createNiceMock(HttpServletResponse.class);
+    ServletOutputStream outputStream = EasyMock.createNiceMock(ServletOutputStream.class);
+    CookieResponseWrapper responseWrapper = new CookieResponseWrapper(response, outputStream);
+
+    EasyMock.replay(principal, services, context, request);
+
+    WebSSOResource webSSOResponse = new WebSSOResource();
+    webSSOResponse.request = request;
+    webSSOResponse.response = responseWrapper;
+    webSSOResponse.context = context;
+    webSSOResponse.init();
+
+    try {
+      webSSOResponse.doGet();
+    } catch (WebApplicationException e) {
+      // Expected
+      int status = e.getResponse().getStatus();
+      assertEquals(HttpStatus.SC_BAD_REQUEST, status);
+    }
+  }
+
+
+  @Test
   public void testTopologyDefinedWhitelist() throws Exception {
     final String testServiceRole = "TEST";
 
index f4d8383..f23ba55 100644 (file)
@@ -32,7 +32,9 @@ import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import java.io.IOException;
+import java.io.UnsupportedEncodingException;
 import java.net.URISyntaxException;
+import java.net.URLDecoder;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
@@ -140,7 +142,14 @@ public class GatewayDispatchFilter extends AbstractGatewayFilter {
       if (whitelist != null) {
         String requestURI = request.getRequestURI();
 
-        isAllowed = RegExUtils.checkWhitelist(whitelist, requestURI);
+        String decodedURL = null;
+        try {
+          decodedURL = URLDecoder.decode(requestURI, "UTF-8");
+        } catch (UnsupportedEncodingException e) {
+          //
+        }
+
+        isAllowed = RegExUtils.checkWhitelist(whitelist, (decodedURL != null ? decodedURL : requestURI));
 
         if (!isAllowed) {
           LOG.dispatchDisallowed(requestURI);
index 37df2f6..ae5e519 100644 (file)
@@ -44,7 +44,8 @@ public class WhitelistUtils {
   public static String getDispatchWhitelist(HttpServletRequest request) {
     String whitelist = null;
 
-    GatewayConfig config = (GatewayConfig) request.getServletContext().getAttribute("org.apache.knox.gateway.config");
+    GatewayConfig config =
+                      (GatewayConfig) request.getServletContext().getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE);
     if (config != null) {
       List<String> whitelistedServiceRoles = new ArrayList<>();
       whitelistedServiceRoles.addAll(DEFAULT_SERVICE_ROLES);
index 1ad7b18..02f6fa1 100644 (file)
@@ -26,6 +26,7 @@ import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import java.io.IOException;
 import java.lang.reflect.Method;
+import java.net.URLEncoder;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
@@ -88,7 +89,8 @@ public class GatewayDispatchFilterTest {
     doTestServiceDispatchWhitelist(Collections.singletonList(serviceRole),
                                    null,
                                    serviceRole,
-                                   "http://www.notonmylist.org:9999", false);
+                                   "http://www.notonmylist.org:9999",
+                                   false);
   }
 
   /**
@@ -118,7 +120,8 @@ public class GatewayDispatchFilterTest {
                                    "knoxbox.test.org",
                                    null,
                                    serviceRole,
-                                   "http://www.notonmylist.org:9999", false);
+                                   "http://www.notonmylist.org:9999",
+                                   false);
   }
 
   /**
@@ -137,6 +140,38 @@ public class GatewayDispatchFilterTest {
   }
 
   /**
+   * If the dispatch service is configured to honor the whitelist, but no whitelist is configured, then the default
+   * whitelist should be applied. If the dispatch URL does match the default whitelist, then the dispatch should be
+   * allowed.
+   */
+  @Test
+  public void testServiceDispatchWhitelistNoWhiteListForRole_encodedurl_valid() throws Exception {
+    final String serviceRole = "TESTROLE";
+    doTestServiceDispatchWhitelist(Collections.singletonList(serviceRole),
+                                   null,
+                                   serviceRole,
+                                   URLEncoder.encode("http://localhost:9999", "UTF-8"),
+                                   true);
+  }
+
+
+  /**
+   * If the dispatch service is configured to honor the whitelist, but DEFAULT whitelist is configured, then the default
+   * whitelist should be applied. If the dispatch URL does match the default whitelist, then the dispatch should be
+   * allowed.
+   */
+  @Test
+  public void testServiceDispatchWhitelistNoWhiteListForRole_encodedurl_invalid() throws Exception {
+    final String serviceRole = "TESTROLE";
+    doTestServiceDispatchWhitelist(Collections.singletonList(serviceRole),
+                                   "DEFAULT",
+                                   serviceRole,
+                                   URLEncoder.encode("http://www.notonmylist.org:9999", "UTF-8"),
+                                   false);
+  }
+
+
+  /**
    * An empty whitelist should be treated as the default whitelist.
    */
   @Test