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

Rework SELinux labeling more #397

Merged
merged 1 commit into from
Mar 18, 2024
Merged

Conversation

cgwalters
Copy link
Collaborator

First, in the install code, acquire a proper policy object.

Add helpers for writing files/directories that take a policy object and operate solely using fd-relative operations and don't fork off helper processes.

This is a notable cleanup because we don't need to juggle absolute file paths and fds, which avoids a lot of confusion. Our usage of a wrapper for the cap-std-ext atomic write API for generating files ensures that if the file is present, it will always have the correct label without any race conditions.

Change the one place we now call chcon as a helper process to be an explicit recursive selinux relabeling. In the future we should switch to using a direct API instead of forking off /usr/bin/chcon - then everything would be fd-relative.

Copy link

openshift-ci bot commented Mar 17, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@github-actions github-actions bot added the area/install Issues related to `bootc install` label Mar 17, 2024
@cgwalters cgwalters force-pushed the more-labeling branch 2 times, most recently from 943f601 to d4f4311 Compare March 17, 2024 22:05
@cgwalters cgwalters marked this pull request as ready for review March 17, 2024 22:12
@cgwalters cgwalters force-pushed the more-labeling branch 2 times, most recently from 560e6b4 to 0cbe32e Compare March 18, 2024 00:08
@cgwalters
Copy link
Collaborator Author

Ah OK right,

ERROR Failed to look up context for "./boot.1.1": No data available (os error 61)

We have more forcible labeling to do here. Conceptually this one should be ostree side probably.

@cgwalters
Copy link
Collaborator Author

Ohh yeah, looking at the warning output there's quite a bit of work in ostree to handle this. Updated ostreedev/ostree#2804 (comment)

Maybe what we need here for all of that is just a single pass "relabel everything that isn't a deployment with usr_t".

Although

No SELinux label found for: "./deploy/default/deploy/f512f95348cbaa175f3361dea0f09ccaadd089909dacc3ffb0c0b7e602f4f2dc.0/etc/tmpfiles.d/bootc-root-ssh.conf"

That one should have worked with this. But not sure about blocking on that.

Copy link
Member

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@jeckersb jeckersb left a comment

Choose a reason for hiding this comment

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

Nice improvement! 👍

lib/src/install.rs Outdated Show resolved Hide resolved
@cgwalters cgwalters force-pushed the more-labeling branch 3 times, most recently from 2362ace to 9ba1f47 Compare March 18, 2024 20:58
@cgwalters
Copy link
Collaborator Author

OK yeah, I'm going to have to set up a dev/test environment for this "install from host with selinux=0" case, but...I think this latest push should get us a lot closer to working in that path.

@cgwalters cgwalters force-pushed the more-labeling branch 2 times, most recently from 0f914c1 to 0a5f152 Compare March 18, 2024 22:29
First, in the install code, acquire a proper policy object.

Add helpers for writing files/directories that take a policy
object and operate *solely* using fd-relative operations and
don't fork off helper processes.

This is a notable cleanup because we don't need to juggle
absolute file paths *and* fds, which avoids a lot of confusion.
Our usage of a wrapper for the cap-std-ext atomic write API
for generating files ensures that if the file is present,
it will always have the correct label without any race conditions.

Change the one place we now call `chcon` as a helper process
to be an explicit recursive selinux relabeling.  In the
future we should switch to using a direct API instead of
forking off `/usr/bin/chcon` - then everything would be fd-relative.

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters
Copy link
Collaborator Author

OK cool! Now we're just down to

No SELinux label found for: "./deploy/default/deploy/6f41b63072258824399f24f39c499e5ce22dd8e5d6942917b2b21b11f4ab5463.0/.ostree.cfs"

Which should be pretty tractable to fix in ostree. (In practice it's not really relevant I think because policy isn't loaded until post-switchroot)

@cgwalters cgwalters merged commit d039f26 into containers:main Mar 18, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/install Issues related to `bootc install`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants