Skip to content

Core: Expose HostnameVerificationPolicy in TLSConfigurer #15500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adutra wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Core: Expose HostnameVerificationPolicy in TLSConfigurer #15500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7502238
Expose HostnameVerificationPolicy in TLSConfigurer
adutra Mar 3, 2026
6e3cfb8
set default to CLIENT
adutra Mar 10, 2026
19fc736
declare all BC artifacts
adutra Mar 19, 2026
2b92d46
add test
adutra Mar 20, 2026
e8b1810
add comment
adutra Mar 20, 2026
c686919
don't expose HostnameVerificationPolicy
adutra Mar 31, 2026
b5bb63a
Address review feedback: split try blocks
adutra Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,13 @@ 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
Copy link
Copy Markdown
Contributor

@nastra nastra Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth at least adding a comment on why these dependencies are being added, since it's not obvious

testImplementation libs.bouncycastle.bcutil
testImplementation libs.bouncycastle.bcprov
}
}

Expand Down
10 changes: 9 additions & 1 deletion core/src/main/java/org/apache/iceberg/rest/HTTPClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -410,13 +412,19 @@ static HttpClientConnectionManager configureConnectionManager(Map<String, String

TLSConfigurer tlsConfigurer = loadTlsConfigurer(properties);
if (tlsConfigurer != null) {
HostnameVerifier customVerifier = tlsConfigurer.hostnameVerifier();
HostnameVerificationPolicy verificationPolicy =
customVerifier != null
? HostnameVerificationPolicy.CLIENT
: HostnameVerificationPolicy.BUILTIN;
connectionManagerBuilder.setTlsSocketStrategy(
new DefaultClientTlsStrategy(
tlsConfigurer.sslContext(),
tlsConfigurer.supportedProtocols(),
tlsConfigurer.supportedCipherSuites(),
SSLBufferMode.STATIC,
tlsConfigurer.hostnameVerifier()));
verificationPolicy,
customVerifier));
}

return connectionManagerBuilder.build();
Expand Down
12 changes: 10 additions & 2 deletions core/src/main/java/org/apache/iceberg/rest/auth/TLSConfigurer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
package org.apache.iceberg.rest.auth;

import java.util.Map;
import javax.annotation.Nullable;
import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.SSLContext;
import org.apache.hc.client5.http.ssl.HttpsSupport;
import org.apache.hc.core5.ssl.SSLContexts;

public interface TLSConfigurer {
Expand All @@ -32,8 +32,16 @@ default SSLContext sslContext() {
return SSLContexts.createDefault();
}

/**
* Returns a custom {@link HostnameVerifier} to use for hostname verification, or {@code null} to
* use the default JSSE built-in hostname verifier.
*
* <p>If 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() {
Copy link
Copy Markdown
Contributor

@stevenzwu stevenzwu Mar 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RussellSpitzer @adutra this is the method where I suggested adding Javadoc. or you think this is redundant and the intention is obvious from the API already?

/**
 * If overwritten, only the custom verifier is executed and
 * Java Secure Socket Extension (JSSE) built-in verifier won't be executed.
 */

I also like Russell's suggestion from email of making this method returning null so that we can get rid of the new method above.

Copy link
Copy Markdown
Contributor Author

@adutra adutra Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevenzwu @RussellSpitzer I implemented the suggested approach, PTAL.

I want to stress though that returning null instead of HttpsSupport.getDefaultHostnameVerifier() here is a behavioral change.

The built-in verifier and Apache's default verifier both do roughly the same verifications, but the latter is more strict/complete. They also kick in in different phases and throw different exceptions.

Copy link
Copy Markdown
Contributor

@stevenzwu stevenzwu Mar 31, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to stress though that returning null instead of HttpsSupport.getDefaultHostnameVerifier() here is a behavioral change.

Not really a behavior change btw 1.10 and 1.11 since the original change from Bryant hasn't been released it.

And returning null is essentially the same as using the default built-in hostname verifier.

Copy link
Copy Markdown
Contributor Author

@adutra adutra Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really a behavior change btw 1.10 and 1.11 since the original change from Bryant hasn't been released it.

From what I see, TLSConfigurer has been released in 1.10.

return HttpsSupport.getDefaultHostnameVerifier();
return null;
}

