jbonofre commented on code in PR #1363:
URL: https://github.com/apache/arrow-java/pull/1363#discussion_r4420614808
##########
flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java:
##########
@@ -840,6 +844,42 @@ public Builder withDisableCertificateVerification(
return this;
}
+ /**
+ * 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).
+ * Ignored when encryption is disabled.
+ *
+ * @param hostnameOverride the hostname to verify the server certificate
against, or null or
+ * blank to use the host.
+ * @return this instance.
+ */
+ public Builder withHostnameOverride(@Nullable final String
hostnameOverride) {
+ final String trimmed = hostnameOverride == null ? null :
hostnameOverride.trim();
+ this.hostnameOverride = trimmed == null || trimmed.isEmpty() ? null :
trimmed;
+ return this;
+ }
+
+ /**
+ * Gets the hostname override to use when connecting to an endpoint
location. The override is
+ * only valid for the host it was configured for, so endpoints on any
other host are verified
+ * against their own hostname. Like the override itself, it is ignored
when the original
+ * connection does not use encryption.
+ *
+ * @param endpointUri the endpoint location.
+ * @return the hostname override if the endpoint is on the original host,
otherwise null.
+ */
+ @VisibleForTesting
+ @Nullable String getHostnameOverrideForEndpoint(final URI endpointUri) {
+ final String endpointHost = endpointUri.getHost();
Review Comment:
`URI#getHost` keeps the brackets on IPv6 literals, but `host` doesn't always
have them. JDBC URLs are fine (also parsed with `URI#getHost`), but when the
host comes from `Properties` or the DataSource as `::1`, the root connection
works (`Location.forGrpcTls` adds the brackets) while
`"[::1]".equalsIgnoreCase("::1")` is false.
Endpoints on the same host then silently skip the override and fail
verification.
Could we strip the brackets on both sides before comparing (and add a test
with `withHost("::1")`? Something like:
```java
private static String stripBrackets(final String host) {
return host != null && host.startsWith("[") && host.endsWith("]")
? host.substring(1, host.length() - 1)
: host;
}
```
##########
docs/source/flight_sql_jdbc_driver.rst:
##########
@@ -118,6 +118,17 @@ case-sensitive. The supported parameters are:
- Path to PEM-encoded root certificates for TLS - use this as
an alternative to ``trustStore``
+ * - hostnameOverride
+ - null
+ - When TLS is enabled, the hostname to verify the server certificate
+ 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
+ the server are only verified against this hostname if they are on
+ the same host as the connection. Ignored when ``useEncryption`` is
Review Comment:
Whichever way the port question goes (see my comment on
`getHostnameOverrideForEndpoint`), could we make this explicit? Either "on the
same host and port as the connection" or "on the same host as the connection,
on any port".
##########
flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java:
##########
@@ -840,6 +844,42 @@ public Builder withDisableCertificateVerification(
return this;
}
+ /**
+ * 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).
+ * Ignored when encryption is disabled.
+ *
+ * @param hostnameOverride the hostname to verify the server certificate
against, or null or
+ * blank to use the host.
+ * @return this instance.
+ */
+ public Builder withHostnameOverride(@Nullable final String
hostnameOverride) {
+ final String trimmed = hostnameOverride == null ? null :
hostnameOverride.trim();
+ this.hostnameOverride = trimmed == null || trimmed.isEmpty() ? null :
trimmed;
+ return this;
+ }
+
+ /**
+ * Gets the hostname override to use when connecting to an endpoint
location. The override is
+ * only valid for the host it was configured for, so endpoints on any
other host are verified
+ * against their own hostname. Like the override itself, it is ignored
when the original
+ * connection does not use encryption.
+ *
+ * @param endpointUri the endpoint location.
+ * @return the hostname override if the endpoint is on the original host,
otherwise null.
+ */
+ @VisibleForTesting
+ @Nullable String getHostnameOverrideForEndpoint(final URI endpointUri) {
+ final String endpointHost = endpointUri.getHost();
+ if (!useEncryption
+ || hostnameOverride == null
+ || endpointHost == null
+ || !endpointHost.equalsIgnoreCase(host)) {
Review Comment:
This ignores the port (the builder test asserts it), so the override applies
to every TLS endpoint on the connection host. If the server returns an endpoint
on another port of that host that serves a different certificate (e.g.
`grpc+tls://10.0.0.5:9000` with a cert for `10.0.0.5` or its own name). It's
now verified against `hostnameOverride` and fails, and there's no way to opt
out.
Could we match on host and port, so the override only applies to the
location it was configured for?
```suggestion
|| endpointUri.getPort() != port
|| !endpointHost.equalsIgnoreCase(host)) {
```
If covering every port on the host is need, I'm fine keeping it, but I would
love to document it.
--
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]
