search

carlmontanari / scrapli

Fast, flexible, sync/async, Python 3.7+ screen scraping client specifically for network devices
https://carlmontanari.github.io/scrapli/
MIT License
575 stars 59 forks source link

Using system transport with ssh_config_file = True does not merge user and system config files #336

Closed bennnnnnnn closed 3 months ago

bennnnnnnn commented 3 months ago

Describe the bug Currently setting ssh_config_file to True and using the system transport, the logic in _resolve_ssh_config from the BaseDriver still collapses this to a single file (first the user config file, then the system config file) to pass using -F to the actual ssh client.

This means that if a user has a user config file, any config existing in the system config file will not get picked up.

To Reproduce

  1. Set some options in /etc/ssh/ssh_conf and some options in ~/.ssh/config
  2. Run scrapli and connect to a host
  3. Notice that the system ssh conf is not picked up

Expected behavior I understand this abstraction exists because of the different ssh clients which don't have native ssh config support, but I think it would be better to omit the -F if ssh_config_file is True. Optionally ssh_config_files could also become a list and we could then pass both user and system config to the ssh client. (some changes probably needed to the config parsing code for the other ssh clients then). Maybe there could also be a separate flag to read the system config files, separate from the ssh_config_file argument?

carlmontanari commented 3 months ago

hey @bennnnnnnn with so many n's!

hmm, yeah this is a good one. I dislike the idea of changing the ssh_config_file arg since that would be obviously be a big change to the public side of things. I very much like the idea of just dropping the -F bit if ssh_config_file is just True though, that seems like a much better idea. If it's still explicit we can still explicitly set it of course.

implementing this in my old spaghetti mess may be a bit annoying since the transport just gets an empty string or a path. since we already pass the transport to the _setup_ssh_file_args method though I guess we could just have another check for "if transport is system and ssh_config_file is true, then return some special placeholder" then the transport could just check if the ssh_config_file is like "yes" (obviously not that but you get the idea) then we know to not put any -F arg in...

thoughts? ima have to sleep on it probably but if you think that sounds sane would you be down for a pr?

bennnnnnnn commented 3 months ago

Hi Carl,

Fully agree with your points above! Also don't be so harsh on your older code :-)

I'll send in a PR.

PS: This is a fantastic package! Amazing you built this by yourself, keep up the great work!

carlmontanari commented 3 months ago

thanks @bennnnnnnn ! will close this and we can follow up in the pr, thanks for raising this and the work on it!!