From 7d64aebdd32c0578fae2a5a9b022f5906b9152ca Mon Sep 17 00:00:00 2001 From: Rick Elrod Date: Fri, 16 Apr 2021 12:18:00 -0500 Subject: [PATCH] [become] Fix solaris permissions regression Change: - Regression introduced in #70785 - When macOS chmod ACL syntax is used, Solaris-derived chmods return with a status of 5. This is also used for our sshpass handling, because sshpass will return 5 on auth failure. This means on Solaris, we incorrectly assume auth failure when we reach this branch of logic and try to run chmod with macOS syntax. - We now wrap this specific use of chmod in an exception handler that looks for AnsibleAuthenticationFailure and skips over it. This adds another authentication attempt (something we normally avoid to prevent account lockout), but seems better than the regression of not allowing other fallbacks to be used. - Without this patch, if setfacl fails on Solaris (and sshpass is used), we do not try common_remote_group or world-readable tmpdir fallbacks. Test Plan: - New unit Signed-off-by: Rick Elrod --- .../fragments/macos-solaris-regression.yml | 4 +++ lib/ansible/plugins/action/__init__.py | 19 ++++++++++--- test/units/plugins/action/test_action.py | 28 +++++++++++++++---- 3 files changed, 42 insertions(+), 9 deletions(-) create mode 100644 changelogs/fragments/macos-solaris-regression.yml diff --git a/changelogs/fragments/macos-solaris-regression.yml b/changelogs/fragments/macos-solaris-regression.yml new file mode 100644 index 00000000000..a7c685bff6d --- /dev/null +++ b/changelogs/fragments/macos-solaris-regression.yml @@ -0,0 +1,4 @@ +bugfixes: + - >- + become - fix a regression on Solaris where chmod can return 5 which we + interpret as auth failure and stop trying become tmpdir permission fallbacks diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index ed3f3eff13b..ccb9002fd0e 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/lib/ansible/plugins/action/__init__.py @@ -18,7 +18,7 @@ import time from abc import ABCMeta, abstractmethod from ansible import constants as C -from ansible.errors import AnsibleError, AnsibleConnectionFailure, AnsibleActionSkip, AnsibleActionFail, AnsiblePluginRemovedError +from ansible.errors import AnsibleError, AnsibleConnectionFailure, AnsibleActionSkip, AnsibleActionFail, AnsiblePluginRemovedError, AnsibleAuthenticationFailure from ansible.executor.module_common import modify_module from ansible.executor.interpreter_discovery import discover_interpreter, InterpreterDiscoveryRequiredError from ansible.module_utils.common._collections_compat import Sequence @@ -624,9 +624,20 @@ class ActionBase(with_metaclass(ABCMeta, object)): # 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 + try: + res = self._remote_chmod([chmod_acl_mode] + remote_paths, '+a') + except AnsibleAuthenticationFailure as e: + # Solaris-based chmod will return 5 when it sees an invalid mode, + # and +a is invalid there. Because it returns 5, which is the same + # thing sshpass returns on auth failure, our sshpass code will + # assume that auth failed. If we don't handle that case here, none + # of the other logic below will get run. This is fairly hacky and a + # corner case, but probably one that shows up pretty often in + # Solaris-based environments (and possibly others). + pass + else: + 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 diff --git a/test/units/plugins/action/test_action.py b/test/units/plugins/action/test_action.py index 86ef3d2e95f..b31048c1dc2 100644 --- a/test/units/plugins/action/test_action.py +++ b/test/units/plugins/action/test_action.py @@ -27,7 +27,7 @@ from ansible import constants as C from units.compat import unittest from units.compat.mock import patch, MagicMock, mock_open -from ansible.errors import AnsibleError +from ansible.errors import AnsibleError, AnsibleAuthenticationFailure from ansible.module_utils.six import text_type from ansible.module_utils.six.moves import shlex_quote, builtins from ansible.module_utils._text import to_bytes @@ -332,6 +332,9 @@ class TestActionBase(unittest.TestCase): remote_paths = ['/tmp/foo/bar.txt', '/tmp/baz.txt'] remote_user = 'remoteuser1' + # Used for skipping down to common group dir. + CHMOD_ACL_FLAGS = ('+a', 'A+user:remoteuser2:r:allow') + def runWithNoExpectation(execute=False): return action_base._fixup_perms2( remote_paths, @@ -455,12 +458,27 @@ class TestActionBase(unittest.TestCase): ['remoteuser2 allow read'] + remote_paths, '+a') + # This case can cause Solaris chmod to return 5 which the ssh plugin + # treats as failure. To prevent a regression and ensure we still try the + # rest of the cases below, we mock the thrown exception here. + # This function ensures that only the macOS case (+a) throws this. + def raise_if_plus_a(definitely_not_underscore, mode): + if mode == '+a': + raise AnsibleAuthenticationFailure() + return {'rc': 0, 'stdout': '', 'stderr': ''} + + action_base._remote_chmod.side_effect = raise_if_plus_a + assertSuccess() + # Step 3e: Common group + def rc_1_if_chmod_acl(definitely_not_underscore, mode): + rc = 0 + if mode in CHMOD_ACL_FLAGS: + rc = 1 + return {'rc': rc, 'stdout': '', 'stderr': ''} + 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='') + action_base._remote_chmod.side_effect = rc_1_if_chmod_acl get_shell_option = action_base.get_shell_option action_base.get_shell_option = MagicMock()