saltstack-formulas / openssh-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
90 stars 297 forks source link

sshd_config ends up almost empty when trying to modify one value #123

Closed arthurzenika closed 6 years ago

arthurzenika commented 6 years ago

When adding the following pillar and using openssh.config :

sshd_config:
  PermitRootLogin: no

I end up with the following file

# This file is managed by salt. Manual changes risk being overwritten.
# The contents of the original sshd_config are kept on the bottom for
# quick reference.
# See the sshd_config(5) manpage for details

PermitRootLogin no

I'm not sure this is a good result, are the default values shipped by the distro always the default values upstream ? I don't think so.

Any chance of getting this config to be handled in a way that keeps the default values shipped by the distro. Maybe with https://docs.saltstack.com/en/latest/ref/states/all/salt.states.ini_manage.html ?

0xf10e commented 6 years ago

I think the last discussion about this ended with "if no pillars are set don't touch the file at all" but this doesn't look like the intended result for a minimal config either… Keeping all the different defaults for all the different platforms and their versions in the formula would be difficult to maintain so we relied on the sensible compiled-in defaults existing for most values. AFAICT sshd_config(5) doesn't support includes so we can't just use overrides in a 2nd file and I don't know if we have a decent parser for this format (not .ini, mostly key/value but with Match blocks possible).

javierbertoli commented 6 years ago

I'm not sure this is a good result, are the default values shipped by the distro always the default values upstream ? I don't think so.

@arthurlogilab, without meaning that the results you pasted are 'good or bad', Those result will end up using the distro-provided defaults, with your particular changes overwriting the distro-provided defaults.

Generally speaking, I think distro-provided are 'upstream's defaults + changes that make the daemon work nicely with our distribution', so I'd say that the results you get with the that config file will be what you want (upstream -> distro-provided changes -> your-changes).

I don't know if we have a decent parser for this format (not .ini, mostly key/value but with Match blocks possible).

@0xf10e , I think that we might try to create a resource that dumps the current working config (run sshd -T to dump the current config and write that down to the /etc/ssh/sshd_config file), but I'm not sure if that's something we'd like to do by default. Perhaps adding a explicit_verbose_config: true or something like that so, whoever prefers the complete config can get it?

OTOH, as a dumb fix, perhaps @arthurlogilab might want to run this command in his host/s and dump those values to his pillar/s.

arthurzenika commented 6 years ago

@javierbertoli did you take a look at my pull request ? I does what I'm describing.

@0xf10e as the "Sections" part of the wikipedia page states about ini files : https://en.wikipedia.org/wiki/INI_file "Keys may (but need not) be grouped into arbitrarily named sections. " So sshd_config according to this definition is an ini file and can be handled with ini.options_present by using no sections and separator = ' ' (as implemented in the PR)

0xf10e commented 6 years ago

As long as there are no Match blocks like those:

PasswordAuthentication no  
X11Forwarding no  
### this should be on the bottom of the config file   
### Enable password authentication for IP 1.2.3.4   
Match Address 1.2.3.4  
 PasswordAuthentication yes  

Match User John Address 172.16.1.*   
X11Forwarding yes  

(From Limit access to openssh features with the Match option)