WIP - DO NOT MERGE - Allow users to ignore errors when removing built-in policy

[richm]

If you attempt to remove built-in policy, you will get an error like this:

Port tcp/NNNN is defined in policy, cannot be deleted

If you want to have the role ignore errors like this, use
selinux_ignore_builtin_removal: true

[richm]

This will help us greatly simplify the code in the logging role (and probably several other roles where we want to use the selinux role) see https://github.com/linux-system-roles/logging/pull/292/files#diff-86218b7c9c831a3bdd5513a24d268f2701a9d814094c4dd9b6219e58d8a04d92R303

[richm]

[citest]

[richm]

[citest]

[nhosoi]

lgtm

I verified that we could get rid of this extra treatment for the selinux custom ports by applying this pr.
https://github.com/linux-system-roles/logging/pull/292/files#diff-d3f39b8ac347ef1e1b1de226be985fa4c3e9350f7d86bee8711513c587dedefa
Thanks, @richm.

[richm]

[citest pending]

[nhosoi]

Are the failures like this https://dl.fedoraproject.org/pub/alt/linuxsystemroles/logs/lsr-citool_selinux-122-26a848d_RHEL-9.1.0-20220824.0_20220825-025015/artifacts/tests_port-FAILED.log will be solved when this pr gluetool-modules mr/187 is moved to the production?

[bachradsusi]

Could you please explain what exactly does logging role when it tries to remove default mapping?

Have you considered locales?

[root@fedora selinux]# export LANG=ar.utf8
[root@fedora selinux]# semanage port -d -t ssh_port_t -p tcp 22
ValueError: المنفذ tcp/22 معرف في السياسة، لا يمكن حذفه

[richm]

Are the failures like this https://dl.fedoraproject.org/pub/alt/linuxsystemroles/logs/lsr-citool_selinux-122-26a848d_RHEL-9.1.0-20220824.0_20220825-025015/artifacts/tests_port-FAILED.log will be solved when this pr gluetool-modules mr/187 is moved to the production?

Yes. This is because we don't have support for collection-requirements.yml

[richm]

Could you please explain what exactly does logging role when it tries to remove default mapping?

@nhosoi can you provide some explanation/examples?

Have you considered locales?

[root@fedora selinux]# export LANG=ar.utf8
[root@fedora selinux]# semanage port -d -t ssh_port_t -p tcp 22
ValueError: المنفذ tcp/22 معرف في السياسة، لا يمكن حذفه

Ok - so my approach won't work. I don't suppose there is a unique numeric exit code in this situation . . . if not, then another way is - the role creates a list of local changes, and if the policy passed to seport is not a local change, it will be skipped if the flag is set.

[richm]

@bachradsusi The problem is that the selinux role allows you to do { ports: '20514', proto: 'tcp', setype: 'syslogd_port_t', state: 'present' } - I guess the seport module is either doing the equivalent of semanage port -a -t syslogd_port_t -p tcp 20514 and ignoring the error ValueError: Port tcp/20514 already defined, or it uses some other implementation using the selinux python libraries. But if you try to do the converse operation { ports: '20514', proto: 'tcp', setype: 'syslogd_port_t', state: 'absent' } the role gives an error. Either the role should give an error in both cases, or in neither case. And - the role should not expect the user of the role to be able to determine what is the built-in policy on all of rhel7, rhel8, rhel9, and fedora, and avoid doing state: 'absent' on built-in policy.

For example - I am setting up logging across rhel7, rhel8, rhel9, and fedora systems. All I know is that the syslog listener port must be allowed in policy with syslogd_port_t - so when I'm installing, I want to ensure that the selected port is allowed by policy. So I use the selinux role - it doesn't give me an error that "hey, this policy is already set in the base policy", it just silently ignores it. Some time later, I want to disable the syslog listener port, so I want to disallow it in the policy, the converse of the operation I used to allow it, by using { ports: '20514', proto: 'tcp', setype: 'syslogd_port_t', state: 'absent' } - only this time it gives me an error.

