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 <[email protected]>
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 <[email protected]>
Reviewers: Enrico Olivelli <[email protected]>, Mate Szalay-Beko
<[email protected]>
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<Object> 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<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.
*
* @param authInfo List of id objects. id contains scheme and
authentication info
@@ -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;
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();
+ }
+ }
+}
