Skip to content

Commit 301a67f

Browse files
authored
GH-1442: [JDBC] Harden hostnameOverride handling and tests (#1443)
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.
1 parent 30b00a9 commit 301a67f

6 files changed

Lines changed: 98 additions & 23 deletions

File tree

‎docs/source/flight_sql_jdbc_driver.rst‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,10 @@ case-sensitive. The supported parameters are:
124124
against and to send for SNI and as the gRPC ``:authority`` header,
125125
instead of **HOSTNAME**. Use this when
126126
connecting through a tunnel, a private endpoint, or an IP address
127-
that does not match the server certificate. Endpoints returned by
127+
that does not match the server certificate. The value replaces the
128+
whole ``:authority``, so **PORT** is not sent unless it is included,
129+
for example ``flight.example.com:32010``; only the hostname is used
130+
for SNI and certificate verification. Endpoints returned by
128131
the server are only verified against this hostname if they are on
129132
the same host and port as the connection. Ignored when
130133
``useEncryption`` is false.

‎flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -718,7 +718,8 @@ public static final class Builder {
718718
public Builder() {}
719719

720720
/**
721-
* Copies the builder.
721+
* Copies the builder. The hostname override is not copied, since it is only valid for the
722+
* original host and port.
722723
*
723724
* @param original The builder to base this copy off of.
724725
*/
@@ -735,7 +736,6 @@ public Builder() {}
735736
this.token = original.token;
736737
this.useEncryption = original.useEncryption;
737738
this.disableCertificateVerification = original.disableCertificateVerification;
738-
this.hostnameOverride = original.hostnameOverride;
739739
this.useSystemTrustStore = original.useSystemTrustStore;
740740
this.tlsRootCertificatesPath = original.tlsRootCertificatesPath;
741741
this.clientCertificatePath = original.clientCertificatePath;
@@ -846,7 +846,8 @@ public Builder withDisableCertificateVerification(
846846

847847
/**
848848
* Sets the hostname to use for TLS certificate verification and SNI instead of the host this
849-
* handler connects to. It is also sent as the gRPC authority ({@code :authority} header).
849+
* handler connects to. It is also sent as the gRPC authority ({@code :authority} header),
850+
* replacing both the host and the port, so it may include a port to keep one in the authority.
850851
* Ignored when encryption is disabled.
851852
*
852853
* @param hostnameOverride the hostname to verify the server certificate against, or null or

‎flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/ArrowFlightConnectionConfigImpl.java‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,15 @@ public String getTlsRootCertificatesPath() {
115115
/**
116116
* Gets the hostname to use for TLS certificate verification and SNI instead of the host.
117117
*
118-
* @return the hostname override, or null if not set.
118+
* @return the hostname override, or null if not set or blank.
119119
*/
120-
public String getHostnameOverride() {
121-
return ArrowFlightConnectionProperty.HOSTNAME_OVERRIDE.getString(properties);
120+
public @Nullable String getHostnameOverride() {
121+
final String hostnameOverride =
122+
ArrowFlightConnectionProperty.HOSTNAME_OVERRIDE.getString(properties);
123+
if (hostnameOverride == null || hostnameOverride.trim().isEmpty()) {
124+
return null;
125+
}
126+
return hostnameOverride.trim();
122127
}
123128

124129
public String getClientCertificatePath() {

‎flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ConnectionTlsHostnameOverrideTest.java‎

Lines changed: 63 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,11 @@
2222
import static org.junit.jupiter.api.Assertions.assertTrue;
2323
import static org.junit.jupiter.api.Assertions.fail;
2424

25+
import io.grpc.Metadata;
26+
import io.grpc.ServerCall;
27+
import io.grpc.ServerCallHandler;
28+
import io.grpc.ServerInterceptor;
29+
import io.grpc.netty.NettyServerBuilder;
2530
import java.net.URLEncoder;
2631
import java.nio.charset.StandardCharsets;
2732
import java.security.cert.CertificateException;
@@ -35,6 +40,8 @@
3540
import java.util.Collections;
3641
import java.util.Deque;
3742
import java.util.Properties;
43+
import java.util.concurrent.atomic.AtomicReference;
44+
import java.util.function.Consumer;
3845
import org.apache.arrow.driver.jdbc.authentication.UserPasswordAuthentication;
3946
import org.apache.arrow.driver.jdbc.utils.ArrowFlightConnectionConfigImpl.ArrowFlightConnectionProperty;
4047
import org.apache.arrow.driver.jdbc.utils.FlightSqlTestCertificates;
@@ -76,6 +83,8 @@ public class ConnectionTlsHostnameOverrideTest {
7683
FlightSqlTestCertificates.exampleTlsCerts().get(0);
7784
private static final String tlsRootCertsPath = certKey.cert.getPath();
7885

86+
private static final AtomicReference<String> tlsServerAuthority = new AtomicReference<>();
87+
7988
private static BufferAllocator allocator;
8089
private static FlightServer tlsServer;
8190

@@ -84,13 +93,7 @@ public static void setUp() throws Exception {
8493
DriverManager.registerDriver(new ArrowFlightJdbcDriver());
8594
allocator = new RootAllocator(Long.MAX_VALUE);
8695
tlsServer =
87-
FlightServer.builder(
88-
allocator, Location.forGrpcTls(IP_ADDRESS, 0), new MockFlightSqlProducer())
89-
.headerAuthenticator(
90-
new UserPasswordAuthentication.Builder()
91-
.user(USER, PASSWORD)
92-
.build()
93-
.authenticate())
96+
serverBuilder(Location.forGrpcTls(IP_ADDRESS, 0), tlsServerAuthority)
9497
.useTls(certKey.cert, certKey.key)
9598
.build()
9699
.start();
@@ -102,6 +105,32 @@ public static void tearDown() throws Exception {
102105
allocator.close();
103106
}
104107

108+
/**
109+
* Creates a server that requires {@link #USER} and {@link #PASSWORD} and records the {@code
110+
* :authority} of each call it receives in {@code authority}.
111+
*/
112+
private static FlightServer.Builder serverBuilder(
113+
final Location location, final AtomicReference<String> authority) {
114+
return FlightServer.builder(allocator, location, new MockFlightSqlProducer())
115+
.headerAuthenticator(
116+
new UserPasswordAuthentication.Builder().user(USER, PASSWORD).build().authenticate())
117+
.transportHint(
118+
"grpc.builderConsumer",
119+
(Consumer<NettyServerBuilder>)
120+
builder ->
121+
builder.intercept(
122+
new ServerInterceptor() {
123+
@Override
124+
public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(
125+
final ServerCall<ReqT, RespT> call,
126+
final Metadata headers,
127+
final ServerCallHandler<ReqT, RespT> next) {
128+
authority.set(call.getAuthority());
129+
return next.startCall(call, headers);
130+
}
131+
}));
132+
}
133+
105134
private static String tlsUrl(final String host, final int port) throws Exception {
106135
return String.format(
107136
"jdbc:arrow-flight-sql://%s:%d?useEncryption=true&useSystemTrustStore=false&%s=%s",
@@ -184,19 +213,39 @@ public void testBlankHostnameOverrideIsIgnored() throws Exception {
184213
assertCertificateRejected(serverUrl(IP_ADDRESS) + "&hostnameOverride=");
185214
}
186215

216+
@Test
217+
public void testHostnameOverrideIsSentAsAuthority() throws Exception {
218+
// The override replaces the whole authority, so the port is only sent if the override has one.
219+
tlsServerAuthority.set(null);
220+
try (Connection connection =
221+
DriverManager.getConnection(
222+
serverUrl(IP_ADDRESS) + "&hostnameOverride=" + CERTIFICATE_HOSTNAME)) {
223+
assertEquals(CERTIFICATE_HOSTNAME, tlsServerAuthority.get());
224+
}
225+
226+
// With a port, only the hostname is used to verify the server certificate.
227+
final String overrideWithPort = CERTIFICATE_HOSTNAME + ":" + tlsServer.getPort();
228+
tlsServerAuthority.set(null);
229+
try (Connection connection =
230+
DriverManager.getConnection(
231+
serverUrl(IP_ADDRESS) + "&hostnameOverride=" + overrideWithPort)) {
232+
assertEquals(overrideWithPort, tlsServerAuthority.get());
233+
}
234+
}
235+
187236
@Test
188237
public void testHostnameOverrideIsIgnoredWithoutEncryption() throws Exception {
238+
final AtomicReference<String> authority = new AtomicReference<>();
189239
try (FlightServer server =
190-
FlightServer.builder(
191-
allocator, Location.forGrpcInsecure(IP_ADDRESS, 0), new MockFlightSqlProducer())
192-
.build()
193-
.start();
240+
serverBuilder(Location.forGrpcInsecure(IP_ADDRESS, 0), authority).build().start();
194241
Connection connection =
195242
DriverManager.getConnection(
196243
String.format(
197-
"jdbc:arrow-flight-sql://%s:%d?useEncryption=false&hostnameOverride=%s",
198-
IP_ADDRESS, server.getPort(), "other.example.com"))) {
199-
assertTrue(connection.isValid(0));
244+
"jdbc:arrow-flight-sql://%s:%d?useEncryption=false&user=%s&password=%s"
245+
+ "&hostnameOverride=%s",
246+
IP_ADDRESS, server.getPort(), USER, PASSWORD, "other.example.com"))) {
247+
// The connection's own host and port are sent instead of the override.
248+
assertEquals(IP_ADDRESS + ":" + server.getPort(), authority.get());
200249
}
201250
}
202251

‎flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandlerBuilderTest.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -155,14 +155,16 @@ public void testDefaults() {
155155
}
156156

157157
@Test
158-
public void testHostnameOverrideIsCopied() {
158+
public void testHostnameOverrideIsNotCopied() {
159159
final ArrowFlightSqlClientHandler.Builder rootBuilder =
160160
new ArrowFlightSqlClientHandler.Builder().withHostnameOverride("flight.example.com");
161161

162162
final ArrowFlightSqlClientHandler.Builder copiedBuilder =
163163
new ArrowFlightSqlClientHandler.Builder(rootBuilder);
164164

165-
assertEquals("flight.example.com", copiedBuilder.hostnameOverride);
165+
// The override is only valid for the original host and port, so a copy must not carry it to
166+
// another location by default.
167+
assertNull(copiedBuilder.hostnameOverride);
166168
}
167169

168170
@Test

‎flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/ArrowFlightConnectionConfigImplTest.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import static org.apache.arrow.driver.jdbc.utils.ArrowFlightConnectionConfigImpl.ArrowFlightConnectionProperty.USE_CLIENT_CACHE;
2929
import static org.apache.arrow.driver.jdbc.utils.ArrowFlightConnectionConfigImpl.ArrowFlightConnectionProperty.USE_ENCRYPTION;
3030
import static org.hamcrest.CoreMatchers.is;
31+
import static org.hamcrest.CoreMatchers.nullValue;
3132
import static org.hamcrest.MatcherAssert.assertThat;
3233

3334
import java.time.Duration;
@@ -37,6 +38,7 @@
3738
import java.util.stream.Stream;
3839
import org.apache.arrow.driver.jdbc.utils.ArrowFlightConnectionConfigImpl.ArrowFlightConnectionProperty;
3940
import org.junit.jupiter.api.BeforeEach;
41+
import org.junit.jupiter.api.Test;
4042
import org.junit.jupiter.params.ParameterizedTest;
4143
import org.junit.jupiter.params.provider.Arguments;
4244
import org.junit.jupiter.params.provider.MethodSource;
@@ -89,6 +91,19 @@ public void testUseEncryptionRejectsFalsyValues(String value) {
8991
assertThat(arrowFlightConnectionConfig.useEncryption(), is(false));
9092
}
9193

94+
@ParameterizedTest
95+
@ValueSource(strings = {"", " "})
96+
public void testBlankHostnameOverrideIsNull(String value) {
97+
properties.put(HOSTNAME_OVERRIDE.camelName(), value);
98+
assertThat(arrowFlightConnectionConfig.getHostnameOverride(), is(nullValue()));
99+
}
100+
101+
@Test
102+
public void testHostnameOverrideIsTrimmed() {
103+
properties.put(HOSTNAME_OVERRIDE.camelName(), " flight.example.com ");
104+
assertThat(arrowFlightConnectionConfig.getHostnameOverride(), is("flight.example.com"));
105+
}
106+
92107
public static Stream<Arguments> provideParameters() {
93108
int port = RANDOM.nextInt(Short.toUnsignedInt(Short.MAX_VALUE));
94109
boolean useEncryption = RANDOM.nextBoolean();

0 commit comments

Comments
 (0)