Ensure SELinux label policy is applied when merging image layers

[jeamland]

This has some bearing on #388.

When importing a container image via ostree container image pull ..., care is taken to maintain SELinux label policy when importing the layers, vis:

ostree-rs-ext/lib/src/container/store.rs

Lines 869 to 876 in 9a4743a

	// An important aspect of this is that we SELinux label the derived layers using
	// the base policy.
	let opts = crate::tar::WriteTarOptions {
	base: Some(base_commit.clone()),
	selinux: true,
	allow_nonusr: root_is_transient,
	retain_var: self.ostree_v2024_3,
	};

and then in write_tar itself:

ostree-rs-ext/lib/src/tar/write.rs

Lines 335 to 343 in 9a4743a

	let sepolicy = if options.selinux {
	if let Some(base) = options.base {
	Some(sepolicy_from_base(&repo, &base).context("tar: Preparing sepolicy")?)
	} else {
	None
	}
	} else {
	None
	};

However when it comes time to merge these layers with the base commit, we fail to apply the SELinux policy of the base commit:

ostree-rs-ext/lib/src/container/store.rs

Lines 973 to 985 in 9a4743a

	let modifier =
	ostree::RepoCommitModifier::new(ostree::RepoCommitModifierFlags::CONSUME, None);
	modifier.set_devino_cache(&devino);
	let mt = ostree::MutableTree::new();
	repo.write_dfd_to_mtree(
	(*td).as_raw_fd(),
	rootpath,
	&mt,
	Some(&modifier),
	cancellable,
	)
	.context("Writing merged filesystem to mtree")?;

This change adds a call to ostree::RepoCommitModifier::set_sepolicy_from_commit() to ensure that the SELinux labelling is consistent with the base commit.

[cgwalters]

The merge shouldn't introduce any new files or directories though; we should always be taking the underlying object. So I think this will just unnecessarily force us to recompute the labels without fixing the core bug from #388 right?

[jeamland]

So the relationship to #388 may not be there but there is a noticeable difference in result nonetheless.

tl;dr: The merge commit seems to relabel files based on the context in which the merge commit happens rather than the policy of the base commit. Working shown in (possibly overenthusiastic) detail below.

The scenario I'm working with here is that I've got an ostree commit created using rpm-ostree compose tree. I've exported this with ostree container encapsulate, then used the resulting container image to create a new container image, in this case installing another rpm (I chose telnet) not present in the original. I've then pulled this image back in to the original ostree repo using ostree container image pull. The ostree container image pull is being done inside a container.

When I run this process without this patch, I see an oddly large number of changes between the base commit and the commit based off the new container image:

# ostree diff --repo /tmp/test_repo base_commit ostree/container/image/oci-archive_3A__2F_customisation__test/layered_2E_tar | wc -l
6886

Examining this more closely we see some additions we expect:

# ostree diff --repo /tmp/test_repo base_commit ostree/container/image/oci-archive_3A__2F_customisation__test/layered_2E_tar | grep telnet
M    /usr/etc/selinux/targeted/active/modules/100/telnet
A    /usr/bin/telnet
A    /usr/share/doc/telnet
A    /usr/share/doc/telnet/README
A    /usr/share/man/man1/telnet.1.gz

But we also see a bunch of file modifications that seem unnecessary, to pick one example out of this case a whole bunch of zoneinfo files have changed:

# ostree diff --repo /tmp/test_repo base_commit ostree/container/image/oci-archive_3A__2F_customisation__test/layered_2E_tar | grep /usr/share/zoneinfo
M    /usr/share/zoneinfo
M    /usr/share/zoneinfo/Africa
M    /usr/share/zoneinfo/America
M    /usr/share/zoneinfo/America/Argentina
M    /usr/share/zoneinfo/America/Indiana
M    /usr/share/zoneinfo/America/Kentucky
M    /usr/share/zoneinfo/America/North_Dakota
M    /usr/share/zoneinfo/Antarctica
M    /usr/share/zoneinfo/Arctic
M    /usr/share/zoneinfo/Asia
M    /usr/share/zoneinfo/Atlantic
M    /usr/share/zoneinfo/Australia
M    /usr/share/zoneinfo/Brazil
M    /usr/share/zoneinfo/Canada
M    /usr/share/zoneinfo/Chile
M    /usr/share/zoneinfo/Etc
M    /usr/share/zoneinfo/Europe
M    /usr/share/zoneinfo/Indian
M    /usr/share/zoneinfo/Mexico
M    /usr/share/zoneinfo/Pacific
M    /usr/share/zoneinfo/US
[...]

Digging in to these we see that the only change is in the metadata, and the only change there is to the label:

