diff --git a/README.md b/README.md index 87d1cc2fd..41fb4edb3 100644 --- a/README.md +++ b/README.md @@ -1144,21 +1144,20 @@ However, the java client also needs a keystore and truststore.jks files for crea ### TLS client versus server checks When creating a connection, client TLS behavior is set while creating options. -The client assumes TLS is requested if there is an SSLContext instance in the options. -There are two ways one exists: -1. The user directly supplied one -2. A default one was created since one was not supplied, but a supplied server url has a secure protocol such as `tls`, `wss` or `opentls` +The client assumes TLS is requested if there is an SSLContext instance in the options. There are two ways one exists: +1. The user directly supplied one, or supplied the keystore and truststore it is built from +2. A default one was created because `secure()` or `tlsFirst()` was called, or because a supplied server url has a secure protocol such as `tls`, `wss` or `opentls` -If there is a mismatch, an IOException will be thrown while connecting. +If there is a mismatch, an IOException will be thrown while connecting and the next server is tried, since a single server can be misconfigured. The options decide for every server of a connection, so when every server of one round over the server list fails with the mismatch, retrying cannot help: the connection is closed without reconnecting, and the initial connect throws. | server config | client options | result | |---------------|-------------------|----------------------------------------------| -| required | tls not requested | mismatch, "SSL required by server." | -| available | tls not requested | ok | -| neither | tls not requested | ok | +| required | tls not requested | mismatch, "SSL required by server." | +| available | tls not requested | ok | +| neither | tls not requested | ok | | required | tls requested | ok | | available | tls requested | ok | -| neither | tls requested | mismatch, "SSL connection wanted by client." | +| neither | tls requested | mismatch, "SSL connection wanted by client." | ### TLS Handshake First diff --git a/docs/TLS_CONFIGURATION.md b/docs/TLS_CONFIGURATION.md index d81d14f39..877b9c117 100644 --- a/docs/TLS_CONFIGURATION.md +++ b/docs/TLS_CONFIGURATION.md @@ -12,8 +12,8 @@ The context is accepted or created once, when `Options.Builder.build()` runs, in 2. `sslContextFactory(SSLContextFactory)` was set, or the `io.nats.client.ssl.context.factory.class` property named a class: the factory builds the context from the keystore, truststore and algorithm properties. 3. `keystore` or `truststore` was set, by builder or property: the client builds a context from those JKS files with the `tls.algorithm` property, `SunX509` by default. 4. `opentls()` was called, the `io.nats.client.opentls` property is true, or any bootstrap server has the `opentls://` scheme: the trust-all context described in section 3. -5. `secure()` was called, the `io.nats.client.secure` property is true, or any bootstrap server has the `tls://` or `wss://` scheme: `SSLContext.getDefault()`, described in section 2. -6. Otherwise no context, and the connection is plain TCP. A server that requires TLS then fails the connect with "SSL required by server." +5. `secure()` or `tlsFirst()` was called, the `io.nats.client.secure` property is true, or any bootstrap server has the `tls://` or `wss://` scheme: `SSLContext.getDefault()`, described in section 2. +6. Otherwise no context, and the connection is plain TCP. A server that requires TLS then fails the attempt with "SSL required by server."; section 8 says when that closes the connection. Rules 4 and 5 look at the bootstrap servers only when neither `opentls` nor `secure` was set explicitly. Asking for both is an error, section 5. @@ -87,8 +87,8 @@ The README's TLS Certs section shows how the test keystore and truststore are pr ## 7. TLS handshake first -`tlsFirst()` performs the TLS handshake before reading the server's INFO, for servers configured with `handshake_first`. It requires a context: building options with `tlsFirst()` and no context from the rules above fails with "SSL context required for tls handshake first". +`tlsFirst()` performs the TLS handshake before reading the server's INFO, for servers configured with `handshake_first`. It asks for TLS: when rules 1 to 4 of section 1 give no context, it selects `SSLContext.getDefault()`, rule 5. ## 8. What the server and client expect of each other -Whether the client attempts the TLS upgrade is decided by whether it has a context; whether the server requires or offers TLS comes from its INFO. The README's "TLS client versus server checks" table lists the six combinations and the two that fail with an `IOException`. +Whether the client attempts the TLS upgrade is decided by whether it has a context; whether the server requires or offers TLS comes from its INFO. The README's "TLS client versus server checks" table lists the six combinations and the two that fail with an `IOException`. Either mismatch fails the attempt and the next server is tried, since a single server can be misconfigured. When every server of one full round over the server list fails with the mismatch, retrying cannot help, because the same options are used for every server, and the connection is closed: the initial connect throws, also with reconnect-on-connect, and a reconnect ends with the connection closed. A server that cannot be reached is not a mismatch, so a round that contains one is not conclusive and the normal reconnect rules apply. The scheme of a url that a custom `ServerPool` hands out does not change which servers the client uses TLS with; only the options decide. diff --git a/src/main/java/io/nats/client/Options.java b/src/main/java/io/nats/client/Options.java index 721880403..1d0690422 100644 --- a/src/main/java/io/nats/client/Options.java +++ b/src/main/java/io/nats/client/Options.java @@ -2307,6 +2307,9 @@ public Builder ignoreDiscoveredServers() { * the INFO protocol from the server. If this option is enabled * but the server is not configured to perform the TLS handshake * first, the connection will fail. + *

