Using disk type file instead of type volume would fix apparmor permissions

[electrofelix]

Version Reports:

Distro version of host:

Ubuntu 16.04

Terraform Version Report

Terraform v0.8.8

Libvirt version

1.3.1

terraform-provider-libvirt plugin version (git-hash)

commit a125a6cf000c9fb3300d2d80afcfb1b13b2952b3
Merge: 785ae79 339552a
Author: Alvaro <alvaro.saurin@gmail.com>
Date:   Mon Feb 20 11:44:33 2017 +0100

    Merge pull request #101 from rsokolkov/fix-docs-for-network
    
    [Docs] Clearer description for "addresses" param

---

Description of Issue/Question

Use of disk type volume in the domain XML definition prevents virt-aa-helper from being able to determine the correct file path that should be added to the auto generated apparmor profile. This leads to the requirement to disable apparmor for libvirt which is not always possible in every environment (e.g. no sudo access).

Consequently you'll receive the following message when trying to boot without first setting security = "none" in /etc/libvirt/qemu.conf:

Error applying plan:

1 error(s) occurred:

* libvirt_domain.bootserver: Error creating libvirt domain: [Code-1] [Domain-10] internal error: process exited while connecting to monitor: 2017-05-18T10:15:33.850493Z qemu-system-x86_64: -drive file=/var/lib/libvirt/images/volume-test,format=qcow2,if=none,id=drive-virtio-disk0: Could not open '/var/lib/libvirt/images/volume-test': Permission denied

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.

Looking at the domain XML generated and comparing it to a known working vagrant VM that was working with apparmour enabled highlighted the following differences about the disk definition:

terraform libvirt provider generated disk XML from domain

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

vagrant libvirt provider generated disk XML from domain

    <disk type='file' device='disk'>
      <driver name='qemu' type='qcow2'/>
      <source file='/var/lib/libvirt/images/vagrant-libvirt_default.img'/>
      <target dev='vda' bus='sata'/>
      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
    </disk>

Using this information then lead to the following issue:
https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1343245

While it might be possible for future releases on ubuntu to include a version of virt-aa-helper that will know to look at the volume and pool information in order to resolve the disk image path and add that to the apparmor rules, it seems like it might be useful for this provider to switch the XML generated for the disks used to use a format that is better understood by virt-aa-helper and thus remove the need to disable security on qemu when using.

---

Additional Infos:

Apparmor was enabled in order to debug why the provider didn't work with it, when the vagrant-libvirt provider works.

[litewhatever]

Please re-open as this has re-appeared after 77982e7

[blaggacao]

In my case, contrary to https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1343245, an app-armor rule seems to be created:

# DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT.
  "/var/log/libvirt/**/k8s0.log" w,
  "/var/lib/libvirt/qemu/domain-k8s0/monitor.sock" rw,
  "/var/lib/libvirt/qemu/domain-2-k8s0/*" rw,
  "/var/run/libvirt/**/k8s0.pid" rwk,
  "/run/libvirt/**/k8s0.pid" rwk,
  "/var/run/libvirt/**/*.tunnelmigrate.dest.k8s0" rw,
  "/run/libvirt/**/*.tunnelmigrate.dest.k8s0" rw,
  "/var/lib/libvirt/images/commoninit.iso" rk,
  "/var/lib/libvirt/images/rancher-os" rk,
  "/var/lib/libvirt/images/k8s0.qcow2" rwk,   # HERE
  "/dev/vhost-net" rw,

However the volumes are created as root:

total 133056
drwxrwxrwx+ 2 libvirt-qemu kvm       4096 Mar 11 11:47 .
drwxr-xr-x  7 root         root      4096 Mar  3 22:47 ..
-rw-r--r--+ 1 libvirt-qemu kvm     372736 Mar 11 11:47 commoninit.iso
-rw-r--r--+ 1 root         root    196616 Mar 11 11:47 k8s0.qcow2
-rw-r--r--+ 1 root         root    196616 Mar 11 11:47 k8s1.qcow2
-rw-r--r--+ 1 root         root    196616 Mar 11 11:47 k8s2.qcow2
-rw-r--r--+ 1 libvirt-qemu kvm  135266304 Mar 11 11:47 rancher-os

