[ADR40] TLS mismatch: close the connection after a full round of mismatches; tlsFirst selects the default context

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

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.

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.
+         * <p>
+         * 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");
             }
 

src/main/java/io/nats/client/impl/NatsConnection.java

@@ -63,6 +63,8 @@ class NatsConnection implements Connection {
 
     protected CompletableFuture<DataPort> 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<NatsUri> 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);
                 }
             }
         }

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";