From 7502238c548fee87426591bde914a4a2be0b18e0 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Tue, 3 Mar 2026 12:04:33 +0100 Subject: [PATCH 1/7] Expose HostnameVerificationPolicy in TLSConfigurer Apache HttpClient 5.4 introduced a new component: `HostnameVerificationPolicy`, which determines whether hostname verification is done by the JSSE provider (at socket level, during TLS handshake), the HttpClient (after TLS handshake), or both. This change exposes `HostnameVerificationPolicy` in `TLSConfigurer`. This component is particularly useful when attempting to bypass hostname verification, e.g. by using the `NoopHostnameVerifier`. The default policy is set to `BOTH`, which produces the same result as before. --- build.gradle | 1 + .../org/apache/iceberg/rest/HTTPClient.java | 1 + .../iceberg/rest/auth/TLSConfigurer.java | 5 + .../apache/iceberg/rest/TestHTTPClient.java | 105 ++++++++++++++++++ gradle/libs.versions.toml | 2 + 5 files changed, 114 insertions(+) diff --git a/build.gradle b/build.gradle index 765a7fe8203e..23ddfdd44d0d 100644 --- a/build.gradle +++ b/build.gradle @@ -399,6 +399,7 @@ project(':iceberg-core') { exclude group: 'junit' } testImplementation libs.awaitility + testImplementation libs.bouncycastle.bcpkix } } diff --git a/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java b/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java index 86eceba21c95..8b1a0a3d2fb9 100644 --- a/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java +++ b/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java @@ -416,6 +416,7 @@ static HttpClientConnectionManager configureConnectionManager(Map properties = + Map.of(HTTPClient.REST_TLS_CONFIGURER, LaxTLSConfigurer.class.getName()); + + try (HTTPClient client = + HTTPClient.builder(properties) + .uri(String.format("https://127.0.0.1:%d", tlsPort)) + .withAuthSession(AuthSession.EMPTY) + .build()) { + + if (policy == HostnameVerificationPolicy.CLIENT) { + // With hostname verification performed by the HttpClient *only*, + // the request should succeed + assertThatCode(() -> client.head(path, Map.of(), (unused) -> {})) + .doesNotThrowAnyException(); + } else { + // With hostname verification performed by the JSSE provider, + // the request should fail with a CertificateException + assertThatThrownBy(() -> client.head(path, Map.of(), (unused) -> {})) + .rootCause() + .isInstanceOf(CertificateException.class) + .hasMessage("No subject alternative names matching IP address 127.0.0.1 found"); + } + } + } + } + @Test public void testSocketTimeout() throws IOException { long socketTimeoutMs = 2000L; diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 8cd9e566b367..a2c7622d645c 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -36,6 +36,7 @@ awaitility = "4.3.0" awssdk-bom = "2.42.8" azuresdk-bom = "1.3.5" awssdk-s3accessgrants = "2.4.1" +bouncycastle = "1.83" bson-ver = "4.11.5" caffeine = "2.9.3" calcite = "1.41.0" @@ -111,6 +112,7 @@ awssdk-bom = { module = "software.amazon.awssdk:bom", version.ref = "awssdk-bom" awssdk-s3accessgrants = { module = "software.amazon.s3.accessgrants:aws-s3-accessgrants-java-plugin", version.ref = "awssdk-s3accessgrants" } azuresdk-bom = { module = "com.azure:azure-sdk-bom", version.ref = "azuresdk-bom" } bson = { module = "org.mongodb:bson", version.ref = "bson-ver"} +bouncycastle-bcpkix = { module = "org.bouncycastle:bcpkix-jdk18on", version.ref = "bouncycastle" } caffeine = { module = "com.github.ben-manes.caffeine:caffeine", version.ref = "caffeine" } calcite-core = { module = "org.apache.calcite:calcite-core", version.ref = "calcite" } calcite-druid = { module = "org.apache.calcite:calcite-druid", version.ref = "calcite" } From 6e3cfb879af364a05e7220bbd3f744a4c6209d14 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Tue, 10 Mar 2026 18:46:03 +0100 Subject: [PATCH 2/7] set default to CLIENT --- .../main/java/org/apache/iceberg/rest/auth/TLSConfigurer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/auth/TLSConfigurer.java b/core/src/main/java/org/apache/iceberg/rest/auth/TLSConfigurer.java index 0065a2d9ce49..ad7334c691d0 100644 --- a/core/src/main/java/org/apache/iceberg/rest/auth/TLSConfigurer.java +++ b/core/src/main/java/org/apache/iceberg/rest/auth/TLSConfigurer.java @@ -34,7 +34,7 @@ default SSLContext sslContext() { } default HostnameVerificationPolicy hostnameVerificationPolicy() { - return HostnameVerificationPolicy.BOTH; + return HostnameVerificationPolicy.CLIENT; } default HostnameVerifier hostnameVerifier() { From 19fc736f0f3e519d0acd0743bc9c9e157825139c Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Fri, 20 Mar 2026 00:09:42 +0100 Subject: [PATCH 3/7] declare all BC artifacts --- build.gradle | 2 ++ gradle/libs.versions.toml | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 23ddfdd44d0d..673fc5413627 100644 --- a/build.gradle +++ b/build.gradle @@ -400,6 +400,8 @@ project(':iceberg-core') { } testImplementation libs.awaitility testImplementation libs.bouncycastle.bcpkix + testImplementation libs.bouncycastle.bcutil + testImplementation libs.bouncycastle.bcprov } } diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index a2c7622d645c..0726b23e9a09 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -36,7 +36,7 @@ awaitility = "4.3.0" awssdk-bom = "2.42.8" azuresdk-bom = "1.3.5" awssdk-s3accessgrants = "2.4.1" -bouncycastle = "1.83" +bouncycastle = "1.82" bson-ver = "4.11.5" caffeine = "2.9.3" calcite = "1.41.0" @@ -113,6 +113,8 @@ awssdk-s3accessgrants = { module = "software.amazon.s3.accessgrants:aws-s3-acces azuresdk-bom = { module = "com.azure:azure-sdk-bom", version.ref = "azuresdk-bom" } bson = { module = "org.mongodb:bson", version.ref = "bson-ver"} bouncycastle-bcpkix = { module = "org.bouncycastle:bcpkix-jdk18on", version.ref = "bouncycastle" } +bouncycastle-bcprov = { module = "org.bouncycastle:bcprov-jdk18on", version.ref = "bouncycastle" } +bouncycastle-bcutil = { module = "org.bouncycastle:bcutil-jdk18on", version.ref = "bouncycastle" } caffeine = { module = "com.github.ben-manes.caffeine:caffeine", version.ref = "caffeine" } calcite-core = { module = "org.apache.calcite:calcite-core", version.ref = "calcite" } calcite-druid = { module = "org.apache.calcite:calcite-druid", version.ref = "calcite" } From 2b92d46b3baad1284438e43d3298bd64cdb0e89c Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Fri, 20 Mar 2026 13:24:39 +0100 Subject: [PATCH 4/7] add test --- .../apache/iceberg/rest/TestHTTPClient.java | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java b/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java index 7e6743b819f8..e7e71af1676f 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java @@ -500,6 +500,79 @@ public void testTLSConfigurerHostnameVerificationPolicy( } } + /** + * A TLSConfigurer that overrides sslContext() and hostnameVerifier() but does NOT override + * hostnameVerificationPolicy(), relying on the default (CLIENT). This directly tests that the + * default hostname verification policy allows the NoopHostnameVerifier to take effect. + */ + public static class ClientPolicyTLSConfigurer implements TLSConfigurer { + + @Override + public SSLContext sslContext() { + try { + KeyStore keyStore = + new KeyStoreFactory(Configuration.configuration(), new MockServerLogger()) + .loadOrCreateKeyStore(); + TrustManagerFactory tmf = + TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); + tmf.init(keyStore); + SSLContext sslContext = SSLContext.getInstance("TLSv1.2"); + sslContext.init(null, tmf.getTrustManagers(), null); + return sslContext; + } catch (Exception e) { + throw new RuntimeException("Failed to create SSLContext", e); + } + } + + @Override + public HostnameVerifier hostnameVerifier() { + return NoopHostnameVerifier.INSTANCE; + } + } + + @Test + public void testTLSConfigurerDefaultHostnameVerificationPolicy(@TempDir Path temp) + throws IOException { + + // Start a dedicated MockServer with a certificate that does NOT include 127.0.0.1 or localhost + // in its SANs. The default hostname verification policy (CLIENT) should allow the + // NoopHostnameVerifier to bypass hostname verification, so the request should succeed. + Configuration tlsConfig = Configuration.configuration(); + tlsConfig.proactivelyInitialiseTLS(true); + tlsConfig.preventCertificateDynamicUpdate(true); + tlsConfig.sslCertificateDomainName("example.com"); + tlsConfig.sslSubjectAlternativeNameIps(Sets.newHashSet("1.2.3.4")); + tlsConfig.sslSubjectAlternativeNameDomains(Sets.newHashSet("example.com")); + tlsConfig.directoryToSaveDynamicSSLCertificate(temp.toFile().getAbsolutePath()); + + int tlsPort = PORT + 1; + try (ClientAndServer server = startClientAndServer(tlsConfig, tlsPort)) { + + String path = "tls/default-policy/path"; + HttpRequest mockRequest = + request() + .withPath("/" + path) + .withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT)); + HttpResponse mockResponse = response().withStatusCode(200).withBody("TLS response"); + server.when(mockRequest).respond(mockResponse); + + Map properties = + Map.of(HTTPClient.REST_TLS_CONFIGURER, ClientPolicyTLSConfigurer.class.getName()); + + try (HTTPClient client = + HTTPClient.builder(properties) + .uri(String.format("https://127.0.0.1:%d", tlsPort)) + .withAuthSession(AuthSession.EMPTY) + .build()) { + + // With the default hostname verification policy (CLIENT) and NoopHostnameVerifier, + // the request should succeed even though the certificate SANs don't match 127.0.0.1 + assertThatCode(() -> client.head(path, Map.of(), (unused) -> {})) + .doesNotThrowAnyException(); + } + } + } + @Test public void testSocketTimeout() throws IOException { long socketTimeoutMs = 2000L; From e8b1810fa44ac55f921f05232df700bf372a9222 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Fri, 20 Mar 2026 13:26:18 +0100 Subject: [PATCH 5/7] add comment --- build.gradle | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/build.gradle b/build.gradle index 673fc5413627..92b27c57db8b 100644 --- a/build.gradle +++ b/build.gradle @@ -399,6 +399,10 @@ project(':iceberg-core') { exclude group: 'junit' } testImplementation libs.awaitility + + // Lock BouncyCastle versions to avoid version mismatches + // when these dependencies are added transitively. + // Required for TLS tests with MockServer. testImplementation libs.bouncycastle.bcpkix testImplementation libs.bouncycastle.bcutil testImplementation libs.bouncycastle.bcprov From c68691973ddbe58f321c0d3c27fbd284e7802bc1 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Tue, 31 Mar 2026 17:24:52 +0200 Subject: [PATCH 6/7] don't expose HostnameVerificationPolicy --- .../org/apache/iceberg/rest/HTTPClient.java | 11 +- .../iceberg/rest/auth/TLSConfigurer.java | 17 +- .../apache/iceberg/rest/TestHTTPClient.java | 178 ++++++------------ 3 files changed, 72 insertions(+), 134 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java b/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java index 8b1a0a3d2fb9..c359404ec6be 100644 --- a/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java +++ b/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java @@ -29,6 +29,7 @@ import java.util.concurrent.ConcurrentMap; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; +import javax.net.ssl.HostnameVerifier; import org.apache.hc.client5.http.auth.AuthScope; import org.apache.hc.client5.http.auth.CredentialsProvider; import org.apache.hc.client5.http.auth.UsernamePasswordCredentials; @@ -43,6 +44,7 @@ import org.apache.hc.client5.http.io.HttpClientConnectionManager; import org.apache.hc.client5.http.protocol.HttpClientContext; import org.apache.hc.client5.http.ssl.DefaultClientTlsStrategy; +import org.apache.hc.client5.http.ssl.HostnameVerificationPolicy; import org.apache.hc.core5.http.ContentType; import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpHeaders; @@ -410,14 +412,19 @@ static HttpClientConnectionManager configureConnectionManager(MapIf a non-null verifier is returned, only the custom verifier is executed and the JSSE + * built-in hostname verifier won't be executed. + */ + @Nullable default HostnameVerifier hostnameVerifier() { - return HttpsSupport.getDefaultHostnameVerifier(); + return null; } default String[] supportedProtocols() { diff --git a/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java b/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java index e7e71af1676f..d7da7c3b9946 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java @@ -52,7 +52,6 @@ import org.apache.hc.client5.http.impl.auth.BasicCredentialsProvider; import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager; import org.apache.hc.client5.http.io.HttpClientConnectionManager; -import org.apache.hc.client5.http.ssl.HostnameVerificationPolicy; import org.apache.hc.client5.http.ssl.NoopHostnameVerifier; import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpStatus; @@ -407,121 +406,21 @@ public void testLoadTLSConfigurerNotImplementTLSConfigurer() { .hasMessageContaining("does not implement TLSConfigurer"); } - public static class LaxTLSConfigurer implements TLSConfigurer { - - private static HostnameVerificationPolicy policy = HostnameVerificationPolicy.BUILTIN; - - static void setPolicy(HostnameVerificationPolicy policy) { - LaxTLSConfigurer.policy = policy; - } + /** A TLSConfigurer that relies on the default (built-in) JSSE verifier. */ + public static class BuiltInHostnameVerifierTLSConfigurer implements TLSConfigurer { @Override public SSLContext sslContext() { - // Create an SSLContext that trusts MockServer's CA certificate but still performs hostname - // verification during SSL handshake - try { - KeyStore keyStore = - new KeyStoreFactory(Configuration.configuration(), new MockServerLogger()) - .loadOrCreateKeyStore(); - TrustManagerFactory tmf = - TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); - tmf.init(keyStore); - SSLContext sslContext = SSLContext.getInstance("TLSv1.2"); - sslContext.init(null, tmf.getTrustManagers(), null); - return sslContext; - } catch (Exception e) { - throw new RuntimeException("Failed to create SSLContext", e); - } - } - - @Override - public HostnameVerificationPolicy hostnameVerificationPolicy() { - return policy; - } - - @Override - public HostnameVerifier hostnameVerifier() { - // Disable hostname verification at HttpClient level (post SSL handshake) - return NoopHostnameVerifier.INSTANCE; - } - } - - @ParameterizedTest - @EnumSource(HostnameVerificationPolicy.class) - public void testTLSConfigurerHostnameVerificationPolicy( - HostnameVerificationPolicy policy, @TempDir Path temp) throws IOException { - - // Start a dedicated MockServer with a certificate that does NOT include 127.0.0.1 or localhost - // in its SANs. Hostname verification must be disabled for this test to pass. - Configuration tlsConfig = Configuration.configuration(); - tlsConfig.proactivelyInitialiseTLS(true); - tlsConfig.preventCertificateDynamicUpdate(true); - tlsConfig.sslCertificateDomainName("example.com"); - tlsConfig.sslSubjectAlternativeNameIps(Sets.newHashSet("1.2.3.4")); - tlsConfig.sslSubjectAlternativeNameDomains(Sets.newHashSet("example.com")); - tlsConfig.directoryToSaveDynamicSSLCertificate(temp.toFile().getAbsolutePath()); - - int tlsPort = PORT + 1; - try (ClientAndServer server = startClientAndServer(tlsConfig, tlsPort)) { - - String path = "tls/path"; - HttpRequest mockRequest = - request() - .withPath("/" + path) - .withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT)); - HttpResponse mockResponse = response().withStatusCode(200).withBody("TLS response"); - server.when(mockRequest).respond(mockResponse); - - LaxTLSConfigurer.setPolicy(policy); - - Map properties = - Map.of(HTTPClient.REST_TLS_CONFIGURER, LaxTLSConfigurer.class.getName()); - - try (HTTPClient client = - HTTPClient.builder(properties) - .uri(String.format("https://127.0.0.1:%d", tlsPort)) - .withAuthSession(AuthSession.EMPTY) - .build()) { - - if (policy == HostnameVerificationPolicy.CLIENT) { - // With hostname verification performed by the HttpClient *only*, - // the request should succeed - assertThatCode(() -> client.head(path, Map.of(), (unused) -> {})) - .doesNotThrowAnyException(); - } else { - // With hostname verification performed by the JSSE provider, - // the request should fail with a CertificateException - assertThatThrownBy(() -> client.head(path, Map.of(), (unused) -> {})) - .rootCause() - .isInstanceOf(CertificateException.class) - .hasMessage("No subject alternative names matching IP address 127.0.0.1 found"); - } - } + return mockServerSSLContext(); } } - /** - * A TLSConfigurer that overrides sslContext() and hostnameVerifier() but does NOT override - * hostnameVerificationPolicy(), relying on the default (CLIENT). This directly tests that the - * default hostname verification policy allows the NoopHostnameVerifier to take effect. - */ - public static class ClientPolicyTLSConfigurer implements TLSConfigurer { + /** A TLSConfigurer that overrides hostnameVerifier() to return a custom verifier. */ + public static class CustomHostnameVerifierTLSConfigurer implements TLSConfigurer { @Override public SSLContext sslContext() { - try { - KeyStore keyStore = - new KeyStoreFactory(Configuration.configuration(), new MockServerLogger()) - .loadOrCreateKeyStore(); - TrustManagerFactory tmf = - TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); - tmf.init(keyStore); - SSLContext sslContext = SSLContext.getInstance("TLSv1.2"); - sslContext.init(null, tmf.getTrustManagers(), null); - return sslContext; - } catch (Exception e) { - throw new RuntimeException("Failed to create SSLContext", e); - } + return mockServerSSLContext(); } @Override @@ -530,13 +429,27 @@ public HostnameVerifier hostnameVerifier() { } } + private static SSLContext mockServerSSLContext() { + try { + KeyStore keyStore = + new KeyStoreFactory(Configuration.configuration(), new MockServerLogger()) + .loadOrCreateKeyStore(); + TrustManagerFactory tmf = + TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); + tmf.init(keyStore); + SSLContext sslContext = SSLContext.getInstance("TLSv1.2"); + sslContext.init(null, tmf.getTrustManagers(), null); + return sslContext; + } catch (Exception e) { + throw new RuntimeException("Failed to create SSLContext", e); + } + } + @Test - public void testTLSConfigurerDefaultHostnameVerificationPolicy(@TempDir Path temp) - throws IOException { + public void testTLSConfigurerHostnameVerifier(@TempDir Path temp) throws IOException { - // Start a dedicated MockServer with a certificate that does NOT include 127.0.0.1 or localhost - // in its SANs. The default hostname verification policy (CLIENT) should allow the - // NoopHostnameVerifier to bypass hostname verification, so the request should succeed. + // Start a dedicated MockServer with a certificate that does NOT include + // 127.0.0.1 or localhost in its SANs. Configuration tlsConfig = Configuration.configuration(); tlsConfig.proactivelyInitialiseTLS(true); tlsConfig.preventCertificateDynamicUpdate(true); @@ -548,7 +461,7 @@ public void testTLSConfigurerDefaultHostnameVerificationPolicy(@TempDir Path tem int tlsPort = PORT + 1; try (ClientAndServer server = startClientAndServer(tlsConfig, tlsPort)) { - String path = "tls/default-policy/path"; + String path = "tls/hostname-verifier/path"; HttpRequest mockRequest = request() .withPath("/" + path) @@ -556,18 +469,33 @@ public void testTLSConfigurerDefaultHostnameVerificationPolicy(@TempDir Path tem HttpResponse mockResponse = response().withStatusCode(200).withBody("TLS response"); server.when(mockRequest).respond(mockResponse); - Map properties = - Map.of(HTTPClient.REST_TLS_CONFIGURER, ClientPolicyTLSConfigurer.class.getName()); - - try (HTTPClient client = - HTTPClient.builder(properties) - .uri(String.format("https://127.0.0.1:%d", tlsPort)) - .withAuthSession(AuthSession.EMPTY) - .build()) { - - // With the default hostname verification policy (CLIENT) and NoopHostnameVerifier, - // the request should succeed even though the certificate SANs don't match 127.0.0.1 - assertThatCode(() -> client.head(path, Map.of(), (unused) -> {})) + try (HTTPClient builtInVerifierClient = + HTTPClient.builder( + Map.of( + HTTPClient.REST_TLS_CONFIGURER, + BuiltInHostnameVerifierTLSConfigurer.class.getName())) + .uri(String.format("https://127.0.0.1:%d", tlsPort)) + .withAuthSession(AuthSession.EMPTY) + .build(); + HTTPClient customVerifierClient = + HTTPClient.builder( + Map.of( + HTTPClient.REST_TLS_CONFIGURER, + CustomHostnameVerifierTLSConfigurer.class.getName())) + .uri(String.format("https://127.0.0.1:%d", tlsPort)) + .withAuthSession(AuthSession.EMPTY) + .build()) { + + // With no custom hostnameVerifier (null), the BUILTIN policy is used automatically, + // so the JSSE built-in verifier rejects the connection because the SANs don't match + assertThatThrownBy(() -> builtInVerifierClient.head(path, Map.of(), (unused) -> {})) + .rootCause() + .isInstanceOf(CertificateException.class) + .hasMessage("No subject alternative names matching IP address 127.0.0.1 found"); + + // With a custom hostnameVerifier (NoopHostnameVerifier), the CLIENT policy is used + // automatically, so hostname verification is bypassed and the request succeeds + assertThatCode(() -> customVerifierClient.head(path, Map.of(), (unused) -> {})) .doesNotThrowAnyException(); } } From b5bb63a576788c1c0c35397c06ef8085b24ff4d0 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Wed, 1 Apr 2026 15:24:06 +0200 Subject: [PATCH 7/7] Address review feedback: split try blocks --- .../apache/iceberg/rest/TestHTTPClient.java | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java b/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java index d7da7c3b9946..701ae699f136 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java @@ -98,6 +98,7 @@ public class TestHTTPClient { private static RESTClient restClient; public static class DefaultTLSConfigurer implements TLSConfigurer { + public static int count = 0; public DefaultTLSConfigurer() { @@ -106,6 +107,7 @@ public DefaultTLSConfigurer() { } public static class TLSConfigurerMissingNoArgCtor implements TLSConfigurer { + TLSConfigurerMissingNoArgCtor(String str) {} } @@ -469,32 +471,32 @@ public void testTLSConfigurerHostnameVerifier(@TempDir Path temp) throws IOExcep HttpResponse mockResponse = response().withStatusCode(200).withBody("TLS response"); server.when(mockRequest).respond(mockResponse); + // With no custom hostnameVerifier (null), the BUILTIN policy is used automatically, + // so the JSSE built-in verifier rejects the connection because the SANs don't match try (HTTPClient builtInVerifierClient = - HTTPClient.builder( - Map.of( - HTTPClient.REST_TLS_CONFIGURER, - BuiltInHostnameVerifierTLSConfigurer.class.getName())) - .uri(String.format("https://127.0.0.1:%d", tlsPort)) - .withAuthSession(AuthSession.EMPTY) - .build(); - HTTPClient customVerifierClient = - HTTPClient.builder( - Map.of( - HTTPClient.REST_TLS_CONFIGURER, - CustomHostnameVerifierTLSConfigurer.class.getName())) - .uri(String.format("https://127.0.0.1:%d", tlsPort)) - .withAuthSession(AuthSession.EMPTY) - .build()) { - - // With no custom hostnameVerifier (null), the BUILTIN policy is used automatically, - // so the JSSE built-in verifier rejects the connection because the SANs don't match + HTTPClient.builder( + Map.of( + HTTPClient.REST_TLS_CONFIGURER, + BuiltInHostnameVerifierTLSConfigurer.class.getName())) + .uri(String.format("https://127.0.0.1:%d", tlsPort)) + .withAuthSession(AuthSession.EMPTY) + .build()) { assertThatThrownBy(() -> builtInVerifierClient.head(path, Map.of(), (unused) -> {})) .rootCause() .isInstanceOf(CertificateException.class) .hasMessage("No subject alternative names matching IP address 127.0.0.1 found"); + } - // With a custom hostnameVerifier (NoopHostnameVerifier), the CLIENT policy is used - // automatically, so hostname verification is bypassed and the request succeeds + // With a custom hostnameVerifier (NoopHostnameVerifier), the CLIENT policy is used + // automatically, so hostname verification is bypassed and the request succeeds + try (HTTPClient customVerifierClient = + HTTPClient.builder( + Map.of( + HTTPClient.REST_TLS_CONFIGURER, + CustomHostnameVerifierTLSConfigurer.class.getName())) + .uri(String.format("https://127.0.0.1:%d", tlsPort)) + .withAuthSession(AuthSession.EMPTY) + .build()) { assertThatCode(() -> customVerifierClient.head(path, Map.of(), (unused) -> {})) .doesNotThrowAnyException(); } @@ -719,6 +721,7 @@ private static Item doExecuteRequest( } public static class Item implements RESTRequest, RESTResponse { + private Long id; private String data;