With the following config:

# We fetch the latest rancheros release image from their mirrors
resource "libvirt_volume" "ros" {
  name   = "rancher-os"
  source = "https://github.com/rancher/os/releases/download/v1.5.1/rancheros.iso"
  format = "iso"
}
resource "libvirt_volume" "node" {
  name           = "k8s${count.index}.qcow2"
  base_volume_id = "${libvirt_volume.ros.id}"
  count          = "${var.workers_count}"
}

[blaggacao]

I tried with:

service apparmor stop
service apparmor teardown

but got the same:

total 133060
drwxrwxrwx+ 2 libvirt-qemu kvm       4096 Mar 11 12:33 .
drwxr-xr-x  7 root         root      4096 Mar  3 22:47 ..
-rw-r--r--+ 1 libvirt-qemu kvm     372736 Mar 11 12:33 commoninit.iso
-rw-r--r--+ 1 root         root    196616 Mar 11 12:33 k8s0.qcow2
-rw-r--r--+ 1 root         root    196616 Mar 11 12:33 k8s1.qcow2
-rw-r--r--+ 1 root         root    196616 Mar 11 12:33 k8s2.qcow2
-rw-r--r--+ 1 libvirt-qemu kvm  135266304 Mar 11 12:33 rancher-os

[electrofelix]

@blaggacao I think it might be more useful to see the domain XML from virsh dumpxml <node>, I can compare it to what works from machines created with vagrant-libvirt/vagrant-libvirt which is known to work with apparmor, and use that as a baseline to help give some guesses as to what has changed here.

[blaggacao]

Indeed:

<domain type='kvm'>
  <name>k8s0</name>
  <uuid>86b4d04b-d060-4ebd-aec2-a2fba32116f5</uuid>
  <memory unit='KiB'>4194304</memory>
  <currentMemory unit='KiB'>4194304</currentMemory>
  <vcpu placement='static'>1</vcpu>
  <os>
    <type arch='x86_64' machine='pc-i440fx-bionic'>hvm</type>
    <boot dev='hd'/>
  </os>
  <features>
    <acpi/>
    <apic/>
    <pae/>
  </features>
  <clock offset='utc'/>
  <on_poweroff>destroy</on_poweroff>
  <on_reboot>restart</on_reboot>
  <on_crash>destroy</on_crash>
  <devices>
    <emulator>/usr/bin/kvm-spice</emulator>
    <disk type='volume' device='disk'>
      <driver name='qemu' type='qcow2'/>
      <source pool='default' volume='k8s0.qcow2'/>
      <target dev='vda' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/>
    </disk>
    <disk type='file' device='cdrom'>
      <driver name='qemu' type='raw'/>
      <source file='/var/lib/libvirt/images/commoninit.iso'/>
      <target dev='hda' bus='ide'/>
      <readonly/>
      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
    </disk>
    <controller type='usb' index='0' model='piix3-uhci'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
    </controller>
    <controller type='pci' index='0' model='pci-root'/>
    <controller type='ide' index='0'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
    </controller>
    <controller type='virtio-serial' index='0'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
    </controller>
    <interface type='network'>
      <mac address='52:54:00:82:59:16'/>
      <source network='vm_network'/>
      <model type='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
    </interface>
    <serial type='pty'>
      <target type='isa-serial' port='0'>
        <model name='isa-serial'/>
      </target>
    </serial>
    <console type='pty'>
      <target type='serial' port='0'/>
    </console>
    <console type='pty'>
      <target type='virtio' port='1'/>
    </console>
    <channel type='pty'>
      <target type='virtio' name='org.qemu.guest_agent.0'/>
      <address type='virtio-serial' controller='0' bus='0' port='1'/>
    </channel>
    <input type='mouse' bus='ps2'/>
    <input type='keyboard' bus='ps2'/>
    <graphics type='spice' autoport='yes' listen='127.0.0.1'>
      <listen type='address' address='127.0.0.1'/>
    </graphics>
    <video>
      <model type='cirrus' vram='16384' heads='1' primary='yes'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
    </video>
    <memballoon model='virtio'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>
    </memballoon>
    <rng model='virtio'>
      <backend model='random'>/dev/urandom</backend>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/>
    </rng>
  </devices>
