Added support for parsing 'Include' directive in ssh config file. by FDT2k · Pull Request #872 · paramiko/paramiko

FDT2k · 2017-01-08T20:25:37Z

Just added support for parsing the Include directive in ssh config file.

coveralls · 2017-01-08T20:39:37Z

Coverage decreased (-0.2%) to 74.013% when pulling 9d183fe on FDT2k:master into d6d54df on paramiko:master.

coveralls · 2017-01-08T20:39:37Z

Coverage decreased (-0.2%) to 74.013% when pulling 9d183fe on FDT2k:master into d6d54df on paramiko:master.

coveralls · 2017-01-08T20:39:37Z

Coverage decreased (-0.2%) to 74.013% when pulling 9d183fe on FDT2k:master into d6d54df on paramiko:master.

coveralls · 2017-01-08T20:39:37Z

Coverage decreased (-0.2%) to 74.013% when pulling 9d183fe on FDT2k:master into d6d54df on paramiko:master.

ploxiln · 2017-01-08T22:50:31Z

paramiko/config.py

        self._config = []

-    def parse(self, file_obj):
+    def parse(self, file_obj,parsed_files=None):


style nit: space after comma

ploxiln · 2017-01-08T22:50:51Z

paramiko/config.py

+
+                path = value
+                # according to SSH documentation if the path is relative, we look forward in ~/.ssh
+                if path[0] != '/' and path[0]!='~':


style nit: spaces around !=

you may want to use os.path.isabs() for the first part of this condition, to perhaps enable better compatibility with windows

ploxiln · 2017-01-08T22:58:23Z

paramiko/config.py

+                            raise Exception("Include loop detected in ssh config file: %s" %path)
+                        with open(filename) as fd:
+                            parsed_files.append(path);
+                            self.parse(fd,parsed_files)


more minor style nits - space after comma, no semicolon at end of the previous line, spaces around % three lines up

ploxiln · 2017-01-08T23:01:39Z

Overall looks good to me.

coveralls · 2017-01-09T06:37:53Z

Coverage decreased (-0.1%) to 74.082% when pulling 4c97102 on FDT2k:master into d6d54df on paramiko:master.

coveralls · 2017-01-09T06:41:20Z

Coverage decreased (-0.3%) to 73.903% when pulling a225bc8 on FDT2k:master into d6d54df on paramiko:master.

coveralls · 2017-01-09T06:44:13Z

Coverage decreased (-0.1%) to 74.082% when pulling a225bc8 on FDT2k:master into d6d54df on paramiko:master.

coveralls · 2017-01-09T06:44:13Z

Coverage decreased (-0.1%) to 74.041% when pulling a225bc8 on FDT2k:master into d6d54df on paramiko:master.

ploxiln · 2017-02-24T18:46:12Z

Worth documenting here that this is a new feature in ssh 7.3p1, and this PR does seem to reproduce it pretty closely.

 Include
         Include the specified configuration file(s).  Mul‐
         tiple pathnames may be specified and each pathname
         may contain glob(3) wildcards and, for user config‐
         urations, shell-like “~” references to user home
         directories.  Files without absolute paths are
         assumed to be in ~/.ssh if included in a user con‐
         figuration file or /etc/ssh if included from the
         system configuration file.  Include directive may
         appear inside a Match or Host block to perform con‐
         ditional inclusion.

bitprophet · 2017-03-01T23:32:58Z

paramiko/config.py

+                for filename in glob.iglob(path):
+                    if os.path.isfile(filename):
+                        if filename in parsed_files:
+                            raise Exception("Include loop detected in ssh config file: %s" % filename)


