smolnar82 commented on code in PR #1461: URL: https://github.com/apache/knox/pull/1461#discussion_r4338902495
########## gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/HttpClientResourceRetrieverTest.java: ########## @@ -0,0 +1,251 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.knox.gateway.services.token.impl; + +import com.nimbusds.jose.jwk.JWKSet; +import com.nimbusds.jose.util.Resource; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.knox.gateway.services.knoxidf.trustedoidcissuer.TestJwksHttpsServer; +import org.apache.knox.gateway.util.MetadataHttpClients; +import org.apache.knox.gateway.util.TruststoreSSLContextUtils; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import javax.net.ssl.SSLSocketFactory; +import java.io.IOException; +import java.net.InetAddress; +import java.net.Socket; +import java.net.URI; +import java.net.URL; +import java.security.KeyStore; +import java.util.ArrayList; +import java.util.List; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +public class HttpClientResourceRetrieverTest { + + private static final int SIZE_LIMIT = 51200; + private static final int TIMEOUT_MS = 2000; + + private TestJwksHttpsServer server; + private URL jwksUrl; + private SSLSocketFactory trustingFactory; + + @Before + public void setUp() throws Exception { + server = TestJwksHttpsServer.start(); + jwksUrl = URI.create(server.getJwksUrl()).toURL(); + final KeyStore truststore = KeyStore.getInstance(KeyStore.getDefaultType()); + truststore.load(null, null); + truststore.setCertificateEntry("ca", server.getCertificate()); + trustingFactory = TruststoreSSLContextUtils.getTruststoreSSLContext(truststore).getSocketFactory(); + } + + @After + public void tearDown() throws Exception { + if (server != null) { + server.close(); + } + } + + @Test + public void testRetrievesTheKeySet() throws Exception { + try (CloseableHttpClient client = clientTrusting(trustingFactory)) { + final Resource resource = new HttpClientResourceRetriever(client, SIZE_LIMIT).retrieveResource(jwksUrl); + + assertEquals(1, JWKSet.parse(resource.getContent()).getKeys().size()); + } + } + + /* + * The reason this retriever exists. BouncyCastle's JSSE takes the hostname for HTTPS endpoint + * identification from the socket; HttpsURLConnection, which Nimbus uses by default, creates the + * socket with no host and so the check fails before the certificate is examined. Apache + * HttpClient layers TLS over a connected socket and hands it the target host, which is what + * makes endpoint identification work for every provider without any special casing. + */ + @Test + public void testSuppliesTheTargetHostToTheSocket() throws Exception { + final HostRecordingSocketFactory recording = new HostRecordingSocketFactory(trustingFactory); + + try (CloseableHttpClient client = clientTrusting(recording)) { + new HttpClientResourceRetriever(client, SIZE_LIMIT).retrieveResource(jwksUrl); + } + + /* + * Only the layered call is assertable: HttpClient creates the underlying plain socket through + * its own connection factory, never through this one, so a "nothing hostless was created" + * assertion here would hold no matter what the production code did. + */ + assertEquals("the target host must reach the socket, or BouncyCastle cannot identify the endpoint", + List.of("localhost"), recording.layeredHosts); + } + + /* + * The host is not fixed at construction: it comes from the URL of each request, so a JWKS + * endpoint named by IP literal gets that IP and one named by DNS gets that name. Requests the + * same server by a second spelling to show the recorded host tracks the URL rather than being + * whatever the first request happened to use. + */ + @Test + public void testTheHostComesFromTheRequestedUrl() throws Exception { + final HostRecordingSocketFactory recording = new HostRecordingSocketFactory(trustingFactory); + /* + * The address "localhost" resolves to on this host, which is the one Jetty bound - assuming + * 127.0.0.1 would fail at connect wherever localhost is ::1. + */ + final String loopbackIp = InetAddress.getByName("localhost").getHostAddress(); + final URL byLoopbackIp = + URI.create("https://"; + hostForUrl(loopbackIp) + ":" + jwksUrl.getPort() + jwksUrl.getPath()).toURL(); + + try (CloseableHttpClient client = clientTrusting(recording)) { + final HttpClientResourceRetriever retriever = new HttpClientResourceRetriever(client, SIZE_LIMIT); + retriever.retrieveResource(jwksUrl); + try { + /* + * The certificate is issued for localhost, so this spelling may fail verification - which + * is correct, and beside the point: the host reaches the socket either way. + */ + retriever.retrieveResource(byLoopbackIp); + } catch (IOException expectedForAnUnmatchedName) { // NOPMD - asserted on the record below + /* ignored */ Review Comment: The k8s case, an IP URL with an IP-SAN cert, is never shown to succeed. Please add a positive test: a cert with an iPAddress SAN for loopback, fetched via `https://127.0.0.1:<port>/jwks`. Optionally add a BCJSSE run (bctls-jdk18on, test scope) to reproduce the original failure. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
