Issue #4138 - use HttpClient for OpenID Authentication

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

jetty-openid/pom.xml

@@ -33,6 +33,11 @@
       <artifactId>jetty-server</artifactId>
       <version>${project.version}</version>
     </dependency>
+    <dependency>
+      <groupId>org.eclipse.jetty</groupId>
+      <artifactId>jetty-client</artifactId>
+      <version>${project.version}</version>
+    </dependency>
     <dependency>
       <groupId>org.eclipse.jetty</groupId>
       <artifactId>jetty-security</artifactId>
@@ -54,10 +59,5 @@
       <artifactId>jetty-test-helper</artifactId>
       <scope>test</scope>
     </dependency>
-    <dependency>
-      <groupId>org.eclipse.jetty</groupId>
-      <artifactId>jetty-client</artifactId>
-      <version>${project.version}</version>
-    </dependency>
   </dependencies>
 </project>

jetty-openid/src/main/config/etc/jetty-openid.xml

@@ -1,8 +1,16 @@
 <?xml version="1.0"?>
 <!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "http://www.eclipse.org/jetty/configure_9_3.dtd">
 <Configure id="Server" class="org.eclipse.jetty.server.Server">
-  <New id="HttpClientFactory" class="org.eclipse.jetty.security.openid.OpenIdHttpClientFactory">
+  <Get id="ThreadPool" name="threadPool"/>
-    <Arg type="Boolean"><Property name="jetty.openid.trustAllCertificates" default="false"/></Arg>
+  <New id="HttpClient" class="org.eclipse.jetty.client.HttpClient">
+    <Arg>
+      <New class="org.eclipse.jetty.util.ssl.SslContextFactory$Client">
+        <Set name="trustAll" type="boolean">false</Set>
+        <Set name="endpointIdentificationAlgorithm">https</Set>
+      </New>
+    </Arg>
+    <Set name="followRedirects" type="boolean">false</Set>
+    <Set name="executor"><Ref refid="ThreadPool"/></Set>
   </New>
   <New id="OpenIdConfiguration" class="org.eclipse.jetty.security.openid.OpenIdConfiguration">
     <Arg><Property name="jetty.openid.provider" deprecated="jetty.openid.openIdProvider"/></Arg>
@@ -10,7 +18,7 @@
     <Arg><Property name="jetty.openid.provider.tokenEndpoint"/></Arg>
     <Arg><Property name="jetty.openid.clientId"/></Arg>
     <Arg><Property name="jetty.openid.clientSecret"/></Arg>
-    <Arg><Ref refid="HttpClientFactory"/></Arg>
+    <Arg><Ref refid="HttpClient"/></Arg>
     <Call name="addScopes">
       <Arg>
         <Call class="org.eclipse.jetty.util.StringUtil" name="csvSplit">
@@ -24,7 +32,6 @@
       <New class="org.eclipse.jetty.security.openid.OpenIdLoginService">
         <Arg><Ref refid="OpenIdConfiguration"/></Arg>
         <Arg><Ref refid="BaseLoginService"/></Arg>
-        <Arg><Ref refid="HttpClientFactory"/></Arg>
         <Call name="setAuthenticateNewUsers">
           <Arg type="boolean">
             <Property name="jetty.openid.authenticateNewUsers" default="false"/>

jetty-openid/src/main/config/modules/openid.mod

@@ -22,9 +22,6 @@ etc/jetty-openid.xml
 ## The OpenID Identity Provider's issuer ID (the entire URL *before* ".well-known/openid-configuration")
 # jetty.openid.provider=https://id.example.com/~
 
-## Whether or not all certificates of the OpenID Connect provider's should be trusted. Only set to true during testing.
-# jetty.openid.trustAllCertificates=false
-
 ## The OpenID Identity Provider's authorization endpoint (optional if the metadata of the OP is accessible)
 # jetty.openid.provider.authorizationEndpoint=https://id.example.com/authorization
 

jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdConfiguration.java

@@ -28,8 +28,10 @@ import java.util.stream.Collectors;
 
 import org.eclipse.jetty.client.HttpClient;
 import org.eclipse.jetty.util.ajax.JSON;
+import org.eclipse.jetty.util.component.ContainerLifeCycle;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
 
 /**
  * Holds the configuration for an OpenID Connect service.
@@ -37,18 +39,19 @@ import org.eclipse.jetty.util.log.Logger;
  * This uses the OpenID Provider URL with the path {@link #CONFIG_PATH} to discover
  * the required information about the OIDC service.
  */
