Repository navigation
GH-1319: [JDBC] Add hostnameOverride connection property for TLS certificate verification and SNI - #1363
GH-1319: [JDBC] Add hostnameOverride connection property for TLS certificate verification and SNI#1363fredjoonpark wants to merge 3 commits into
Conversation
|
@lidavidm @laurentgo @wgtmac @jbonofre any chance we can get this reviewed? |
|
@fredjoonpark I'm resuming the review on this one. |
| if (!useEncryption | ||
| || hostnameOverride == null | ||
| || endpointHost == null | ||
| || !endpointHost.equalsIgnoreCase(host)) { |
There was a problem hiding this 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?
| || !endpointHost.equalsIgnoreCase(host)) { | |
| || 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.
There was a problem hiding this comment.
OK we can go with your suggestion. I'd originally left the port out for servers that hand out endpoints on other ports of the same host, but we can add this as an opt-in flag later if anyone asks for it.
The builder test now expects no override on a different port, and the endpoint integration test is split- an endpoint at the connection's host and port uses the override, and one on a different port fails verification.
| */ | ||
| @VisibleForTesting | ||
| @Nullable String getHostnameOverrideForEndpoint(final URI endpointUri) { | ||
| final String endpointHost = endpointUri.getHost(); |
There was a problem hiding this 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:
private static String stripBrackets(final String host) {
return host != null && host.startsWith("[") && host.endsWith("]")
? host.substring(1, host.length() - 1)
: host;
}There was a problem hiding this comment.
Yes good catch. Both sides now go through stripBrackets before the comparison, and I added a test for it.
| 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 |
There was a problem hiding this 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".
|
I will rebase, as the CI has been fixed on |
…S certificate verification and SNI
Normalize hostnameOverride in the builder so an empty or blank value (e.g. from a templated URL) falls back to the connection host instead of failing with an invalid authority. Also skip the override for endpoints when encryption is disabled, and document that it is sent as the gRPC :authority.
c707cd6 to
7ee704d
Compare
Only apply hostnameOverride to endpoint locations on the same host and port as the connection, so endpoints on other ports are verified against their own hostname. Ignore IPv6 brackets when comparing hosts, since a host set through properties may not have them.
What's Changed
Adds an optional
hostnameOverrideconnection property to the Flight SQL JDBC driver. When TLS is enabled, the driver verifies the server certificate against this hostname and sends it for SNI and as the gRPC:authority, instead of using the host in the JDBC URL. This lets the driver connect with full certificate verification through SSH tunnels, port-forwarding, private endpoints, or IP addresses that don't match the server certificate. Today the only option in those cases isdisableCertificateVerification=true.hostnameOverride(string, default unset) inArrowFlightConnectionProperty, passed fromArrowFlightConnectiontoArrowFlightSqlClientHandler.Builder.Builder.build()callsNettyClientBuilder.overrideHostname(...)when encryption is on. The property is ignored whenuseEncryption=false, and a blank value is treated as unset.getStreams(): the override is applied only when the endpoint host and port match the original connection (host compared case-insensitively, IPv6 brackets ignored). Endpoints at any other location, or any endpoint when the original connection isn't encrypted, are verified against their own hostname, as they are today. This logic is inBuilder.getHostnameOverrideForEndpoint(URI)so it can be unit-tested.docs/source/flight_sql_jdbc_driver.rstnext totlsRootCerts.Example:
Tests
ConnectionTlsHostnameOverrideTest. It uses the existing test certificate, which is issued forlocalhostand has no IP SAN. Its servers bind to127.0.0.1and the tests connect there, so they don't depend on howlocalhostresolves. Failure cases assert that the cause is aCertificateException, not just anySQLException:No subject alternative names present);hostnameOverride=localhost, set in the URL or inProperties, it succeeds with full verification;hostnameOverride=behaves as unset;ArrowFlightSqlClientHandlerBuilderTest: the default is unset and the builder copy keeps the value. The endpoint rule is covered for same host and port, different port, no port, different case, IPv6 with and without brackets, different host, unencrypted connection, blank value, and no override set.ArrowFlightConnectionConfigImplTest: the property is parsed.This change is additive and has no breaking changes.
Closes #1319.