Make --disable-selinux work on hosts that have selinux=0
#340
Conversation
ckyrouac
force-pushed
the
selinux-fix
branch
from
0f223be to
3f72760
Compare
There was a problem hiding this comment.
For testing coverage...a bit tricky right now as we'd need to boot a system with your case of selinux=0 and run an install from it. We have a legacy here of using the coreos "kola" testing framework, and I am hoping that we can eventually align with what's happening in
https://gitlab.com/bootc-org/tests
I'm OK without an integration test here for this for now, it's not like we're likely to regress on normal pull requests.
lib/src/lsm.rs
Outdated
| let path = "/proc/1/root/sys/fs/selinux/enforce"; | ||
| if Path::new(path).exists() { | ||
| let enabled = std::fs::read_to_string(path)?; | ||
| return Ok(enabled.eq("1")); |
There was a problem hiding this comment.
It's a bit more idiomatic to do enabled == "1"
However...I think we actually should work even if enforce == 0. So this check can probably just boil down to
Path::new("/proc/1/root/sys/fs/selinux/enforce").try_exists().map_err(Into::into)
or so right?
There was a problem hiding this comment.
However...I think we actually should work even if enforce == 0
To elaborate slightly, this is the current case in Anaconda - it boots with enforcing=0. Our existing install logic should work just fine in this case.
It's really the case of selinux=0 (fully disabled at runtime) that we need a fallback behavior.
There was a problem hiding this comment.
ah that makes sense, if the labels exist then bootc should work. For some reason that is not the case right now (i.e. the labels are broken on the image when installing from a host with selinux set to permissive), I guess that is what @bcrochet is working on?
I fixed the first commit with your suggestions. I also added a second commit to prioritize the --disable-selinux flag. It seems wrong to silently ignore the flag if the host has selinux enabled. I'm probably missing something though. Flipping those if statements makes install to-disk --disable-selinuc work from a host with selinux set to permissive though.
ckyrouac
force-pushed
the
selinux-fix
branch
from
3f72760 to
5708cfb
Compare
|
Looks generally good, just two nits:
First commit maybe:
And the second commit for example:
or so? |
This ensures we handle the case where SELinux is compile in the kernel (e.g. Fedora) but where it's disabled at runtime via selinux=0. fixes containers#303 Signed-off-by: ckyrouac <[email protected]>
If the user disables SELinux, we should always honor that and not care about the host state. fixes: containers#303 Signed-off-by: ckyrouac <[email protected]>
ckyrouac
force-pushed
the
selinux-fix
branch
from
5708cfb to
3921b71
Compare
|
thanks for the reviews! |
cgwalters
changed the title
--disable-selinux work on hosts that have selinux=0
Prior to this fix the selinux_enabled function would always return true, even if selinux was set to disabled or permissive.
fixed #303