Skip to content

Commit

Permalink
Fix #2254 - Apply whitelist host protection to ProxyServlet
Browse files Browse the repository at this point in the history
  • Loading branch information
tadayosi committed Jan 17, 2017
1 parent 2e06770 commit 503ed37
Show file tree
Hide file tree
Showing 13 changed files with 196 additions and 50 deletions.
11 changes: 11 additions & 0 deletions hawtio-base/src/main/webapp/WEB-INF/web.xml
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,17 @@
<servlet>
<servlet-name>jolokia-proxy</servlet-name>
<servlet-class>io.hawt.web.ProxyServlet</servlet-class>
<!--
Comma-separated list of allowed target hosts. It is required for security.
'*' allows all hosts but keep in mind it's vulnerable to security attacks.
-->
<init-param>
<param-name>proxyWhitelist</param-name>
<param-value>
localhost,
127.0.0.1
</param-value>
</init-param>
<load-on-startup>1</load-on-startup>
</servlet>
<servlet-mapping>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@

<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-plugin-plugin</artifactId>
<version>3.2</version>
<configuration>
<!-- see http://jira.codehaus.org/browse/MNG-5346 -->
<skipErrorNoDescriptorsFound>true</skipErrorNoDescriptorsFound>
Expand Down
28 changes: 0 additions & 28 deletions hawtio-system/src/main/java/io/hawt/web/ProxyAddress.java

This file was deleted.

14 changes: 10 additions & 4 deletions hawtio-system/src/main/java/io/hawt/web/ProxyDetails.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.hawt.web;