[nhosoi]

@bachradsusi The problem is that the selinux role allows you to do { ports: '20514', proto: 'tcp', setype: 'syslogd_port_t', state: 'present' } - I guess the seport module is either doing the equivalent of semanage port -a -t syslogd_port_t -p tcp 20514 and ignoring the error ValueError: Port tcp/20514 already defined, or it uses some other implementation using the selinux python libraries. But if you try to do the converse operation { ports: '20514', proto: 'tcp', setype: 'syslogd_port_t', state: 'absent' } the role gives an error. Either the role should give an error in both cases, or in neither case. And - the role should not expect the user of the role to be able to determine what is the built-in policy on all of rhel7, rhel8, rhel9, and fedora, and avoid doing state: 'absent' on built-in policy.

For example - I am setting up logging across rhel7, rhel8, rhel9, and fedora systems. All I know is that the syslog listener port must be allowed in policy with syslogd_port_t - so when I'm installing, I want to ensure that the selected port is allowed by policy. So I use the selinux role - it doesn't give me an error that "hey, this policy is already set in the base policy", it just silently ignores it. Some time later, I want to disable the syslog listener port, so I want to disallow it in the policy, the converse of the operation I used to allow it, by using { ports: '20514', proto: 'tcp', setype: 'syslogd_port_t', state: 'absent' } - only this time it gives me an error.

This is exactly what I ran into while I was working on linux-system-roles/logging#292 (and still am working on...) To workaround the failure with state: absent, I had to retrieve the customized ports and only when the port to be deleted is in the customized port set, I passed the port to the selinux role to delete. It turned out it's not just an issue for the logging role, but it's shared among many roles. Thus, instead of handling it in each role, it'd be ideal to do so in the selinux role.

[bachradsusi]

seport module checks whether there's existing mapping to the same type, port and protocol and if it's already defined it skips it - https://github.com/ansible-collections/community.general/blob/8e59e5252506aeeccb6ca5cfe38662df2f66fb23/plugins/modules/system/seport.py#L201

If the role didn't use seport it could do steps similar to the following steps in shell:

• add/modify syslogd_port_t mapping

# semanage port -a -p tcp -t syslogd_port_t 20514 || semanage port -m -p tcp -t syslogd_port_t 20514
ValueError: Port tcp/20514 already defined
# semanage port -l -C
SELinux Port Type              Proto    Port Number
syslogd_port_t                 tcp      20514

• remove new port mapping

# semanage port -d -p tcp -t syslogd_port_t 20514
# semanage port -l -C
#

The question is whether we want to update selinux role to stop using seport module or whether logging role should implement this on its own and don't use selinux role just for port mappings.

[bachradsusi]

I'd say that the behavior of seport is correct. If it didn't skip already existing mappings and added local modifications then next run with 'absent' would remove just the local modification and left the policy default without warning users that the mapping still exists.

[spetrosi]

lgtm

[richm]

I'd say that the behavior of seport is correct. If it didn't skip already existing mappings and added local modifications then next run with 'absent' would remove just the local modification and left the policy default without warning users that the mapping still exists.

@bachradsusi Here is the problem:
As a sysadmin, I want to deploy logging with a syslogd listener. I want to ensure that port used has the correct syslogd_port_t label. Sometime later, I want to remove the syslogd listener, and ensure that whatever I did to the system for the syslogd_port_t label is undone.

Note that you can replace "deploy logging" with "deploy X" where "X" is one of our 20 system roles that may want to use the selinux role to configure selinux policy for ports, and you can replace "syslogd_port_t" with "XXX_port_t".
We don't want to have code duplicated in every system role to do

if policy XXX_port_t is a local modification
  remove it
else
  do not remove it

I think this should be done in the selinux role.

In addition - I would strongly prefer not to break the existing role API

