From 36d2c3b378435555fbe6ead150b6537b4a7882a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?JB=20Onofr=C3=A9?= Date: Wed, 4 Nov 2026 18:33:02 +0100 Subject: [PATCH] GH-1442: [JDBC] Harden hostnameOverride handling and tests Follow-up to #1363. - Don't copy `hostnameOverride` in the `ArrowFlightSqlClientHandler.Builder` copy constructor. The override is only valid for the original host and port, so a copied builder no longer carries it to another location by default. `getStreams` already sets it explicitly for each endpoint, so behavior is unchanged. - Trim `ArrowFlightConnectionConfigImpl.getHostnameOverride()` and return `null` for blank values, as its Javadoc says. - Record the `:authority` the server receives in `ConnectionTlsHostnameOverrideTest`. The plaintext test now checks that the connection's own host and port are sent, and a new test checks that the override replaces the whole authority and that a port can be included. - Document that the override replaces the port in `:authority`, and that a port can be included. Closes #1442. --- docs/source/flight_sql_jdbc_driver.rst | 5 +- .../client/ArrowFlightSqlClientHandler.java | 7 +- .../ArrowFlightConnectionConfigImpl.java | 11 ++- .../ConnectionTlsHostnameOverrideTest.java | 77 +++++++++++++++---- ...rrowFlightSqlClientHandlerBuilderTest.java | 6 +- .../ArrowFlightConnectionConfigImplTest.java | 15 ++++ 6 files changed, 98 insertions(+), 23 deletions(-) diff --git a/docs/source/flight_sql_jdbc_driver.rst b/docs/source/flight_sql_jdbc_driver.rst index 0c6be0a2d4..9d01c4faeb 100644 --- a/docs/source/flight_sql_jdbc_driver.rst +++ b/docs/source/flight_sql_jdbc_driver.rst @@ -124,7 +124,10 @@ case-sensitive. The supported parameters are: against and to send for SNI and as the gRPC ``:authority`` header, instead of **HOSTNAME**. Use this when connecting through a tunnel, a private endpoint, or an IP address - that does not match the server certificate. Endpoints returned by + that does not match the server certificate. The value replaces the + whole ``:authority``, so **PORT** is not sent unless it is included, + for example ``flight.example.com:32010``; only the hostname is used + for SNI and certificate verification. Endpoints returned by the server are only verified against this hostname if they are on the same host and port as the connection. Ignored when ``useEncryption`` is false. diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java index 346b0c0324..c49aa110f4 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java @@ -718,7 +718,8 @@ public static final class Builder { public Builder() {} /** - * Copies the builder. + * Copies the builder. The hostname override is not copied, since it is only valid for the + * original host and port. * * @param original The builder to base this copy off of. */ @@ -735,7 +736,6 @@ public Builder() {} this.token = original.token; this.useEncryption = original.useEncryption; this.disableCertificateVerification = original.disableCertificateVerification; - this.hostnameOverride = original.hostnameOverride; this.useSystemTrustStore = original.useSystemTrustStore; this.tlsRootCertificatesPath = original.tlsRootCertificatesPath; this.clientCertificatePath = original.clientCertificatePath; @@ -846,7 +846,8 @@ public Builder withDisableCertificateVerification( /** * Sets the hostname to use for TLS certificate verification and SNI instead of the host this - * handler connects to. It is also sent as the gRPC authority ({@code :authority} header). + * handler connects to. It is also sent as the gRPC authority ({@code :authority} header), + * replacing both the host and the port, so it may include a port to keep one in the authority. * Ignored when encryption is disabled. * * @param hostnameOverride the hostname to verify the server certificate against, or null or diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/ArrowFlightConnectionConfigImpl.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/ArrowFlightConnectionConfigImpl.java index 929d6241f8..32c18a5608 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/ArrowFlightConnectionConfigImpl.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/ArrowFlightConnectionConfigImpl.java @@ -115,10 +115,15 @@ public String getTlsRootCertificatesPath() { /** * Gets the hostname to use for TLS certificate verification and SNI instead of the host. * - * @return the hostname override, or null if not set. + * @return the hostname override, or null if not set or blank. */ - public String getHostnameOverride() { - return ArrowFlightConnectionProperty.HOSTNAME_OVERRIDE.getString(properties); + public @Nullable String getHostnameOverride() { + final String hostnameOverride = + ArrowFlightConnectionProperty.HOSTNAME_OVERRIDE.getString(properties); + if (hostnameOverride == null || hostnameOverride.trim().isEmpty()) { + return null; + } + return hostnameOverride.trim(); } public String getClientCertificatePath() { diff --git a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ConnectionTlsHostnameOverrideTest.java b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ConnectionTlsHostnameOverrideTest.java index 67bfcd186f..47de7d4c07 100644 --- a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ConnectionTlsHostnameOverrideTest.java +++ b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ConnectionTlsHostnameOverrideTest.java @@ -22,6 +22,11 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; +import io.grpc.Metadata; +import io.grpc.ServerCall; +import io.grpc.ServerCallHandler; +import io.grpc.ServerInterceptor; +import io.grpc.netty.NettyServerBuilder; import java.net.URLEncoder; import java.nio.charset.StandardCharsets; import java.security.cert.CertificateException; @@ -35,6 +40,8 @@ import java.util.Collections; import java.util.Deque; import java.util.Properties; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; import org.apache.arrow.driver.jdbc.authentication.UserPasswordAuthentication; import org.apache.arrow.driver.jdbc.utils.ArrowFlightConnectionConfigImpl.ArrowFlightConnectionProperty; import org.apache.arrow.driver.jdbc.utils.FlightSqlTestCertificates; @@ -76,6 +83,8 @@ public class ConnectionTlsHostnameOverrideTest { FlightSqlTestCertificates.exampleTlsCerts().get(0); private static final String tlsRootCertsPath = certKey.cert.getPath(); + private static final AtomicReference tlsServerAuthority = new AtomicReference<>(); + private static BufferAllocator allocator; private static FlightServer tlsServer; @@ -84,13 +93,7 @@ public static void setUp() throws Exception { DriverManager.registerDriver(new ArrowFlightJdbcDriver()); allocator = new RootAllocator(Long.MAX_VALUE); tlsServer = - FlightServer.builder( - allocator, Location.forGrpcTls(IP_ADDRESS, 0), new MockFlightSqlProducer()) - .headerAuthenticator( - new UserPasswordAuthentication.Builder() - .user(USER, PASSWORD) - .build() - .authenticate()) + serverBuilder(Location.forGrpcTls(IP_ADDRESS, 0), tlsServerAuthority) .useTls(certKey.cert, certKey.key) .build() .start(); @@ -102,6 +105,32 @@ public static void tearDown() throws Exception { allocator.close(); } + /** + * Creates a server that requires {@link #USER} and {@link #PASSWORD} and records the {@code + * :authority} of each call it receives in {@code authority}. + */ + private static FlightServer.Builder serverBuilder( + final Location location, final AtomicReference authority) { + return FlightServer.builder(allocator, location, new MockFlightSqlProducer()) + .headerAuthenticator( + new UserPasswordAuthentication.Builder().user(USER, PASSWORD).build().authenticate()) + .transportHint( + "grpc.builderConsumer", + (Consumer) + builder -> + builder.intercept( + new ServerInterceptor() { + @Override + public ServerCall.Listener interceptCall( + final ServerCall call, + final Metadata headers, + final ServerCallHandler next) { + authority.set(call.getAuthority()); + return next.startCall(call, headers); + } + })); + } + private static String tlsUrl(final String host, final int port) throws Exception { return String.format( "jdbc:arrow-flight-sql://%s:%d?useEncryption=true&useSystemTrustStore=false&%s=%s", @@ -184,19 +213,39 @@ public void testBlankHostnameOverrideIsIgnored() throws Exception { assertCertificateRejected(serverUrl(IP_ADDRESS) + "&hostnameOverride="); } + @Test + public void testHostnameOverrideIsSentAsAuthority() throws Exception { + // The override replaces the whole authority, so the port is only sent if the override has one. + tlsServerAuthority.set(null); + try (Connection connection = + DriverManager.getConnection( + serverUrl(IP_ADDRESS) + "&hostnameOverride=" + CERTIFICATE_HOSTNAME)) { + assertEquals(CERTIFICATE_HOSTNAME, tlsServerAuthority.get()); + } + + // With a port, only the hostname is used to verify the server certificate. + final String overrideWithPort = CERTIFICATE_HOSTNAME + ":" + tlsServer.getPort(); + tlsServerAuthority.set(null); + try (Connection connection = + DriverManager.getConnection( + serverUrl(IP_ADDRESS) + "&hostnameOverride=" + overrideWithPort)) { + assertEquals(overrideWithPort, tlsServerAuthority.get()); + } + } + @Test public void testHostnameOverrideIsIgnoredWithoutEncryption() throws Exception { + final AtomicReference authority = new AtomicReference<>(); try (FlightServer server = - FlightServer.builder( - allocator, Location.forGrpcInsecure(IP_ADDRESS, 0), new MockFlightSqlProducer()) - .build() - .start(); + serverBuilder(Location.forGrpcInsecure(IP_ADDRESS, 0), authority).build().start(); Connection connection = DriverManager.getConnection( String.format( - "jdbc:arrow-flight-sql://%s:%d?useEncryption=false&hostnameOverride=%s", - IP_ADDRESS, server.getPort(), "other.example.com"))) { - assertTrue(connection.isValid(0)); + "jdbc:arrow-flight-sql://%s:%d?useEncryption=false&user=%s&password=%s" + + "&hostnameOverride=%s", + IP_ADDRESS, server.getPort(), USER, PASSWORD, "other.example.com"))) { + // The connection's own host and port are sent instead of the override. + assertEquals(IP_ADDRESS + ":" + server.getPort(), authority.get()); } } diff --git a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandlerBuilderTest.java b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandlerBuilderTest.java index 9de00009a8..d7fc765d64 100644 --- a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandlerBuilderTest.java +++ b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandlerBuilderTest.java @@ -155,14 +155,16 @@ public void testDefaults() { } @Test - public void testHostnameOverrideIsCopied() { + public void testHostnameOverrideIsNotCopied() { final ArrowFlightSqlClientHandler.Builder rootBuilder = new ArrowFlightSqlClientHandler.Builder().withHostnameOverride("flight.example.com"); final ArrowFlightSqlClientHandler.Builder copiedBuilder = new ArrowFlightSqlClientHandler.Builder(rootBuilder); - assertEquals("flight.example.com", copiedBuilder.hostnameOverride); + // The override is only valid for the original host and port, so a copy must not carry it to + // another location by default. + assertNull(copiedBuilder.hostnameOverride); } @Test diff --git a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/ArrowFlightConnectionConfigImplTest.java b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/ArrowFlightConnectionConfigImplTest.java index 13b98b3d23..216bfa3c06 100644 --- a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/ArrowFlightConnectionConfigImplTest.java +++ b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/ArrowFlightConnectionConfigImplTest.java @@ -28,6 +28,7 @@ import static org.apache.arrow.driver.jdbc.utils.ArrowFlightConnectionConfigImpl.ArrowFlightConnectionProperty.USE_CLIENT_CACHE; import static org.apache.arrow.driver.jdbc.utils.ArrowFlightConnectionConfigImpl.ArrowFlightConnectionProperty.USE_ENCRYPTION; import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.nullValue; import static org.hamcrest.MatcherAssert.assertThat; import java.time.Duration; @@ -37,6 +38,7 @@ import java.util.stream.Stream; import org.apache.arrow.driver.jdbc.utils.ArrowFlightConnectionConfigImpl.ArrowFlightConnectionProperty; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -89,6 +91,19 @@ public void testUseEncryptionRejectsFalsyValues(String value) { assertThat(arrowFlightConnectionConfig.useEncryption(), is(false)); } + @ParameterizedTest + @ValueSource(strings = {"", " "}) + public void testBlankHostnameOverrideIsNull(String value) { + properties.put(HOSTNAME_OVERRIDE.camelName(), value); + assertThat(arrowFlightConnectionConfig.getHostnameOverride(), is(nullValue())); + } + + @Test + public void testHostnameOverrideIsTrimmed() { + properties.put(HOSTNAME_OVERRIDE.camelName(), " flight.example.com "); + assertThat(arrowFlightConnectionConfig.getHostnameOverride(), is("flight.example.com")); + } + public static Stream provideParameters() { int port = RANDOM.nextInt(Short.toUnsignedInt(Short.MAX_VALUE)); boolean useEncryption = RANDOM.nextBoolean();