From d451f818cf8e7cf2f619a5832c7fe5e5ed968c6a Mon Sep 17 00:00:00 2001 From: Jaikiran Pai Date: Sun, 10 Mar 2024 13:48:14 +0000 Subject: [PATCH] 8326381: com.sun.net.httpserver.HttpsParameters and SSLStreams incorrectly handle needClientAuth and wantClientAuth Reviewed-by: djelinski --- .../sun/net/httpserver/HttpsParameters.java | 40 +- .../sun/net/httpserver/SSLStreams.java | 10 +- .../HttpsParametersClientAuthTest.java | 400 ++++++++++++++++++ 3 files changed, 446 insertions(+), 4 deletions(-) create mode 100644 test/jdk/com/sun/net/httpserver/HttpsParametersClientAuthTest.java diff --git a/src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsParameters.java b/src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsParameters.java index 5943b70615c..3a0a3d76680 100644 --- a/src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsParameters.java +++ b/src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsParameters.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2005, 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2005, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -90,9 +90,13 @@ public abstract class HttpsParameters { * Returns a copy of the array of ciphersuites or {@code null} if none * have been set. * + * @deprecated It is recommended that the SSL parameters be configured and + * read through the use of {@link #setSSLParameters(SSLParameters) SSLParameters}. + * * @return a copy of the array of ciphersuites or {@code null} if none have * been set */ + @Deprecated(since = "23") public String[] getCipherSuites() { return cipherSuites != null ? cipherSuites.clone() : null; } @@ -100,8 +104,13 @@ public abstract class HttpsParameters { /** * Sets the array of ciphersuites. * + * @deprecated It is recommended that the SSL parameters be configured and + * read through the use of {@link #setSSLParameters(SSLParameters) SSLParameters}. Use + * {@link SSLParameters#setCipherSuites(String[])} instead. + * * @param cipherSuites the array of ciphersuites (or {@code null}) */ + @Deprecated(since = "23") public void setCipherSuites(String[] cipherSuites) { this.cipherSuites = cipherSuites != null ? cipherSuites.clone() : null; } @@ -110,9 +119,13 @@ public abstract class HttpsParameters { * Returns a copy of the array of protocols or {@code null} if none have been * set. * + * @deprecated It is recommended that the SSL parameters be configured and + * read through the use of {@link #setSSLParameters(SSLParameters) SSLParameters}. + * * @return a copy of the array of protocols or {@code null} if none have been * set */ + @Deprecated(since = "23") public String[] getProtocols() { return protocols != null ? protocols.clone() : null; } @@ -120,8 +133,13 @@ public abstract class HttpsParameters { /** * Sets the array of protocols. * + * @deprecated It is recommended that the SSL parameters be configured and + * read through the use of {@link #setSSLParameters(SSLParameters) SSLParameters}. Use + * {@link SSLParameters#setProtocols(String[])} instead. + * * @param protocols the array of protocols (or {@code null}) */ + @Deprecated(since = "23") public void setProtocols(String[] protocols) { this.protocols = protocols != null ? protocols.clone() : null; } @@ -129,8 +147,12 @@ public abstract class HttpsParameters { /** * Returns whether client authentication should be requested. * + * @deprecated It is recommended that the SSL parameters be configured and + * read through the use of {@link #setSSLParameters(SSLParameters) SSLParameters}. + * * @return whether client authentication should be requested */ + @Deprecated(since = "23") public boolean getWantClientAuth() { return wantClientAuth; } @@ -139,17 +161,27 @@ public abstract class HttpsParameters { * Sets whether client authentication should be requested. Calling this * method clears the {@code needClientAuth} flag. * + * @deprecated It is recommended that the SSL parameters be configured and + * read through the use of {@link #setSSLParameters(SSLParameters) SSLParameters}. Use + * {@link SSLParameters#setWantClientAuth(boolean)} instead. + * * @param wantClientAuth whether client authentication should be requested */ + @Deprecated(since = "23") public void setWantClientAuth(boolean wantClientAuth) { this.wantClientAuth = wantClientAuth; + this.needClientAuth = false; } /** * Returns whether client authentication should be required. * + * @deprecated It is recommended that the SSL parameters be configured and + * read through the use of {@link #setSSLParameters(SSLParameters) SSLParameters}. + * * @return whether client authentication should be required */ + @Deprecated(since = "23") public boolean getNeedClientAuth() { return needClientAuth; } @@ -158,9 +190,15 @@ public abstract class HttpsParameters { * Sets whether client authentication should be required. Calling this method * clears the {@code wantClientAuth} flag. * + * @deprecated It is recommended that the SSL parameters be configured and + * read through the use of {@link #setSSLParameters(SSLParameters) SSLParameters}. Use + * {@link SSLParameters#setNeedClientAuth(boolean)} instead. + * * @param needClientAuth whether client authentication should be required */ + @Deprecated(since = "23") public void setNeedClientAuth(boolean needClientAuth) { this.needClientAuth = needClientAuth; + this.wantClientAuth = false; } } diff --git a/src/jdk.httpserver/share/classes/sun/net/httpserver/SSLStreams.java b/src/jdk.httpserver/share/classes/sun/net/httpserver/SSLStreams.java index b740189b3e7..5cc16befa96 100644 --- a/src/jdk.httpserver/share/classes/sun/net/httpserver/SSLStreams.java +++ b/src/jdk.httpserver/share/classes/sun/net/httpserver/SSLStreams.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2005, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2005, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -66,6 +66,7 @@ class SSLStreams { wrapper = new EngineWrapper (chan, engine); } + @SuppressWarnings("deprecation") private void configureEngine(HttpsConfigurator cfg, InetSocketAddress addr){ if (cfg != null) { Parameters params = new Parameters (cfg, addr); @@ -85,8 +86,11 @@ class SSLStreams { ); } catch (IllegalArgumentException e) { /* LOG */} } - engine.setNeedClientAuth (params.getNeedClientAuth()); - engine.setWantClientAuth (params.getWantClientAuth()); + if (params.getNeedClientAuth()) { + engine.setNeedClientAuth(true); + } else if (params.getWantClientAuth()) { + engine.setWantClientAuth(true); + } if (params.getProtocols() != null) { try { engine.setEnabledProtocols ( diff --git a/test/jdk/com/sun/net/httpserver/HttpsParametersClientAuthTest.java b/test/jdk/com/sun/net/httpserver/HttpsParametersClientAuthTest.java new file mode 100644 index 00000000000..ded0474071b --- /dev/null +++ b/test/jdk/com/sun/net/httpserver/HttpsParametersClientAuthTest.java @@ -0,0 +1,400 @@ +/* + * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +import java.io.FileInputStream; +import java.io.IOException; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.SocketException; +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.net.http.HttpResponse.BodyHandlers; +import java.security.AccessController; +import java.security.KeyStore; +import java.security.PrivilegedExceptionAction; +import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.atomic.AtomicInteger; + +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLHandshakeException; +import javax.net.ssl.SSLParameters; +import javax.net.ssl.TrustManagerFactory; + +import com.sun.net.httpserver.HttpExchange; +import com.sun.net.httpserver.HttpHandler; +import com.sun.net.httpserver.HttpsConfigurator; +import com.sun.net.httpserver.HttpsParameters; +import com.sun.net.httpserver.HttpsServer; +import jdk.test.lib.net.SimpleSSLContext; +import jdk.test.lib.net.URIBuilder; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import static java.net.http.HttpClient.Builder.NO_PROXY; +import static java.net.http.HttpClient.Version.HTTP_1_1; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +/* + * @test + * @bug 8326381 + * @summary verifies that the setNeedClientAuth() and setWantClientAuth() + * methods on HttpsParameters class work as expected + * @library /test/lib + * @build jdk.test.lib.net.SimpleSSLContext jdk.test.lib.net.URIBuilder + * @run junit HttpsParametersClientAuthTest + */ +public class HttpsParametersClientAuthTest { + + private static final AtomicInteger TID = new AtomicInteger(); + private static final ThreadFactory SRV_THREAD_FACTORY = (r) -> { + final Thread t = new Thread(r); + t.setDaemon(true); + t.setName("server-thread-" + TID.incrementAndGet()); + return t; + }; + + /** + * verifies default values of {@link HttpsParameters#setNeedClientAuth(boolean)} + * and {@link HttpsParameters#setWantClientAuth(boolean)} methods + */ + @Test + public void testDefaultClientAuth() throws Exception { + // test default values + HttpsParameters defaultParams = new Params(); + assertFalse(defaultParams.getNeedClientAuth(), + "needClientAuth was expected to be false but wasn't"); + assertFalse(defaultParams.getWantClientAuth(), + "wantClientAuth was expected to be false but wasn't"); + } + + /** + * sets {@link HttpsParameters#setNeedClientAuth(boolean)} and verifies + * that subsequent calls to {@link HttpsParameters#getNeedClientAuth()} returns + * the set value and {@link HttpsParameters#getWantClientAuth()} returns false + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testNeedClientAuth(final boolean initialWantClientAuth) throws Exception { + HttpsParameters needClientAuthParams = new Params(); + // first set wantClientAuth to an initial value to verify that it later gets reset + needClientAuthParams.setWantClientAuth(initialWantClientAuth); + // needClientAuth = true and thus wantClientAuth = false + needClientAuthParams.setNeedClientAuth(true); + assertTrue(needClientAuthParams.getNeedClientAuth(), + "needClientAuth was expected to be true but wasn't"); + assertFalse(needClientAuthParams.getWantClientAuth(), + "wantClientAuth was expected to be false but wasn't"); + // now set needClientAuth = false and verify that both needClientAuth and wantClientAuth + // are now false + needClientAuthParams.setNeedClientAuth(false); + assertFalse(needClientAuthParams.getNeedClientAuth(), + "needClientAuth was expected to be false but wasn't"); + assertFalse(needClientAuthParams.getWantClientAuth(), + "wantClientAuth was expected to be false but wasn't"); + } + + /** + * sets {@link HttpsParameters#setWantClientAuth(boolean)} and verifies + * that subsequent calls to {@link HttpsParameters#getWantClientAuth()} returns + * the set value and {@link HttpsParameters#getNeedClientAuth()} returns false + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testWantClientAuth(final boolean initialNeedClientAuth) throws Exception { + HttpsParameters wantClientAuthParams = new Params(); + // first set needClientAuth to an initial value to verify that it later gets reset + wantClientAuthParams.setNeedClientAuth(initialNeedClientAuth); + // wantClientAuth = true and thus needClientAuth = false + wantClientAuthParams.setWantClientAuth(true); + assertTrue(wantClientAuthParams.getWantClientAuth(), + "wantClientAuth was expected to be true but wasn't"); + assertFalse(wantClientAuthParams.getNeedClientAuth(), + "needClientAuth was expected to be false but wasn't"); + // now set wantClientAuth = false and verify that both wantClientAuth and needClientAuth + // are now false + wantClientAuthParams.setWantClientAuth(false); + assertFalse(wantClientAuthParams.getWantClientAuth(), + "wantClientAuth was expected to be false but wasn't"); + assertFalse(wantClientAuthParams.getNeedClientAuth(), + "needClientAuth was expected to be false but wasn't"); + } + + /** + * Starts a {@link HttpsServer} by + * {@linkplain HttpsParameters#setNeedClientAuth(boolean) setting needClientAuth} as + * {@code true}. The client is then configured either to present the client certificates + * during the TLS handshake or configured not to present them. In the case where the + * client presents the client certificates, the HTTP request issued by the client is + * expected to pass. In the other case where the client doesn't present the certificates, + * the test verifies that the HTTP request fails due to a connection error + * (caused by TLS handshake failure) + * + * @param presentClientCerts true if the client should present certificates + * during TLS handshake, false otherwise + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testServerNeedClientAuth(final boolean presentClientCerts) throws Exception { + // SSLContext which contains both the key and the trust material and will be used + // by the server + final SSLContext serverSSLCtx = new SimpleSSLContext().get(); + assertNotNull(serverSSLCtx, "could not create SSLContext"); + final HttpsConfigurator configurator = new HttpsConfigurator(serverSSLCtx) { + @Override + public void configure(final HttpsParameters params) { + // we intentionally don't call params.setSSLParameters() + // and instead call params.setNeedClientAuth() + params.setNeedClientAuth(true); // "require" the client to present certs + } + }; + try (final ExecutorService executor = Executors.newCachedThreadPool(SRV_THREAD_FACTORY)) { + final HttpsServer server = createHttpsServer(executor, configurator); + server.start(); + System.out.println("started server at " + server.getAddress()); + try { + final HttpClient.Builder builder = createClientBuilder(); + // if the client is expected to present client certificates, then + // we construct a SSLContext which has both key and trust material. + // otherwise we construct a SSLContext that only has trust material + // and thus won't present certificates during TLS handshake + final SSLContext clientSSLCtx = presentClientCerts + ? serverSSLCtx : onlyTrustStoreContext(); + // construct the client using the SSLContext + try (final HttpClient client = builder.sslContext(clientSSLCtx) + .build()) { + // issue a request + final URI reqURI = URIBuilder.newBuilder() + .scheme("https") + .host(server.getAddress().getAddress()) + .port(server.getAddress().getPort()) + .path("/") + .build(); + System.out.println("issuing request to " + reqURI); + final HttpResponse resp; + try { + resp = client.send(HttpRequest.newBuilder(reqURI).build(), + BodyHandlers.discarding()); + if (!presentClientCerts) { + // request was expected to fail since the server was configured to force + // the client to present the client cert, but the client didn't + // present any + fail("request was expected to fail, but didn't"); + } + assertEquals(200, resp.statusCode(), "unexpected response code"); + // verify the client did present the certs + assertTrue(resp.sslSession().isPresent(), "missing SSLSession on response"); + assertNotNull(resp.sslSession().get().getLocalCertificates(), + "client was expected to present certs to the server, but didn't"); + } catch (IOException ioe) { + if (presentClientCerts) { + // wasn't expected to fail, just let the exception propagate + throw ioe; + } + // verify it failed due to right reason + Throwable cause = ioe; + while (cause != null) { + // either of SocketException or SSLHandshakeException are OK + if (cause instanceof SocketException se) { + final String msg = se.getMessage(); + assertTrue(msg != null && msg.contains("Connection reset"), + "unexpected message in SocketException: " + msg); + System.out.println("received the expected exception: " + se); + break; + } else if (cause instanceof SSLHandshakeException she) { + final String msg = she.getMessage(); + assertTrue(msg != null && msg.contains("certificate_required"), + "unexpected message in SSLHandshakeException: " + msg); + System.out.println("received the expected exception: " + she); + break; + } + cause = cause.getCause(); + } + if (cause == null) { + // didn't find expected exception, rethrow original exception + throw ioe; + } + } + } + } finally { + System.out.println("Stopping server at " + server.getAddress()); + server.stop(0 /* delay */); + } + } + } + + /** + * Starts a {@link HttpsServer} by + * {@linkplain HttpsParameters#setWantClientAuth(boolean) setting wantClientAuth} as + * {@code true}. The client is then configured to either present the client certificates + * during the TLS handshake or configured not to present them. In both these cases the + * HTTP request issued by the client is expected to pass. + * + * @param presentClientCerts true if the client should present certificates + * during TLS handshake, false otherwise + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testServerWantClientAuth(final boolean presentClientCerts) throws Exception { + // SSLContext which contains both the key and the trust material and will be used + // by the server + final SSLContext serverSSLCtx = new SimpleSSLContext().get(); + assertNotNull(serverSSLCtx, "could not create SSLContext"); + final HttpsConfigurator configurator = new HttpsConfigurator(serverSSLCtx) { + @Override + public void configure(final HttpsParameters params) { + // we intentionally don't call params.setSSLParameters() + // and instead call params.setWantClientAuth() + params.setWantClientAuth(true); // "request" the client to present certs + } + }; + try (final ExecutorService executor = Executors.newCachedThreadPool(SRV_THREAD_FACTORY)) { + final HttpsServer server = createHttpsServer(executor, configurator); + server.start(); + System.out.println("started server at " + server.getAddress()); + try { + final HttpClient.Builder builder = createClientBuilder(); + // if the client is expected to present client certificates, then + // we construct a SSLContext which has both key and trust material. + // otherwise we construct a SSLContext that only has trust material + // and thus won't present certificates during TLS handshake + final SSLContext clientSSLCtx = presentClientCerts + ? serverSSLCtx : onlyTrustStoreContext(); + // construct the client using the SSLContext + try (final HttpClient client = builder.sslContext(clientSSLCtx) + .build()) { + // issue a request + final URI reqURI = URIBuilder.newBuilder() + .scheme("https") + .host(server.getAddress().getAddress()) + .port(server.getAddress().getPort()) + .path("/") + .build(); + System.out.println("issuing request to " + reqURI); + final HttpResponse resp = client.send( + HttpRequest.newBuilder(reqURI).build(), BodyHandlers.discarding()); + assertEquals(200, resp.statusCode(), "unexpected response code"); + if (presentClientCerts) { + // verify the client did present the certs + assertTrue(resp.sslSession().isPresent(), "missing SSLSession on response"); + assertNotNull(resp.sslSession().get().getLocalCertificates(), + "client was expected to present certs to the server, but didn't"); + } + } + } finally { + System.out.println("Stopping server at " + server.getAddress()); + server.stop(0 /* delay */); + } + } + } + + private static HttpsServer createHttpsServer(final Executor executor, + final HttpsConfigurator configurator) + throws IOException { + final InetAddress loopback = InetAddress.getLoopbackAddress(); + final HttpsServer server = HttpsServer.create( + new InetSocketAddress(loopback, 0), 0 /* backlog */); + server.setExecutor(executor); + server.setHttpsConfigurator(configurator); + server.createContext("/", new AllOKHandler()); + return server; + } + + private static HttpClient.Builder createClientBuilder() { + return HttpClient.newBuilder().version(HTTP_1_1) + .proxy(NO_PROXY); + } + + /** + * Creates and returns a {@link SSLContext} which only has trust material + * and doesn't have any test specific keys. + */ + private static SSLContext onlyTrustStoreContext() throws Exception { + final KeyStore keyStore = loadTestKeyStore(); + final TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX"); + tmf.init(keyStore); + final SSLContext ctx = SSLContext.getInstance("TLS"); + // initialize with only trust managers + ctx.init(null, tmf.getTrustManagers(), null); + return ctx; + } + + private static KeyStore loadTestKeyStore() throws Exception { + return AccessController.doPrivileged( + new PrivilegedExceptionAction() { + @Override + public KeyStore run() throws Exception { + final String testKeys = System.getProperty("test.src") + + "/" + + "../../../../../../test/lib/jdk/test/lib/net/testkeys"; + try (final FileInputStream fis = new FileInputStream(testKeys)) { + final char[] passphrase = "passphrase".toCharArray(); + final KeyStore ks = KeyStore.getInstance("PKCS12"); + ks.load(fis, passphrase); + return ks; + } + } + }); + } + + // no-op implementations of the abstract methods of HttpsParameters + private static final class Params extends HttpsParameters { + + @Override + public HttpsConfigurator getHttpsConfigurator() { + // no-op + return null; + } + + @Override + public InetSocketAddress getClientAddress() { + // no-op + return null; + } + + @Override + public void setSSLParameters(SSLParameters params) { + // no-op + } + } + + // A HttpHandler which just returns 200 response code + private static final class AllOKHandler implements HttpHandler { + + private static final int NO_RESPONSE_BODY = -1; + + @Override + public void handle(final HttpExchange exchange) throws IOException { + System.out.println("responding to request: " + exchange.getRequestURI()); + exchange.sendResponseHeaders(200, NO_RESPONSE_BODY); + } + } +}