Bug 842307

(In reply to comment #0)
> up SELinux to label these paths so that sockets created by libguestfs
> under here are 

... labeled automatically.

Why not the other way around?

/var/lib/libvirt/libguest and ~/.libvirt/libguestfs?

This would just work with current labeling, Any reason these directories should be owned by libguest?  By the way it would be easy to add the alternative, but I 
would label them the same.

Also I think the desktop team wants to move away from creating new content in ~/ and move it to places like /run/user/3267 and maybe /run/libvirt/libguest

libguestfs

I don't think I really care where the directory is located.  danpb?

If we add /var/lib/libvirt/libguestfs then that would require a
trivial change to libvirt.spec.

I'm less clear about ~/.libvirt (or ~/.cache/libvirt as it now
appears to be called).  Do I just mkdir a directory under
here and go?

Furthermore I still don't think that SELinux is the whole story.
We've still got the problem that qemu runs as qemu.qemu.  In the
case where non-root libguestfs talks to root libvirtd how do I know
which path to use?  Can non-root libguestfs create the socket in
/var?

My suggestion is that libguest use its own directory, so it doesn't have to deal with the fact that libvirt has moved its own directories.

IMHO, non-root libguestfs shouldn't talk to a root libvirtd - it should always use the session instance, thus avoiding this problem.

If these are just temporary sock files they should be created in /run, no reason to have them stick around in the homedir or /var/lib

Yes, they are temporary in all cases.

Ok lets go with

$XDG_RUNTIME_DIR/libguestfs
and
/var/run/libguestfs

And I will setup SELinux labeling for these.

Thanks.

Should that be /run or /var/run?  (I know /var/run is a symlink
to /run, just not clear which one I should be writing to)

Also this change only needs to be made in Rawhide.

If your code is only going to run on systemd machines, then /run should be fine.  I am adding

/var/run/libguestfs(/.*)?      gen_context(system_u:object_r:virt_var_run_t,s0)

/var/run/user/[^/]*/libguestfs(/.*)?   gen_context(system_u:object_r:virt_home_t,s0)


Since upstream policy requires /var/run and we have a rule in policy that states everything under /run == /var/run

I've pushed this upstream into libguestfs:
https://github.com/libguestfs/libguestfs/commit/493e80ab4e7199bdbb44b16535448d65944eca66

Note that sVirt confinement is not yet enabled (waiting for
this change to go into selinux-policy + further testing).

Did this change get into selinux-policy yet?

Since F18 has branched now, we'd really like it in
F18 & Rawhide.

The current package has the labels.

matchpathcon /run/libguestfs
/run/libguestfs	system_u:object_r:virt_var_run_t:s0

 matchpathcon /run/user/1234/libguestfs
/run/user/1234/libguestfs	system_u:object_r:virt_home_t:s0

selinux-policy-3.11.1-7.fc18.noarch

So now we have two problems.

Running libguestfs as non-root gives this error:

