knox-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kris...@apache.org
Subject [knox] branch master updated: KNOX-1842 - Upgrade httpclient to 4.5.10 (#176)
Date Tue, 05 Nov 2019 15:08:08 GMT
This is an automated email from the ASF dual-hosted git repository.

krisden pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/knox.git


The following commit(s) were added to refs/heads/master by this push:
     new d9a3eb9  KNOX-1842 - Upgrade httpclient to 4.5.10 (#176)
d9a3eb9 is described below

commit d9a3eb9307e91ef1b40d72fed9f22511fe4b1646
Author: Kevin Risden <risdenk@users.noreply.github.com>
AuthorDate: Tue Nov 5 09:38:44 2019 -0500

    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>
---
 .../gateway/dispatch/DefaultHttpClientFactory.java | 11 +++-
 .../dispatch/DefaultHttpClientFactoryTest.java     | 29 ++++++++-
 .../apache/knox/test/mock/MockRequestMatcher.java  | 14 +++-
 .../apache/knox/gateway/GatewayBasicFuncTest.java  | 74 ++++++++++++++++++++++
 pom.xml                                            |  2 +-
 5 files changed, 125 insertions(+), 5 deletions(-)

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 @@ public class DefaultHttpClientFactory implements HttpClientFactory {
     }
   }
 
-  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 @@ public class DefaultHttpClientFactory implements HttpClientFactory {
     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.replay;
 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 @@ public class DefaultHttpClientFactoryTest {
     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 @@ public class DefaultHttpClientFactoryTest {
     }
     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 @@ public class MockRequestMatcher {
   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 @@ public class MockRequestMatcher {
     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 @@ public class MockRequestMatcher {
               " 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 @@ public class MockRequestMatcher {
 
   @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.response.Response;
 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.AuthScope;
 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.MatcherAssert.assertThat;
 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 @@ public class GatewayBasicFuncTest {
   }
 
   @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>


Mime
View raw message