Browse Source

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/
ae9e05bfc3

Change-Id: I2ce344330905eef437ef3f89a2a01169a30df8ab
Closes-Bug: #1482382
prashkre 1 year ago
parent
commit
21ae27e66d
2 changed files with 47 additions and 3 deletions
  1. 22
    2
      oslo_concurrency/processutils.py
  2. 25
    1
      oslo_concurrency/tests/unit/test_processutils.py

+ 22
- 2
oslo_concurrency/processutils.py View File

@@ -503,9 +503,21 @@ def trycmd(*args, **kwargs):
503 503
 
504 504
 def ssh_execute(ssh, cmd, process_input=None,
505 505
                 addl_env=None, check_exit_code=True,
506
-                binary=False, timeout=None):
506
+                binary=False, timeout=None,
507
+                sanitize_stdout=True):
507 508
     """Run a command through SSH.
508 509
 
510
+    :param ssh:             An SSH Connection object.
511
+    :param cmd:             The command string to run.
512
+    :param check_exit_code: If an exception should be raised for non-zero
513
+                            exit.
514
+    :param timeout:         Max time in secs to wait for command execution.
515
+    :param sanitize_stdout: Defaults to True. If set to True, stdout is
516
+                            sanitized i.e. any sensitive information like
517
+                            password in command output will be masked.
518
+    :returns:               (stdout, stderr) from command execution through
519
+                            SSH.
520
+
509 521
     .. versionchanged:: 1.9
510 522
        Added *binary* optional parameter.
511 523
     """
@@ -537,13 +549,21 @@ def ssh_execute(ssh, cmd, process_input=None,
537 549
         # mask_password() requires Unicode on Python 3
538 550
         stdout = os.fsdecode(stdout)
539 551
         stderr = os.fsdecode(stderr)
540
-    stdout = strutils.mask_password(stdout)
552
+
553
+    if sanitize_stdout:
554
+        stdout = strutils.mask_password(stdout)
555
+
541 556
     stderr = strutils.mask_password(stderr)
542 557
 
543 558
     # exit_status == -1 if no exit code was returned
544 559
     if exit_status != -1:
545 560
         LOG.debug('Result was %s' % exit_status)
546 561
         if check_exit_code and exit_status != 0:
562
+            # In case of errors in command run, due to poor implementation of
563
+            # command executable program, there might be chance that it leaks
564
+            # sensitive information like password to stdout. In such cases
565
+            # stdout needs to be sanitized even though sanitize_stdout=False.
566
+            stdout = strutils.mask_password(stdout)
547 567
             raise ProcessExecutionError(exit_code=exit_status,
548 568
                                         stdout=stdout,
549 569
                                         stderr=stderr,

+ 25
- 1
oslo_concurrency/tests/unit/test_processutils.py View File

@@ -758,7 +758,8 @@ class SshExecuteTestCase(test_base.BaseTestCase):
758 758
         fake_stdout.channel.recv_exit_status.return_value = rc
759 759
         fake_stdout.read.return_value = b'password="secret"'
760 760
 
761
-        fake_stderr = six.BytesIO(b'password="foobar"')
761
+        fake_stderr = mock.Mock()
762
+        fake_stderr.read.return_value = b'password="foobar"'
762 763
 
763 764
         command = 'ls --password="bar"'
764 765
 
@@ -778,6 +779,20 @@ class SshExecuteTestCase(test_base.BaseTestCase):
778 779
             self.assertEqual('ls --password="***"', err.cmd)
779 780
             self.assertNotIn('secret', str(err))
780 781
             self.assertNotIn('foobar', str(err))
782
+
783
+            # test ssh_execute with sanitize_stdout=False
784
+            err = self.assertRaises(processutils.ProcessExecutionError,
785
+                                    processutils.ssh_execute,
786
+                                    connection, command,
787
+                                    check_exit_code=check,
788
+                                    sanitize_stdout=False)
789
+
790
+            self.assertEqual(rc, err.exit_code)
791
+            self.assertEqual('password="***"', err.stdout)
792
+            self.assertEqual('password="***"', err.stderr)
793
+            self.assertEqual('ls --password="***"', err.cmd)
794
+            self.assertNotIn('secret', str(err))
795
+            self.assertNotIn('foobar', str(err))
781 796
         else:
782 797
             o, e = processutils.ssh_execute(connection, command,
783 798
                                             check_exit_code=check)
@@ -786,6 +801,15 @@ class SshExecuteTestCase(test_base.BaseTestCase):
786 801
             self.assertIn('password="***"', fixture.output)
787 802
             self.assertNotIn('bar', fixture.output)
788 803
 
804
+            # test ssh_execute with sanitize_stdout=False
805
+            o, e = processutils.ssh_execute(connection, command,
806
+                                            check_exit_code=check,
807
+                                            sanitize_stdout=False)
808
+            self.assertEqual('password="secret"', o)
809
+            self.assertEqual('password="***"', e)
810
+            self.assertIn('password="***"', fixture.output)
811
+            self.assertNotIn('bar', fixture.output)
812
+
789 813
     def test_compromising_ssh1(self):
790 814
         self._test_compromising_ssh(rc=-1, check=True)
791 815
 

Loading…
Cancel
Save