-public class OpenIdConfiguration implements Serializable
+public class OpenIdConfiguration extends ContainerLifeCycle implements Serializable
 {
     private static final Logger LOG = Log.getLogger(OpenIdConfiguration.class);
     private static final long serialVersionUID = 2227941990601349102L;
     private static final String CONFIG_PATH = "/.well-known/openid-configuration";
 
+    private final HttpClient httpClient;
     private final String issuer;
-    private final String authEndpoint;
-    private final String tokenEndpoint;
     private final String clientId;
     private final String clientSecret;
     private final List<String> scopes = new ArrayList<>();
+    private String authEndpoint;
+    private String tokenEndpoint;
 
     /**
      * Create an OpenID configuration for a specific OIDC provider.
@@ -58,7 +61,7 @@ public class OpenIdConfiguration implements Serializable
      */
     public OpenIdConfiguration(String provider, String clientId, String clientSecret)
     {
-        this(provider, null, null, clientId, clientSecret, null);
+        this(provider, null, null, clientId, clientSecret, newHttpClient());
     }
 
     /**
@@ -68,39 +71,53 @@ public class OpenIdConfiguration implements Serializable
      * @param tokenEndpoint the URL of the OpenID provider's token endpoint if configured.
      * @param clientId OAuth 2.0 Client Identifier valid at the Authorization Server.
      * @param clientSecret The client secret known only by the Client and the Authorization Server.
-     * @param factory A factory that can create the {@link HttpClient} which will discover the provider's metadata.
+     * @param httpClient The {@link HttpClient} instance to use.
      */
     public OpenIdConfiguration(String issuer, String authorizationEndpoint, String tokenEndpoint,
-                               String clientId, String clientSecret, OpenIdHttpClientFactory factory)
+                               String clientId, String clientSecret, HttpClient httpClient)
     {
         this.issuer = issuer;
         this.clientId = clientId;
         this.clientSecret = clientSecret;
+        this.authEndpoint = authorizationEndpoint;
+        this.tokenEndpoint = tokenEndpoint;
+        this.httpClient = httpClient;
 
-        if (issuer == null)
+        if (this.issuer == null)
-            throw new IllegalArgumentException("Provider was not configured");
+            throw new IllegalArgumentException("Issuer was not configured");
 
-        if (tokenEndpoint == null || authorizationEndpoint == null)
+        addBean(this.httpClient);
+    }
+
+    @Override
+    protected void doStart() throws Exception
     {
-            Map<String, Object> discoveryDocument = fetchOpenIdConnectMetadata(issuer, factory == null ?
+        super.doStart();
-                new HttpClient() : factory.createHttpClient());
 
-            this.authEndpoint = (String)discoveryDocument.get("authorization_endpoint");
+        if (authEndpoint == null || tokenEndpoint == null)
-            if (this.authEndpoint == null)
+        {
+            Map<String, Object> discoveryDocument = fetchOpenIdConnectMetadata(issuer, httpClient);
+
+            authEndpoint = (String)discoveryDocument.get("authorization_endpoint");
+            if (authEndpoint == null)
                 throw new IllegalArgumentException("authorization_endpoint");
 
-            this.tokenEndpoint = (String)discoveryDocument.get("token_endpoint");
+            tokenEndpoint = (String)discoveryDocument.get("token_endpoint");
-            if (this.tokenEndpoint == null)
+            if (tokenEndpoint == null)
                 throw new IllegalArgumentException("token_endpoint");
 
             if (!Objects.equals(discoveryDocument.get("issuer"), issuer))
-                LOG.warn("The provider in the metadata is not correct.");
+                throw new IllegalArgumentException("The provider in the metadata is not correct.");
         }
-        else
+    }
+
+    private static HttpClient newHttpClient()
     {
-            this.authEndpoint = authorizationEndpoint;
+        SslContextFactory.Client sslContextFactory = new SslContextFactory.Client(false);
-            this.tokenEndpoint = tokenEndpoint;
+        sslContextFactory.setEndpointIdentificationAlgorithm("https");
-        }
+        HttpClient client = new HttpClient(sslContextFactory);
+        client.setFollowRedirects(false);
+        return client;
     }
 
     private static Map<String, Object> fetchOpenIdConnectMetadata(String provider, HttpClient httpClient)
@@ -118,14 +135,12 @@ public class OpenIdConfiguration implements Serializable
             if (parsedResult instanceof Map)
             {
                 Map<?, ?> rawResult = (Map)parsedResult;
-
                 result = rawResult.entrySet().stream()
                         .collect(Collectors.toMap(it -> it.getKey().toString(), Map.Entry::getValue));
             }
             else
             {
                 LOG.warn("OpenID provider did not return a proper JSON object response. Result was '{}'", responseBody);
-
                 throw new IllegalStateException("Could not parse OpenID provider's malformed response");
             }
 