import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import javax.servlet.http.HttpServletRequest;
Expand All @@ -11,7 +12,7 @@
/**
* A helper object to store the proxy location details
*/
public class ProxyDetails implements ProxyAddress {
public class ProxyDetails {
private static final transient Logger LOG = LoggerFactory.getLogger(ProxyDetails.class);

private boolean invalid;
Expand Down Expand Up @@ -99,14 +100,21 @@ private void parsePathInfo(String pathInfo) {
}
}

public boolean isAllowed(List<String> whitelist) {
if (whitelist.contains("*")) {
return true;
}
// host may contain port number! (e.g. "localhost:9000")
return whitelist.contains(host.split(":")[0]);
}

@Override
public String toString() {
return "ProxyDetails{" +
userName + "@" + getFullProxyUrl()
+ "}";
}

@Override
public String getFullProxyUrl() {
if (invalid) {
return null;
Expand Down Expand Up @@ -141,12 +149,10 @@ public int getPort() {
return port;
}

@Override
public String getUserName() {
return userName;
}

@Override
public String getPassword() {
return password;
}
Expand Down
55 changes: 40 additions & 15 deletions hawtio-system/src/main/java/io/hawt/web/ProxyServlet.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.hawt.web;

import io.hawt.system.Helpers;
import io.hawt.util.Strings;
import org.apache.commons.codec.binary.Base64;
import org.apache.http.*;
Expand All @@ -18,6 +19,8 @@
import org.apache.http.message.BasicHttpRequest;
import org.apache.http.message.HeaderGroup;
import org.apache.http.util.EntityUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.servlet.ServletConfig;
import javax.servlet.ServletException;
Expand All @@ -34,8 +37,10 @@
import java.security.NoSuchAlgorithmException;
import java.security.cert.X509Certificate;
import java.util.BitSet;
import java.util.Collections;
import java.util.Enumeration;
import java.util.Formatter;
import java.util.List;

/**
* An HTTP reverse proxy/gateway servlet. It is designed to be extended for customization
Expand All @@ -57,38 +62,39 @@
*/
public class ProxyServlet extends HttpServlet {

private static final transient Logger LOG = LoggerFactory.getLogger(ProxyServlet.class);

/* INIT PARAMETER NAME CONSTANTS */

/**
* A boolean parameter name to enable logging of input and target URLs to the servlet log.
*
* @deprecated Use SLF4J {@link Logger}
*/
@Deprecated
public static final String P_LOG = "log";

/**
* A boolean parameter name to enable forwarding of the client IP
*/
public static final String P_FORWARDEDFOR = "forwardip";

/**
* The parameter name for the target (destination) URI to proxy to.
*/
private static final String P_TARGET_URI = "targetUri";

/**
* Whether we accept self-signed SSL certificates
*/
private static final String PROXY_ACCEPT_SELF_SIGNED_CERTS = "hawtio.proxyDisableCertificateValidation";
private static final String PROXY_ACCEPT_SELF_SIGNED_CERTS_ENV = "PROXY_DISABLE_CERT_VALIDATION";

public static final String PROXY_WHITELIST = "proxyWhitelist";
public static final String HAWTIO_PROXY_WHITELIST = "hawtio." + PROXY_WHITELIST;

/* MISC */

protected boolean doLog = false;
protected boolean doForwardIP = true;
protected boolean acceptSelfSignedCerts = false;
/**
* User agents shouldn't send the url fragment but what if it does?
*/
protected boolean doSendUrlFragment = true;

protected List<String> whitelist;

protected CloseableHttpClient proxyClient;
private CookieStore cookieStore;
Expand All @@ -102,6 +108,17 @@ public String getServletInfo() {
public void init(ServletConfig servletConfig) throws ServletException {
super.init(servletConfig);

String whitelistStr = servletConfig.getInitParameter(PROXY_WHITELIST);
if (System.getProperty(HAWTIO_PROXY_WHITELIST) != null) {
whitelistStr = System.getProperty(HAWTIO_PROXY_WHITELIST);
}
if (Strings.isBlank(whitelistStr)) {
whitelist = Collections.emptyList();
} else {
whitelist = Collections.unmodifiableList(Strings.split(whitelistStr, ","));
}
LOG.info("Proxy whitelist: {}", whitelist);

String doForwardIPString = servletConfig.getInitParameter(P_FORWARDEDFOR);
if (doForwardIPString != null) {
this.doForwardIP = Boolean.parseBoolean(doForwardIPString);
Expand Down Expand Up @@ -148,6 +165,7 @@ public void destroy() {
proxyClient.close();
} catch (IOException e) {
log("While destroying servlet, shutting down httpclient: " + e, e);
LOG.error("While destroying servlet, shutting down httpclient: " + e, e);
}
super.destroy();
}
Expand All @@ -157,14 +175,19 @@ protected void service(HttpServletRequest servletRequest, HttpServletResponse se
throws ServletException, IOException {
// Make the Request
//note: we won't transfer the protocol version because I'm not sure it would truly be compatible
ProxyAddress proxyAddress = parseProxyAddress(servletRequest);
if (proxyAddress == null || proxyAddress.getFullProxyUrl() == null) {
ProxyDetails proxyDetails = parseProxyDetails(servletRequest);
if (proxyDetails == null || proxyDetails.getFullProxyUrl() == null) {
servletResponse.setStatus(HttpServletResponse.SC_NOT_FOUND);
return;
}
if (!proxyDetails.isAllowed(whitelist)) {
LOG.debug("Rejecting {}", proxyDetails);
Helpers.doForbidden(servletResponse);
return;
}

String method = servletRequest.getMethod();
String proxyRequestUri = proxyAddress.getFullProxyUrl();
String proxyRequestUri = proxyDetails.getFullProxyUrl();

URI targetUriObj;
try {
Expand All @@ -188,8 +211,8 @@ protected void service(HttpServletRequest servletRequest, HttpServletResponse se

copyRequestHeaders(servletRequest, proxyRequest, targetUriObj);

String username = proxyAddress.getUserName();
String password = proxyAddress.getPassword();
String username = proxyDetails.getUserName();
String password = proxyDetails.getPassword();

if (Strings.isNotBlank(username) && Strings.isNotBlank(password)) {
String encodedCreds = Base64.encodeBase64String((username + ":" + password).getBytes());
Expand Down Expand Up @@ -219,6 +242,7 @@ protected void service(HttpServletRequest servletRequest, HttpServletResponse se
if (doLog) {
log("proxy " + method + " uri: " + servletRequest.getRequestURI() + " -- " + proxyRequest.getRequestLine().getUri());
}
LOG.debug("proxy {} uri: {} -- {}", method, servletRequest.getRequestURI(), proxyRequest.getRequestLine().getUri());
proxyResponse = proxyClient.execute(URIUtils.extractHost(targetUriObj), proxyRequest);

// Process the response
Expand All @@ -228,6 +252,7 @@ protected void service(HttpServletRequest servletRequest, HttpServletResponse se
if (doLog) {
log("Authentication Failed on remote server " + proxyRequestUri);
}
LOG.debug("Authentication Failed on remote server {}", proxyRequestUri);
} else if (doResponseRedirectOrNotModifiedLogic(servletRequest, servletResponse, proxyResponse, statusCode, targetUriObj)) {
//the response is already "committed" now without any body to send
//TODO copy response headers?
Expand Down Expand Up @@ -268,7 +293,7 @@ protected void service(HttpServletRequest servletRequest, HttpServletResponse se
}
}

protected ProxyAddress parseProxyAddress(HttpServletRequest servletRequest) {
protected ProxyDetails parseProxyDetails(HttpServletRequest servletRequest) {
return new ProxyDetails(servletRequest);
}

Expand Down
42 changes: 42 additions & 0 deletions hawtio-system/src/test/java/io/hawt/web/ProxyDetailsTest.java
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
package io.hawt.web;

import java.util.Arrays;
import java.util.List;

import org.junit.Test;

import javax.servlet.http.HttpServletRequest;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

Expand Down Expand Up @@ -234,4 +239,41 @@ public void testInvalidQuery() throws Exception {
assertEquals("getScheme()", "http", details.getScheme());
assertEquals("getStringProxyURL()", "http://127.0.0.1:54155/jolokia//exec/org.apache.camel:context=camel-1,type=context,name=%22camel-1%22/canSendToEndpoint(java.lang.String)/activemq:!/!/queue:newOrder?maxDepth=7&maxCollectionSize=5000&ignoreErrors=true&canonicalNaming=false", details.getFullProxyUrl());
}

@Test
public void testIsAllowed() throws Exception {
HttpServletRequest mockReq = mock(HttpServletRequest.class);
when(mockReq.getPathInfo())
.thenReturn("/localhost/9000/jolokia/")
.thenReturn("/localhost:8181/jolokia/")
.thenReturn("/www.myhost.com/jolokia/")
.thenReturn("/www.banned.com/jolokia/");

List<String> whitelist = Arrays.asList("localhost", "www.myhost.com");
ProxyDetails details1 = new ProxyDetails(mockReq);
ProxyDetails details2 = new ProxyDetails(mockReq);
ProxyDetails details3 = new ProxyDetails(mockReq);
ProxyDetails details4 = new ProxyDetails(mockReq);
assertTrue("localhost/9000", details1.isAllowed(whitelist));
assertTrue("localhost:8181", details2.isAllowed(whitelist));
assertTrue("www.myhost.com", details3.isAllowed(whitelist));
assertFalse("www.banned.com", details4.isAllowed(whitelist));
}

@Test
public void testIsAllowedWithAllowAll() throws Exception {
HttpServletRequest mockReq = mock(HttpServletRequest.class);
when(mockReq.getPathInfo())
.thenReturn("/localhost/9000/jolokia/")
.thenReturn("/www.myhost.com/jolokia/")
.thenReturn("/www.banned.com/jolokia/");

List<String> whitelist = Arrays.asList("*");
ProxyDetails details1 = new ProxyDetails(mockReq);
ProxyDetails details2 = new ProxyDetails(mockReq);
ProxyDetails details3 = new ProxyDetails(mockReq);
assertTrue("localhost", details1.isAllowed(whitelist));
assertTrue("www.myhost.com", details2.isAllowed(whitelist));
assertTrue("www.banned.com", details3.isAllowed(whitelist));
}
}
17 changes: 16 additions & 1 deletion hawtio-util/src/main/java/io/hawt/util/Strings.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package io.hawt.util;

import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;

/**
* String utility.
*/
public class Strings {
public static String trimString(String value, int max) {
Expand Down Expand Up @@ -32,7 +37,7 @@ public static String sanitize(String name) {
if (isBlank(name)) {
return name;
}
return name.replaceAll("[^0-9a-zA-Z\\+\\.\\(\\)_\\-]","");
return name.replaceAll("[^0-9a-zA-Z\\+\\.\\(\\)_\\-]", "");
}

/**
Expand All @@ -47,4 +52,14 @@ public static String sanitizeDirectory(String name) {
}
return sanitize(name).replace(".", "");
}

public static List<String> split(String text, String delimiter) {
if (text == null || delimiter == null) {
throw new IllegalArgumentException("Both 'text' and 'delimiter' should not be null.");
}
return Arrays.stream(text.split(delimiter))
.map(s -> s.trim())
.filter(s -> isNotBlank(s))
.collect(Collectors.toList());
}
}
32 changes: 32 additions & 0 deletions hawtio-util/src/test/java/io/hawt/util/StringsTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package io.hawt.util;

import java.util.Arrays;

import org.junit.Test;

import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;

public class StringsTest {

@Test
public void split() {
assertThat(Strings.split("abc", ","), is(Arrays.asList("abc")));
assertThat(Strings.split("a,b,c", ","), is(Arrays.asList("a", "b", "c")));
assertThat(Strings.split("a, b, c", ","), is(Arrays.asList("a", "b", "c")));
assertThat(Strings.split(",a,,b,,c,", ","), is(Arrays.asList("a", "b", "c")));
try {
Strings.split(null, ",");
fail();
} catch (IllegalArgumentException e) {
// success
}
try {
Strings.split("a, b, c", null);
fail();
} catch (IllegalArgumentException e) {
// success
}
}
}
Loading

0 comments on commit 503ed37

Please sign in to comment.