Fix ssh config to handle custom options per Host

[fullyint]

Example problem

Currently, a config such as ssh_remote_hosts: ['host1', 'host2'] would yield this in ssh_config:

# ssh_config

Host host1
Host host2
...
PasswordAuthentication no
...

Let's check the resulting PasswordAuthentication per host.

# shell

# this is expected
$ ssh -G host2 | grep passwordauth
passwordauthentication no

# this is probably unintended
$ ssh -G host1 | grep passwordauth
passwordauthentication yes

# same unintended `yes` for any host missing from `ssh_remote_hosts`
$ ssh -G host3 | grep passwordauth
passwordauthentication yes

host2 is the only host to benefit from the secure ssh options set in this role. All other hosts would use the insecure defaults (e.g., including Ciphers, MACs, Kex, etc.).

Proposed fix

Given that the purpose of this role is to create more secure ssh defaults for all hosts, I figure ssh_remote_hosts was intended as a place for users specify a few custom options that differ from the new secure defaults.

This PR changes the handling of ssh_remote_hosts such that a variable definition such as this...

# defaults/main.yml

ssh_remote_hosts:
  - names: ['example.com', 'example2.com']
    options: ['Port 2222', 'ForwardAgent yes']
  - names: ['example3.com']
    options: ['StrictHostKeyChecking no']

... would produce this in the ssh_config ...

# ssh_config

# Host-specific configuration
Host example.com example2.com
  Port 2222
  ForwardAgent yes

Host example3.com
  StrictHostKeyChecking no

# Global defaults for all Hosts
Host *
...

That way all hosts get all the secure ssh defaults of this role, and users may specify custom overrides in ssh_remote_hosts.

Notes

I believe this change does not affect tests in dev-sec/ssh-baseline's current controls/ssh_spec.rb.

However, changing the structure of the ssh_remote_hosts variable would be a breaking change for users with any definition other than ssh_remote_hosts: []. They would see templating errors unless they understand they must restructure their ssh_remote_hosts variable.

Maybe that error would be important as a way to call users' attention to the fact that their current ssh_remote_hosts may not be functioning as they expect. Or perhaps you would prefer to give a new name to this restructured variable, e.g., ssh_options_per_host.

I believe the problem addressed in this PR is also present in dev-sec/chef-ssh-hardening and dev-sec/puppet-ssh-hardening.

[rndmh3ro]

That's a great PR, thanks for that!

I'm testing it right now, seems good so far.

On the breaking change issue: yes, that's a potential problem for users. That's why I've put it into the 4.0 milestone in accordance with semver (I hope).

One thing I'd like:

• Please add the vars into the play in the default.yml, like this:

---
- name: wrapper playbook for kitchen testing "ansible-ssh-hardening" with custom settings
  hosts: localhost
  roles:
    - ansible-ssh-hardening
  vars:
    network_ipv6_enable: true
    ssh_allow_root_with_key: true
    ssh_client_password_login: true
    ssh_client_cbc_required: true
    ssh_server_weak_hmac: true
    ssh_client_weak_kex: true
    ssh_remote_hosts:
    - names: ['example.com', 'example2.com']
      options: ['Port 2222', 'ForwardAgent yes']
    - names: ['example3.com']
      options: ['StrictHostKeyChecking no']
    
- name: wrapper playbook for kitchen testing "ansible-ssh-hardening" with default settings
  hosts: localhost
  roles:
    - ansible-ssh-hardening

[fullyint]

Thanks for reviewing!

I've added a commit with the requested changes. I can push a squashed branch if you'd like, or you could do a squash & merge with the GitHub GUI when it's time to merge.

[rndmh3ro]

I'm sorry, but I cannot test right now and I do not think that I will be able to test it this year...
If no one else will test and merge it, I will definitly do it in the beginning of next year, though!

Thanks for your PR again!

[conorsch]

On the breaking change issue: yes, that's a potential problem for users. That's why I've put it into the 4.0 milestone in accordance with semver (I hope).

That's great to hear, thanks for sticking to semver! Sounds like more testing here would be helpful, so I'll hit the docs and circle back to confirm the additions are working as intended.

[rndmh3ro]

Hey @fullyint. Sorry this is taking so long. Manually testing was a PITA so I spent the last weeks trying to make testing every OS in travis possible. Could you please rebase against master to see if the travis tests run trough?

As fot the conflict in default.yml: We need the pre_tasks in the play, but I'd like you to readd the vars and the newly added vars!

[fullyint]

I rebased and squashed commits. I added back in the play that runs the role with the custom settings. Hopefully I've understood your intention regarding...

... the conflict in default.yml: We need the pre_tasks in the play, but I'd like you to readd the vars and the newly added vars!