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

Add support of manual aut_methods for SSH2 connection #173

wants to merge 1 commit into from

Conversation

maximumG
Copy link
Contributor

@maximumG maximumG commented Aug 7, 2018

Regarding discussion in issue #172


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)

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)

@@ -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)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 76.576% when pulling f542b1e on maximumG:auth_method_as_param into cc0b63b on knipknap:master.

@maximumG
Copy link
Contributor Author

Anyone would be able to check this pull request, please ?

@@ -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.

@@ -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.

@knipknap knipknap mentioned this pull request Aug 28, 2018
@maximumG
Copy link
Contributor Author

Sorry for the long delay for this pull request. I will try to work on this ASAP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants