Do not provide the key if it comes from the SSH configuration #648

vincentbernat · 2017-01-13T19:17:20Z

This is a followup of #628 where this change was initially pushed but
reverted because I didn't remember why I did it. Before #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 #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.

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.

coveralls · 2017-01-13T19:20:22Z

Coverage decreased (-0.0006%) to 98.975% when pulling 51a68d1 on vincentbernat:fix/private-ssh-key into 999c883 on Juniper:master.

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.

vincentbernat · 2017-01-13T19:49:41Z

An alternative solution would be to enable the user to say if they want to use the agent or not. allow_agent=None in the function signature. if allow_agent is None, use the current heuristic to decide its value, otherwise leave it as is.

IMO, we should always use the agent: ssh doesn't offer the option to not query the agent but I suppose people put that for some reason.

jnpr-community-netdev · 2017-01-14T18:55:02Z

Autobot: Would an admin like to run functional tests?

vnitinv · 2017-01-18T07:10:45Z

Autobot: Would an admin like to run functional tests?

vnitinv · 2017-01-18T07:15:27Z

ok to test.

vnitinv · 2017-02-03T10:00:26Z

lib/jnpr/junos/device.py

@@ -218,7 +218,6 @@ def _sshconf_lkup(self):
                self._hostname = found.get('hostname', self._hostname)
                self._port = found.get('port', self._port)
                self._conf_auth_user = found.get('user')
-                self._conf_ssh_private_key_file = found.get('identityfile')


Not sure why is this lined removed.
So if I have entered my private ssh file as identityfile in my ssh cofig. witout this line those private key file will be ignored and its like everytime user need to pass with ssh_private_key_file parameter.

In case of multiple device usage with different private file for different device. identityfile is of much use.

The information is still present in the SSH configuration that is provided to ncclient. So, your use case still work.

The line is removed because when _conf_ssh_private_key_file is defined, it disables the SSH agent (which is quite surprising as no other SSH client does that). So, if the key is stored in the agent, that doesn't work.

vnitinv · 2017-03-24T10:30:19Z

Can one of the admins verify this patch?

vincentbernat · 2017-11-12T18:40:40Z

Is it something to do for this PR to be merged? People using an agent to store their SSH keys cannot use junos-eznc without this patch as agent is bypassed when providing a key directly to junos-eznc or through ~/.ssh/config.

vnitinv · 2017-11-13T04:02:29Z

ok to test

jnpr-community-netdev · 2017-11-13T04:04:52Z

Build triggered. sha1 is merged.

jnpr-community-netdev · 2017-11-13T04:05:03Z

Build started sha1 is merged.

jnpr-community-netdev · 2017-11-13T04:09:02Z

Build finished. 730 tests run, 0 skipped, 0 failed.

jnpr-community-netdev · 2017-11-13T04:36:29Z

Build finished.

vnitinv · 2017-11-15T06:27:59Z

Sorry for the delay @vincentbernat I need to test this change before I can merge. I will do it next week.

paravoid · 2018-11-27T02:29:11Z

Ping? This has been biting me for the past three years :)

vnitinv · 2018-11-27T09:26:11Z

Hi @paravoid, Can you explain the scenario of your test. In my test environment, I dont see any issue.

allow_agent is set to False when there is any explicit value passed by user for passwd/ssh private key. If both are None allow_agent is set to True and hence its upto ncclient/paramiko to make use of ssh config file/identity to make the connection to the device.

paravoid · 2018-11-27T12:24:26Z

Sure! Selecting a SSH key file is orthogonal to using an agent (an agent may have multiple keys installed, and a user may want to pick different ones for different hosts).

In my case, I have an .ssh/config that looks a little bit like this:

Host router.example.org
        User paravoid
        IdentityFile ~/.ssh/id_rsa.network

…and this works as intended with OpenSSH. I actually have a much more complicated config with statements such as UserKnownHostsFile, IdentitiesOnly, ProxyJump etc., but this is just a simplified PoC of the bug in question.

The code right now does this:

allow_agent = bool((self._auth_password is None) and
                   (self._ssh_private_key_file is None))

…with self._conf_ssh_private_key_file = found.get('identityfile'), hence why this is broken.

It really feels odd to me that py-junos-eznc has logic around SSH config options and their combinations. It doesn't feel like like an SSH configuration logic belongs in a Juniper-specific module at all. More broadly, it looks like in the NAPALM->py-junos-eznc->ncclient->paramiko chain… there is some logic around SSH private key files, passwords and/or agent use in each of those modules :( This makes things very complicated to handle corner cases, debug or add new features for (e.g. ProxyJump)…

vincentbernat · 2019-03-14T09:10:05Z

This bug is still biting me 2 years later. Again, the root cause of all this pain is the ability to disable querying local agent. This is not an option the regular ssh client allows. An easy fix would be to always set to True and see who complains.

vnitinv · 2019-03-14T10:28:19Z

Sorry @vincentbernat for late response, we will update you about this by EOD today with the code changes we plan. You can go through the plan. If all good, will take it forward.

rsmekala · 2019-03-14T10:34:33Z

@vincentbernat We will be going with allow_agent logic. A new option allow_agent would be added to the Device function.

A more detailed view of the changes.

Option bool allow_agent
Default False