+ * TLS Handshake First asks for TLS. When no SSL context comes from the other + * settings, the default SSL context is used, the same as {@link #secure()}. * @return the Builder for chaining */ public Builder tlsFirst() { @@ -2482,6 +2485,11 @@ public Options build() throws IllegalStateException { } } + // tls first asks for TLS: when nothing else has chosen a context, it uses the default one + if (tlsFirst && !useTrustAllTls) { + useDefaultTls = true; + } + // One SSL context serves every server of a connection, so it cannot both verify // certificates (secure, tls, wss) and trust all of them (opentls). Asking for both, // by the builder, by properties or by a server list that mixes the schemes, is an @@ -2511,6 +2519,7 @@ else if (useDefaultTls) { } if (tlsFirst && sslContext == null) { + // only an SSLContextFactory that returned null gets here, every other path has a context by now throw new IllegalStateException("SSL context required for tls handshake first"); } diff --git a/src/main/java/io/nats/client/impl/NatsConnection.java b/src/main/java/io/nats/client/impl/NatsConnection.java index 1f981f89f..f65db1aef 100644 --- a/src/main/java/io/nats/client/impl/NatsConnection.java +++ b/src/main/java/io/nats/client/impl/NatsConnection.java @@ -63,6 +63,8 @@ class NatsConnection implements Connection { protected CompletableFuture dataPortFuture; protected DataPort dataPort; + // the TLS mismatch the current attempt failed with, null when it did not; see upgradeToSecureIfNeeded + private volatile String attemptTlsMismatch; protected NatsUri currentServer; protected NatsUri connectingServer; protected NatsUri lastServer; @@ -247,6 +249,8 @@ protected void connectImpl(boolean reconnectOnConnect) throws InterruptedExcepti Set failList = new HashSet<>(); boolean keepGoing = true; + String tlsMismatch = null; // the mismatch of the last attempt that failed with one + boolean everyAttemptTlsMismatch = true; // false once an attempt failed for another reason NatsUri first = null; NatsUri cur; while (keepGoing && (cur = serverPool.peekNextServer()) != null) { @@ -266,6 +270,7 @@ else if (cur.equals(first)) { break; } connectError.set(null); // new on each attempt + attemptTlsMismatch = null; timeTraceLogger.trace("setting status to connecting"); updateStatus(Status.CONNECTING, resolved, cur); @@ -287,6 +292,13 @@ else if (cur.equals(first)) { if (this.isAuthenticationError(err)) { this.serverAuthErrors.put(resolved, err); } + + if (attemptTlsMismatch == null) { + everyAttemptTlsMismatch = false; + } + else { + tlsMismatch = attemptTlsMismatch; + } } if (!isConnected() && !isClosed()) { @@ -296,6 +308,13 @@ else if (cur.equals(first)) { } if (!isConnected() && !isClosed()) { + if (tlsMismatch != null && everyAttemptTlsMismatch) { + // One round over the whole server list, and every attempt failed with the TLS mismatch between the + // options and the server. The options are the same for every server, so retrying cannot help. + timeTraceLogger.trace("every server failed with a TLS mismatch, closing"); + close(); + throw new IOException(tlsMismatch + " Every server of the connection failed with this TLS mismatch, so the connection was closed without retrying: " + failList); + } if (reconnectOnConnect) { timeTraceLogger.trace("trying to reconnect on connect"); reconnectImpl(); // call the impl here otherwise the tryingToConnect guard will block the behavior @@ -470,6 +489,8 @@ protected void reconnectImpl() throws InterruptedException { protected void reconnectImplConnect() throws InterruptedException { int totalRounds = 0; + int roundAttempts = 0; // attempts made in the current round over the pool + boolean roundAllTlsMismatch = true; // false once an attempt of the round failed for another reason NatsUri first = null; NatsUri cur; while ((cur = serverPool.nextServer()) != null) { @@ -481,6 +502,13 @@ protected void reconnectImplConnect() throws InterruptedException { } else if (first.equals(cur)) { // went around the pool an entire time + if (roundAttempts > 0 && roundAllTlsMismatch) { + // every server failed with the TLS mismatch between the options and the server, + // retrying cannot help; the caller closes the connection + return; + } + roundAttempts = 0; + roundAllTlsMismatch = true; invokeReconnectDelayHandler(++totalRounds); } @@ -492,6 +520,7 @@ else if (first.equals(cur)) { return; } connectError.set(null); // reset on each loop + attemptTlsMismatch = null; if (isDisconnectingOrClosed() || this.isClosing()) { return; } @@ -513,6 +542,11 @@ else if (first.equals(cur)) { } serverAuthErrors.put(resolved, err); } + + roundAttempts++; + if (attemptTlsMismatch == null) { + roundAllTlsMismatch = false; + } } if (!isConnected() && !isClosed()) { @@ -760,15 +794,21 @@ protected void upgradeToSecureIfNeeded(NatsUri nuri) throws IOException { // required | isTLSRequired() | ok // available | isTLSRequired() | ok // neither | isTLSRequired() | mismatch + // A mismatch fails the attempt and the next server is tried, since a single server can be + // misconfigured. The options decide TLS for every server, so once every server of one round + // over the list has failed with the mismatch, retrying cannot help and the connect and + // reconnect loops close the connection instead. See connectImpl and reconnectImplConnect. ServerInfo serverInfo = getServerInfo(); if (options.isTLSRequired()) { if (!serverInfo.isTLSRequired() && !serverInfo.isTLSAvailable()) { - throw new IOException("SSL connection wanted by client."); + attemptTlsMismatch = "SSL connection wanted by client."; + throw new IOException(attemptTlsMismatch); } dataPort.upgradeToSecure(); } else if (serverInfo.isTLSRequired()) { - throw new IOException("SSL required by server."); + attemptTlsMismatch = "SSL required by server."; + throw new IOException(attemptTlsMismatch); } } } diff --git a/src/test/java/io/nats/client/OptionsTests.java b/src/test/java/io/nats/client/OptionsTests.java index b5bfb4793..5f9557b5d 100644 --- a/src/test/java/io/nats/client/OptionsTests.java +++ b/src/test/java/io/nats/client/OptionsTests.java @@ -1206,6 +1206,35 @@ public void testInheritedUserInfoInConnectOptions() throws URISyntaxException { assertTrue(connectString.contains("\"user\":\"optuser\""), connectString); } + @Test + public void testTlsFirstAsksForTls() throws Exception { + SSLContext ctx = SslTestingHelper.createTestSSLContext(); + SSLContext originalDefault = SSLContext.getDefault(); + try { + SSLContext.setDefault(ctx); + + // tls first without any other tls setting selects the default context, the same as secure() + Options o = new Options.Builder().tlsFirst().build(); + assertTrue(o.isTlsFirst()); + assertTrue(o.isTLSRequired()); + assertEquals(ctx, o.getSslContext()); + + // a context from another setting is kept + SSLContext supplied = SslTestingHelper.createTestSSLContext(); + o = new Options.Builder().tlsFirst().sslContext(supplied).build(); + assertEquals(supplied, o.getSslContext()); + o = new Options.Builder().tlsFirst().opentls().build(); + assertNotNull(o.getSslContext()); + assertNotEquals(ctx, o.getSslContext()); + o = new Options.Builder().server("opentls://localhost").tlsFirst().build(); + assertNotNull(o.getSslContext()); + assertNotEquals(ctx, o.getSslContext()); + } + finally { + SSLContext.setDefault(originalDefault); + } + } + @Test public void testTokenSupplier() { String serverURI = "nats://localhost:2222"; diff --git a/src/test/java/io/nats/client/impl/TLSConnectTests.java b/src/test/java/io/nats/client/impl/TLSConnectTests.java index 7011e3a48..9ed40710a 100644 --- a/src/test/java/io/nats/client/impl/TLSConnectTests.java +++ b/src/test/java/io/nats/client/impl/TLSConnectTests.java @@ -32,6 +32,7 @@ import java.security.cert.CertificateException; import java.time.Duration; import java.util.Date; +import java.util.List; import java.util.Properties; import java.util.concurrent.CountDownLatch; import java.util.concurrent.Future; @@ -342,6 +343,157 @@ public void testServerSecureClientNotMismatch() { }); } + @Test + public void testTlsMismatchWithEveryServerClosesWithoutRetrying() throws Exception { + // The options decide whether the client uses TLS, for every server of the connection. When every server of + // one round over the list fails with a TLS mismatch, retrying cannot help: the connect closes the connection + // and throws, without reconnecting on connect. + try (NatsTestServer tls = new NatsTestServer("src/test/resources/tls.conf", false); + NatsTestServer tls2 = new NatsTestServer("src/test/resources/tls.conf", false); + NatsTestServer plain = new NatsTestServer(); + NatsTestServer plain2 = new NatsTestServer()) { + // the servers require TLS, the options have no context + _testTlsMismatchWithEveryServer(new Options.Builder(), tls, tls2, "SSL required by server."); + // the servers have no TLS, the options have a context + _testTlsMismatchWithEveryServer(new Options.Builder().sslContext(SslTestingHelper.createTestSSLContext()), plain, plain2, "SSL connection wanted by client."); + } + } + + private static void _testTlsMismatchWithEveryServer(Options.Builder builder, NatsTestServer s1, NatsTestServer s2, String mismatch) { + ListenerForTesting listener = new ListenerForTesting(); + Options options = builder + .servers(new String[]{s1.getURI(), s2.getURI()}) + .noRandomize() + .maxReconnects(3) + .reconnectWait(Duration.ofSeconds(1)) + .connectionListener(listener) + .errorListener(listener) + .build(); + + long start = System.nanoTime(); + IOException e = assertThrows(IOException.class, () -> Nats.connectReconnectOnConnect(options)); + long elapsedMillis = (System.nanoTime() - start) / 1_000_000; + assertTrue(e.getMessage().startsWith(mismatch), e.getMessage()); + assertTrue(elapsedMillis < 1000, "closed after one round, not after a reconnect wait: " + elapsedMillis + " ms"); + // one attempt per server and no reconnect round + assertEquals(2, countMismatches(listener, mismatch), listener.getExceptions().toString()); + } + + private static long countMismatches(ListenerForTesting listener, String mismatch) { + return listener.getExceptions().stream().filter(x -> String.valueOf(x.getMessage()).contains(mismatch)).count(); + } + + @Test + public void testTlsMismatchWithOneServerDoesNotStopTheConnect() throws Exception { + // A single server can be misconfigured. A mismatch with it fails that attempt and the next server is tried. + try (NatsTestServer tls = new NatsTestServer("src/test/resources/tls.conf", false); + NatsTestServer plain = new NatsTestServer()) { + // no context: the server that requires TLS is passed over, the plain one is reached + ListenerForTesting listener = new ListenerForTesting(); + Options options = new Options.Builder() + .servers(new String[]{tls.getURI(), plain.getURI()}) + .noRandomize() + .maxReconnects(0) + .errorListener(listener) + .build(); + try (Connection nc = standardConnectionWait(Nats.connect(options))) { + assertEquals(plain.getNatsPort(), options.createURIForServer(nc.getConnectedUrl()).getPort()); + } + assertEquals(1, countMismatches(listener, "SSL required by server."), listener.getExceptions().toString()); + + // a context: the plain server is passed over, the one with TLS is reached + listener = new ListenerForTesting(); + options = new Options.Builder() + .servers(new String[]{plain.getURI(), tls.getURI()}) + .noRandomize() + .maxReconnects(0) + .sslContext(SslTestingHelper.createTestSSLContext()) + .errorListener(listener) + .build(); + try (Connection nc = standardConnectionWait(Nats.connect(options))) { + assertEquals(tls.getNatsPort(), options.createURIForServer(nc.getConnectedUrl()).getPort()); + } + assertEquals(1, countMismatches(listener, "SSL connection wanted by client."), listener.getExceptions().toString()); + } + } + + @Test + public void testTlsMismatchNextToAnUnreachableServerKeepsReconnecting() throws Exception { + // An unreachable server is not a mismatch, it may come back: the round is not conclusive and the normal + // reconnect rules apply, so the reconnect budget is used before the connection closes. + ListenerForTesting listener = new ListenerForTesting(); + try (NatsTestServer tls = new NatsTestServer("src/test/resources/tls.conf", false)) { + Options options = new Options.Builder() + .servers(new String[]{tls.getURI(), "nats://localhost:" + NatsTestServer.nextPort()}) + .noRandomize() + .maxReconnects(1) + .reconnectWait(Duration.ofMillis(100)) + .connectionListener(listener) + .errorListener(listener) + .build(); + listener.prepForStatusChange(Events.CLOSED); + Connection nc = Nats.connectReconnectOnConnect(options); // does not throw: the budget is used, then the connection closes + assertTrue(listener.waitForStatusChange(5, TimeUnit.SECONDS)); + assertEquals(Connection.Status.CLOSED, nc.getStatus()); + // the initial round and at least one reconnect round reached the server that requires TLS + assertTrue(countMismatches(listener, "SSL required by server.") >= 2, listener.getExceptions().toString()); + } + } + + @Test + public void testServerStartsRequiringTlsClientHasNoContextClosesOnReconnect() throws Exception { + // A connected client without an SSL context meets a server that requires TLS on reconnect. The pool has that + // one server, so one attempt is a full round: the reconnect stops and the connection is closed, however many + // reconnects remain. + ListenerForTesting listener = new ListenerForTesting(); + try (NatsTestServer ts = new NatsTestServer()) { + Options options = new Options.Builder() + .server(ts.getURI()) + .maxReconnects(10) + .reconnectWait(Duration.ofMillis(100)) + .connectionListener(listener) + .errorListener(listener) + .build(); + assertFalse(options.isTLSRequired()); + ProxyConnection conn = new ProxyConnection(options, SERVER_UNCHANGED); + conn.connect(false); + standardConnectionWait(conn); + + conn.serverType = SERVER_CLAIMS_TLS_REQUIRED; + listener.prepForStatusChange(Events.CLOSED); + conn.forceReconnect(); + assertTrue(listener.waitForStatusChange(5, TimeUnit.SECONDS)); + assertEquals(Connection.Status.CLOSED, conn.getStatus()); + List exceptions = listener.getExceptions(); + long attempts = exceptions.stream().filter(x -> String.valueOf(x.getMessage()).contains("SSL required by server")).count(); + assertEquals(1, attempts, exceptions.toString()); + } + } + + @Test + public void testTlsFirstWithoutContextUsesTheDefaultContext() throws Exception { + if (TestBase.atLeast2_10_3(ensureRunServerInfo())) { + SSLContext originalDefault = SSLContext.getDefault(); + try (NatsTestServer ts = new NatsTestServer( + NatsTestServer.builder() + .configFilePath("src/test/resources/tls_first.conf") + .skipConnectValidate()) + ) { + SSLContext.setDefault(SslTestingHelper.createTestSSLContext()); + Options options = new Options.Builder() + .server(ts.getURI()) + .maxReconnects(0) + .tlsFirst() + .build(); + assertTrue(options.isTLSRequired()); + assertCanConnectAndPubSub(options); + } + finally { + SSLContext.setDefault(originalDefault); + } + } + } + @Test public void testClientSecureServerNotMismatch() { assertThrows(IOException.class, () -> { @@ -411,14 +563,20 @@ public void testSSLContextFactoryPropertiesPassOnCorrectly() throws NoSuchAlgori assertEquals("tlsAlgorithm", factory.properties.getTlsAlgorithm()); } - private static final int SERVER_INSECURE = 1; - private static final int SERVER_TLS_AVAILABLE = 2; - private static final int SERVER_TLS_REQUIRED = 3; + private static final int SERVER_UNCHANGED = 0; // INFO passed through + private static final int SERVER_INSECURE = 1; // tls_required removed from INFO + private static final int SERVER_TLS_AVAILABLE = 2; // tls_required turned into tls_available + private static final int SERVER_TLS_REQUIRED = 3; // INFO passed through, the server itself requires TLS + private static final int SERVER_CLAIMS_TLS_REQUIRED = 4; // tls_required added to the INFO of a plain server static class ProxyConnection extends NatsConnection { int serverType; public ProxyConnection(String servers, boolean tlsFirst, ErrorListener listener, int serverType) throws Exception { - super(makeMiddleman(servers, tlsFirst, listener)); + this(makeMiddleman(servers, tlsFirst, listener), serverType); + } + + public ProxyConnection(Options options, int serverType) throws Exception { + super(options); this.serverType = serverType; } @@ -443,6 +601,8 @@ protected void handleInfo(String infoJson) { super.handleInfo(infoJson.replace(",\"tls_required\":true", "")); break; case SERVER_TLS_AVAILABLE: super.handleInfo(infoJson.replace("\"tls_required\":true", "\"tls_available\":true")); break; + case SERVER_CLAIMS_TLS_REQUIRED: + super.handleInfo(infoJson.replaceFirst("\\{", "{\"tls_required\":true,")); break; default: super.handleInfo(infoJson); }