Repository: ansible/ansible
Pull request: Allow macOS ACLs to work for unpriv -> unpriv
Change:
- Use `chmod +a` in the fallback chain to allow MacOS to use ACLs to
  allow an unprivileged user to become an unprivileged user.

Test Plan:
- CI, new tests

Tickets:
- Fixes #70648

Signed-off-by: Rick Elrod <rick@elrod.me>

##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->

<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Feature Pull Request

##### COMPONENT NAME
action

Problem statement:
Use `chmod` instead of `setfacl` on macOS when becoming an unprivileged user
##### SUMMARY
Support become unprivileged user on macOS without `allow_world_readable_tmpfiles=true`

##### ISSUE TYPE
- Feature Idea

##### COMPONENT NAME
action, shell, and copy

##### ADDITIONAL INFORMATION
When using become where the login user and the become user are both unprivileged, Ansible attempts to securely change file permissions to only allow the become user to read the files. It uses `setfacl` and falls back to `chown`. Both of these fail on macOS, as there is no `setfacl` command and `chown` is not permitted.

macOS actually does provide the same functionality as `setfacl`, using `chmod +a`. The syntax is a bit different from `setfacl`, but it should be able to do what Ansible needs.

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/copy.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/copy.py)
* [`lib/ansible/modules/shell.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/shell.py)
* [`lib/ansible/plugins/action/__init__.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/action/__init__.py)

If these files are incorrect, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
Yep, this seems like a reasonable idea to me.
I think ultimately this is going to require a feature we don't have yet, which has been termed `ansible_platform`

We don't really know what platform we are talking to at this point, so it's hard to make that differentiation currently.

cc @nitzmahone

Commit message: Allow MacOS ACLs to work for unpriv -> unpriv

Change:
- Use `chmod +a` in the fallback chain to allow MacOS to use ACLs to
  allow an unprivileged user to become an unprivileged user.

Test Plan:
- CI, new tests

Tickets:
- Fixes #70648

Signed-off-by: Rick Elrod <rick@elrod.me>

Patch to review:
diff --git a/changelogs/fragments/macos-chmod-acl.yml b/changelogs/fragments/macos-chmod-acl.yml
new file mode 100644
index 00000000000000..cbe067c79796d7
--- /dev/null
+++ b/changelogs/fragments/macos-chmod-acl.yml
@@ -0,0 +1,2 @@
+minor_changes:
+  - When connecting as an unprivileged user, and becoming an unprivileged user, we now fall back to also trying ``chmod +a`` which works on MacOS and makes use of ACLs.
diff --git a/docs/docsite/rst/user_guide/become.rst b/docs/docsite/rst/user_guide/become.rst
index 2ab7cba9de4ae8..161b22cda84c92 100644
--- a/docs/docsite/rst/user_guide/become.rst
+++ b/docs/docsite/rst/user_guide/become.rst
@@ -147,7 +147,10 @@ Next, if POSIX ACLs are **not** available or :command:`setfacl` could not be
 run, Ansible will attempt to change ownership of the module file using
 :command:`chown` for systems which support doing so as an unprivileged user.
 
