diff --git a/oslo/concurrency/processutils.py b/oslo/concurrency/processutils.py index 28c81ee..ad49566 100644 --- a/oslo/concurrency/processutils.py +++ b/oslo/concurrency/processutils.py @@ -242,7 +242,8 @@ def trycmd(*args, **kwargs): def ssh_execute(ssh, cmd, process_input=None, addl_env=None, check_exit_code=True): - LOG.debug('Running cmd (SSH): %s', cmd) + sanitized_cmd = strutils.mask_password(cmd) + LOG.debug('Running cmd (SSH): %s', sanitized_cmd) if addl_env: raise InvalidArgumentError(_('Environment not supported over SSH')) @@ -256,7 +257,10 @@ def ssh_execute(ssh, cmd, process_input=None, # NOTE(justinsb): This seems suspicious... # ...other SSH clients have buffering issues with this approach stdout = stdout_stream.read() + sanitized_stdout = strutils.mask_password(stdout) stderr = stderr_stream.read() + sanitized_stderr = strutils.mask_password(stderr) + stdin_stream.close() exit_status = channel.recv_exit_status() @@ -266,11 +270,11 @@ def ssh_execute(ssh, cmd, process_input=None, LOG.debug('Result was %s' % exit_status) if check_exit_code and exit_status != 0: raise ProcessExecutionError(exit_code=exit_status, - stdout=stdout, - stderr=stderr, - cmd=cmd) + stdout=sanitized_stdout, + stderr=sanitized_stderr, + cmd=sanitized_cmd) - return (stdout, stderr) + return (sanitized_stdout, sanitized_stderr) def get_worker_count(): diff --git a/tests/unit/test_processutils.py b/tests/unit/test_processutils.py index 5146020..829807b 100644 --- a/tests/unit/test_processutils.py +++ b/tests/unit/test_processutils.py @@ -16,6 +16,7 @@ from __future__ import print_function import errno +import logging import multiprocessing import os import tempfile @@ -332,3 +333,57 @@ class SshExecuteTestCase(test_base.BaseTestCase): def test_fails(self): self.assertRaises(processutils.ProcessExecutionError, processutils.ssh_execute, FakeSshConnection(1), 'ls') + + def _test_compromising_ssh(self, rc, check): + fixture = self.useFixture(fixtures.FakeLogger(level=logging.DEBUG)) + fake_stdin = six.StringIO() + + fake_stdout = mock.Mock() + fake_stdout.channel.recv_exit_status.return_value = rc + fake_stdout.read.return_value = 'password="secret"' + + fake_stderr = six.StringIO('password="foobar"') + + command = 'ls --password="bar"' + + connection = mock.Mock() + connection.exec_command.return_value = (fake_stdin, fake_stdout, + fake_stderr) + + if check and rc != -1 and rc != 0: + err = self.assertRaises(processutils.ProcessExecutionError, + processutils.ssh_execute, + connection, command, + check_exit_code=check) + + self.assertEqual(rc, err.exit_code) + self.assertEqual(err.stdout, 'password="***"') + self.assertEqual(err.stderr, 'password="***"') + self.assertEqual(err.cmd, 'ls --password="***"') + self.assertNotIn('secret', str(err)) + self.assertNotIn('foobar', str(err)) + else: + o, e = processutils.ssh_execute(connection, command, + check_exit_code=check) + self.assertEqual('password="***"', o) + self.assertEqual('password="***"', e) + self.assertIn('password="***"', fixture.output) + self.assertNotIn('bar', fixture.output) + + def test_compromising_ssh1(self): + self._test_compromising_ssh(rc=-1, check=True) + + def test_compromising_ssh2(self): + self._test_compromising_ssh(rc=0, check=True) + + def test_compromising_ssh3(self): + self._test_compromising_ssh(rc=1, check=True) + + def test_compromising_ssh4(self): + self._test_compromising_ssh(rc=1, check=False) + + def test_compromising_ssh5(self): + self._test_compromising_ssh(rc=0, check=False) + + def test_compromising_ssh6(self): + self._test_compromising_ssh(rc=-1, check=False)