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

deploy: Try to rebuild policy in new deployment if needed #2569

Merged
merged 2 commits into from
Mar 29, 2022

Conversation

WOnder93
Copy link
Contributor

Whenever the user has SELinux enabled and has any local
modules/modifications installed, it is necessary to rebuild the policy
in the final deployment, otherwise ostree will leave the binary policy
files unchanged from last deployment as it detects difference against
the base content (in rpm-ostree case this is the RPM content).

To avoid the situation where the policy binaries go stale once any local
customization of the policy is made, try to rebuild the policy as part
of sysroot_finalize_deployment(). Use the special
--rebuild-if-modules-changed switch, which detects if the input module
files have changed relative to last time the policy was built and skips
the most time-consuming part of the rebuild process if modules are
unchanged (thus making this a relatively cheap operation if the user
hasn't made any modifications to the shipped policy).

As suggested by Jonathan Lebon, this uses bubblewrap (via
g_spawn_sync()) to perform the rebuild inside the deployment's
filesystem tree, which also means that ostree will have a runtime
dependency on bubblewrap.

Partially addresses: coreos/fedora-coreos-tracker#701

@openshift-ci
Copy link

openshift-ci bot commented Mar 18, 2022

Hi @WOnder93. 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.

@dustymabe
Copy link
Contributor

/ok-to-test

@WOnder93
Copy link
Contributor Author

BTW, I did test this in a dummy Fedora CoreOS VM where I reproduced the issue (basically by installing a dummy SELinux module and then doing rpm-ostree rebase ... that involved a selinux-policy package upgrade) and I was able to verify that the SELinux policy was up to date afterwards with this patch.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot for working on this!

It looks like this is updating the libglnx submodule unintentionally.

src/libostree/ostree-sysroot-deploy.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot-deploy.c Outdated Show resolved Hide resolved
Comment on lines 2940 to 2941
if (!g_spawn_check_exit_status(exit_status, NULL))
g_message ("Failed to refresh SELinux policy - the policy contents may be inconsistent");
Copy link
Member

Choose a reason for hiding this comment

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

I guess you made this non-fatal to handle ratcheting it in? Let's figure out instead how to auto-detect it so we can tighten this. Two obvious options:

  • Feature detection at runtime: e.g. search for switch in output of semodule --help? This would be best I think but can get quite verbose.
  • Compile-time version check: add autoconf goop to detect if libselinux is high enough? Hmm, it looks like on Fedora semodule is part of policycoreutils. But they're all versioned together, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second option won't work, because we depend on a given semodule version inside the deployment, which may not match the runtime where the ostree is running (may be an older/newer/completely different deployment).

The first option I had thought off, it just didn't seem worth it. But since you request it, I'll try to do it this way.

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd vote to key this off an environment variable; we already have OSTREE_SYSROOT_OPTS, so it could be a new OSTREE_SYSROOT_OPTS=semodule-hard-fail to start. In the FCOS/RHCOS side we can manage rolling that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already went with scanning the --help output, which turned out to be not that verbose nor ugly. Please have a look and let me know if it works for you.

It's not entirely clear to me how it would work with the env variable, but the idea doesn't sound very appealing to me... Yet, it's not up to me so I'll submit to whatever you guys prefer :)

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Thank you so much for working on this!

"--bind", "var", "/var",
"--symlink", "/usr/lib", "/lib",
"--symlink", "/usr/lib32", "/lib32",
"--symlink", "/usr/lib64", "/lib64",
Copy link
Member

Choose a reason for hiding this comment

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

One messy thing is this is distribution-specific. Looks like there's some bits related to this here too https://github.com/flatpak/flatpak/blob/aac1205d66e03facc965279d5825272597b305d0/common/flatpak-run.c#L77

I think it's OK for now, but we will likely need to be updating this.

Copy link
Member

Choose a reason for hiding this comment

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

Coming in to this late, but why not do an --ro-bind of . to / followed by the writable binds for /usr, /etc and /var? Presumably the usr-merge symlinks are already setup properly in the deployment.

Comment on lines 2940 to 2941
if (!g_spawn_check_exit_status(exit_status, NULL))
g_message ("Failed to refresh SELinux policy - the policy contents may be inconsistent");
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd vote to key this off an environment variable; we already have OSTREE_SYSROOT_OPTS, so it could be a new OSTREE_SYSROOT_OPTS=semodule-hard-fail to start. In the FCOS/RHCOS side we can manage rolling that out.

@cgwalters
Copy link
Member

cgwalters commented Mar 22, 2022

Tangentially related but I want to mention; on the coreos side we've been pulling things in a whole new direction with https://github.com/coreos/enhancements/blob/main/os/coreos-layering.md

(There the policy rebuild would happen in a container and be applied centrally, not happen per host/machine)

EDIT: To be clear I think we do want both; it should work to build a custom image but e.g. test out a per-machine setsebool and the like.

@WOnder93 WOnder93 force-pushed the finalize-deployment-selinux-policy branch from a7e8064 to 51bbb72 Compare March 23, 2022 17:28
@cgwalters
Copy link
Member

Hmm, the transactionality test failed...but passed on rerun. Unfortunately it seems we don't save logs from the failing case, grr.

Offhand...it seems like this should be transactional. But let me see if I can replicate any failures locally.

Whenever the user has SELinux enabled and has any local
modules/modifications installed, it is necessary to rebuild the policy
in the final deployment, otherwise ostree will leave the binary policy
files unchanged from last deployment as it detects difference against
the base content (in rpm-ostree case this is the RPM content).

To avoid the situation where the policy binaries go stale once any local
customization of the policy is made, try to rebuild the policy as part
of sysroot_finalize_deployment(). Use the special
--rebuild-if-modules-changed switch, which detects if the input module
files have changed relative to last time the policy was built and skips
the most time-consuming part of the rebuild process if modules are
unchanged (thus making this a relatively cheap operation if the user
hasn't made any modifications to the shipped policy).

As suggested by Jonathan Lebon, this uses bubblewrap (via
g_spawn_sync()) to perform the rebuild inside the deployment's
filesystem tree, which also means that ostree will have a runtime
dependency on bubblewrap.

Partially addresses: coreos/fedora-coreos-tracker#701

Signed-off-by: Ondrej Mosnacek <[email protected]>
@cgwalters cgwalters force-pushed the finalize-deployment-selinux-policy branch from 51bbb72 to edb4f38 Compare March 28, 2022 21:21
Let's log when we don't find the expected CLI argument which
will help debug things.
@cgwalters
Copy link
Member

Our merge process doesn't squash fixup! so I went ahead and did that and also rebased to pick up the fixes to the test suite, ran locally and seems to work.

Added a commit on top for more logging. Now that I look, we don't have the updated semodule in CI here. I'm looking at trying to use FCOS next-devel i.e. F36 but running into another issue...

In the meantime though I think we can merge this.

@cgwalters cgwalters merged commit fdfb353 into ostreedev:main Mar 29, 2022
@WOnder93
Copy link
Contributor Author

Our merge process doesn't squash fixup! so I went ahead and did that and also rebased to pick up the fixes to the test suite, ran locally and seems to work.

Then maybe CONTRIBUTING.md needs updating? ;)

In the meantime though I think we can merge this.

🎉

@cgwalters
Copy link
Member

Just to xref, openshift/os@e974146 landed which is a worse version of this - in particular the policy is only rebuilt early during the boot, at which point we may already be subject to problems with drift. This PR rebuilds before booting, which avoids those issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants