[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 <rick@elrod.me>
pull/74418/head
Rick Elrod 4 years ago
parent 2c9389b193
commit 341834fe70

@ -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

@ -18,7 +18,7 @@ import time
from abc import ABCMeta, abstractmethod from abc import ABCMeta, abstractmethod
from ansible import constants as C 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.module_common import modify_module
from ansible.executor.interpreter_discovery import discover_interpreter, InterpreterDiscoveryRequiredError from ansible.executor.interpreter_discovery import discover_interpreter, InterpreterDiscoveryRequiredError
from ansible.module_utils.common._collections_compat import Sequence 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 # 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 # pass that argument as the first element of remote_paths. So we end
# up running `chmod +a [that argument] [file 1] [file 2] ...` # up running `chmod +a [that argument] [file 1] [file 2] ...`
res = self._remote_chmod([chmod_acl_mode] + remote_paths, '+a') try:
if res['rc'] == 0: res = self._remote_chmod([chmod_acl_mode] + remote_paths, '+a')
return remote_paths 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 # Step 3e: Common group
# Otherwise, we're a normal user. We failed to chown the paths to the # Otherwise, we're a normal user. We failed to chown the paths to the

@ -27,7 +27,7 @@ from ansible import constants as C
from units.compat import unittest from units.compat import unittest
from units.compat.mock import patch, MagicMock, mock_open 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 import text_type
from ansible.module_utils.six.moves import shlex_quote, builtins from ansible.module_utils.six.moves import shlex_quote, builtins
from ansible.module_utils._text import to_bytes 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_paths = ['/tmp/foo/bar.txt', '/tmp/baz.txt']
remote_user = 'remoteuser1' remote_user = 'remoteuser1'
# Used for skipping down to common group dir.
CHMOD_ACL_FLAGS = ('+a', 'A+user:remoteuser2:r:allow')
def runWithNoExpectation(execute=False): def runWithNoExpectation(execute=False):
return action_base._fixup_perms2( return action_base._fixup_perms2(
remote_paths, remote_paths,
@ -455,12 +458,27 @@ class TestActionBase(unittest.TestCase):
['remoteuser2 allow read'] + remote_paths, ['remoteuser2 allow read'] + remote_paths,
'+a') '+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 # 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 = MagicMock()
action_base._remote_chmod.side_effect = \ action_base._remote_chmod.side_effect = rc_1_if_chmod_acl
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 get_shell_option = action_base.get_shell_option
action_base.get_shell_option = MagicMock() action_base.get_shell_option = MagicMock()

Loading…
Cancel
Save