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

tests: check for avc denials around daemon-reload #1702

Open
wants to merge 1 commit into
base: testing-devel
Choose a base branch
from

Conversation

mike-nguyen
Copy link
Member

@dustymabe
Copy link
Member

Maybe instead of doing this we should pursue coreos/coreos-assembler#2067

@mike-nguyen
Copy link
Member Author

There's good discussion on that PR about whether it makes sense to have it run on every test since some tests may purposely invoke errors. I'm inclined to keep this for now unless there are objections.

@dustymabe
Copy link
Member

There's good discussion on that PR about whether it makes sense to have it run on every test since some tests may purposely invoke errors. I'm inclined to keep this for now unless there are objections.

I think that's fair especially since that PR hasn't merged.

dustymabe
dustymabe previously approved these changes Apr 29, 2022
@dustymabe
Copy link
Member

Note that this test essentially becomes a catchall for any SELinux issue that happens by default on boot. I think that's fine, but the test name and description are probably more narrowly focused that they should be because I suspect it will fail for other reasons a lot more than it will fail for the reason this test was written.

Can you try running this agains a local build of next-devel and rawhide to make sure it passes before we merge?

@mike-nguyen
Copy link
Member Author

Note that this test essentially becomes a catchall for any SELinux issue that happens by default on boot. I think that's fine, but the test name and description are probably more narrowly focused that they should be because I suspect it will fail for other reasons a lot more than it will fail for the reason this test was written.

Can you try running this agains a local build of next-devel and rawhide to make sure it passes before we merge?

Great point! My intention was to make sure there were no denials before the test and that systemd daemon-reload did not make cause any denials. I'll update the test description. I think I will also move the function into the common library so any test can use it if they choose to.

@cgwalters
Copy link
Member

cgwalters commented May 3, 2022

Note that this test essentially becomes a catchall for any SELinux issue that happens by default on boot. I think that's fine, but the test name and description are probably more narrowly focused that they should be because I suspect it will fail for other reasons a lot more than it will fail for the reason this test was written.

I strongly agree with this.

There's good discussion on that PR about whether it makes sense to have it run on every test since some tests may purposely invoke errors.

Yeah, but I think what we can do on the coreos-assembler side is an an opt-in that lets us ignore errors. Perhaps by logging a special journal message.

There is actually another approach - we could ship a systemd unit that trips into failure mode when a SELinux denial is detected.

The key advantage of this is: it means our existing failure detection for systemd unit Just Works - including things like the console login.

The OpenShift MCO uses systemctl --failed but doesn't know about SELinux either, etc.

We could actually discuss taking such a unit to some selinux-policy package.

I wouldn't object to merging this PR as is but the above has a lot more appeal to me.

@bgilbert
Copy link
Contributor

bgilbert commented May 4, 2022

Yeah, but I think what we can do on the coreos-assembler side is an an opt-in that lets us ignore errors. Perhaps by logging a special journal message.

We have the existing test flags mechanism for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants