Revert "Revert "Enable ZK access control by default""

2 files changed, 27 insertions, 22 deletions

diff --git a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java
index 1c60587b6e4..0a1bfdae3a3 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java
@@ -11,6 +11,7 @@ import java.nio.channels.SocketChannel;
 import java.util.HashSet;
 import java.util.Optional;
 import java.util.Set;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 /**
@@ -29,33 +30,37 @@ public class RestrictedServerCnxnFactory extends NIOServerCnxnFactory {
     
     @Override
     protected NIOServerCnxn createConnection(SocketChannel socket, SelectionKey selection) throws IOException {
+        ImmutableSet<String> allowedZooKeeperClients = findAllowedZooKeeperClients();
+        if (allowedZooKeeperClients.isEmpty()) return super.createConnection(socket, selection);
+        
         String remoteHost = ((InetSocketAddress)socket.getRemoteAddress()).getHostName();
-
-        Optional<ImmutableSet<String>> allowedZooKeeperClients = ZooKeeperServer.getAllowedClientHostnames();
-        if ( ! allowedZooKeeperClients.isPresent()) {
-            log.fine("Allowing connection to ZooKeeper from " + remoteHost + ", as allowed zooKeeper clients is not set");
-            return super.createConnection(socket, selection); // client checking is not activated
-        }
-
-        if ( ! remoteHost.equals("localhost") && ! allowedZooKeeperClients.get().contains(remoteHost)) {
+        if ( ! remoteHost.equals("localhost") && ! allowedZooKeeperClients.contains(remoteHost)) {
             String errorMessage = "Rejecting connection to ZooKeeper from " + remoteHost +
-                                  ": This cluster only allow connection from hosts in: " + allowedZooKeeperClients.get();
-            if ("true".equals(System.getenv("vespa_zkfacade__restrict"))) {
-                log.info(errorMessage);
-                throw new IllegalArgumentException(errorMessage);
-            }
-            else {
-                log.fine("Would reject if activated: " + errorMessage);
-            }
+                                  ": This cluster only allow connection from hosts in: " + allowedZooKeeperClients;
+            log.info(errorMessage);
+            throw new IllegalArgumentException(errorMessage); // log and throw as this exception will be suppressed by zk
         }
-        log.fine("Allowing connection to ZooKeeper from " + remoteHost + ", as it is in " + allowedZooKeeperClients.get());
+        log.fine(() -> "Allowing connection to ZooKeeper from " + remoteHost + ", as it is in " + allowedZooKeeperClients);
         return super.createConnection(socket, selection);
     }
 
+    /** Returns the allowed client host names. If the list is empty any host is allowed. */
+    private ImmutableSet<String> findAllowedZooKeeperClients() {
+        // Environment has precedence. Note that this allows setting restrict to "" to turn off client restriction
+        String environmentAllowedZooKeeperClients = System.getenv("vespa_zkfacade__restrict");
+        if (environmentAllowedZooKeeperClients != null) 
+            return ImmutableSet.copyOf(toHostnameSet(environmentAllowedZooKeeperClients));
+
+        // No environment setting -> use static field
+        return ZooKeeperServer.getAllowedClientHostnames();
+    }
+
     private Set<String> toHostnameSet(String commaSeparatedString) {
         Set<String> hostnames = new HashSet<>();
-        for (String hostname : commaSeparatedString.split(","))
-            hostnames.add(hostname.trim());
+        for (String hostname : commaSeparatedString.split(",")) {
+            if ( ! hostname.trim().isEmpty())
+                hostnames.add(hostname.trim());
+        }
         return hostnames;
     }
 
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java
index f6219877f5e..4eed2173fb5 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java
@@ -28,7 +28,7 @@ public class ZooKeeperServer extends AbstractComponent implements Runnable {
      * This belongs logically to the server instance but must be static to make it accessible
      * from RestrictedServerCnxnFactory, which is created by ZK through reflection.
      */
-    private static volatile Optional<ImmutableSet<String>> allowedClientHostnames = Optional.empty();
+    private static volatile ImmutableSet<String> allowedClientHostnames = ImmutableSet.of();
 
     private static final java.util.logging.Logger log = java.util.logging.Logger.getLogger(ZooKeeperServer.class.getName());
     private static final String ZOOKEEPER_JMX_LOG4J_DISABLE = "zookeeper.jmx.log4j.disable";
@@ -56,11 +56,11 @@ public class ZooKeeperServer extends AbstractComponent implements Runnable {
     
     /** Restrict access to this ZooKeeper server to the given client hosts */
     public static void setAllowedClientHostnames(Collection<String> hostnames) {
-        allowedClientHostnames = Optional.of(ImmutableSet.copyOf(hostnames));
+        allowedClientHostnames = ImmutableSet.copyOf(hostnames);
     }
     
     /** Returns the hosts which are allowed to access this ZooKeeper server, or empty to allow access from anywhere */
-    public static Optional<ImmutableSet<String>> getAllowedClientHostnames() { return allowedClientHostnames; }
+    public static ImmutableSet<String> getAllowedClientHostnames() { return allowedClientHostnames; }
     
     private void writeConfigToDisk(ZookeeperServerConfig config) {
        String cfg = transformConfigToString(config);