Re: [PATCH] apparmor: let image label setting loop over backing files

On Tue, Jan 19, 2021 at 12:15:31 +0100, Christian Ehrhardt wrote: > On Tue, Jan 19, 2021 at 11:43 AM Peter Krempa <pkrempa(a)redhat.com&gt; wrote: > > > > On Tue, Jan 19, 2021 at 11:23:16 +0100, Christian Ehrhardt wrote: > > > When adding a rule for an image file and that image file has a chain > > > of backing files then we need to add a rule for each of those files. > > > > > > To get that iterate over the backing file chain the same way as > > > dac/selinux already do and add a label for each. > > > > > > Fixes: https://gitlab.com/libvirt/libvirt/-/issues/118 > > > > > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; > > > --- > > > src/security/security_apparmor.c | 39 ++++++++++++++++++++++---------- > > > 1 file changed, 27 insertions(+), 12 deletions(-) > > > > > > diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c > > > index 29f0956d22..1f309c0c9f 100644 > > > --- a/src/security/security_apparmor.c > > > +++ b/src/security/security_apparmor.c > > > @@ -756,22 +756,13 @@ AppArmorRestoreInputLabel(virSecurityManagerPtr mgr, [...] > > > +static int > > > +AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, > > > + virDomainDefPtr def, > > > + virStorageSourcePtr src, > > > + virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) > > > +{ > > > + virSecurityLabelDefPtr secdef; > > > + virStorageSourcePtr n; > > > + > > > + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); > > > + if (!secdef || !secdef->relabel) > > > + return 0; > > > + > > > + if (!secdef->imagelabel) > > > + return 0; > > > > So apparmor doesn't support per-image security labels? > > This was present before, it just got moved as part of this change. > IIRC for apparmor that is only generated once in AppArmorGenSecurityLabel > and later on only used to check if the struct is ok (if it would be NULL that > would indicate a non initialized element). > > If I'm missing some further hidden meaning of "imagelabel" please let me know. Here secdef->imagelabel is a global per-VM label, but you can configure a per disk (or rather per-image) label with a <seclabel> element under <source>. See: https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms right the first example. This allows to control labelling of individual files, but I'm not sure if such concept makes sense for apparmor.

For now it seems to be ignored, maybe it should be at least validated if it doesn't make sense.