Repository: ansible/ansible
Pull request: Catch sshpass authentication errors and don't retry multiple times to prevent account lockout
##### SUMMARY

When using the `ssh` connection plugin with an invalid password, `sshpass` returns error code 5 for invalid/incorrect password. The `ssh` connection plugin currently only raises `AnsibleConnectionFailure` for error 255, which causes the host to not be marked as unreachable and the attempt loop in `task_executor.py` will try to connect to the host again. This results in three failed login attempts (by default), which can lock accounts in environments configured to automatically lock accounts.

This PR raises an exception on error code 5 and will make no further password login attempts regardless of how many retries are specified, preventing potential account lockout.

Fixes #48422 

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request


##### COMPONENT NAME
`lib/ansible/plugins/connection/ssh.py`

Problem statement:
add configurable password_retries to prevent account lockout
<!--- Verify first that your feature was not already discussed on GitHub -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
<!--- Describe the new feature/improvement briefly below -->
If a user mistypes a password for a user/pass ssh connection, it can cause account lockout due to ansible attempting to connect and supplying the incorrect password several times. 

##### ISSUE TYPE
- Feature Idea

##### COMPONENT NAME
<!--- Write the short name of the new feature, module, plugin or task below -->

##### ADDITIONAL INFORMATION
<!--- Describe how the feature would be used, why it is needed and what it would solve -->
This could be used to enforce the amount of times Ansible attempts to connect with the provided password via ssh. Similar to how ssh_retries is configurable in env or via ansible.cfg. Currently ssh_retries will cause account lockout at it does not consider a bad password to be an ssh failure.

<!--- Paste example playbooks or commands between quotes below -->
```yaml

```

<!--- HINT: You can also paste gist.github.com links for larger files -->

Hints from issue discussion:
`sshpass` returns error code 5 when invalid/incorrect password is entered. Currently we only check for 255 in the ssh connection plugin. We could handle error code 5 differently, and allow customization via a config setting.
Hi @gamuniz, thank you for submitting this issue!

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: thank_submitter --->
we might want to expand to handle all 'auth' failures (invalid key, user, etc) not just passwords

Commit message: Catch SSH authentication errors and don't retry multiple times to prevent account lock out

Signed-off-by: Sam Doran <sdoran@redhat.com>

Patch to review:
diff --git a/lib/ansible/plugins/connection/ssh.py b/lib/ansible/plugins/connection/ssh.py
index 813220d460e2d1..bf6cd0801d7800 100644
--- a/lib/ansible/plugins/connection/ssh.py
+++ b/lib/ansible/plugins/connection/ssh.py
@@ -151,8 +151,8 @@
             - 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
@@ -307,6 +307,57 @@ class AnsibleControlPersistBrokenPipeError(AnsibleError):
     pass
 
 
+class AnsiblePasswordAuthenticationFailure(AnsibleError):
+    pass
+
+
+def _handle_error(remaining_retries, command, return_tuple, no_log, host, display=display):
+
+    # sshpass errors
+    if command == b'sshpass':
+        if return_tuple[0] == 5:
+            msg = 'Invalid/incorrect 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 AnsiblePasswordAuthenticationFailure(msg)
+        # sshpass returns codes are 1-6. We handle 5 previously, so this catches other scenarios.
+        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()))
+            raise AnsibleConnectionFailure(msg)
+
+    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 return_tuple[0] in range(1, 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 +366,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 +385,7 @@ def wrapped(self, *args, **kwargs):
                 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 +401,18 @@ def wrapped(self, *args, **kwargs):
                     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 AnsiblePasswordAuthenticationFailure as e:
+                # Raise AnsibleConnectionFailer so it is captured and handled properly by TaskExecutor._execute()
+                raise AnsibleConnectionFailure(e)
+
             except (AnsibleConnectionFailure, Exception) as e:
+
                 if attempt == remaining_tries - 1:
                     raise
                 else:
@@ -375,9 +421,9 @@ def wrapped(self, *args, **kwargs):
                         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)
 


Code review comments:
diff_hunk: @@ -307,6 +307,57 @@ class AnsibleControlPersistBrokenPipeError(AnsibleError):
     pass
 
 
+class AnsiblePasswordAuthenticationFailure(AnsibleError):
+    pass
+
+
+def _handle_error(remaining_retries, command, return_tuple, no_log, host, display=display):
+
+    # sshpass errors
+    if command == b'sshpass':
+        if return_tuple[0] == 5:
+            msg = 'Invalid/incorrect 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 AnsiblePasswordAuthenticationFailure(msg)
+        # sshpass returns codes are 1-6. We handle 5 previously, so this catches other scenarios.
+        elif return_tuple[0] in [1, 2, 3, 4, 6]:
path: lib/ansible/plugins/connection/ssh.py
text: @user1:
minor bikeshed: might be more effective to do a range check? >= 1 and <= 6? since 5 has already been taken care of

@author:
The equivalent statement using comparsions seems a bit harder to read and understand:

```python
elif 1 <= return_tuple[0] < 5 or return_tuple[0] == 6:
    ...
```

Microperformance testing with `timeit` shows the comparison as slightly faster, but think the easier to read/maintain syntax makes it worth the very minor performance difference.

@user1:
`elif 1 <= return_tuple[0] <= 6:` the 5 was already handled, but yeah, as i said, not a blocker this is tiny possible perf gain.

@author:
Ok, I see. The `if` removes the need to exclude it from the comparison.

diff_hunk: @@ -307,6 +307,57 @@ class AnsibleControlPersistBrokenPipeError(AnsibleError):
     pass
 
 
+class AnsiblePasswordAuthenticationFailure(AnsibleError):
+    pass
+
+
+def _handle_error(remaining_retries, command, return_tuple, no_log, host, display=display):
+
+    # sshpass errors
+    if command == b'sshpass':
+        if return_tuple[0] == 5:
+            msg = 'Invalid/incorrect 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 AnsiblePasswordAuthenticationFailure(msg)
+        # sshpass returns codes are 1-6. We handle 5 previously, so this catches other scenarios.
+        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()))
+            raise AnsibleConnectionFailure(msg)
+
+    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 return_tuple[0] in range(1, 254):
path: lib/ansible/plugins/connection/ssh.py
text: @user1:
same here, unsure comparing to range vs comparing limits is more efficient

@author:
Using a comparison is much more efficient here. I'll change this.