This is an automated email from the ASF dual-hosted git repository. symat pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/zookeeper.git The following commit(s) were added to refs/heads/master by this push: new d8561f6 ZOOKEEPER-3979: Clean up/robustify audit logs d8561f6 is described below commit d8561f620fa8611e9a6819d9879b0f18e5a404a9 Author: Damien Diederen AuthorDate: Mon Nov 16 08:03:48 2020 +0000 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 Reviewers: Enrico Olivelli , Mate Szalay-Beko Closes #1533 from ztzg/ZOOKEEPER-3979-robustify-audit-logs --- .../src/main/resources/markdown/zookeeperAdmin.md | 10 +++ .../org/apache/zookeeper/audit/AuditHelper.java | 2 +- .../java/org/apache/zookeeper/server/Request.java | 36 ++++------ .../server/auth/DigestAuthenticationProvider.java | 8 +++ .../zookeeper/server/auth/ProviderRegistry.java | 8 ++- .../org/apache/zookeeper/server/util/AuthUtil.java | 29 +++++++- .../zookeeper/audit/Log4jAuditLoggerTest.java | 2 +- .../zookeeper/test/DigestAuthDisabledTest.java | 82 ++++++++++++++++++++++ 8 files changed, 149 insertions(+), 28 deletions(-) diff --git a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md b/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md index a14559e..7dc9063 100644 --- a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md +++ b/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** diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/audit/AuditHelper.java b/zookeeper-server/src/main/java/org/apache/zookeeper/audit/AuditHelper.java index ce0d58a..ec4ca13 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/audit/AuditHelper.java +++ b/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); } diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java index 0a8bde6..a68203b 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java +++ b/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() { diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/DigestAuthenticationProvider.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/DigestAuthenticationProvider.java index 8b64154..6e26875 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/DigestAuthenticationProvider.java +++ b/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"; } diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/ProviderRegistry.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/ProviderRegistry.java index 856cf78..3315e44 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/ProviderRegistry.java +++ b/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 en = System.getProperties().keys(); while (en.hasMoreElements()) { String k = (String) en.nextElement(); diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/util/AuthUtil.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/util/AuthUtil.java index 7e5ca4f..f404e25 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/util/AuthUtil.java +++ b/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; @@ -41,6 +42,32 @@ public final class AuthUtil { } /** + * 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 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. * * @param authInfo List of id objects. id contains scheme and authentication info @@ -49,7 +76,7 @@ public final class AuthUtil { public static List getClientInfos(List authInfo) { List 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; diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/audit/Log4jAuditLoggerTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/audit/Log4jAuditLoggerTest.java index 8a5f781..0080b01 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/audit/Log4jAuditLoggerTest.java +++ b/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() { diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/DigestAuthDisabledTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/DigestAuthDisabledTest.java new file mode 100644 index 0000000..bbbc63f --- /dev/null +++ b/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(); + } + } +}