diff options
| -rw-r--r-- | changelogs/fragments/ssh_connection_invalid_password.yaml | 2 | ||||
| -rw-r--r-- | lib/ansible/errors/__init__.py | 5 | ||||
| -rw-r--r-- | lib/ansible/plugins/connection/paramiko_ssh.py | 10 | ||||
| -rw-r--r-- | lib/ansible/plugins/connection/ssh.py | 91 | ||||
| -rw-r--r-- | test/units/plugins/connection/test_ssh.py | 28 |
5 files changed, 114 insertions, 22 deletions
diff --git a/changelogs/fragments/ssh_connection_invalid_password.yaml b/changelogs/fragments/ssh_connection_invalid_password.yaml new file mode 100644 index 0000000000..c04fa0679b --- /dev/null +++ b/changelogs/fragments/ssh_connection_invalid_password.yaml @@ -0,0 +1,2 @@ +bugfixes: + - ssh connection - do not retry with invalid credentials to prevent account lockout (https://github.com/ansible/ansible/issues/48422) diff --git a/lib/ansible/errors/__init__.py b/lib/ansible/errors/__init__.py index 35ceaf4949..d165720f86 100644 --- a/lib/ansible/errors/__init__.py +++ b/lib/ansible/errors/__init__.py @@ -219,6 +219,11 @@ class AnsibleConnectionFailure(AnsibleRuntimeError): pass +class AnsibleAuthenticationFailure(AnsibleConnectionFailure): + '''invalid username/password/key''' + pass + + class AnsibleCallbackError(AnsibleRuntimeError): ''' a callback failure ''' pass diff --git a/lib/ansible/plugins/connection/paramiko_ssh.py b/lib/ansible/plugins/connection/paramiko_ssh.py index e5b51bad0a..dbcb734b32 100644 --- a/lib/ansible/plugins/connection/paramiko_ssh.py +++ b/lib/ansible/plugins/connection/paramiko_ssh.py @@ -141,7 +141,12 @@ from distutils.version import LooseVersion from binascii import hexlify from ansible import constants as C -from ansible.errors import AnsibleError, AnsibleConnectionFailure, AnsibleFileNotFound +from ansible.errors import ( + AnsibleAuthenticationFailure, + AnsibleConnectionFailure, + AnsibleError, + AnsibleFileNotFound, +) from ansible.module_utils.six import iteritems from ansible.module_utils.six.moves import input from ansible.plugins.connection import ConnectionBase @@ -355,6 +360,9 @@ class Connection(ConnectionBase): ) except paramiko.ssh_exception.BadHostKeyException as e: raise AnsibleConnectionFailure('host key mismatch for %s' % e.hostname) + except paramiko.ssh_exception.AuthenticationException as e: + msg = 'Invalid/incorrect username/password. {0}'.format(to_text(e)) + raise AnsibleAuthenticationFailure(msg) except Exception as e: msg = to_text(e) if u"PID check failed" in msg: diff --git a/lib/ansible/plugins/connection/ssh.py b/lib/ansible/plugins/connection/ssh.py index 813220d460..61637d8301 100644 --- a/lib/ansible/plugins/connection/ssh.py +++ b/lib/ansible/plugins/connection/ssh.py @@ -151,8 +151,8 @@ DOCUMENTATION = ''' - section: ssh_connection key: retries vars: - - name: ansible_ssh_retries - version_added: '2.7' + - name: ansible_ssh_retries + version_added: '2.7' port: description: Remote port to connect to. type: int @@ -280,7 +280,12 @@ import time from functools import wraps from ansible import constants as C -from ansible.errors import AnsibleError, AnsibleConnectionFailure, AnsibleFileNotFound +from ansible.errors import ( + AnsibleAuthenticationFailure, + AnsibleConnectionFailure, + AnsibleError, + AnsibleFileNotFound, +) from ansible.errors import AnsibleOptionsError from ansible.compat import selectors from ansible.module_utils.six import PY3, text_type, binary_type @@ -307,6 +312,55 @@ class AnsibleControlPersistBrokenPipeError(AnsibleError): pass +def _handle_error(remaining_retries, command, return_tuple, no_log, host, display=display): + + # sshpass errors + if command == b'sshpass': + # Error 5 is invalid/incorrect password. Raise an exception to prevent retries from locking the account. + if return_tuple[0] == 5: + msg = 'Invalid/incorrect username/password. Skipping remaining {0} retries to prevent account lockout:'.format(remaining_retries) + if remaining_retries <= 0: + msg = 'Invalid/incorrect password:' + if no_log: + msg = '{0} <error censored due to no log>'.format(msg) + else: + msg = '{0} {1}'.format(msg, to_native(return_tuple[2].rstrip())) + raise AnsibleAuthenticationFailure(msg) + + # sshpass returns codes are 1-6. We handle 5 previously, so this catches other scenarios. + # No exception is raised, so the connection is retried. + elif return_tuple[0] in [1, 2, 3, 4, 6]: + msg = 'sshpass error:' + if no_log: + msg = '{0} <error censored due to no log>'.format(msg) + else: + msg = '{0} {1}'.format(msg, to_native(return_tuple[2].rstrip())) + + if return_tuple[0] == 255: + SSH_ERROR = True + for signature in b_NOT_SSH_ERRORS: + if signature in return_tuple[1]: + SSH_ERROR = False + break + + if SSH_ERROR: + msg = "Failed to connect to the host via ssh:" + if no_log: + msg = '{0} <error censored due to no log>'.format(msg) + else: + msg = '{0} {1}'.format(msg, to_native(return_tuple[2]).rstrip()) + raise AnsibleConnectionFailure(msg) + + # For other errors, no execption is raised so the connection is retried and we only log the messages + if 1 <= return_tuple[0] <= 254: + msg = "Failed to connect to the host via ssh:" + if no_log: + msg = '{0} <error censored due to no log>'.format(msg) + else: + msg = '{0} {1}'.format(msg, to_native(return_tuple[2]).rstrip()) + display.vvv(msg, host=host) + + def _ssh_retry(func): """ Decorator to retry ssh/scp/sftp in the case of a connection failure @@ -315,7 +369,8 @@ def _ssh_retry(func): * an exception is caught * ssh returns 255 Will not retry if - * remaining_tries is <2 + * sshpass returns 5 (invalid password, to prevent account lockouts) + * remaining_tries is < 2 * retries limit reached """ @wraps(func) @@ -333,7 +388,7 @@ def _ssh_retry(func): try: return_tuple = func(self, *args, **kwargs) if self._play_context.no_log: - display.vvv('rc=%s, stdout & stderr censored due to no log' % return_tuple[0], host=self.host) + display.vvv('rc=%s, stdout and stderr censored due to no log' % return_tuple[0], host=self.host) else: display.vvv(return_tuple, host=self.host) # 0 = success @@ -349,24 +404,18 @@ def _ssh_retry(func): display.vvv(u"RETRYING BECAUSE OF CONTROLPERSIST BROKEN PIPE") return_tuple = func(self, *args, **kwargs) - if return_tuple[0] == 255: - SSH_ERROR = True - for signature in b_NOT_SSH_ERRORS: - if signature in return_tuple[1]: - SSH_ERROR = False - break - - if SSH_ERROR: - msg = "Failed to connect to the host via ssh: " - if self._play_context.no_log: - msg += '<error censored due to no log>' - else: - msg += to_native(return_tuple[2]) - raise AnsibleConnectionFailure(msg) + remaining_retries = remaining_tries - attempt - 1 + _handle_error(remaining_retries, cmd[0], return_tuple, self._play_context.no_log, self.host) break + # 5 = Invalid/incorrect password from sshpass + except AnsibleAuthenticationFailure as e: + # Raising this exception, which is subclassed from AnsibleConnectionFailure, prevents further retries + raise + except (AnsibleConnectionFailure, Exception) as e: + if attempt == remaining_tries - 1: raise else: @@ -375,9 +424,9 @@ def _ssh_retry(func): pause = 30 if isinstance(e, AnsibleConnectionFailure): - msg = "ssh_retry: attempt: %d, ssh return code is 255. cmd (%s), pausing for %d seconds" % (attempt, cmd_summary, pause) + msg = "ssh_retry: attempt: %d, ssh return code is 255. cmd (%s), pausing for %d seconds" % (attempt + 1, cmd_summary, pause) else: - msg = "ssh_retry: attempt: %d, caught exception(%s) from cmd (%s), pausing for %d seconds" % (attempt, e, cmd_summary, pause) + msg = "ssh_retry: attempt: %d, caught exception(%s) from cmd (%s), pausing for %d seconds" % (attempt + 1, e, cmd_summary, pause) display.vv(msg, host=self.host) diff --git a/test/units/plugins/connection/test_ssh.py b/test/units/plugins/connection/test_ssh.py index 0077fd7d6f..e367fa366c 100644 --- a/test/units/plugins/connection/test_ssh.py +++ b/test/units/plugins/connection/test_ssh.py @@ -25,6 +25,7 @@ import pytest from ansible import constants as C +from ansible.errors import AnsibleAuthenticationFailure from ansible.compat.selectors import SelectorKey, EVENT_READ from units.compat import unittest from units.compat.mock import patch, MagicMock, PropertyMock @@ -501,6 +502,33 @@ class TestSSHConnectionRun(object): @pytest.mark.usefixtures('mock_run_env') class TestSSHConnectionRetries(object): + def test_incorrect_password(self, monkeypatch): + monkeypatch.setattr(C, 'HOST_KEY_CHECKING', False) + monkeypatch.setattr(C, 'ANSIBLE_SSH_RETRIES', 5) + monkeypatch.setattr('time.sleep', lambda x: None) + + self.mock_popen_res.stdout.read.side_effect = [b''] + self.mock_popen_res.stderr.read.side_effect = [b'Permission denied, please try again.\r\n'] + type(self.mock_popen_res).returncode = PropertyMock(side_effect=[5] * 4) + + self.mock_selector.select.side_effect = [ + [(SelectorKey(self.mock_popen_res.stdout, 1001, [EVENT_READ], None), EVENT_READ)], + [(SelectorKey(self.mock_popen_res.stderr, 1002, [EVENT_READ], None), EVENT_READ)], + [], + ] + + self.mock_selector.get_map.side_effect = lambda: True + + self.conn._build_command = MagicMock() + self.conn._build_command.return_value = [b'sshpass', b'-d41', b'ssh', b'-C'] + self.conn.get_option = MagicMock() + self.conn.get_option.return_value = True + + exception_info = pytest.raises(AnsibleAuthenticationFailure, self.conn.exec_command, 'sshpass', 'some data') + assert exception_info.value.message == ('Invalid/incorrect username/password. Skipping remaining 5 retries to prevent account lockout: ' + 'Permission denied, please try again.') + assert self.mock_popen.call_count == 1 + def test_retry_then_success(self, monkeypatch): monkeypatch.setattr(C, 'HOST_KEY_CHECKING', False) monkeypatch.setattr(C, 'ANSIBLE_SSH_RETRIES', 3) |