Partially address concerns in Couchbase restore strategy

This is a preliminary fix for community review. I expect to be making
some changes as well.

This change addresses some concerns about the Couchbase restore
strategy. In particular, the concerns that are addressed include
string injection in some execute calls, and issues around the
permissions on the secret file used to store the password.

This change set aims to fully address the issue around the password
file but is a partial solution to the string injection
problem. Additional changes are required to completely address
that. Some additional bugs have also been raised to address issues
related to other concerns discovered while fixing this bug. More to
come.

Change-Id: Icd8033caea4866f57f8cb4c5861d21797136ff90
Partial-Bug: #1349939

6 files changed, 92 insertions, 32 deletions

diff --git a/trove/guestagent/datastore/couchbase/service.py b/trove/guestagent/datastore/couchbase/service.py
index ff4ba8b8..abc7dc0c 100644
--- a/trove/guestagent/datastore/couchbase/service.py
+++ b/trove/guestagent/datastore/couchbase/service.py
@@ -16,7 +16,9 @@
 import json
 import pexpect
 import os
+import stat
 import subprocess
+import tempfile
 
 from trove.common import cfg
 from trove.common import exception
@@ -279,16 +281,25 @@ class CouchbaseRootAccess(object):
         self.write_password_to_file(root_password)
 
     def write_password_to_file(self, root_password):
-        utils.execute_with_timeout('mkdir',
-                                   '-p',
-                                   system.COUCHBASE_CONF_DIR,
-                                   run_as_root=True,
-                                   root_helper='sudo')
-        utils.execute_with_timeout("sudo sh -c 'echo " +
-                                   root_password +
-                                   ' > ' +
-                                   system.pwd_file + "'",
-                                   shell=True)
+        utils.execute_with_timeout('mkdir', '-p', system.COUCHBASE_CONF_DIR,
+                                   run_as_root=True, root_helper='sudo')
+
+        try:
+            tempfd, tempname = tempfile.mkstemp()
+            os.fchmod(tempfd, stat.S_IRUSR | stat.S_IWUSR)
+            os.write(tempfd, root_password)
+            os.fchmod(tempfd, stat.S_IRUSR)
+            os.close(tempfd)
+        except OSError as err:
+            message = _("An error occurred in saving password "
+                        "(%(errno)s). %(strerror)s.") % {
+                            "errno": err.errno,
+                            "strerror": err.strerror}
+            LOG.exception(message)
+            raise RuntimeError(message)
+
+        utils.execute_with_timeout('mv', tempname, system.pwd_file,
+                                   run_as_root=True, root_helper='sudo')
 
     @staticmethod
     def get_password():
diff --git a/trove/guestagent/strategies/backup/couchbase_impl.py b/trove/guestagent/strategies/backup/couchbase_impl.py
index 97359655..9156ee29 100644
--- a/trove/guestagent/strategies/backup/couchbase_impl.py
+++ b/trove/guestagent/strategies/backup/couchbase_impl.py
@@ -36,13 +36,12 @@ class CbBackup(base.BackupRunner):
     __strategy_name__ = 'cbbackup'
 
     pre_backup_commands = [
-        'rm -rf ' + system.COUCHBASE_DUMP_DIR,
-
-        'mkdir -p ' + system.COUCHBASE_DUMP_DIR,
+        ['rm', '-rf', system.COUCHBASE_DUMP_DIR],
+        ['mkdir', '-p', system.COUCHBASE_DUMP_DIR],
     ]
 
     post_backup_commands = [
-        'rm -rf ' + system.COUCHBASE_DUMP_DIR,
+        ['rm', '-rf', system.COUCHBASE_DUMP_DIR],
     ]
 
     @property
@@ -50,7 +49,7 @@ class CbBackup(base.BackupRunner):
         """
         Creates backup dump dir, tars it up, and encrypts it.
         """
