From 21ae27e66d77f276c23a7ca2eaa8869438fb4d31 Mon Sep 17 00:00:00 2001 From: prashkre Date: Thu, 25 Jan 2018 13:41:42 +0530 Subject: [PATCH] Mask passwords only when command execution fails At many places, processutils.ssh_execute() is being invoked to run a command over ssh and output returned is parsed to get appropriate information. In this flow, unsanitized output is being expected where processutils.ssh_execute() was invoked but found that output like volume details(containing "password" string in its name) is being masked away with strutils.mask_password(stdout) even though no error occured during command execution. This is regression issue from patch[0]. In this fix, stdout and stderr in processutils.ssh_execute() will be masked only when ProcessExecutionError exception is thrown i.e. command execution failed due to some reasons. [0] https://github.com/openstack/oslo.concurrency/commit/ ae9e05bfc3d7ec867bd6ec78c301f11c2bdd0d5f Change-Id: I2ce344330905eef437ef3f89a2a01169a30df8ab Closes-Bug: #1482382 --- oslo_concurrency/processutils.py | 24 +++++++++++++++-- .../tests/unit/test_processutils.py | 26 ++++++++++++++++++- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/oslo_concurrency/processutils.py b/oslo_concurrency/processutils.py index 8479a3a..3ccbced 100644 --- a/oslo_concurrency/processutils.py +++ b/oslo_concurrency/processutils.py @@ -503,9 +503,21 @@ def trycmd(*args, **kwargs): def ssh_execute(ssh, cmd, process_input=None, addl_env=None, check_exit_code=True, - binary=False, timeout=None): + binary=False, timeout=None, + sanitize_stdout=True): """Run a command through SSH. + :param ssh: An SSH Connection object. + :param cmd: The command string to run. + :param check_exit_code: If an exception should be raised for non-zero + exit. + :param timeout: Max time in secs to wait for command execution. + :param sanitize_stdout: Defaults to True. If set to True, stdout is + sanitized i.e. any sensitive information like + password in command output will be masked. + :returns: (stdout, stderr) from command execution through + SSH. + .. versionchanged:: 1.9 Added *binary* optional parameter. """ @@ -537,13 +549,21 @@ def ssh_execute(ssh, cmd, process_input=None, # mask_password() requires Unicode on Python 3 stdout = os.fsdecode(stdout) stderr = os.fsdecode(stderr) - stdout = strutils.mask_password(stdout) + + if sanitize_stdout: + stdout = strutils.mask_password(stdout) + stderr = strutils.mask_password(stderr) # exit_status == -1 if no exit code was returned if exit_status != -1: LOG.debug('Result was %s' % exit_status) if check_exit_code and exit_status != 0: + # In case of errors in command run, due to poor implementation of + # command executable program, there might be chance that it leaks + # sensitive information like password to stdout. In such cases + # stdout needs to be sanitized even though sanitize_stdout=False. + stdout = strutils.mask_password(stdout) raise ProcessExecutionError(exit_code=exit_status, stdout=stdout, stderr=stderr, diff --git a/oslo_concurrency/tests/unit/test_processutils.py b/oslo_concurrency/tests/unit/test_processutils.py index 29576e5..820839f 100644 --- a/oslo_concurrency/tests/unit/test_processutils.py +++ b/oslo_concurrency/tests/unit/test_processutils.py @@ -758,7 +758,8 @@ class SshExecuteTestCase(test_base.BaseTestCase): fake_stdout.channel.recv_exit_status.return_value = rc fake_stdout.read.return_value = b'password="secret"' - fake_stderr = six.BytesIO(b'password="foobar"') + fake_stderr = mock.Mock() + fake_stderr.read.return_value = b'password="foobar"' command = 'ls --password="bar"' @@ -778,6 +779,20 @@ class SshExecuteTestCase(test_base.BaseTestCase): self.assertEqual('ls --password="***"', err.cmd) self.assertNotIn('secret', str(err)) self.assertNotIn('foobar', str(err)) + + # test ssh_execute with sanitize_stdout=False + err = self.assertRaises(processutils.ProcessExecutionError, + processutils.ssh_execute, + connection, command, + check_exit_code=check, + sanitize_stdout=False) + + self.assertEqual(rc, err.exit_code) + self.assertEqual('password="***"', err.stdout) + self.assertEqual('password="***"', err.stderr) + self.assertEqual('ls --password="***"', err.cmd) + self.assertNotIn('secret', str(err)) + self.assertNotIn('foobar', str(err)) else: o, e = processutils.ssh_execute(connection, command, check_exit_code=check) @@ -786,6 +801,15 @@ class SshExecuteTestCase(test_base.BaseTestCase): self.assertIn('password="***"', fixture.output) self.assertNotIn('bar', fixture.output) + # test ssh_execute with sanitize_stdout=False + o, e = processutils.ssh_execute(connection, command, + check_exit_code=check, + sanitize_stdout=False) + self.assertEqual('password="secret"', 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)