• cannot make the role use semantics of semanage port -m instead of semanage port -a
• cannot make the role raise an error if adding a port that is already built-in - that is, cannot make the role use the semantics of semanage port -a if policy is built-in

That's why I'm proposing a parameter that tells the selinux role "I'm removing the label for this port, but it may be a built-in policy, so please ignore it"

[nhosoi]

Note that you can replace "deploy logging" with "deploy X" where "X" is one of our 20 system roles that may want to use the selinux role to configure selinux policy for ports, and you can replace "syslogd_port_t" with "XXX_port_t".

Starting with the logging role, I continue working on the project to use the selinux role in the roles belonging to the linux-system-roles. So far, ha_cluster and metrics share the same issue. I'm pretty sure the rest of the roles would follow.

[richm]

This implementation works by first getting the list of local policy modifications in the usual output form:

http_cache_port_t              tcp      8080, 8118, 8123, 10001-10010
http_cache_port_t              udp      3130
http_port_t                    tcp      80, 81, 443, 488, 8008, 8009, 8443, 9000
i18n_input_port_t              tcp      9010

(NOTE: The above are built-in policy, just for illustration purposes)
Then, the code converts these into the same list of dict form used by selinux_ports. It is a little bit complicated because

• if proto is omitted, the default is tcp, so we have to have an entry with both proto: tcp and without
• if ports is a single value (as opposed to a range like 10001-10010) it can be given as a string '8080' or int 8080, so we have to have an entry with string and another with int

For example, the i18n_input_port_t tcp 9010 will be converted into these 4 entries:

- setype: i18n_input_port_t
  state: absent
  proto: tcp
  ports: 9010
- setype: i18n_input_port_t
  state: absent
  proto: tcp
  ports: "9010"
- setype: i18n_input_port_t
  state: absent
  ports: 9010
- setype: i18n_input_port_t
  state: absent
  ports: "9010"

this is so that we can match any of the 4 possible ways the user might specify to remove this port.

Then, if selinux_ignore_builtin_removal is true, we loop through the selinux_ports values - if the item is adding a label/port, we allow the item. Otherwise, we only allow the item if it is in the local modifications list.

This implementation is quite complex, but it has the benefit of not relying on the l10n output of seport.

I'm open to suggestions about how to otherwise implement this.

[richm]

[citest]

[richm]

[citest]

[nhosoi]

I guess it's too early to start reviewing(?); FYI, with the current pr, this including the selinux role in the logging role works nicely. :)

    # selinux role
    - name: Manage selinux for specified ports
      include_role:
        name: fedora.linux_system_roles.selinux
      vars:
        selinux_ports: "{{ logging_selinux_ports }}"
        selinux_ignore_builtin_removal: true
      when: logging_selinux_ports | length > 0

[richm]

I guess it's too early to start reviewing(?);

If you find a problem in my code, please comment

FYI, with the current pr, this including the selinux role in the logging role works nicely. :)

Good to know - thanks!

    # selinux role
    - name: Manage selinux for specified ports
      include_role:
        name: fedora.linux_system_roles.selinux
      vars:
        selinux_ports: "{{ logging_selinux_ports }}"
        selinux_ignore_builtin_removal: true
      when: logging_selinux_ports | length > 0

[richm]

[citest]

[richm]

[citest]

[richm]

[citest]

[richm]

[citest]

[richm]

[citest]

[richm]

Another option - have a state default - e.g. if you used

      include_role:
        name: linux-system-roles.selinux
      vars:
        selinux_ports:
          - { ports: 2049, setype: 'nfs_port_t', state: 'default' }

then the role would do the following:

• if this is local policy, remove the policy (like state: absent)
• if this is built-in policy, then do nothing (like state: absent with selinux_ignore_delete_builtin)

Not sure if default is correct - perhaps reset or revert or ???

[richm]