libguestfs: [01268ms] create libvirt XML
libguestfs: libvirt XML:\n<?xml version="1.0"?>\n<domain type="kvm" xmlns:qemu="http://libvirt.org/schemas/domain/qemu/1.0">\n  <name>guestfs-7zf7a2nxn6o1tufh</name>\n  <memory unit="MiB">500</memory>\n  <currentMemory unit="MiB">500</currentMemory>\n  <cpu model="host-model"/>\n  <vcpu>1</vcpu>\n  <clock offset="utc"/>\n  <os>\n    <type>hvm</type>\n    <kernel>/home/rjones/d/libguestfs/.guestfs-1000/kernel.17570</kernel>\n    <initrd>/home/rjones/d/libguestfs/.guestfs-1000/initrd.17570</initrd>\n    <cmdline>panic=1 console=ttyS0 udevtimeout=600 no_timer_check acpi=off printk.time=1 cgroup_disable=memory root=/dev/sdb selinux=0 guestfs_verbose=1 TERM=xterm </cmdline>\n  </os>\n  <on_reboot>destroy</on_reboot>\n  <devices>\n    <controller type="scsi" index="0" model="virtio-scsi"/>\n    <disk device="disk" type="file">\n      <source file="/tmp/libguestfs-test-tool-sda-RJeljg"/>\n      <target dev="sda" bus="scsi"/>\n      <driver name="qemu" type="raw"/>\n      <address type="drive" controller="0" bus="0" target="0" unit="0"/>\n    </disk>\n    <disk type="file" device="disk">\n      <source file="/home/rjones/d/libguestfs/.guestfs-1000/root.17570"/>\n      <target dev="sdb" bus="scsi"/>\n      <driver name="qemu" type="raw" cache="unsafe"/>\n      <address type="drive" controller="0" bus="0" target="1" unit="0"/>\n    </disk>\n    <serial type="unix">\n      <source mode="connect" path="/home/rjones/d/libguestfs/libguestfsNGj89u/console.sock"/>\n      <target port="0"/>\n    </serial>\n    <channel type="unix">\n      <source mode="connect" path="/home/rjones/d/libguestfs/libguestfsNGj89u/guestfsd.sock"/>\n      <target type="virtio" name="org.libguestfs.channel.0"/>\n    </channel>\n  </devices>\n  <qemu:commandline>\n    <qemu:arg value="-set"/>\n    <qemu:arg value="drive.drive-scsi0-0-1-0.snapshot=on"/>\n  </qemu:commandline>\n</domain>\n
libguestfs: [01268ms] launch libvirt guest
libguestfs: error: could not create appliance through libvirt: unable to set security context 'system_u:object_r:virt_content_t:s0' on '/home/rjones/d/libguestfs/.guestfs-1000/kernel.17570': Operation not permitted [code=38 domain=24]

It seems like libvirt tries to relabel files, specifically the
<kernel/> file, but cannot do that because it's not running as root.

There's a second problem that occurs as root, but that might not
be a real problem so I'll leave it for now.

> libguestfs: error: could not create appliance through libvirt: unable to set security context 'system_u:object_r:virt_content_t:s0' on '/home/rjones/d/libguestfs/.guestfs-1000/kernel.17570': Operation not permitted [code=38 domain=24]

> It seems like libvirt tries to relabel files, specifically the
> <kernel/> file, but cannot do that because it's not running as root.

As non-root we're allowed to relabel any files that the user has existing permission for. 

The issue in this case was that the kernel was a symlink to a file in /boot, which is owned by root. This can be dealt with by copying the kernel instead of symlinking.

(In reply to comment #13)
> The current package has the labels.
> 
> matchpathcon /run/libguestfs
> /run/libguestfs	system_u:object_r:virt_var_run_t:s0
> 
>  matchpathcon /run/user/1234/libguestfs
> /run/user/1234/libguestfs	system_u:object_r:virt_home_t:s0
> 
> selinux-policy-3.11.1-7.fc18.noarch

I think we were wrong about this, and this piece of policy
is not necessary and should be reversed.

I think it's libvirt's job to label these sockets correctly,
just as it already does with disk drives.  It's a bug in
libvirt that it doesn't do so (bug 853393).  I've sent
a patch to libvir-list which fixes this bug.

Now there is one remaining piece of policy missing to make
the whole thing work, but I've opened a separate bug about
that: bug 857453.

Sorry for the runaround with this bug, but did the
policy added (comment 13) get removed?

No but I am not sure how those directories should be labeled.

We're not going to create those directories at all.

Instead the plan is (bug 857453 - thanks for fixing it so quickly)
to just create ordinary sockets in our regular temp directory,
and have libvirt label them.  This matches how libvirt labels
other things as well, so it seems more natural.

Fixed in selinux-policy-3.11.1-23.fc18.noarch

FYI I noticed that we were not actually using the {/var,}/run/libguestfs
directory anywhere, so I have removed it from the spec file.  I don't
know if it still exists in selinux policy, but likely it can
be removed.