From c906dccefccedd8d00d6aa3eacc76194e8199714 Mon Sep 17 00:00:00 2001
From: Amrith Kumar <amrith@tesora.com>
Date: Thu, 14 Aug 2014 00:52:02 -0400
Subject: [PATCH] Mask passwords in exceptions and error messages

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
---
 oslo/concurrency/processutils.py | 14 ++++++++------
 tests/unit/test_processutils.py  | 33 ++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/oslo/concurrency/processutils.py b/oslo/concurrency/processutils.py
index a521e4e8..d95231b0 100644
--- a/oslo/concurrency/processutils.py
+++ b/oslo/concurrency/processutils.py
@@ -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:
diff --git a/tests/unit/test_processutils.py b/tests/unit/test_processutils.py
index 4a6942a2..51460205 100644
--- a/tests/unit/test_processutils.py
+++ b/tests/unit/test_processutils.py
@@ -24,9 +24,18 @@
 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 @@ def test_with_env_variables(self):
 
         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'