Soft reboot: Detect SELinux policy deltas #1768

gursewak1997 · 2025-11-14T18:47:31Z

Add check to prevent soft reboot when SELinux policies differ between booted and target deployments, since policy is not reloaded across soft reboots.

Fixes: #1760

gemini-code-assist

Code Review

This pull request adds a check to prevent soft reboots when there's a change in SELinux policy between the booted and target deployments. This is a valuable safety measure. The implementation introduces a new function check_selinux_policy_for_soft_reboot and integrates it into the soft reboot logic for both staged updates and rollbacks.

My main feedback is on the logic within check_selinux_policy_for_soft_reboot, which I found to be too permissive. It incorrectly allows a soft reboot when one deployment has an SELinux policy and the other does not. This could lead to a system running in an insecure state. I've provided a critical-severity comment with a suggested code change to address this.

crates/lib/src/cli.rs

cgwalters

I know this raises the bar for landing this, but we really need to have tests when landing changes as a general rule.

We could definitely handle this in a better way but because right now of the two primary test frameworks (GHA and TF) neither offer an elegant way to have a registry by default (though we could of course) we end up just building images locally and booting those.

Simplest way to test this is to inject a local selinux policy module, there's various docs online for this.

cgwalters · 2025-11-14T18:52:39Z

crates/lib/src/cli.rs

+    let target_policy = crate::lsm::new_sepolicy_at(&target_fd)?;
+
+    // If either deployment doesn't have a policy, we can't compare
+    // In this case, we'll allow soft reboot (policy will be loaded on boot)


I don't think that's true, we should not allow soft reboot in the degenerate case that selinux is enabled in one but not the other.

cgwalters · 2025-11-14T18:56:04Z

crates/lib/src/cli.rs

