Bug 1585 - Allow an `Include' option which reads another config file in place and does not error out when `Include' file not readable
Summary: Allow an `Include' option which reads another config file in place and does n...
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: ssh (show other bugs)
Version: 5.2p1
Hardware: All All
: P3 enhancement
Assignee: Assigned to nobody
URL:
Keywords: patch
Depends on:
Blocks: V_7_3
  Show dependency treegraph
 
Reported: 2009-04-02 21:29 EST by Gavin Beatty
Modified: 2016-08-04 18:23 EST (History)
28 users (show)

See Also:


Attachments
Attached is a patch for an almost complete implementation. All that remains is the "file not readable is not an error" logic. (5.17 KB, patch)
2009-04-02 21:29 EST, Gavin Beatty
no flags Details | Diff
Include option patch for OpenSSH 6.2 (4.60 KB, patch)
2013-05-24 15:08 EST, Christian Kujau
no flags Details | Diff
proposed patch for ssh config (2.88 KB, patch)
2015-06-10 19:15 EST, Jakub Jelen
no flags Details | Diff
Include support and regress test (10.12 KB, patch)
2016-02-17 13:16 EST, Damien Miller
no flags Details | Diff
remove duplicate code from regress test (2.83 KB, patch)
2016-08-04 18:23 EST, Jakub Jelen
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Beatty 2009-04-02 21:29:47 EST
Created attachment 1623 [details]
Attached is a patch for an almost complete implementation. All that remains is the "file not readable is not an error" logic.

Adding the following to your config file should read the named file in place.

Include "~/.ssh/config.customer1"

If the file is not found, only a warning should be given, like so:

warning("%s line %d: Include file \"%s\" not found - skipping.", ...);

~/ and ~username expansion should be available.

Attached is a patch for an almost complete implementation. All that remains is the "file not readable is not an error" logic.
Comment 1 jakobhilden 2012-05-21 16:59:52 EST
+1 Would be a very, very useful feature.  And I know a bunch of other devs who also need this.
Comment 2 Tomas Pospisek 2012-12-08 05:45:31 EST
+1 I needed just this today

The usecase is the following:

* my employer is maintaining a ssh_config file that registers all machines
* I have some settings and hosts of my own

How do I usefully integrate those two files?
*t
Comment 3 Pierre Adam 2013-01-20 23:53:36 EST
+1 This feature could be very useful.

@Tomas Pospisek : For now you can use only do some cheat with a shell script.
For example having your configuration file in ~/.ssh/config.d and make that type of alias:
alias ssh="cat ~/.ssh/config.d/* > ~/.ssh/config ; ssh"
It my way to keep my config file up-to-date.

I subscribe to this ticket.

(In reply to comment #2)
> +1 I needed just this today
> 
> The usecase is the following:
> 
> * my employer is maintaining a ssh_config file that registers all
> machines
> * I have some settings and hosts of my own
> 
> How do I usefully integrate those two files?
> *t
Comment 4 StalkR 2013-02-04 00:43:13 EST
+1
Comment 5 dkived 2013-04-19 04:33:50 EST
+1 here as well.

I'm trying to set up a secure Apt archive using ssh to limit access. I'm also making a Debian package to set up access to the archive, so it would be nice to add the Host stanza to a separate file for maintenance reasons.

Methods such as using aliases/scripts to aggregate the various config files before execing ssh won't work for this, as apt-get is not going to call an shell-defined alias.

Guess I'm gonna have to fall back to my old method of using scripts to rewrite /etc/ssh/ssh_config - which is a horrible way to do things. This is precisely why pretty much every major Linux service uses config.d directories instead of monolithic config files. EXCEPT OPENSSH.

This request has now been ignored for 4 years. It would be nice to see some sort of response.
Comment 6 amontero 2013-04-24 03:00:31 EST
Definitely +1 to this!

An alternative (and more generic) approach would be doing this by intercepting .conf file reads (via FUSE?). After thinking a little about this and googling, I've found something that might be a good start:
https://code.google.com/p/scriptfs/

@dkived: Maybe you can try this for your scenario and post how it works. I haven't tried it but looks like it can do the trick.
Comment 7 joshua.shaffner 2013-05-13 04:57:24 EST
+1
Comment 8 Christian Kujau 2013-05-24 15:08:22 EST
Created attachment 2274 [details]
Include option patch for OpenSSH 6.2

This is really just a modified version of Gavin Beatty's patch, slightly altered so it'll apply cleanly to OpenSSH 6.2 (i.e. yesterdays CVS checkout).
Comment 9 amontero 2013-06-11 03:55:30 EST
Another, FUSE-based solution (not tested myself):
https://github.com/markhellewell/sshconfigfs
Comment 10 Christian Kujau 2013-06-11 09:23:48 EST
While FUSE-based solutions are available, they're hardly portable and not available on most of the platforms listed on http://openssh.com/portable.html.

What does it take to convince the developers to include Gavin's patch into mainline?
Comment 11 schmurfy 2014-02-04 22:12:11 EST
2009 ? This bug is rather young, I am sure at least 10 years is mandatory to have something as complex as this integrated... There are even patchs attached without even a comment on them.

As far as I can tell user interface is clearly not the focus of either OpenSSL or OpenSSH, just take a look at the OpenSSL api if you need convincing.

The funny thing is that if security is the concern here this is stupid since users will do things that may open a real security risk instead of relying on a proven built in Include which could just apply the same rules it uses when loading the main ".ssh/config".

What is preventing this from getting in ???
This a really helpful feature if you want to work with generated files without destroying you main config file.
Comment 12 Heikki Levanto 2014-06-03 19:08:33 EST
+1

I would also like to see this feature. My use case is that we have a company-wide ssh_config for all our servers, and I need to have some for my own private things as well.

I think it would be even more cool to have a conf.d style directory, at least under /etc/ssh, so we could make a package that installs our company's file(s) there. But a simple include directive would solve all my immediate problems, either in /etc/ssh_config, and/or in ~/.ssh/config.
Comment 13 emlyn.corrin 2014-10-28 01:02:06 EST
+1
Any progress on this?
Comment 14 victor.engmark 2014-12-04 06:04:15 EST
Is the "low-hanging-fruit" keyword really appropriate?
Comment 15 andrew 2014-12-15 05:21:15 EST
+1

The usefulness of this is increasing with the increasing use of automation in system administration.  It's increasingly feasible to automate and distribute configuration files populated with host details, but there's no good way to integrate this with a user's own preferences.
Comment 16 hvjunk 2015-03-27 02:04:20 EST
++1

This gets even more needed as I'm managing multiple clients's systems, and they update/change their information/settings, and I need to update my config file trying to find where what is inserted.
At least a directory with separate files for each client would've helped/solved that issue.

Not to mention to keep my laptop, home desktop and work jumphosts all in sync, or with only the specifics needed for the specific host/device, there the include files directives would be great and helpfull
Comment 17 Stefan 2015-04-17 21:56:24 EST
+1

I'm writing software that uses ssh-agent. And as ssh-agent does not support command line options, I can't make ssh use a config file within the application configuration directory using the "-F" parameter. I have to rewrite the config in the user home, which is a terrible thing. It would be way better to add an include to the application specific one that can then be managed by the application alone.
Comment 18 andrew 2015-04-17 23:26:38 EST
I think this is an obviously useful thing to add to openssh, and I've yet to see any argument against it, but given that it's been 6 years I'm not holding my breath.

As a simple workaround, I'd suggest people create a directory called `.ssh/config.d`, touch `.ssh/config.d/00_empty` and add a line to their `.bashrc` like `cat .ssh/config.d/* > .ssh/config`.

All of that could be done in the skeleton home directory for new users.  (On a debian system, that's /etc/skel), or it could be done through a configuration management system like puppet if you use that.

This is slightly different to the include statement idea, but should cover most use cases.  For the actual include statement approach, I'm sure it would be possible to do something similar as a pre-processed macro.

Is there a better way to trigger this than in .bashrc?
Comment 19 victor.engmark 2015-04-23 02:24:08 EST
(In reply to andrew from comment #18)
> Is there a better way to trigger this than in .bashrc?

.bashrc is easy and convenient, but it's not safe. Simply put, how do you guarantee that starting a terminal, `exec $ SHELL` or simply running `bash` doesn't interfere with a background `ssh` process? You should instead apply such changes at the *login* level, but that depends on how you start a login session. I use ~/.xprofile <https://github.com/l0b0/tilde/blob/2247c76e4851eff63643b31179c0b1ca97e995fb/.xprofile>, but that may not be appropriate for you.
Comment 20 Jakub Jelen 2015-06-10 19:15:05 EST
Created attachment 2647 [details]
proposed patch for ssh config

I don't know why there is not to used function tilde_expand_filename, which does exactly the same thing that you are implementing on these 80 lines.

It works fine for including single file, but for conf.d use cases (described in #2351 and #1613) would be great to have possibility to include all directory.

I massaged this patch to do so (tilde, wildcard) and I would like to open the discussion on this topic again. There is a lot of users who would appreciate this feature in upstream, but no upstream response.

This feature, especially if massaged for sshd_config (can be applied also for server with small change), would be really helpful for packaging specific configuration dependent on installed packages without rewriting the only config file. To have the report complete, I link Fedora feature request we were discussing recently:
https://bugzilla.redhat.com/show_bug.cgi?id=1225752
Comment 21 Tomas Pospisek 2015-06-11 06:25:40 EST
@Jakub Jelen specifically, but to the other people here in general as well.

I have not studied the proposed patches, but one problem that I am seeing in general with the approach is, that once we start including/merging multiple configurations, we will start seeing conflicts of config options and overrides.

This does not *have to* be a big problem in specific cases, but it is a problem in the general case, when one starts mixing configuration bits coming from different sources with different trusts.

ssh does not warn when you have twice the same setting with different options, so configuration snipplets from your company can override your own settings without you noticing. It can change host settings. It can change security settings. It can change how ssh connects to where.

I myself am using a "cat ~/.ssh/config.d/* > ~/.ssh/config" approach myself and that certainly works and is useful for me, but I have also been already bitten by the above mentioned problem once. It wasn't anything serious, but stuff did start behaving slightly unexpectedly until I noticed that an imported bit of ssh config had changed...
Comment 22 Damien Miller 2016-02-17 13:16:17 EST
Created attachment 2790 [details]
Include support and regress test

There were a few problems with the previous patches, most due to host/match state persisting between files in non-intuitive ways. This diff covers all the cases that I can think off and adds a regression test.
Comment 23 Jakub Jelen 2016-03-04 03:11:43 EST
(In reply to Damien Miller from comment #22)
> There were a few problems with the previous patches, most due to
> host/match state persisting between files in non-intuitive ways.

This was partially intention. There is nothing worse than including file into some unknown context and when the file returns, getting different context. But in this seems to be solved in pretty clever way and also tested in your current patch.

> This diff covers all the cases that I can think off and adds a
> regression test.

Thank you for having a look into that. I finally got to try that and it seems to be working as expected (both regress tests and hand-checking).

Some thoughts/bugs:

 * if glob does not match any file, there should be at least debug/verbose log about that. Certainly not fatal (as the bug title proposes), but at least for debugging purposes. Something like this:

    verbose("%s: glob %s does not match any file", __func__, arg2);

 * in the last regress test, the include line is commented out, which makes last tests pass regardless the included files:

@@ -332,7 +333,7 @@
-+#Include $OBJ/ssh_config.i.*
++Include $OBJ/ssh_config.i.*
Comment 24 Damien Miller 2016-04-15 13:01:08 EST
Slightly modified patch applied, this will be in openssh-7.3

commit dc7990be865450574c7940c9880567f5d2555b37
Author: djm@openbsd.org <djm@openbsd.org>
Date:   Fri Apr 15 00:30:19 2016 +0000

    upstream commit
    
    Include directive for ssh_config(5); feedback & ok markus@
    
    Upstream-ID: ae3b76e2e343322b9f74acde6f1e1c5f027d5fff
commit 35f22dad263cce5c61d933ae439998cb965b8748
Author: djm@openbsd.org <djm@openbsd.org>
Date:   Fri Apr 15 00:31:10 2016 +0000

    upstream commit
    
    regression test for ssh_config Include directive
    
    Upstream-Regress-ID: 46a38c8101f635461c506d1aac2d96af80f97f1e
Comment 25 Tomas Pospisek 2016-04-16 03:21:36 EST
Thanks to everybody how contributed patches and helped moving this featureinto ssh. And in particular to Damien to applying patch and tests and including the feature!

Thanks
*t
Comment 26 Damien Miller 2016-08-02 10:42:27 EST
Close all resolved bugs after 7.3p1 release
Comment 27 Jakub Jelen 2016-08-04 18:23:27 EST
Created attachment 2859 [details]
remove duplicate code from regress test

Not sure how this get in, but the code in the regress/cfginclude.sh is duplicated (unlike in the patch attached to this bugzilla), except of the umask part, set to the first copy, and the part

    # Ensure that recursive includes are bounded.

which is only in the second copy.

The attached patch removes the duplicate code. No reopening, since it is not very critical, but if would be nice to have cleaned up :)