-
Notifications
You must be signed in to change notification settings - Fork 344
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
Make SCP and StartShell behaves the same way as Device for SSH #649
base: master
Are you sure you want to change the base?
Conversation
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.
The `allow_agent` setting was used with NC but not with SCP. Store it in the device and reuse it for SCP. The private key was stored in a dictionnary, but this is not needed as Paramiko's `connect()` would default to `None` when not provided. Grab the SSH key filename from SSH configuration as Paramiko won't do it for us. For this reason, this commit is a followup to the one in Juniper#648.
This is a preparation to reuse the client for other purposes, notably for starting a shell.
Autobot: Would an admin like to run functional tests? |
Autobot: Would an admin like to run functional tests? |
ok to test. |
# we want to enable the ssh-agent if-and-only-if we are | ||
# not given a password or an ssh key file. | ||
# in this condition it means we want to query the agent | ||
# for available ssh keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we can also have ssh key file protected by passphrase. That passphrase in passed to Device as password.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you provide a password, the agent is disabled and Paramiko will fetch the key and decrypt it. The agent doesn't allow you to provide a password for an encrypted key either (it will ask the user for that if needed). Moreover, this code is already present as is. It's just moved here to be able to reuse its result for scp.
IMO, the ability to disable the agent part is odd. The regular "ssh" client doesn't have such an option. It adds complexity and I don't know why this has been implemented. We should just get rid of that.
Can one of the admins verify this patch? |
ok to test |
@stacywsmith Can you please review this pull request. |
Build finished. 551 tests run, 0 skipped, 0 failed. |
This is a followup to #648 (and this PR includes the commit from #648, so it shouldn't be merged before #648).
First commit makes small changes to SCP. This is mostly cosmetic with the exception of the agent and the private key. This is quite similar to what ncclient is doing with the same parameters that were provided in
Device
.The second commit moves the creation of the paramiko SSH client into a dedicated function.
The third commit reuses this dedicated function for
StartShell()
.