</domain>

[electrofelix]

I'm going to have to look through the code, but I was sure this was supposed to add disks using the file type rather than the volume type which would result in the additional helper utils around libvirt doing the right thing

[electrofelix]

So looks like the regression is from #380, specifically commit accb12b switched back to using volumes in preference to files which does not work with apparmor, then subsequently #489 simply moves the code around around between files.

@dmacvicar looks like you changed the code around to switch back to prefering volumes rather than files? Should this be switched back as I don't see the libvirt helper utils for apparmor supporting?

One solution might be to leave everything else using volume id's but change setDisks in libvirt/domain.go (below) to define the XML using the file paths?

terraform-provider-libvirt/libvirt/domain.go

Lines 426 to 476 in ee4094d

	diskVolume, err := virConn.LookupStorageVolByKey(volumeKey.(string))
	if err != nil {
	return fmt.Errorf("Can't retrieve volume %s: %v", volumeKey.(string), err)
	}
	diskVolumeName, err := diskVolume.GetName()
	if err != nil {
	return fmt.Errorf("Can't retrieve name for volume %s", volumeKey.(string))
	}
	diskPool, err := diskVolume.LookupPoolByVolume()
	if err != nil {
	return fmt.Errorf("Can't retrieve pool for volume %s", volumeKey.(string))
	}
	diskPoolName, err := diskPool.GetName()
	if err != nil {
	return fmt.Errorf("Can't retrieve name for pool of volume %s", volumeKey.(string))
	}
	// find out the format of the volume in order to set the appropriate
	// driver
	volumeDef, err := newDefVolumeFromLibvirt(diskVolume)
	if err != nil {
	return err
	}
	if volumeDef.Target != nil && volumeDef.Target.Format != nil && volumeDef.Target.Format.Type != "" {
	if volumeDef.Target.Format.Type == "qcow2" {
	log.Print("[DEBUG] Setting disk driver to 'qcow2' to match disk volume format")
	disk.Driver = &libvirtxml.DomainDiskDriver{
	Name: "qemu",
	Type: "qcow2",
	}
	}
	if volumeDef.Target.Format.Type == "raw" {
	log.Print("[DEBUG] Setting disk driver to 'raw' to match disk volume format")
	disk.Driver = &libvirtxml.DomainDiskDriver{
	Name: "qemu",
	Type: "raw",
	}
	}
	} else {
	log.Printf("[WARN] Disk volume has no format specified: %s", volumeKey.(string))
	}
	disk.Source = &libvirtxml.DomainDiskSource{
	Volume: &libvirtxml.DomainDiskSourceVolume{
	Pool: diskPoolName,
	Volume: diskVolumeName,
	},
	}

[dmacvicar]

I explained in #567

@dmacvicar looks like you changed the code around to switch back to prefering volumes rather than files? Should this be switched back as I don't see the libvirt helper utils for apparmor supporting?

accb12b is not a regression. We want our users to be able to attach disks present in different types of volume pools. Using filenames makes the assumption that the file is a qcow2 backed by a directory pool, which is not the case.

If the user creates a volume using whatever backing storage, we want the terraform provider to be independent of that choice, and refer to the volume by its top abstraction, which is the (pool, name) pair.

You are asking to remove that abstraction, because a very specific tool was implemented incorrectly, making assumptions about implementation.

For example, if you create a pool of type "Physical disk", the volume xml would look like this:

