Skip to content

Add support for ssh config include#2307

Open
mrmeszaros wants to merge 17 commits intoparamiko:mainfrom
mrmeszaros:config-includes
Open

Add support for ssh config include#2307
mrmeszaros wants to merge 17 commits intoparamiko:mainfrom
mrmeszaros:config-includes

Conversation

@mrmeszaros
Copy link
Copy Markdown

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
Copy Markdown

(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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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.

@BonnetAdam
Copy link
Copy Markdown

Hi, can this be merged, i really need this and still want to use stable version of paramiko... Thanks...

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?

4 participants