-New in Ansible 2.10, if the :command:`chown` fails, Ansible will then check the
+New in Ansible 2.11, at this point, Ansible will try :command:`chmod +a` which
+is a MacOS-specific way of setting ACLs on files.
+
+New in Ansible 2.10, if all of the above fails, Ansible will then check the
 value of the configuration setting ``ansible_common_remote_group``. Many
 systems will allow a given user to change the group ownership of a file to a
 group the user is in. As a result, if the second unprivileged user (the
diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py
index 62bdd851158bed..1a0fbd13a73b75 100644
--- a/lib/ansible/plugins/action/__init__.py
+++ b/lib/ansible/plugins/action/__init__.py
@@ -510,7 +510,9 @@ def _fixup_perms2(self, remote_paths, remote_user=None, execute=True):
           file with chown which only works in case the remote_user is
           privileged or the remote systems allows chown calls by unprivileged
           users (e.g. HP-UX)
-        * If the chown fails, we check if ansible_common_remote_group is set.
+        * If the above fails, we next try 'chmod +a' which is a MacOS way of
+          setting ACLs on files.
+        * If the above fails, we check if ansible_common_remote_group is set.
           If it is, we attempt to chgrp the file to its value. This is useful
           if the remote_user has a group in common with the become_user. As the
           remote_user, we can chgrp the file to that group and allow the
@@ -563,12 +565,16 @@ def _fixup_perms2(self, remote_paths, remote_user=None, execute=True):
         if execute:
             chmod_mode = 'rx'
             setfacl_mode = 'r-x'
+            # Apple patches their "file_cmds" chmod with ACL support
+            chmod_acl_mode = '{0} allow read,execute'.format(become_user)
         else:
             chmod_mode = 'rX'
             # TODO: this form fails silently on freebsd.  We currently
             # never call _fixup_perms2() with execute=False but if we
             # start to we'll have to fix this.
             setfacl_mode = 'r-X'
+            # Apple
+            chmod_acl_mode = '{0} allow read'.format(become_user)
 
         # Step 3a: Are we able to use setfacl to add user ACLs to the file?
         res = self._remote_set_user_facl(
@@ -605,7 +611,15 @@ def _fixup_perms2(self, remote_paths, remote_user=None, execute=True):
                 'Unprivileged become user would be unable to read the '
                 'file.')
 
-        # Step 3d: Common group
+        # Step 3d: Try macOS's special chmod + ACL
+        # macOS chmod's +a flag takes its own argument. As a slight hack, we
+        # pass that argument as the first element of remote_paths. So we end
+        # up running `chmod +a [that argument] [file 1] [file 2] ...`
+        res = self._remote_chmod([chmod_acl_mode] + remote_paths, '+a')
+        if res['rc'] == 0:
+            return remote_paths
+
+        # Step 3e: Common group
         # Otherwise, we're a normal user. We failed to chown the paths to the
         # unprivileged user, but if we have a common group with them, we should
         # be able to chown it to that.
diff --git a/test/integration/targets/become_unprivileged/aliases b/test/integration/targets/become_unprivileged/aliases
index 1067ca06a55741..c96617f60c764b 100644
--- a/test/integration/targets/become_unprivileged/aliases
+++ b/test/integration/targets/become_unprivileged/aliases
@@ -2,3 +2,4 @@ destructive
 shippable/posix/group1
 skip/aix
 needs/ssh
+needs/root
diff --git a/test/integration/targets/become_unprivileged/chmod_acl_macos/test.yml b/test/integration/targets/become_unprivileged/chmod_acl_macos/test.yml
new file mode 100644
index 00000000000000..eaa5f5f82bbd77
--- /dev/null
+++ b/test/integration/targets/become_unprivileged/chmod_acl_macos/test.yml
@@ -0,0 +1,26 @@
+- name: Tests for chmod +a ACL functionality on macOS
+  hosts: ssh
+  gather_facts: yes
+  remote_user: unpriv1
+  become: yes
+  become_user: unpriv2
+
+  tasks:
+    - name: Get AnsiballZ temp directory
+      action: tmpdir
+      register: tmpdir
+      become_user: unpriv2
+      become: yes
+
+    - name: run whoami
+      command: whoami
+      register: whoami
+
+    - name: Ensure we used the right fallback
+      shell: ls -le /var/tmp/ansible*/*_command.py
+      register: ls
+
+    - assert:
+        that:
+          - whoami.stdout == "unpriv2"
+          - "'user:unpriv2 allow read' in ls.stdout"
diff --git a/test/integration/targets/become_unprivileged/runme.sh b/test/integration/targets/become_unprivileged/runme.sh
index b7f386129205ce..7a3f7b87b10e4d 100755
--- a/test/integration/targets/become_unprivileged/runme.sh
+++ b/test/integration/targets/become_unprivileged/runme.sh
@@ -2,6 +2,11 @@
 
 set -eux
 
+export ANSIBLE_KEEP_REMOTE_FILES=True
+ANSIBLE_ACTION_PLUGINS="$(pwd)/action_plugins"
+export ANSIBLE_ACTION_PLUGINS
+export ANSIBLE_BECOME_PASS='iWishIWereCoolEnoughForRoot!'
+
 begin_sandwich() {
     ansible-playbook setup_unpriv_users.yml -i inventory -v "$@"
 }
@@ -24,12 +29,24 @@ end_sandwich() {
 trap "end_sandwich \"\$@\"" EXIT
 
 # Common group tests
-begin_sandwich "$@"
-  ansible-playbook common_remote_group/setup.yml -i inventory -v "$@"
-  export ANSIBLE_KEEP_REMOTE_FILES=True
-  export ANSIBLE_COMMON_REMOTE_GROUP=commongroup
-  export ANSIBLE_BECOME_PASS='iWishIWereCoolEnoughForRoot!'
-  ANSIBLE_ACTION_PLUGINS="$(pwd)/action_plugins"
-  export ANSIBLE_ACTION_PLUGINS
-  ansible-playbook common_remote_group/test.yml -i inventory -v "$@"
-end_sandwich "$@"
+# Skip on macOS, chmod fallback will take over.
+# 1) chmod is stupidly hard to disable, so hitting this test case on macOS would
+#    be a suuuuuuper edge case scenario
+# 2) even if we can trick it so chmod doesn't exist, then other things break.
+#    Ansible wants a `chmod` around, even if it's not the final thing that gets
+#    us enough permission to run the task.
+if [[ "$OSTYPE" != darwin* ]]; then
+  begin_sandwich "$@"
+    ansible-playbook common_remote_group/setup.yml -i inventory -v "$@"
+    export ANSIBLE_COMMON_REMOTE_GROUP=commongroup
+    ansible-playbook common_remote_group/test.yml -i inventory -v "$@"
+  end_sandwich "$@"
+fi
+
+if [[ "$OSTYPE" == darwin* ]]; then
+  begin_sandwich "$@"
+    # In the default case this should happen on macOS, so no need for a setup
+    # It should just work.
+    ansible-playbook chmod_acl_macos/test.yml -i inventory -v "$@"
+  end_sandwich "$@"
+fi
diff --git a/test/units/plugins/action/test_action.py b/test/units/plugins/action/test_action.py
index 4ac793d57e6c69..86ef3d2e95f465 100644
--- a/test/units/plugins/action/test_action.py
+++ b/test/units/plugins/action/test_action.py
@@ -358,6 +358,9 @@ def _helper(option, *args, **kwargs):
                 return args_kv.get(option, default)
             return _helper
 
+        action_base.get_become_option = MagicMock()
+        action_base.get_become_option.return_value = 'remoteuser2'
+
         # Step 1: On windows, we just return remote_paths
         action_base._connection._shell._IS_WINDOWS = True
         assertSuccess(execute=False)
@@ -412,12 +415,6 @@ def _helper(option, *args, **kwargs):
             'stdout': '',
             'stderr': '',
         }
-        action_base._remote_chmod.return_value = {
-            'rc': 0,
-            'stdout': 'some stuff here',
-            'stderr': '',
-        }
-        assertSuccess(execute=True)
         action_base._remote_chmod.return_value = {
             'rc': 1,
             'stdout': 'some stuff here',
@@ -426,6 +423,12 @@ def _helper(option, *args, **kwargs):
         assertThrowRegex(
             'Failed to set file mode on remote temporary file',
             execute=True)
+        action_base._remote_chmod.return_value = {
+            'rc': 0,
+            'stdout': 'some stuff here',
+            'stderr': '',
+        }
+        assertSuccess(execute=True)
 
         # Step 3c: chown
         action_base._remote_chown = MagicMock()
@@ -446,7 +449,18 @@ def _helper(option, *args, **kwargs):
         assertThrowRegex('user would be unable to read the file.')
         remote_user = 'remoteuser1'
 
-        # Step 3d: Common group
+        # Step 3d: chmod +a on osx
+        assertSuccess()
+        action_base._remote_chmod.assert_called_with(
+            ['remoteuser2 allow read'] + remote_paths,
+            '+a')
+
+        # Step 3e: Common group
+        action_base._remote_chmod = MagicMock()
+        action_base._remote_chmod.side_effect = \
+            lambda x, y: \
+            dict(rc=1, stdout='', stderr='') if y == '+a' \
+            else dict(rc=0, stdout='', stderr='')
 
         get_shell_option = action_base.get_shell_option
         action_base.get_shell_option = MagicMock()
@@ -463,11 +477,6 @@ def _helper(option, *args, **kwargs):
         }
         # TODO: Add test to assert warning is shown if
         # ALLOW_WORLD_READABLE_TMPFILES is set in this case.
-        action_base._remote_chmod.return_value = {
-            'rc': 0,
-            'stdout': '',
-            'stderr': '',
-        }
         assertSuccess()
         action_base._remote_chgrp.assert_called_once_with(
             remote_paths,
@@ -486,6 +495,7 @@ def _helper(option, *args, **kwargs):
             'stderr': '',
         }
         assertSuccess()
+        action_base._remote_chmod = MagicMock()
         action_base._remote_chmod.return_value = {
             'rc': 1,
             'stdout': 'some stuff here',


Code review comments:
diff_hunk: @@ -510,7 +510,9 @@ def _fixup_perms2(self, remote_paths, remote_user=None, execute=True):
           file with chown which only works in case the remote_user is
           privileged or the remote systems allows chown calls by unprivileged
           users (e.g. HP-UX)
-        * If the chown fails, we check if ansible_common_remote_group is set.
+        * If the above fails, we next try 'chmod +a' which is a MacOS way of
path: lib/ansible/plugins/action/__init__.py
text: @user1:
```suggestion
        * If the above fails, we next try 'chmod +a' which is a macOS way of
```