Merge pull request #923 from moreati/issue915

Fix [DEPRECATION WARNING]: The '_remote_checksum()' method is deprecated.

ansible_mitogen/plugins/action/mitogen_fetch.py

@@ -18,23 +18,17 @@ from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
 import os
-
-from ansible.module_utils._text import to_bytes
+import base64
+from ansible.errors import AnsibleError, AnsibleActionFail, AnsibleActionSkip
+from ansible.module_utils.common.text.converters import to_bytes, to_text
 from ansible.module_utils.six import string_types
 from ansible.module_utils.parsing.convert_bool import boolean
 from ansible.plugins.action import ActionBase
-from ansible.utils.hashing import checksum, md5, secure_hash
-from ansible.utils.path import makedirs_safe
-
+from ansible.utils.display import Display
+from ansible.utils.hashing import checksum, checksum_s, md5, secure_hash
+from ansible.utils.path import makedirs_safe, is_subpath
 
-REMOTE_CHECKSUM_ERRORS = {
-    '0': "unable to calculate the checksum of the remote file",
-    '1': "the remote file does not exist",
-    '2': "no read permission on remote file",
-    '3': "remote file is a directory, fetch cannot work on directories",
-    '4': "python isn't present on the system.  Unable to compute checksum",
-    '5': "stdlib json was not found on the remote machine. Only the raw module can work without those installed",
-}
+display = Display()
 
 
 class ActionModule(ActionBase):
@@ -45,36 +39,94 @@ class ActionModule(ActionBase):
             task_vars = dict()
 
         result = super(ActionModule, self).run(tmp, task_vars)
+        del tmp  # tmp no longer has any effect
+
         try:
             if self._play_context.check_mode:
-                result['skipped'] = True
-                result['msg'] = 'check mode not (yet) supported for this module'
-                return result
+                raise AnsibleActionSkip('check mode not (yet) supported for this module')
 
+            source = self._task.args.get('src', None)
+            original_dest = dest = self._task.args.get('dest', None)
             flat = boolean(self._task.args.get('flat'), strict=False)
             fail_on_missing = boolean(self._task.args.get('fail_on_missing', True), strict=False)
             validate_checksum = boolean(self._task.args.get('validate_checksum', True), strict=False)
 
+            msg = ''
             # validate source and dest are strings FIXME: use basic.py and module specs
-            source = self._task.args.get('src')
             if not isinstance(source, string_types):
-                result['msg'] = "Invalid type supplied for source option, it must be a string"
+                msg = "Invalid type supplied for source option, it must be a string"
 
-            dest = self._task.args.get('dest')
             if not isinstance(dest, string_types):
-                result['msg'] = "Invalid type supplied for dest option, it must be a string"
+                msg = "Invalid type supplied for dest option, it must be a string"
+
+            if source is None or dest is None:
+                msg = "src and dest are required"
 
-            if result.get('msg'):
-                result['failed'] = True
-                return result
+            if msg:
+                raise AnsibleActionFail(msg)
 
             source = self._connection._shell.join_path(source)
             source = self._remote_expand_user(source)
 
-            # calculate checksum for the remote file, don't bother if using
-            # become as slurp will be used Force remote_checksum to follow
-            # symlinks because fetch always follows symlinks
-            remote_checksum = self._remote_checksum(source, all_vars=task_vars, follow=True)
+            remote_stat = {}
+            remote_checksum = None
+            if True:
+                # Get checksum for the remote file even using become. Mitogen doesn't need slurp.
+                # Follow symlinks because fetch always follows symlinks
+                try:
+                    remote_stat = self._execute_remote_stat(source, all_vars=task_vars, follow=True)
+                except AnsibleError as ae:
+                    result['changed'] = False
+                    result['file'] = source
+                    if fail_on_missing:
+                        result['failed'] = True
+                        result['msg'] = to_text(ae)
+                    else:
+                        result['msg'] = "%s, ignored" % to_text(ae, errors='surrogate_or_replace')
+
+                    return result
+
+                remote_checksum = remote_stat.get('checksum')
+                if remote_stat.get('exists'):
+                    if remote_stat.get('isdir'):
+                        result['failed'] = True
+                        result['changed'] = False
+                        result['msg'] = "remote file is a directory, fetch cannot work on directories"
+
+                        # Historically, these don't fail because you may want to transfer
+                        # a log file that possibly MAY exist but keep going to fetch other
+                        # log files. Today, this is better achieved by adding
+                        # ignore_errors or failed_when to the task.  Control the behaviour
+                        # via fail_when_missing
+                        if not fail_on_missing:
+                            result['msg'] += ", not transferring, ignored"
+                            del result['changed']
+                            del result['failed']
+
+                        return result
+
+            # use slurp if permissions are lacking or privilege escalation is needed
+            remote_data = None
+            if remote_checksum in (None, '1', ''):
+                slurpres = self._execute_module(module_name='ansible.legacy.slurp', module_args=dict(src=source), task_vars=task_vars)
+                if slurpres.get('failed'):
+                    if not fail_on_missing:
+                        result['file'] = source
+                        result['changed'] = False
+                    else:
+                        result.update(slurpres)
+
+                    if 'not found' in slurpres.get('msg', ''):
+                        result['msg'] = "the remote file does not exist, not transferring, ignored"
+                    elif slurpres.get('msg', '').startswith('source is a directory'):
+                        result['msg'] = "remote file is a directory, fetch cannot work on directories"
+
+                    return result
+                else:
+                    if slurpres['encoding'] == 'base64':
+                        remote_data = base64.b64decode(slurpres['content'])
+                    if remote_data is not None:
+                        remote_checksum = checksum_s(remote_data)
 
             # calculate the destination name
             if os.path.sep not in self._connection._shell.join_path('a', ''):
