Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add timeout to authentication step #869

Merged
merged 7 commits into from Jun 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions paramiko/auth_handler.py
Expand Up @@ -21,6 +21,7 @@
"""

import weakref
import time
from paramiko.common import cMSG_SERVICE_REQUEST, cMSG_DISCONNECT, \
DISCONNECT_SERVICE_NOT_AVAILABLE, DISCONNECT_NO_MORE_AUTH_METHODS_AVAILABLE, \
cMSG_USERAUTH_REQUEST, cMSG_SERVICE_ACCEPT, DEBUG, AUTH_SUCCESSFUL, INFO, \
Expand Down Expand Up @@ -188,6 +189,7 @@ def _get_session_blob(self, key, service, username):
return m.asbytes()

def wait_for_response(self, event):
max_ts = time.time() + self.transport.auth_timeout if self.transport.auth_timeout is not None else None
while True:
event.wait(0.1)
if not self.transport.is_active():
Expand All @@ -197,6 +199,9 @@ def wait_for_response(self, event):
raise e
if event.is_set():
break
if max_ts is not None and max_ts <= time.time():
raise AuthenticationException('Authentication timeout.')

if not self.is_authenticated():
e = self.transport.get_exception()
if e is None:
Expand Down
8 changes: 6 additions & 2 deletions paramiko/client.py
Expand Up @@ -226,7 +226,8 @@ def connect(
gss_kex=False,
gss_deleg_creds=True,
gss_host=None,
banner_timeout=None
banner_timeout=None,
auth_timeout=None
):
"""
Connect to an SSH server and authenticate to it. The server's host key
Expand Down Expand Up @@ -278,7 +279,8 @@ def connect(
The targets name in the kerberos database. default: hostname
:param float banner_timeout: an optional timeout (in seconds) to wait
for the SSH banner to be presented.

:param float auth_timeout: an optional timeout (in seconds) to wait for
an authentication response.
:raises BadHostKeyException: if the server's host key could not be
verified
:raises AuthenticationException: if authentication failed
Expand Down Expand Up @@ -335,6 +337,8 @@ def connect(
t.set_log_channel(self._log_channel)
if banner_timeout is not None:
t.banner_timeout = banner_timeout
if auth_timeout is not None:
t.auth_timeout = auth_timeout
t.start_client(timeout=timeout)
ResourceManager.register(self, t)

Expand Down
2 changes: 1 addition & 1 deletion paramiko/transport.py
Expand Up @@ -383,7 +383,7 @@ def __init__(self,
self.completion_event = None # user-defined event callbacks
self.banner_timeout = 15 # how long (seconds) to wait for the SSH banner
self.handshake_timeout = 15 # how long (seconds) to wait for the handshake to finish after SSH banner sent.

self.auth_timeout = 30 # how long (seconds) to wait for the auth response.

# server mode:
self.server_mode = False
Expand Down
3 changes: 3 additions & 0 deletions sites/www/changelog.rst
Expand Up @@ -2,6 +2,9 @@
Changelog
=========

* :feature:`add-auth-timeout` Adds a timeout for the authentication process.
This is a fix to prevent the client getting stuck if an SSH server becomes
un-responsive during the authentication. Credit to ``@timsavage``.
* :support:`866 backported` (also :issue:`838`) Remove an old test-related file
we don't support, and add PyPy to Travis-CI config. Thanks to Pierce Lopez
for the final patch and Pedro Rodrigues for an earlier edition.
Expand Down
25 changes: 25 additions & 0 deletions tests/test_auth.py
Expand Up @@ -23,6 +23,7 @@
import sys
import threading
import unittest
from time import sleep

from paramiko import Transport, ServerInterface, RSAKey, DSSKey, \
BadAuthenticationType, InteractiveQuery, \
Expand Down Expand Up @@ -73,6 +74,9 @@ def check_auth_password(self, username, password):
return AUTH_SUCCESSFUL
if username == 'bad-server':
raise Exception("Ack!")
if username == 'unresponsive-server':
sleep(5)
return AUTH_SUCCESSFUL
return AUTH_FAILED

def check_auth_publickey(self, username, key):
Expand Down Expand Up @@ -232,3 +236,24 @@ def test_8_auth_gets_disconnected(self):
except:
etype, evalue, etb = sys.exc_info()
self.assertTrue(issubclass(etype, AuthenticationException))

def test_9_auth_non_responsive(self):
"""
verify that authentication times out if server takes to long to
respond (or never responds).
"""
auth_timeout = self.tc.auth_timeout
self.tc.auth_timeout = 2 # Reduce to 2 seconds to speed up test

try:
self.start_server()
self.tc.connect()
try:
remain = self.tc.auth_password('unresponsive-server', 'hello')
except:
etype, evalue, etb = sys.exc_info()
self.assertTrue(issubclass(etype, AuthenticationException))
self.assertTrue('Authentication timeout' in str(evalue))
finally:
# Restore value
self.tc.auth_timeout = auth_timeout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setUp() is run for every test method, so it's a new client for every test, so this shouldn't be needed

21 changes: 21 additions & 0 deletions tests/test_client.py
Expand Up @@ -60,6 +60,9 @@ def get_allowed_auths(self, username):
def check_auth_password(self, username, password):
if (username == 'slowdive') and (password == 'pygmalion'):
return paramiko.AUTH_SUCCESSFUL
if (username == 'slowdive') and (password == 'unresponsive-server'):
time.sleep(5)
return paramiko.AUTH_SUCCESSFUL
return paramiko.AUTH_FAILED

def check_auth_publickey(self, username, key):
Expand Down Expand Up @@ -380,6 +383,24 @@ def test_8_auth_trickledown(self):
)
self._test_connection(**kwargs)

def test_9_auth_timeout(self):
"""
verify that the SSHClient has a configurable auth timeout
"""
threading.Thread(target=self._run).start()
host_key = paramiko.RSAKey.from_private_key_file(test_path('test_rsa.key'))
public_host_key = paramiko.RSAKey(data=host_key.asbytes())

self.tc = paramiko.SSHClient()
self.tc.get_host_keys().add('[%s]:%d' % (self.addr, self.port), 'ssh-rsa', public_host_key)
# Connect with a half second auth timeout
kwargs = dict(self.connect_kwargs, password='unresponsive-server', auth_timeout=0.5)
self.assertRaises(
paramiko.AuthenticationException,
self.tc.connect,
**kwargs
)

def test_update_environment(self):
"""
Verify that environment variables can be set by the client.
Expand Down