ZOOKEEPER-3979: Clean up/robustify audit logs

This is a minimally disruptive mitigation for the issue reported in ZOOKEEPER-3979, "Clients can corrupt the audit log."

A new property allows disabling the "legacy" `digest` authentication mechanism, which could be used by "an attacker" to inject unsanitized data into audit logs.

In general, ZooKeeper administrators should disable unused authentication providers, and ensure that the ones which remain enabled to not produce user IDs susceptible to confuse audit log parsers.

The rest of the patch is made of assorted small cleanups which should not have any impact on operation or security.

(Note that the patch *series* attached to https://github.com/apache/zookeeper/pull/1519 contains additional measures, such as filtering audit user IDs by authentication scheme, but those seem to be overkill for typical deployment scenarios.  That code could still be fished out and polished if the circumstances evolved.)

Author: Damien Diederen <dd@crosstwine.com>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org>

Closes #1533 from ztzg/ZOOKEEPER-3979-robustify-audit-logs

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

@@ -1440,6 +1440,16 @@ Beside this page, you can also find useful information about client side configu
 The ZooKeeper Wiki also has useful pages about [ZooKeeper SSL support](https://cwiki.apache.org/confluence/display/ZOOKEEPER/ZooKeeper+SSL+User+Guide),
 and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/display/ZOOKEEPER/ZooKeeper+and+SASL).
 
+* *DigestAuthenticationProvider.enabled* :
+    (Java system property: **zookeeper.DigestAuthenticationProvider.enabled**)
+    **New in 3.7:**
+    Determines whether the `digest` authentication provider is
+    enabled.  The default value is **true** for backwards
+    compatibility, but it may be a good idea to disable this provider
+    if not used, as it can result in misleading entries appearing in
+    audit logs
+    (see [ZOOKEEPER-3979](https://issues.apache.org/jira/browse/ZOOKEEPER-3979))
+
 * *DigestAuthenticationProvider.superDigest* :
     (Java system property: **zookeeper.DigestAuthenticationProvider.superDigest**)
     By default this feature is **disabled**

zookeeper-server/src/main/java/org/apache/zookeeper/audit/AuditHelper.java

@@ -179,7 +179,7 @@ public final class AuditHelper {
     }
 
     private static void log(Request request, String path, String op, String acls, String createMode, Result result) {
-        log(request.getUsers(), op, path, acls, createMode,
+        log(request.getUsersForAudit(), op, path, acls, createMode,
                 request.cnxn.getSessionIdHex(), request.cnxn.getHostAddress(), result);
     }
 

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

@@ -463,30 +463,20 @@ public class Request {
     }
 
     /**
-     * Returns comma separated list of users authenticated in the current
-     * session
+     * Returns a formatted, comma-separated list of the user IDs
+     * associated with this {@code Request}, or {@code null} if no
+     * user IDs were found.
+     *
+     * The return value is used for audit logging.  While it may be
+     * easy on the eyes, it is underspecified: it does not mention the
+     * corresponding {@code scheme}, nor are its components escaped.
+     * This is not a security feature.
+     *
+     * @return a comma-separated list of user IDs, or {@code null} if
+     * no user IDs were found.
      */
-    public String getUsers() {
-        if (authInfo == null) {
-            return (String) null;
-        }
-        if (authInfo.size() == 1) {
-            return AuthUtil.getUser(authInfo.get(0));
-        }
-        StringBuilder users = new StringBuilder();
-        boolean first = true;
-        for (Id id : authInfo) {
-            String user = AuthUtil.getUser(id);
-            if (user != null) {
-                if (first) {
-                    first = false;
-                } else {
-                    users.append(",");
-                }
-                users.append(user);
-            }
-        }
-        return users.toString();
+    public String getUsersForAudit() {
+        return AuthUtil.getUsers(authInfo);
     }
 
     public TxnDigest getTxnDigest() {

zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/DigestAuthenticationProvider.java

@@ -47,6 +47,8 @@ public class DigestAuthenticationProvider implements AuthenticationProvider {
         LOG.info("ACL digest algorithm is: {}", DIGEST_ALGORITHM);
     }
 
+    private static final String DIGEST_AUTH_ENABLED = "zookeeper.DigestAuthenticationProvider.enabled";
+
     /** specify a command line property with key of
      * "zookeeper.DigestAuthenticationProvider.superDigest"
      * and value of "super:<base64encoded(SHA1(password))>" to enable
@@ -54,6 +56,12 @@ public class DigestAuthenticationProvider implements AuthenticationProvider {
      */
     private static final String superDigest = System.getProperty("zookeeper.DigestAuthenticationProvider.superDigest");
 
+    public static boolean isEnabled() {
+        boolean enabled = Boolean.parseBoolean(System.getProperty(DIGEST_AUTH_ENABLED, "true"));
+        LOG.info("{} = {}", DIGEST_AUTH_ENABLED, enabled);
+        return enabled;
+    }
+
     public String getScheme() {
         return "digest";
     }

zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/ProviderRegistry.java

@@ -45,9 +45,13 @@ public class ProviderRegistry {
     public static void initialize() {
         synchronized (ProviderRegistry.class) {
             IPAuthenticationProvider ipp = new IPAuthenticationProvider();
-            DigestAuthenticationProvider digp = new DigestAuthenticationProvider();
             authenticationProviders.put(ipp.getScheme(), ipp);
-            authenticationProviders.put(digp.getScheme(), digp);
+
+            if (DigestAuthenticationProvider.isEnabled()) {
+                DigestAuthenticationProvider digp = new DigestAuthenticationProvider();
+                authenticationProviders.put(digp.getScheme(), digp);
+            }
+
             Enumeration<Object> en = System.getProperties().keys();
             while (en.hasMoreElements()) {
                 String k = (String) en.nextElement();

zookeeper-server/src/main/java/org/apache/zookeeper/server/util/AuthUtil.java

@@ -19,6 +19,7 @@ package org.apache.zookeeper.server.util;
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.stream.Collectors;
 import org.apache.zookeeper.data.ClientInfo;
 import org.apache.zookeeper.data.Id;
 import org.apache.zookeeper.server.auth.AuthenticationProvider;
@@ -40,6 +41,32 @@ public final class AuthUtil {
         return provider == null ? null : provider.getUserName(id.getId());
     }
 
+    /**
+     * Returns a formatted, comma-separated list of the user IDs held
+     * in {@code authInfo}, or {@code null} if no user IDs were found.
+     *
+     * Note that while the result may be easy on the eyes, it is
+     * underspecified: it does not mention the corresponding {@code
+     * scheme}, nor are its components escaped.  It is intended for
+     * for logging, and is not a security feature.
+     *
+     * @param authInfo A list of {@code Id} objects, or {@code null}.
+     * @return a comma-separated list of user IDs, or {@code null} if
+     * no user IDs were found.
+     */
+    public static String getUsers(List<Id> authInfo) {
+        if (authInfo == null) {
+            return null;
+        }
+
+        String formatted = authInfo.stream()
+            .map(AuthUtil::getUser)
+            .filter(name -> name != null && !name.trim().isEmpty())
+            .collect(Collectors.joining(","));
+
+        return formatted.isEmpty() ? null : formatted;
+    }
+
     /**
      * Gets user from id to prepare ClientInfo.
      *
@@ -49,7 +76,7 @@ public final class AuthUtil {
     public static List<ClientInfo> getClientInfos(List<Id> authInfo) {
         List<ClientInfo> clientAuthInfo = new ArrayList<>(authInfo.size());
         authInfo.forEach(id -> {
-            String user = AuthUtil.getUser(id);
+            String user = getUser(id);
             clientAuthInfo.add(new ClientInfo(id.getScheme(), user == null ? "" : user));
         });
         return clientAuthInfo;

zookeeper-server/src/test/java/org/apache/zookeeper/audit/Log4jAuditLoggerTest.java

@@ -301,7 +301,7 @@ public class Log4jAuditLoggerTest extends QuorumPeerTestBase {
         ServerCnxn next = getServerCnxn();
         Request request = new Request(next, -1, -1, -1, null,
                 next.getAuthInfo());
-        return request.getUsers();
+        return request.getUsersForAudit();
     }
 
     private String getIp() {

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

@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.test;
+
+import static org.junit.jupiter.api.Assertions.fail;
+import java.io.IOException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.TestableZooKeeper;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher.Event.KeeperState;
+import org.apache.zookeeper.ZooKeeper;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+public class DigestAuthDisabledTest extends ClientBase {
+
+    @BeforeAll
+    public static void setUpEnvironment() {
+        System.setProperty("zookeeper.DigestAuthenticationProvider.enabled", "false");
+    }
+
+    @AfterAll
+    public static void cleanUpEnvironment() {
+        System.clearProperty("zookeeper.DigestAuthenticationProvider.enabled");
+    }
+
+    private final CountDownLatch authFailed = new CountDownLatch(1);
+
+    @Override
+    protected TestableZooKeeper createClient(String hp) throws IOException, InterruptedException {
+        MyWatcher watcher = new MyWatcher();
+        return createClient(watcher, hp);
+    }
+
+    private class MyWatcher extends CountdownWatcher {
+
+        @Override
+        public synchronized void process(WatchedEvent event) {
+            if (event.getState() == KeeperState.AuthFailed) {
+                authFailed.countDown();
+            } else {
+                super.process(event);
+            }
+        }
+
+    }
+
+    @Test
+    public void testDigestAuthDisabledTriggersAuthFailed() throws Exception {
+        ZooKeeper zk = createClient();
+        try {
+            zk.addAuthInfo("digest", "roger:muscadet".getBytes());
+            zk.getData("/path1", false, null);
+            fail("Should get auth state error");
+        } catch (KeeperException.AuthFailedException e) {
+            if (!authFailed.await(CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS)) {
+                fail("Should have called my watcher");
+            }
+        } finally {
+            zk.close();
+        }
+    }
+}