openstack/oslo.concurrency
Code Issues Proposed changes

Mask passwords in exceptions and error messages

Browse Source 
When a ProcessExecutionError is thrown by processutils.execute(), the
exception may contain information such as password. Upstream
applications that just log the message (as several appear to do) could
inadvertently expose these passwords to a user with read access to the
log files. It is therefore considered prudent to invoke
strutils.mask_password() on the command, stdout and stderr in the
exception. A test case has been added to ensure that all three are
properly masked.

OSSA is aware of this change request.

Originally-Submitted-In: I173dfb865e84eb7dee54a22c76db1e4f125a0a8a

Change-Id: Ie122db5f19802f519b96ed024ab3f2b5eede3eee
Closes-Bug: #1343604
This commit is contained in:
Amrith Kumar 2014-08-14 00:52:02 -04:00
parent 37b90eda3e
commit c906dccefc
2 changed files with 41 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
oslo/concurrency/processutils.py
View File

@ -150,12 +150,12 @@ def execute(*cmd, **kwargs):
cmd = shlex.split(root_helper) + list(cmd)
cmd = map(str, cmd)
sanitized_cmd = strutils.mask_password(' '.join(cmd))
while attempts > 0:
attempts -= 1
try:
LOG.log(loglevel, 'Running cmd (subprocess): %s',
strutils.mask_password(' '.join(cmd)))
LOG.log(loglevel, _('Running cmd (subprocess): %s'), sanitized_cmd)
_PIPE = subprocess.PIPE # pylint: disable=E1101
if os.name == 'nt':
@ -192,16 +192,18 @@ def execute(*cmd, **kwargs):
LOG.log(loglevel, 'Result was %s' % _returncode)
if not ignore_exit_code and _returncode not in check_exit_code:
(stdout, stderr) = result
sanitized_stdout = strutils.mask_password(stdout)
sanitized_stderr = strutils.mask_password(stderr)
raise ProcessExecutionError(exit_code=_returncode,
stdout=stdout,
stderr=stderr,
cmd=' '.join(cmd))
stdout=sanitized_stdout,
stderr=sanitized_stderr,
cmd=sanitized_cmd)
return result
except ProcessExecutionError:
if not attempts:
raise
else:
LOG.log(loglevel, '%r failed. Retrying.', cmd)
LOG.log(loglevel, _('%r failed. Retrying.'), sanitized_cmd)
if delay_on_retry:
greenthread.sleep(random.randint(20, 200) / 100.0)
finally:

33
tests/unit/test_processutils.py
View File

@ -24,9 +24,18 @@ import fixtures
import mock
from oslotest import base as test_base
import six
import stat
from oslo.concurrency import processutils
TEST_EXCEPTION_AND_MASKING_SCRIPT = """#!/bin/bash
# This is to test stdout and stderr
# and the command returned in an exception
# when a non-zero exit code is returned
echo onstdout --password='"secret"'
echo onstderr --password='"secret"' 1>&2
exit 38"""
class UtilsTest(test_base.BaseTestCase):
# NOTE(jkoelker) Moar tests from nova need to be ported. But they
@ -217,6 +226,30 @@ grep foo
self.assertIn('SUPER_UNIQUE_VAR=The answer is 42', out)
def test_exception_and_masking(self):
tmpfilename = self.create_tempfiles(
[["test_exceptions_and_masking",
TEST_EXCEPTION_AND_MASKING_SCRIPT]], ext='bash')[0]
os.chmod(tmpfilename, (stat.S_IRWXU |
stat.S_IRGRP |
stat.S_IXGRP |
stat.S_IROTH |
stat.S_IXOTH))
err = self.assertRaises(processutils.ProcessExecutionError,
processutils.execute,
tmpfilename, 'password="secret"',
'something')
self.assertEqual(38, err.exit_code)
self.assertEqual(err.stdout, 'onstdout --password="***"\n')
self.assertEqual(err.stderr, 'onstderr --password="***"\n')
self.assertEqual(err.cmd, ' '.join([tmpfilename,
'password="***"',
'something']))
self.assertNotIn('secret', str(err))
def fake_execute(*cmd, **kwargs):
return 'stdout', 'stderr'