+/// Check if SELinux policy differs between booted and target deployments.
+/// Returns an error if SELinux is enabled and the policies differ.
+#[context("Checking SELinux policy compatibility with soft reboot")]
+fn check_selinux_policy_for_soft_reboot(


This helps, but I think what we really want is to change has_soft_reboot_capability.

But then that raises the next thing in that in theory this change should go in libostree which has a deployment_can_soft_reboot API which already does checks like this.

OTOH, I don't mind filling it in here either, we can just have a // TODO lower into ostree to start.

The other thing of course related to this is that when we get around to soft reboots + composefs, we'll need similar logic there.

Made some changes pertaining to this.

cgwalters · 2025-11-17T18:52:20Z

crates/lib/src/status.rs

+        return true;
+    }
+
+    let Ok(booted_fd) = crate::utils::deployment_fd(sysroot, booted_deployment) else {


I don't expect this operation to ever fail, I don't think we should "swallow errors" in a case like this.

cgwalters · 2025-11-17T18:54:55Z

crates/lib/src/status.rs

+    let Ok(booted_fd) = crate::utils::deployment_fd(sysroot, booted_deployment) else {
+        return false; // Can't check, be conservative
+    };
+    let Ok(booted_policy) = crate::lsm::new_sepolicy_at(&booted_fd) else {


This one...hmmm...it'd be quite surprising to me if we failed to init the selinux policy - and if we keep the code as is, failure to do so should at least use our log_err_or_else helper (which hmm I guess could nowadays e.g. at least always log to the systemd journal?).

I would say though we should just keep using the ? here too.

cgwalters · 2025-11-17T18:55:52Z

crates/lib/src/status.rs

+        }
+        (Some(booted), Some(target)) => {
+            // Both have policies, checksums must match
+            // SAFETY: new_sepolicy_at filters out policies without checksums


Yes though I think we could do

let booted_policy = booted_policy.map(|p| p.csum()); let target_policy = target_policy(|p| p.csum());

And then we just compare those two

cgwalters · 2025-11-17T18:56:45Z

tmt/tests/booted/test-soft-reboot-selinux-policy.nu

+# Inject a local SELinux policy change by modifying file_contexts
+# This will change the policy checksum between deployments
+RUN mkdir -p /opt/bootc-test-selinux-policy && \
+    echo '/opt/bootc-test-selinux-policy /opt/bootc-test-selinux-policy' >> /etc/selinux/targeted/contexts/files/file_contexts.subs_dist || true


Why the || true? In general I don't think we should ignore errors

cgwalters · 2025-11-17T18:57:30Z

tmt/tests/booted/test-soft-reboot-selinux-policy.nu

+
+    bootc image copy-to-storage
+
+    # Create a derived container that doesn't change the policy


We're already covering the same-policy scenario in most other tests, I don't think we need to do this.

gursewak1997 · 2025-11-18T01:18:52Z

The test fails because modifying file_contexts.local doesn't change the compiled policy checksum that ostree uses. The checksum comes from the compiled policy binary, not source files. We need a different approach to change the policy checksum (e.g., installing a policy module or using different base images).
The implementation in status.rs is correct afaik

cgwalters · 2025-11-18T16:32:35Z

The test fails because modifying file_contexts.local doesn't change the compiled policy checksum that ostree uses. The checksum comes from the compiled policy binary, not source files. We need a different approach to change the policy checksum (e.g., installing a policy module or using different base images).

Right let's install a policy module then.

https://github.com/containers/composefs-rs/blob/0f636031a1ec81cdd9e7f674909ef6b75c2642cb/examples/uki/Containerfile#L49
is one example of this

cgwalters · 2025-11-19T01:11:26Z

tmt/tests/booted/test-soft-reboot-selinux-policy.nu

+
+# Create a minimal SELinux policy module that will change the policy checksum
+# We install it to ensure it's part of the deployment filesystem
+RUN mkdir -p /tmp/bootc-test-policy && \\


This one cries out to be a heredoc via RUN <<EORUN like we do in other places and we can drop all the && \\

cgwalters · 2025-11-19T01:12:22Z

tmt/tests/booted/test-soft-reboot-selinux-policy.nu

+    # Ensure the policy module is in /usr/etc/selinux for the deployment
+    mkdir -p /usr/etc/selinux/targeted/modules/active/modules && \\
+    if [ -d /etc/selinux/targeted/modules/active/modules ]; then \\
+        cp -a /etc/selinux/targeted/modules/active/modules/* /usr/etc/selinux/targeted/modules/active/modules/ 2>/dev/null || true; \\


I think "error suppression and masking" via things like 2>/dev/null || true need justification.

But all of this stuff related to /usr/etc just looks wrong/unnecessary to me; I think the semodule -i should be sufficient.

BTW a bootc container lint --fatal-warnings at the end of this build should have errored out due to having both /etc and /usr/etc

cgwalters · 2025-11-19T01:13:24Z

tmt/tests/booted/test-soft-reboot-selinux-policy.nu

+    assert (not ("/run/nextroot" | path exists)) "Soft reboot should not be prepared when policies differ"
+
+    # Reset and do a full reboot instead
+    ostree admin prepare-soft-reboot --reset


I think this one should be unnecessary as we shouldn't have queued any soft reboot state in the first place

Add check to prevent soft reboot when SELinux policies differ between booted and target deployments, since policy is not reloaded across soft reboots. Assisted-by: Cursor (Auto) Signed-off-by: gursewak1997 <[email protected]>

bootc-bot bot requested a review from jmarrero November 14, 2025 18:47

gemini-code-assist bot reviewed Nov 14, 2025

View reviewed changes

crates/lib/src/cli.rs Outdated Show resolved Hide resolved

gursewak1997 force-pushed the fix-1760-detect-selinux-policy-deltas-soft-reboot branch 2 times, most recently from 7001036 to 770289f Compare November 14, 2025 18:58

cgwalters reviewed Nov 14, 2025

View reviewed changes

gursewak1997 force-pushed the fix-1760-detect-selinux-policy-deltas-soft-reboot branch 6 times, most recently from 3fdbb49 to dcbe65e Compare November 14, 2025 20:18

cgwalters reviewed Nov 17, 2025

View reviewed changes

gursewak1997 force-pushed the fix-1760-detect-selinux-policy-deltas-soft-reboot branch 2 times, most recently from 6c48e6e to 006d882 Compare November 17, 2025 23:01

gursewak1997 force-pushed the fix-1760-detect-selinux-policy-deltas-soft-reboot branch from 006d882 to dd6e419 Compare November 18, 2025 22:07

cgwalters requested changes Nov 19, 2025

View reviewed changes

Soft reboot: Detect SELinux policy deltas

4e4c5b9

Add check to prevent soft reboot when SELinux policies differ between booted and target deployments, since policy is not reloaded across soft reboots. Assisted-by: Cursor (Auto) Signed-off-by: gursewak1997 <[email protected]>

gursewak1997 force-pushed the fix-1760-detect-selinux-policy-deltas-soft-reboot branch from dd6e419 to 4e4c5b9 Compare November 19, 2025 19:03


		bootc image copy-to-storage

		# Create a derived container that doesn't change the policy

Soft reboot: Detect SELinux policy deltas #1768

Are you sure you want to change the base?

Soft reboot: Detect SELinux policy deltas #1768

Conversation

gursewak1997 commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gursewak1997 commented Nov 18, 2025

Uh oh!

cgwalters commented Nov 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gursewak1997 commented Nov 14, 2025 •

edited

Loading