Add support for sshd_config include files

[nvergottini]

Add include_dir parameter for specifying an include directory at the top of sshd_config.

Add ssh::server::config_file resource type for creating config files within the include directory. Provides include parameter for including externally managed config files. This is primarily intended for including crypto policies in RedHat 9 family.

Add data for RedHat 9 family to add include directory and config file to load crypto policies for OpenSSH server by default.

[nvergottini]

I developed this change after I discovered that using crypto policies on Oracle Linux 9 to manage sshd crypto settings was not working like it did with Oracle Linux 8. Without the "Include /etc/ssh/sshd_config.d/*.conf" at the top of sshd_config, the "Include /etc/crypto-policies/back-ends/opensshserver.config" at the top of /etc/ssh/sshd_config.d/50-redhat.conf was not being loaded, so crypto policies were not being applied. EL8 uses a completely different mechanism to manage sshd crypto settings that was not affected by the use of this module.

[okopop]

Would be great if this could be implemented asap, it's a crucial part of getting sshd crypto settings to work properly from EL9 and forward. On EL8 it was not needed but now it does not work correctly without the "Include" part as described. We are in great need of it.

[saz]

I'll have a look at it tomorrow

[okopop]

Hi, @saz and @nvergottini Great job, sorry for late response and testing, I have tried it in our environment now and have some comments.

I think there have been some missunderstanding regarding management of the 50-redhat.conf file.

1: The /etc/ssh/sshd_config.d/50-redhat.conf already exist(from openssh-server-8.7p1-38.el9_4.4.x86_64 on rhel9 in our case) and should not be messed with/overwritten by default in data/RedHat-9.yaml

The 50-redhat.conf file consists of "Include /etc/crypto-policies/back-ends/opensshserver.config" and other ssh settings by default already.

i.e. remove these lines. It should be optional to customize this file further.

ssh::server::config_files:
  50-redhat:
    include: '/etc/crypto-policies/back-ends/opensshserver.config'

2: The important part, the Include part in /etc/ssh/sshd_config looks good, it should be added as the default, no further adjustments. i.e.

# cat /etc/ssh/sshd_config
# File is managed by Puppet
Include /etc/ssh/sshd_config.d/*.conf
...

[nvergottini]

I would agree with that change.

[saz]

The 50-redhat.conf file consists of "Include /etc/crypto-policies/back-ends/opensshserver.config" and other ssh settings by default already.

i.e. remove these lines. It should be optional to customize this file further.

ssh::server::config_files:
  50-redhat:
    include: '/etc/crypto-policies/back-ends/opensshserver.config'

As the file resource has purge => true by default, all unmanaged files will be removed in /etc/ssh/sshd_config.d. Maybe we should consider a way of marking a file within the sshd_config.d dir as safe? It can be done with a simple file resource, but I'd still suggest some kind of wrapper around it to avoid duplications (e.g. directory path).

[okopop]

From our point of view that may be a bit overkill. If we just remove the default "ssh::server::config_files:" in data/RedHat-9.yaml we can use include_dir_purge: false in hiera. Then we have all the options we really need.

E.g. Scenario 1: If we want to manage everything under /etc/ssh/sshd_config.d/ we can do that by defining it in hiera, and enforce it with the default include_dir_purge: true.

Scenario 2: If we want a combination of both, i.e. creating specific config-files from hiera and also using drops-in coming from rpms, we can, with include_dir_purge: false.

BUT for that to work correctly recurse must also be set to false. Maybe automatically set "recurse" to the same as "purge", they need to be disabled and enabled together, or maybe create a "include_dir_recurse".

Test: If you set "include_dir_purge: false" but keep "recurse => true" we see this behaviour:

Files under /etc/ssh/sshd_config.d/ are created with 0600 from the module. As expected from "data/common.yaml -> ssh::server::sshd_config_mode: '0600'"

If we then create a /etc/ssh/sshd_config.d/xyz.conf file manually(mimic what an rpm would do) and give it 0600, the module wants to change the xyz.conf file to 0700. I guess it must do so because file resource "file { $ssh::server::include_dir:" has "mode => $ssh::server::include_dir_mode," which is 0700.

If you set both purge and recurse to false only files created by the module will get touched and managed. As described in scenario 2 above. Which is desired behaviour.

If you think some kind of whitelisting is a better solution, that works too.

[saz]

I wouldn't see it as overkill. My expectation would be, to have sane defaults set.

If I'm taking some other module as an example, I like the cleanup within the config (or in this case: include) dir and would expect that. Anything, which isn't managed, should be removed, as it might change something in an unexpected way.

Therefore, purging by default isn't wrong for me. Having a way to define something, which should be kept seems logical to me, if, by default, things are getting purged. This way, you don't have to explicitly remove some config snippet with ensure => absent (I, personally, tend to forget that step :grin: )

The mode issue is easily solvable, by setting the mode to 0600 as the file type will handle it correctly on directories. This way, files and directories will get the proper permissions: 0700 for directories, 0600 for files.

[okopop]

Thanks for the fast response, good stuff!👍 Oh, I wrote some of it in a hurry, I can see now when I read it that it could be misunderstood, I just explained some possible scenarios. I expressed myself poorly, sorry, my point or intention was not to change the default value of purge to false, purging by default is great.

My point was that if you choose to disable purge for whatever reason (it's possible because it's a parameter after all) recurse needs to change with it in this current implementation, or solved in some other way, to not get this weird behaviour with permissions on other files(if any exist), that's all. I was just demonstrating what we saw in our tests. We are happy with whatever path this takes.