Apparmor prevents using storage pools and hostdev networks

Hi Simon,
thanks for your report - so we did not get far enough with bug 1641618 which only solved things for direct zvols, but not for disks from a pool.

I'm afraid there might be no part generating that yet in the aa-helper, but I'll look into it and report back here once I know more details.

Extending your already good testcase description:

# create a simple guest
 $ sudo apt-get install uvtool-libvirt zfsutils-linux
 $ uvt-simplestreams-libvirt --verbose sync --source http://cloud-images.ubuntu.com/daily arch=amd64 label=daily release=xenial
 $ ssh-keygen
 $ uvt-kvm create --password=ubuntu testguest release=xenial arch=amd64 label=daily
# create a zpool to use
 $ for i in $(seq 1 3); do dd if=/dev/zero of=/tmp/fdisk${i} bs=1M count=1024; done
 $ sudo zpool create internal /tmp/fdisk*
# make pool in libvirt and guest disk foo
 $ virsh pool-define-as internal zfs
 $ virsh pool-start internal
 $ virsh vol-create-as internal foo 2G
# link up zpool, by adding this to the guest
    <disk type='volume' device='disk'>
      <driver name='qemu' type='raw' cache='none'/>
      <source pool='internal' volume='foo'/>
      <target dev='vdc' bus='virtio'/>
    </disk>
# start the guest
$ virsh start testguest

All run into:
Could not open '/dev/zvol/internal/foo': Permission denied

And I can see the reported Deny:
apparmor="DENIED" operation="open" [...] name="/dev/zd0" [...]

That said setting to confirmed for now.
Also I checked this applies to all of releases X-Z.

Need to dive into aa-helper how close or far that is as of today to get this done.

These are mostly notes to remember later:
In such a setup the pool definition has the base pool path

$ virsh pool-dumpxml internal
<pool type='zfs'>
  <name>internal</name>
  <uuid>5e83970c-dc95-41af-bd10-9d9001dc9cba</uuid>
  <capacity unit='bytes'>3170893824</capacity>
  <allocation unit='bytes'>2147573760</allocation>
  <available unit='bytes'>1023320064</available>
  <source>
    <name>internal</name>
  </source>
  <target>
    <path>/dev/zvol/internal</path>
  </target>
</pool>

The volume holds the respective subvol foo

$ virsh vol-dumpxml --pool internal foo
<volume type='block'>
  <name>foo</name>
  <key>/dev/zvol/internal/foo</key>
  <source>
  </source>
  <capacity unit='bytes'>2147483648</capacity>
  <allocation unit='bytes'>2147483648</allocation>
  <target>
    <path>/dev/zvol/internal/foo</path>
  </target>
</volume>

And confimring as well that /etc/apparmor.d/libvirt/libvirt-<UUID>.files has no references to either /dev/zvol/internal/foo nor the symlink target.

Apparmor can't by design follow symlinks (https://bugs.launchpad.net/apparmor/+bug/1485055).
So test-inserting into /etc/apparmor.d/abstractions/libvirt-qemu:
- /dev/zvol/internal/foo rw, => still fails
- /dev/zd0 rw, => works (guest sees disk as expected)
So does any generic rule.

So the following might serve as a temporary workaround adding "/dev/zd[0-9]* rw" to /etc/apparmor.d/abstractions/libvirt-qemu.
Simon I'm sure you had that already, but this is for whoever else comes by.

I see that this needs dev-activity -> upstream-libvirt -> merge new libvirt -> SRUs so I wanted to provide some sort of workaround.

TODO:
- get aa-helper to consider pool zvols
- resolve symlink as we need the target in the rule

Hello Christian,

On 2017-03-30 06:18 AM, ChristianEhrhardt wrote:
> So the following might serve as a temporary workaround adding "/dev/zd[0-9]* rw" to /etc/apparmor.d/abstractions/libvirt-qemu.

What I did something similar but less convenient. My goal was to keep
the per-VM isolation so I added the corresponding "/dev/zdXX rw" rule to
the /etc/apparmor.d/libvirt/libvirt-$uuid file and reload that profile.

