-
Notifications
You must be signed in to change notification settings - Fork 739
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
ssh: explicitly enable or disable the service at boot #771
Conversation
ansible.builtin.service: | ||
name: "{{ sshd_service_name }}" | ||
enabled: "{{ ssh_server_service_enabled }}" | ||
become: true |
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.
Why did you add become here?
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 copied the behavior from the handlers/main.yml
but I've just realized that the become
is already passed as an arg in tasks/main.yml
, I'll correct this part.
In the mean time, shouldn't we also check that the package is installed with
- name: Install openssh
ansible.builtin.package:
name: "{{ ssh_pkg_name }}"
state: present
for the same reasons I listed above? The role is almost complete (from a "I want to have a functioning ssh server or client" POV) and it seems quite strange to install and enable the service outside of this role. I'm curious about your thoughts on that
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.
In the past, we always assumed that ssh is installed. But since we're now already changing how ssh is started (see https://github.com/dev-sec/ansible-collection-hardening/blob/master/roles/ssh_hardening/tasks/disable-systemd-socket.yml) I don't see any reason not to install it, too.
Do you want to create an additional PR for this?
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.
Indeed, I'll create an MR for that if it's ok 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.
Yes, please. :)
Signed-off-by: Sevan Murriguian-Watrin <git@byh0ki.fr>
72057b1
to
a7fe280
Compare
Hello,
This PR aims to explicitly allow someone to enable or disable the sshd service at boot. They are 2 main reasons/use cases:
Hopefully my variable naming is not too confusing, maybe we could rename the
ssh_server_enabled
var but this would create a braking change :/S.