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

Add support for ssh config include #2307

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

mrmeszaros
Copy link

Fixes #1892
Fixes #1609

Should also fix fabric/fabric#2215 once released

Some solutions felt a bit dirty to me, so feel free to give feedback.

@PeterJCLaw
Copy link

(Speaking as someone who's been following this feature for a while & still interested; prompted by the closure of #1892 which this presumably replaces)

Thanks for putting this together @mrmeszaros :)

Do you feel this PR is ready for review/testing etc. (absolutely no worries if not!) -- wondering if I could help by offering a review and some testing perhaps? (I realise there's a merge conflict, though it appears trivial to resolve)

@mrmeszaros
Copy link
Author

@PeterJCLaw I am 90% confident this is ready, so feel free to review it.

I tried adding some unit tests, but it would be nice if You could try and break it, to see where are the weak points.

10% in me says, that the internal representation could be improved, but it would need some more think juice. If You have ideas, please feel free to share, I am open for discussions.

PS.: I used ssh -G $TEST_HOST for a reference how OpenSSH compiles the config files.
PPS.: I will (hopefully) resolve the conflict today and push it up here. (lol)

Based on the ssh_config man pages relative includes should use
the default ssh config home (~/.ssh) as a base
Just as openssh, it will currently silently ignore missing files.
This solution is kind of hacky, as it breaks up the context blocks into
parts based on the `Include`, so that they are used in the order of
definition and not at the end of the block.

A proper solution would be to use an ordered collection of options,
during parsing, that ensures ordered lookup during the lookup phase.

OrderedDict was tried as a drop-in, but it is not sufficient enough,
as defined in the include-order-2 test case.
The storage of the parsed configs might need to be reviewed,
as the include support introduced complexity.

Due to how includes work, the order of the include is also relevant
during lookup time.

One possible solution would be to flatten the parsed config:
to compile the host/match conditions through includes during parsing.
This way the lookup could be much simpler.
@csueiras
Copy link

I installed this branch in my local repository and when calling get_hostnames() in the config object, I got the error:

.venv/lib/python3.9/site-packages/paramiko/config.py", line 430, in get_hostnames
    hosts.update(entry["host"])
KeyError: 'host'

Which I can see was added as part of this PR here:

https://github.com/mrmeszaros/paramiko/blame/39d82a15da01885c4688dddee2d1ea18ec5b0a05/paramiko/config.py#L430

My failing config has a Match host <myhost> exec ... (I installed Invoke per the documentation for support for exec).

@mrmeszaros
Copy link
Author

mrmeszaros commented Apr 2, 2024

@csueiras I will look into it - thanks for trying this out <3

Could You please send a failing config?
Or maybe give some hints about the ordering of the directives / stanzas etc. - to help me reproduce the issue?

@mrmeszaros
Copy link
Author

mrmeszaros commented Apr 11, 2024

@csueiras I have done some investigating! I have some mixed news.

Firstly:

It seems to me, that this was not introduced by my code (this is the good/bad news part).
I have tested Your usecase on the main branch, and it throws the same KeyError.
The get_hostnames() method is poorly tested - it only occurs in one test-case (that uses the robey test config file):

https://github.com/paramiko/paramiko/blob/51eb55debf2ebfe56f38378005439a029a48225f/tests/test_config.py#L370C1-L372C1

I am not sure how it should work, and how it is currently used. I did not find a lot of documentation regarding how it should handle host directives inside a match.

Secondly

I think I can provide a hot-fix (this is the good news part).
However, I am unsure of it's purpose, so for that I would need more info.
So, feel free to give some help.

Up Next

I think we should find/create a separate Issue / PR and ping the maintainer.
Hopefully that one - being a hot-fix - should be merged more quickly.

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.

Honoring openssh Includes Do you have any intention to ever support ssh_config Include files?
3 participants