> I see that this needs dev-activity -> upstream-libvirt -> merge new
> libvirt -> SRUs so I wanted to provide some sort of workaround.

Yes, makes sense and your workaround is easier. Having this eventually
land in a SRU would be greatly appreciated.

> TODO:
> - get aa-helper to consider pool zvols
> - resolve symlink as we need the target in the rule

That is correct, Apparmor always operate on the destination file. There
should already be code in aa-helper to track down the destination file
as I assume the situation is pretty similar to that of LVM.

As always, thanks for the precise problem dissection and fast response!

FYI - I haven't forgotten, just flooded with other load

Damn it seems I can't find the hours yet - I really beg your pardon Simon as I like you as an active community member! But I also have PTO next week and I'm not sure I get to it before that.
I'll assign to myself to not forget about it.

No worries, I have a good feeling of how busy you are from the bug
notifications I get. Knowing that you will look into it is already a
great deal, so thanks again.

On 2017-04-05 03:33 AM, ChristianEhrhardt wrote:
> Damn it seems I can't find the hours yet - I really beg your pardon Simon as I like you as an active community member! But I also have PTO next week and I'm not sure I get to it before that.
> I'll assign to myself to not forget about it.
>
> ** Changed in: libvirt (Ubuntu Zesty)
> Assignee: (unassigned) => ChristianEhrhardt (paelzer)
>
> ** Tags added: server-next
>

I don't seem to find time shortly, so I drop server-next tag.
But I ensured it is tracked to remind me and make me feel bad :-/

Back on this, currently trying to build up a case where this can be tested from git (had some obstacles):
- In the dev system (from local dir) not all apparmor rules apply
- In a container the zfs actions are not all possible
- So we need a KVM driving a 2nd-level KVM for all of this.

0. get a multi-cpu KVM guest with build env

1. normal uvtool based guest in there
2. prep zfs as outlined in c#3
4. check if bug triggers and confinement is active
   $ sudo aa-status | grep -E 'libv|qemu'
5. share the repo dir
    <filesystem type='mount' accessmode='passthrough'>
      <source dir='/home/paelzer/work/libvirt/libvirt-upstream-git-root'/>
      <target dir='libvirt-git'/>
    </filesystem>
   And then in guest:
   $ sudo mkdir -p /home/paelzer/work/libvirt/libvirt-upstream-git-root
   $ sudo mount -t 9p -o trans=virtio libvirt-git /home/paelzer/work/libvirt/libvirt-upstream-git-root
5. switch to locally built repo
   (built on host and used in guest as root), install into the system
   $ sudo make install
6. check you have the new version
7. Check contained aa status
   $ sudo aa-status | grep -E 'libv|qemu'
8. check the bug still triggers running from that

That is close to a ppa build and install being easier :-)
It is also easier to retest for others on the bug and more reliable to catch the way will work in Ubuntu.
So while (somehwat) working gogin on with local dev and then shoving it onto test systems through a ppa build.

Initially this can be run locally just with virt-aa-helper as any virt-aa-helper dev.
This mostly is like c#3, but with a service from local build and with libvtool wrapper to gdb the virt-aa-helper.

