ZOOKEEPER-3388: Allow client port to support plaintext and encrypted …

…connections simultaneously

Author: Brian Nixon <nixon@fb.com>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Michael Han <hanm@apache.org>

Closes #944 from enixon/client-port-uni

build.xml

@@ -37,7 +37,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
 
     <property name="audience-annotations.version" value="0.5.0" />
 
-    <property name="netty.version" value="4.1.29.Final"/>
+    <property name="netty.version" value="4.1.36.Final"/>
 
     <property name="junit.version" value="4.12"/>
     <property name="mockito.version" value="2.27.0"/>

pom.xml

@@ -277,7 +277,7 @@
     <mockito.version>2.27.0</mockito.version>
     <hamcrest.version>1.3</hamcrest.version>
     <commons-cli.version>1.2</commons-cli.version>
-    <netty.version>4.1.29.Final</netty.version>
+    <netty.version>4.1.36.Final</netty.version>
     <jetty.version>9.4.17.v20190418</jetty.version>
     <jackson.version>2.9.9</jackson.version>
     <json.version>1.1.1</json.version>

zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md

@@ -1190,7 +1190,13 @@ encryption/authentication/authorization performed by the service.
     (Java system properties: **zookeeper.ssl.handshakeDetectionTimeoutMillis** and **zookeeper.ssl.quorum.handshakeDetectionTimeoutMillis**)
     **New in 3.5.5:**
     TBD
-        
+
+* *client.portUnification*:
+    (Java system properties: **zookeeper.client.portUnification**)
+    Specifies that the client port should accept SSL connections
+    (using the same configuration as the secure client port).
+    Default: false
+
 
 <a name="Experimental+Options%2FFeatures"></a>
 

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

@@ -23,10 +23,16 @@ import java.net.Socket;
 import java.util.Arrays;
 
 import javax.net.ssl.SSLContext;
+import java.util.Collections;
+import java.util.List;
+
 import javax.net.ssl.SSLParameters;
 import javax.net.ssl.SSLServerSocket;
 import javax.net.ssl.SSLSocket;
 
+import io.netty.handler.ssl.IdentityCipherSuiteFilter;
+import io.netty.handler.ssl.JdkSslContext;
+import io.netty.handler.ssl.SslContext;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -45,6 +51,7 @@ public class SSLContextAndOptions {
     private final X509Util x509Util;
     private final String[] enabledProtocols;
     private final String[] cipherSuites;
+    private final List<String> cipherSuitesAsList;
     private final X509Util.ClientAuth clientAuth;
     private final SSLContext sslContext;
     private final int handshakeDetectionTimeoutMillis;
@@ -60,7 +67,9 @@ public class SSLContextAndOptions {
         this.x509Util = requireNonNull(x509Util);
         this.sslContext = requireNonNull(sslContext);
         this.enabledProtocols = getEnabledProtocols(requireNonNull(config), sslContext);
-        this.cipherSuites = getCipherSuites(config);
+        String[] ciphers = getCipherSuites(config);
+        this.cipherSuites = ciphers;
+        this.cipherSuitesAsList = Collections.unmodifiableList(Arrays.asList(ciphers));
         this.clientAuth = getClientAuth(config);
         this.handshakeDetectionTimeoutMillis = getHandshakeDetectionTimeoutMillis(config);
     }
@@ -97,6 +106,18 @@ public class SSLContextAndOptions {
         return configureSSLServerSocket(sslServerSocket);
     }
 
+    public SslContext createNettyJdkSslContext(SSLContext sslContext, boolean isClientSocket) {
+        return new JdkSslContext(
+                sslContext,
+                isClientSocket,
+                cipherSuitesAsList,
+                IdentityCipherSuiteFilter.INSTANCE,
+                null,
+                isClientSocket ? X509Util.ClientAuth.NONE.toNettyClientAuth() : clientAuth.toNettyClientAuth(),
+                enabledProtocols,
+                false);
+    }
+
     public int getHandshakeDetectionTimeoutMillis() {
         return handshakeDetectionTimeoutMillis;
     }

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

@@ -80,7 +80,7 @@ public abstract class X509Util implements Closeable, AutoCloseable {
         }
     }
 