-        cmd = 'tar cPf - ' + system.COUCHBASE_DUMP_DIR
+        cmd = 'tar cpPf - ' + system.COUCHBASE_DUMP_DIR
         return cmd + self.zip_cmd + self.encrypt_cmd
 
     def _save_buckets_config(self, password):
@@ -60,16 +59,16 @@ class CbBackup(base.BackupRunner):
                                    shell=True, timeout=300)
 
     def _backup(self, password):
-        utils.execute_with_timeout('/opt/couchbase/bin/cbbackup ' +
-                                   system.COUCHBASE_REST_API + ' ' +
-                                   system.COUCHBASE_DUMP_DIR +
-                                   ' -u root -p ' + password,
-                                   shell=True, timeout=600)
+        utils.execute_with_timeout(['/opt/couchbase/bin/cbbackup',
+                                    system.COUCHBASE_REST_API,
+                                    system.COUCHBASE_DUMP_DIR,
+                                    '-u', 'root', '-p', password],
+                                   timeout=600)
 
     def _run_pre_backup(self):
         try:
             for cmd in self.pre_backup_commands:
-                utils.execute_with_timeout(cmd, shell=True)
+                utils.execute_with_timeout(cmd)
             root = service.CouchbaseRootAccess()
             pw = root.get_password()
             self._save_buckets_config(pw)
@@ -88,14 +87,15 @@ class CbBackup(base.BackupRunner):
                     else:
                         LOG.info(_("All buckets are memcached.  "
                                    "Skipping backup."))
-            utils.execute_with_timeout("mv " + OUTFILE + " " +
-                                       system.COUCHBASE_DUMP_DIR,
-                                       shell=True)
+            utils.execute_with_timeout(['mv', OUTFILE,
+                                        system.COUCHBASE_DUMP_DIR])
             if pw != "password":
                 # Not default password, backup generated root password
-                utils.execute_with_timeout('sudo cp ' + system.pwd_file +
-                                           ' ' + system.COUCHBASE_DUMP_DIR,
-                                           shell=True)
+                utils.execute_with_timeout(['cp', '-p',
+                                            system.pwd_file,
+                                            system.COUCHBASE_DUMP_DIR],
+                                           run_as_root=True,
+                                           root_helper='sudo')
         except exception.ProcessExecutionError as p:
             LOG.error(p)
             raise p
@@ -103,7 +103,7 @@ class CbBackup(base.BackupRunner):
     def _run_post_backup(self):
         try:
             for cmd in self.post_backup_commands:
-                utils.execute_with_timeout(cmd, shell=True)
+                utils.execute_with_timeout(cmd)
         except exception.ProcessExecutionError as p:
             LOG.error(p)
             raise p
