ensure predictable permissions on module artifacts (#84948) (#85073)

* ensure predictable permissions on module artifacts (#84948)

and test it!

(cherry picked from commit 9f894b81c2)

* missing aliases

changelogs/fragments/ensure_remote_perms.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - Ansible will now ensure predictable permissions on remote artifacts, until now it only ensured executable and relied on system masks for the rest.

lib/ansible/plugins/action/__init__.py

@@ -634,12 +634,12 @@ class ActionBase(ABC):
         # done. Make the files +x if we're asked to, and return.
         if not self._is_become_unprivileged():
             if execute:
-                # Can't depend on the file being transferred with execute permissions.
+                # Can't depend on the file being transferred with required permissions.
                 # Only need user perms because no become was used here
-                res = self._remote_chmod(remote_paths, 'u+x')
+                res = self._remote_chmod(remote_paths, 'u+rwx')
                 if res['rc'] != 0:
                     raise AnsibleError(
-                        'Failed to set execute bit on remote files '
+                        'Failed to set permissions on remote files '
                         '(rc: {0}, err: {1})'.format(
                             res['rc'],
                             to_native(res['stderr'])))
@@ -680,10 +680,10 @@ class ActionBase(ABC):
             return remote_paths
 
         # Step 3b: Set execute if we need to. We do this before anything else
-        # because some of the methods below might work but not let us set +x
+        # because some of the methods below might work but not let us set
-        # as part of them.
+        # permissions as part of them.
         if execute:
-            res = self._remote_chmod(remote_paths, 'u+x')
+            res = self._remote_chmod(remote_paths, 'u+rwx')
             if res['rc'] != 0:
                 raise AnsibleError(
                     'Failed to set file mode or acl on remote temporary files '

test/integration/targets/ansiballz_debug/aliases

@@ -0,0 +1,3 @@
+shippable/posix/group5
+context/controller
+gather_facts/no

test/integration/targets/ansiballz_debug/tasks/main.yml

@@ -0,0 +1,59 @@
+- name: Run a module while preserving the generated AnsiballZ wrapper
+  command: ansible -m ping localhost -vvv
+  environment:
+    ANSIBLE_KEEP_REMOTE_FILES: 1
+  register: wrapper
+
+- name: Locate the generated AnsiballZ wrapper
+  set_fact:
+    generated_wrapper: "{{ (wrapper.stdout | regex_search('PUT .*? TO (/.*?/AnsiballZ_ping.py)', '\\1'))[0] }}"
+
+- name: Check permissions
+  stat:
+    path: '{{ generated_wrapper }}'
+  register: wrapper_stats
+
+- name: Ensure permissions
+  assert:
+    that:
+      - wrapper_stats.stat.executable is true
+      - wrapper_stats.stat.readable is true
+      - wrapper_stats.stat.writeable is true
+
+- name: Explode the wrapper
+  command: "{{ generated_wrapper }} explode"
+  register: explode
+
+- name: Locate the exploded results
+  set_fact:
+    exploded_dir: "{{ (explode.stdout | regex_search('^Module expanded into:\n(.*)$', '\\1', multiline=True))[0] }}"
+
+- name: Spot check the exploded results contents
+  assert:
+    that:
+      - (exploded_dir + '/args') is file
+      - (exploded_dir + '/ansible/modules/ping.py') is file
+
+- name: Execute the wrapper
+  command: "{{ generated_wrapper }} execute"
+  register: execute
+
+- name: Deserialize the result
+  set_fact:
+    result: "{{ execute.stdout | from_json }}"
+
+- name: Spot check the result
+  assert:
+    that:
+      - result.invocation.module_args.data == "pong"
+      - result.ping == "pong"
+
+- name: Remove wrapper
+  file:
+    path: "{{ generated_wrapper }}"
+    state: absent
+
+- name: Remove exploded files
+  file:
+    path: "{{ exploded_dir }}"
+    state: absent

test/units/plugins/action/test_action.py

@@ -401,7 +401,7 @@ class TestActionBase(unittest.TestCase):
             'stderr': 'and here',
         }
         assertThrowRegex(
-            'Failed to set execute bit on remote files',
+            'Failed to set permissions on remote files',
             execute=True)
 
         # Step 3: we are becoming unprivileged
@@ -416,7 +416,7 @@ class TestActionBase(unittest.TestCase):
         }
         assertSuccess()
 
-        # Step 3b: chmod +x if we need to
+        # Step 3b: chmod +rwx if we need to
         # To get here, setfacl failed, so mock it as such.
         action_base._remote_set_user_facl.return_value = {
             'rc': 1,