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

Lookup user from ~/.ssh/config #628

Merged
merged 1 commit into from
Dec 5, 2016

Conversation

vincentbernat
Copy link
Contributor

Due to a change in commit b03e611, the user wasn't extracted from
user (or specified) SSH configuration. Add back the call to parse SSH
configuration.

Also, don't use the parsed private keyfile as it will be done
automatically by the underlying Paramiko. Using the parsed key doesn't
work, either because this is a list and not a string or because it is
encrypted and I am using an agent.

@coveralls
Copy link

coveralls commented Nov 21, 2016

Coverage Status

Coverage increased (+0.2%) to 97.037% when pulling e1187aa on vincentbernat:fix/user-from-ssh-config into b84b8a6 on Juniper:master.

@ydnath
Copy link
Member

ydnath commented Nov 21, 2016

@stacywsmith and @vnitinv to review

@@ -833,11 +833,11 @@ def __init__(self, *vargs, **kvargs):
self._conf_ssh_private_key_file = None
# user can get updated by ssh_config
self._ssh_config = kvargs.get('ssh_config')
self._sshconf_lkup()
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree adding this line which will override self._conf_ssh_private_key_file None value set in line 833 if that is configured in ssh config file.

# but if user or private key is explicit from call, then use it.
self._auth_user = kvargs.get('user') or self._conf_auth_user or \
self._auth_user
self._ssh_private_key_file = kvargs.get('ssh_private_key_file') \
or self._conf_ssh_private_key_file
self._ssh_private_key_file = kvargs.get('ssh_private_key_file')
Copy link
Contributor

Choose a reason for hiding this comment

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

why "or self._conf_ssh_private_key_file" was removed. If user has configured private key file in ssh config file, by removing the code the value will be set to None only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. I don't know why I removed it. I have amended the commit.

Copy link
Contributor

@vnitinv vnitinv left a comment

Choose a reason for hiding this comment

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

find my comments inline to code changes.

Due to a change in commit b03e611, the user wasn't extracted from
user (or specified) SSH configuration. Add back the call to parse SSH
configuration.

Also, don't use the parsed private keyfile as it will be done
automatically by the underlying Paramiko. Using the parsed key doesn't
work, either because this is a list and not a string or because it is
encrypted and I am using an agent.
@coveralls
Copy link

coveralls commented Dec 5, 2016

Coverage Status

Coverage increased (+0.001%) to 96.814% when pulling 9d91b76 on vincentbernat:fix/user-from-ssh-config into b84b8a6 on Juniper:master.

@stacywsmith stacywsmith merged commit 03051ab into Juniper:master Dec 5, 2016
vincentbernat added a commit to vincentbernat/py-junos-eznc that referenced this pull request Jan 13, 2017
This is a followup of Juniper#628 where this change was initially pushed but
reverted because I didn't remember why I did it. Before Juniper#628, when an
identity is provided in the SSH configuration, it was not copied in
`_conf_ssh_private_key_file` due to a bug. After fixing the bug in Juniper#628,
the key is now copied.

However, the SSH configuration is provided to the `connect()` method
which will use it if needed. Therefore, this is not needed. Moreover, if
the key is provided by an agent and/or encrypted, this won't work as,
later in the code, `allow_agent` will be set to `False` due to the
presence of a private key.
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.

5 participants