Since even this is not the most straight forward thing if you never done it here a short log how to do so:

 $ sudo ./src/virtlockd -f /etc/libvirt/virtlockd.conf -d
 $ sudo ./src/virtlogd -f /etc/libvirt/virtlogd.conf -d
 $ sudo ./daemon/libvirtd -f /etc/libvirt/libvirtd.conf -d
 # an xml containing the snippet of c#3
    <disk type='volume' device='disk'>
      <driver name='qemu' type='raw' cache='none'/>
      <source pool='internal' volume='foo'/>
      <target dev='vdc' bus='virtio'/>
    </disk>
 $ ./tools/virsh (all you need to set up the pool as in c#3)
 # run like:
 $ ./src/virt-aa-helper --lt-debug --create --dryrun --uuid 'libvirt-e2f807e2-4ee6-496f-b3cc-926b7c7cefb3' < ../bug-1677398-zfs-pool/test1.xml
 # debug like:
 $ libtool --mode=execute gdb ./src/virt-aa-helper
 (gdb) run --create --dryrun --uuid 'libvirt-e2f807e2-4ee6-496f-b3cc-926b7c7cefb3' < ../bug-1677398-zfs-pool/test1.xml

As we knew the arg to qemu uses the link like:
  -drive file=/dev/zvol/internal/foo,format=raw
But we need the base device, so in virt-aa-helper we need to:
1. the volume is a disk entry (not an FS), so we need to get the volume entry from the guest xml
2. from pool+volume get to the real path (like the -drive arg construction would do)
3. need to readlink to the link target.

When the guest gets parsed all that virt-aa-helper initially has is the disk element.
This has in our case only:
p *(disk->src->srcpool)
$10 = {pool = 0x100236b90 "internal", volume = 0x100236440 "foo", voltype = 0, pooltype = 0, actualtype = 0, mode = 0}

Nothing in virt-aa-helper yet cares about that - it does not deliver a "virDomainDiskGetSource" and thereby gets skipped.
But with that we would know pool and volume (step #1 above).

Next is getting the pool details from that, to do so we align to what the actual usage of the attribute does.
In virStorageTranslateDiskSourcePool is that flow which mostly is
- virStoragePoolLookupByName
  -> virStoragePoolIsActive (check if active)
  -> virStorageVolLookupByName (now one has the volume)
  -> virStorageVolGetPath

It seems designed to be safe to use virStorageTranslateDiskSourcePool, the overall pattern is to call it before handling the other attributes. It's return value is 0 if it was no srcpool disk anyway. If it is a sourcepool but it failed for any reason then it is -1.
That makes it a safe pre-call before running on the disk elements data like def->src->path whicih it sets.

That structurally seems to be what all other code does, so it might be wise to follow that.
The problem is that this was meant to be called with an active connection (first argument).
This can not be avoided by calling the sub-functions directly as this is needed in virStoragePoolLookupByName as the storage drivers it calls are in that struct.

virConnectOpenReadOnly and co would initialize the connection, but they would be env specific.
Also as it seems the header is meant for users of libvirt, but not the local tools.
I'll report back when I found the proper initialization (e.g. the empty one via virGetConnect works but is obviously uninitialized and rejects the lookup method).

I'm not sure if upstream likes that virt-aa-helper now needs to talk to the socket, but they can suggest otherwise if they don't.
I now have something that calls the lookup correctly.
It then fails to find the pool:
  libvirt: Storage Driver error : Storage pool not found: no storage pool with matching name 'internal'

While at the sametime
$ ./tools/virsh pool-info internal
Name: internal
UUID: 9bdda23f-3410-4a2c-9f93-850308d19445
State: running

Going on here tmrw

After having slept on that for a night I consider that not a good way.
The current lifecycle of virt-aa-helper just doesn't match to create a local virConnectPtr context, yet OTOH without one we can't translate the pool as the code that spawns the guest would.

Before I implement some useless code to be nack'ed for its misconception I readed out to the libvirt devel mailing list outlining these issues and hope that feedback from there unblocks me.

Going on with another virt-aa-helper bug until the discussion resolved.

[1]: https://www.redhat.com/archives/libvir-list/2017-September/msg00557.html

I found that bug 1343245 is about the same general issue.
The descriptions in there were great, but since I started to document the debug and potential coding and more here I dupp'ed it onto here.
The thoughts there also already were around "how to get the translation done".

I changed title and description here - especially of the testcase - to now cover both.
This shall show that it is a more general issue.

On the bug itself still waiting for upstream feedback to my mail linked in c#16

In the same scope of required "out of context information" fall cases of vfio devices for hostdevs.
Those work fine if defined in the guest or added to the guest.

But if only referred by an interface like:
    <interface type='network'>
      <mac address='3a:73:83:14:99:0e'/>
      <source network='pf-et0p0' portgroup='storage'/>
      <model type='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
    </interface>

And the definition being external like:
<network>
  <name>pf-et0p0</name>
  <uuid>c1415c6d-11d7-417e-8113-ed5439e5ee44</uuid>
  <forward mode='hostdev' managed='yes'>
    <driver name='vfio'/>
    <pf dev='et0p0'/>
  </forward>
  <portgroup name='ext' default='yes'>
    <vlan>
      <tag id='30'/>
    </vlan>
  </portgroup>
  <portgroup name='lab2'>
    <vlan>
      <tag id='51'/>
    </vlan>
  </portgroup>
  <portgroup name='storage'>
    <vlan>
      <tag id='61'/>
    </vlan>
  </portgroup>
</network>

It fails.
As again in this case virt-aa-helper has no means yet to introspect the extra info needed to convert all this to paths.

TODO: retest these with the domain label callbacks implemented, maybe some of the devices/images might trigger that

I have the same problem using raw lvm logical volumes as disk on Ubuntu 18.04.
When i try to start a vm with virt-manager qemu says Permission denied on device.

The lvm uses device mapper to map the logical volumes so i need to handle devices like

brw-rw---- 1 libvirt-qemu kvm 253, 4 mar 17 13:24 /dev/dm-4

After a bunch of failure attempts to start the vm machine, the following steps was successful:

I run the aa-logprof (apparmor-utils package) util to update the apparmor profiles using the syslog, and
i add the following line in /etc/apparmor.d/abstractions/libvirt-qemu

...
  /dev/dm* krw,

just before the /dev/net/tun rw, line.

If the line already exists just replace "rw" with "krw" to permit file lock operations.

Now it seems vm runs well again.

Hi Ultrabit, can you please include the DENIED lines from your dmesg or auditd logs?

Thanks

Up, causing issues with terraform libvirt provider :/

The linked/original bug is quite old. Looks not like there is a quick solutions.

Looks like many people have that issue now with the terraform libvirt provider.
Even the default storage pool /var/lib/libvirt/images is not working.

audit: type=1400 audit(1553443109.481:44): apparmor="DENIED" operation="open" profile="libvirt-40112047-230a-4971-a3cf-341cb2c14402" name="/mnt/images/ubuntu-admin-qcow2" pid=49125 comm="qemu-system-x86" requested_mask="wr" denied_mask="wr" fsuid=64055 ouid=64055

What is the best quick hack for that right now?

Hi Nicolas,
yeah that isn't easy to fix and at least I didn't find the time to develop something completely new to cover this yet.

I challenge the statement "Even the default storage pool /var/lib/libvirt/images is not working", it does and it does well.
And for things that are under the control of Ubuntu in the Archive even a few alternative paths work (openstack, uvtool, ...).

The issue you report is -not- using the default paths, the Deny lists "/mnt/images/ubuntu-admin-qcow2" which clearly is not in one of the common paths.

In general for using uncommon paths [1] the solution is that an admin has to declare those paths as allowed in a local apparmor include. So if terraform would usually /a/b/c it should also either recommend the admin to do so or even consider adding it to the files itself.

[1]: https://wiki.ubuntu.com/LibvirtApparmor#Using_uncommon_paths

I've written a quick patch that seems to fix the storage pool side of this issue, at least for dir/fs like pool types. It does connect to the libvirtd socket; I saw that there was some concern with that approach earlier, but this solution didn't require any changes to virt-aa-helper's calling conventions. Are we still opposed to having virt-aa-helper connect to libvirtd?

A couple notes about what is needed to use this, which might be obvious to those more experienced with libvirtd than myself:
* You may need to use a common file extension for the AppArmor profile to allow virt-aa-helper itself to inspect your image files.
* If you also need the AppArmor profile to allow read access to a backing file chain, your domain will need to have a driver element in the disk device definition, with the type attribute set appropriately.

The attachment "virt-aa-helper-support-pools.patch" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

Hi Garry,
why thanks for the patch - I was missing that update earlier in the year and beg your pardon.

#1
Why did you drop the "load the storage driver so that backing store can be accessed" section - that will continue to be needed at least for other storage types so I wonder why this was dropped.

#2
/etc/libvirt/libvirt.conf usually only defins URIs, that isn't needed unless we have a change that starts to have virt-aa-helper to re-query libvirt (and even then there might be multiple libvirts running and you can't know it is "this config file").
Will that be read by the virGetConnectStorage call in virDomainDiskTranslateSourcePool of your diff?

#3 around virDomainDiskTranslateSourcePool
I see, interesting approach.
So you'd want to translate the source pool into the disk struct.
And if it isn't a pool it will be an early exit with RC=0 and go on as usual
Do you have an example from the disk structure content from before/after your changes?

I have subscribed Garry to increase the chance he is seeing and replying to my questions?

FYI - dir based pools already work if they are in a common and expected path like seen in
/etc/apparmor.d/usr.lib.libvirt.virt-aa-helper

  @{HOME}/ r,
  @{HOME}/** r,
  /var/lib/libvirt/images/ r,
  /var/lib/libvirt/images/** r,
  # nova base images (LP: #907269)
  /var/lib/nova/images/** r,
  /var/lib/nova/instances/_base/** r,
  # nova snapshots (LP: #1244694)
  /var/lib/nova/instances/snapshots/** r,
  # nova base/snapshot files in snapped nova (LP: #1644507)
  /var/snap/nova-hypervisor/common/instances/_base/** r,
  /var/snap/nova-hypervisor/common/instances/snapshots/** r,
  # eucalyptus (LP: #564914)
  /var/lib/eucalyptus/instances/**/disk* r,
  # eucalyptus loader (LP: #637544)
  /var/lib/eucalyptus/instances/**/loader* r,
  # for uvtool
  /var/lib/uvtool/libvirt/images/** r,
  # for multipass
  /var/snap/multipass/common/data/multipassd/vault/instances/** r,
  /{media,mnt,opt,srv}/** r,
  # For virt-sandbox
  /{,var/}run/libvirt/**/[sv]d[a-z] r,

If you need to run out of a more uncommon path you just need to add yours to
  /etc/apparmor.d/local/usr.lib.libvirt.virt-aa-helper
(more at https://ubuntu.com/server/docs/virtualization-libvirt)

That will allow virt-aa-helper to track these paths and add rules as needed.
This is working for various common use cases as uvtool or nova already.

Never the less I find it interesting to "see what happens" if calling into virDomainDiskTranslateSourcePool so if you have some time please consider answering my questions above.

I was giving this a try
PPA: https://launchpad.net/~paelzer/+archive/ubuntu/lp-1677398-pool-experiment-groovy

Changes:
- Add a silly sleep to catch it in flight more easily
- add the core elment of the patch around virDomainDiskTranslateSourcePool
- build without optimization
- install related debug symbols and gdb

Then run:
$ while ! pidof virt-aa-helper; do sleep 0.01; done; gdb -p $(pidof virt-aa-helper)
And execute a start of the guest set up as in this bug description

Indeed the read to /etc/libvirt/libvirt.conf is from the call to virDomainDiskTranslateSourcePool as I have assumed above.

[ 628.266012] audit: type=1400 audit(1590487555.258:74): apparmor="DENIED" operation="open" profile="virt-aa-helper" name="/etc/libvirt/libvirt.conf" pid=3683 comm="virt-aa-helper" requested_mask="r" denied_mask="r" fsuid=0 ouid=0

But in the long run we can't rely on either libvirt.conf nor anything else - as there are many places that can define the connection URL. Like ENV overrides and such, there might even be multiple libvirts running, so we can't just trial&error through the usual paths.

But for now on these experiments I'll allow that access.

Iterating over the usual disks into the pools.
(gdb) p *disk->src->srcpool
$9 = {pool = 0x565040c0a590 "internal", volume = 0x565040c09ad0 "foo", voltype = 0, pooltype = 0, actualtype = 0, mode = 0}
(gdb) p *disk->src->srcpool
$11 = {pool = 0x565040c093d0 "testvg", volume = 0x565040c09650 "guest1", voltype = 0, pooltype = 0, actualtype = 0, mode = 0}

And then e.g. in my case the default connection fails.
31231 virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def)
...
31246 if (!(conn = virGetConnectStorage()))

Due to that it returns -1 and we can't get any extra info from the disk

Inside virGetConnectGeneric I see that it can't get a existing cached connection via
145 conn = virThreadLocalGet(threadPtr);
(gdb) p conn
$2 = (virConnectPtr) 0x0

That was expected, then it probes via geteuid and since virt-aa-helper runs like libvirtd as root decides to use
(gdb) p uri
$7 = 0x55a1b10bcce0 "storage:///system"

Then it tries to open this, the backtrace up to here looks like
#0 virConnectOpenInternal (name=0x55a1b10bcce0 "storage:///system", auth=0x0, flags=0) at ../../../src/libvirt.c:820
#1 0x00007f014f83fbb4 in virConnectOpen (name=name@entry=0x55a1b10bcce0 "storage:///system") at ../../../src/libvirt.c:1076
#2 0x00007f014f83b3dd in virGetConnectGeneric (threadPtr=<optimized out>, name=0x7f014f9081a2 "storage") at ../../../src/driver.c:156
#3 0x00007f014f7069c5 in virDomainDiskTranslateSourcePool (def=0x55a1b1074cc0) at ../../../src/conf/domain_conf.c:31246
#4 0x000055a1b0d1552a in get_files (ctl=0x7ffe043579e0) at ../../../src/security/virt-aa-helper.c:951
#5 vahParseArgv (argv=<optimized out>, argc=<optimized out>, ctl=0x7ffe043579e0) at ../../../src/security/virt-aa-helper.c:1442
#6 main (argc=<optimized out>, argv=<optimized out>) at ../../../src/security/virt-aa-helper.c:1492

With these arguments:
Breakpoint 4, virConnectOpenInternal (name=0x55a1b10bcce0 "storage:///system", auth=0x0, flags=0) at ../../../src/libvirt.c:820

(here is the libvirt.conf load BTW)

It tries via storage driver then:

(gdb) p virConnectDriverTab[0]->hypervisorDriver->name
$20 = 0x7f014b8438e4 "storage"
(gdb) p *(virConnectDriverTab[0]->uriSchemes)
$22 = 0x7f014b8438e4 "storage"

Init goes well and it calls into
(gdb) p virConnectDriverTab[0]->hypervisorDriver->connectOpen
$31 = (virDrvConnectOpen) 0x7f014b830490 <storageConnectOpen>

But that returns -2

(gdb) bt
#0 storageConnectOpen (conn=0x55a1b109f920, auth=0x0, conf=0x55a1b10bcce0, flags=0) at ../../../src/storage/storage_driver.c:397
#1 0x00007f014f83e822 in virConnectOpenInternal (name=<optimized out>, auth=0x0, flags=0) at ../../../src/libvirt.c:1000
#2 0x00007f014f83fbb4 in virConnectOpen (name=name@entry=0x55a1b10af4b0 "storage:///system") at ../../../src/libvirt.c:1076
#3 0x00007f014f83b3dd in virGetConnectGeneric (threadPtr=<optimized out>, name=0x7f014f9081a2 "storage") at ../../../src/driver.c:156
#4 0x00007f014f7069c5 in virDomainDiskTranslateSourcePool (def=0x55a1b1075050) at ../../../src/conf/domain_conf.c:31246
#5 0x000055a1b0d1552a in get_files (ctl=0x7ffe043579e0) at ../../../src/security/virt-aa-helper.c:951
#6 vahParseArgv (argv=<optimized out>, argc=<opti...

FYI after some debugging I was chatting with libvirt upstream and setting up a new storage context inside virt-aa-helper really won't work architecturally.

But I've found as part of the same discussion that there is a chance we can move the profile load a bit back until after qemuProcessPrepareDomainStorage has translated the paths and modify virDomainDiskSourceFormat to render the translated paths.

There are some constraints and I've sent a mail to Jdstrand about this to clarify before I write code for this.

Thanks for subscribing me; I'm a launchpad newbie and didn't realize that wasn't automatic when I posted here. I'll try to answer your questions to the best of my recollection.

#1
I can't remember exactly why I needed to drop the storage driver load; I think the local storage driver it started didn't have all the configuration I needed to resolve the disk paths (probably needed the pool config). If I remember correctly, after reading around the source code it looked like I could get it to connect to a remote storage driver automatically by eliminating the local one.

#2
I agree that we can't always know where libvirt.conf is, but here I allowed the default that was in use by my system to move forward. Other locations could be allowed, and a more general approach would be a good idea.

#3
I had inspected those structs using gdb when I was working on that patch, but don't have an example on-hand right now

My memory of why I was doing this is hazy at this point. I think I was trying to grant each VM access to only its own disk layers (so adding to the profile template does not accomplish this), and I needed virt-aa-helper to traverse a qcow image's backing chain to grant read access to the parents (which didn't work without resolving it's path).

I'll try to find time soon to look into the cause of the failure you mention. My patch did work for me and I've been starting VMs with the generated profiles, but it's very possible I broke something else. I don't have a ton of experience with libvirt yet.

Sorry if this is obvious, but it's been a while since I was working on this: what do you mean by "setting up a new storage context" in your last comment?

This is somewhat off topic, but why does the AppArmor security driver use a helper binary (virt-aa-helper)? If it's mostly historical reasons, would there be motivation to move the logic from the helper in-process similarly to the SELinux driver in the future? It seems like that might simplify accessing information of the sort we're talking about here (e.g. pool configs, hostdev network info).

Hi Garry,
thanks for your reply.

Q: what do you mean by "setting up a new storage context" in your last comment?
A: the code was not only trying to connect to libvirtd, by tracking in gdb I found that it was also trying to itself do some actions that would make virt-aa-helper behave like the backend in libvirtd.
So it would set up its own "context" that might differ from what libvirtd sees.

Q: why does the AppArmor security driver use a helper binary (virt-aa-helper)?
A: libvirt by the nature of what it needs to do needs a lot of capabilities.
   Splitting that in two would allow for two different profiles.
   I think it was meant to allow the virt-aa-helper profile to allow more and
   later restrict the libvirtd profile a bit more (hasn't happened yet).
   And it was meant to deal with manipulating AppArmor.
   Also this was 11 years ago, so yes it might be worth to revisit (I had the
   same thoughts, and to admit keep forgetting why I considered it impossible
   the last time)

As I mentioned above, I think the approach in your patch is wrong in the long run (causing too much other issues). But experimenting and discussing about it gave me some good alternative ideas.
I'm waiting for Jamie to reply to my mail and then hopefully can give it a try on a different implementation.

I finally had time to revisit this. It appears that not only did my patch not connect to the remote libvirtd storage driver (as Christian pointed out), but that the storage driver does not establish remote connections at all. As such, I agree with Christian that my patch is a dead-end and would also advise anyone finding this now to avoid applying it, even temporarily.

Has there been any forward movement on the other proposed solution?

I stumbled upon the same issue but with dir based pools. I have all relevant information posted on a SO question, do you want me to paste them here too? The question is here: https://stackoverflow.com/questions/63767647/virt-aa-helper-doesnt-add-path-for-storage-pool-in-apparmor-generated-rules

> I stumbled upon the same issue but with dir based pools. I have all
> relevant information posted on a SO question, do you want me to paste
> them here too?

Thanks George,
yeah this is another case where it would need to "talk back to the
storage subsystem" of libvirt to get info from the pools for the
base-path.
If you could just paste the XML and the Denial here that would help as
another testcase once it is implemented.

There is a comment in this bug that says that dir based pools aren't affected, but it seems they are. Here's the disk definition in XML using a "default" named pool that resides in /var/lib/libvirt/images.

<disk type='volume' device='disk'>
  <driver name='qemu' type='qcow2'/>
  <source pool='default' volume='awesome.qcow2'/>
  <target dev='vda' bus='virtio'/>
  <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/>
</disk>

Starting the VM with virsh start, yield the following error in dmesg:

[10757.098291] audit: type=1400 audit(1599423932.042:131): apparmor="DENIED" operation="open" profile="libvirt-b68582b8-0f35-4298-afd8-45c89ff3cbaa" name="/var/lib/libvirt/images/awesome.qcow2" pid=8654 comm="qemu-system-x86" requested_mask="r" denied_mask="r" fsuid=64055 ouid=64055

> There is a comment in this bug that says that dir based pools aren't
> affected, but it seems they are.

Should probably be more like:
Dir based with full path work, Dir based with just filename needs path
from the pool and fails.

> Here's the disk definition in XML using
> a "default" named pool that resides in /var/lib/libvirt/images.

Thanks!

Hey guys,

I'm trying to play with Terraform and it's failing too! Ubuntu 20.04.1.

Guide:

https://fabianlee.org/2020/02/22/kvm-terraform-and-cloud-init-to-create-local-kvm-resources/

NOTE: I'm using the latest "terraform-provider-libvirt-0.6.3" binary for Ubuntu 20.04.

The "terraform apply" fails with:

Error: Error creating libvirt domain: virError(Code=1, Domain=10, Message='internal error: process exited while connecting to monitor: 2020-11-17T01:55:34.375538Z qemu-system-x86_64: -blockdev {"driver":"file","filename":"/var/lib/libvirt/images/simple-os_image","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}: Could not open '/var/lib/libvirt/images/simple-os_image': Permission denied')

The generated XML looks just like the one from comment #39!

As you guys can see, the path is "normal". The file permissions are accessible to Libvirt users/groups. The QEMU host is working normally with virt-manager.

Any quick fix!?

I found the quick "fix"...

"Double check that `security_driver = "none"` is uncommented in `/etc/libvirt/qemu.conf` and issue `sudo systemctl restart libvirt-bin` to restart the daemon."

https://github.com/dmacvicar/terraform-provider-libvirt/commit/22f096d9

Doesn't sound good. But it worked! lol

I hit this issue today when trying `terraform-provider-libvirt`. So I spent some time debugging it. Below are my findings.

1. Dir-based pools are affected. I didn't tried with default one as I created custom storage pool in terraform (`/srv/libvirt/images`). Then I was able to catch `/etc/apparmor.d/libvirt/libvirt-XXXX*` files and check their contents. There was no `/srv/libvirt/images` path so it's definitely was not added there.

2. Then I found `/**.qcow{,2} r,` in the `/etc/apparmor.d/usr.lib.libvirt.virt-aa-helper` file. So my image should be allowed by this rule. But I was still unable to create VM. I tried to add my path here without any success though.

3. Next thing to try was `/etc/apparmor.d/libvirt/TEMPLATE.qemu`. I added my pool path there AAAAND it works after. So this is what I have in TEMPLATE.qemu file now:

```
#
# This profile is for the domain whose UUID matches this file.
#

#include <tunables/global>

profile LIBVIRT_TEMPLATE flags=(attach_disconnected) {
  #include <abstractions/libvirt-qemu>
  # Allow access to custom storage pool
  "/srv/libvirt/images/" r,
  "/srv/libvirt/images/**" rwk,
}
```

Now those paths appears in libvirt/libvirt-XXXX file and access is finally allowed.

Please let me know if there is better way to do this.

Hi Yury,
until implemented for real adding apparmor rules for the uncommon paths are the way to go.
The difference I'd suggest to your solution is to use local overrides since they will neither prompt you nor be overwritten on updates.

This can be done in:
# allow virt-aa-helper to generate per-guest rules in an uncommon path
/etc/apparmor.d/local/usr.lib.libvirt.virt-aa-helper
# allow things for an individual guests
/etc/apparmor.d/libvirt/libvirt-<uuid>
# allow something for all guests
/etc/apparmor.d/local/abstractions/libvirt-qemu

In the particular case the best way should be an entry like
   /srv/libvirt/images/** r,
in /etc/apparmor.d/local/usr.lib.libvirt.virt-aa-helper

That is especially good since each individual guest will still only get rules added to allow "his own storage" as configured in the guest XML.
In your solution as comparison an exploited guest A could access the storage of guest B.

This now has a related upstream issue https://gitlab.com/libvirt/libvirt/-/issues/135

Also check bug 1573192 if it might be resolved by this as well.