@@ -139,6 +154,11 @@ public class OpenIdConfiguration implements Serializable
         }
     }
 
+    public HttpClient getHttpClient()
+    {
+        return httpClient;
+    }
+
     public String getAuthEndpoint()
     {
         return authEndpoint;

jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdCredentials.java

@@ -18,29 +18,19 @@
 
 package org.eclipse.jetty.security.openid;
 
-import java.io.DataOutputStream;
 import java.io.IOException;
-import java.io.InputStream;
 import java.io.Serializable;
-import java.net.HttpURLConnection;
-import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.Base64;
-import java.util.Collections;
 import java.util.Map;
-import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.TimeoutException;
-import java.util.stream.Collectors;
 
 import org.eclipse.jetty.client.HttpClient;
 import org.eclipse.jetty.client.api.ContentResponse;
 import org.eclipse.jetty.client.api.Request;
 import org.eclipse.jetty.client.util.FormContentProvider;
 import org.eclipse.jetty.util.Fields;
-import org.eclipse.jetty.util.IO;
-import org.eclipse.jetty.util.UrlEncoded;
 import org.eclipse.jetty.util.ajax.JSON;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
@@ -51,7 +41,7 @@ import org.eclipse.jetty.util.log.Logger;
  *
  * <p>
  * This is constructed with an authorization code from the authentication request. This authorization code
- * is then exchanged using {@link #claimAuthCode} for a response containing the ID Token and Access Token.
+ * is then exchanged using {@link #redeemAuthCode(HttpClient)} for a response containing the ID Token and Access Token.
  * The response is then validated against the {@link OpenIdConfiguration}.
  * </p>
  */
@@ -88,7 +78,7 @@ public class OpenIdCredentials implements Serializable
         return response;
     }
 
-    public void redeemAuthCode(HttpClient httpClient) throws IOException
+    public void redeemAuthCode(HttpClient httpClient) throws Throwable
     {
         if (LOG.isDebugEnabled())
             LOG.debug("redeemAuthCode() {}", this);
@@ -129,14 +119,8 @@ public class OpenIdCredentials implements Serializable
     private void validateClaims()
     {
         // Issuer Identifier for the OpenID Provider MUST exactly match the value of the iss (issuer) Claim.
-        Object assertedIssuer = claims.get("iss");
+        if (!configuration.getIssuer().equals(claims.get("iss")))
-        String configuredIssuer = configuration.getIssuer();
-
-        if (!configuredIssuer.equals(assertedIssuer))
-        {
-            LOG.warn("Issuers don't match. Configured = {}, asserted = {}", configuredIssuer, assertedIssuer);
             throw new IllegalArgumentException("Issuer Identifier MUST exactly match the iss Claim");
-        }
 
         // The aud (audience) Claim MUST contain the client_id value.
         validateAudience();
@@ -239,11 +223,7 @@ public class OpenIdCredentials implements Serializable
         return paddedEncodedJwtSection;
     }
 
-    private Map<String, Object> claimAuthCode(HttpClient httpClient, String authCode)
+    private Map<String, Object> claimAuthCode(HttpClient httpClient, String authCode) throws Throwable
-    {
-        Map<String, Object> result;
-
-        try
     {
         Fields fields = new Fields();
         fields.add("code", authCode);
@@ -257,39 +237,9 @@ public class OpenIdCredentials implements Serializable
                 .timeout(10, TimeUnit.SECONDS);
         ContentResponse response = request.send();
         String responseBody = response.getContentAsString();
-            Object parsedResult = JSON.parse(responseBody);
+        if (LOG.isDebugEnabled())
+            LOG.debug("Authentication response: {}", responseBody);
 
-            if (parsedResult instanceof Map)
+        return (Map)JSON.parse(responseBody);
-            {
-                Map<?, ?> rawResult = (Map)parsedResult;
-
-                result = rawResult.entrySet().stream().collect(Collectors.toMap(it -> it.getKey().toString(), Map.Entry::getValue));
-
-                LOG.debug("Got result from token server: {}", result);
-            }
-            else
-            {
-                LOG.warn("OpenID provider did not return a proper JSON object response. Result was '{}'", responseBody);
-
-                throw new IllegalStateException("Could not parse OpenID provider's malformed response");
-            }
-        }
-        catch (InterruptedException e)
-        {
-            LOG.debug("Call to token endpoint was interrupted", e);
-            result = Collections.emptyMap();
-        }
-        catch (ExecutionException e)
-        {
-            LOG.warn("Call to token endpoint was failed", e);
-            result = Collections.emptyMap();
-        }
-        catch (TimeoutException e)
-        {
-            LOG.warn("Call to token endpoint was did not complete in a timely manner", e);
-            result = Collections.emptyMap();
-        }
-
-        return result;
     }
 }

jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdHttpClientFactory.java

@@ -1,62 +0,0 @@
-//
-//  ========================================================================
-//  Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd.
-//  ------------------------------------------------------------------------
-//  All rights reserved. This program and the accompanying materials
-//  are made available under the terms of the Eclipse Public License v1.0
-//  and Apache License v2.0 which accompanies this distribution.
-//
-//      The Eclipse Public License is available at
-//      http://www.eclipse.org/legal/epl-v10.html
-//
-//      The Apache License v2.0 is available at
-//      http://www.opensource.org/licenses/apache2.0.php
-//
-//  You may elect to redistribute this code under either of these licenses.
-//  ========================================================================
-//
-
-package org.eclipse.jetty.security.openid;
-
-import org.eclipse.jetty.client.HttpClient;
-import org.eclipse.jetty.client.HttpClientTransport;
-import org.eclipse.jetty.client.http.HttpClientTransportOverHTTP;
-import org.eclipse.jetty.util.ssl.SslContextFactory;
-
-public final class OpenIdHttpClientFactory
-{
-    private final boolean trustAll;
-
-    public OpenIdHttpClientFactory()
-    {
-        this(false);
-    }
-
-    public OpenIdHttpClientFactory(boolean trustAll)
-    {
-        this.trustAll = trustAll;
-    }
-
-    public HttpClient createHttpClient()
-    {
-        SslContextFactory.Client sslContextFactory = new SslContextFactory.Client(trustAll);
-
-        sslContextFactory.setEndpointIdentificationAlgorithm("https");
-
-        HttpClientTransport transport = new HttpClientTransportOverHTTP();
-        HttpClient client = new HttpClient(transport, sslContextFactory);
-
-        client.setFollowRedirects(false);
-
-        try
-        {
-            client.start();
-        }
-        catch (Exception e)
-        {
-            throw new RuntimeException(e);
-        }
-
-        return client;
-    }
-}

jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdLoginService.java

@@ -41,15 +41,15 @@ public class OpenIdLoginService extends ContainerLifeCycle implements LoginServi
 {
     private static final Logger LOG = Log.getLogger(OpenIdLoginService.class);
 
-    private final OpenIdConfiguration _configuration;
+    private final OpenIdConfiguration configuration;
     private final LoginService loginService;
     private final HttpClient httpClient;
     private IdentityService identityService;
     private boolean authenticateNewUsers;
 
-    public OpenIdLoginService(OpenIdConfiguration configuration, OpenIdHttpClientFactory factory)
+    public OpenIdLoginService(OpenIdConfiguration configuration)
     {
-        this(configuration, null, factory);
+        this(configuration, null);
     }
 
     /**
@@ -59,23 +59,24 @@ public class OpenIdLoginService extends ContainerLifeCycle implements LoginServi
      * @param configuration the OpenID configuration to use.
      * @param loginService the wrapped LoginService to defer to for user roles.
      */
-    public OpenIdLoginService(OpenIdConfiguration configuration, LoginService loginService, OpenIdHttpClientFactory factory)
+    public OpenIdLoginService(OpenIdConfiguration configuration, LoginService loginService)
     {
-        _configuration = configuration;
+        this.configuration = configuration;
         this.loginService = loginService;
-        this.httpClient = factory.createHttpClient();
+        this.httpClient = configuration.getHttpClient();
+        addBean(this.configuration);
         addBean(this.loginService);
     }
 
     @Override
     public String getName()
     {
-        return _configuration.getIssuer();
+        return configuration.getIssuer();
     }
 
     public OpenIdConfiguration getConfiguration()
     {
-        return _configuration;
+        return configuration;
     }
 
     @Override

jetty-openid/src/test/java/org/eclipse/jetty/security/openid/OpenIdAuthenticationTest.java

@@ -102,7 +102,7 @@ public class OpenIdAuthenticationTest
         OpenIdConfiguration configuration = new OpenIdConfiguration(openIdProvider.getProvider(), CLIENT_ID, CLIENT_SECRET);
 
         // Configure OpenIdLoginService optionally providing a base LoginService to provide user roles
-        OpenIdLoginService loginService = new OpenIdLoginService(configuration, new OpenIdHttpClientFactory());
+        OpenIdLoginService loginService = new OpenIdLoginService(configuration);//, hashLoginService);
         securityHandler.setLoginService(loginService);
 
         Authenticator authenticator = new OpenIdAuthenticator(configuration, "/error");