geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jensde...@apache.org
Subject [geode] branch develop updated: GEODE-4318: Ensure that passwords are correctly redacted in the gfsh history file (#3372)
Date Fri, 29 Mar 2019 13:08:55 GMT
This is an automated email from the ASF dual-hosted git repository.

jensdeppe pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/develop by this push:
     new a0e3a45  GEODE-4318: Ensure that passwords are correctly redacted in the gfsh history
file (#3372)
a0e3a45 is described below

commit a0e3a45d2664a74f21506de9be2f6402ce5e19cf
Author: Jens Deppe <jdeppe@pivotal.io>
AuthorDate: Fri Mar 29 06:08:40 2019 -0700

    GEODE-4318: Ensure that passwords are correctly redacted in the gfsh history file (#3372)
    
    - Remove redact method in GfshHistory in favor of
      ArgumentRedactor.redact
    
    Co-authored-by: Jens Deppe <jdeppe@pivotal.io>
    Co-authored-by: Donal Evans <donalevans86@gmail.com>
---
 .../commands/HistoryCommandIntegrationTest.java    | 39 ++++++++++++++++++++--
 .../internal/cli/shell/GfshHistoryJUnitTest.java   |  2 +-
 .../geode/internal/util/ArgumentRedactor.java      |  2 +-
 .../geode/management/internal/cli/Launcher.java    |  4 +--
 .../geode/management/internal/cli/shell/Gfsh.java  |  8 ++---
 .../internal/cli/shell/jline/GfshHistory.java      | 18 ++--------
 6 files changed, 48 insertions(+), 25 deletions(-)

diff --git a/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/commands/HistoryCommandIntegrationTest.java
b/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/commands/HistoryCommandIntegrationTest.java
index b7afb54..88f0e72 100644
--- a/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/commands/HistoryCommandIntegrationTest.java
+++ b/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/commands/HistoryCommandIntegrationTest.java
@@ -19,9 +19,12 @@ import static org.assertj.core.api.Assertions.assertThat;
 
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.Files;
+import java.util.List;
 
 import org.junit.After;
 import org.junit.ClassRule;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 
@@ -33,8 +36,8 @@ public class HistoryCommandIntegrationTest {
   @ClassRule
   public static GfshCommandRule gfsh = new GfshCommandRule();
 
-  @ClassRule
-  public static TemporaryFolder temporaryFolder = new TemporaryFolder();
+  @Rule
+  public TemporaryFolder temporaryFolder = new TemporaryFolder();
 
   @After
   public void tearDown() throws Exception {
@@ -83,4 +86,36 @@ public class HistoryCommandIntegrationTest {
     // only the history --clear is in the history now.
     assertThat(gfsh.getGfsh().getGfshHistory().size()).isEqualTo(1);
   }
+
+  @Test
+  public void testHistoryContainsRedactedPasswordWithEquals() throws IOException {
+    gfsh.executeCommand("connect --password=redacted");
+    File historyFile = temporaryFolder.newFile("history.txt");
+    historyFile.delete();
+    assertThat(historyFile).doesNotExist();
+
+    String command = "history --file=" + historyFile.getAbsolutePath();
+    gfsh.executeAndAssertThat(command).statusIsSuccess();
+
+    assertThat(historyFile).exists();
+
+    List<String> historyLines = Files.readAllLines(historyFile.toPath());
+    assertThat(historyLines.get(0)).isEqualTo("0: connect --password=********");
+  }
+
+  @Test
+  public void testHistoryContainsRedactedPasswordWithoutEquals() throws IOException {
+    gfsh.executeCommand("connect --password redacted");
+    File historyFile = temporaryFolder.newFile("history.txt");
+    historyFile.delete();
+    assertThat(historyFile).doesNotExist();
+
+    String command = "history --file=" + historyFile.getAbsolutePath();
+    gfsh.executeAndAssertThat(command).statusIsSuccess();
+
+    assertThat(historyFile).exists();
+
+    List<String> historyLines = Files.readAllLines(historyFile.toPath());
+    assertThat(historyLines.get(0)).isEqualTo("0: connect --password ********");
+  }
 }
diff --git a/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/shell/GfshHistoryJUnitTest.java
b/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/shell/GfshHistoryJUnitTest.java
index f1899dd..70d9f17 100644
--- a/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/shell/GfshHistoryJUnitTest.java
+++ b/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/shell/GfshHistoryJUnitTest.java
@@ -75,7 +75,7 @@ public class GfshHistoryJUnitTest {
     gfsh.executeScriptLine("connect --password=foo");
 
     List<String> lines = Files.readAllLines(gfshHistoryFile.toPath());
-    assertEquals("connect --password=*****", lines.get(1));
+    assertEquals("connect --password=********", lines.get(1));
   }
 
   @Test
diff --git a/geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java
b/geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java
index 2120478..172d449 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java
@@ -190,7 +190,7 @@ public class ArgumentRedactor {
   /**
    * Determine whether a option's argument should be redacted.
    *
-   * @param option The option option in question.
+   * @param option The option in question.
    *
    * @return true if the value should be redacted, otherwise false.
    */
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/Launcher.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/Launcher.java
index 49c35c9..6ada088 100755
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/Launcher.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/Launcher.java
@@ -28,10 +28,10 @@ import org.apache.geode.internal.ExitCode;
 import org.apache.geode.internal.GemFireVersion;
 import org.apache.geode.internal.PureJavaMode;
 import org.apache.geode.internal.lang.StringUtils;
+import org.apache.geode.internal.util.ArgumentRedactor;
 import org.apache.geode.management.internal.cli.i18n.CliStrings;
 import org.apache.geode.management.internal.cli.shell.Gfsh;
 import org.apache.geode.management.internal.cli.shell.GfshConfig;
-import org.apache.geode.management.internal.cli.shell.jline.GfshHistory;
 
 /**
  * Launcher class for :
@@ -228,7 +228,7 @@ public class Launcher {
             String command = commandsToExecute.get(i);
             // sanitize the output string to not show the password
             System.out.println(GfshParser.LINE_SEPARATOR + "(" + (i + 1) + ") Executing -
"
-                + GfshHistory.redact(command) + GfshParser.LINE_SEPARATOR);
+                + ArgumentRedactor.redact(command) + GfshParser.LINE_SEPARATOR);
             if (!gfsh.executeScriptLine(command) || gfsh.getLastExecutionStatus() != 0) {
               exitRequest = ExitShellRequest.FATAL_EXIT;
             }
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
index 3fbee6c..ccc6e8d 100755
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
@@ -778,11 +778,11 @@ public class Gfsh extends JLineShell {
     String originalString = expandedPropCommandsMap.get(processedLine);
     if (originalString != null) {
       // In history log the original command string & expanded line as a comment
-      super.logCommandToOutput(GfshHistory.redact(originalString));
-      super.logCommandToOutput(GfshHistory.redact("// Post substitution"));
-      super.logCommandToOutput(GfshHistory.redact("//" + processedLine));
+      super.logCommandToOutput(ArgumentRedactor.redact(originalString));
+      super.logCommandToOutput(ArgumentRedactor.redact("// Post substitution"));
+      super.logCommandToOutput(ArgumentRedactor.redact("//" + processedLine));
     } else {
-      super.logCommandToOutput(GfshHistory.redact(processedLine));
+      super.logCommandToOutput(ArgumentRedactor.redact(processedLine));
     }
   }
 
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/jline/GfshHistory.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/jline/GfshHistory.java
index 1725757..635bae3 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/jline/GfshHistory.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/jline/GfshHistory.java
@@ -14,11 +14,10 @@
  */
 package org.apache.geode.management.internal.cli.shell.jline;
 
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
-
 import jline.console.history.MemoryHistory;
 
+import org.apache.geode.internal.util.ArgumentRedactor;
+
 /**
  * Overrides jline.History to add History without newline characters.
  *
@@ -26,23 +25,12 @@ import jline.console.history.MemoryHistory;
  */
 public class GfshHistory extends MemoryHistory {
 
-  // Pattern which is intended to pick up any params containing the word 'password'.
-  private static final Pattern passwordRe =
-      Pattern.compile("(--[^=\\s]*password[^=\\s]*\\s*=\\s*)([^\\s]*)");
-
   // let the history from history file get added initially
   private boolean autoFlush = true;
 
-  public static String redact(String buffer) {
-    String trimmed = buffer.trim();
-    Matcher matcher = passwordRe.matcher(trimmed);
-    String sanitized = matcher.replaceAll("$1*****");
-    return sanitized;
-  }
-
   public void addToHistory(String buffer) {
     if (isAutoFlush()) {
-      super.add(redact(buffer));
+      super.add(ArgumentRedactor.redact(buffer.trim()));
     }
   }
 


Mime
View raw message