System Roles should consistently use ansible_managed in configuration files it manages

nhosoi commented 2 years ago

bz#2044640

Commented files by tests_mesh_cert.yml:

==> /etc/ipsec.d/mesh.conf <==
#
# Ansible managed
#
conn private
      type=tunnel
      left=%defaultroute

==> /etc/ipsec.d/policies/clear <==
#
# Ansible managed
#
198.51.100.0/24

==> /etc/ipsec.d/policies/private <==
#
# Ansible managed
#
203.0.113.0/24

==> /etc/ipsec.d/policies/private-or-clear <==
#
# Ansible managed
#
10.0.2.0/24

169.254.0.0/16

richm commented 2 years ago

[citest]

nhosoi commented 2 years ago

[citest]

I don't see CI tests' results... (I was expecting to see the failures in the collection format.) puzzled

richm commented 2 years ago

@ueno can we have comments in the /etc/ipsec.d/policies/* files?

richm commented 2 years ago

You'll need to change this:

        - name: check that secrets file contains correct information
          set_fact:
            __vpn_success: false
          when: >-
            item.content | b64decode is not
            search('^@' + __vpn_main_hostname + ' @host0' +
            (idx+1)|string + '\.local : PSK *')
          loop: '{{ __vpn_register_secrets.results }}'
          loop_control:
            index_var: idx

maybe use multiline=True like this:

        - name: check that secrets file contains correct information
          set_fact:
            __vpn_success: false
          when: >-
            item.content | b64decode is not
            search('^@' + __vpn_main_hostname + ' @host0' +
            (idx+1)|string + '\.local : PSK *', multiline=True)
          loop: '{{ __vpn_register_secrets.results }}'
          loop_control:
            index_var: idx

That way the ^ will match beginning of string or \n

richm commented 2 years ago

another point about the policies files - we are using a different method to write them - for the other files, if we use template:, we completely replace the contents every time - but for the policies files, we can only ensure that the given CIDR is in the file - we have no way to remove CIDR values from the file - it seems to me that we should replace the CIDR values every time - in which case, we can just use a simple template: with a simple policy.j2 which just adds the CIDR values using a loop - then we can have a simple {{ ansible_managed | comment }} header in the file.

nhosoi commented 2 years ago

@ueno can we have comments in the /etc/ipsec.d/policies/* files?

At least, original policy files from the provider rpm package (libreswan) have comments like this.

# cat clear.rpmnew 
# This file defines the set of network destinations for which
# communication should always be in the clear.
#
<<snip>>
#
# examples:
# 10.0.1.0/24
# 2a03:6000:1004:1::/64

nhosoi commented 2 years ago

another point about the policies files - we are using a different method to write them - for the other files, if we use template:, we completely replace the contents every time - but for the policies files, we can only ensure that the given CIDR is in the file - we have no way to remove CIDR values from the file - it seems to me that we should replace the CIDR values every time - in which case, we can just use a simple template: with a simple policy.j2 which just adds the CIDR values using a loop - then we can have a simple {{ ansible_managed | comment }} header in the file.

+1

Also, I'd think this patch would add "# Ansible managed" in the middle of the policy files if they already exist. (I'm a bit confused why the initial contents, e.g., https://github.com/linux-system-roles/vpn/pull/45#issuecomment-1030083725 are not kept in private, clear, and private-or-clear. Are they removed when the role starts???)

richm commented 2 years ago

another point about the policies files - we are using a different method to write them - for the other files, if we use template:, we completely replace the contents every time - but for the policies files, we can only ensure that the given CIDR is in the file - we have no way to remove CIDR values from the file - it seems to me that we should replace the CIDR values every time - in which case, we can just use a simple template: with a simple policy.j2 which just adds the CIDR values using a loop - then we can have a simple {{ ansible_managed | comment }} header in the file.

+1

Also, I'd think this patch would add "# Ansible managed" in the middle of the policy files if they already exist. (I'm a bit confused why the initial contents, e.g., #45 (comment) are not kept in private, clear, and private-or-clear. Are they removed when the role starts???)

Good question - I don't know - does the role somehow remove or clear those files somewhere?

nhosoi commented 2 years ago

Good question - I don't know - does the role somehow remove or clear those files somewhere?

It turned out the vpn tests generates empty policy files if __vpn_opportunistic: true is set as in tests_mesh_cert.yml. If I modify the code not to create them, one of the updated policy files look like this. (The Ansible managed comment does not appear at the top.)

$ cat private
# This file defines the set of CIDRs (network/mask-length) to which
# we MUST communicate in the clear. Otherwise traffic is blocked. This
# is enforced (and can be tweaked) by setting the negotiationshunt= and
# failureshunt= to drop.
<<snip>>
#  0.0.0.0/0  tcp  25  0
#
# WARNING: This file is managed by ANSIBLE.
# template: policy.j2
# modified on: 2022-02-03 17:40:53
# by user: nhosoi
# from ansiblemaster: localhost.localdomain
#
203.0.113.0/24

nhosoi commented 2 years ago

another point about the policies files - we are using a different method to write them - for the other files, if we use template:, we completely replace the contents every time - but for the policies files, we can only ensure that the given CIDR is in the file - we have no way to remove CIDR values from the file - it seems to me that we should replace the CIDR values every time - in which case, we can just use a simple template: with a simple policy.j2 which just adds the CIDR values using a loop - then we can have a simple {{ ansible_managed | comment }} header in the file.

I'm reading the doc [0] to understand the vpn/libreswan configuration better. (sorry, my knowledge is really limited... :p) In the example in the doc, it suggests to append a node to the policy files as follows (like the current vpn role behaves).

# echo "10.15.34.0/24" >> /etc/ipsec.d/policies/private-or-clear

I'm wondering it might depend on the use case -- append vs replace? Should it be programmable or just generating policy files from scratch is better?

[0] - https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/9-beta/html/securing_networks/configuring-a-vpn-with-ipsec_securing-networks#configuring-a-mesh-vpn_configuring-a-vpn-with-ipsec

richm commented 2 years ago

another point about the policies files - we are using a different method to write them - for the other files, if we use template:, we completely replace the contents every time - but for the policies files, we can only ensure that the given CIDR is in the file - we have no way to remove CIDR values from the file - it seems to me that we should replace the CIDR values every time - in which case, we can just use a simple template: with a simple policy.j2 which just adds the CIDR values using a loop - then we can have a simple {{ ansible_managed | comment }} header in the file.

I'm reading the doc [0] to understand the vpn/libreswan configuration better. (sorry, my knowledge is really limited... :p) In the example in the doc, it suggests to append a node to the policy files as follows (like the current vpn role behaves).
# echo "10.15.34.0/24" >> /etc/ipsec.d/policies/private-or-clear
I'm wondering it might depend on the use case -- append vs replace? Should it be programmable or just generating policy files from scratch is better?

[0] - https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/9-beta/html/securing_networks/configuring-a-vpn-with-ipsec_securing-networks#configuring-a-mesh-vpn_configuring-a-vpn-with-ipsec

I think the docs are making several assumptions:

the only way to manage the file is by using the cli
there is no other process for managing the file
the given CIDR is not already in the file
there are no overlapping CIDRs in the file, or CIDRs in the file which would conflict with the given CIDR

Since Ansible is managing the file, we should not worry about other processes managing the file (@ueno please correct me if I'm wrong on this), and the vpn role should be able to completely manage it.

There should be a template for the entire file, which has the {{ ansible_managed | comment }} header in it, and a loop which writes the give CIDRs to the file
The role should be changed to use the template: module instead of lineinfile:

nhosoi commented 2 years ago

I think the docs are making several assumptions:

the only way to manage the file is by using the cli

there is no other process for managing the file

the given CIDR is not already in the file

there are no overlapping CIDRs in the file, or CIDRs in the file which would conflict with the given CIDR

Since Ansible is managing the file, we should not worry about other processes managing the file (@ueno please correct me if I'm wrong on this), and the vpn role should be able to completey manage it.

Ack. Unless further advice is given by @ueno , I'm updating the policy file generation code as @richm described. Thanks!

richm commented 2 years ago

one question, otherwise, lgtm

nhosoi commented 2 years ago

Thank you, @richm . I was testing the same change. :p 5966d7a

Squashed and pushed.

nhosoi commented 2 years ago

lgtm

Could you please merge it? Thank you, @richm!

linux-system-roles / vpn

System Roles should consistently use ansible_managed in configuration files it manages #45