[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>
This commit is contained in:
parent
7786dbbdc1
commit
7d64aebdd3
3 changed files with 42 additions and 9 deletions
4
changelogs/fragments/macos-solaris-regression.yml
Normal file
4
changelogs/fragments/macos-solaris-regression.yml
Normal file
|
@ -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…
Reference in a new issue