openssh bugs - Sep 2015 - [Bug 2468] New: Option to include external files to sshd_config

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

            Bug ID: 2468
           Summary: Option to include external files to sshd_config
           Product: Portable OpenSSH
           Version: 7.1p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jjelen at redhat.com
        Depends on: 2463

Created attachment 2706
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2706&action=edit
proposed patch

This topic was discussed in few bugs (namely 1613, 2146, 1585) for
client side and the last one also contains patch that can be applied to
the current openssh. Having the same feature for server side makes the
same sense for me so I created patch also for server side.

To understand the main reason behind this feature, it is our
"system-wide crypto policy", which should allow us to enforce specific
security policies in uniform way over the most crypto tools in whole
system.

This feature will also allow us to have default drop-in directory,
which will also improve the packaging possibilities for third party
tools and will make possible to update main config without conflict
with changes made by users.

All tests still passing. If you would like from me to implement also
test cases to cover this feature, let me know. I see there are tests
for most of the recent features.

Please note, that the prerequisite for this feature is solving the bug
#2463 (openbsd compat glob), which makes sshd segfault with this patch
in kerberos library.


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2463
[Bug 2463] Conflict with openbsd compat glob() function in shared
libraries
-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468
Bug 2468 depends on bug 2463, which changed state.

Bug 2463 Summary: Conflict with openbsd compat glob() function in shared
libraries
https://bugzilla.mindrot.org/show_bug.cgi?id=2463

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
IMO the desirable semantics of Include in sshd_config are even more
subtle and fraught than those of ssh_config. For example, how should
the following behave:

Port 22
Include /etc/ssh/config_a
PasswordAuthentication no
Include /etc/ssh/config_b

Match user djm
Banner /etc/banner
Include /etc/ssh/config_c

---

If inclusion operates just by pasting text in, then config_a could
radically alter the following configuration if it includes a Match
directive.

Similarly, config_c's application conditional on the previous Match
succeeding?

I wish we had a brace-ful configuration language - it would make
resolving these way simpler :/

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Zdenek Sedlak <dev at apgrco.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dev at apgrco.com

--- Comment #2 from Zdenek Sedlak <dev at apgrco.com> ---
I would propose to simply pasting the lines from the config file at the
position where the Include option is placed in the sshd_config, and
furthermore, it would be nice to have an Includedir option like the
sudo has.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2706|0                           |1
        is obsolete|                            |

--- Comment #3 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 2869
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2869&action=edit
Include server side (with same semantics as client)

