Skip to content
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

SELinux config section Bugfix #312

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

gcalcaterra
Copy link

Proposed changes

Describe the use case and detail of the change. If this PR addresses an issue on GitHub, make sure to include a link to that issue using one of the supported keywords here in this description (not in the title of the PR).

Hi,
After testing this role in RHEL 8, I've realized the SELinux section wasn't working, it only needed two things to work correctly:

  • The yum package policycoreutils-python-utils so semanage module can be found
  • A "when" condition correctly set so the task that configures sets the SELinux mode can be idempotent.

Please, let me know of any improvements needed in this PR. Thanks.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • [Yes] I have read the CONTRIBUTING document
  • [] I have added Molecule tests that prove my fix is effective or that my feature works
    SELinux cannot be tested in containers and Molecule uses docker containers. Before my changes, I didn't see any already created tests for SELinux. If you think these tests are very important now, I can try to integrate Molecule with Vagrant and test there, just let me know
  • [Yes] I have checked that any relevant Molecule tests pass after adding my changes
  • [Yes] I have updated any relevant documentation (defaults/main/*.yml, README.md and CHANGELOG.md)
    There is no need for more documentations, as my changes are only a bugfix to make the SELinux section works as intended.

As how the "when" condition was before: "not (ansible_check_mode and nginx_config_selinux_enforcing)", it was giving TRUE when it shouldn't because with the "and" when one of the two items is a FALSE, the whole parenthesis becomes FALSE.
In the corrected way, it skips when the nginx_config_selinux_enforcing is in TRUE, as it should.
The package policycoreutils-python-utils is needed in RHEL8, if it is no installed the playbook prints and ERROR with ModuleNotFoundError message for the 'semanage' module.
@gcalcaterra gcalcaterra requested a review from alessfg as a code owner February 24, 2023 20:46
@alessfg alessfg added the bug Something isn't working label Feb 28, 2023
@alessfg alessfg added this to the 0.6.0 milestone Feb 28, 2023
@alessfg
Copy link
Collaborator

alessfg commented Feb 28, 2023

I have a feeling we might be able to get rid of those conditionals altogether? I am looking at how the "core" NGINX role handles SELinux (https://github.com/nginxinc/ansible-role-nginx/blob/main/tasks/prerequisites/setup-selinux.yml#L2-L13) and by all accounts it is true that we need to set SELinux to permissive no matter what to run any of the other SELinux tasks. The key place where nginx_config_selinux_enforcing makes sense is when at the end we are deciding whether we need to set SELinux back to enforcing.

A different question altogether too, do you know if we need policycoreutils if we are installing policycoreutils-python-utils? And do you know if the latter package is available through RHEL 7 to 9? You opening this PR made me realize that the Molecule tests always skip the SELinux step since it's not installed by default on any of the relevant images (I didn't implement SELinux and blindly assumed it was installed by default -- goes to show my lack of hands on experience with SELinux). In turn this also means that this might be a good opportunity to do some work on the Molecule pipeline and ensure the relevant images have SELinux installed before running tests. It should hopefully only require a couple changes to the Molecule Dockerfile (https://github.com/nginxinc/ansible-role-nginx-config/blob/main/molecule/common/Dockerfile.j2).

@gcalcaterra
Copy link
Author

Hi @alessfg, thanks for taking a look at my input. Some more info and questions below.

... and by all accounts it is true that we need to set SELinux to permissive no matter what to run any of the other SELinux tasks.

Is this an affirmation? Why SELinux needs to be set to permissive first to run the SELinux tasks? Shouldn't it just be enabled (permissive or enforcing mode)?

A different question altogether too, do you know if we need policycoreutils if we are installing policycoreutils-python-utils?

The yum package policycoreutils is needed, it is a policycoreutils-python-utils dependency though, so we could not set it explicitly and it will be installed still with policycoreutils-python-utils. I tested it in a RHEL 8.7 with this command [0] (yum-utils need to be installed first):
repoquery --requires --recursive --resolve policycoreutils-python-utils | grep -i policycoreutils-

And do you know if the latter package is available through RHEL 7 to 9?

I'm taking this as homework so I can answer you better latter.

You opening this PR made me realize that the Molecule tests always skip the SELinux step since it's not installed by default on any of the relevant images (I didn't implement SELinux and blindly assumed it was installed by default -- goes to show my lack of hands on experience with SELinux). In turn this also means that this might be a good opportunity to do some work on the Molecule pipeline and ensure the relevant images have SELinux installed before running tests. It should hopefully only require a couple changes to the Molecule Dockerfile (https://github.com/nginxinc/ansible-role-nginx-config/blob/main/molecule/common/Dockerfile.j2).

I don't think SELinux can be tested in containers. Take a look a this [1]. Am I wrong here? That's why I suggest you if the need to test SELinux tasks to use the Vagrant plugin for Molecule [2] as an option, as vagrant uses VMs.

[0] Is there any way to retrieve a dependency tree from yum?
[1] How to enable SELinux inside of a CentOS Docker container?
[2] https://github.com/ansible-community/molecule-vagrant

Let me know your thoughts, if you may.

@alessfg alessfg modified the milestones: 0.6.0, 0.6.1 Mar 15, 2023
@alessfg
Copy link
Collaborator

alessfg commented Apr 10, 2023

Hey sorry it took me so long get back to this PR. I've been terribly busy these past couple months.

Re your points:

Is this an affirmation? Why SELinux needs to be set to permissive first to run the SELinux tasks? Shouldn't it just be enabled (permissive or enforcing mode)?

As far as I understand, SELinux needs to be set to permissive to make changes to its configuration? I might be wrong on this, but if I am, we should open bug fix for https://github.com/nginxinc/ansible-role-nginx/blob/main/tasks/prerequisites/setup-selinux.yml#L10-L13.

I'm taking this as homework so I can answer you better latter.

Did you have a chance to look into this?

I don't think SELinux can be tested in containers. Take a look a this [1]. Am I wrong here? That's why I suggest you if the need to test SELinux tasks to use the Vagrant plugin for Molecule [2] as an option, as vagrant uses VMs.

You are totally right. We could potentially create a Vagrant based scenario to test SELinux, but that might take a little bit more time/effort to implement, so it probably belongs on a separate PR (if not in a separate role altogether, the core Ansible NGINX role).

@gcalcaterra
Copy link
Author

gcalcaterra commented Apr 17, 2023

Hi @alessfg

Is this an affirmation? Why SELinux needs to be set to permissive first to run the SELinux tasks? Shouldn't it just be enabled (permissive or enforcing mode)?

As far as I understand, SELinux needs to be set to permissive to make changes to its configuration? I might be wrong on this, but if I am, we should open bug fix for https://github.com/nginxinc/ansible-role-nginx/blob/main/tasks/prerequisites/setup-selinux.yml#L10-L13.

From what I understand the SELinux needs to only be in permissive or enforcing mode, so I believe yes, some changes must be made to the ansible-role-nginx project.

There is a point to consider though (maybe it could be an improvement), if SELinux is already in enforcing or permissive, there's no problem, if it is disabled and should be changed to permissive or enforcing, there is a good practice that is to reboot and relabel files as explained in here 2.4. Enabling SELinux on systems that previously had it disabled.

Did you have a chance to look into this?

Yes. The package policycoreutils-python-utils is available in RHEL 8 and RHEL 9. In RHEL 7 there is no exactly that package, there is a policycoreutils-python, that might do the trick (haven't had the chance to test it). So there may be some updates to do if you want to make it work for those 3 major versions of RHEL.

You are totally right. We could potentially create a Vagrant based scenario to test SELinux, but that might take a little bit more time/effort to implement, so it probably belongs on a separate PR (if not in a separate role altogether, the core Ansible NGINX role).

Agree about the more time/effort.

Let me know how would you like to continue this. I may be able to help a little more.
Regards.

@alessfg
Copy link
Collaborator

alessfg commented Apr 17, 2023

Ok thanks for the info. Based on what you are saying I think it'd be great if you could:
a) Update the PR to ensure RHEL 7-9 works as intended
b) Merge the tasks setting SELinux to permissive/enforcing. We only need one (and we should let the user decide to which mode they want to set SELinux).
c) Open a PR in https://github.com/nginxinc/ansible-role-nginx with a similar fix to what I discussed in b)

Re rebooting/relabeling SELinux, I think that level of interaction with the target host is out of the scope of this role. Users running the role can then reboot the system at their own discretion if necessary. At most, I would add a note in the docs.

Copy link

github-actions bot commented Nov 7, 2024

✅ All required contributors have signed the F5 CLA for this PR. Thank you!
Posted by the CLA Assistant Lite bot.

@gcalcaterra
Copy link
Author

I have hereby read the F5 CLA and agree to its terms

@alessfg alessfg force-pushed the main branch 3 times, most recently from c32c384 to 7a27221 Compare November 30, 2024 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants