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.

Sign up for GitHub

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

Jump to bottom

Only check SELinux states if "getenforce" command exists #3353

Merged
merged 1 commit into from
May 12, 2020
Merged

Only check SELinux states if "getenforce" command exists #3353

merged 1 commit into from
May 12, 2020

Conversation

MichaIng
Copy link
Contributor

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes, and have included unit tests where possible.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)

What does this PR aim to accomplish and how does this PR accomplish the above?:

  • Currently, if the SELinux config file exists, installed SELinux is assumed.
  • But removing e.g. an APT package via "apt-get remove" leaves config files in place, or they could be present for other reasons.
  • If the getenforce command is not present but the config file is, currently the installer exists without error message when calling getenforce due to "set -e".
  • With this change, the presence of getenforce command is checked first. If it is not present, selinux-utils is not installed, which is a core part of SELinux, pulled in by selinux-basics as well. So it can be assumed that no SELinux is active if this command is missing.

Related forum error report: https://discourse.pi-hole.net/t/upgrade-to-5-0-failing/32068

@MichaIng
Only check SELinux states if "getenforce" command exists
7d79cf5 
- Currently, if the SELinux config file exists, installed SELinux is assumed.
- But removing e.g. an APT package via "apt-get remove" leaves config files in place, or they could be present for other reasons.
- If the getenforce command is not present but the config file is, currently the installer exists without error message when calling getenforce due to "set -e".
- With this change, the presence of getenforce command is checked first. If it is not present, selinux-utils is not installed, which is a core part of SELinux, pulled in by selinux-basics as well. So it can be assumed that no SELinux is active if this command is missing.

Signed-off-by: MichaIng <micha@dietpi.com>
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/upgrade-to-5-0-failing/32068/47

PromoFaux
PromoFaux approved these changes May 12, 2020
View reviewed changes
Copy link
Member

@PromoFaux PromoFaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Raspbian with selinux-utils installed / uninstalled / and with just the config. Works as expected.

I think this is a sensible workaround whilst we decide internally whether or not we are going to continue checking selinux relating stuff at all

@PromoFaux PromoFaux requested a review from bcambl May 12, 2020 22:51
@PromoFaux
Copy link
Member

@bcambl Can I get a second pair of eyes on this please. It seems to be to be a sensible workaround for the meantime.

Yes there shouldn't be any situation where the config file exists and getenforce does not, but maybe there is. I can see the logic in at least skipping over this section of the code if getenforce does not exist. On deb based repos anyway, you're more adept with the dark side than I

@bcambl
Copy link
Member

bcambl commented May 12, 2020

should be harmless. The debug log will provide context for future issues as it checks both individually.

@PromoFaux PromoFaux merged commit d524f9a into pi-hole:development May 12, 2020
@MichaIng MichaIng deleted the patch-1 branch May 13, 2020 10:54
@MichaIng MichaIng mentioned this pull request May 21, 2020
Closed
8 tasks
@PromoFaux PromoFaux mentioned this pull request Jul 5, 2020
Merged
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-5-1-released/35577/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PromoFaux PromoFaux PromoFaux approved these changes

@bcambl bcambl bcambl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MichaIng @pralor-bot @PromoFaux @bcambl