This should be some more specific exception class, using just Exception is almost never useful (it's impossible to catch without catching everything else too!); not sure what works best though. Maybe ValueError, or a new custom exception class (meh).

bitprophet · 2017-03-01T23:39:41Z

This is a comparatively easy part of the code to test so I'd love it if tests could be added.

bitprophet · 2017-03-01T23:41:48Z

Was gonna drop this as a line comment but not sure I trust github to display it even after the code is modified, and it got complex enough to be a permanent part of the discussion. It's re: the treatment of the file path stuff on line 87:

That's not how I'd interpreting the current OpenSSH docs...it's just stating that it allows normal shell-like globbing and user home directory references (e.g. ~/some-dir/some_file.conf expands to be /home/myusername/some-dir/some_file.conf - not ~/.ssh/some-dir/some_file.conf).

What they mean by "assuming ~/.ssh" is simply that the implicit relative location is ~/.ssh, so Include foo means Include ~/.ssh/foo:

Files without absolute paths are assumed to be in ~/.ssh if included in a user configuration file or /etc/ssh if included from the system configuration file.

Also, that brings up another wrinkle, namely the detection of whether the current file being parsed is a "user" or "system" configuration file (and they do not even mention how it is expected to behave if it is a runtime-specified file...). Need to think about that, I could see a few different ways to handle it:

Look at the file path of the file object, if possible, and if it appears to be a user or system file, interpret relative paths as documented
Simply grab the os.path.dirname of that file path and use it as the relative root; this automatically works for regular user/system conf files, but is possibly more poorly defined for runtime files and especially files loaded via Include itself (!).
- At the same time, this feels like a relatively intuitive behavior to me - that a conf file include would be relative to where the including file is located.
- But generally, especially given this feature area's whole point, we want to ape OpenSSH's behavior as closely as possible...so maybe source diving is needed.
The specific-to-Python case of the file object being unrelated to a real file (i.e. a StringIO or other FLO) is poorly defined here, but I reckon somebody expecting to use a relative-path Include directive in such a situation is probably just asking for trouble...

tescalada · 2017-05-19T17:09:35Z

I think the relative locations handling here is correct.

My ssh config uses Include config.d/*
which then expands to parse out /Users/tristan/.ssh/config.d/*

I tried adding Include ~/sshtest
which correctly parsed the file in /Users/tristan/sshtest

tescalada · 2017-05-19T17:47:29Z

I would be happy to help out by adding some unit testing here, but it does not seem as straight forward to add the tests as it is for the other config tests. I would either need to add mock as a requirement to the project, or drop some test config files into the repo to be able to parse finding and reading the actual files (although I would not be able to test ~ this way).

Do you have a preference on those two options, or a better option I didn't think of?

brycefisher · 2017-08-24T03:40:04Z

This PR makes me super happy!! I'd love to see this get merged in the nearish future and I'm happy to help contribute some tests if @FDT2k would be amenable to PRs against this PR :-)

It seems like the work that needs doing here is:

Add tests somehow... Would @ploxiln / @bitprophet or others object to introducing mock as a dependency into this project to help with testing things like tilde expansion?
Get consensus on how to handle relative file paths between. It appears the current implementation in this PR would expand:
- Include foo -> $HOME/.ssh/foo
- Include ~/foo -> $HOME/foo
- Include /foo -> /foo
  To me that seems right as is. @bitprophet should that be something else?
Decide whether or not check if the file is a system or user configuration. My opinion is that we ought to skip that check and revisit this design if it causes issues for users in the future. If we really wanted to do this check, then we could look at what OpenSSH does -- but again, I would just do the tilde expansion regardless to keep the implementation simple and portable.
Raise a more specific exception in the case of a file inclusion loop. I have no opinion on what a better one would be, but I agree more specific than Exception would be great. I'd be happy to provide ValueError instead.

brycefisher · 2017-09-05T19:02:28Z

@bitprophet @ploxiln looks like both @tescalada and I are willing to help get this PR over the finish 🏆

I just wanted to say thanks for maintaining this awesome project and all the love and attention you've put into this. Whenever you all have a minute, we'd love to get your two cents on the items @tescalada and I asked about. Thanks again!!!

haakonn · 2017-12-06T11:04:39Z

This doesn't support globs, such as this:

Include config.d/*.conf

This leads to Exception: Include loop detected in ssh config file: /home/username/.ssh/config.d/*.conf (but there is no include loop).

PMickael · 2017-12-20T14:04:48Z

How about a merge ? 🙏

humiaozuzu · 2018-04-13T13:00:32Z

Any update on this PR?

ghost · 2018-04-22T15:37:31Z

Rebased against current master, works fine.

@bitprophet

That's not how I'd interpreting the current OpenSSH docs...it's just stating that it allows normal
shell-like globbing and user home directory references (e.g. ~/some-dir/some_file.conf expands
to be /home/myusername/some-dir/some_file.conf - not ~/.ssh/some-dir/some_file.conf).

Original author @FDT2k implementation is correct : I compared file expansion with native openssh client on Debian stable.

Also, that brings up another wrinkle, namely the detection of whether the current file being parsed
is a "user" or "system" configuration file (and they do not even mention how it is expected to behave
if it is a runtime-specified file...)

Good catch, please see #1194.

@haakonn

This doesn't support globs, such as this:
Include config.d/*.conf

I cannot reproduce your issue : importing patterns with wildcards patterns work fine here. It seems you are trying to import a file multiple times. (see 9d183fe#diff-a58d9daf5b4cbdffb4dee739593a7813R101)

danihodovic · 2020-12-13T18:37:46Z

How about a merge ? 🙏

guenhter · 2021-06-10T05:55:41Z

@FDT2k this is such a useful feature. Would you mind working in the one comment of the review.

jsiverskog · 2021-08-11T08:34:55Z

i cleaned this up and added a test (#1892)

anarcat · 2023-04-04T15:58:51Z

this PR should be closed in favor of #1892.

bitprophet · 2023-05-12T17:46:21Z

Yup, rolling into #1892

Added support for parsing 'Include' directive in ssh config file.

9d183fe

ploxiln reviewed Jan 8, 2017

View reviewed changes

code style and loop detection bugfix

4c97102

fix typo

a225bc8

FDT2k force-pushed the master branch from 3919cd3 to a225bc8 Compare January 9, 2017 06:38

bitprophet added Feature ssh_config Needs changelog/docs Needs tests labels Mar 1, 2017

bitprophet reviewed Mar 1, 2017

View reviewed changes

ploxiln mentioned this pull request Aug 24, 2017

Support the Include directive in ssh config #1044

Closed

ghost mentioned this pull request Apr 22, 2018

Support include directives in ssh_config files #1194

Closed

ganeshrn mentioned this pull request Nov 28, 2018

netconf connection plugin shall allow master ~/.ssh/config file ansible/ansible#48439

Closed

bitprophet mentioned this pull request Jul 2, 2019

Add support for new-ish 'Match' config directive #717

Closed

WinstonN mentioned this pull request Jan 22, 2020

Do you have any intention to ever support ssh_config Include files? #1609

Open

bitprophet added this to the p1 milestone Dec 23, 2020

totaam mentioned this pull request Jun 11, 2021

paramiko ssh config parser can't parse included ssh config files. Xpra-org/xpra#3163

Closed

jsiverskog mentioned this pull request Aug 11, 2021

support ssh config include directive #1892

Closed

bitprophet modified the milestones: Subsequent feature release, Next feature release Sep 24, 2021

robdennis mentioned this pull request May 19, 2022

[BUG] Include directive in sshconfig results in parse error when using salt-ssh / sshconfig roster saltstack/salt#62084

Open

bitprophet closed this May 12, 2023

Uh oh!

Conversation

FDT2k commented Jan 8, 2017

Uh oh!

coveralls commented Jan 8, 2017

Uh oh!

coveralls commented Jan 8, 2017

Uh oh!

coveralls commented Jan 8, 2017

Uh oh!

coveralls commented Jan 8, 2017

Uh oh!

ploxiln Jan 8, 2017

Choose a reason for hiding this comment

Uh oh!

ploxiln Jan 8, 2017

Choose a reason for hiding this comment

Uh oh!

ploxiln Jan 8, 2017

Choose a reason for hiding this comment

Uh oh!

ploxiln Jan 8, 2017

Choose a reason for hiding this comment

Uh oh!

ploxiln commented Jan 8, 2017

Uh oh!

coveralls commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 9, 2017

Uh oh!

ploxiln commented Feb 24, 2017

Uh oh!

bitprophet Mar 1, 2017

Choose a reason for hiding this comment

Uh oh!

bitprophet commented Mar 1, 2017

Uh oh!

bitprophet commented Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tescalada commented May 19, 2017

Uh oh!

tescalada commented May 19, 2017

Uh oh!

brycefisher commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brycefisher commented Sep 5, 2017

Uh oh!

haakonn commented Dec 6, 2017

Uh oh!

PMickael commented Dec 20, 2017

Uh oh!

humiaozuzu commented Apr 13, 2018

Uh oh!

ghost commented Apr 22, 2018

Uh oh!

danihodovic commented Dec 13, 2020

Uh oh!

guenhter commented Jun 10, 2021

Uh oh!

jsiverskog commented Aug 11, 2021

Uh oh!

anarcat commented Apr 4, 2023

Uh oh!

bitprophet commented May 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

coveralls commented Jan 9, 2017 •

edited

Loading

coveralls commented Jan 9, 2017 •

edited

Loading

coveralls commented Jan 9, 2017 •

edited

Loading

bitprophet commented Mar 1, 2017 •

edited

Loading

brycefisher commented Aug 24, 2017 •

edited

Loading