diff --git a/trove/guestagent/strategies/restore/couchbase_impl.py b/trove/guestagent/strategies/restore/couchbase_impl.py
index f3daddb8..185fd3fe 100644
--- a/trove/guestagent/strategies/restore/couchbase_impl.py
+++ b/trove/guestagent/strategies/restore/couchbase_impl.py
@@ -35,7 +35,7 @@ class CbBackup(base.RestoreRunner):
     Implementation of Restore Strategy for Couchbase.
     """
     __strategy_name__ = 'cbbackup'
-    base_restore_cmd = 'sudo tar xPf -'
+    base_restore_cmd = 'sudo tar xpPf -'
 
     def __init__(self, *args, **kwargs):
         super(CbBackup, self).__init__(*args, **kwargs)
diff --git a/trove/tests/unittests/backup/test_backupagent.py b/trove/tests/unittests/backup/test_backupagent.py
index b8f83a91..6cec405d 100644
--- a/trove/tests/unittests/backup/test_backupagent.py
+++ b/trove/tests/unittests/backup/test_backupagent.py
@@ -217,7 +217,7 @@ class BackupAgentTest(testtools.TestCase):
         utils.execute_with_timeout = Mock(return_value=None)
         cbbackup = couchbase_impl.CbBackup('cbbackup', extra_opts='')
         self.assertIsNotNone(cbbackup)
-        str_cbbackup_cmd = ("tar cPf - /tmp/backups | "
+        str_cbbackup_cmd = ("tar cpPf - /tmp/backups | "
                             "gzip | openssl enc -aes-256-cbc -salt -pass "
                             "pass:default_aes_cbc_key")
         self.assertEqual(str_cbbackup_cmd, cbbackup.cmd)
diff --git a/trove/tests/unittests/guestagent/test_backups.py b/trove/tests/unittests/guestagent/test_backups.py
index e70671c7..2cd38b25 100644
--- a/trove/tests/unittests/guestagent/test_backups.py
+++ b/trove/tests/unittests/guestagent/test_backups.py
@@ -69,9 +69,9 @@ PREPARE = ("sudo innobackupex --apply-log /var/lib/mysql "
            "--ibbackup xtrabackup 2>/tmp/innoprepare.log")
 CRYPTO_KEY = "default_aes_cbc_key"
 
-CBBACKUP_CMD = "tar cPf - /tmp/backups"
+CBBACKUP_CMD = "tar cpPf - /tmp/backups"
 
-CBBACKUP_RESTORE = "sudo tar xPf -"
+CBBACKUP_RESTORE = "sudo tar xpPf -"
 
 
 class GuestAgentBackupTest(testtools.TestCase):
diff --git a/trove/tests/unittests/guestagent/test_couchbase_manager.py b/trove/tests/unittests/guestagent/test_couchbase_manager.py
index a1235f08..e86eeac8 100644
--- a/trove/tests/unittests/guestagent/test_couchbase_manager.py
+++ b/trove/tests/unittests/guestagent/test_couchbase_manager.py
@@ -12,8 +12,14 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import mock
+import os
+import stat
+import tempfile
 import testtools
 from mock import MagicMock
+from mock import Mock
+from trove.common import utils
 from trove.common.context import TroveContext
 from trove.guestagent import volume
 from trove.guestagent import backup
@@ -127,3 +133,46 @@ class GuestAgentCouchbaseManagerTest(testtools.TestCase):
         #verification/assertion
         couch_service.CouchbaseApp.stop_db.assert_any_call(
             do_not_start_on_reboot=False)
+
+    def __fake_mkstemp(self):
+        self.tempfd, self.tempname = self.original_mkstemp()
+        return self.tempfd, self.tempname
+
+    def __fake_mkstemp_raise(self):
+        raise OSError(11, 'Resource temporarily unavailable')
+
+    def __cleanup_tempfile(self):
+        if self.tempname:
+            os.unlink(self.tempname)
+
+    @mock.patch.object(utils, 'execute_with_timeout', Mock(return_value=0))
+    def test_write_password_to_file1(self):
+        self.original_mkstemp = tempfile.mkstemp
+        self.tempname = None
+
+        with mock.patch.object(tempfile,
+                               'mkstemp',
+                               self.__fake_mkstemp):
+            self.addCleanup(self.__cleanup_tempfile)
+
+            rootaccess = couch_service.CouchbaseRootAccess()
+            rootaccess.write_password_to_file('mypassword')
+
+            filepermissions = os.stat(self.tempname).st_mode
+            self.assertEqual(
+                filepermissions & 0o777, stat.S_IRUSR)
+
+    @mock.patch.object(utils, 'execute_with_timeout', Mock(return_value=0))
+    def test_write_password_to_file2(self):
+        self.original_mkstemp = tempfile.mkstemp
+        self.tempname = None
+
+        with mock.patch.object(tempfile,
+                               'mkstemp',
+                               self.__fake_mkstemp_raise):
+
+            rootaccess = couch_service.CouchbaseRootAccess()
+
+            self.assertRaises(RuntimeError,
+                              rootaccess.write_password_to_file,
+                              'mypassword')