Created attachment 2706 [details] 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.
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 :/
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.
Created attachment 2869 [details] 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).
If this is implemented, a default/implicit include of /etc/sshd_config.d/* would be much appreciated.
(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.
*** Bug 2351 has been marked as a duplicate of this bug. ***
Created attachment 3223 [details] Include server side (with same semantics as client) - updated for 7.9p1 Here is an updated patch that applies against 7.9p1
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?
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).
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.
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).
Created attachment 3250 [details] 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.
Hey Damien, Any chance you could take a look at the updated patch from Jakub?
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.
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.
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.
Created attachment 3333 [details] sample sshd_config update command to implement behaviour on GNU Linux with systemd
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.
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.
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.
Created attachment 3350 [details] revised patch reworked version of the patch using the preferred sys/queue.h linked list macros and a few other stylistic tweaks
Created attachment 3351 [details] regress patch slightly-tweaked regression test
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.
this has been committed and will be in openssh-8.2
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?
(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.
closing resolved bugs as of 8.6p1 release
Woops, wrong bug, sorry for the noise!