KNOX-1842 - Upgrade httpclient to 4.5.10 (#176)
HttpClient 4.5.7 broke url normalization. Knox
didn't have any tests for this case and so we
had to revert after the fact. HttpClient 4.5.8
fixed a lot of the url normalization and some
libraries decided to turn url normalization off.
This commit does the following:
* Adds a test for %2F - KNOX-1005
* This test passes under HttpClient 4.5.6 and 4.5.8+
* It breaks as expected under HttpClient 4.5.7
* Adds an explicit config enabling url normalization
* Ensures that we are in control of url normalization
* Adds a test for this configuration as well
* Test with both HttpClient normalization enabled and disabled
* `rest-assured` doesn't expose `RequestConfig` to disable
url normalization
* Shows how to use HttpClient in `GatewayBasicFuncTest`
All the url safe characters like %2F are fixed by HTTPCLIENT-1968.
The case of `/abc///def` is normalized to `/abc/def` the same
way that Knox does internally with `getPathInfo` and Java URI.
Signed-off-by: Kevin Risden <krisden@apache.org>diff --git a/gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/DefaultHttpClientFactory.java b/gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/DefaultHttpClientFactory.java
index d505d12..2db6c12 100644
--- a/gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/DefaultHttpClientFactory.java
+++ b/gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/DefaultHttpClientFactory.java
@@ -192,7 +192,7 @@
}
}
- private static RequestConfig getRequestConfig( FilterConfig config ) {
+ static RequestConfig getRequestConfig( FilterConfig config ) {
RequestConfig.Builder builder = RequestConfig.custom();
int connectionTimeout = getConnectionTimeout( config );
if ( connectionTimeout != -1 ) {
@@ -203,6 +203,15 @@
if( socketTimeout != -1 ) {
builder.setSocketTimeout( socketTimeout );
}
+
+ // HttpClient 4.5.7 is broken for %2F handling with url normalization.
+ // However, HttpClient 4.5.8+ (HTTPCLIENT-1968) has reasonable url
+ // normalization that matches what Knox already does related to url handling.
+ //
+ // If this view changes later, need to change here as well as make sure
+ // rest-assured doesn't use the old HttpClient behavior.
+ builder.setNormalizeUri(true);
+
return builder.build();
}
diff --git a/gateway-spi/src/test/java/org/apache/knox/gateway/dispatch/DefaultHttpClientFactoryTest.java b/gateway-spi/src/test/java/org/apache/knox/gateway/dispatch/DefaultHttpClientFactoryTest.java
index 14a55a3..6ceeeb2 100644
--- a/gateway-spi/src/test/java/org/apache/knox/gateway/dispatch/DefaultHttpClientFactoryTest.java
+++ b/gateway-spi/src/test/java/org/apache/knox/gateway/dispatch/DefaultHttpClientFactoryTest.java
@@ -26,11 +26,13 @@
import static org.easymock.EasyMock.verify;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
import org.apache.http.client.HttpClient;
+import org.apache.http.client.config.RequestConfig;
import org.apache.knox.gateway.config.GatewayConfig;
-import org.apache.knox.gateway.services.ServiceType;
import org.apache.knox.gateway.services.GatewayServices;
+import org.apache.knox.gateway.services.ServiceType;
import org.apache.knox.gateway.services.security.AliasService;
import org.apache.knox.gateway.services.security.KeystoreService;
import org.junit.Test;
@@ -200,6 +202,29 @@
verify(keystoreService, gatewayServices, filterConfig);
}
+ @Test
+ public void testHttpClientPathNormalization() {
+ GatewayConfig gatewayConfig = createMock(GatewayConfig.class);
+ expect(gatewayConfig.getHttpClientConnectionTimeout()).andReturn(20000).once();
+ expect(gatewayConfig.getHttpClientSocketTimeout()).andReturn(20000).once();
+
+ ServletContext servletContext = createMock(ServletContext.class);
+ expect(servletContext.getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE)).andReturn(gatewayConfig).atLeastOnce();
+
+ FilterConfig filterConfig = createMock(FilterConfig.class);
+ expect(filterConfig.getServletContext()).andReturn(servletContext).atLeastOnce();
+ expect(filterConfig.getInitParameter("httpclient.connectionTimeout")).andReturn(null).once();
+ expect(filterConfig.getInitParameter("httpclient.socketTimeout")).andReturn(null).once();
+
+ replay(gatewayConfig, servletContext, filterConfig);
+
+ RequestConfig requestConfig = DefaultHttpClientFactory.getRequestConfig(filterConfig);
+
+ assertTrue(requestConfig.isNormalizeUri());
+
+ verify(gatewayConfig, servletContext, filterConfig);
+ }
+
private KeyStore loadKeyStore(String keyStoreFile, String password, String storeType)
throws CertificateException, IOException, KeyStoreException, NoSuchAlgorithmException {
KeyStore keyStore = KeyStore.getInstance(storeType);
@@ -208,4 +233,4 @@
}
return keyStore;
}
-}
\ No newline at end of file
+}
diff --git a/gateway-test-utils/src/main/java/org/apache/knox/test/mock/MockRequestMatcher.java b/gateway-test-utils/src/main/java/org/apache/knox/test/mock/MockRequestMatcher.java
index 9191922..f8f6915 100644
--- a/gateway-test-utils/src/main/java/org/apache/knox/test/mock/MockRequestMatcher.java
+++ b/gateway-test-utils/src/main/java/org/apache/knox/test/mock/MockRequestMatcher.java
@@ -56,6 +56,7 @@
private MockResponseProvider response;
private Set<String> methods;
private String pathInfo;
+ private String requestURI;
private String requestURL;
Map<String,Matcher> headers;
Set<Cookie> cookies;
@@ -95,6 +96,11 @@
return this;
}
+ public MockRequestMatcher requestURI( String requestURI ) {
+ this.requestURI = requestURI;
+ return this;
+ }
+
public MockRequestMatcher requestUrl( String requestUrl ) {
this.requestURL = requestUrl;
return this;
@@ -208,6 +214,12 @@
" does not have the expected pathInfo",
request.getPathInfo(), is( pathInfo ) );
}
+ if( requestURI != null ) {
+ assertThat(
+ "Request " + request.getMethod() + " " + request.getRequestURL() +
+ " does not have the expected requestURI",
+ request.getRequestURI(), is(requestURI) );
+ }
if( requestURL != null ) {
assertThat(
"Request " + request.getMethod() + " " + request.getRequestURL() +
@@ -319,7 +331,7 @@
@Override
public String toString() {
- return "from=" + from + ", pathInfo=" + pathInfo;
+ return "from=" + from + ", pathInfo=" + pathInfo + ", requestURI=" + requestURI;
}
private static List<NameValuePair> parseQueryString( String queryString ) {
diff --git a/gateway-test/src/test/java/org/apache/knox/gateway/GatewayBasicFuncTest.java b/gateway-test/src/test/java/org/apache/knox/gateway/GatewayBasicFuncTest.java
index e79b827..307f007 100644
--- a/gateway-test/src/test/java/org/apache/knox/gateway/GatewayBasicFuncTest.java
+++ b/gateway-test/src/test/java/org/apache/knox/gateway/GatewayBasicFuncTest.java
@@ -28,6 +28,7 @@
import io.restassured.specification.ResponseSpecification;
import org.apache.commons.io.filefilter.WildcardFileFilter;
import org.apache.commons.lang3.ArrayUtils;
+import org.apache.http.HttpHeaders;
import org.apache.http.HttpHost;
import org.apache.http.HttpResponse;
import org.apache.http.HttpStatus;
@@ -35,15 +36,19 @@
import org.apache.http.auth.UsernamePasswordCredentials;
import org.apache.http.client.AuthCache;
import org.apache.http.client.CredentialsProvider;
+import org.apache.http.client.config.RequestConfig;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpPost;
+import org.apache.http.client.methods.HttpPut;
import org.apache.http.client.protocol.HttpClientContext;
+import org.apache.http.entity.ByteArrayEntity;
import org.apache.http.entity.StringEntity;
import org.apache.http.impl.auth.BasicScheme;
import org.apache.http.impl.client.BasicAuthCache;
import org.apache.http.impl.client.BasicCredentialsProvider;
import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.impl.client.HttpClients;
import org.apache.http.util.EntityUtils;
import org.apache.knox.gateway.util.KnoxCLI;
import org.apache.knox.test.TestUtils;
@@ -99,6 +104,7 @@
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.text.IsEmptyString.isEmptyString;
+import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.xmlmatchers.XmlMatchers.isEquivalentTo;
import static org.xmlmatchers.transform.XmlConverters.the;
@@ -1665,6 +1671,74 @@
}
@Test( timeout = TestUtils.MEDIUM_TIMEOUT )
+ public void testEncodedForwardSlash() throws IOException {
+ LOG_ENTER();
+ String username = "hbase";
+ String password = "hbase-password";
+
+ String resourceName = "hbase/table-data";
+ String singleRowPath = "/table/%2F%2Ftestrow";
+
+ //PUT request
+ driver.getMock( "WEBHBASE" )
+ .expect()
+ .method( "PUT" )
+ .requestURI( singleRowPath ) // Need to use requestURI since pathInfo is url decoded
+ .contentType( ContentType.JSON.toString() )
+ .respond()
+ .status( HttpStatus.SC_OK );
+
+ CredentialsProvider credentialsProvider = new BasicCredentialsProvider();
+ UsernamePasswordCredentials credentials = new UsernamePasswordCredentials(username, password);
+ credentialsProvider.setCredentials(AuthScope.ANY, credentials);
+ // Show that normalizing from HttpClient doesn't change the behavior of %2F handling
+ // with HttpClient 4.5.8+ - HTTPCLIENT-1968
+ RequestConfig requestConfig = RequestConfig.custom().setNormalizeUri(false).build();
+ try(CloseableHttpClient httpClient = HttpClients.custom()
+ .setDefaultCredentialsProvider(credentialsProvider)
+ .setDefaultRequestConfig(requestConfig)
+ .build()) {
+
+ HttpPut httpPut = new HttpPut(driver.getUrl("WEBHBASE") + singleRowPath);
+ httpPut.setHeader("X-XSRF-Header", "jksdhfkhdsf");
+ httpPut.setHeader(HttpHeaders.CONTENT_TYPE, org.apache.http.entity.ContentType.APPLICATION_JSON.getMimeType());
+ httpPut.setEntity(new ByteArrayEntity(driver.getResourceBytes(resourceName + ".json")));
+
+ HttpResponse response = httpClient.execute(httpPut);
+ assertEquals(200, response.getStatusLine().getStatusCode());
+ }
+ driver.assertComplete();
+
+ driver.getMock( "WEBHBASE" )
+ .expect()
+ .method( "PUT" )
+ .requestURI( singleRowPath ) // Need to use requestURI since pathInfo is url decoded
+ .contentType( ContentType.JSON.toString() )
+ .respond()
+ .status( HttpStatus.SC_OK );
+
+ // There is no way to change the normalization behavior of the
+ // HttpClient created by rest-assured since RequestConfig isn't
+ // exposed. Instead the above test uses HttpClient directly,
+ // this shows that url normalization doesn't matter with 4.5.8+.
+ // If this view changes, don't use rest-assured for this type of
+ // testing.
+ // See: https://github.com/rest-assured/rest-assured/issues/497
+ given()
+ .urlEncodingEnabled(false) // make sure to avoid rest-assured automatic url encoding
+ .auth().preemptive().basic( username, password )
+ .header("X-XSRF-Header", "jksdhfkhdsf")
+ .body( driver.getResourceBytes( resourceName + ".json" ) )
+ .contentType( ContentType.JSON.toString() )
+ .then()
+ .statusCode( HttpStatus.SC_OK )
+ .when().put(driver.getUrl("WEBHBASE") + singleRowPath);
+ driver.assertComplete();
+
+ LOG_EXIT();
+ }
+
+ @Test( timeout = TestUtils.MEDIUM_TIMEOUT )
public void testHBaseInsertDataIntoTable() throws IOException {
LOG_ENTER();
String username = "hbase";
diff --git a/pom.xml b/pom.xml
index 6ffe6e4..ffa1465 100644
--- a/pom.xml
+++ b/pom.xml
@@ -186,7 +186,7 @@
<hadoop.version>3.2.1</hadoop.version>
<hamcrest.version>2.2</hamcrest.version>
<hamcrest-json.version>0.2</hamcrest-json.version>
- <httpclient.version>4.5.6</httpclient.version>
+ <httpclient.version>4.5.10</httpclient.version>
<httpcore.version>4.4.12</httpcore.version>
<j2e-pac4j.version>4.1.0</j2e-pac4j.version>
<jackson.version>2.10.0</jackson.version>
