-
Notifications
You must be signed in to change notification settings - Fork 198
Conversation
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.
Looks good! I've requested some minor adjustments.
@@ -1,3 +1,4 @@ | |||
- name: restart sshd | |||
service: name={{ sshd_service_name }} state=restarted | |||
when: "(ssh_server_enabled|bool)" | |||
become: yes |
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.
Adding become: yes
to the playbook fixes this, so I'd rather not have this here, as mentioned here: #81
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.
Man, this is such a sucky bug. I don't want to run my roles as root, so I don't use become: yes
at the playbook level. It's supposed to be fixed, so that you can do an import_role
and put the become: yes
on that, but it doesn't work correctly. I think it would work, if there were no include_role
calls between the import and the calling of the handler, but my implementation uses an include
. That's why I put the become
on the handler itself. I can take it off, but it means I'm still going to have to fork to get it working for me. Not a huge deal, but not ideal.
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.
You're right. Let's keep it in for now and see if someone's got a problem with it.
tasks/main.yml
Outdated
- name: include selinux specific tasks | ||
include_tasks: selinux.yml | ||
when: sestatus.rc == 0 | ||
- include_tasks: main_2.yml |
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.
Can you call this file hardening.yml
?
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.
Np
tasks/main.yml
Outdated
include_tasks: selinux.yml | ||
when: sestatus.rc == 0 | ||
- include_tasks: main_2.yml | ||
when: ssh_hardening_enabled | default(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.
Please add the variable to the defaults.yml
and into the documentation. It should be true.
Then please remove | default(true)
from this condition.
I don't want any undocumented and undefined variables in the code.
Thanks @jcheroske ! |
@rndmh3ro You're welcome! And thank you for such an awesome set of roles! |
I looked at your mysql role, and named the flag based on that naming convention.
The big issue with what I just submitted is that the ssh restart handler doesn't work without adding
become: yes
directly to it. Ansible seems like a mass of bugs when it comes to usingnotify
with other constructs likebecome
orinclude
or whatever. If you mess around with it and figure out a way to get abecome: yes
set on theinclude_role
to propagate all the way to the handler, I would love to see that. I'm on Ansible 2.6.2 and it still doesn't work right for me. The docs discuss limitations with usinginclude_*
, but they are not as detailed as I would like.Other than that issue, which may be of no consequence to you, this works perfectly.