Test config before applying it

[alxwr]

I managed to lock myself out of two servers for the third time now. :-) (Aparently HostKeyAlgorithms is unknown to OpenSSH_6.7p1 Debian-5+deb8u3...) In anticipation of funny configuration bugs and/or my stupdity this PR adds a test prior to the change of sshd_config.

1. Fill sshd_config.saltstack-test with the exact same content which sshd_config should later contain.
2. Test it! sshd -t -f /path/to/sshd_config.saltstack-test
3. Iff (if and only if) that command succeeded change the actual config.

[alxwr]

Changes done according to review. :-)

[aboe76]

@alxwr , only issue I can see with this, is that it leaves a /var/tmp/sshd_config.saltstack-test file when it's done. luckily with the same permissions as the original sshd_config file but can this file be cleaned when all is good?

[alxwr]

@aboe76 I thought about this too. The test file is left intentionally to prevent ever-changing states. :-) It is world-readable as is /etc/ssh/sshd_config, so there's consistency, but we could also 0600 it.

One solution would be to compare config and test file using hashes, but I suppose this is going into a rather convoluted onlyif in order not to trigger changes. I just don't think that's necessary, because I see no (blocking) problem in leaving the file. (Correct me if I'm wrong! :-) )

[javierbertoli]

@aboe76, I have the same concern that you do regarding the file left behind, but then got to the same conclusion that @alxwr: as the macro is using the same permissions for the config file and the test file, security will be as strong as the one enforced on the config file.

Now that you mention it too and double thinking alternative approaches, I remembered what we use in the postgresl formula.

I think that, as we're using a file.managed state, maybe we can just use a check_cmd (and eventually tmp_ext) as described in the file.managed docs. This would let us skip the whole macro and double states approach.

Using check_cmd, I think

sshd_config:
  file.managed:
    - name: {{ openssh.sshd_config }}
    - source: {{ openssh.sshd_config_src }}
    - template: jinja
    - user: {{ openssh.sshd_config_user }}
    - group: {{ openssh.sshd_config_group }}
    - mode: {{ openssh.sshd_config_mode }}
    - check_cmd: /usr/sbin/sshd -t -f
    - watch_in:
      - service: {{ openssh.service }}

will do it, right? In the documetation on the link above you see an example that uses wathc_cmd to perform a check on the sudo files, with the same purpose we're trying to achieve here.

What do you think? Wouldn't this be cleaner?

[alxwr]

@javierbertoli Learning new tricks \o/ I'll give it a try.

[alxwr]

check_cmd is awesome! (memo to myself: RTM)

----------
          ID: sshd_config
    Function: file.managed
        Name: /etc/ssh/sshd_config
      Result: False
     Comment: check_cmd execution failed
              /tmp/tmpdRh2Tx: line 140: Bad configuration option: Aasdfasdf
              /tmp/tmpdRh2Tx: terminating, 1 bad configuration options
     Started: 12:53:50.633572
    Duration: 92.588 ms
     Changes:

Thanks @javierbertoli & @aboe76 for taking the time to review my PR! Now it's a two-line change.

[aboe76]

@alxwr , no problem, looks good to me, @javierbertoli would you do the honors?

[javierbertoli]

A pleasure! 👍 Thanks, guys!