Add keepalived_global_defs configuration directive #56
Conversation
templates/keepalived.conf.j2
Outdated
| @@ -13,6 +13,10 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
|
|
|||
| global_defs { | |||
| enable_script_security | |||
| } | |||
There was a problem hiding this comment.
I think this should not be part of the template by default, instead we should a loop over any global_defs, and write their value.
so we should probably have something like:
{% if keepalived_global_defs is defined %}
{% for def in keepalived_global_defs.items() %}
{{ def }}
{% endfor %}
{% endif %}
This we can pass a variable, whose content would be:
keepalived_global_defs:
- enable_script_security
This way global notification scripts can work too.
|
That makes sense. I'll rework it. |
|
The change seem ok, but it failed on my ci, I will do manual testing to ensure everything is fine there. |
defaults/main.yml
Outdated
| # Any strings provided here will appear in the `global_defs` secction of the | ||
| # keepalived configuration file. | ||
| keepalived_global_defs: | ||
| - enable_script_security |
There was a problem hiding this comment.
if we have global_defs as a list, the
keepalived_global_defs.items()
doesn't make sense. (See comment below)
There was a problem hiding this comment.
I don't think we should have it by default. Instead we should pass it to the role if someone wants to use it. It also doesn't break upgrades if make it optional.
templates/keepalived.conf.j2
Outdated
| @@ -13,6 +13,14 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
|
|
|||
| {% if keepalived_global_defs is defined %} | |||
| global_defs { | |||
| {% for def in keepalived_global_defs.items() %} | |||
There was a problem hiding this comment.
if keepalived_global_defs is a list, we should just iterate over list elements:
{% for def in keepalived_global_defs %}
defaults/main.yml
Outdated
| # Any strings provided here will appear in the `global_defs` secction of the | ||
| # keepalived configuration file. | ||
| keepalived_global_defs: | ||
| - enable_script_security |
There was a problem hiding this comment.
I don't think we should have it by default. Instead we should pass it to the role if someone wants to use it. It also doesn't break upgrades if make it optional.
When `enable_script_security` is not present in keepalived.conf,
the daemon will print a warning in the journal:
SECURITY VIOLATION - scripts are being executed but
script_security not enabled.
This patch enables script security which ensures that a non-root
user cannot maliciously alter a script that keepalived may be running
as root.
This fixes Launchpad bug 1742487:
https://bugs.launchpad.net/openstack-ansible/+bug/1742487
|
@evrardjp The default is now empty. Hopefully this is okay. |
When defined in the defaults, the keepalived_global_defs is defined would always be true.
|
I've added a patch onto your branch, to make it really optional. I am now testing manually. |
evrardjp
changed the title
When
enable_script_securityis not present in keepalived.conf,the daemon will print a warning in the journal:
This patch enables script security which ensures that a non-root
user cannot maliciously alter a script that keepalived may be running
as root.
This fixes Launchpad bug 1742487:
https://bugs.launchpad.net/openstack-ansible/+bug/1742487