-    static final String DEFAULT_PROTOCOL = "TLSv1.2";
+    public static final String DEFAULT_PROTOCOL = "TLSv1.2";
     private static String[] getGCMCiphers() {
         return new String[] {
             "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256",
@@ -129,9 +129,15 @@ public abstract class X509Util implements Closeable, AutoCloseable {
      * If the config property is not set, the default value is NEED.
      */
     public enum ClientAuth {
-        NONE,
-        WANT,
-        NEED;
+        NONE(io.netty.handler.ssl.ClientAuth.NONE),
+        WANT(io.netty.handler.ssl.ClientAuth.OPTIONAL),
+        NEED(io.netty.handler.ssl.ClientAuth.REQUIRE);
+
+        private final io.netty.handler.ssl.ClientAuth nettyAuth;
+
+        ClientAuth(io.netty.handler.ssl.ClientAuth nettyAuth) {
+            this.nettyAuth = nettyAuth;
+        }
 
         /**
          * Converts a property value to a ClientAuth enum. If the input string is empty or null, returns
@@ -146,6 +152,10 @@ public abstract class X509Util implements Closeable, AutoCloseable {
             }
             return ClientAuth.valueOf(prop.toUpperCase());
         }
+
+        public io.netty.handler.ssl.ClientAuth toNettyClientAuth() {
+            return nettyAuth;
+        }
     }
 
     private String sslProtocolProperty = getConfigPrefix() + "protocol";

zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxnFactory.java

@@ -25,6 +25,7 @@ import java.security.KeyManagementException;
 import java.security.NoSuchAlgorithmException;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
@@ -32,6 +33,7 @@ import java.util.concurrent.atomic.AtomicReference;
 
 import javax.net.ssl.SSLContext;
 import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
 import javax.net.ssl.SSLPeerUnverifiedException;
 import javax.net.ssl.SSLSession;
 import javax.net.ssl.X509KeyManager;
@@ -43,6 +45,7 @@ import io.netty.buffer.ByteBufAllocator;
 import io.netty.channel.Channel;
 import io.netty.channel.ChannelDuplexHandler;
 import io.netty.channel.ChannelFuture;
+import io.netty.channel.ChannelHandler;
 import io.netty.channel.ChannelHandler.Sharable;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelInitializer;
@@ -54,6 +57,9 @@ import io.netty.channel.group.ChannelGroup;
 import io.netty.channel.group.ChannelGroupFuture;
 import io.netty.channel.group.DefaultChannelGroup;
 import io.netty.channel.socket.SocketChannel;
+import io.netty.handler.ssl.OpenSsl;
+import io.netty.handler.ssl.OptionalSslHandler;
+import io.netty.handler.ssl.SslContext;
 import io.netty.handler.ssl.SslHandler;
 import io.netty.util.AttributeKey;
 import io.netty.util.ReferenceCountUtil;
@@ -63,16 +69,32 @@ import io.netty.util.concurrent.GenericFutureListener;
 import org.apache.zookeeper.KeeperException;
 import org.apache.zookeeper.common.ClientX509Util;
 import org.apache.zookeeper.common.NettyUtils;
+import org.apache.zookeeper.common.SSLContextAndOptions;
 import org.apache.zookeeper.common.X509Exception;
 import org.apache.zookeeper.common.X509Exception.SSLContextException;
 import org.apache.zookeeper.server.auth.ProviderRegistry;
 import org.apache.zookeeper.server.auth.X509AuthenticationProvider;
+import org.apache.zookeeper.server.quorum.QuorumPeerConfig;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 public class NettyServerCnxnFactory extends ServerCnxnFactory {
     private static final Logger LOG = LoggerFactory.getLogger(NettyServerCnxnFactory.class);
 
+    /**
+     * Allow client-server sockets to accept both SSL and plaintext connections
+     */
+    public static final String PORT_UNIFICATION_KEY = "zookeeper.client.portUnification";
+    private final boolean shouldUsePortUnification;
+
+    /**
+     * The first byte in TLS protocol is the content type of the subsequent record.
+     * Handshakes use value 22 (0x16) so the first byte offered on any TCP connection
+     * attempting to establish a TLS connection will be this value.
+     * https://tools.ietf.org/html/rfc8446#page-79
+     */
+    private static final byte TLS_HANDSHAKE_RECORD_TYPE = 0x16;
+
     private final ServerBootstrap bootstrap;
     private Channel parentChannel;
     private final ChannelGroup allChannels =
@@ -91,6 +113,66 @@ public class NettyServerCnxnFactory extends ServerCnxnFactory {
     private static final AtomicReference<ByteBufAllocator> TEST_ALLOCATOR =
             new AtomicReference<>(null);
 
+    /**
+     * A handler that detects whether the client would like to use
+     * TLS or not and responds in kind. The first bytes are examined
+     * for the static TLS headers to make the determination and
+     * placed back in the stream with the correct ChannelHandler
+     * instantiated.
+     */
+    class DualModeSslHandler extends OptionalSslHandler {
+        DualModeSslHandler(SslContext sslContext) {
+            super(sslContext);
+        }
+
+        @Override
+        protected void decode(ChannelHandlerContext context, ByteBuf in, List<Object> out) throws Exception {
+            if (in.readableBytes() >= 5) {
+                super.decode(context, in, out);
+            } else if (in.readableBytes() > 0) {
+                // It requires 5 bytes to detect a proper ssl connection. In the
+                // case that the server receives fewer, check if we can fail to plaintext.
+                // This will occur when for any four letter work commands.
+                if (TLS_HANDSHAKE_RECORD_TYPE != in.getByte(0)) {
+                    LOG.debug("first byte {} does not match TLS handshake, failing to plaintext", in.getByte(0));
+                    handleNonSsl(context);
+                }
+            }
+        }
+
+        /**
+         * pulled directly from OptionalSslHandler to allow for access
+         * @param context
+         */
+        private void handleNonSsl(ChannelHandlerContext context) {
+            ChannelHandler handler = this.newNonSslHandler(context);
+            if (handler != null) {
+                context.pipeline().replace(this, this.newNonSslHandlerName(), handler);
+            } else {
+                context.pipeline().remove(this);
+            }
+        }
+
+        @Override
+        protected SslHandler newSslHandler(ChannelHandlerContext context, SslContext sslContext) {
+            NettyServerCnxn cnxn = Objects.requireNonNull(context.channel().attr(CONNECTION_ATTRIBUTE).get());
+            LOG.debug("creating ssl handler for session {}", cnxn.getSessionId());
+            SslHandler handler = super.newSslHandler(context, sslContext);
+            Future<Channel> handshakeFuture = handler.handshakeFuture();
+            handshakeFuture.addListener(new CertificateVerifier(handler, cnxn));
+            return handler;
+        }
+
+        @Override
+        protected ChannelHandler newNonSslHandler(ChannelHandlerContext context) {
+            NettyServerCnxn cnxn = Objects.requireNonNull(context.channel().attr(CONNECTION_ATTRIBUTE).get());
+            LOG.debug("creating plaintext handler for session {}", cnxn.getSessionId());
+            allChannels.add(context.channel());
+            addCnxn(cnxn);
+            return super.newNonSslHandler(context);
+        }
+    }
+
     /**
      * This is an inner class since we need to extend ChannelDuplexHandler, but
      * NettyServerCnxnFactory already extends ServerCnxnFactory. By making it inner
@@ -124,7 +206,7 @@ public class NettyServerCnxnFactory extends ServerCnxnFactory {
                 SslHandler sslHandler = ctx.pipeline().get(SslHandler.class);
                 Future<Channel> handshakeFuture = sslHandler.handshakeFuture();
                 handshakeFuture.addListener(new CertificateVerifier(sslHandler, cnxn));
-            } else {
+            } else if (!shouldUsePortUnification) {
                 allChannels.add(ctx.channel());
                 addCnxn(cnxn);
             }
@@ -218,28 +300,51 @@ public class NettyServerCnxnFactory extends ServerCnxnFactory {
             }
             super.write(ctx, msg, promise);
         }
+    }
 
-        private final class CertificateVerifier implements GenericFutureListener<Future<Channel>> {
-            private final SslHandler sslHandler;
-            private final NettyServerCnxn cnxn;
+    final class CertificateVerifier implements GenericFutureListener<Future<Channel>> {
+        private final SslHandler sslHandler;
+        private final NettyServerCnxn cnxn;
 
-            CertificateVerifier(SslHandler sslHandler, NettyServerCnxn cnxn) {
-                this.sslHandler = sslHandler;
-                this.cnxn = cnxn;
-            }
+        CertificateVerifier(SslHandler sslHandler, NettyServerCnxn cnxn) {
+            this.sslHandler = sslHandler;
+            this.cnxn = cnxn;
+        }
 
-            /**
-             * Only allow the connection to stay open if certificate passes auth
-             */
-            public void operationComplete(Future<Channel> future) throws SSLPeerUnverifiedException {
-                if (future.isSuccess()) {
-                    if (LOG.isDebugEnabled()) {
-                        LOG.debug("Successful handshake with session 0x{}",
-                                Long.toHexString(cnxn.getSessionId()));
-                    }
-                    SSLEngine eng = sslHandler.engine();
+        /**
+         * Only allow the connection to stay open if certificate passes auth
+         */
+        public void operationComplete(Future<Channel> future) {
+            if (future.isSuccess()) {
+                if (LOG.isDebugEnabled()) {
+                    LOG.debug("Successful handshake with session 0x{}",
+                            Long.toHexString(cnxn.getSessionId()));
+                }
+                SSLEngine eng = sslHandler.engine();
+                // Don't try to verify certificate if we didn't ask client to present one
+                if (eng.getNeedClientAuth() || eng.getWantClientAuth()) {
                     SSLSession session = eng.getSession();
-                    cnxn.setClientCertificateChain(session.getPeerCertificates());
+                    try {
+                        cnxn.setClientCertificateChain(session.getPeerCertificates());
+                    } catch (SSLPeerUnverifiedException e) {
+                        if (eng.getNeedClientAuth()) {
+                            // Certificate was requested but not present
+                            LOG.error("Error getting peer certificates", e);
+                            cnxn.close();
+                            return;
+                        } else {
+                            // Certificate was requested but was optional
+                            // TODO: what auth info should we set on the connection?
+                            final Channel futureChannel = future.getNow();
+                            allChannels.add(Objects.requireNonNull(futureChannel));
+                            addCnxn(cnxn);
+                            return;
+                        }
+                    } catch (Exception e) {
+                        LOG.error("Error getting peer certificates", e);
+                        cnxn.close();
+                        return;
+                    }
 
                     String authProviderProp
                             = System.getProperty(x509Util.getSslAuthProviderProperty(), "x509");
@@ -249,7 +354,7 @@ public class NettyServerCnxnFactory extends ServerCnxnFactory {
                                     ProviderRegistry.getProvider(authProviderProp);
 
                     if (authProvider == null) {
-                        LOG.error("Auth provider not found: {}", authProviderProp);
+                        LOG.error("X509 Auth provider not found: {}", authProviderProp);
                         cnxn.close();
                         return;
                     }
@@ -261,15 +366,15 @@ public class NettyServerCnxnFactory extends ServerCnxnFactory {
                         cnxn.close();
                         return;
                     }
-
-                    final Channel futureChannel = future.getNow();
-                    allChannels.add(Objects.requireNonNull(futureChannel));
-                    addCnxn(cnxn);
-                } else {
-                    LOG.error("Unsuccessful handshake with session 0x{}",
-                            Long.toHexString(cnxn.getSessionId()));
-                    cnxn.close();
                 }
+
+                final Channel futureChannel = future.getNow();
+                allChannels.add(Objects.requireNonNull(futureChannel));
+                addCnxn(cnxn);
+            } else {
+                LOG.error("Unsuccessful handshake with session 0x{}",
+                        Long.toHexString(cnxn.getSessionId()));
+                cnxn.close();
             }
         }
     }
@@ -290,6 +395,18 @@ public class NettyServerCnxnFactory extends ServerCnxnFactory {
     NettyServerCnxnFactory() {
         x509Util = new ClientX509Util();
 
+        boolean usePortUnification = Boolean.getBoolean(PORT_UNIFICATION_KEY);
+        LOG.info("{}={}", PORT_UNIFICATION_KEY, usePortUnification);
+        if (usePortUnification) {
+            try {
+                QuorumPeerConfig.configureSSLAuth();
+            } catch (QuorumPeerConfig.ConfigException e) {
+                LOG.error("unable to set up SslAuthProvider, turning off client port unification", e);
+                usePortUnification = false;
+            }
+        }
+        this.shouldUsePortUnification = usePortUnification;
+
         EventLoopGroup bossGroup = NettyUtils.newNioOrEpollEventLoopGroup(
                 NettyUtils.getClientReachableLocalInetAddressCount());
         EventLoopGroup workerGroup = NettyUtils.newNioOrEpollEventLoopGroup();
@@ -306,7 +423,9 @@ public class NettyServerCnxnFactory extends ServerCnxnFactory {
                     protected void initChannel(SocketChannel ch) throws Exception {
                         ChannelPipeline pipeline = ch.pipeline();
                         if (secure) {
-                            initSSL(pipeline);
+                            initSSL(pipeline, false);
+                        } else if (shouldUsePortUnification) {
+                            initSSL(pipeline, true);
                         }
                         pipeline.addLast("servercnxnfactory", channelHandler);
                     }
@@ -315,37 +434,41 @@ public class NettyServerCnxnFactory extends ServerCnxnFactory {
         this.bootstrap.validate();
     }
 
-    private synchronized void initSSL(ChannelPipeline p)
+    private synchronized void initSSL(ChannelPipeline p, boolean supportPlaintext)
             throws X509Exception, KeyManagementException, NoSuchAlgorithmException {
         String authProviderProp = System.getProperty(x509Util.getSslAuthProviderProperty());
-        SSLContext sslContext;
+        SslContext nettySslContext;
         if (authProviderProp == null) {
-            sslContext = x509Util.getDefaultSSLContext();
+            SSLContextAndOptions sslContextAndOptions = x509Util.getDefaultSSLContextAndOptions();
+            nettySslContext = sslContextAndOptions.createNettyJdkSslContext(
+                        sslContextAndOptions.getSSLContext(), false);
         } else {
-            sslContext = SSLContext.getInstance("TLSv1");
+            SSLContext sslContext = SSLContext.getInstance(ClientX509Util.DEFAULT_PROTOCOL);
             X509AuthenticationProvider authProvider =
-                    (X509AuthenticationProvider)ProviderRegistry.getProvider(
+                    (X509AuthenticationProvider) ProviderRegistry.getProvider(
                             System.getProperty(x509Util.getSslAuthProviderProperty(), "x509"));
 
-            if (authProvider == null)
-            {
+            if (authProvider == null) {
                 LOG.error("Auth provider not found: {}", authProviderProp);
                 throw new SSLContextException(
                         "Could not create SSLContext with specified auth provider: " +
-                        authProviderProp);
+                                authProviderProp);
             }
 
-            sslContext.init(new X509KeyManager[] { authProvider.getKeyManager() },
-                            new X509TrustManager[] { authProvider.getTrustManager() },
-                            null);
+            sslContext.init(new X509KeyManager[]{authProvider.getKeyManager()},
+                    new X509TrustManager[]{authProvider.getTrustManager()},
+                    null);
+            nettySslContext = x509Util.getDefaultSSLContextAndOptions()
+                    .createNettyJdkSslContext(sslContext,false);
         }
 
-        SSLEngine sslEngine = sslContext.createSSLEngine();
-        sslEngine.setUseClientMode(false);
-        sslEngine.setNeedClientAuth(true);
-
-        p.addLast("ssl", new SslHandler(sslEngine));
-        LOG.info("SSL handler added for channel: {}", p.channel());
+        if (supportPlaintext) {
+            p.addLast("ssl", new DualModeSslHandler(nettySslContext));
+            LOG.debug("dual mode SSL handler added for channel: {}", p.channel());
+        } else {
+            p.addLast("ssl", nettySslContext.newHandler(p.channel().alloc()));
+            LOG.debug("SSL handler added for channel: {}", p.channel());
+        }
     }
 
     @Override

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java

@@ -500,7 +500,7 @@ public class QuorumPeerConfig {
      *             If authentication scheme is configured but authentication
      *             provider is not configured.
      */
-    private void configureSSLAuth() throws ConfigException {
+    public static void configureSSLAuth() throws ConfigException {
         try (ClientX509Util clientX509Util = new ClientX509Util()) {
             String sslAuthProp = "zookeeper.authProvider." + System.getProperty(clientX509Util.getSslAuthProviderProperty(), "x509");
             if (System.getProperty(sslAuthProp) == null) {

zookeeper-server/src/test/java/org/apache/zookeeper/test/ClientSSLTest.java

@@ -28,6 +28,7 @@ import org.apache.zookeeper.ZooDefs;
 import org.apache.zookeeper.ZooKeeper;
 import org.apache.zookeeper.client.ZKClientConfig;
 import org.apache.zookeeper.common.ClientX509Util;
+import org.apache.zookeeper.server.NettyServerCnxnFactory;
 import org.apache.zookeeper.server.ServerCnxnFactory;
 import org.apache.zookeeper.server.quorum.QuorumPeerTestBase;
 import org.junit.After;
@@ -41,6 +42,7 @@ public class ClientSSLTest extends QuorumPeerTestBase {
 
     @Before
     public void setup() {
+        System.setProperty(NettyServerCnxnFactory.PORT_UNIFICATION_KEY, Boolean.TRUE.toString());
         clientX509Util = new ClientX509Util();
         String testDataPath = System.getProperty("test.data.dir", "src/test/resources/data");
         System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, "org.apache.zookeeper.server.NettyServerCnxnFactory");
@@ -53,7 +55,8 @@ public class ClientSSLTest extends QuorumPeerTestBase {
     }
 
     @After
-    public void teardown() throws Exception {
+    public void teardown() {
+        System.clearProperty(NettyServerCnxnFactory.PORT_UNIFICATION_KEY);
         System.clearProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY);
         System.clearProperty(ZKClientConfig.ZOOKEEPER_CLIENT_CNXN_SOCKET);
         System.clearProperty(ZKClientConfig.SECURE_CLIENT);
@@ -64,6 +67,18 @@ public class ClientSSLTest extends QuorumPeerTestBase {
         clientX509Util.close();
     }
 
+    /**
+     * This test checks that client SSL connections work in the absence of a
+     * secure port when port unification is set up for the plaintext port.
+     *
+     * This single client port will be tested for handling both plaintext
+     * and SSL traffic.
+     */
+    @Test
+    public void testClientServerUnifiedPort() throws Exception {
+        testClientServerSSL(false);
+    }
+
     /**
      * This test checks that client - server SSL works in cluster setup of ZK servers, which includes:
      * 1. setting "secureClientPort" in "zoo.cfg" file.
@@ -75,6 +90,10 @@ public class ClientSSLTest extends QuorumPeerTestBase {
      */
     @Test
     public void testClientServerSSL() throws Exception {
+        testClientServerSSL(true);
+    }
+
+    public void testClientServerSSL(boolean useSecurePort) throws Exception {
         final int SERVER_COUNT = 3;
         final int clientPorts[] = new int[SERVER_COUNT];
         final Integer secureClientPorts[] = new Integer[SERVER_COUNT];
@@ -82,16 +101,20 @@ public class ClientSSLTest extends QuorumPeerTestBase {
         for (int i = 0; i < SERVER_COUNT; i++) {
             clientPorts[i] = PortAssignment.unique();
             secureClientPorts[i] = PortAssignment.unique();
-            String server = String.format("server.%d=localhost:%d:%d:participant;localhost:%d",
+            String server = String.format("server.%d=127.0.0.1:%d:%d:participant;127.0.0.1:%d%n",
                     i, PortAssignment.unique(), PortAssignment.unique(), clientPorts[i]);
-            sb.append(server + "\n");
+            sb.append(server);
         }
         String quorumCfg = sb.toString();
 
 
         MainThread[] mt = new MainThread[SERVER_COUNT];
         for (int i = 0; i < SERVER_COUNT; i++) {
-            mt[i] = new MainThread(i, quorumCfg, secureClientPorts[i], true);
+            if (useSecurePort) {
+                mt[i] = new MainThread(i, quorumCfg, secureClientPorts[i], true);
+            } else {
+                mt[i] = new MainThread(i, quorumCfg, true);
+            }
             mt[i].start();
         }
 
@@ -103,8 +126,8 @@ public class ClientSSLTest extends QuorumPeerTestBase {
         for (int i = 0; i < SERVER_COUNT; i++) {
             Assert.assertTrue("waiting for server " + i + " being up",
                     ClientBase.waitForServerUp("127.0.0.1:" + clientPorts[i], TIMEOUT));
-
-            ZooKeeper zk = ClientBase.createZKClient("127.0.0.1:" + secureClientPorts[i], TIMEOUT);
+            final int port = useSecurePort ? secureClientPorts[i] : clientPorts[i];
+            ZooKeeper zk = ClientBase.createZKClient("127.0.0.1:" + port, TIMEOUT);
             // Do a simple operation to make sure the connection is fine.
             zk.create("/test", "".getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
             zk.delete("/test", -1);