The rhel-6/ansible 2.9 failure is due to reboot taking too long - I guess it is trying to relabel the filesystem during boot? The timeout is 300 seconds, and the previous reboot takes 245 seconds - I'm guessing 300 seconds is just on the edge of the time, and sometimes it takes a little longer than that. I guess we'll need a PR to change that to 360 or 420?

[nhosoi]

Another option - have a state default - e.g. if you used

      include_role:
        name: linux-system-roles.selinux
      vars:
        selinux_ports:
          - { ports: 2049, setype: 'nfs_port_t', state: 'default' }

then the role would do the following:

* if this is local policy, remove the policy (like `state: absent`)

* if this is built-in policy, then do nothing (like `state: absent` with `selinux_ignore_delete_builtin`)

Not sure if default is correct - perhaps reset or revert or ???

I like the idea. I'd think it's more secure compared to ignoring the specific error.

And +1 to reset.

[bachradsusi]

I'd say that the behavior of seport is correct. If it didn't skip already existing mappings and added local modifications then next run with 'absent' would remove just the local modification and left the policy default without warning users that the mapping still exists.

@bachradsusi Here is the problem: As a sysadmin, I want to deploy logging with a syslogd listener. I want to ensure that port used has the correct syslogd_port_t label. Sometime later, I want to remove the syslogd listener, and ensure that whatever I did to the system for the syslogd_port_t label is undone.

Note that you can replace "deploy logging" with "deploy X" where "X" is one of our 20 system roles that may want to use the selinux role to configure selinux policy for ports, and you can replace "syslogd_port_t" with "XXX_port_t". We don't want to have code duplicated in every system role to do

if policy XXX_port_t is a local modification
  remove it
else
  do not remove it

I think this should be done in the selinux role.

In addition - I would strongly prefer not to break the existing role API

* cannot make the role use semantics of `semanage port -m` instead of `semanage port -a`

At the moment selinux role does not use semanage for managing ports, it uses seport ansible module.

* cannot make the role raise an error if adding a port that is already built-in - that is, cannot make the role use the semantics of `semanage port -a` if policy is built-in

In shell semanage port -a ... || semanage -m ... would exit with status 0.

That's why I'm proposing a parameter that tells the selinux role "I'm removing the label for this port, but it may be a built-in policy, so please ignore it"

What about other way around:

A calling role knows that it could want to remove the mapping it requests, so it say that it wants add new port mappings (instead of just checking that the mapping is present), e.g. state: 'added' and selinux role would use semanage port -a ... || semanage -m ... for this state. And when the calling role wants to remove it, it uses the same state: 'absent'.

I think that the code for this would be much simpler than the analysis of local modification in the current proposal.

[richm]

At the moment selinux role does not use semanage for managing ports, it uses seport ansible module.

Yes, I know. I have looked at the module source code too. What I'm saying is that we cannot change the behavior, the semantics, of the selinux role - that would break the role API.

In shell semanage port -a ... || semanage -m ... would exit with status 0.

. . . and would break the role API if we changed the role to use this.

A calling role knows that it could want to remove the mapping it requests, so it say that it wants add new port mappings (instead of just checking that the mapping is present), e.g. state: 'added' and selinux role would use semanage port -a ... || semanage -m ... for this state. And when the calling role wants to remove it, it uses the same state: 'absent'.

I think that the code for this would be much simpler than the analysis of local modification in the current proposal.

Yes. Use cases:

• User needs to ensure some ports are allowed now, and reset in the future, and doesn't know if the ports are built-in - use state: added and state: absent
• User knows the difference between built-in ports and local ports - user knows to only use state: present and state: absent for non-builtin ports
• User is managing selinux policy for all applications for entire machine - user can use selinux_ports_purge: true to remove all local modifications without having to know which ports are built-in or not

Ok - I'll work on a PoC implementation

[bachradsusi]

There's still logical problem with absent for builtin ports - after added and absent the port will be still present so users would really have know what this mean and how to use it correctly.

