Re: [libvirt] [PATCH 4/4] apparmor: let AppArmorSetSecurityImageLabel append rules

On Tue, Nov 19, 2019 at 10:12 PM Jamie Strandboge <jamie(a)canonical.com&gt; wrote: > > On Wed, 16 Oct 2019, Christian Ehrhardt wrote: > > > There are currently broken use cases, e.g. snapshotting more than one disk at > > once like: > > $ virsh snapshot-create-as --domain eoan --disk-only --atomic > > --diskspec vda,snapshot=no --diskspec vdb,snapshot=no > > --diskspec vdc,file=/test/disk1.snapshot1.qcow,snapshot=external > > --diskspec vdd,file=/test/disk2.snapshot1.qcow,snapshot=external > > The command above will iterate from qemuDomainSnapshotCreateDiskActive and > > eventually add /test/disk1.snapshot1.qcow first (appears in the rules) > > to then later add /test/disk2.snapshot1.qcow and while doing so throwing > > away the former rule causing it to fail. > > > > All other calls to (re)load_profile already use append=true when adding > > rules append=false is only used when restoring rules [1]. > > > > Fix this by letting AppArmorSetSecurityImageLabel use append=true as well. > > > > Bugs: > > https://bugs.launchpad.net/libvirt/+bug/1845506 > > https://bugzilla.redhat.com/show_bug.cgi?id=1746684 > > > > [1]: https://bugs.launchpad.net/libvirt/+bug/1845506/comments/13 > > > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; > > --- > > src/security/security_apparmor.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c > > index 320d69e52a..4dd7ba20b4 100644 > > --- a/src/security/security_apparmor.c > > +++ b/src/security/security_apparmor.c > > @@ -812,7 +812,7 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, > > return -1; > > } > > > > - return reload_profile(mgr, def, src->path, false); > > + return reload_profile(mgr, def, src->path, true); > > src/security/security_manager.c shows that > virSecurityManagerSetImageLabel() is called to label 'a storage image > with the configured security label'. Based on your report, it seems that > in addition to the underlying disk (vda in this case), it is also called > for each additional disk (ie, vdc and vdd). Yes in the "transaction" for e.g. a snapshot on multiple disks there will be calls for both disks before any of them becomes part of the VM-definition. That is where the second call "removes" the first rule and then things fail. To be clear, I didn't add any "disk" in the broken use-case it adds further backing image chain elements to existing disks as part of the snapshot action. So if I snapshot in one shot vdb and vdc it is called for each of those adding the new paths for the respective backing chain that grows. If you ever power cycle that it will be part of the definition and virt-aa-helper resolves backing chains as needed. > While your patch to append > makes sense for this scenario, what happens if you update the vm > definition to use 'vde' instead of 'vda', is the vda access removed and > vde added with vdc and vdd remaining? Hmm - not sure what exactly you mean, but lets try to break it ... This splits into three major paths to consider: a) adding/removing disks while the guest is off. This isn't interesting as that way it will be part (or not) of the definition when it starts. The guest will get initial rules based on that definition on start, nothing special. b) adding/removing disks at runtime of a guest b1) you can edit a live definition in XML, but that will only add/modify the disk on next start Even if you now add another disk via proper hot-add later the first will not be added as it isn't really part of the active definition yet. b2) you can hot add/remove a disk, those will be properly labelled and added/removed via their labelling calls on that action c) snapshots at runtime of the guest (that was the broken case) As mentioned above it will need to add new elements to the backing-chain c1) snapshot one disk will work, the call to AppArmorSetSecurityImageLabel will add the new rule needed c2) snapshot multiple disks at once then it will fail without the fix here the second call to AppArmorSetSecurityImageLabel will revoke the former rule Sorry, I don't find the " update the vm definition to use 'vde' instead of 'vda'" that you meant in either of the paths a/b/c above. I'll try to reach you on IRC later on to sort this out ...

> What if we remove vda and replace > it with only vde, are vda, vdc and vdd removed? I fear that making this > change will result in scenarios where the rule is (correctly) added, but > previous rules are not removed. > > Can you comment on if this is working correctly? Is it possible to have > tests that demonstrate everything is working as intended? > > -- > Jamie Strandboge | http://www.canonical.com -- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd