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

[Christian Ehrhardt]

On Tue, Nov 19, 2019 at 10:12 PM Jamie Strandboge <jamie at canonical.com> 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 at canonical.com>
> > ---
> >  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