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

Honoring openssh Includes #2215

Open
jonocodes opened this issue Jul 12, 2022 · 6 comments · May be fixed by paramiko/paramiko#2307
Open

Honoring openssh Includes #2215

jonocodes opened this issue Jul 12, 2022 · 6 comments · May be fixed by paramiko/paramiko#2307

Comments

@jonocodes
Copy link

Describe the bug

Is there a way to honor open ssh Include statements somehow? I understand 'Include' is not a supported keyword in paramiko,
but perhaps there are workarounds for when it is in use. If I connect to the host in openssh, can that existing connection be used, instead of issuing a new one?

To Reproduce
Steps to reproduce the behaviour (please attach a minimal example):

  1. create .ssh/config that uses an Include statement that defines 'fabhost1'
  2. ssh fabhost1 - works fine, expected
  3. try to issue a command via fabric to fabhost1
  4. fabric does not know how to find that host

Example config
~/.ssh/config

Include ~/.ssh/fabhosts_config

~/.ssh/fabhosts_config

Host fabhost1
    Hostname a.b.c.d

Expected behaviour
I would like to be able to connect to the fabhost1.

Environment
Linux / OpenSSH

@anarcat
Copy link

anarcat commented Apr 4, 2023

for the record, someone figured out a hack around this problem in paramiko/paramiko#1609 (comment)

if i read it correctly, they hardcode a ssh config path in their fabfile.py by hijacking get_config...

There's also an actual PR in paramiko to add support for the Include parameter directly, in paramiko/paramiko#1892

So if people could test that out or review the code, it would probably help fixing that issue! :)

@mrmeszaros
Copy link

mrmeszaros commented Nov 23, 2023

Currently the last commit on paramiko/paramiko#2307 breaks fabric, as fabric relies on the paramiko.SSHConfig._config variable.

Accessing fields that start with an underscore (like the aforementioned _config) is highly discouraged, as those are usually part of the implementation and prone to changes, like here.

Therefore, first, the reason for the usage (of paramiko.SSHConfig._config in the code) should be discovered, and a better way of interacting should be chosen.

@kucharskim
Copy link

kucharskim commented Nov 23, 2023

Therefore, first, the reason for the usage should be discovered, and a better way of interacting should be chosen.

I am using includes in my ~/.ssh/config and hence, some machines are not reachable by fabric, because of specific ssh settings needed to reach them, for example specific User, or ProxyCommand, or HostName, or ProxyJump, etc.

@mrmeszaros
Copy link

@kucharskim - sorry, I wasn't clear: I meant that the fabric code uses parts of paramiko that are prone to changes.

@davidjmemmett
Copy link
Contributor

davidjmemmett commented Nov 24, 2023 via email

@mrmeszaros
Copy link

@davidjmemmett Marginally, but Yes!

I have an open pull request into paramiko, that implements the parsing of Include statements. However, it changes the internal representation:
Instead of the SSHConfig._config field, it would use the new field SSHConfig._config_by_file and SSHConfig._config_root (You can check it here: paramiko/paramiko#2307)

This would cause a problem in fabric, as currently fabric's implementation relies on that specific field - for example here, on line 231:

fabric/fabric/config.py

Lines 221 to 232 in 3342252

def _clone_init_kwargs(self, *args, **kw):
# Parent kwargs
kwargs = super()._clone_init_kwargs(*args, **kw)
# Transmit our internal SSHConfig via explicit-obj kwarg, thus
# bypassing any file loading. (Our extension of clone() above copies
# over other attributes as well so that the end result looks consistent
# with reality.)
new_config = SSHConfig()
# TODO: as with other spots, this implies SSHConfig needs a cleaner
# public API re: creating and updating its core data.
new_config._config = copy.deepcopy(self.base_ssh_config._config)
return dict(kwargs, ssh_config=new_config)

This one would be easy to fix - one can simply deepcopy the whole SSHConfig, however, there are more places to check and fix.

This is what I tried to refer to when I said

Therefore, first, the reason for the usage (of paramiko.SSHConfig._config in the code) should be discovered, and a better way of interacting should be chosen.

Sorry for the confusion - I hope this clears it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants