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

relabel-etc: Split remount into, remount and relabel-etc #3267

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ericcurtin
Copy link
Collaborator

Split these into two different binaries we two different resonsibilities. They don't share so much functionality.

@ericcurtin ericcurtin self-assigned this Jun 19, 2024
@ericcurtin ericcurtin marked this pull request as draft June 19, 2024 12:02
@github-actions github-actions bot added the area/prepare-root Issue relates to ostree-prepare-root label Jun 19, 2024
@ericcurtin
Copy link
Collaborator Author

Incomplete and untested right now

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.

Thanks so much for working on this!

Conflicts=umount.target
# Run after core mounts
After=-.mount var.mount
After=systemd-remount-fs.service
Copy link
Member

Choose a reason for hiding this comment

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

This one can (and should) be Before=systemd-remount-fs.service instead.

Copy link
Member

Choose a reason for hiding this comment

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

OK I was thinking about this more and I think we're in a situation where actually we tell people using composefs in particular to just mount the rootfs writable in the initramfs, so the whole role of systemd-remount-fs.service is a no-op from our PoV basically.

At least...I thought we enforced that, but now I can't find it.

OnFailure=emergency.target
Conflicts=umount.target
# Run after core mounts
After=-.mount var.mount
Copy link
Member

Choose a reason for hiding this comment

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

Don't need var.mount here.
Actually let's just use:
RequiresMountsFor=/etc.

Comment on lines +28 to +29
Before=systemd-random-seed.service plymouth-read-write.service systemd-journal-flush.service systemd-sysusers.service
Before=systemd-tmpfiles-setup.service systemd-rfkill.service systemd-rfkill.socket
Copy link
Member

Choose a reason for hiding this comment

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

We can drop all this stuff, as all of that is After=systemd-remount-fs.service.

But I'm fine going belt-and-suspenders here with e.g. Before=ostree-remount.service - which already has all those Before= itself.

To combine all this I'd suggest:

Before=local-fs-pre.target systemd-remount-fs.service
# This one is already itself After=systemd-remount-fs.service, but for redundancy
Before=ostree-remount.service

glnx_autofd int fd = open (OTCORE_RUN_BOOTED, O_RDONLY | O_CLOEXEC);
if (fd < 0)
{
/* We really expect that nowadays that everything is done in the initramfs,
Copy link
Member

Choose a reason for hiding this comment

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

Don't need this copy here.

* Today systemd remounts / (recursively) as shared, so we're undoing that as early
* as possible. See also a copy of this in ostree-prepare-root.c.
*/
if (mount ("none", "/sysroot", NULL, MS_REC | MS_PRIVATE, NULL) < 0)
Copy link
Member

Choose a reason for hiding this comment

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

Also drop this

[Service]
Type=oneshot
RemainAfterExit=yes
ExecStart=/usr/lib/ostree/ostree-relabel-etc
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, so far we haven't introduced many new little binaries, but instead we've been tending to dispatch via hidden CLI verbs, see e.g. ostree admin finalize-staged. I'd slightly prefer that, i.e. we introduce a new ostree admin relabel-etc (which should barf if it's not run under systemd, i.e. we check INVOCATION_ID etc).

@@ -109,6 +109,11 @@ require_internal_units (const char *normal_dir, const char *early_dir, const cha
< 0)
return glnx_throw_errno_prefix (error, "symlinkat");

