virsh snapshot-create-as fails when --disk-only is specified

[KarlGoetz]

Reporting per chat in #apparmor . Since that suggestion I've done enough to establish its highly likely a dupe of one of the tickets I reference further down but I'm reporting so someone more experienced can determine where it should be linked to.

I'm seeing denials like this (on ubuntu 18.04) when trying to run virsh snapshot-create-as server-here --name "Auto snapshot $(date --rfc-3339=seconds)" --atomic --disk-only ; the profile does include the libvirt abstraction file which specifies rmix for that binary. is the problem that its being invoked without a full path?

type=AVC msg=audit(1597890133.739:39299): apparmor="DENIED" operation="open" profile="libvirt-a93f9c40-05ef-fa3d-d1fd-c8a36fa533a6" name=2F7661722F6C69622F6C6962766972742F696D616765732F736572766572323031392D30322E4175746F20736E617073686F7420323032302D30382D32302031323A32323A31322B31303A3030 pid=43589 comm="qemu-system-x86" requested_mask="r" denied_mask="r" fsuid=64055 ouid=64055

For the record, trying without spaces we have the same error. but the name of the snapshot simply isn't encoded.

type=AVC msg=audit(1597891120.185:39322): apparmor="DENIED" operation="open" profile="libvirt-a93f9c40-05ef-fa3d-d1fd-c8a36fa533a6" name="/var/lib/libvirt/images/server-here.xxx661722F6C69622F6C6962661722F6C69622F6C696F6C69622F6C6962766ssssss972742xxx" pid=43589 comm="qemu-system-x86" requested_mask="r" denied_mask="r" fsuid=64055 ouid=64055

Further research showed that this succeeds:
virsh snapshot-create-as server-02 --name "Auto snapshot $(date --rfc-3339=seconds)" --atomic

So its when --disk-only becomes involved the failure occurs. that means https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1320221 and https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1004606 are likely to already indicate (if not capture) my problem. https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1417288 relates to the functionality but doesn't cover my usecase

As a final point; I needed to add rwk and re run aa-enforce on the instance's profile (not libvirt-qemu).

vi /etc/apparmor.d/abstractions/libvirt-qemu
aa-enforce /etc/apparmor.d/libvirt/libvirt-a93f9c40-05ef-fa3d-d1fd-c8a36fa533a6
virsh snapshot-create-as server-here --name "xxx661722F6C69622F6C6962661722F6C69622F6C696F6C69622F6C6962766ssssss972742xxx" --atomic --disk-only
Domain snapshot xxx661722F6C69622F6C6962661722F6C69622F6C696F6C69622F6C6962766ssssss972742xxx created

It appears to me that whatever generates the .files listing should consider derived names ; it would be better than the `/var/lib/libvirt/images/** rwk,` I used in terms of confinement.

disk one original: server-name-1.img
disk two original: server-name-2.img
disk two snapshot: server-name-2.xx22F6C69622F6C6962661722F6C69622F6C696F6C69622F6C6962766ssssss972742xxx
disk one snapshot: server-name-1.xxx661722F6C69622F6C6962661722F6C69622F6C696F6C69622F6C6962766ssssss972742xxx

[KarlGoetz (kgoetz)]

After running the script at the bottom snapshots work using the incantation below.

virsh snapshot-create-as server-name --name "postscript" --atomic --disk-only
Domain snapshot postscript created

#!/bin/bash
# Script to broaden the permissions libvirts apparmor-helper script gives to instances.
# See also https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1892306
# Possibly not the ideal fix but better than the alternatives (I could see)
# <email address hidden>

# Bail if nothing passed
if [ -z $1 ] ; then
  echo "Pass anything which identifies instance to libvirt - uuid, id, name."
  exit 1
else
  current_instance=$1
fi

# Root is required to change libvirt/apparmor settings
if ! [ `id -u` -eq 0 ]; then
  echo "Root is required to run this script"
  exit 1
fi

# Determine running status
instance_running=`virsh dominfo $current_instance |grep 'State' |awk '{ print $2 }'`
# Extract instance security label; used to configure apparmour
instance_seclabel=`virsh dominfo $current_instance |grep 'Security label' |awk '{ print $3 }'`
# Shortcut to file
instance_aa_config=/etc/apparmor.d/libvirt/$instance_seclabel

if ! [ 'running' == $instance_running ]; then
  # Start the intance
  virsh_start_retcode=`virsh start $current_instance`
  if [ $virsh_start_retcode -gt 0 ]; then
    exit $virsh_start_retcode
  fi
fi

# List current block devices, try to extract actual device then make a permitting wildcard based on it
for blockdev in $(virsh domblklist 24 |grep 'libvirt/images' |awk '{ print $2}' |sed -e 's|\..*$||'); do
  echo " \"${blockdev}.*\" rwk," >> ${instance_aa_config}.files
