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

05core: relabel /var from initrd #245

Merged
merged 1 commit into from
Dec 5, 2019

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Nov 29, 2019

Now that the kernel supports relabeling from the initrd, we can clean up
some hacks here. After running systemd-tmpfiles, immediately relabel
what we just wrote using setfiles. This allows us to drop the Z /var
tmpfiles.d dropin, and hacking around systemd-random-seed.service
ordering.

For compatibility with RHCOS, I added a fallback in coreos-relabel so
that if initrd relabeling isn't supported, we just use tmpfiles.d
dropins. (RHCOS today sources the 05core overlay directly, and I think
doing it this way is cleaner than splitting things out into a separate
overlay).

I've also snuck in a fix for the live case with persistent /var so we
don't always relabel everything in there.

@jlebon
Copy link
Member Author

jlebon commented Nov 29, 2019

So with this + coreos/ignition#846, try:

$ curl -LO https://gist.githubusercontent.com/jlebon/889d34b481a8c2abee618cf448a4c6c6/raw/e42dcfa7407d75704c29a5aec2f9d87757a10fdf/config-var-partitions.ign
$ cosa run -i config-var-partitions.ign --size 15
...
[core@coreos ~]$ findmnt
TARGET                           SOURCE     FSTYPE     OPTIONS
...
|-/var                           /dev/vda5  xfs        rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota
| |-/var/home                    /dev/vda6  xfs        rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota
| |-/var/log                     /dev/vda9  xfs        rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota
| | `-/var/log/journal           /dev/vda10 xfs        rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota
| |-/var/srv                     /dev/vda11 xfs        rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota
| `-/var/lib                     /dev/vda7  xfs        rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota
|   `-/var/lib/containers        /dev/vda8  xfs        rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota
...
[core@coreos ~]$ journalctl | grep avc
[core@coreos ~]$ ls -laZ /var/home/core/hello.txt
-rw-r--r--. 1 root root unconfined_u:object_r:user_home_t:s0 11 Nov 29 17:12 /var/home/core/hello.txt

@cgwalters
Copy link
Member

Now that the kernel supports relabeling from the initrd

Is that in a Fedora kernel build yet? It looks like it's not in an upstream release yet:

walters@toolbox ~/s/linux> git log -n 1 --format=fuller
commit 596cf45cbf6e4fa7bcb0df33e373a7d062b644b5 (HEAD -> master, origin/master, origin/HEAD)
Merge: c3bfc5dd73c6 937790699be9
Author:     Linus Torvalds <torvalds@linux-foundation.org>
AuthorDate: Sun Dec 1 20:36:41 2019 -0800
Commit:     Linus Torvalds <torvalds@linux-foundation.org>
CommitDate: Sun Dec 1 20:36:41 2019 -0800

    Merge branch 'akpm' (patches from Andrew)
    
    Merge updates from Andrew Morton:
     "Incoming:
    
       - a small number of updates to scripts/, ocfs2 and fs/buffer.c
    
       - most of MM
    
      I still have quite a lot of material (mostly not MM) staged after
      linux-next due to -next dependencies. I'll send those across next week
      as the preprequisites get merged up"
    
    * emailed patches from Andrew Morton <akpm@linux-foundation.org>: (135 commits)
      mm/page_io.c: annotate refault stalls from swap_readpage
      mm/Kconfig: fix trivial help text punctuation
      mm/Kconfig: fix indentation
      mm/memory_hotplug.c: remove __online_page_set_limits()
      mm: fix typos in comments when calling __SetPageUptodate()
      mm: fix struct member name in function comments
      mm/shmem.c: cast the type of unmap_start to u64
      mm: shmem: use proper gfp flags for shmem_writepage()
      mm/shmem.c: make array 'values' static const, makes object smaller
      userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
      fs/userfaultfd.c: wp: clear VM_UFFD_MISSING or VM_UFFD_WP during userfaultfd_register()
      userfaultfd: wrap the common dst_vma check into an inlined function
      userfaultfd: remove unnecessary WARN_ON() in __mcopy_atomic_hugetlb()
      userfaultfd: use vma_pagesize for all huge page size calculation
      mm/madvise.c: use PAGE_ALIGN[ED] for range checking
      mm/madvise.c: replace with page_size() in madvise_inject_error()
      mm/mmap.c: make vma_merge() comment more easy to understand
      mm/hwpoison-inject: use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs fops
      autonuma: reduce cache footprint when scanning page tables
      autonuma: fix watermark checking in migrate_balanced_pgdat()
      ...
walters@toolbox ~/s/linux> git tag --contains 3e3e24b42043eceb97ed834102c2d094dfd7aaa6
walters@toolbox ~/s/linux> 

echo 0 > ${CAN_SETFILES_STAMP}
fi
fi
grep 1 ${CAN_SETFILES_STAMP}
Copy link
Member

Choose a reason for hiding this comment

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

grep -q to avoid unnecessary output.

fatal() {
err "$@"
exit 1
}
Copy link
Member

Choose a reason for hiding this comment

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

At some point we should get these basics into dracut-lib.sh or so.

err " e.g.: $0 /etc/passwd '/etc/group*'"
fi

source /sysroot/etc/selinux/config
Copy link
Member

Choose a reason for hiding this comment

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

Maybe silently do nothing if that file doesn't exist, to be nicer to people who want to build custom systems without SELinux? There's a similar pattern with other tools like restorecon.

@cgwalters
Copy link
Member

cgwalters commented Dec 2, 2019

Reusing this for a tangentially related q; remind me again why we don't load policy from the initramfs?

The complexity of handling #184 is much higher than /var (this PR) - basically if we have policy loaded we can rely on "default" labels being created.

Although, maybe a crude hammer would be a patch for setfiles that only labeled unlabeled files - we can rely on ostree to set the contexts for objects. It's just things like the repo/config, the object directories etc. that I don't quite want to patch ostree to explicitly set.

@jlebon
Copy link
Member Author

jlebon commented Dec 2, 2019

Is that in a Fedora kernel build yet? It looks like it's not in an upstream release yet:

Yeah, it was backported in https://bugzilla.redhat.com/show_bug.cgi?id=1758597 --> https://src.fedoraproject.org/rpms/kernel/c/8bb21014a55f50b1a734322639fffadafcd79672?branch=f31

Reusing this for a tangentially related q; remind me again why we don't load policy from the initramfs?

See discussions with Lennart in coreos/ignition#635, starting from coreos/ignition#635 (comment).

Although, maybe a crude hammer would be a patch for setfiles that only labeled unlabeled files - we can rely on ostree to set the contexts for objects.

OK yeah, I think this may be a problem. The issue is that while we can set labels, we can't read them back. So restoring the rootfs using mv as is done in #184 will likely not copy back the labels correctly. I'm looking into loosening this too and making sure we can get #184 to work. To compare, this would work if it were a bare-user or archive repo (since OSTree wouldn't have to actually call getxattr for security.selinux). (I guess we could also get the labels from the policy in the OSTree commit, but... yuck, there's potential for corruption there.)

Now that the kernel supports relabeling from the initrd, we can clean up
some hacks here. After running `systemd-tmpfiles`, immediately relabel
what we just wrote using `setfiles`. This allows us to drop the `Z /var`
tmpfiles.d dropin, and hacking around `systemd-random-seed.service`
ordering.

For compatibility with RHCOS, I added a fallback in `coreos-relabel` so
that if initrd relabeling isn't supported, we just use tmpfiles.d
dropins. (RHCOS today sources the `05core` overlay directly, and I think
doing it this way is cleaner than splitting things out into a separate
overlay).

I've also snuck in a fix for the live case with persistent `/var` so we
don't always relabel everything in there.
@jlebon jlebon force-pushed the pr/relabel-initrd branch from 6fcc31d to 9cbcd3a Compare December 3, 2019 14:19
@jlebon
Copy link
Member Author

jlebon commented Dec 3, 2019

Fixups! ⬆️

@cgwalters
Copy link
Member

cgwalters commented Dec 5, 2019

To compare, this would work if it were a bare-user or archive repo (since OSTree wouldn't have to actually call getxattr for security.selinux).

But that would break the scenario where we're not replacing the rootfs.

@jlebon
Copy link
Member Author

jlebon commented Dec 5, 2019

But that would break the scenario where we're not replacing the rootfs.

I meant if the source repo we're using to replace the rootfs wasn't bare (e.g. pulled over the network or something). Not that the embedded OSTree itself would be. :) I was more expanding on why it's problematic rather than suggesting a change.

@cgwalters
Copy link
Member

The issue is that while we can set labels, we can't read them back. So restoring the rootfs using mv as is done in #184 will likely not copy back the labels correctly. I'm looking into loosening this too and making sure we can get #184 to work.

Ah, yes. So we need another kernel patch.

@cgwalters cgwalters merged commit 0169fb4 into coreos:testing-devel Dec 5, 2019
jlebon added a commit to jlebon/ignition that referenced this pull request Dec 19, 2019
On OSTree systems, those are just symlinks in the deployment root. If
they're not labeled correctly already, it signals an issue with the disk
creation process itself (and might also signal that the next time a
deployment root is created, it'll also be mislabeled).

Anyway, even on non-OSTree systems, it seems reasonable to expect that
`/home` and `/root` at least already exist and don't need to be created
(and thus don't need to be relabeld).

It's possible that [fixing `getxattr` without a policy
loaded](coreos/fedora-coreos-config#245 (comment))
would also fix this, since `setfiles` would see that the symlinks were
already correctly labeled.

In effect, this is completing what coreos#632 started.

Closes: coreos/fedora-coreos-tracker#339
jlebon added a commit to jlebon/ignition that referenced this pull request Dec 20, 2019
On OSTree systems, those are just symlinks in the deployment root. If
they're not labeled correctly already, it signals an issue with the disk
creation process itself (and might also signal that the next time a
deployment root is created, it'll also be mislabeled).

Anyway, even on non-OSTree systems, it seems reasonable to expect that
`/home` and `/root` at least already exist and don't need to be created
(and thus don't need to be relabeld).

It's possible that [fixing `getxattr` without a policy
loaded](coreos/fedora-coreos-config#245 (comment))
would also fix this, since `setfiles` would see that the symlinks were
already correctly labeled.

In effect, this is completing what coreos#632 started.

Closes: coreos/fedora-coreos-tracker#339
@jlebon jlebon deleted the pr/relabel-initrd branch April 23, 2023 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants