Transfer ownership of files over ssh with sudo/su (#616)

* Transfer ownership of files over ssh with sudo/su.

Adds a step to SSH file uploads that assigns read permissions via
ACLs to the sudo/su user, and uses cp instead of mv to allow the
the intended user to take ownership.

* Use format to maintain python2 compatibility

* Modify StringCommand for consistent styling.

Co-authored-by: Nick Barrett <nick@fizzadar.com>

* Modify StringCommand for consistent styling.

Co-authored-by: Nick Barrett <nick@fizzadar.com>

* Only run setfacl command if a sudo/su user is specified

* Remove redundant chown command

The chown command is no longer required because we now cp the file as
the sudo/su user, so the file is created with the correct ownership.

* Remove temporary files after upload

* Modify test cases for new SSH upload commands

* Linting whitespaces.

* Expand SSH connector tests to cover copy & acl failures.

Co-authored-by: ben <ben@pop-os.localdomain>
Co-authored-by: Nick Barrett <nick@fizzadar.com>

Author: 3 people

pyinfra/api/connectors/ssh.py

@@ -420,16 +420,26 @@ def put_file(
         temp_file = state.get_temp_filename(remote_filename)
         _put_file(host, filename_or_io, temp_file)
 
-        # Execute run_shell_command w/sudo and/or su_user
-        command = StringCommand('mv', temp_file, QuoteString(remote_filename))
-
-        # Move it to the su_user if present
+        # Make sure our sudo/su user can access the file
         if su_user:
-            command = StringCommand(command, '&&', 'chown', su_user, QuoteString(remote_filename))
-
-        # Otherwise any sudo_user
+            command = StringCommand('setfacl', '-m', 'u:{0}:r'.format(su_user), temp_file)
         elif sudo_user:
-            command = StringCommand(command, '&&', 'chown', sudo_user, QuoteString(remote_filename))
+            command = StringCommand('setfacl -m u:{0}:r'.format(sudo_user), temp_file)
+        if su_user or sudo_user:
+            status, _, stderr = run_shell_command(
+                state, host, command,
+                sudo=False,
+                print_output=print_output,
+                print_input=print_input,
+                **command_kwargs
+            )
+
+            if status is False:
+                logger.error('Error on handover to sudo/su user: {0}'.format('\n'.join(stderr)))
+                return False
+
+        # Execute run_shell_command w/sudo and/or su_user
+        command = StringCommand('cp', temp_file, QuoteString(remote_filename))
 
         status, _, stderr = run_shell_command(
             state, host, command,
@@ -443,6 +453,21 @@ def put_file(
             logger.error('File upload error: {0}'.format('\n'.join(stderr)))
             return False
 
+        # Delete the temporary file now that we've successfully copied it
+        command = StringCommand('rm', '-f', temp_file)
+
+        status, _, stderr = run_shell_command(
+            state, host, command,
+            sudo=False,
+            print_output=print_output,
+            print_input=print_input,
+            **command_kwargs
+        )
+
+        if status is False:
+            logger.error('Unable to remove temporary file: {0}'.format('\n'.join(stderr)))
+            return False
+
     # No sudo and no su_user, so just upload it!
     else:
         _put_file(host, filename_or_io, remote_filename)

tests/test_connectors/test_ssh.py

@@ -507,10 +507,8 @@ def test_put_file_sudo(self, fake_sftp_client, fake_ssh_client):
         assert status is True
 
         fake_ssh_client().exec_command.assert_called_with((
-            "sudo -H -n -u ubuntu sh -c 'mv "
-            '/tmp/pyinfra-de01e82cb691e8a31369da3c7c8f17341c44ac24 '
-            "\'\"\'\"\'not another file\'\"\'\"\' && chown ubuntu "
-            "\'\"\'\"\'not another file\'\"\'\"\'\'"
+            "sh -c 'rm -f "
+            "/tmp/pyinfra-de01e82cb691e8a31369da3c7c8f17341c44ac24'"
         ), get_pty=False)
 
         fake_sftp_client.from_transport().putfo.assert_called_with(
@@ -519,7 +517,7 @@ def test_put_file_sudo(self, fake_sftp_client, fake_ssh_client):
 
     @patch('pyinfra.api.connectors.ssh.SSHClient')
     @patch('pyinfra.api.connectors.ssh.SFTPClient')
-    def test_put_file_su_user_fail(self, fake_sftp_client, fake_ssh_client):
+    def test_put_file_su_user_fail_acl(self, fake_sftp_client, fake_ssh_client):
         inventory = make_inventory(hosts=('anotherhost',))
         State(inventory, Config())
         host = inventory.get_host('anotherhost')
@@ -540,9 +538,46 @@ def test_put_file_su_user_fail(self, fake_sftp_client, fake_ssh_client):
         assert status is False
 
         fake_ssh_client().exec_command.assert_called_with((
-            "su centos -c 'sh -c '\"'\"'mv "
+            "sh -c 'setfacl -m u:centos:r "
+            "/tmp/pyinfra-43db9984686317089fefcf2e38de527e4cb44487'"
+        ), get_pty=False)
+
+        fake_sftp_client.from_transport().putfo.assert_called_with(
+            fake_open(), '/tmp/pyinfra-43db9984686317089fefcf2e38de527e4cb44487',
+        )
+
+    @patch('pyinfra.api.connectors.ssh.SSHClient')
+    @patch('pyinfra.api.connectors.ssh.SFTPClient')
+    def test_put_file_su_user_fail_copy(self, fake_sftp_client, fake_ssh_client):
+        inventory = make_inventory(hosts=('anotherhost',))
+        State(inventory, Config())
+        host = inventory.get_host('anotherhost')
+        host.connect()
+
+        stdout_mock = MagicMock()
+        exit_codes = [0, 1]
+        stdout_mock.channel.recv_exit_status.side_effect = lambda: exit_codes.pop(0)
+        fake_ssh_client().exec_command.return_value = MagicMock(), stdout_mock, MagicMock()
+
+        fake_open = mock_open(read_data='test!')
+        with patch('pyinfra.api.util.open', fake_open, create=True):
+            status = host.put_file(
+                'not-a-file', 'not-another-file',
+                print_output=True,
+                su_user='centos',
+            )
+
+        assert status is False
+
+        fake_ssh_client().exec_command.assert_any_call((
+            "sh -c 'setfacl -m u:centos:r "
+            "/tmp/pyinfra-43db9984686317089fefcf2e38de527e4cb44487'"
+        ), get_pty=False)
+
+        fake_ssh_client().exec_command.assert_called_with((
+            "su centos -c 'sh -c '\"'\"'cp "
             '/tmp/pyinfra-43db9984686317089fefcf2e38de527e4cb44487 '
-            "not-another-file && chown centos not-another-file'\"'\"''"
+            "not-another-file'\"'\"''"
         ), get_pty=False)
 
         fake_sftp_client.from_transport().putfo.assert_called_with(

words.txt

@@ -6,6 +6,7 @@
 4
 64kb
 700
+acl
 addrs
 addsitedir
 adhoc