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 support of manual aut_methods for SSH2 connection #173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion Exscript/protocols/protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,8 @@ def __init__(self,
verify_fingerprint=True,
account_factory=None,
banner_timeout=20,
encoding='latin-1'):
encoding='latin-1',
auth_methods=[]):
"""
Constructor.
The following events are provided:
Expand Down Expand Up @@ -252,6 +253,9 @@ def __init__(self,
:keyword banner_timeout: The time to wait for the banner.
:type encoding: str
:keyword encoding: The encoding of data received from the remote host.
:type auth_methods: list
:keyword auth_methods: The SSH authentication method to process (default to all supported

Choose a reason for hiding this comment

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

line too long (97 > 79 characters)

by the remote device)
"""
self.data_received_event = Event()
self.otp_requested_event = Event()
Expand Down Expand Up @@ -282,6 +286,8 @@ def __init__(self,
self.banner_timeout = banner_timeout
self.encoding = encoding
self.send_data = None
self.auth_methods = auth_methods
Copy link
Owner

@knipknap knipknap Aug 28, 2018

Choose a reason for hiding this comment

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

I would do a sanity check here, i.E.:

for method in auth_methods:
    if method not in auth_types:
        raise ValueError('unsupported auth_method: ' + repr(method))

Copy link
Contributor Author

@maximumG maximumG Aug 29, 2018

Choose a reason for hiding this comment

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

I thought about this check in the beginning. However the auth_type dict is in the ssh2.py module while this should be done in module protocol.py. If we import ssh2 in protocol we will end up with a circular import.

Another solution would be to have the auth_method attribute set directly inside ssh2 and not in protocol

Which one would you prefer to choose ?

Copy link
Owner

Choose a reason for hiding this comment

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

I would leave "self.auth_methods = auth_methods" in the protocol adapter, but only check the sanity in the SSH2 adapter. That would allow for code that can be interchanged between Telnet and SSH.


if stdout is None:
self.stdout = StringIO()
else:
Expand Down Expand Up @@ -611,6 +617,24 @@ def get_timeout(self):
"""
return self.timeout

def set_auth_methods(self, methods):
"""
Defines the SSH2 list of authentication methods allowed

:type methods: list
:param methods: A list of authentication methods (check Exscript.protocols.ssh2.auth_type)

Choose a reason for hiding this comment

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

line too long (98 > 79 characters)

"""
self.auth_methods = methods

def get_auth_methods(self):
"""
Returns the current SSH2 authentication methods allowed.

:rtype: list
:return: A list of authentication SSH2 methods allowed.
"""
return self.auth_methods

def _connect_hook(self, host, port):
"""
Should be overwritten.
Expand Down
2 changes: 2 additions & 0 deletions Exscript/protocols/ssh2.py
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,8 @@ def _paramiko_auth_autokey(self, username, password):

def _get_auth_methods(self, allowed_types):
auth_methods = []
if self.auth_methods:
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't it make more sense to do something like

auth_method_handlers = []
if self.auth_methods:
    auth_methods = [m for m in self.auth_methods if m in allowed_types]
else:
    auth_methods = allowed_types
for method in auth_methods:
    for type_name in auth_types[method]:
        auth_method_handlers.append(getattr(self, type_name))
return auth_method_handlers

Otherwise Exscript wouldn try to authenticate using unsupported methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are absolutely right. Thanks for the review - I will change the code.

allowed_types = self.auth_methods
for method in allowed_types:
for type_name in auth_types[method]:
auth_methods.append(getattr(self, type_name))
Expand Down
14 changes: 14 additions & 0 deletions tests/Exscript/protocols/ProtocolTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,20 @@ def testSetDriver(self):
def testGetDriver(self):
pass # Already tested in testSetDriver()

def testSetAuthMethods(self):
self.assertListEqual(self.protocol.get_auth_methods(), [])

self.protocol.set_auth_methods([])
self.assertTrue(self.protocol.get_auth_methods() is not None)
self.assertListEqual(self.protocol.get_auth_methods(), [])

self.protocol.set_auth_methods(['password', 'publickey'])
self.assertTrue(self.protocol.get_auth_methods() is not None)
self.assertListEqual(self.protocol.get_auth_methods(), ['password', 'publickey'])

Choose a reason for hiding this comment

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

line too long (89 > 79 characters)


def testGetAuthMethods(self):
pass

def testGetBanner(self):
self.assertEqual(self.protocol.get_banner(), None)
if self.protocol.__class__ == Protocol:
Expand Down