ZOOKEEPER-5058: Remove special characters from ensemble name before logging in EnsembleAuthenticationProvider

conf/logback.xml

@@ -31,13 +31,16 @@
   <property name="zookeeper.log.maxfilesize" value="256MB" />
   <property name="zookeeper.log.maxbackupindex" value="20" />
 
+  <conversionRule conversionWord="crlf"
+                  converterClass="org.owasp.security.logging.mask.CRLFConverter" />
+
   <!--
     console
     Add "console" to root logger if you want to use this
   -->
   <appender name="CONSOLE" class="ch.qos.logback.core.ConsoleAppender">
     <encoder>
-      <pattern>%d{ISO8601} [myid:%X{myid}] - %-5p [%t:%C{1}@%L] - %m%n</pattern>
+      <pattern>%d{ISO8601} [myid:%X{myid}] - %-5p [%t:%C{1}@%L] - %crlf(%msg) %n</pattern>
     </encoder>
     <filter class="ch.qos.logback.classic.filter.ThresholdFilter">
       <level>${zookeeper.console.threshold}</level>

pom.xml

@@ -667,6 +667,11 @@
         <artifactId>logback-classic</artifactId>
         <version>${logback-version}</version>
       </dependency>
+      <dependency>
+        <groupId>org.owasp</groupId>
+        <artifactId>security-logging-logback</artifactId>
+        <version>1.1.6</version>
+      </dependency>
       <dependency>
         <groupId>org.jmockit</groupId>
         <artifactId>jmockit</artifactId>

zookeeper-server/pom.xml

@@ -85,6 +85,12 @@
       <scope>runtime</scope>
       <optional>true</optional>
     </dependency>
+    <dependency>
+      <groupId>org.owasp</groupId>
+      <artifactId>security-logging-logback</artifactId>
+      <scope>runtime</scope>
+      <optional>true</optional>
+    </dependency>
     <dependency>
       <groupId>org.eclipse.jetty</groupId>
       <artifactId>jetty-server</artifactId>

zookeeper-server/src/test/java/org/apache/zookeeper/server/auth/EnsembleAuthenticationProviderTest.java

@@ -0,0 +1,64 @@
+/*
+ * 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.server.auth;
+
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import ch.qos.logback.classic.Level;
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.charset.StandardCharsets;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.server.ServerCnxn;
+import org.apache.zookeeper.test.LoggerTestTool;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+public class EnsembleAuthenticationProviderTest {
+    private static LoggerTestTool loggerTestTool;
+
+    @BeforeAll
+    public static void setupBeforeClass() {
+        loggerTestTool = new LoggerTestTool(EnsembleAuthenticationProvider.class, Level.INFO);
+    }
+
+    @AfterAll
+    public static void tearDownAfterClass() throws Exception {
+        loggerTestTool.close();
+    }
+
+    @Test
+    public void testLogForgeryWithSpecialCharacters() throws IOException {
+        ServerCnxn mockServerCnxn = mock(ServerCnxn.class);
+        InetSocketAddress mockAddress = new InetSocketAddress("127.0.0.1", 1234);
+        doReturn(mockAddress).when(mockServerCnxn).getRemoteSocketAddress();
+
+        EnsembleAuthenticationProvider provider = new EnsembleAuthenticationProvider();
+        provider.setEnsembleNames("test-ensemble");
+
+        byte[] authData = "andor-ensemble\nTHIS SHOULD\t NOT\r BE HERE".getBytes(StandardCharsets.UTF_8);
+        KeeperException.Code err = provider.handleAuthentication(mockServerCnxn, authData);
+        String logLine = loggerTestTool.readLogLine("andor-ensemble");
+        Assertions.assertFalse(logLine.contains("\n"), "Log line should not contain newline");
+        Assertions.assertFalse(logLine.contains("\r"), "Log line should not contain carriage return");
+
+        Assertions.assertEquals(KeeperException.Code.BADARGUMENTS, err);
+    }
+}

zookeeper-server/src/test/resources/logback.xml

@@ -20,10 +20,12 @@
  Define some default values that can be overridden by system properties
 -->
 <configuration>
+  <conversionRule conversionWord="crlf"
+                  converterClass="org.owasp.security.logging.mask.CRLFConverter" />
 
   <appender name="CONSOLE" class="ch.qos.logback.core.ConsoleAppender">
     <encoder>
-      <pattern>%d{ISO8601} [myid:%X{myid}] - %-5p [%t:%C{1}@%L] - %m%n</pattern>
+      <pattern>%d{ISO8601} [myid:%X{myid}] - %-5p [%t:%C{1}@%L] - %crlf(%msg) %n</pattern>
     </encoder>
     <filter class="ch.qos.logback.classic.filter.ThresholdFilter">
       <level>INFO</level>