# ostree ls -dXC --repo /tmp/test_repo base_commit /usr/share/zoneinfo/posix/Australia
d00755 0 0      0 4d0c29a0ee66111181a3b003c64a2aad3be8c0d59c4a7270be3dae58c5bb29ed 832a63bf3be2edb1d2306f191f01d86ca7bb0c69ba2b13cd711e6f1f53ec8572 { [(b'security.selinux', b'system_u:object_r:locale_t:s0')] } /usr/share/zoneinfo/posix/Australia
# ostree ls -dXC --repo  /tmp/test_repo ostree/container/image/oci-archive_3A__2F_customisation__test/layered_2E_tar /usr/share/zoneinfo/posix/Australia
d00755 0 0      0 4d0c29a0ee66111181a3b003c64a2aad3be8c0d59c4a7270be3dae58c5bb29ed 2b7ec15cc38eb013c1dc5db23a7879aaa9a21d8d4f3fbd69c7563a38743af64e { [(b'security.selinux', b'system_u:object_r:container_file_t:s0:c240,c499')] } /usr/share/zoneinfo/posix/Australia

So the next question is when did the label change happen? Luckily we can examine the individual layer commits. I see four of those and examining each we see:

# ostree ls -dXC --repo /tmp/test_repo ostree/container/blob/sha256_3A_017ac54d6797e1c173c7e20ee61ae24b701e2493d146db3f45aa918dffeb0347 /usr/share/zoneinfo/posix/Australia
error: Inspecting path '/usr/share/zoneinfo/posix/Australia': No such file or directory: /usr/share/zoneinfo
# ostree ls -dXC --repo /tmp/test_repo ostree/container/blob/sha256_3A_3ddaa625f2f100642534cba90880a57d31eb3cf0bf1f2e6d2c23988729f18049 /usr/share/zoneinfo/posix/Australia
error: Inspecting path '/usr/share/zoneinfo/posix/Australia': No such file or directory: /usr/share
# ostree ls -dXC --repo /tmp/test_repo ostree/container/blob/sha256_3A_525cd6a36ca994a49be414fdaf46457fc970f1f092aeec9f8160f76bf57c1124 /usr/share/zoneinfo/posix/Australia
error: Inspecting path '/usr/share/zoneinfo/posix/Australia': No such file or directory: /usr/share
# ostree ls -dXC --repo /tmp/test_repo ostree/container/blob/sha256_3A_b0bba405443017b77512bf3b28e217d894c56de48ad11e71e115d8e453b24127 /usr/share/zoneinfo/posix/Australia
d00755 0 0      0 4d0c29a0ee66111181a3b003c64a2aad3be8c0d59c4a7270be3dae58c5bb29ed 832a63bf3be2edb1d2306f191f01d86ca7bb0c69ba2b13cd711e6f1f53ec8572 { [(b'security.selinux', b'system_u:object_r:locale_t:s0')] } /usr/share/zoneinfo/posix/Australia

So the only place that directory shows up is in that last layer and with the correct label and, confirming a suspicion:

# ostree rev-parse --repo /tmp/test_repo ostree/container/blob/sha256_3A_b0bba405443017b77512bf3b28e217d894c56de48ad11e71e115d8e453b24127
1ce09beeae1185fcf8fbd850c7ce3ec309616ec39848e1351d5debf3942f631f
# ostree rev-parse --repo /tmp/test_repo base_commit
1ce09beeae1185fcf8fbd850c7ce3ec309616ec39848e1351d5debf3942f631f

Yep, it's the same as the base commit. So the only operation that touched that label was the merge commit.

Doing the process again but with the patch to add the SELinux policy during the merge commit:

# ostree diff --repo /tmp/test_repo base_commit ostree/container/image/oci-archive_3A__2F_customisation__test/layered_2E_tar | wc -l
30

That seems more like what I expected, as does this:

# ostree ls -dXC --repo /tmp/test_repo ostree/container/image/oci-archive_3A__2F_customisation__test/layered_2E_tar /usr/share/zoneinfo/posix/Australia
d00755 0 0      0 4d0c29a0ee66111181a3b003c64a2aad3be8c0d59c4a7270be3dae58c5bb29ed 832a63bf3be2edb1d2306f191f01d86ca7bb0c69ba2b13cd711e6f1f53ec8572 { [(b'security.selinux', b'system_u:object_r:locale_t:s0')] } /usr/share/zoneinfo/posix/Australia

[cgwalters]

OK so you're seeing this skew when running in a container; makes sense. But wait ugh...that implies that the labels we've been using for layers actually use whatever we see from the raw xattrs, but...

OK in your case when you were pulling into a repo in a container image, were you using an archive mode or bare-user? You must have been.

I think this bug will only occur when operating on one of those repo modes, because when operating on bare repos the checkout will always see the right xattrs.

Hmm it's a bit unfortunate here because I think this may actually force the normal "bare" repo case to do another trip through all the 4000 regexps in the policy to handle labeling for each file.

Utimately composefs is going to make this 1000% saner, but doing a hard switch to that gets tricky.