@@ -83,13 +135,14 @@ class ActionModule(ActionBase):
             else:
                 source_local = source
 
-            dest = os.path.expanduser(dest)
+            # ensure we only use file name, avoid relative paths
+            if not is_subpath(dest, original_dest):
+                # TODO: ? dest = os.path.expanduser(dest.replace(('../','')))
+                raise AnsibleActionFail("Detected directory traversal, expected to be contained in '%s' but got '%s'" % (original_dest, dest))
+
             if flat:
                 if os.path.isdir(to_bytes(dest, errors='surrogate_or_strict')) and not dest.endswith(os.sep):
-                    result['msg'] = "dest is an existing directory, use a trailing slash if you want to fetch src into that directory"
-                    result['file'] = dest
-                    result['failed'] = True
-                    return result
+                    raise AnsibleActionFail("dest is an existing directory, use a trailing slash if you want to fetch src into that directory")
                 if dest.endswith(os.sep):
                     # if the path ends with "/", we'll use the source filename as the
                     # destination filename
@@ -106,23 +159,7 @@ class ActionModule(ActionBase):
                     target_name = self._play_context.remote_addr
                 dest = "%s/%s/%s" % (self._loader.path_dwim(dest), target_name, source_local)
 
-            dest = dest.replace("//", "/")
-
-            if remote_checksum in REMOTE_CHECKSUM_ERRORS:
-                result['changed'] = False
-                result['file'] = source
-                result['msg'] = REMOTE_CHECKSUM_ERRORS[remote_checksum]
-                # Historically, these don't fail because you may want to transfer
-                # a log file that possibly MAY exist but keep going to fetch other
-                # log files. Today, this is better achieved by adding
-                # ignore_errors or failed_when to the task.  Control the behaviour
-                # via fail_when_missing
-                if fail_on_missing:
-                    result['failed'] = True
-                    del result['changed']
-                else:
-                    result['msg'] += ", not transferring, ignored"
-                return result
+            dest = os.path.normpath(dest)
 
             # calculate checksum for the local file
             local_checksum = checksum(dest)
@@ -132,7 +169,15 @@ class ActionModule(ActionBase):
                 makedirs_safe(os.path.dirname(dest))
 
                 # fetch the file and check for changes
-                self._connection.fetch_file(source, dest)
+                if remote_data is None:
+                    self._connection.fetch_file(source, dest)
+                else:
+                    try:
+                        f = open(to_bytes(dest, errors='surrogate_or_strict'), 'wb')
+                        f.write(remote_data)
+                        f.close()
+                    except (IOError, OSError) as e:
+                        raise AnsibleActionFail("Failed to fetch the file: %s" % e)
                 new_checksum = secure_hash(dest)
                 # For backwards compatibility. We'll return None on FIPS enabled systems
                 try:
@@ -157,10 +202,6 @@ class ActionModule(ActionBase):
                 result.update(dict(changed=False, md5sum=local_md5, file=source, dest=dest, checksum=local_checksum))
 
         finally:
-            try:
-                self._remove_tmp_path(self._connection._shell.tmpdir)
-            except AttributeError:
-                # .tmpdir was added to ShellModule in v2.6.0, so old versions don't have it
-                pass
+            self._remove_tmp_path(self._connection._shell.tmpdir)
 
         return result

docs/changelog.rst

@@ -25,6 +25,7 @@ v0.3.3.dev0
 * :gh:issue:`918` Support Python 3.10
 * :gh:issue:`920` Support Ansible :ans:conn:`~podman` connection plugin
 * :gh:issue:`836` :func:`mitogen.utils.with_router` decorator preserves the docstring in addition to the name.
+* :gh:issue:`936` :ans:mod:`fetch` no longer emits `[DEPRECATION WARNING]: The '_remote_checksum()' method is deprecated.`
 
 
 v0.3.2 (2022-01-12)

tests/ansible/regression/issue_615__streaming_transfer.yml

@@ -1,23 +1,32 @@
 # issue #615: 'fetch' with become: was internally using slurp.
 
-- hosts: target
+- hosts: test-targets
   any_errors_fatal: True
   gather_facts: no
+  # Without Mitogen this causes Ansible to use the slurp module, which is *slow*
   become: true
   vars:
     mitogen_ssh_compression: false
   tasks:
-    - shell: |
-        dd if=/dev/zero of=/tmp/512mb.zero bs=1048576 count=512;
-        chmod go= /tmp/512mb.zero
+    - block:
+        - shell: |
+            dd if=/dev/zero of=/tmp/512mb.zero bs=1048576 count=512;
+            chmod go= /tmp/512mb.zero
 
-    - fetch:
-        src: /tmp/512mb.zero
-        dest: /tmp/fetch-out
+        - fetch:
+            src: /tmp/512mb.zero
+            dest: /tmp/fetch-out
 
-    - file:
-        path: /tmp/fetch-out
-        state: absent
-      delegate_to: localhost
+        - file:
+            path: /tmp/512mb.zero
+            state: absent
+
+        - file:
+            path: /tmp/fetch-out
+            state: absent
+          delegate_to: localhost
+          run_once: true
+      when:
+        - is_mitogen
   tags:
     - issue_615