Patch for client connection leak when using digest authentication Choose Digest over Basic if the server returns both

Author: alessandro.gherardi

connectors/apache-connector/src/test/java/org/glassfish/jersey/apache/connector/AuthTest.java

@@ -64,6 +64,7 @@
 import org.apache.http.auth.AuthScope;
 import org.apache.http.auth.UsernamePasswordCredentials;
 import org.apache.http.client.CredentialsProvider;
+import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
 import org.junit.Ignore;
 import org.junit.Test;
 import static org.junit.Assert.assertEquals;
@@ -168,6 +169,38 @@ public String getFilter(@Context HttpHeaders h) {
             return "GET";
         }
 
+        @GET
+        @Path("digest")
+        public String getDigest(@Context HttpHeaders h) {
+            String value = h.getRequestHeaders().getFirst("Authorization");
+            if (value == null) {
+                throw new WebApplicationException(
+                        Response.status(401).header("WWW-Authenticate", "Digest realm=\"WallyWorld\"")
+                            .entity("Forbidden").build());
+            }
+
+            return "GET";
+        }
+
+        @GET
+        @Path("basicAndDigest")
+        public String getBasicAndDigest(@Context HttpHeaders h) {
+            String value = h.getRequestHeaders().getFirst("Authorization");
+            if (value == null) {
+                throw new WebApplicationException(
+                        Response.status(401).header("WWW-Authenticate", "Basic realm=\"WallyWorld\"")
+                            .header("WWW-Authenticate", "Digest realm=\"WallyWorld\"")
+                            .entity("Forbidden").build());
+            } else if (value.startsWith("Basic")) {
+                throw new WebApplicationException(
+                        Response.status(401).header("WWW-Authenticate", "Basic realm=\"WallyWorld\"")
+                            .header("WWW-Authenticate", "Digest realm=\"WallyWorld\"")
+                            .entity("Digest authentication expected").build());
+            }
+
+            return "GET";
+        }
+
         @POST
         public String post(@Context HttpHeaders h, String e) {
             requestCount++;
@@ -259,6 +292,37 @@ public void testAuthGetWithClientFilter() {
         assertEquals("GET", r.request().get(String.class));
     }
 
+    @Test
+    public void testAuthGetWithDigestFilter() {
+        ClientConfig cc = new ClientConfig();
+        PoolingHttpClientConnectionManager cm = new PoolingHttpClientConnectionManager();
+        cc.connectorProvider(new ApacheConnectorProvider());
+        cc.property(ApacheClientProperties.CONNECTION_MANAGER, cm);
+        Client client = ClientBuilder.newClient(cc);
+        client.register(HttpAuthenticationFeature.universal("name", "password"));
+        WebTarget r = client.target(getBaseUri()).path("test/digest");
+
+        assertEquals("GET", r.request().get(String.class));
+
+        // Verify the connection that was used for the request is available for reuse
+        // and no connections are leased
+        assertEquals(cm.getTotalStats().getAvailable(), 1);
+        assertEquals(cm.getTotalStats().getLeased(), 0);
+    }
+
+    @Test
+    public void testAuthGetWithBasicAndDigestFilter() {
+        ClientConfig cc = new ClientConfig();
+        PoolingHttpClientConnectionManager cm = new PoolingHttpClientConnectionManager();
+        cc.connectorProvider(new ApacheConnectorProvider());
+        cc.property(ApacheClientProperties.CONNECTION_MANAGER, cm);
+        Client client = ClientBuilder.newClient(cc);
+        client.register(HttpAuthenticationFeature.universal("name", "password"));
+        WebTarget r = client.target(getBaseUri()).path("test/basicAndDigest");
+
+        assertEquals("GET", r.request().get(String.class));
+    }
+
     @Test
     @Ignore("JERSEY-1750: Cannot retry request with a non-repeatable request entity. How to buffer the entity?"
             + " Allow repeatable write in jersey?")

core-client/src/main/java/org/glassfish/jersey/client/authentication/BasicAuthenticator.java

@@ -40,6 +40,8 @@
 
 package org.glassfish.jersey.client.authentication;
 
+import java.io.IOException;
+
 import javax.ws.rs.client.ClientRequestContext;
 import javax.ws.rs.client.ClientResponseContext;
 import javax.ws.rs.core.HttpHeaders;
@@ -113,7 +115,8 @@ public void filterRequest(ClientRequestContext request) throws RequestAuthentica
      * new request was done with digest authentication information and authentication was successful.
      * @throws ResponseAuthenticationException in case that basic credentials missing or are in invalid format
      */
-    public boolean filterResponseAndAuthenticate(ClientRequestContext request, ClientResponseContext response) {
+    public boolean filterResponseAndAuthenticate(ClientRequestContext request, ClientResponseContext response)
+            throws IOException {
         final String authenticate = response.getHeaders().getFirst(HttpHeaders.WWW_AUTHENTICATE);
         if (authenticate != null && authenticate.trim().toUpperCase().startsWith("BASIC")) {
             HttpAuthenticationFilter.Credentials credentials = HttpAuthenticationFilter

core-client/src/main/java/org/glassfish/jersey/client/authentication/HttpAuthenticationFilter.java

@@ -49,9 +49,9 @@
 import java.util.List;
 import java.util.Map;
 
+import javax.annotation.Priority;
 import javax.ws.rs.Priorities;
 import javax.ws.rs.client.Client;
-import javax.ws.rs.client.ClientBuilder;
 import javax.ws.rs.client.ClientRequestContext;
 import javax.ws.rs.client.ClientRequestFilter;
 import javax.ws.rs.client.ClientResponseContext;
@@ -66,8 +66,6 @@
 import javax.ws.rs.core.MultivaluedMap;
 import javax.ws.rs.core.Response;
 
-import javax.annotation.Priority;
-
 import org.glassfish.jersey.client.ClientProperties;
 import org.glassfish.jersey.client.internal.LocalizationMessages;
 
@@ -222,15 +220,21 @@ public void filter(ClientRequestContext request, ClientResponseContext response)
         Type result = null; // which authentication is requested: BASIC or DIGEST
         boolean authenticate;
 
+        // If the server requests both BASIC and DIGEST, prefer DIGEST since it's stronger
+        // (see https://tools.ietf.org/html/rfc2617#section-4.6)
         if (response.getStatus() == Response.Status.UNAUTHORIZED.getStatusCode()) {
-            String authString = response.getHeaders().getFirst(HttpHeaders.WWW_AUTHENTICATE);
-            if (authString != null) {
-                final String upperCaseAuth = authString.trim().toUpperCase();
-                if (upperCaseAuth.startsWith("BASIC")) {
-                    result = Type.BASIC;
-                } else if (upperCaseAuth.startsWith("DIGEST")) {
-                    result = Type.DIGEST;
-                } else {
+            List<String> authStrings = response.getHeaders().get(HttpHeaders.WWW_AUTHENTICATE);
+            if (authStrings != null) {
+                for (String authString : authStrings) {
+                    final String upperCaseAuth = authString.trim().toUpperCase();
+                    if (result == null && upperCaseAuth.startsWith("BASIC")) {
+                        result = Type.BASIC;
+                    } else if (upperCaseAuth.startsWith("DIGEST")) {
+                        result = Type.DIGEST;
+                    }
+                }
+                 
+                if (result == null) {
                     // unknown authentication -> this filter cannot authenticate with this method
                     return;
                 }
@@ -292,10 +296,20 @@ private void updateCache(ClientRequestContext request, boolean success, Type ope
      * @param newAuthorizationHeader {@code Authorization} header that should be added to the new request.
      * @return {@code true} is the authentication was successful ({@code true} if 401 response code was not returned;
      * {@code false} otherwise).
+     * @throws IOException 
      */
-    static boolean repeatRequest(ClientRequestContext request, ClientResponseContext response, String newAuthorizationHeader) {
+    static boolean repeatRequest(ClientRequestContext request, ClientResponseContext response, String newAuthorizationHeader)
+            throws IOException {
+        // If the failed response has an entity stream, close it. We must do this to avoid leaking a connection
+        // when we replace the entity stream of the failed response with that of the repeated response (see below).
+        // Notice that by closing the entity stream before sending the repeated request we allow the connection allocated
+        // to the failed request to be reused, if possible, for the repeated request.
+        if (response.hasEntity()) {
+            response.getEntityStream().close();
+            response.setEntityStream(null);
+        }
+      
         Client client = request.getClient();
-
         String method = request.getMethod();
         MediaType mediaType = request.getMediaType();
         URI lUri = request.getUri();
@@ -318,6 +332,12 @@ static boolean repeatRequest(ClientRequestContext request, ClientResponseContext
 
         builder.property(REQUEST_PROPERTY_FILTER_REUSED, "true");
 
+        // Copy other properties, if any, from the original request
+        for (String propertyName : request.getPropertyNames()) {
+            Object propertyValue = request.getProperty(propertyName);
+            builder.property(propertyName, propertyValue);
+        }
+
         Invocation invocation;
         if (request.getEntity() == null) {
             invocation = builder.build(method);