default String[] supportedProtocols() {
Expand Down
109 changes: 109 additions & 0 deletions core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,37 +35,48 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import java.io.IOException;
import java.net.SocketTimeoutException;
import java.nio.file.Path;
import java.security.KeyStore;
import java.security.cert.CertificateException;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.TimeUnit;
import java.util.function.Consumer;
import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.SSLContext;
import javax.net.ssl.TrustManagerFactory;
import org.apache.hc.client5.http.auth.AuthScope;
import org.apache.hc.client5.http.auth.UsernamePasswordCredentials;
import org.apache.hc.client5.http.config.ConnectionConfig;
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.NoopHostnameVerifier;
import org.apache.hc.core5.http.HttpHost;
import org.apache.hc.core5.http.HttpStatus;
import org.apache.iceberg.IcebergBuild;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.rest.auth.AuthSession;
import org.apache.iceberg.rest.auth.TLSConfigurer;
import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.iceberg.rest.responses.ErrorResponseParser;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EnumSource;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockserver.configuration.Configuration;
import org.mockserver.integration.ClientAndServer;
import org.mockserver.logging.MockServerLogger;
import org.mockserver.matchers.Times;
import org.mockserver.model.HttpRequest;
import org.mockserver.model.HttpResponse;
import org.mockserver.socket.tls.KeyStoreFactory;
import org.mockserver.verify.VerificationTimes;

/**
Expand All @@ -87,6 +98,7 @@ public class TestHTTPClient {
private static RESTClient restClient;

public static class DefaultTLSConfigurer implements TLSConfigurer {

public static int count = 0;

public DefaultTLSConfigurer() {
Expand All @@ -95,6 +107,7 @@ public DefaultTLSConfigurer() {
}

public static class TLSConfigurerMissingNoArgCtor implements TLSConfigurer {

TLSConfigurerMissingNoArgCtor(String str) {}
}

Expand Down Expand Up @@ -395,6 +408,101 @@ public void testLoadTLSConfigurerNotImplementTLSConfigurer() {
.hasMessageContaining("does not implement TLSConfigurer");
}

/** A TLSConfigurer that relies on the default (built-in) JSSE verifier. */
public static class BuiltInHostnameVerifierTLSConfigurer implements TLSConfigurer {

@Override
public SSLContext sslContext() {
return mockServerSSLContext();
}
}

/** A TLSConfigurer that overrides hostnameVerifier() to return a custom verifier. */
public static class CustomHostnameVerifierTLSConfigurer implements TLSConfigurer {

@Override
public SSLContext sslContext() {
return mockServerSSLContext();
}

@Override
public HostnameVerifier hostnameVerifier() {
return NoopHostnameVerifier.INSTANCE;
}
}

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 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.
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/hostname-verifier/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);

// 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()) {
assertThatThrownBy(() -> builtInVerifierClient.head(path, Map.of(), (unused) -> {}))
.rootCause()
.isInstanceOf(CertificateException.class)
.hasMessage("No subject alternative names matching IP address 127.0.0.1 found");
Copy link
Copy Markdown
Member

@RussellSpitzer RussellSpitzer Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently this is wording JDK specific, May come up later if folks are testing on other architectures. But I think we are mostly testing on OpenJDK, Oracle JDK and the other OpenJDK based versions.

OpenJDK / Oracle JDK: "No subject alternative names matching IP address 127.0.0.1 found" — this is the message in sun.security.util.HostnameChecker.

IBM Semeru / J9: IBM's JSSE implementation has its own HostnameChecker equivalent with different wording. Historically it uses messages like "No name matching 127.0.0.1 found" or similar variations.

Eclipse Temurin / Azul Zulu: These are typically OpenJDK-based and share the message, but there's no guarantee across versions — the sun.security package is internal and its strings can change between releases without notice.

}

// 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();
}
}
}

@Test
public void testSocketTimeout() throws IOException {
long socketTimeoutMs = 2000L;
Expand Down Expand Up @@ -613,6 +721,7 @@ private static Item doExecuteRequest(
}

public static class Item implements RESTRequest, RESTResponse {

private Long id;
private String data;

Expand Down
4 changes: 4 additions & 0 deletions gradle/libs.versions.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.82"
bson-ver = "4.11.5"
caffeine = "2.9.3"
calcite = "1.41.0"
Expand Down Expand Up @@ -111,6 +112,9 @@ 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" }
Copy link
Copy Markdown
Contributor

@nandorKollar nandorKollar Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is new dependency, do we have to add the licence of this dependency to one of the LICENSE files in Iceberg?

Copy link
Copy Markdown
Contributor Author

@adutra adutra Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, since it's used only in tests.

Copy link
Copy Markdown
Contributor

@nandorKollar nandorKollar Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying.

Copy link
Copy Markdown
Contributor

@steveloughran steveloughran Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's good because bouncycastle complicates export rules, or at least due diligence

Copy link
Copy Markdown
Contributor

@stevenzwu stevenzwu Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumes the bouncycastle bcpkix is a transitive dep already pulled in by the mockerserver lib. is the purpose here just to upgrade the version?

Copy link
Copy Markdown
Contributor Author

@adutra adutra Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good question! Short answer: yes.

MockServer does bring BouncyCastle transitively — bcprov, bcpkix, and bcutil – but at version 1.72, which is somewhat old.

The problem is that other deps (Hadoop) bring bcprov 1.82, and Gradle resolves the conflict by upgrading bcprov to 1.82. But bcpkix and bcutil stay at 1.72, since nothing else declares a higher version. This creates a mismatch: bcpkix 1.72 + bcprov 1.82 = NoClassDefFoundError.

I only upgraded bcpkix here, because that is enough for running the TLS tests. If you prefer, I can also explicitly upgrade bcutil.

Copy link
Copy Markdown
Contributor

@stevenzwu stevenzwu Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is probably good to just ensure the same version for bouncycastle family of jars. That is the current practice. E.g., there is just one version number for the two jetty jars in the toml file.

Copy link
Copy Markdown
Contributor Author

@adutra adutra Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right: I implemented your suggestion.

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" }
Expand Down
Loading