Skip to content

Commit

Permalink
Mask secrets in CommandInvokedEvent and Command logger (#25307)
Browse files Browse the repository at this point in the history 
* Mask secrets in CommandInvokedEvent and Command logger

Extracted ExecutionContext inner class to package-private class CommandRunnerExecutionContext. Was too big and deserved a file on its own.

Moved mask method to ParameterMap and added unit tests.

A test to verfiy that the command logger doesn't log passwords
  • Loading branch information
@OndroMih
OndroMih authored Jan 15, 2025
1 parent 8de2acf commit 2f694fa
Show file tree
Hide file tree
Showing 12 changed files with 506 additions and 280 deletions.
17 changes: 2 additions & 15 deletions appserver/admingui/common/src/main/java/org/glassfish/admingui/common/util/RestUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
import java.io.IOException;
import java.net.URLEncoder;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
Expand Down Expand Up @@ -71,6 +70,7 @@
import org.xml.sax.SAXException;

import static com.sun.logging.LogCleanerUtil.neutralizeForLog;
import static org.glassfish.common.util.Constants.PASSWORD_ATTRIBUTE_NAMES;

/**
*
Expand Down Expand Up @@ -286,7 +286,7 @@ public static Map maskOffPassword(Map<String, Object> attrs) {

for (Map.Entry<String, Object> e : attrs.entrySet()) {
String key = e.getKey().toLowerCase(GuiUtil.guiLocale);
if (pswdAttrList.contains(key)) {
if (PASSWORD_ATTRIBUTE_NAMES.contains(key)) {
masked.put(e.getKey(), "*******");
} else {
masked.put(e.getKey(), e.getValue());
Expand Down Expand Up @@ -967,17 +967,4 @@ public boolean verify(String host, SSLSession sslSession) {
}
}

/*
* This is a list of attribute name of password for different command. We need to mask its value during logging.
*/
private static final List<String> pswdAttrList = Arrays.asList(
"sshpassword", /* create-node-ssh , setup-ssh , update-node, update-node-ssh */
"dbpassword", /* jms-availability-service */
"jmsdbpassword", /* configure-jms-cluster */
"password", /* change-admin-password */
"newpassword", /* change-admin-password */
"jmsdbpassword", /* configure-jms-cluster */
"mappedpassword", /* create-connector-security-map, update-connector-security-map */
"userpassword", /* create-file-user , update-file-user */
"aliaspassword" /* create-password-alias , update-password-alias */);
}
6 changes: 4 additions & 2 deletions ...er/itest-tools/src/main/java/org/glassfish/main/itest/tools/GlassFishTestEnvironment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ public class GlassFishTestEnvironment {
private static final File PASSWORD_FILE = findPasswordFile("password.txt");

private static final int ASADMIN_START_DOMAIN_TIMEOUT = 30_000;
private static final int ASADMIN_START_DOMAIN_TIMEOUT_FOR_DEBUG = 300_000;

static {
LOG.log(Level.INFO, "Using basedir: {0}", BASEDIR);
Expand All @@ -87,9 +88,11 @@ public class GlassFishTestEnvironment {
// START_DOMAIN_TIMEOUT for us waiting for the end of the asadmin start-domain process.
asadmin.withEnv("AS_START_TIMEOUT", Integer.toString(ASADMIN_START_DOMAIN_TIMEOUT - 5000));
}
final int timeout = isStartDomainSuspendEnabled()
? ASADMIN_START_DOMAIN_TIMEOUT_FOR_DEBUG : ASADMIN_START_DOMAIN_TIMEOUT;
// This is the absolutely first start - if it fails, all other starts will fail too.
// Note: --suspend implicitly enables --debug
assertThat(asadmin.exec(ASADMIN_START_DOMAIN_TIMEOUT, "start-domain",
assertThat(asadmin.exec(timeout,"start-domain",
isStartDomainSuspendEnabled() ? "--suspend" : "--debug"), asadminOK());
}

Expand Down Expand Up @@ -259,7 +262,6 @@ public static void createFileUser(String realmName, String user, String password
}
}


/**
* This will delete the jobs.xml file
*/
Expand Down
88 changes: 67 additions & 21 deletions appserver/itest-tools/src/main/java/org/glassfish/main/itest/tools/asadmin/Asadmin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,18 @@
*
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
*/

package org.glassfish.main.itest.tools.asadmin;

import com.sun.enterprise.universal.process.ProcessManager;
import com.sun.enterprise.universal.process.ProcessManagerException;
import com.sun.enterprise.universal.process.ProcessManagerTimeoutException;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.nio.file.StandardOpenOption;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
Expand All @@ -37,12 +41,12 @@
import static org.hamcrest.MatcherAssert.assertThat;

/**
* Tool for executing asadmin/asadmin.bat commands.
* The tool is stateless.
* Tool for executing asadmin/asadmin.bat commands. The tool is stateless.
*
* @author David Matejcek
*/
public class Asadmin {

private static final Logger LOG = Logger.getLogger(Asadmin.class.getName());

private static final int DEFAULT_TIMEOUT_MSEC = 30 * 1000;
Expand All @@ -59,26 +63,27 @@ public class Asadmin {
private final File adminPasswordFile;
private final boolean terse;
private final Map<String, String> environment = new HashMap<>();

private final Map<String, String> additionalPasswords = new HashMap<>();

/**
* Creates a stateless instance of the tool.
*
* @param asadmin - executable file
* @param adminUser - username authorized to use the domain
* @param adminPasswordFile - a file containing admin's password set as <code>AS_ADMIN_PASSWORD=...</code>
* @param adminPasswordFile - a file containing admin's password set as
* <code>AS_ADMIN_PASSWORD=...</code>
*/
public Asadmin(final File asadmin, final String adminUser, final File adminPasswordFile) {
this(asadmin, adminUser, adminPasswordFile, false);
}


/**
* Creates a stateless instance of the tool.
*
* @param asadmin - executable file
* @param adminUser - username authorized to use the domain
* @param adminPasswordFile - a file containing admin's password set as <code>AS_ADMIN_PASSWORD=...</code>
* @param adminPasswordFile - a file containing admin's password set as
* <code>AS_ADMIN_PASSWORD=...</code>
* @param terse - to produce output, minimized and suitable for parsing.
*/
public Asadmin(final File asadmin, final String adminUser, final File adminPasswordFile, final boolean terse) {
Expand All @@ -88,7 +93,6 @@ public Asadmin(final File asadmin, final String adminUser, final File adminPassw
this.terse = terse;
}


/**
* Adds environment property set for the asadmin execution.
*
Expand All @@ -101,6 +105,27 @@ public Asadmin withEnv(final String name, final String value) {
return this;
}

/**
* Adds a password to the password file.
*
* @param name Name in the password file
* @param secretValue Value in the password file
* @return this
*/
public Asadmin withPassword(final String name, final String secretValue) {
this.additionalPasswords.put(name, secretValue);
return this;
}

/**
* Removes all custom passwords.
*
* @return this
*/
public Asadmin resetPasswords() {
this.additionalPasswords.clear();
return this;
}

/**
* @return asadmin command file name
Expand All @@ -109,7 +134,6 @@ public String getCommandName() {
return asadmin.getName();
}


public <T> KeyAndValue<T> getValue(final String key, final Function<String, T> transformer) {
List<KeyAndValue<T>> result = get(key, transformer);
if (result.isEmpty()) {
Expand All @@ -121,20 +145,19 @@ public <T> KeyAndValue<T> getValue(final String key, final Function<String, T> t
return result.get(0);
}


public <T> List<KeyAndValue<T>> get(final String key, final Function<String, T> transformer) {
AsadminResult result = exec("get", key);
assertThat(result, asadminOK());
return Arrays.stream(result.getStdOut().split(System.lineSeparator())).map(KEYVAL_SPLITTER)
.filter(Objects::nonNull).map(kv -> new KeyAndValue<>(kv.getKey(), transformer.apply(kv.getValue())))
.collect(Collectors.toList());
.filter(Objects::nonNull).map(kv -> new KeyAndValue<>(kv.getKey(), transformer.apply(kv.getValue())))
.collect(Collectors.toList());
}


/**
* Executes the command with arguments asynchronously with {@value #DEFAULT_TIMEOUT_MSEC} ms timeout.
* The command can be attached by the attach command.
* You should find the job id in the {@link AsadminResult#getStdOut()} as <code>Job ID: [0-9]+</code>
* Executes the command with arguments asynchronously with
* {@value #DEFAULT_TIMEOUT_MSEC} ms timeout. The command can be attached by
* the attach command. You should find the job id in the
* {@link AsadminResult#getStdOut()} as <code>Job ID: [0-9]+</code>
*
* @param args
* @return {@link AsadminResult} never null.
Expand All @@ -144,16 +167,19 @@ public DetachedTerseAsadminResult execDetached(final String... args) {
}

/**
* Executes the command with arguments synchronously with {@value #DEFAULT_TIMEOUT_MSEC} ms timeout.
* Executes the command with arguments synchronously with
* {@value #DEFAULT_TIMEOUT_MSEC} ms timeout.
*
* @param args
* @return {@link AsadminResult} never null.
*/
public AsadminResult exec(final String... args) {
return exec(DEFAULT_TIMEOUT_MSEC, false, args);
}

/**
* Executes the command with arguments synchronously with given timeout in millis.
* Executes the command with arguments synchronously with given timeout in
* millis.
*
* @param timeout timeout in millis
* @param args command and arguments.
Expand All @@ -163,24 +189,43 @@ public AsadminResult exec(final int timeout, final String... args) {
return exec(timeout, false, args);
}

private File getPasswordFile() {
try {
if (!additionalPasswords.isEmpty()) {
final Path tempPasswordFile = Files.createTempFile("pwd", "txt");
Files.copy(adminPasswordFile.toPath(), tempPasswordFile, StandardCopyOption.REPLACE_EXISTING);
String additionalContent = additionalPasswords.entrySet().stream()
.map(entry -> entry.getKey() + "=" + entry.getValue())
.collect(Collectors.joining("\n"));
Files.writeString(tempPasswordFile, "\n" + additionalContent, StandardOpenOption.APPEND);
return tempPasswordFile.toFile();
}
return adminPasswordFile;
} catch (IOException e) {
throw new IllegalStateException("Could not create the temporary password file.", e);
}

}

/**
* Executes the command with arguments.
*
* @param timeout timeout in millis
* @param detachedAndTerse detached command is executed asynchronously, can be attached later by the attach command.
* @param detachedAndTerse detached command is executed asynchronously, can
* be attached later by the attach command.
* @param args command and arguments.
* @return {@link AsadminResult} never null.
*/
private AsadminResult exec(final int timeout, final boolean detachedAndTerse, final String... args) {
final List<String> parameters = Arrays.asList(args);
LOG.log(Level.INFO, "exec(timeout={0}, detached={1}, args={2})",
new Object[] {timeout, detachedAndTerse, parameters});
new Object[]{timeout, detachedAndTerse, parameters});
final List<String> command = new ArrayList<>();
command.add(asadmin.getAbsolutePath());
command.add("--user");
command.add(adminUser);
command.add("--passwordfile");
command.add(adminPasswordFile.getAbsolutePath());
command.add(getPasswordFile().getAbsolutePath());
if (detachedAndTerse) {
command.add("--terse=true");
command.add("--detach");
Expand Down Expand Up @@ -230,4 +275,5 @@ private AsadminResult exec(final int timeout, final boolean detachedAndTerse, fi
}
return result;
}

}
33 changes: 25 additions & 8 deletions ...logger/src/test/java/org/glassfish/main/test/extras/commandlogger/CommandLoggerITest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 */
package org.glassfish.main.test.extras.commandlogger;

import com.google.common.collect.Streams;

import static org.glassfish.main.itest.tools.GlassFishTestEnvironment.getAsadmin;
import static org.glassfish.main.itest.tools.asadmin.AsadminResultMatcher.asadminOK;
import static org.hamcrest.CoreMatchers.allOf;
Expand All @@ -24,12 +26,18 @@
import static org.hamcrest.Matchers.not;

import java.io.IOException;
import java.util.Arrays;
import java.util.List;

import org.glassfish.main.itest.tools.asadmin.Asadmin;
import org.glassfish.main.itest.tools.asadmin.CollectLogFiles;
import org.hamcrest.CoreMatchers;
import org.hamcrest.Matcher;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;

import static java.util.stream.Stream.of;

/**
*
* @author Ondro Mihalyi
Expand Down Expand Up @@ -58,7 +66,11 @@ public void testLogWriteCommands(String logMode, boolean logWriteOp, boolean log
clearLogFile();

// execute some write command, it doesn't have to complete successfully
ASADMIN.exec("delete-system-property", "X");
ASADMIN.exec("delete-system-property", "propertyX");
ASADMIN.withPassword("AS_ADMIN_ALIASPASSWORD", "secretPassword")
.exec("create-password-alias", "mytestalias");
ASADMIN.resetPasswords();
ASADMIN.exec("delete-password-alias", "mytestalias");
// execute some read command
ASADMIN.exec("list-applications", "--long");
// exxecute some internal command
Expand All @@ -68,14 +80,17 @@ public void testLogWriteCommands(String logMode, boolean logWriteOp, boolean log
.collect()
.getServerLogLines();

assertCommandNotLogged(lines, "secretPassword");
if (logWriteOp) {
assertCommandLogged(lines, "admin", "delete-system-property X");
assertCommandLogged(lines, "admin", "delete-system-property", "propertyX");
assertCommandLogged(lines, "admin", "create-password-alias", "mytestalias");
} else {
assertCommandNotLogged(lines, "delete-system-property");
assertCommandNotLogged(lines, "create-password-alias");
}

if (logReadOp) {
assertCommandLogged(lines, "admin", "list-applications --long");
assertCommandLogged(lines, "admin", "list-applications", "--long");
} else {
assertCommandNotLogged(lines, "list-applications");
}
Expand All @@ -96,11 +111,13 @@ private void assertCommandNotLogged(final List<String> lines, String command) {
assertThat("log", lines, everyItem(not(containsString(command))));
}

private void assertCommandLogged(final List<String> lines, String user, String fullCommand) {
assertThat("server.log", lines, hasItem(allOf(containsString(user),
containsString(fullCommand)
)
));
private void assertCommandLogged(final List<String> lines, String user, String... commandParts) {
final Matcher[] containsAllStrings = Streams.concat(of(containsString(user)),
Arrays.stream(commandParts)
.map(CoreMatchers::containsString))
.toArray(Matcher[]::new);

assertThat("server.log", lines, hasItem(allOf(containsAllStrings)));
}

}
15 changes: 15 additions & 0 deletions nucleus/common/common-util/src/main/java/org/glassfish/common/util/Constants.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package org.glassfish.common.util;

import java.util.Set;

/**
* These are constants that can be used throughout the server
*
Expand All @@ -37,4 +39,17 @@ public class Constants {
*/
public final static int IMPORTANT_RUN_LEVEL_SERVICE = 50;

/*
* This is a list of attribute names that hold passwords for various admin commands. We need to mask their value during logging.
*/
public final static Set<String> PASSWORD_ATTRIBUTE_NAMES = Set.of(
"sshpassword", /* create-node-ssh , setup-ssh , update-node, update-node-ssh */
"dbpassword", /* jms-availability-service */
"password", /* change-admin-password */
"newpassword", /* change-admin-password */
"jmsdbpassword", /* configure-jms-cluster */
"mappedpassword", /* create-connector-security-map, update-connector-security-map */
"userpassword", /* create-file-user , update-file-user */
"aliaspassword" /* create-password-alias , update-password-alias */
);
}
Loading

0 comments on commit 2f694fa

Please sign in to comment.