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

Fix install_t again #284

Merged
merged 3 commits into from
Jan 25, 2024
Merged

Fix install_t again #284

merged 3 commits into from
Jan 25, 2024

Conversation

cgwalters
Copy link
Collaborator

@cgwalters cgwalters commented Jan 24, 2024

lsm: Make setenforce 0 fallback require BOOTC_SETENFORCE0_FALLBACK

We shouldn't perform global system mutation without an opt-in.
As painful as it is.

Signed-off-by: Colin Walters [email protected]


lsm: Test if we have install_t capability

Hardcoding install_t is a bit ugly; maybe at some point
things change so that spc_t has install_t privileges.

Let's do a runtime check if we can set an invalid label; if
so then we're good.

Signed-off-by: Colin Walters [email protected]


lsm: Make a not-nosuid /tmp

This was the thing that was breaking our unconfined_t -> install_t
transition; the host /tmp is nosuid. It simplifies things
here to just make our own, so do that.

Signed-off-by: Colin Walters [email protected]


We shouldn't perform global system mutation without an opt-in.
As painful as it is.

Signed-off-by: Colin Walters <[email protected]>
Hardcoding `install_t` is a bit ugly; maybe at some point
things change so that `spc_t` has `install_t` privileges.

Let's do a runtime check if we can set an invalid label; if
so then we're good.

Signed-off-by: Colin Walters <[email protected]>
Copy link

openshift-ci bot commented Jan 24, 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

@cgwalters
Copy link
Collaborator Author

I realized we were silently falling back to setenforce 0 - this was motivated by trying to stop doing that by default.

This was the thing that was breaking our `unconfined_t` -> `install_t`
transition; the host `/tmp` is `nosuid`.  It simplifies things
here to just make our own, so do that.

Signed-off-by: Colin Walters <[email protected]>
@github-actions github-actions bot added the area/install Issues related to `bootc install` label Jan 25, 2024
@cgwalters cgwalters changed the title Two install_t changes Fix install_t again Jan 25, 2024
@cgwalters cgwalters marked this pull request as ready for review January 25, 2024 12:52
@cgwalters
Copy link
Collaborator Author

OK finally figured out what broke the install_t thing - it was 1af4932 which then pretty quickly got papered over by 600c64d

So this fix I'm pretty confident in.

@jeckersb jeckersb self-requested a review January 25, 2024 14:30
@cgwalters
Copy link
Collaborator Author

Another important thing in this change is that instead of trying to just check the security context is install_t even we actually test that it works by setting an unmapped security context. So that's much more robust.

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.

LGTM, I didn't know about the /proc/1/root trick, interesting!

@jeckersb jeckersb merged commit cbe6062 into containers:main Jan 25, 2024
12 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.

2 participants