done

# Re-enforce using the updated block device/file config
apparmor_parser -r -v $instance_aa_config

[Christian Ehrhardt  (paelzer)]

Hi,
bug 1320221 is about virt-aa-helper being denied which is different.
A dup of 1417288 it might be, but that is so old that I'd not want to wake it up :-)

> "It appears to me that whatever generates the .files listing should consider derived names ..."

Yeah for some scenarios that is correct and for others it would be a truly evil security hole.
What I'd want in this case is to allow you to add rules per-guest which stay across restarts of the guest and without e.g. aa-enforce that you needed. That is implemented in new version by me via bug 1745114 (>=Groovy). Until then your script seems a complex but valid workaround.

The question that remains is if libvirt should have issued a labeling call for this file that would have added a rule to allow that filename. Also the fact that using spaces changes the behavior so drastically seems odd. I'd need to debug the case but am very busy atm.
I'll keep it open and hope to get into it some-when the next few days.

[Christian Ehrhardt  (paelzer)]

This seemed too familiar, two bugs that are even closer but fixed are:
https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1681839
https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1845506

This was mostly around multi-invocations of the same.
This explains why I remembered something, I fixed both :-)

BTW - on which Ubuntu release are you?
Could you give it a try with the packages as they are in Groovy please if this might actually just be a backports request?

[KarlGoetz (kgoetz)]

Hi paelzer,

https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1845506 seems a very good description of my problem, wonder why I missed it (also great work on tracking it down!).

We're on Ubuntu 18.04, not sure if we'll be moving to 20.04 in the near term or not (it depends on Other Things).

Not sure about your final request (sorry) - are you asking me to do a local backport to test the packages or build a new server to test the instance/s on ?

Karl.

[Christian Ehrhardt  (paelzer)]

> (also great work on tracking it down!).

I had an unfair advantage of implementing it in the first place :-)

> We're on Ubuntu 18.04, not sure if we'll be moving to 20.04 in the near
> term or not (it depends on Other Things).
>
> Not sure about your final request (sorry) - are you asking me to do a
> local backport to test the packages or build a new server to test the
> instance/s on ?

The latter, spawn a test server and try it there.

Instead of going all the way to 20.04 or 20.10 you might consider
giving the cloud archive [1] a try.
That would make newer libvirt/qemu available on your 18.04 system and
therefore be available easier for you.
A check if the existing fix helps you or if we need more would be awesome.
Would be like:
 $ sudo add-apt-repository cloud-archive:ussuri
 $ apt update
 $ apt upgrade
 # Check your case with these versions

[1]: https://wiki.ubuntu.com/OpenStack/CloudArchive

[KarlGoetz (kgoetz)]

Good news - my snapshot works.

virsh snapshot-create-as server-name --name "Auto snapshot $(date --rfc-3339=seconds)" --atomic --disk-only
Domain snapshot Auto snapshot 2020-09-17 10:17:49+10:00 created

$ lsb_release -r
Release: 18.04

$ apt policy qemu-system-x86
qemu-system-x86:
  Installed: 1:4.2-3ubuntu6.3~cloud0
  Candidate: 1:4.2-3ubuntu6.3~cloud0
  Version table:
 *** 1:4.2-3ubuntu6.3~cloud0 500
        500 http://ubuntu-cloud.archive.canonical.com/ubuntu bionic-updates/ussuri/main amd64 Packages
        100 /var/lib/dpkg/status

Is a chance the fix can be backported to bionic-updates or will using Cloud Archive be the only supported mechanism to get access to the fix?

I see similar package versions in 20.04 but I'm not sure if I can sell an upgrade for this one fix :)

[Christian Ehrhardt  (paelzer)]

Hi Karl,
glad I could help with that.

The change is part of a bigger series that needs to drop a apparmor reload, change the way AppArmorSetSecurityImageLabel is called and only then finally can use the append functionality in this case.

The SRU [1] policy is rather strict on avoiding potential regressions for existing users (or at least need to be tremendously outweighted by the gains in rare cases) and this could have unwanted side effects. Furthermore the willingness to risk that reduces for cases like this where
 a) you'd have the option to slightly change the use-case to get it working and
 b) there already is an active Ubuntu LTS released that fixes the issue

So I'm inclined to say no I won't backport it to Bionic unless there is a stronger reason/need to do so.

[1]: https://wiki.ubuntu.com/StableReleaseUpdates

[KarlGoetz (kgoetz)]

Completely understood, thanks for spelling it out so clearly. Knowing that option is closed to us we can weight the relative merits of a full upgrade vs using the cloud archive to get a stack which supports our requirements.

Karl.