KEYCLOAK-16828 Fix HttpClient failures and close HttpResponses

server-spi-private/src/main/java/org/keycloak/connections/httpclient/HttpClientProvider.java

@@ -17,17 +17,27 @@
 
 package org.keycloak.connections.httpclient;
 
-import org.apache.http.client.HttpClient;
 import org.keycloak.provider.Provider;
 
 import java.io.IOException;
 import java.io.InputStream;
+import org.apache.http.impl.client.CloseableHttpClient;
 
 /**
  * @author <a href="mailto:sthorger@redhat.com">Stian Thorgersen</a>
  */
 public interface HttpClientProvider extends Provider {
-    HttpClient getHttpClient();
+    /**
+     * Returns the {@code CloseableHttpClient} that can be freely used.
+     * <p>
+     * <b>The returned {@code HttpClient} instance must never be {@code close()}d by the caller.</b>
+     * <p>
+     * Closing the {@code HttpClient} instance is responsibility of this provider. However,
+     * the objects created via the returned {@code HttpClient} need to be closed properly
+     * by the code that instantiated them.
+     * @return 
+     */
+    CloseableHttpClient getHttpClient();
 
     /**
      * Helper method

services/src/main/java/org/keycloak/authentication/authenticators/x509/CertificateValidator.java

@@ -19,7 +19,6 @@
 package org.keycloak.authentication.authenticators.x509;
 
 import org.apache.http.HttpResponse;
-import org.apache.http.client.HttpClient;
 import org.apache.http.client.methods.HttpGet;
 
 import org.keycloak.common.util.Time;
@@ -47,7 +46,6 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.net.URI;
 import java.net.URISyntaxException;
-import java.net.URLConnection;
 import java.security.GeneralSecurityException;
 import java.security.cert.CRLException;
 import java.security.cert.CertPathValidatorException;
@@ -65,6 +63,9 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.util.EntityUtils;
 
 /**
  * @author <a href="mailto:pnalyvayko@agi.com">Peter Nalyvayko</a>
@@ -297,17 +298,18 @@ public class CertificateValidator {
             try {
                 logger.debugf("Loading CRL from %s", remoteURI.toString());
 
-                HttpClient httpClient = session.getProvider(HttpClientProvider.class).getHttpClient();
+                CloseableHttpClient httpClient = session.getProvider(HttpClientProvider.class).getHttpClient();
                 HttpGet get = new HttpGet(remoteURI);
                 get.setHeader("Pragma", "no-cache");
                 get.setHeader("Cache-Control", "no-cache, no-store");
-                HttpResponse response = httpClient.execute(get);
+                try (CloseableHttpResponse response = httpClient.execute(get)) {
-                InputStream content = response.getEntity().getContent();
+                    try {
-                try {
+                        InputStream content = response.getEntity().getContent();
-                    X509CRL crl = loadFromStream(cf, content);
+                        X509CRL crl = loadFromStream(cf, content);
-                    return Collections.singleton(crl);
+                        return Collections.singleton(crl);
-                } finally {
+                    } finally {
-                    content.close();
+                        EntityUtils.consumeQuietly(response.getEntity());
+                    }
                 }
             }
             catch(IOException ex) {

services/src/main/java/org/keycloak/authentication/authenticators/x509/OCSPUtils.java

@@ -19,8 +19,6 @@
 package org.keycloak.authentication.authenticators.x509;
 
 import org.apache.http.HttpHeaders;
-import org.apache.http.HttpResponse;
-import org.apache.http.client.HttpClient;
 import org.apache.http.client.config.RequestConfig;
 import org.apache.http.client.methods.HttpPost;
 import org.apache.http.entity.ByteArrayEntity;
@@ -69,6 +67,8 @@ import java.security.cert.*;
 import java.util.*;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.impl.client.CloseableHttpClient;
 
 /**
  * @author <a href="mailto:brat000012001@gmail.com">Peter Nalyvayko</a>
@@ -163,7 +163,7 @@ public final class OCSPUtils {
     }
 
     private static OCSPResp getResponse(KeycloakSession session, OCSPReq ocspReq, URI responderUri) throws IOException {
-        HttpClient httpClient = session.getProvider(HttpClientProvider.class).getHttpClient();
+        CloseableHttpClient httpClient = session.getProvider(HttpClientProvider.class).getHttpClient();
         HttpPost post = new HttpPost(responderUri);
         post.setHeader(HttpHeaders.CONTENT_TYPE, "application/ocsp-request");
 
@@ -176,16 +176,20 @@ public final class OCSPUtils {
         post.setEntity(new ByteArrayEntity(ocspReq.getEncoded()));
 
         //Get Response
-        HttpResponse response = httpClient.execute(post);
+        try (CloseableHttpResponse response = httpClient.execute(post)) {
+            try {
+                if (response.getStatusLine().getStatusCode() / 100 != 2) {
+                    String errorMessage = String.format("Connection error, unable to obtain certificate revocation status using OCSP responder \"%s\", code \"%d\"",
+                        responderUri.toString(), response.getStatusLine().getStatusCode());
+                    throw new IOException(errorMessage);
+                }
 
-        if (response.getStatusLine().getStatusCode() / 100 != 2) {
+                byte[] data = EntityUtils.toByteArray(response.getEntity());
-            String errorMessage = String.format("Connection error, unable to obtain certificate revocation status using OCSP responder \"%s\", code \"%d\"",
+                return new OCSPResp(data);
-                responderUri.toString(), response.getStatusLine().getStatusCode());
+            } finally {
-            throw new IOException(errorMessage);
+                EntityUtils.consumeQuietly(response.getEntity());
+            }
         }
-
-        byte[] data = EntityUtils.toByteArray(response.getEntity());
-        return new OCSPResp(data);
     }
 
     /**

services/src/main/java/org/keycloak/authentication/forms/RegistrationRecaptcha.java

@@ -17,9 +17,7 @@
 
 package org.keycloak.authentication.forms;
 
-import org.apache.http.HttpResponse;
 import org.apache.http.NameValuePair;
-import org.apache.http.client.HttpClient;
 import org.apache.http.client.entity.UrlEncodedFormEntity;
 import org.apache.http.client.methods.HttpPost;
 import org.apache.http.message.BasicNameValuePair;
@@ -47,13 +45,16 @@ import org.keycloak.services.messages.Messages;
 import org.keycloak.services.validation.Validation;
 import org.keycloak.util.JsonSerialization;
 
-import javax.ws.rs.core.MultivaluedMap;
 import java.io.InputStream;
+import javax.ws.rs.core.MultivaluedMap;
 import java.util.ArrayList;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.util.EntityUtils;
 
 /**
  * @author <a href="mailto:bill@burkecentral.com">Bill Burke</a>
@@ -150,7 +151,7 @@ public class RegistrationRecaptcha implements FormAction, FormActionFactory, Con
     }
 
     protected boolean validateRecaptcha(ValidationContext context, boolean success, String captcha, String secret) {
-        HttpClient httpClient = context.getSession().getProvider(HttpClientProvider.class).getHttpClient();
+        CloseableHttpClient httpClient = context.getSession().getProvider(HttpClientProvider.class).getHttpClient();
         HttpPost post = new HttpPost("https://www." + getRecaptchaDomain(context.getAuthenticatorConfig()) + "/recaptcha/api/siteverify");
         List<NameValuePair> formparams = new LinkedList<>();
         formparams.add(new BasicNameValuePair("secret", secret));
@@ -159,14 +160,15 @@ public class RegistrationRecaptcha implements FormAction, FormActionFactory, Con
         try {
             UrlEncodedFormEntity form = new UrlEncodedFormEntity(formparams, "UTF-8");
             post.setEntity(form);
-            HttpResponse response = httpClient.execute(post);
+            try (CloseableHttpResponse response = httpClient.execute(post)) {
-            InputStream content = response.getEntity().getContent();
+                InputStream content = response.getEntity().getContent();
-            try {
+                try {
-                Map json = JsonSerialization.readValue(content, Map.class);
+                    Map json = JsonSerialization.readValue(content, Map.class);
-                Object val = json.get("success");
+                    Object val = json.get("success");
-                success = Boolean.TRUE.equals(val);
+                    success = Boolean.TRUE.equals(val);
-            } finally {
+                } finally {
-                content.close();
+                    EntityUtils.consumeQuietly(response.getEntity());
+                }
             }
         } catch (Exception e) {
             ServicesLogger.LOGGER.recaptchaFailed(e);

services/src/main/java/org/keycloak/connections/httpclient/DefaultHttpClientFactory.java

@@ -19,7 +19,6 @@ package org.keycloak.connections.httpclient;
 
 import org.apache.http.HttpEntity;
 import org.apache.http.HttpResponse;
-import org.apache.http.client.HttpClient;
 import org.apache.http.client.entity.EntityBuilder;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.methods.HttpPost;
@@ -37,6 +36,8 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.security.KeyStore;
 import java.util.concurrent.TimeUnit;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.util.EntityUtils;
 
 /**
  * The default {@link HttpClientFactory} for {@link HttpClientProvider HttpClientProvider's} used by Keycloak for outbound HTTP calls.
@@ -70,7 +71,7 @@ public class DefaultHttpClientFactory implements HttpClientFactory {
 
         return new HttpClientProvider() {
             @Override
-            public HttpClient getHttpClient() {
+            public CloseableHttpClient getHttpClient() {
                 return httpClient;
             }
 
@@ -83,16 +84,15 @@ public class DefaultHttpClientFactory implements HttpClientFactory {
             public int postText(String uri, String text) throws IOException {
                 HttpPost request = new HttpPost(uri);
                 request.setEntity(EntityBuilder.create().setText(text).setContentType(ContentType.TEXT_PLAIN).build());
-                HttpResponse response = httpClient.execute(request);
+                try (CloseableHttpResponse response = httpClient.execute(request)) {
-                try {
+                    try {
-                    return response.getStatusLine().getStatusCode();
+                        return response.getStatusLine().getStatusCode();
-                } finally {
+                    } finally {
-                    HttpEntity entity = response.getEntity();
+                        EntityUtils.consumeQuietly(response.getEntity());
-                    if (entity != null) {
-                        InputStream is = entity.getContent();
-                        if (is != null) is.close();
                     }
-
+                } catch (Throwable t) {
+                    logger.warn(t.getMessage(), t);
+                    throw t;
                 }
             }
 

services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java

@@ -18,9 +18,7 @@
 package org.keycloak.protocol.saml;
 
 import org.apache.http.HttpEntity;
-import org.apache.http.HttpResponse;
 import org.apache.http.NameValuePair;
-import org.apache.http.client.HttpClient;
 import org.apache.http.client.entity.UrlEncodedFormEntity;
 import org.apache.http.client.methods.HttpPost;
 import org.apache.http.message.BasicNameValuePair;
@@ -89,6 +87,9 @@ import java.util.Map;
 import java.util.Objects;
 import java.util.UUID;
 import java.util.concurrent.atomic.AtomicReference;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.util.EntityUtils;
 
 /**
  * @author <a href="mailto:bill@burkecentral.com">Bill Burke</a>
@@ -694,7 +695,7 @@ public class SamlProtocol implements LoginProtocol {
             return Response.serverError().build();
         }
 
-        HttpClient httpClient = session.getProvider(HttpClientProvider.class).getHttpClient();
+        CloseableHttpClient httpClient = session.getProvider(HttpClientProvider.class).getHttpClient();
         for (int i = 0; i < 2; i++) { // follow redirects once
             try {
                 List<NameValuePair> formparams = new ArrayList<NameValuePair>();
@@ -705,25 +706,20 @@ public class SamlProtocol implements LoginProtocol {
                 UrlEncodedFormEntity form = new UrlEncodedFormEntity(formparams, "UTF-8");
                 HttpPost post = new HttpPost(logoutUrl);
                 post.setEntity(form);
-                HttpResponse response = httpClient.execute(post);
+                try (CloseableHttpResponse response = httpClient.execute(post)) {
-                try {
+                    try {
-                    int status = response.getStatusLine().getStatusCode();
+                        int status = response.getStatusLine().getStatusCode();
-                    if (status == 302 && !logoutUrl.endsWith("/")) {
+                        if (status == 302 && !logoutUrl.endsWith("/")) {
-                        String redirect = response.getFirstHeader(HttpHeaders.LOCATION).getValue();
+                            String redirect = response.getFirstHeader(HttpHeaders.LOCATION).getValue();
-                        String withSlash = logoutUrl + "/";
+                            String withSlash = logoutUrl + "/";
-                        if (withSlash.equals(redirect)) {
+                            if (withSlash.equals(redirect)) {
-                            logoutUrl = withSlash;
+                                logoutUrl = withSlash;
-                            continue;
+                                continue;
+                            }
                         }
+                    } finally {
+                        EntityUtils.consumeQuietly(response.getEntity());
                     }
-                } finally {
-                    HttpEntity entity = response.getEntity();
-                    if (entity != null) {
-                        InputStream is = entity.getContent();
-                        if (is != null)
-                            is.close();
-                    }
-
                 }
             } catch (IOException e) {
                 logger.warn("failed to send saml logout", e);

services/src/main/java/org/keycloak/services/managers/ResourceAdminManager.java

@@ -16,9 +16,7 @@
  */
 package org.keycloak.services.managers;
 
-import org.apache.http.HttpResponse;
 import org.apache.http.NameValuePair;
-import org.apache.http.client.HttpClient;
 import org.apache.http.client.entity.UrlEncodedFormEntity;
 import org.apache.http.client.methods.HttpPost;
 import org.apache.http.message.BasicNameValuePair;
@@ -60,6 +58,8 @@ import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.stream.Stream;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.impl.client.CloseableHttpClient;
 
 /**
  * @author <a href="mailto:bill@burkecentral.com">Bill Burke</a>
@@ -212,22 +212,27 @@ public class ResourceAdminManager {
             if (logoutToken != null) {
                 parameters.add(new BasicNameValuePair(OAuth2Constants.LOGOUT_TOKEN, token));
             }
-            HttpClient httpClient = session.getProvider(HttpClientProvider.class).getHttpClient();
+            CloseableHttpClient httpClient = session.getProvider(HttpClientProvider.class).getHttpClient();
             UrlEncodedFormEntity formEntity;
             formEntity = new UrlEncodedFormEntity(parameters, "UTF-8");
             post.setEntity(formEntity);
-            HttpResponse response = httpClient.execute(post);
+            try (CloseableHttpResponse response = httpClient.execute(post)) {
-            int status = response.getStatusLine().getStatusCode();
+                try {
-            EntityUtils.consumeQuietly(response.getEntity());
+                    int status = response.getStatusLine().getStatusCode();
-            boolean success = status == 204 || status == 200;
+                    EntityUtils.consumeQuietly(response.getEntity());
-            logger.debugf("logout success for %s: %s", managementUrl, success);
+                    boolean success = status == 204 || status == 200;
-            return Response.status(status).build();
+                    logger.debugf("logout success for %s: %s", managementUrl, success);
+                    return Response.status(status).build();
+                } finally {
+                    EntityUtils.consumeQuietly(response.getEntity());
+                }
+            }
         } catch (IOException e) {
             ServicesLogger.LOGGER.logoutFailed(e, resource.getClientId());
             return Response.serverError().build();
         } finally {
             if (post != null) {
-                post.releaseConnection();
+                post.reset();
             }
         }
     }

services/src/test/java/org/keycloak/connections/httpclient/DefaultHttpClientFactoryTest.java

@@ -58,13 +58,12 @@ public class DefaultHttpClientFactoryTest {
 		factory.init(scope(values));
 		KeycloakSession session = new DefaultKeycloakSession(new DefaultKeycloakSessionFactory());
 		HttpClientProvider provider = factory.create(session);
-		CloseableHttpResponse response;
+        Optional<String> testURL = getTestURL();
-		try(CloseableHttpClient httpClient = (CloseableHttpClient) provider.getHttpClient()){
+        Assume.assumeTrue( "Could not get test url for domain", testURL.isPresent() );
-			Optional<String> testURL = getTestURL();
+		try (CloseableHttpClient httpClient = (CloseableHttpClient) provider.getHttpClient();
-			Assume.assumeTrue( "Could not get test url for domain", testURL.isPresent() );
+          CloseableHttpResponse response = httpClient.execute(new HttpGet(testURL.get()))) {
-			response = httpClient.execute(new HttpGet(testURL.get()));
+    		assertEquals(HttpStatus.SC_NOT_FOUND,response.getStatusLine().getStatusCode());
 		}
-		assertEquals(HttpStatus.SC_NOT_FOUND,response.getStatusLine().getStatusCode());
 	}
 
 	@Test(expected = SSLPeerUnverifiedException.class)

testsuite/integration-arquillian/tests/base/src/test/resources/META-INF/keycloak-server.json

@@ -124,7 +124,7 @@
 
     "connectionsHttpClient": {
         "default": {
-            "max-connection-idle-time-millis": 1000
+            "max-connection-idle-time-millis": 100
         }
     },