install: Add final SELinux relabel of the physical root filesystem#2035
install: Add final SELinux relabel of the physical root filesystem#2035jeckersb wants to merge 1 commit intobootc-dev:mainfrom
Conversation
|
Assuming this actually works properly (seems to work for me locally but we'll see what the matrix says), we should be able to revert the install parts of #2025, keeping the booted system part as well as the test re-enablement. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a final, comprehensive SELinux relabeling pass on the physical root filesystem just before finalization. This is a robust approach to ensure all files have correct SELinux labels. The logic correctly handles different installation backends by skipping the ostree/deploy directory for ostree-based installs (where its contents are already labeled) and relabeling the entire tree for other scenarios like composefs. The implementation is clean, well-commented, and correctly placed within the installation flow. The changes look solid and improve the reliability of the installation process regarding SELinux contexts.
Perform a full SELinux relabel pass over the physical root filesystem as the very last step before filesystem finalization. This ensures all files on the physical root have correct SELinux labels. The ostree/deploy directory is skipped since its contents are already correctly labeled as part of the container image deployment. If ostree/deploy doesn't exist (e.g. composefs backend), the entire tree is relabeled. Assisted-by: OpenCode (claude-opus-4-6) Signed-off-by: John Eckersberg <jeckersb@redhat.com>
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Isn't the goal here to prove this works by dropping the enforcing=0 in the composefs case? If this does fix it we should of course do that, if it doesn't...I think we should just keep trying until it does right?
| } | ||
|
|
||
| // As the very last step before filesystem finalization, do a full SELinux | ||
| // relabel of the physical root filesystem. We skip ostree/deploy because |
There was a problem hiding this comment.
I think (but there's $details here) we actually need to precisely skip deployment roots not just all of ostree/deploy.
However...can't we also drop the current ostree-only relabeling step with this? That would really prove things out.
There was a problem hiding this comment.
Is there a difference in the labeling between ostree-with-composefs vs ostree-without-composefs? In the composefs case I don't think it matters too much how accurate the labels are since the source of truth ends up in the erofs (but does it populate the erofs from the underlying files? I admit I haven't looked too much into that before now...).
But in the non-composefs case (do we even use that anywhere?) the labels are the final labels.
There was a problem hiding this comment.
Is there a difference in the labeling between ostree-with-composefs vs ostree-without-composefs? In the composefs case I don't think it matters too much how accurate the labels are since the source of truth ends up in the erofs (but does it populate the erofs from the underlying files? I admit I haven't looked too much into that before now...).
There's not a difference AFAIK no, but a problem is that in the ostree code path we still sometimes for legacy reasons operate directly on a deployment root without the EROFS (running semodule e.g.) and so the labels need to be correct on the underlying files still too.
Anyways though what we're trying to solve here is the labeling of everything else; ostree should be getting labels of the deployment root correct.
|
Looks like CI mostly worked, except: And yes I realized later in the evening after I had walked away from this that I forgot to remove the |
The relabeling code should probably be changed to use https://docs.rs/cap-std-ext/latest/cap_std_ext/dirext/trait.CapStdExtDirExt.html#tymethod.walk with noxdev |
|
Doesn't seem enough for the files to get just any label, because |
|
Ah right I think that's because this code needs to pass an override path of |
|
Circling back to this comment it feels like the most correct approach here would be to label all of the composefs objects with some custom type along with some policy glue (via https://fedoraproject.org/wiki/SELinux/IndependentPolicy ?) to make it function similarly to |
|
I agree it'd be better to have a dedicated type for composefs objects. In theory we could ship custom policy addition to add it now without blocking on upstream. There's a variety of examples of this, e.g. https://github.com/cockpit-project/cockpit/tree/main/selinux I personally would say it seems way easier to just use |
2 participants
Perform a full SELinux relabel pass over the physical root filesystem as
the very last step before filesystem finalization. This ensures all files
on the physical root have correct SELinux labels.
The ostree/deploy directory is skipped since its contents are already
correctly labeled as part of the container image deployment. If
ostree/deploy doesn't exist (e.g. composefs backend), the entire tree is
relabeled.
Assisted-by: OpenCode (claude-opus-4-6)
Signed-off-by: John Eckersberg jeckersb@redhat.com