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

[WIP] Adds allow_agent option to Device() object #920

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

rsmekala
Copy link
Contributor

Problem

User currently cannot control the value of allow_agent being passed to ncclient.connect().

Analysis

PyEZ internally sets the value of allow_agent before passing it to ncclient. It is set to True only if PyEZ cannot gather the passwd or ssh_private_keyfile.

In case the user has not specified passwd or ssh_private_keyfile, PyEZ will use the ssh_config to populate the variables. In this case, since PyEZ is able to gather the credentials allow_agent is set to False.

The above logic works fine for a simpler SSH configuration, where only ssh_private_keyfile, port and user are mentioned int SSH configuration, any additional options mentioned in the file are ignored.

Solution

To add a new optional argument bool allow_agent. There are 3 cases:

  1. allow_agent is True: PyEZ will not load the values of ssh_private_key_file or passwd and allow_agent will be passed as True to ncclient.connect().

  2. allow_agent is Fale: PyEZ will load the values of above variables if it is able to gather and allow_agent will be passed as False to ncclient.connect().

  3. allow_agent is not provided: PyEZ will fallback to default behavior and set the value of allow_agent at runtime, based on whether it is able to gather the credentials or not.

Note: PyEZ will prefer the user-provided value of allow_agent over the runtime value

Related issues:
#648

@rsmekala rsmekala requested a review from vnitinv March 28, 2019 10:40
@jnpr-community-netdev
Copy link

Can one of the admins verify this patch?

@coveralls
Copy link

coveralls commented Mar 28, 2019

Coverage Status

Coverage increased (+0.3%) to 96.77% when pulling 0e5ee3b on rsmekala:allow_agent into 96a647b on Juniper:master.

@vincentbernat
Copy link
Contributor

This is a bit better than the current situation as we can workaround the inability to use an agent without modifying the code. However, this is still a bit weird to have to pass an additional option to make it
work. This is also inconvenient for a lib: I'll have to beg any dependent project to expose this option.

From all the conversations around this we had, no one was able to explain why we try to disable the agent functionality in the first place. The regular OpenSSH client (ssh) doesn't have such an option: a user cannot ask to not use the agent. So, unless someone explains why we try to do things differently, a simpler solution would be to just set self._allow_agent to True. Could you tell me what happens for you when you set allow_agent to True?

As a compromise, in #648, I am disabling the agent only if a private key is specifically provided. When the private key is fetched from ~/.ssh/config, keep the agent in case the key is encrypted. This doesn't make sense for me, but at least, I don't need to change anything to make my setup work (since I am specifying the private key in my ~/.ssh/config).

@rsmekala
Copy link
Contributor Author

rsmekala commented Apr 8, 2019

@stacywsmith Can you please provide your input on the issue ??

@stacywsmith
Copy link
Contributor

@rsmekala I'm really not sure why allow_agent is being set as it currently is. Maybe you can review #281 where it was introduced. If possible, I would recommend against adding yet another parameter to Device(). I'd prefer @vincentbernat suggestion to just leave allow_agent enabled all the time, but I'm not 100% sure that doesn't break anything. Again, the info in #281 might provide some clue about the reasons for the current behavior.

@vincentbernat
Copy link
Contributor

In #281, there is the mention of a popup when enabling the agent. I suppose this is a reference to OSX. Can someone on OSX test if there is a popup when enabling the agent?

@paravoid
Copy link

I agree with @vincentbernat here. To be honest, I also don't understand why py-junos-eznc has to have all of this complex logic around SSH options, including this, but also hardcoding a path to .ssh/config (under os.getenv('HOME')), as well as options like identityfile etc.

In the eznc -> ncclient -> paramiko chain, you'd think that the SSH client behavior would be something left to the SSH implementation (= paramiko), which would in turn parse an OpenSSH-compatible .ssh/config (or not), and not something the higher level libraries would have to each bother with.

The current code is very simplistic and broken with so many different ways -- anything but a very simple configuration will not work. allow_agent is only part of the problem. ProxyCommand (#153) is another one. Pretty sure ProxyJump wouldn't work either. etc.

@jnpr-community-netdev
Copy link

Can one of the admins verify this patch?

@rsmekala rsmekala changed the title Adds allow_agent option to Device() object [WIP] Adds allow_agent option to Device() object Sep 27, 2019
@jameskirsop
Copy link

Pretty sure ProxyJump wouldn't work either. etc.

Can confirm ProxyJump does not work, See #1073. I'm with @paravoid, I can't understand why this module can't leave SSH to a downstream library like paramiko.

@vnitinv vnitinv requested review from rahkumar651991 and removed request for vnitinv August 31, 2020 05:52
@ydnath ydnath requested review from dineshbaburam91 and removed request for rahkumar651991 May 11, 2022 12:18
@gaima8
Copy link

gaima8 commented Nov 24, 2023

#1284 is an updated version of this patch.

Curiously ProxyJump doesn't work with this PR combined with Juniper/ansible-junos-stdlib#634 but ProxyCommand does.
I've never used ProxyJump before though so it might not work for me anyway.

Host juniperdevice
    User bob
# Does work
    ProxyCommand ssh -l gaima8 jumphost -W %h:%p
# Doesn't have any effect
#   ProxyJump gaima8@jumphost

"bob" is my user on the juniperdevice but not my user locally.

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.

8 participants