Skip to content

Commit 4f45d34

Browse files
YLChen-007cyl-authCopilotDaanHoogland
authored and
dhslove
committed
fix that log sensitive infomation in cmd of script (apache#12024)
* fix that log sensitive infomation in cmd of script * Remove unnecessary line break in Script.java * Update utils/src/main/java/com/cloud/utils/script/Script.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Refactor logging in Script class to simplify handling of sensitive arguments * Improve command logging in Script class to include full command line when debugging * Remove unused _passwordCommand flag from Script class to simplify code * Update utils/src/main/java/com/cloud/utils/script/Script.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Remove unused import for KeyStoreUtils * Update utils/src/main/java/com/cloud/utils/script/Script.java --------- Co-authored-by: chenyoulong20g@ict.ac.cn <chenyoulong20g@ict.ac.cn> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: dahn <daan@onecht.net> Co-authored-by: dahn <daan.hoogland@gmail.com>
1 parent 10ba9d7 commit 4f45d34

4 files changed

Lines changed: 93 additions & 41 deletions

File tree

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtUpdateHostPasswordCommandWrapper.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@ public Answer execute(final UpdateHostPasswordCommand command, final LibvirtComp
3737
final String newPassword = command.getNewPassword();
3838

3939
final Script script = libvirtUtilitiesHelper.buildScript(libvirtComputingResource.getUpdateHostPasswdPath());
40-
script.add(username, newPassword);
40+
script.add(username);
41+
script.addSensitive(newPassword);
4142
final String result = script.execute();
4243

4344
if (result != null) {

‎plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixUpdateHostPasswordCommandWrapper.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public Answer execute(final UpdateHostPasswordCommand command, final CitrixResou
4747
try {
4848
logger.debug("Executing password update command on host: {} for user: {}", hostIp, username);
4949
final String hostPassword = citrixResourceBase.getPwdFromQueue();
50-
result = xenServerUtilitiesHelper.executeSshWrapper(hostIp, 22, username, null, hostPassword, cmdLine.toString());
50+
result = xenServerUtilitiesHelper.executeSshWrapper(hostIp, 22, username, null, hostPassword, cmdLine);
5151
} catch (final Exception e) {
5252
return new Answer(command, false, e.getMessage());
5353
}

‎utils/src/main/java/com/cloud/utils/script/Script.java‎

Lines changed: 60 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,10 @@
2929
import java.net.URL;
3030
import java.util.ArrayList;
3131
import java.util.Arrays;
32+
import java.util.HashSet;
3233
import java.util.List;
3334
import java.util.Properties;
35+
import java.util.Set;
3436
import java.util.concurrent.Callable;
3537
import java.util.concurrent.ExecutionException;
3638
import java.util.concurrent.Executors;
@@ -42,7 +44,6 @@
4244
import java.util.concurrent.atomic.AtomicReference;
4345
import java.util.stream.Collectors;
4446

45-
import org.apache.cloudstack.utils.security.KeyStoreUtils;
4647
import org.apache.commons.collections.CollectionUtils;
4748
import org.apache.commons.io.IOUtils;
4849
import org.apache.logging.log4j.LogManager;
@@ -66,8 +67,8 @@ public class Script implements Callable<String> {
6667
private static final int DEFAULT_TIMEOUT = 3600 * 1000; /* 1 hour */
6768
private volatile boolean _isTimeOut = false;
6869

69-
private boolean _passwordCommand = false;
7070
private boolean avoidLoggingCommand = false;
71+
private final Set<Integer> sensitiveArgIndices = new HashSet<>();
7172

7273
private static final ScheduledExecutorService s_executors = Executors.newScheduledThreadPool(10, new NamedThreadFactory("Script"));
7374

@@ -145,6 +146,11 @@ public void add(String param) {
145146
_command.add(param);
146147
}
147148

149+
public void addSensitive(String param) {
150+
_command.add(param);
151+
sensitiveArgIndices.add(_command.size() - 1);
152+
}
153+
148154
public Script set(String name, String value) {
149155
_command.add(name);
150156
_command.add(value);
@@ -163,7 +169,7 @@ protected String buildCommandLine(String[] command) {
163169
if (sanitizeViCmdParameter(cmd, builder) || sanitizeRbdFileFormatCmdParameter(cmd, builder)) {
164170
continue;
165171
}
166-
if (obscureParam) {
172+
if (obscureParam || sensitiveArgIndices.contains(i)) {
167173
builder.append("******").append(" ");
168174
obscureParam = false;
169175
} else {
@@ -172,7 +178,6 @@ protected String buildCommandLine(String[] command) {
172178

173179
if ("-y".equals(cmd) || "-z".equals(cmd)) {
174180
obscureParam = true;
175-
_passwordCommand = true;
176181
}
177182
}
178183
return builder.toString();
@@ -240,8 +245,8 @@ static String stackTraceAsString(Throwable throwable) {
240245
public String execute(OutputInterpreter interpreter) {
241246
String[] command = _command.toArray(new String[_command.size()]);
242247
String commandLine = buildCommandLine(command);
243-
if (_logger.isDebugEnabled() && !avoidLoggingCommand) {
244-
_logger.debug(String.format("Executing command [%s].", commandLine.split(KeyStoreUtils.KS_FILENAME)[0]));
248+
if (_logger.isDebugEnabled() ) {
249+
_logger.debug(String.format("Executing command [%s].", commandLine));
245250
}
246251

247252
try {
@@ -264,48 +269,62 @@ public String execute(OutputInterpreter interpreter) {
264269
_thread = Thread.currentThread();
265270
ScheduledFuture<String> future = null;
266271
if (_timeout > 0) {
267-
_logger.trace(String.format("Scheduling the execution of command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
272+
_logger.trace(String.format(
273+
"Scheduling the execution of command [%s] with a timeout of [%s] milliseconds.",
274+
commandLine, _timeout));
268275
future = s_executors.schedule(this, _timeout, TimeUnit.MILLISECONDS);
269276
}
270277

271278
long processPid = _process.pid();
272279
Task task = null;
273280
if (interpreter != null && interpreter.drain()) {
274-
_logger.trace(String.format("Executing interpreting task of process [%s] for command [%s].", processPid, commandLine));
281+
_logger.trace(String.format("Executing interpreting task of process [%s] for command [%s].",
282+
processPid, commandLine));
275283
task = new Task(interpreter, ir);
276284
s_executors.execute(task);
277285
}
278286

279287
while (true) {
280-
_logger.trace(String.format("Attempting process [%s] execution for command [%s] with timeout [%s].", processPid, commandLine, _timeout));
288+
_logger.trace(String.format("Attempting process [%s] execution for command [%s] with timeout [%s].",
289+
processPid, commandLine, _timeout));
281290
try {
282291
if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) {
283-
_logger.trace(String.format("Process [%s] execution for command [%s] completed within timeout period [%s].", processPid, commandLine,
292+
_logger.trace(String.format(
293+
"Process [%s] execution for command [%s] completed within timeout period [%s].",
294+
processPid, commandLine,
284295
_timeout));
285296
if (_process.exitValue() == 0) {
286-
_logger.debug(String.format("Successfully executed process [%s] for command [%s].", processPid, commandLine));
297+
_logger.debug(String.format("Successfully executed process [%s] for command [%s].",
298+
processPid, commandLine));
287299
if (interpreter != null) {
288300
if (interpreter.drain()) {
289-
_logger.trace(String.format("Returning task result of process [%s] for command [%s].", processPid, commandLine));
301+
_logger.trace(
302+
String.format("Returning task result of process [%s] for command [%s].",
303+
processPid, commandLine));
290304
return task.getResult();
291305
}
292-
_logger.trace(String.format("Returning interpretation of process [%s] for command [%s].", processPid, commandLine));
306+
_logger.trace(
307+
String.format("Returning interpretation of process [%s] for command [%s].",
308+
processPid, commandLine));
293309
return interpreter.interpret(ir);
294310
} else {
295311
// null return exitValue apparently
296-
_logger.trace(String.format("Process [%s] for command [%s] exited with value [%s].", processPid, commandLine,
312+
_logger.trace(String.format("Process [%s] for command [%s] exited with value [%s].",
313+
processPid, commandLine,
297314
_process.exitValue()));
298315
return String.valueOf(_process.exitValue());
299316
}
300317
} else {
301-
_logger.warn(String.format("Execution of process [%s] for command [%s] failed.", processPid, commandLine));
318+
_logger.warn(String.format("Execution of process [%s] for command [%s] failed.",
319+
processPid, commandLine));
302320
break;
303321
}
304322
}
305323
} catch (InterruptedException e) {
306324
if (!_isTimeOut) {
307-
_logger.debug(String.format("Exception [%s] occurred; however, it was not a timeout. Therefore, proceeding with the execution of process [%s] for command "
308-
+ "[%s].", e.getMessage(), processPid, commandLine), e);
325+
_logger.debug(String.format(
326+
"Exception [%s] occurred; however, it was not a timeout. Therefore, proceeding with the execution of process [%s] for command [%s].",
327+
e.getMessage(), processPid, commandLine), e);
309328
continue;
310329
}
311330
} finally {
@@ -318,18 +337,17 @@ public String execute(OutputInterpreter interpreter) {
318337
TimedOutLogger log = new TimedOutLogger(_process);
319338
Task timedoutTask = new Task(log, ir);
320339

321-
_logger.trace(String.format("Running timed out task of process [%s] for command [%s].", processPid, commandLine));
340+
_logger.trace(String.format("Running timed out task of process [%s] for command [%s].", processPid,
341+
commandLine));
322342
timedoutTask.run();
323-
if (!_passwordCommand) {
324-
_logger.warn(String.format("Process [%s] for command [%s] timed out. Output is [%s].", processPid, commandLine, timedoutTask.getResult()));
325-
} else {
326-
_logger.warn(String.format("Process [%s] for command [%s] timed out.", processPid, commandLine));
327-
}
343+
_logger.warn(String.format("Process [%s] for command [%s] timed out. Output is [%s].", processPid,
344+
commandLine, timedoutTask.getResult()));
328345

329346
return ERR_TIMEOUT;
330347
}
331348

332-
_logger.debug(String.format("Exit value of process [%s] for command [%s] is [%s].", processPid, commandLine, _process.exitValue()));
349+
_logger.debug(String.format("Exit value of process [%s] for command [%s] is [%s].", processPid,
350+
commandLine, _process.exitValue()));
333351

334352
BufferedReader reader = new BufferedReader(new InputStreamReader(_process.getInputStream()), 128);
335353

@@ -340,19 +358,24 @@ public String execute(OutputInterpreter interpreter) {
340358
error = String.valueOf(_process.exitValue());
341359
}
342360

343-
_logger.warn(String.format("Process [%s] for command [%s] encountered the error: [%s].", processPid, commandLine, error));
361+
_logger.warn(String.format("Process [%s] for command [%s] encountered the error: [%s].", processPid,
362+
commandLine, error));
344363

345364
return error;
346365
} catch (SecurityException ex) {
347-
_logger.warn(String.format("Exception [%s] occurred. This may be due to an attempt of executing command [%s] as non root.", ex.getMessage(), commandLine),
366+
_logger.warn(String.format(
367+
"Exception [%s] occurred. This may be due to an attempt of executing command [%s] as non root.",
368+
ex.getMessage(), commandLine),
348369
ex);
349370
return stackTraceAsString(ex);
350371
} catch (Exception ex) {
351-
_logger.warn(String.format("Exception [%s] occurred when attempting to run command [%s].", ex.getMessage(), commandLine), ex);
372+
_logger.warn(String.format("Exception [%s] occurred when attempting to run command [%s].",
373+
ex.getMessage(), commandLine), ex);
352374
return stackTraceAsString(ex);
353375
} finally {
354376
if (_process != null) {
355-
_logger.trace(String.format("Destroying process [%s] for command [%s].", _process.pid(), commandLine));
377+
_logger.trace(
378+
String.format("Destroying process [%s] for command [%s].", _process.pid(), commandLine));
356379
IOUtils.closeQuietly(_process.getErrorStream());
357380
IOUtils.closeQuietly(_process.getOutputStream());
358381
IOUtils.closeQuietly(_process.getInputStream());
@@ -363,9 +386,10 @@ public String execute(OutputInterpreter interpreter) {
363386

364387
public String executeIgnoreExitValue(OutputInterpreter interpreter, int exitValue) {
365388
String[] command = _command.toArray(new String[_command.size()]);
389+
String commandLine = buildCommandLine(command);
366390

367391
if (_logger.isDebugEnabled()) {
368-
_logger.debug(String.format("Executing: %s", buildCommandLine(command).split(KeyStoreUtils.KS_FILENAME)[0]));
392+
_logger.debug(String.format("Executing: %s", commandLine));
369393
}
370394

371395
try {
@@ -376,7 +400,7 @@ public String executeIgnoreExitValue(OutputInterpreter interpreter, int exitValu
376400

377401
_process = pb.start();
378402
if (_process == null) {
379-
_logger.warn(String.format("Unable to execute: %s", buildCommandLine(command)));
403+
_logger.warn(String.format("Unable to execute: %s", commandLine));
380404
return String.format("Unable to execute the command: %s", command[0]);
381405
}
382406

@@ -440,11 +464,8 @@ public String executeIgnoreExitValue(OutputInterpreter interpreter, int exitValu
440464
Task timedoutTask = new Task(log, ir);
441465

442466
timedoutTask.run();
443-
if (!_passwordCommand) {
444-
_logger.warn(String.format("Timed out: %s. Output is: %s", buildCommandLine(command), timedoutTask.getResult()));
445-
} else {
446-
_logger.warn(String.format("Timed out: %s", buildCommandLine(command)));
447-
}
467+
_logger.warn(String.format("Timed out: %s. Output is: %s", commandLine,
468+
timedoutTask.getResult()));
448469

449470
return ERR_TIMEOUT;
450471
}
@@ -468,7 +489,7 @@ public String executeIgnoreExitValue(OutputInterpreter interpreter, int exitValu
468489
_logger.warn("Security Exception....not running as root?", ex);
469490
return stackTraceAsString(ex);
470491
} catch (Exception ex) {
471-
_logger.warn(String.format("Exception: %s", buildCommandLine(command)), ex);
492+
_logger.warn(String.format("Exception: %s", commandLine), ex);
472493
return stackTraceAsString(ex);
473494
} finally {
474495
if (_process != null) {
@@ -517,9 +538,9 @@ public void run() {
517538
} catch (Exception ex) {
518539
result = stackTraceAsString(ex);
519540
} finally {
520-
done = true;
521-
notifyAll();
522-
IOUtils.closeQuietly(reader);
541+
done = true;
542+
notifyAll();
543+
IOUtils.closeQuietly(reader);
523544
}
524545
}
525546
}

‎utils/src/test/java/com/cloud/utils/script/ScriptTest.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,4 +78,34 @@ public void testGetExecutableAbsolutePath() {
7878
String result = Script.getExecutableAbsolutePath("ls");
7979
Assert.assertTrue(List.of("/usr/bin/ls", "/bin/ls").contains(result));
8080
}
81+
82+
@Test
83+
public void testBuildCommandLineWithSensitiveData() {
84+
Script script = new Script("test.sh");
85+
script.add("normal-arg");
86+
script.addSensitive("sensitive-arg");
87+
String commandLine = script.toString();
88+
Assert.assertEquals("test.sh normal-arg ****** ", commandLine);
89+
}
90+
91+
@Test
92+
public void testBuildCommandLineWithMultipleSensitiveData() {
93+
Script script = new Script("test.sh");
94+
script.add("normal-arg");
95+
script.addSensitive("sensitive-arg1");
96+
script.add("another-normal-arg");
97+
script.addSensitive("sensitive-arg2");
98+
String commandLine = script.toString();
99+
Assert.assertEquals("test.sh normal-arg ****** another-normal-arg ****** ", commandLine);
100+
}
101+
102+
@Test
103+
public void testBuildCommandLineWithLegacyPasswordOption() {
104+
Script script = new Script("test.sh");
105+
script.add("-y");
106+
script.add("legacy-password");
107+
String commandLine = script.toString();
108+
Assert.assertEquals("test.sh -y ****** ", commandLine);
109+
}
110+
81111
}

0 commit comments

Comments
 (0)