From 72cca01cd4656c9d152b5ce67c0204f6e8580582 Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Fri, 5 Aug 2016 18:40:28 -0700 Subject: [PATCH] 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 | 46 +++++++++++----------- lib/ansible/plugins/action/assemble.py | 2 +- lib/ansible/plugins/action/async.py | 18 ++++----- lib/ansible/plugins/action/copy.py | 7 +++- lib/ansible/plugins/action/patch.py | 2 +- lib/ansible/plugins/action/script.py | 2 +- lib/ansible/plugins/action/template.py | 2 +- lib/ansible/plugins/action/unarchive.py | 2 +- lib/ansible/plugins/shell/__init__.py | 42 ++++++-------------- lib/ansible/plugins/shell/powershell.py | 6 +-- test/utils/shippable/remote-integration.sh | 2 +- 11 files changed, 55 insertions(+), 76 deletions(-) diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index b4f71b17a9a..d6e4ffbe980 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/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 diff --git a/lib/ansible/plugins/action/assemble.py b/lib/ansible/plugins/action/assemble.py index 0898465a03b..ed842f729c7 100644 --- a/lib/ansible/plugins/action/assemble.py +++ b/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,)) diff --git a/lib/ansible/plugins/action/async.py b/lib/ansible/plugins/action/async.py index 1ea099437d1..ccfebb951a8 100644 --- a/lib/ansible/plugins/action/async.py +++ b/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)) diff --git a/lib/ansible/plugins/action/copy.py b/lib/ansible/plugins/action/copy.py index 43a5c6633db..95dc4ade29b 100644 --- a/lib/ansible/plugins/action/copy.py +++ b/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. diff --git a/lib/ansible/plugins/action/patch.py b/lib/ansible/plugins/action/patch.py index 7f388fce8f9..bd5eb92f301 100644 --- a/lib/ansible/plugins/action/patch.py +++ b/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( diff --git a/lib/ansible/plugins/action/script.py b/lib/ansible/plugins/action/script.py index 99a8f978cf5..7b280ea5427 100644 --- a/lib/ansible/plugins/action/script.py +++ b/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() diff --git a/lib/ansible/plugins/action/template.py b/lib/ansible/plugins/action/template.py index 88ccdcd2a51..760db83ba82 100644 --- a/lib/ansible/plugins/action/template.py +++ b/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( diff --git a/lib/ansible/plugins/action/unarchive.py b/lib/ansible/plugins/action/unarchive.py index f558b35e94f..a160ad83d29 100644 --- a/lib/ansible/plugins/action/unarchive.py +++ b/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( diff --git a/lib/ansible/plugins/shell/__init__.py b/lib/ansible/plugins/shell/__init__.py index 9bd92562b55..44af0abb741 100644 --- a/lib/ansible/plugins/shell/__init__.py +++ b/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) diff --git a/lib/ansible/plugins/shell/powershell.py b/lib/ansible/plugins/shell/powershell.py index 505b2e01da7..1e5c71cf497 100644 --- a/lib/ansible/plugins/shell/powershell.py +++ b/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): diff --git a/test/utils/shippable/remote-integration.sh b/test/utils/shippable/remote-integration.sh index cf3f6ce7291..8b523f225fa 100644 --- a/test/utils/shippable/remote-integration.sh +++ b/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 ~/