When set to True, Pyez will forward the ssh_config file to the paramiko agent instead of parsing it.
When set to False, Pyez will fallback to default behavior i.e Pyez will parse the ssh_config and the file will not be forwarded to paramiko agent.

Any comments/inputs on the logic ??

rsmekala · 2019-04-01T09:40:03Z

@vincentbernat @paravoid The functionality will be merged via #920. Can you please review the changes, and let us know if you have any additional requirements ??

gaima8 · 2023-11-24T22:58:07Z

#1284 is another go at resolving this.
In my case I have an IdentityFile defined at the top level of my ~/.ssh/config but for the Juniper devices I must use a yubikey so have no private key on disk to use for them.

With allow_agent defined it simply won't look for an IdentityFile and pass allow_agent=True to ncclient.

The current logic around the SSH agent is flawed and tentatives to fix it to cover more cases only leads to more complexity. As seen in Juniper#648, to this day, nobody really knows why we disable the agent. In the meantime, many valid use cases just don't work because of disabling the agent, including the good practice to use encrypted private key files. I think it is time to bite the bullet and just leave the agent always on: "ssh" from OpenSSH does not have an option to disable the agent and nobody complains about this.

vincentbernat · 2023-11-25T04:08:28Z

I wanted to get consensus here before opening yet another pull request, but after years, still no consensus. Nobody was able to explain why we try to disable the agent, something that is not possible with "ssh" from OpenSSH. I think the best way forward is to mimic this and always leave the agent enabled. Nobody ever demonstrated this would break their workflow. Adding more options and workarounds will just makes the code more complex and brittle. I have opened #1285 to remove the logic around disabling the agent and it should be favored over of the current PR.

This was referenced Jan 13, 2017

Make SCP and StartShell behaves the same way as Device for SSH #649

Open

Fixed ssh-key auth #281

Merged

vnitinv reviewed Feb 3, 2017

View reviewed changes

vnitinv assigned rsmekala Nov 30, 2018

rsmekala mentioned this pull request Mar 28, 2019

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

Open

rsmekala mentioned this pull request Jan 16, 2020

juniper_junos_config does not work with bastion configuration Juniper/ansible-junos-stdlib#454

Open

jameskirsop mentioned this pull request Aug 31, 2020

Add support for SSH's ProxyJump command #1073

Open

ydnath requested a review from dineshbaburam91 May 11, 2022 12:19

vincentbernat mentioned this pull request Nov 25, 2023

Do not disable SSH agent #1285

Open

vincentbernat closed this Nov 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not provide the key if it comes from the SSH configuration #648

Do not provide the key if it comes from the SSH configuration #648

vincentbernat commented Jan 13, 2017

coveralls commented Jan 13, 2017 •

edited

Loading

vincentbernat commented Jan 13, 2017

jnpr-community-netdev commented Jan 14, 2017

vnitinv commented Jan 18, 2017

vnitinv commented Jan 18, 2017

vnitinv Feb 3, 2017 •

edited

Loading

vincentbernat Feb 3, 2017

vnitinv commented Mar 24, 2017

vincentbernat commented Nov 12, 2017

vnitinv commented Nov 13, 2017

jnpr-community-netdev commented Nov 13, 2017

jnpr-community-netdev commented Nov 13, 2017

jnpr-community-netdev commented Nov 13, 2017

jnpr-community-netdev commented Nov 13, 2017

vnitinv commented Nov 15, 2017

paravoid commented Nov 27, 2018

vnitinv commented Nov 27, 2018

paravoid commented Nov 27, 2018

vincentbernat commented Mar 14, 2019

vnitinv commented Mar 14, 2019 •

edited

Loading

rsmekala commented Mar 14, 2019 •

edited

Loading

rsmekala commented Apr 1, 2019

gaima8 commented Nov 24, 2023

vincentbernat commented Nov 25, 2023

Do not provide the key if it comes from the SSH configuration #648

Do not provide the key if it comes from the SSH configuration #648

Conversation

vincentbernat commented Jan 13, 2017

coveralls commented Jan 13, 2017 • edited Loading

vincentbernat commented Jan 13, 2017

jnpr-community-netdev commented Jan 14, 2017

vnitinv commented Jan 18, 2017

vnitinv commented Jan 18, 2017

vnitinv Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

vincentbernat Feb 3, 2017

Choose a reason for hiding this comment

vnitinv commented Mar 24, 2017

vincentbernat commented Nov 12, 2017

vnitinv commented Nov 13, 2017

jnpr-community-netdev commented Nov 13, 2017

jnpr-community-netdev commented Nov 13, 2017

jnpr-community-netdev commented Nov 13, 2017

jnpr-community-netdev commented Nov 13, 2017

vnitinv commented Nov 15, 2017

paravoid commented Nov 27, 2018

vnitinv commented Nov 27, 2018

paravoid commented Nov 27, 2018

vincentbernat commented Mar 14, 2019

vnitinv commented Mar 14, 2019 • edited Loading

rsmekala commented Mar 14, 2019 • edited Loading

rsmekala commented Apr 1, 2019

gaima8 commented Nov 24, 2023

vincentbernat commented Nov 25, 2023

coveralls commented Jan 13, 2017 •

edited

Loading

vnitinv Feb 3, 2017 •

edited

Loading

vnitinv commented Mar 14, 2019 •

edited

Loading

rsmekala commented Mar 14, 2019 •

edited

Loading