From 0d7c144ce44cd40ffa7c109a027d0927961d6a63 Mon Sep 17 00:00:00 2001 From: Rick Elrod Date: Tue, 4 Aug 2020 13:32:48 -0500 Subject: [PATCH] Allow macOS ACLs to work for unpriv -> unpriv (#70785) 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 --- changelogs/fragments/macos-chmod-acl.yml | 2 ++ docs/docsite/rst/user_guide/become.rst | 5 ++- lib/ansible/plugins/action/__init__.py | 18 ++++++++-- .../targets/become_unprivileged/aliases | 1 + .../chmod_acl_macos/test.yml | 26 ++++++++++++++ .../targets/become_unprivileged/runme.sh | 35 ++++++++++++++----- test/units/plugins/action/test_action.py | 34 +++++++++++------- 7 files changed, 97 insertions(+), 24 deletions(-) create mode 100644 changelogs/fragments/macos-chmod-acl.yml create mode 100644 test/integration/targets/become_unprivileged/chmod_acl_macos/test.yml diff --git a/changelogs/fragments/macos-chmod-acl.yml b/changelogs/fragments/macos-chmod-acl.yml new file mode 100644 index 00000000000..ed517e22491 --- /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 2ab7cba9de4..9bf70894b71 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 11d77817427..15ce29eb0d0 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/lib/ansible/plugins/action/__init__.py @@ -518,7 +518,9 @@ class ActionBase(with_metaclass(ABCMeta, object)): 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 @@ -571,12 +573,16 @@ class ActionBase(with_metaclass(ABCMeta, object)): 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( @@ -613,7 +619,15 @@ class ActionBase(with_metaclass(ABCMeta, object)): '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 1067ca06a55..c96617f60c7 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 00000000000..eaa5f5f82bb --- /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 b7f38612920..7a3f7b87b10 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 4ac793d57e6..86ef3d2e95f 100644 --- a/test/units/plugins/action/test_action.py +++ b/test/units/plugins/action/test_action.py @@ -358,6 +358,9 @@ class TestActionBase(unittest.TestCase): 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 @@ class TestActionBase(unittest.TestCase): '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 @@ class TestActionBase(unittest.TestCase): 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 @@ class TestActionBase(unittest.TestCase): 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 @@ class TestActionBase(unittest.TestCase): } # 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 @@ class TestActionBase(unittest.TestCase): 'stderr': '', } assertSuccess() + action_base._remote_chmod = MagicMock() action_base._remote_chmod.return_value = { 'rc': 1, 'stdout': 'some stuff here',