-
-
Notifications
You must be signed in to change notification settings - Fork 608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use more secure sshd defaults #716
Conversation
* Depreciate the sshd role (#561) * Use more secure defaults for client and server ssh connections
Switch roles for more secure defaults in ssh
Formerly printed Hosts on successive lines, applying all options to the final Host only.
Only one Port will be used, so don't loop over a list of Ports.
Tested all these new additions using Fedora 25 as my desktop OS, working with no problems. As for the docs it might not be a bad idea to reference using |
Why don't we just bump Ansible requirements along with this PR? |
# =================== | ||
|
||
# Address family should always be limited to the active network configuration. | ||
AddressFamily {{ 'any' if network_ipv6_enable else 'inet' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a bunch of these in the templates. Can they be converted to use ternary
? I think it reads better:
AddressFamily {{ network_ipv6_enable | ternary('any', 'inet') }}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I prefer ternary
as well (for consistency with the rest of Trellis), I refrained from implementing Trellis coding conventions, trying leave the original dev-sec/ansible-ssh-hardening
role as intact as possible for simpler comparison with upstream.
If we choose to apply Trellis conventions, we could consider the following:
- use
ternary
filter in place of pythonic if/else, as you suggested - break list var values out into multiline yaml, e.g., instead of
var: ['val', 'val2']
- add space around var names, e.g.,
{{port}}
becomes{{ port }}
- align the various
# ssh
and# sshd
comments indefaults/main.yml
- edit code comments for succinctness, grammar, capitalization consistency
- either parameterize or remove the
IdentityFile
entries inopenssh.conf.j2
template - maybe simplify a handful of templating chunks using this pattern:
-{% if ssh_allow_users -%}
-AllowUsers {{ssh_allow_users}}
-{% endif %}
+AllowUsers {{ ssh_allow_users | default('')}}
I believe it's ok -- and has the same effect -- to have empty parameters, e.g., to fall back to default of empty string. I could test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes to all of those unfortunately :) the code is in Trellis so it should conform to our standards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind helping with one of the files so you don't have to do it all, I can just make a gist and then you can just copy paste and rebase in what helps clean it up. Let me know if you want to split it up a little.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://gist.github.com/RiFi2k/41f3874f9747c9a5e611b105e373100a
Cleaned up and edited text on ssh-hardening/defaults/main.yml hopefully it saves a little time for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated all the formatting 🎉
AddressFamily {{ 'any' if network_ipv6_enable else 'inet' }} | ||
|
||
{% for host in ssh_remote_hosts -%} | ||
{% if loop.first %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #744 |
A discourse thread prompted #561, a call to create the more secure sshd defaults. @RiFi2k implemented these improvements in #702 (based on dev-sec/ansible-ssh-hardening and stribika), adding them to the
ssh-hardening
branch. After a few minor additions to that branch, this PR is the final step in replacing the former Trellissshd
role with the newssh-hardening
role. It works fine in my manual testing on macOS and Win 10 (with cmder).Impact on users: Host key change
After provisioning with this revised role, servers' host keys will change from ECDSA to ED25519. The next ssh connection attempt after provisioning will fail with the warning that the host key has changed, so users will have to
ssh-keygen -R 12.34.56.78
.Latest Ansible versions give a clear warning about the host key change, but older Ansible versions do not (unless
-vvvv
is added), so some users may be confused why the ssh connection fails when it worked previously.(Edit: This paragraph is not applicable to the alternative proposed in #744 where the
HostKeyAlgorithms
option inansible.cfg
leads the server to offer the correct host key on first connection.) Note that even a fresh server will face this issue because the first connection for provisioning will use the ECDSA key then the next connection will use the ED25519 key. This is an inconvenience, the common price for better security.Exact changes
On the remote server,
sshd -T
outputs all the sshd server configs andssh -G example.com
outputs all the ssh client configs. Based on that output, here is a diff of configs: current Trellis vs after this PR.Docs
I think the only absolutely required docs change is in the remote server setup docs: switch the variable name from
sshd_permit_root_login
tossh_allow_root_with_key
. However, we might consider adding a helpful note about that expected host key change described above/cc @aried3r @isynergy-development