<volume type='block'>
  <name>loop0p1</name>
  <key>/dev/loop0p1</key>
  <source>
    <device path='/dev/loop0'>
      <extent start='32256' end='542868480'/>
    </device>
  </source>
  <capacity unit='bytes'>542836224</capacity>
  <allocation unit='bytes'>542836224</allocation>
  <physical unit='bytes'>542836224</physical>
  <target>
    <path>/dev/loop0p1</path>
    <format type='none'/>
    <permissions>
      <mode>0660</mode>
      <owner>0</owner>
      <group>486</group>
    </permissions>
    <timestamps>
      <atime>1554648074.662222336</atime>
      <mtime>1554648074.662222336</mtime>
      <ctime>1554648074.662222336</ctime>
    </timestamps>
  </target>
</volume>

So I believe virt-aa-helper is implemented incorrectly, and it should do the right thing, which is to look at eg. target/path, which seems to be used at least for all volumes than end mapped to the filesystem.

Applying the workaround you suggest, would prevent people from using a eg. physical disk based pool.

[electrofelix]

Given how linux works in representing everything as I file, I was expecting that ultimately all volumes would represetable as files so it would be good to be clear on exactly where this breaks down as opposed to what doesn't work with this first pass.

It would seem given that the current behaviour forces a requirement to use root for anyone using ubuntu, resulting in a non trivial security impact, if there is no way to translate all volume representations to a file representation, then it seems like a user controlled option to enable this behaviour where needed with a warning message about the limitations and to highlight it is a requirement due to how virt-aa-helper works, would be a reasonable compromise to help as many users as possible. Thoughts?

[dmacvicar]

Given how linux works in representing everything as I file, I was expecting that ultimately all volumes would represetable as files so it would be good to be clear on exactly where this breaks down as opposed to what doesn't work with this first pass.

Most volumes are indeed mapped to a file/path. It is just that file= is an internal representation of one type of pools, while the path attribute of the target element of a volume seems to be the "file representation" common to all volumes.

The current behavior does not force anything. I believe virt-aa-helper is an incomplete/wrong implementation (I could be wrong too).

My suspicion would be https://gitlab.com/libvirt/libvirt/blob/52970dec5b4d0fd1a9baa593b46a33bd7eeaf6b8/src/security/virt-aa-helper.c#L965

    for (i = 0; i < ctl->def->ndisks; i++) {
        virDomainDiskDefPtr disk = ctl->def->disks[i];

        if (!virDomainDiskGetSource(disk))
            continue;
        /* XXX - if we knew the qemu user:group here we could send it in
         *        so that the open could be re-tried as that user:group.
         */
        if (!disk->src->backingStore) {
            bool probe = ctl->allowDiskFormatProbing;
            virStorageFileGetMetadata(disk->src, -1, -1, probe, false);
        }

        /* XXX passing ignoreOpenFailure = true to get back to the behavior
         * from before using virDomainDiskDefForeachPath. actually we should
         * be passing ignoreOpenFailure = false and handle open errors more
         * careful than just ignoring them.
         */
        if (virDomainDiskDefForeachPath(disk, true, add_file_path, &buf) < 0)
            goto cleanup;
    }

It basically reads the source element of each disk and assumes (probably from the early libvirt days) that there is a path element. I'd guess that function is designed in the times disk was directly tied to files.
I am not an expert in libvirt internals, but it looks like it should check if the disk source is of type "volume", and if that is the case, get the volume definition and do the same with the path attribute of the volume's target. I am almost 99% sure, that for the terraform provider itself, it is better to stay tied to the assumption that the disk is backed by a volume (which could be a file), instead of assuming that every volume backing a disk is a file.

@cbosdo may have some wisdom for us.

[zeenix]

This seems to be stale. I'm guessing it's not relevant anymore or not affecting enough people? If so, please do close it.

[haxwithaxe]

This is unfortunately still relevant :/ I'm having this problem on Debian bullseye.
The libvirt devs seem unwilling to implement libvirt-aa-helper in the way they really should. In this issue they say they won't fix it. In the end of the thread they give their logic so you can skip to the end and you won't miss much. It boils down to (paraphrasing) "it's hard" and "there is already a way to do it that works".
While I agree it's not your job to fix their issues it would be really nice to have an option to explicitly use file type instead of volume type disks.