if (symlinkat (SYSTEM_DATA_UNIT_PATH "/ostree-relabel-etc.service", normal_dir_dfd,
Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with this as is, but just a note: I think we can change things so that the initramfs code itself acts as a generator and writes the enablement symlink into /run directly then.

As is it's a bit silly because the initramfs writes the file in /run which says that transient etc is enabled, then we boot in the real root, run a binary which reads that file and exits if the flag isn't found...but we can bypass all that and be slightly more efficient by just telling systemd whether or not to run the binary.

Split these into two different binaries we two different
resonsibilities. They don't share so much functionality.

Signed-off-by: Eric Curtin <[email protected]>
@@ -0,0 +1,179 @@
/*
* Copyright (C) 2011 Colin Walters <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

Tangentially related, I think you can update this to be you and or/alex, or just say (C) Red Hat, Inc.

* You should have received a copy of the GNU Lesser General Public
* License along with this library. If not, see <https://www.gnu.org/licenses/>.
*
* Author: Colin Walters <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

I've been dropping this in new code BTW, "git log" is source of truth and I don't think I wrote any of this code.

static void
relabel_dir_for_upper (const char *upper_path, const char *real_path, gboolean is_dir)
{
#ifdef HAVE_SELINUX
Copy link
Member

Choose a reason for hiding this comment

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

Optional: I think this entire binary could become conditional on SELinux actually, i.e. we don't even compile or ship it if built without selinux support.

@jlebon
Copy link
Member

jlebon commented Jun 19, 2024

Hmm, why aren't we doing the relabeling from the initramfs instead? Current kernels allow relabeling before a policy is loaded. This is notably used by Ignition: https://github.com/coreos/ignition/blob/b2ea35258bf5367a3522a4bea7f71063954b740c/internal/exec/util/selinux.go#L80.

From my experience, doing relabeling from the real root causes a lot of issues and quickly becomes a game of whack-a-race condition.

@jlebon
Copy link
Member

jlebon commented Jun 19, 2024

Seems to work at first glance:

# https://gitlab.com/fedora/bootc/examples/-/blob/main/transient-etc/Containerfile
FROM quay.io/centos-bootc/centos-bootc:stream9
# Configure the initramfs, see https://github.com/ostreedev/ostree/blob/main/man/ostree-prepare-root.xml
RUN echo -e '[etc]\ntransient=true' >> /usr/lib/ostree/prepare-root.conf && \
    set -x; kver=$(cd /usr/lib/modules && echo *); dracut -vf /usr/lib/modules/$kver/initramfs.img $kver

I bootc install to-disk the resulting image, boot it, add rd.break, and then from the switchroot shell:

$ chroot /sysroot setfiles -vF /etc/selinux/targeted/contexts/files/file_contexts /etc
Relabeled /etc from <no context> to system_u:object_r:etc_t:s0
Relabeled /etc/systemd/system from system_u:object_r:etc_t:s0 to system_u:object_r:systemd_unit_file_t:s0
Relabeled /etc/systemd/system/basic.target.wants from system_u:object_r:etc_t:s0 to system_u:object_r:systemd_unit_file_t:s0
Relabeled /etc/systemd/system/default.target.wants from system_u:object_r:etc_t:s0 to system_u:object_r:systemd_unit_file_t:s0
Relabeled /etc/systemd/system/getty.target.wants from system_u:object_r:etc_t:s0 to system_u:object_r:systemd_unit_file_t:s0
Relabeled /etc/systemd/system/graphical.target.wants from system_u:object_r:etc_t:s0 to system_u:object_r:systemd_unit_file_t:s0
Relabeled /etc/systemd/system/local-fs.target.wants from system_u:object_r:etc_t:s0 to system_u:object_r:systemd_unit_file_t:s0
Relabeled /etc/systemd/system/multi-user.target.wants from system_u:object_r:etc_t:s0 to system_u:object_r:systemd_unit_file_t:s0
Relabeled /etc/systemd/system/network-online.target.wants from system_u:object_r:etc_t:s0 to system_u:object_r:systemd_unit_file_t:s0
Relabeled /etc/systemd/system/sockets.target.wants from system_u:object_r:etc_t:s0 to system_u:object_r:systemd_unit_file_t:s0
Relabeled /etc/systemd/system/sysinit.target.wants from system_u:object_r:etc_t:s0 to system_u:object_r:systemd_unit_file_t:s0
Relabeled /etc/systemd/system/timers.target.wants from system_u:object_r:etc_t:s0 to system_u:object_r:systemd_unit_file_t:s0

(Though you'd normally add setfiles to your initrd and use -r /sysroot instead of doing a chroot.)

@cgwalters
Copy link
Member

Relabeling from the initramfs indeed sounds way better. The only thing I'm a little uncertain about is the semantics for how e.g. selinux=0 or other flags for configuring it in the real root are handled. Maybe running setfiles handles this already?

@ericcurtin
Copy link
Collaborator Author

ericcurtin commented Jun 20, 2024

I like the idea of using setfiles or something similar, less code to maintain...

I think selinux=0, etc. is easy to handle in systemd .service file:

ConditionKernelCommandLine=!selinux=0

We may be able to achieve this with zero C code here, just write a well crafted systemd .service file.

Although, I would rather we relabelled from rootfs, even if it meant running this .service file in it's own exclusive time slot.

We put a lot of effort in Automotive for example to make our initramfs super tiny like < 10M .

Ideally we wouldn't relabel on boot at all tbh, it's another thing to do during boot and again we are trying to cut down on doing tasks at boot to make that as fast as possible. Any reason we can't do this at image composition time or shutdown?

/.autorelabel for example seems to perform relabeling on shutdown

@cgwalters
Copy link
Member

Ideally we wouldn't relabel on boot at all tbh, it's another thing to do during boot and again we are trying to cut down on doing tasks at boot to make that as fast as possible. Any reason we can't do this at image composition time or shutdown?

Automotive is using transient etc, this is about the in-memory inodes for the overlayfs. There's no real way to shortcut this today, we have a kind of complex interlock between selinux policy loading vs the mount.

However as far as the cost...slimming down what's in /etc by default will inherently make this cheaper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/prepare-root Issue relates to ostree-prepare-root do-not-merge/work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants