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

libostree: write selinux xattr when on non-selinux systems #3151

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Jan 31, 2024

Currently when writing data for selinux systems on a non-selinux system there will be no labels. This is because
ostree_sepolicy_setfscreatecon() just returns TRUE on non-selinux systems and xattr writing for security.seliux is filtered out.

This patches uses the suggestion of Colin Walters (thanks!) from #2804 and detects if the host has selinux enabled and if not just skips filtering the xattrs for selinux.

[draft as this will need a test (and validation of the basic approach by someone experienced) and I will need a hint how to best test this]

Copy link

openshift-ci bot commented Jan 31, 2024

Hi @mvo5. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@cgwalters
Copy link
Member

/ok-to-test

@cgwalters cgwalters added difficulty/medium medium complexity/difficutly issue needs-ok-to-test triaged This issue has been evaluated and is valid reward/medium Fixing this will be notably useful and removed ok-to-test labels Jan 31, 2024
@cgwalters
Copy link
Member

[draft as this will need a test (and validation of the basic approach by someone experienced) and I will need a hint how to best test this]

I can help with a test case

@cgwalters
Copy link
Member

#3152 will help verify this, though to make it work we'll want to add a "no really try to use selinux even on non-selinux" host arg to bootc install.

Alternatively we can just do a direct ubuntu deploy here too, I can look at that.

@mvo5
Copy link
Contributor Author

mvo5 commented Jan 31, 2024

#3152 will help verify this, though to make it work we'll want to add a "no really try to use selinux even on non-selinux" host arg to bootc install.

Alternatively we can just do a direct ubuntu deploy here too, I can look at that.

Thank you!, I really appreciate the quick reply here I have no opinion about the best approach here and will happily follow your lead. I will just need a few hints how to best approach this. Alternatively feel free to push directly to this PR or I can mark it ready and we test indirectly via the approaches outlined. I am happy either way :)

cgwalters added a commit to cgwalters/bootc that referenced this pull request Jan 31, 2024
We're going to investigate supporting installing SELinux-enabled
targets from a SELinux-disabled host.  This environment
variable will allow bypassing the check.

xref ostreedev/ostree#3151
cgwalters added a commit to cgwalters/bootc that referenced this pull request Jan 31, 2024
We're going to investigate supporting installing SELinux-enabled
targets from a SELinux-disabled host.  This environment
variable will allow bypassing the check.

xref ostreedev/ostree#3151
cgwalters added a commit to cgwalters/bootc that referenced this pull request Jan 31, 2024
We're going to investigate supporting installing SELinux-enabled
targets from a SELinux-disabled host.  This environment
variable will allow bypassing the check.

xref ostreedev/ostree#3151

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

OK containers/bootc#293 helps us get farther here for testing.

And yes...trying this out I notice that yep, we are not handling the labeling for all of the "basic infrastructure dirs" here, like /ostree/deploy etc. I will look at that in the background, it's going to be a bit of a can of worms though.

@cgwalters
Copy link
Member

I did containers/bootc#294 push this farther, but there's definitely still that can of worms.

@cgwalters cgwalters force-pushed the selinux-labels-on-non-selinux-hosts branch 4 times, most recently from 790fa17 to c78f9f1 Compare February 7, 2024 02:14
@cgwalters cgwalters marked this pull request as ready for review February 7, 2024 02:14
@cgwalters
Copy link
Member

I've updated this with a test, and this verifies that it improves things as we now correctly have labels for etc in the deployment root where we didn't before.

@mvo5
Copy link
Contributor Author

mvo5 commented Feb 7, 2024

I've updated this with a test, and this verifies that it improves things as we now correctly have labels for etc in the deployment root where we didn't before.

Thank you! Very nice how straightforward it was to add a small test for this.

@cgwalters
Copy link
Member

cgwalters commented Feb 7, 2024

@mvo5 just one more bug to fix here, we need to handle the build without selinux enabled, xref https://github.com/ostreedev/ostree/actions/runs/7809000169/job/21300094068?pr=3151

@mvo5 mvo5 force-pushed the selinux-labels-on-non-selinux-hosts branch from c78f9f1 to d5f87d7 Compare February 8, 2024 14:37
mvo5 and others added 2 commits February 8, 2024 15:51
Currently when writing data for selinux systems on a non-selinux
system there will be no labels. This is because
`ostree_sepolicy_setfscreatecon()` just returns TRUE on non-selinux
systems and xattr writing for `security.seliux` is filtered out.

This patches uses the suggestion of Colin Walters (thanks!) from
ostreedev#2804 and detects if
the host has selinux enabled and if not just skips filtering the
xattrs for selinux.
As we work to change ostree to set up the labels
for things even in a selinux-host-disabled case, let's test
it here.
@mvo5 mvo5 force-pushed the selinux-labels-on-non-selinux-hosts branch from d5f87d7 to 5cfc5c7 Compare February 8, 2024 14:54
@cgwalters
Copy link
Member

/ok-to-test

@cgwalters cgwalters enabled auto-merge February 8, 2024 21:14
@cgwalters cgwalters merged commit 751ec90 into ostreedev:main Feb 8, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty/medium medium complexity/difficutly issue ok-to-test reward/medium Fixing this will be notably useful triaged This issue has been evaluated and is valid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants