GEODE-9805: Do not log arguments of Radish AUTH command (apache#7094)

- Rather than returning the arguments of the AUTH command, only return how many arguments there are when toString() is called Authored-by: Donal Evans <[email protected]>
ringles · Nov 9, 2021 · 0cb1736 · 0cb1736
1 parent 9f41e48
commit 0cb1736
Show file tree

Hide file tree

Showing 4 changed files with 140 additions and 14 deletions.
diff --git a/...ionTest/java/org/apache/geode/redis/internal/executor/connection/AuthIntegrationTest.java b/...ionTest/java/org/apache/geode/redis/internal/executor/connection/AuthIntegrationTest.java
@@ -17,17 +17,26 @@
 package org.apache.geode.redis.internal.executor.connection;
 
 import static org.apache.geode.distributed.ConfigurationProperties.LOCATORS;
+import static org.apache.geode.distributed.ConfigurationProperties.LOG_FILE;
 import static org.apache.geode.distributed.ConfigurationProperties.LOG_LEVEL;
 import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static org.apache.geode.redis.internal.RedisConstants.SERVER_ERROR_MESSAGE;
 import static org.apache.geode.redis.internal.RedisProperties.REDIS_REGION_NAME_PROPERTY;
 import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.util.Properties;
+import java.util.Scanner;
+
 import org.junit.After;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+import org.junit.rules.TemporaryFolder;
 import redis.clients.jedis.Jedis;
 
 import org.apache.geode.cache.CacheFactory;
@@ -39,6 +48,10 @@
 import org.apache.geode.internal.cache.InternalCache;
 import org.apache.geode.redis.internal.GeodeRedisServer;
 import org.apache.geode.redis.internal.RedisConstants;
+import org.apache.geode.security.AuthenticationExpiredException;
+import org.apache.geode.security.AuthenticationFailedException;
+import org.apache.geode.security.ResourcePermission;
+import org.apache.geode.security.SecurityManager;
 
 public class AuthIntegrationTest extends AbstractAuthIntegrationTest {
 
@@ -50,6 +63,10 @@ public class AuthIntegrationTest extends AbstractAuthIntegrationTest {
   @Rule
   public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
 
+  @Rule
+  public TemporaryFolder temporaryFolder = new TemporaryFolder();
+  public static final String LOGGED_PASSWORD = "aStringThatShouldNotAppearInTheLogs";
+
   @After
   public void tearDown() {
     server.shutdown();
@@ -94,7 +111,6 @@ private void setupCache(String username, boolean withSecurityManager) throws Exc
      * setting this value.
      */
     System.setProperty("io.netty.eventLoopThreads", "1");
-    port = AvailablePortHelper.getRandomAvailableTCPPort();
     CacheFactory cf = new CacheFactory();
     cf.set(LOG_LEVEL, "error");
     cf.set(MCAST_PORT, "0");
@@ -106,6 +122,7 @@ private void setupCache(String username, boolean withSecurityManager) throws Exc
       cf.set(ConfigurationProperties.SECURITY_MANAGER, SimpleSecurityManager.class.getName());
     }
     cache = cf.create();
+    port = AvailablePortHelper.getRandomAvailableTCPPort();
     server = new GeodeRedisServer("localhost", port, (InternalCache) cache);
     server.getRegionProvider().getSlotAdvisor().getBucketSlots();
     this.jedis = new Jedis("localhost", port, 100000);
@@ -227,4 +244,84 @@ public void givenSecurityWithWritePermission_getCommandFails() throws Exception
     assertThatThrownBy(() -> jedis.get("foo"))
         .hasMessageContaining(RedisConstants.ERROR_NOT_AUTHORIZED);
   }
+
+  @Test
+  public void givenSecurity_authCommandPasswordIsNotLoggedAtDebugLevel() throws IOException {
+    File logFile = temporaryFolder.newFile();
+
+    createRadishServerWithLogFileAndSecurityManager(logFile, "fine", NoOpSecurityManager.class);
+
+    jedis.auth(LOGGED_PASSWORD);
+
+    checkLogFileForPassword(logFile, LOGGED_PASSWORD);
+  }
+
+  @Test
+  public void givenSecurity_authCommandPasswordIsNotLoggedOnException() throws IOException {
+    File logFile = temporaryFolder.newFile();
+
+    createRadishServerWithLogFileAndSecurityManager(logFile, "info",
+        ThrowsOnAuthorizeSecurityManager.class);
+
+    // The first AUTH command will authenticate the user, and the second will cause the authorize()
+    // method on the ThrowsOnAuthorizeSecurityManager to be invoked, throwing an exception
+    jedis.auth(LOGGED_PASSWORD);
+    assertThatThrownBy(() -> jedis.auth(LOGGED_PASSWORD))
+        .hasMessageContaining(SERVER_ERROR_MESSAGE);
+
+    checkLogFileForPassword(logFile, LOGGED_PASSWORD);
+  }
+
+  private void createRadishServerWithLogFileAndSecurityManager(File logFile, String logLevel,
+      Class<?> securityManager) {
+    cache = new CacheFactory()
+        .set(LOG_FILE, logFile.getAbsolutePath())
+        .set(LOG_LEVEL, logLevel)
+        .set(MCAST_PORT, "0")
+        .set(LOCATORS, "")
+        .set(ConfigurationProperties.SECURITY_MANAGER, securityManager.getName())
+        .create();
+    port = AvailablePortHelper.getRandomAvailableTCPPort();
+    server = new GeodeRedisServer("localhost", port, (InternalCache) cache);
+    jedis = new Jedis("localhost", port, 100000);
+  }
+
+  private void checkLogFileForPassword(File logFile, String password) throws FileNotFoundException {
+    Scanner scanner = new Scanner(logFile);
+    boolean loggedAUTH = false;
+    while (scanner.hasNextLine()) {
+      String line = scanner.nextLine();
+      assertThat(line)
+          .as("Log file should not contain password")
+          .doesNotContain(password);
+      if (line.contains("AUTH")) {
+        loggedAUTH = true;
+      }
+    }
+    assertThat(loggedAUTH).as("Expected to log that an AUTH command was executed").isTrue();
+  }
+
+  public static class NoOpSecurityManager implements SecurityManager {
+
+    @Override
+    public Object authenticate(Properties credentials)
+        throws AuthenticationFailedException, AuthenticationExpiredException {
+      return credentials.getProperty(USER_NAME);
+    }
+  }
+
+  public static class ThrowsOnAuthorizeSecurityManager implements SecurityManager {
+
+    @Override
+    public Object authenticate(Properties credentials)
+        throws AuthenticationFailedException, AuthenticationExpiredException {
+      return credentials.getProperty(USER_NAME);
+    }
+
+    @Override
+    public boolean authorize(Object principal, ResourcePermission permission)
+        throws AuthenticationExpiredException {
+      throw new RuntimeException();
+    }
+  }
 }
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/Command.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/Command.java
@@ -169,12 +169,16 @@ public RedisKey getKey() {
 
   @Override
   public String toString() {
-    StringBuilder b = new StringBuilder();
-    for (byte[] rawCommand : this.commandElems) {
-      b.append(getHexEncodedString(rawCommand));
-      b.append(' ');
+    if (commandType.equals(RedisCommandType.AUTH)) {
+      return "AUTH command with " + (commandElems.size() - 1) + " argument(s)";
+    } else {
+      StringBuilder b = new StringBuilder();
+      for (byte[] rawCommand : this.commandElems) {
+        b.append(getHexEncodedString(rawCommand));
+        b.append(' ');
+      }
+      return b.toString();
     }
-    return b.toString();
   }
 
   public static String getHexEncodedString(byte[] data) {

diff --git a/...or-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java b/...or-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
@@ -314,8 +314,7 @@ public RegionProvider getRegionProvider() {
 
   /**
    * Get the default username. This is the username that will be passed to the
-   * {@link SecurityManager} in response to
-   * an {@code AUTH password} command.
+   * {@link SecurityManager} in response to an {@code AUTH password} command.
    */
   public String getRedisUsername() {
     return redisUsername;

diff --git a/geode-for-redis/src/test/java/org/apache/geode/redis/internal/netty/CommandJUnitTest.java b/geode-for-redis/src/test/java/org/apache/geode/redis/internal/netty/CommandJUnitTest.java
@@ -21,17 +21,15 @@
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 
 import org.junit.Test;
 
 import org.apache.geode.redis.internal.RedisCommandType;
 
-
 /**
  * Test case for the command
- *
- *
  */
 public class CommandJUnitTest {
 
@@ -44,12 +42,12 @@ public void testCommand() {
     assertThatThrownBy(() -> new Command(list1))
         .hasMessageContaining("List of command elements cannot be empty");
 
-    List<byte[]> list2 = new ArrayList<byte[]>();
+    List<byte[]> list2 = new ArrayList<>();
 
     assertThatThrownBy(() -> new Command(list2))
         .hasMessageContaining("List of command elements cannot be empty");
 
-    List<byte[]> list3 = new ArrayList<byte[]>();
+    List<byte[]> list3 = new ArrayList<>();
     list3.add(stringToBytes("Garbage"));
 
     Command cmd = new Command(list3);
@@ -72,7 +70,7 @@ public void testCommand() {
 
   @Test
   public void verifyGetCommandArguments() {
-    Command cmd = new Command(Arrays.asList(stringToBytes("cmd")));
+    Command cmd = new Command(Collections.singletonList(stringToBytes("cmd")));
     assertThat(cmd.getCommandArguments()).isEmpty();
 
     cmd = new Command(Arrays.asList(stringToBytes("cmd"), stringToBytes("arg1")));
@@ -84,4 +82,32 @@ public void verifyGetCommandArguments() {
         stringToBytes("arg2"));
 
   }
+
+  @Test
+  public void toStringForAUTHCommandDoesNotReturnArguments() {
+    String password = "password";
+    byte[] authBytes = stringToBytes(RedisCommandType.AUTH.name());
+    byte[] passwordBytes = stringToBytes(password);
+
+    Command authCommandWithOneArgument = new Command(Arrays.asList(authBytes, passwordBytes));
+    assertThat(authCommandWithOneArgument.toString()).doesNotContain(password);
+
+    Command authCommandWithTwoArguments =
+        new Command(Arrays.asList(authBytes, passwordBytes, passwordBytes));
+    assertThat(authCommandWithTwoArguments.toString()).doesNotContain(password);
+  }
+
+  @Test
+  public void toStringForAUTHCommandReturnsNumberOfArguments() {
+    String password = "password";
+    byte[] authBytes = stringToBytes(RedisCommandType.AUTH.name());
+    byte[] passwordBytes = stringToBytes(password);
+
+    Command authCommandWithOneArgument = new Command(Arrays.asList(authBytes, passwordBytes));
+    assertThat(authCommandWithOneArgument.toString()).contains("AUTH command with 1 argument(s)");
+
+    Command authCommandWithTwoArguments =
+        new Command(Arrays.asList(authBytes, passwordBytes, passwordBytes));
+    assertThat(authCommandWithTwoArguments.toString()).contains("AUTH command with 2 argument(s)");
+  }
 }