(In reply to Zdenek Sedlak from comment #2)
> I would propose to simply pasting the lines from the config file at
> the position where the Include option is placed in the sshd_config
That was the initial idea and, if I am right. It is the way how it was
initially implemented. But the client side config was implemented in
more complex manner, that even the Include depends on the Match context
so implementing server side in different way does not seem like a good
idea.
> it would be nice to have an Includedir option like
> the sudo has.
Include with GLOB expansion does the same without additional complexity
of another option.

Reading the implementation of the client side config include, there
should be certainly some limit to the recursion and some regression
sanity test to make sure it does what it is supposed to do. Introducing
some more complexity would make it much harder to understand what is
going on there (though the debug log level is very helpful here).

(In reply to Damien Miller from comment #1)
> If inclusion operates just by pasting text in, then config_a could
> radically alter the following configuration if it includes a Match
> directive.
> 
> Similarly, config_c's application conditional on the previous Match
> succeeding?
The other possibility would be to reset the context for each include
file, but that looks even more confusing to me.
> I wish we had a brace-ful configuration language - it would make
> resolving these way simpler :/
That would be nice-to-have, but probably impossible to change now.

Attaching a new patch with a regression tests, providing the same
behavior as the client side config include.

Also added a check to make sure that the Include list is not empty
(missing in the client).

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Paul Wise <pabs3 at bonedaddy.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pabs3 at bonedaddy.net

--- Comment #4 from Paul Wise <pabs3 at bonedaddy.net> ---
If this is implemented, a default/implicit include of
/etc/sshd_config.d/* would be much appreciated.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

--- Comment #5 from Jakub Jelen <jjelen at redhat.com> ---
(In reply to Paul Wise from comment #4)
> If this is implemented, a default/implicit include of
> /etc/sshd_config.d/* would be much appreciated.
Yes, we have this already in fedora in client configuration files. The
server should go with the same approach. Incorporating this uniformly
into the default configuration file would be nice.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

SpiderX <spiderx at spiderx.dp.ua> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |spiderx at spiderx.dp.ua

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

online at bendem.be changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |lmwangi at gmail.com

--- Comment #6 from online at bendem.be ---
*** Bug 2351 has been marked as a duplicate of this bug. ***

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

online at bendem.be changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |online at bendem.be

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Patrick McLean <chutzpah at gentoo.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2869|0                           |1
        is obsolete|                            |
                 CC|                            |chutzpah at gentoo.org

--- Comment #7 from Patrick McLean <chutzpah at gentoo.org> ---
Created attachment 3223
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3223&action=edit
Include server side (with same semantics as client) - updated for 7.9p1

Here is an updated patch that applies against 7.9p1

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Benjamin Gilbert <bgilbert at backtick.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bgilbert at backtick.net

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Dusty Mabe <dusty at dustymabe.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dusty at dustymabe.com

--- Comment #8 from Dusty Mabe <dusty at dustymabe.com> ---
For the new CoreOS initiatives (Fedora CoreOS and Red Hat CoreOS) we
are looking at this functionality as we'd like to set
`PasswordAuthentication no` and `PermitRootLogin prohibit-password` by
default but make it easy for users to override those settings by using
drop-in config file fragments.

For context see
https://github.com/coreos/fedora-coreos-tracker/issues/138

Is it possible a feature such as this would be considered for inclusion
in sshd?

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Sean P. Kane <spkane00 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |spkane00 at gmail.com

--- Comment #9 from Sean P. Kane <spkane00 at gmail.com> ---
This would be very useful in many Linux workflows, including CoreOS.

Making the sshd_config compose-able can be vital in systems, where a
default sshd_config is provided, but another team might need to add a
configuration feature or two, without completely over-ridding the base
config.

I don't think that we need something more than sorting the files if
included using a glob and and then a simple paste on the configs (in
the sorted order) exactly where the include is located.

Most of the time, I only think that two people, teams or systems) will
be managing this, so while there may be edge cases, they should be
reasonably easy to avoid/fix (especially if documented).

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

--- Comment #10 from Damien Miller <djm at mindrot.org> ---
I took a quick look at this patch and it seem ok wrt the configuration
parsing side. However, it doesn't do the right thing wrt sshd's
self-reexecution.

When sshd accepts a connection, instead of just fork(2)ing a subprocess
to handle, it forks and re-executes sshd to ensure each child process
gets a different memory layout, re-randomised stack cookies, etc.

Part of the re-execution shuffle is passing the entire sshd_config from
the listener sshd process to the re-executed one. This ensures that the
configuration used is the one that sshd was originally started with,
not the one that happens to be in the filesystem at the time the
connection was received.

This patch doesn't do that. I think maybe if you extended include_list
to record the full text of each included file and then
marshaled/demarsheled that in
sshd.c:send_rexec_state()/recv_rexec_state() then you'd be close to
good.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

--- Comment #11 from Jakub Jelen <jjelen at redhat.com> ---
Damien,
thank you for having a look into this patch and pointing out the gaps.
The fulltext is already stored in the include_list structure as a
buffer, but you are right, I missed the re-exec when I was writing the
patch.

I will try to rebase it to the current master and address the re-exec
part (with some minor cleanups).

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3223|0                           |1
        is obsolete|                            |

--- Comment #12 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 3250
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3250&action=edit
Include server side with re-exec passing the include list (master,
2019)

The attached patch is rebased on current master with several cosmetic
issues fixed. As a disadvantage, it is passing more arguments to the
config processing functions.

The same code is available in my github fork:
https://github.com/Jakuje/openssh-portable/tree/sshd_include

One thing that might be missing is some cleanup of the allocated memory
before exits, which might make sense also for the original cfg value,
when it will no longer be needed.

Let me know if you have some more concerns to address regarding this
patch.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

--- Comment #13 from Dusty Mabe <dusty at dustymabe.com> ---
Hey Damien,

Any chance you could take a look at the updated patch from Jakub?

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Thomas Leavitt <thomas at thomasleavitt.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |thomas at thomasleavitt.org

--- Comment #14 from Thomas Leavitt <thomas at thomasleavitt.org> ---
Sample use case:

I'm setting up nearly separate user accounts on a server as sftp only
in separate chroots (each requiring a separate entry in sshd_config),
and programmatically sourcing the user list from a database. Right now,
every time that list is updated, I'm going to have to append the
relevant per user entries (93 at the moment) separate copy of the main
part of sshd_config, then overwrite /etc/ssh/sshd_config and restart
sshd. If I want to update sshd_config without doing this, I'm going to
have the make the edits in two places. Messy and error prone process,
obviously. 

Would be much cleaner to simply add and delete entries from an included
file or directory.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Ricardo <bugzilla.mindrot at rbartels.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla.mindrot at rbartels.d
                   |                            |e

--- Comment #15 from Ricardo <bugzilla.mindrot at rbartels.de> ---
Hi,

I just tested the patch this patch and it worked great. Looking for
this feature since a while.

I have a similar use case as Thomas. I'm running a backup server where
every user gets its own chroot and some different sshd_options. This
way I can create/delete a config file per user as they come and go.

It would be great to see this feature in 8.1.

Thank you.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Cole Robinson <crobinso at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |crobinso at redhat.com

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Ignaz Forster <prefix_mindrot at digitalimagecorp.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |prefix_mindrot at digitalimage
                   |                            |corp.de

--- Comment #16 from Ignaz Forster <prefix_mindrot at digitalimagecorp.de>
---
We are also very interested in such an option for openSUSE Kubic, for
similar reasons as Fedora CoreOS.
Moreover, during a discussion at this year's "All Systems Go!" it
got
clear that there is a broad interest in splitting package and admin
configuration into different files, both from distribution maintainers
and administrators. The proposed patch would make that possible.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

--- Comment #17 from Ricardo <bugzilla.mindrot at rbartels.de> ---
Created attachment 3333
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3333&action=edit
sample sshd_config update command to implement behaviour on GNU Linux
with systemd

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

--- Comment #18 from Ricardo <bugzilla.mindrot at rbartels.de> ---
Added script implement desired behaviour on GNU LINUX using systemd.

systemd unit drop in:

sudo mkdir -m 755 -p "/etc/systemd/system/sshd.service.d/"
cat <<EOF >
/etc/systemd/system/sshd.service.d/50-compile-sshd-config.conf
[Service]
ExecStartPre=-/usr/local/bin/sshd_config_update
EOF
chmod 644
/etc/systemd/system/sshd.service.d/50-compile-sshd-config.conf
systemctl daemon-reload


Hope this workaround helps someone.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Edgar Hoch <edgar.hoch at ims.uni-stuttgart.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|7.1p1                       |8.1p1
                 CC|                            |edgar.hoch at ims.uni-stuttgar
                   |                            |t.de

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

--- Comment #19 from Jakub Jelen <jjelen at redhat.com> ---
Damien, any update on this one? Any chance to have this re-reviewed and
merged? Are there still some concerns or reasons for not to implement
this from your side? It looks like there is already more than enough
people interested in this functionality.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3333|application/octet-stream    |text/plain
          mime type|                            |

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

--- Comment #20 from Patrick McLean <chutzpah at gentoo.org> ---
Not everyone uses systemd, and this feature is useful to non-systemd
users.

It might be useful to OpenBSD and FreeBSD users as well, not just us
Linux people. It's nicer and less error prone to have the daemon itself
be aware of the extra configuration files rather than having to run an
external script.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3250|0                           |1
        is obsolete|                            |

--- Comment #21 from Damien Miller <djm at mindrot.org> ---
Created attachment 3350
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3350&action=edit
revised patch

reworked version of the patch using the preferred sys/queue.h linked
list macros and a few other stylistic tweaks

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

--- Comment #22 from Damien Miller <djm at mindrot.org> ---
Created attachment 3351
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3351&action=edit
regress patch

slightly-tweaked regression test

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

--- Comment #23 from Jakub Jelen <jjelen at redhat.com>
---
>From fast review, I noticed only one minor issue:
> +				fatal("%s: too manu glob results", __func__);
There is a typo manu -> many

Otherwise it looks good.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #24 from Damien Miller <djm at mindrot.org> ---
this has been committed and will be in openssh-8.2

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

--- Comment #25 from online at bendem.be ---
What was decided with regards to including a file that contains a Match
clause? Is the match reverted to what it was before the include once
the control goes back to the including file?

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

--- Comment #26 from Jakub Jelen <jjelen at redhat.com> ---
(In reply to online from comment #25)
> What was decided with regards to including a file that contains a
> Match clause? Is the match reverted to what it was before the
> include once the control goes back to the including file?
Yes. This is the same as with the client config. Included files do not
affect the match context of the file including them.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Christian Kujau <mindrot at nerdbynature.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mindrot at nerdbynature.de

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #27 from Damien Miller <djm at mindrot.org> ---
closing resolved bugs as of 8.6p1 release

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2468

--- Comment #28 from Paul Wise <pabs3 at bonedaddy.net> ---
Woops, wrong bug, sorry for the noise!

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.