Repository navigation
SNI and TLS hostname verification support - #1638
Conversation
…stname resolution mode Since 2.16.9 the client resolves a hostname to ip addresses before connecting and hands the ip to the SSL socket factory, so no SNI is sent. An endpoint that selects its certificate by SNI, like a TLS terminating proxy or ingress, then serves its default certificate. Before 2.16.9 the hostname reached the TLS layer because the JDK resolved inside InetSocketAddress and the host string was kept. - DataPort: a default connect overload that also receives the uri before resolution. Existing implementations are unchanged. - NatsConnection: tryToConnect passes the unresolved uri to the data port. - SocketDataPort: keeps the hostname as tlsHost, uses it as the peer host and sets it explicitly with SSLParameters.setServerNames before the handshake, in every mode. The explicit call also covers a single label hostname, such as a kubernetes service name, which the JSSE does not derive from the peer host on its own. - Tests: SniTestServer, a loopback server that selects its certificate by SNI, adapted from PR #1637, and TlsHostnameTests covering all six HostnameResolveMode values with TLS after INFO and TLS first, a single label hostname, the explicit ip control case and the data port hand-off. Unresolved runs through a loopback HTTP CONNECT tunnel, the only way that mode connects. Not covered: a bare ip learned from connect_urls still has no hostname to send.
|
Claude finished @scottf's task in 1m 44s —— View job Review of PR #1638
SummaryThis is a small, well-targeted fix: Correctness checks performed:
No bugs, security issues, or thread-safety concerns found in the production code changes ( |
…eHostnames maps to it Unresolved built the socket address with InetSocketAddress.createUnresolved in every case. A plain socket rejects an unresolved address with UnknownHostException, so the mode only connected through a proxy. Now createUnresolved is used only when a proxy is configured, which is what it is for; without one, a single address is resolved at connect time through NatsInetAddress, as the client did before hostname resolution was added in 2.16.9. The hostname is kept as the TLS server name in both cases. The deprecated noResolveHostnames() builder method, the noResolveHostnames property and isNoResolveHostnames() mapped to ResolveToFirst since 2.25.2, which resolves in the client. From 2.16.9 through 2.25.1 they meant that the client does not resolve. They now map to Unresolved, which restores that meaning. Tests: the TlsHostnameTests matrix runs Unresolved on a direct connection in addition to the proxied run, and every execution asserts whether the server pool was asked to resolve, which only the resolving modes do. OptionsTests expectations follow the mapping.
…erifyHostname Off by default. When on, SocketDataPort sets the HTTPS endpoint identification algorithm on the SSL socket before the handshake, so the trust manager checks that the certificate the server presents is issued for the server name: the configured hostname in every HostnameResolveMode, since the hostname is now the SNI server name in every mode, or the ip address when the server was configured or discovered by ip address. The check is made by the trust manager in use. The JDK performs it for its own trust managers and for any plain X509TrustManager it wraps, which includes the trust-all manager of opentls; a custom X509ExtendedTrustManager is responsible for its own identity check. Builder method tlsVerifyHostname(), property io.nats.client.tls.verify.hostname, getter isTlsVerifyHostname(), copied by Builder(Options). Tests: the generated server certificates now carry a DNS subject alternative name, with an optional ip one, and SniTestServer can serve its valid certificate whatever name was requested. TlsHostnameTests: a matching certificate is accepted in the default mode, a valid certificate for another name is rejected with the identity error and accepted when the option is off, an ip literal is checked against ip subject alternative names, and the check applies under the trust-all context. OptionsTests covers the option.
… server that supplied it A server learned from connect_urls as a bare ip address has no hostname to present as the TLS server name, and nothing to verify its certificate against but the ip. As nats.go has done since 2018 with tlsName, it now carries the hostname of the server whose INFO supplied it, one hop only: an origin that is itself an ip address supplies nothing. - ServerPool: a default overload acceptDiscoveredUrls(discoveredServers, origin) that forwards to the existing method, so custom pools are unaffected. - NatsConnection: handleInfo passes the origin, currentServer or, while the initial INFO is being read, the server being connected to. - NatsServerPool: a newly discovered entry whose host is an ip address gets the origin's hostname when the origin was configured with one. Explicit servers and already known entries are untouched. - NatsUri: tlsHost with getTlsHost() and withTlsHost(), carried through reHost. Equality, hash code and the string form are unchanged. Taken from PR #1637. - SocketDataPort: the unresolved uri's tls host is the server name to present. Tests: reconnect to a gossiped bare ip presents the first server's hostname, TLS after INFO and TLS first; the same reconnect with tlsVerifyHostname on, against a certificate with a DNS name and no ip subject alternative name, passes because the check is against that hostname; NatsUri identity is unchanged by the name; the pool saves the origin's hostname per entry, honors ignoreDiscoveredServers, does not override explicit servers, saves nothing from an ip origin or through the overload without an origin.
scottf
changed the title
|
@claude re-review there have been many changes since your first review |
|
Claude finished @scottf's task in 3m 21s —— View job Re-review of PR #1638
SummaryReviewed the full diff against
Tests — No bugs, thread-safety issues, or correctness gaps found. Nothing further to request. |
|
Why hostname verification is opt-in in this PR Turning it on changes which connections succeed, and the Java client has never checked, so an existing deployment could break on upgrade without changing a line of its own configuration. The connections that would start failing with it on by default:
Every other client verifies by default, and that is the right end state. They have done so from the start, so their users have always had matching certificates. Java users have not had to, so some certificates in the field are valid only because nothing checks them. Flipping the default in a 2.x release would surface those as connection failures with no code change on the user's side. Opt-in gets the capability into 2.x now, lets the deployments that want it turn it on, and leaves the default flip for V3, where a behavior change is expected. The same reasoning does not apply to SNI itself, which this PR does turn on by default: sending a name the server can ignore does not fail a connection that worked before, except at an endpoint that rejects unrecognized names. |
2 participants
Since 2.16.9 the client resolves hostnames before connecting and hands the ip to the SSL socket factory, so no SNI is sent and an endpoint that selects its certificate by SNI serves its default certificate.
Features
HostnameResolveMode.NatsConnectionpasses the uri before resolution to the data port through a newDataPort.connect(conn, uri, unresolvedUri, timeoutNanos)default overload;SocketDataPortkeeps it astlsHost, uses it as the peer host and sets it withSSLParameters.setServerNamesbefore the handshake.nats, get SNI too. The JSSE does not derive SNI from a single label host argument; setting the name explicitly covers it.ServerPool.acceptDiscoveredUrls(discoveredServers, origin)default overload;NatsServerPoolstamps the origin's hostname on the new entry, one hop, without overriding explicit or already known servers;NatsUri.tlsHostwithgetTlsHost()andwithTlsHost()carries it throughreHost, taken from Preserve TLS hostname through DNS resolution and server discovery #1637; equality, hash code and string form unchanged.Options.Builder.tlsVerifyHostname(), propertyio.nats.client.tls.verify.hostname,isTlsVerifyHostname(), off by default. Sets theHTTPSendpoint identification algorithm on the socket; the trust manager checks the certificate against the server name, the gossiping server's name for a discovered bare ip, or the ip for an ip literal. Applies under the trust-all context, because the JDK wraps a plainX509TrustManagerand checks in the wrapper; a customX509ExtendedTrustManageris responsible for its own check.ServerPoolandDataPortimplementations are unaffected; both new methods are default methods that forward to the existing ones.Bug fixes
HostnameResolveMode.Unresolvedcould not connect without a proxy: it always usedInetSocketAddress.createUnresolved, which a plain socket rejects withUnknownHostException. NowcreateUnresolvedonly when a proxy is configured; otherwise one address is resolved at connect time throughNatsInetAddress, as the client did before 2.16.9, with the hostname kept for TLS.noResolveHostnames(), thenoResolveHostnamesproperty andisNoResolveHostnames()mapped toResolveToFirstsince 2.25.2, which resolves in the client. They now map toUnresolved, their meaning from 2.16.9 through 2.25.1.Tests
SniTestServer: a loopback NATS server that selects its certificate by SNI, with an expired default certificate; adapted from Preserve TLS hostname through DNS resolution and server discovery #1637 with a configurable certificate host, a DNS subject alternative name plus an optional ip one, and a mode that serves the valid certificate whatever name was requested.ExpiringClientCertUtilgains a subject alternative name overload.hostnameIsSentAsSniInEveryMode: all six modes, TLS after INFO and TLS first, plusUnresolvedthrough a loopback HTTP CONNECT tunnel, 14 executions; each asserts the pool was asked to resolve only in the resolving modes, and the tunneled runs assert the proxy received the hostname, not an ip.singleLabelHostnameIsSentAsSniInEveryMode: the same 14 with the hostnats.explicitIpSendsNoSniAndGetsTheDefaultCertificate: the control case, 2 executions; no SNI, expired certificate,CertificateExpiredException.dataPortIsGivenTheUnresolvedUri: the data port receives the resolved ip and the unresolved hostname and uses the hostname.dataPortWithoutTheUnresolvedUriStillSendsAnUnresolvedHostname: a data port implementing only the olderconnectstill sends the name in a non resolving mode.verifyHostnameAcceptsACertificateForTheHostname: default mode, resolved to 127.0.0.1, certificate for the hostname with no ip SAN, accepted, 2 executions.verifyHostnameRejectsACertificateForAnotherName: a valid, trusted certificate forother.testis rejected with the identity error, and accepted by the same server with the option off.verifyHostnameChecksAnIpLiteralAgainstIpSubjectAlternativeNames:tls://127.0.0.1accepted with an ip SAN and rejected without one.verifyHostnameAppliesUnderTheTrustAllContext: theother.testcertificate is rejected underSSLUtils.createTrustAllTlsContext().discoveredIpRetainsHostnameOnReconnect: the first server gossips the second as a bare ip; after the first closes, the reconnect presents the first server's hostname and gets the valid certificate, 2 executions.discoveredIpIsVerifiedByTheGossipingServersName: the same reconnect with verification on, against a certificate with a DNS name and no ip SAN.tlsHostnameSurvivesResolutionWithoutChangingUriIdentity:reHostto IPv4 and IPv6 keeps the name; equality, hash code and string form unchanged;withTlsHostlikewise.discoverySavesTheOriginHostnameAndHonorsIgnoreDiscoveredServers: the origin's hostname is saved on a discovered bare ip,ignoreDiscoveredServersis honored, and the overload without an origin saves nothing.discoveryKeepsTlsNamesPerEntryAndDoesNotOverrideExplicitServers: names are per entry, explicit servers and already known entries are not overridden, and an ip origin or a discovered ip carrying a name supplies nothing.OptionsTests.testTlsVerifyHostname: default, builder, copy, property true and false.OptionsTests: thenoResolveHostnamesbuilder method and property now expectUnresolved, andResolveToFirstno longer reportsisNoResolveHostnames.TlsHostnameTests43 of 43 andOptionsTests70 of 70 pass, along withTLSConnectTests,ServerPoolTests,ConnectTests,ReconnectTests,WebsocketConnectTests,MaxReconnectResolvedIpsTest,NatsProvidersAndImplementationsTestsandSocketDataPortProxyHostnameTest.