ZOOKEEPER-3195: TLS - disable client-initiated renegotiation

Summary: client-initiated renegotiation is insecure and is vulnerable to MITM attacks.
Unfortunately, the feature is enabled in Java by default. This disables it.
See https://bugs.openjdk.java.net/browse/JDK-7188658 and
https://www.oracle.com/technetwork/java/javase/documentation/tlsreadme-141115.html

Test Plan: manually tested by running a secure ZK server and probing the listening port
with python's sslyze tool (using `sslyze --reneg ...`). Tested on Java 8, 9, 10, and 11.

Author: Ilya Maykov <ilyam@fb.com>

Reviewers: andor@apache.org

Closes #710 from ivmaykov/ZOOKEEPER-3195

zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java

@@ -66,6 +66,21 @@ import org.slf4j.LoggerFactory;
 public abstract class X509Util implements Closeable, AutoCloseable {
     private static final Logger LOG = LoggerFactory.getLogger(X509Util.class);
 
+    private static final String REJECT_CLIENT_RENEGOTIATION_PROPERTY =
+            "jdk.tls.rejectClientInitiatedRenegotiation";
+    static {
+        // Client-initiated renegotiation in TLS is unsafe and
+        // allows MITM attacks, so we should disable it unless
+        // it was explicitly enabled by the user.
+        // A brief summary of the issue can be found at
+        // https://www.ietf.org/proceedings/76/slides/tls-7.pdf
+        if (System.getProperty(REJECT_CLIENT_RENEGOTIATION_PROPERTY) == null) {
+            LOG.info("Setting -D {}=true to disable client-initiated TLS renegotiation",
+                    REJECT_CLIENT_RENEGOTIATION_PROPERTY);
+            System.setProperty(REJECT_CLIENT_RENEGOTIATION_PROPERTY, Boolean.TRUE.toString());
+        }
+    }
+
     static final String DEFAULT_PROTOCOL = "TLSv1.2";
     private static final String[] DEFAULT_CIPHERS_JAVA8 = {
             "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256",

zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java

@@ -17,10 +17,24 @@
  */
 package org.apache.zookeeper.common;
 
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.ServerSocket;
+import java.net.Socket;
 import java.security.Security;
 import java.util.Collection;
-
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import javax.net.ssl.HandshakeCompletedEvent;
+import javax.net.ssl.HandshakeCompletedListener;
 import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLHandshakeException;
 import javax.net.ssl.SSLServerSocket;
 import javax.net.ssl.SSLSocket;
 import javax.net.ssl.X509KeyManager;
@@ -389,6 +403,85 @@ public class X509UtilTest extends BaseX509ParameterizedTestCase {
         }
     }
 
+    private static void forceClose(Socket s) {
+        if (s == null || s.isClosed()) {
+            return;
+        }
+        try {
+            s.close();
+        } catch (IOException e) {
+        }
+    }
+
+    private static void forceClose(ServerSocket s) {
+        if (s == null || s.isClosed()) {
+            return;
+        }
+        try {
+            s.close();
+        } catch (IOException e) {
+        }
+    }
+
+    // This test makes sure that client-initiated TLS renegotiation does not
+    // succeed. We explicitly disable it at the top of X509Util.java.
+    @Test(expected = SSLHandshakeException.class)
+    public void testClientRenegotiationFails() throws Throwable {
+        int port = PortAssignment.unique();
+        ExecutorService workerPool = Executors.newCachedThreadPool();
+        final SSLServerSocket listeningSocket = x509Util.createSSLServerSocket();
+        SSLSocket clientSocket = null;
+        SSLSocket serverSocket = null;
+        final AtomicInteger handshakesCompleted = new AtomicInteger(0);
+        try {
+            InetSocketAddress localServerAddress = new InetSocketAddress(
+                    InetAddress.getLoopbackAddress(), port);
+            listeningSocket.bind(localServerAddress);
+            Future<SSLSocket> acceptFuture;
+            acceptFuture = workerPool.submit(new Callable<SSLSocket>() {
+                @Override
+                public SSLSocket call() throws Exception {
+                    SSLSocket sslSocket = (SSLSocket) listeningSocket.accept();
+                    sslSocket.addHandshakeCompletedListener(new HandshakeCompletedListener() {
+                        @Override
+                        public void handshakeCompleted(HandshakeCompletedEvent handshakeCompletedEvent) {
+                            handshakesCompleted.getAndIncrement();
+                        }
+                    });
+                    Assert.assertEquals(1, sslSocket.getInputStream().read());
+                    try {
+                        // 2nd read is after the renegotiation attempt and will fail
+                        sslSocket.getInputStream().read();
+                        return sslSocket;
+                    } catch (Exception e) {
+                        forceClose(sslSocket);
+                        throw e;
+                    }
+                }
+            });
+            clientSocket = x509Util.createSSLSocket();
+            clientSocket.connect(localServerAddress);
+            clientSocket.getOutputStream().write(1);
+            // Attempt to renegotiate after establishing the connection
+            clientSocket.startHandshake();
+            clientSocket.getOutputStream().write(1);
+            // The exception is thrown on the server side, we need to unwrap it
+            try {
+                serverSocket = acceptFuture.get();
+            } catch (ExecutionException e) {
+                throw e.getCause();
+            }
+        } finally {
+            forceClose(serverSocket);
+            forceClose(clientSocket);
+            forceClose(listeningSocket);
+            workerPool.shutdown();
+            // Make sure the first handshake completed and only the second
+            // one failed.
+            Assert.assertEquals(1, handshakesCompleted.get());
+        }
+    }
+
     // Warning: this will reset the x509Util
     private void setCustomCipherSuites() {
         System.setProperty(x509Util.getCipherSuitesProperty(), customCipherSuites[0] + "," + customCipherSuites[1]);