How hard is the requirement to use selinux role for use case 1. and 2.? For me it feels like the role is not the proper tool for incremental changing of SELinux policy even though it somehow supports it. And for other roles it would not be harder to use directly seport module as it uses same parameters - https://github.com/linux-system-roles/selinux/blob/master/tasks/main.yml#L90

[richm]

There's still logical problem with absent for builtin ports - after added and absent the port will be still present so users would really have know what this mean and how to use it correctly.

Right. Initially, it would when using the selinux role from other system roles, so we would definitely know how to use it correctly. Essentially, we would use state: added instead of state: present everywhere, because there is no way to know, in general, what the built-in policy is, and we don't want to hardcode that knowledge inside every other system role.

How hard is the requirement to use selinux role for use case 1. and 2.? For me it feels like the role is not the proper tool for incremental changing of SELinux policy even though it somehow supports it. And for other roles it would not be harder to use directly seport module as it uses same parameters - https://github.com/linux-system-roles/selinux/blob/master/tasks/main.yml#L90

• This means we have to have a dependency on the unsupported collection community.general in every role, and have to vendor it into every role for downstream purposes
• The role manages the system dependencies for you - if other roles wanted to use the seport module directly, we would have to duplicate and maintain that system dependency management code in every other role
• This still doesn't solve the underlying problem which is that the other role maintainer doesn't know if a port is part of the built-in policy or not (and the seport module does not support state: added and it would be difficult to add that and get it published)

[richm]

Another issue with semanage port -a || semanage port -m:

include_role:
  name: selinux
vars:
  selinux_ports:
    - setype: built_in_port_t
      ports: 1234
      state: added

ok

include_role:
  name: selinux
vars:
  selinux_ports:
    - setype: built_in_port_t
      ports: 1234
      state: absent

ok

include_role:
  name: selinux
vars:
  selinux_ports:
    - setype: built_in_port_t
      ports: 1234
      state: absent

ERROR: ValueError: Port tcp/1234 is defined in policy, cannot be deleted

This is another reason why the selinux_ignore_delete_builtin is good - user doesn't have to care that they already "removed", "reset", (however you want to call it) the port

[bachradsusi]

Proposal: add local parameter, POC - bachradsusi@7e8dd1f

When local: True the role would use local_seport.py script which is based on seport.py module, but it enforces changes to be added when state: present and looks only for local changes when state: absent - bachradsusi@7e8dd1f#diff-93054da44a136a29ed422e7f14005273367f9ce1ae970885a3534a09393179b3R47

$ sudo python3 ../files/local_seport.py 22 tcp ssh_port_t present 
$ sudo semanage port -l -C                                       
SELinux Port Type              Proto    Port Number

ssh_port_t                     tcp      22
$ sudo python3 ../files/local_seport.py 22 tcp ssh_port_t absent 
$ sudo semanage port -l -C                                      
$ sudo python3 ../files/local_seport.py 22 tcp ssh_port_t absent
$ sudo semanage port -l -C                                      

Also bachradsusi@7e8dd1f#diff-2724eed26cf6ff821e08cad7c68bd4a3d9c274ddd7153cc1971ea1d446e26b3eR56

local: false - default - doesn't change the behavior of the role

If this is acceptable solution, the seport.py module could be updated to accept local and local_seport.py could be dropped from this role.

[bachradsusi]

The same concept but as a local module instead of script - master...bachradsusi:linux-system-roles-selinux:seport_local

The difference between ansible collection seport and local seport is available at ansible-collections/community.general@main...bachradsusi:ansible-collections-community.general:seport-local

[bachradsusi]

ansible collection seport PR - ansible-collections/community.general#5203

[richm]

The same concept but as a local module instead of script - master...bachradsusi:linux-system-roles-selinux:seport_local

This works for me - please submit a PR for this, and I will either drop or amend my PR

[richm]

closing in favor of #124