On Tue, Jan 19, 2021 at 12:28 PM Peter Krempa <pkrempa@xxxxxxxxxx> wrote: > > 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@xxxxxxxxxx> 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@xxxxxxxxxxxxx> > > > > --- > > > > 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. AFAIU the concept would not make much sense for apparmor. The seclabel/relable config per source are useful to control dac/selinux and if they put labels on the entity of "a path/file". But in that sense apparmor isn't controlling attributes of the path at all, it is more like a whitelist associated to the qemu-process. > For now it seems to be > ignored, maybe it should be at least validated if it doesn't make sense. I don't yet see how it would fit, but I appreciate the discussion - thanks Peter! -- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
