Use file list, not recursion, in _fixup_perms. (#16924)

Run setfacl/chown/chmod on each temp dir and file.

This fixes temp file permissions handling on platforms such as FreeBSD
which always return success when using find -exec. This is done by
eliminating the use of find when setting up temp files and directories.

Additionally, tests that now pass on FreeBSD have been enabled for CI.

lib/ansible/plugins/action/__init__.py

@@ -293,7 +293,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
 
         return remote_path
 
-    def _fixup_perms(self, remote_path, remote_user, execute=True, recursive=True):
+    def _fixup_perms(self, remote_paths, remote_user, execute=True):
         """
         We need the files we upload to be readable (and sometimes executable)
         by the user being sudo'd to but we want to limit other people's access
@@ -319,15 +319,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         if self._connection._shell.SHELL_FAMILY == 'powershell':
             # This won't work on Powershell as-is, so we'll just completely skip until
             # we have a need for it, at which point we'll have to do something different.
-            return remote_path
-
-        if remote_path is None:
-            # Sometimes code calls us naively -- it has a var which could
-            # contain a path to a tmp dir but doesn't know if it needs to
-            # exist or not.  If there's no path, then there's no need for us
-            # to do work
-            display.debug('_fixup_perms called with remote_path==None.  Sure this is correct?')
-            return remote_path
+            return remote_paths
 
         if self._play_context.become and self._play_context.become_user not in ('root', remote_user):
             # Unprivileged user that's different than the ssh user.  Let's get
@@ -340,17 +332,17 @@ class ActionBase(with_metaclass(ABCMeta, object)):
             else:
                 mode = 'rX'
 
-            res = self._remote_set_user_facl(remote_path, self._play_context.become_user, mode, recursive=recursive, sudoable=False)
+            res = self._remote_set_user_facl(remote_paths, self._play_context.become_user, mode)
             if res['rc'] != 0:
                 # File system acls failed; let's try to use chown next
                 # Set executable bit first as on some systems an
                 # unprivileged user can use chown
                 if execute:
-                    res = self._remote_chmod('u+x', remote_path, recursive=recursive)
+                    res = self._remote_chmod(remote_paths, 'u+x')
                     if res['rc'] != 0:
                         raise AnsibleError('Failed to set file mode on remote temporary files (rc: {0}, err: {1})'.format(res['rc'], res['stderr']))
 
-                res = self._remote_chown(remote_path, self._play_context.become_user, recursive=recursive)
+                res = self._remote_chown(remote_paths, self._play_context.become_user)
                 if res['rc'] != 0 and remote_user == 'root':
                     # chown failed even if remove_user is root
                     raise AnsibleError('Failed to change ownership of the temporary files Ansible needs to create despite connecting as root.  Unprivileged become user would be unable to read the file.')
@@ -359,7 +351,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
                         # chown and fs acls failed -- do things this insecure
                         # way only if the user opted in in the config file
                         display.warning('Using world-readable permissions for temporary files Ansible needs to create when becoming an unprivileged user which may be insecure. For information on securing this, see https://docs.ansible.com/ansible/become.html#becoming-an-unprivileged-user')
-                        res = self._remote_chmod('a+%s' % mode, remote_path, recursive=recursive)
+                        res = self._remote_chmod(remote_paths, 'a+%s' % mode)
                         if res['rc'] != 0:
                             raise AnsibleError('Failed to set file mode on remote files (rc: {0}, err: {1})'.format(res['rc'], res['stderr']))
                     else:
@@ -368,33 +360,33 @@ class ActionBase(with_metaclass(ABCMeta, object)):
             # Can't depend on the file being transferred with execute
             # permissions.  Only need user perms because no become was
             # used here
-            res = self._remote_chmod('u+x', remote_path, recursive=recursive)
+            res = self._remote_chmod(remote_paths, 'u+x')
             if res['rc'] != 0:
                 raise AnsibleError('Failed to set file mode on remote files (rc: {0}, err: {1})'.format(res['rc'], res['stderr']))
 
-        return remote_path
+        return remote_paths
 
-    def _remote_chmod(self, mode, path, recursive=True, sudoable=False):
+    def _remote_chmod(self, paths, mode, sudoable=False):
         '''
         Issue a remote chmod command
         '''
-        cmd = self._connection._shell.chmod(mode, path, recursive=recursive)
+        cmd = self._connection._shell.chmod(paths, mode)
         res = self._low_level_execute_command(cmd, sudoable=sudoable)
         return res
 
-    def _remote_chown(self, path, user, group=None, recursive=True, sudoable=False):
+    def _remote_chown(self, paths, user, sudoable=False):
         '''
         Issue a remote chown command
         '''
-        cmd = self._connection._shell.chown(path, user, group, recursive=recursive)
+        cmd = self._connection._shell.chown(paths, user)
         res = self._low_level_execute_command(cmd, sudoable=sudoable)
         return res
 
-    def _remote_set_user_facl(self, path, user, mode, recursive=True, sudoable=False):
+    def _remote_set_user_facl(self, paths, user, mode, sudoable=False):
         '''
         Issue a remote call to setfacl
         '''
-        cmd = self._connection._shell.set_user_facl(path, user, mode, recursive=recursive)
+        cmd = self._connection._shell.set_user_facl(paths, user, mode)
         res = self._low_level_execute_command(cmd, sudoable=sudoable)
         return res
 
@@ -616,9 +608,17 @@ class ActionBase(with_metaclass(ABCMeta, object)):
 
         environment_string = self._compute_environment_string()
 
+        remote_files = None
+
+        if args_file_path:
+            remote_files = tmp, remote_module_path, args_file_path
+        elif remote_module_path:
+            remote_files = tmp, remote_module_path
+
         # Fix permissions of the tmp path and tmp files.  This should be
         # called after all files have been transferred.
-        self._fixup_perms(tmp, remote_user, recursive=True)
+        if remote_files:
+            self._fixup_perms(remote_files, remote_user)
 
         cmd = ""
         in_data = None

lib/ansible/plugins/action/assemble.py

@@ -159,7 +159,7 @@ class ActionModule(ActionBase):
             xfered = self._transfer_file(path, remote_path)
 
             # fix file permissions when the copy is done as a different user
-            self._fixup_perms(tmp, remote_user, recursive=True)
+            self._fixup_perms((tmp, remote_path), remote_user)
 
             new_module_args.update( dict( src=xfered,))
 

lib/ansible/plugins/action/async.py

@@ -73,17 +73,13 @@ class ActionModule(ActionBase):
                 args_data += '%s="%s" ' % (k, pipes.quote(to_unicode(v)))
             argsfile = self._transfer_data(self._connection._shell.join_path(tmp, 'arguments'), args_data)
 
-        self._fixup_perms(tmp, remote_user, execute=True, recursive=True)
-        # Only the following two files need to be executable but we'd have to
-        # make three remote calls if we wanted to just set them executable.
-        # There's not really a problem with marking too many of the temp files
-        # executable so we go ahead and mark them all as executable in the
-        # line above (the line above is needed in any case [although
-        # execute=False is okay if we uncomment the lines below] so that all
-        # the files are readable in case the remote_user and become_user are
-        # different and both unprivileged)
-        #self._fixup_perms(remote_module_path, remote_user, execute=True, recursive=False)
-        #self._fixup_perms(async_module_path, remote_user, execute=True, recursive=False)
+        remote_paths = tmp, remote_module_path, async_module_path
+
+        # argsfile doesn't need to be executable, but this saves an extra call to the remote host
+        if argsfile:
+            remote_paths += argsfile,
+
+        self._fixup_perms(remote_paths, remote_user, execute=True)
 
         async_limit = self._task.async
         async_jid   = str(random.randint(0, 999999999999))

lib/ansible/plugins/action/copy.py

@@ -213,8 +213,10 @@ class ActionModule(ActionBase):
                 # Define a remote directory that we will copy the file to.
                 tmp_src = self._connection._shell.join_path(tmp, 'source')
 
+                remote_path = None
+
                 if not raw:
-                    self._transfer_file(source_full, tmp_src)
+                    remote_path = self._transfer_file(source_full, tmp_src)
                 else:
                     self._transfer_file(source_full, dest_file)
 
@@ -223,7 +225,8 @@ class ActionModule(ActionBase):
                 self._loader.cleanup_tmp_file(source_full)
 
                 # fix file permissions when the copy is done as a different user
-                self._fixup_perms(tmp, remote_user, recursive=True)
+                if remote_path:
+                    self._fixup_perms((tmp, remote_path), remote_user)
 
                 if raw:
                     # Continue to next iteration if raw is defined.

lib/ansible/plugins/action/patch.py

@@ -63,7 +63,7 @@ class ActionModule(ActionBase):
         tmp_src = self._connection._shell.join_path(tmp, os.path.basename(src))
         self._transfer_file(src, tmp_src)
 
-        self._fixup_perms(tmp, remote_user, recursive=True)
+        self._fixup_perms((tmp, tmp_src), remote_user)
 
         new_module_args = self._task.args.copy()
         new_module_args.update(

lib/ansible/plugins/action/script.py

@@ -81,7 +81,7 @@ class ActionModule(ActionBase):
         self._transfer_file(source, tmp_src)
 
         # set file permissions, more permissive when the copy is done as a different user
-        self._fixup_perms(tmp, remote_user, execute=True, recursive=True)
+        self._fixup_perms((tmp, tmp_src), remote_user, execute=True)
 
         # add preparation steps to one ssh roundtrip executing the script
         env_string = self._compute_environment_string()

lib/ansible/plugins/action/template.py

@@ -166,7 +166,7 @@ class ActionModule(ActionBase):
                 xfered = self._transfer_data(self._connection._shell.join_path(tmp, 'source'), resultant)
 
                 # fix file permissions when the copy is done as a different user
-                self._fixup_perms(tmp, remote_user, recursive=True)
+                self._fixup_perms((tmp, xfered), remote_user)
 
                 # run the copy module
                 new_module_args.update(

lib/ansible/plugins/action/unarchive.py

@@ -97,7 +97,7 @@ class ActionModule(ActionBase):
 
         if copy:
             # fix file permissions when the copy is done as a different user
-            self._fixup_perms(tmp, remote_user, recursive=True)
+            self._fixup_perms((tmp, tmp_src), remote_user)
             # Build temporary module_args.
             new_module_args = self._task.args.copy()
             new_module_args.update(

lib/ansible/plugins/shell/__init__.py

@@ -57,45 +57,25 @@ class ShellBase(object):
     def path_has_trailing_slash(self, path):
         return path.endswith('/')
 
-    def chmod(self, mode, path, recursive=True):
-        path = pipes.quote(path)
-        cmd = ['chmod']
-
-        if recursive:
-            cmd.append('-R') # many chmods require -R before file list
-
-        cmd.extend([mode, path])
+    def chmod(self, paths, mode):
+        cmd = ['chmod', mode]
+        cmd.extend(paths)
+        cmd = [pipes.quote(c) for c in cmd]
 
         return ' '.join(cmd)
 
-    def chown(self, path, user, group=None, recursive=True):
-        path = pipes.quote(path)
-        user = pipes.quote(user)
-
-        cmd = ['chown']
-
-        if recursive:
-            cmd.append('-R') # many chowns require -R before file list
-
-        if group is None:
-            cmd.extend([user, path])
-        else:
-            group = pipes.quote(group)
-            cmd.extend(['%s:%s' % (user, group), path])
+    def chown(self, paths, user):
+        cmd = ['chown', user]
+        cmd.extend(paths)
+        cmd = [pipes.quote(c) for c in cmd]
 
         return ' '.join(cmd)
 
-    def set_user_facl(self, path, user, mode, recursive=True):
+    def set_user_facl(self, paths, user, mode):
         """Only sets acls for users as that's really all we need"""
-        path = pipes.quote(path)
-        mode = pipes.quote(mode)
-        user = pipes.quote(user)
-
         cmd = ['setfacl', '-m', 'u:%s:%s' % (user, mode)]
-        if recursive:
-            cmd = ['find', path, '-exec'] + cmd + ["'{}'", "'+'"]
-        else:
-            cmd.append(path)
+        cmd.extend(paths)
+        cmd = [pipes.quote(c) for c in cmd]
 
         return ' '.join(cmd)
 

lib/ansible/plugins/shell/powershell.py

@@ -68,13 +68,13 @@ class ShellModule(object):
         path = self._unquote(path)
         return path.endswith('/') or path.endswith('\\')
 
-    def chmod(self, mode, path, recursive=True):
+    def chmod(self, paths, mode):
         raise NotImplementedError('chmod is not implemented for Powershell')
 
-    def chown(self, path, user, group=None, recursive=True):
+    def chown(self, paths, user):
         raise NotImplementedError('chown is not implemented for Powershell')
 
-    def set_user_facl(self, path, user, mode, recursive=True):
+    def set_user_facl(self, paths, user, mode):
         raise NotImplementedError('set_user_facl is not implemented for Powershell')
 
     def remove(self, path, recurse=False):

test/utils/shippable/remote-integration.sh

@@ -15,7 +15,7 @@ test_flags="${TEST_FLAGS:-}"
 force_color="${FORCE_COLOR:-1}"
 
 # FIXME: these tests fail
-skip_tags='test_copy,test_template,test_unarchive,test_command_shell,test_sudo,test_become,test_service,test_postgresql,test_mysql_db,test_mysql_user,test_mysql_variables,test_uri,test_get_url'
+skip_tags='test_copy,test_template,test_unarchive,test_command_shell,test_service,test_postgresql,test_mysql_db,test_mysql_user,test_mysql_variables,test_uri,test_get_url'
 
 cd ~/