1. Patchew
  2. Libvirt
  3. View series

[PATCH] apparmor: Don't check for existence of templates upfront

Andrea Bolognani posted 1 patch 2 weeks, 5 days ago 
src/security/security_apparmor.c | 18 ------------------
1 file changed, 18 deletions(-)
[PATCH] apparmor: Don't check for existence of templates upfront
Posted by Andrea Bolognani 2 weeks, 5 days ago 
Currently, if either template is missing AppArmor support is
completely disabled. This means that uninstalling the LXC
driver from a system results in QEMU domains being started
without AppArmor confinement, which obviously doesn't make any
sense.

The problematic scenario was impossible to hit in Debian until
very recently, because all AppArmor files were shipped as part
of the same package; now that the Debian package is much closer
to the Fedora one, and specifically ships the AppArmor files
together with the corresponding driver, it becomes trivial to
trigger it.

Drop the checks entirely. virt-aa-helper, which is responsible
for creating the per-domain profiles starting from the
driver-specific template, already fails if the latter is not
present, so they were always redundant.

https://bugs.debian.org/1081396

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/security/security_apparmor.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 27184aef7f..a62ec1b10d 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -318,27 +318,9 @@ AppArmorSetSecurityHostLabel(virSCSIVHostDevice *dev G_GNUC_UNUSED,
 static virSecurityDriverStatus
 AppArmorSecurityManagerProbe(const char *virtDriver G_GNUC_UNUSED)
 {
-    g_autofree char *template_qemu = NULL;
-    g_autofree char *template_lxc = NULL;
-
     if (use_apparmor() < 0)
         return SECURITY_DRIVER_DISABLE;
 
-    /* see if template file exists */
-    template_qemu = g_strdup_printf("%s/TEMPLATE.qemu", APPARMOR_DIR "/libvirt");
-    template_lxc = g_strdup_printf("%s/TEMPLATE.lxc", APPARMOR_DIR "/libvirt");
-
-    if (!virFileExists(template_qemu)) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("template \'%1$s\' does not exist"), template_qemu);
-        return SECURITY_DRIVER_DISABLE;
-    }
-    if (!virFileExists(template_lxc)) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("template \'%1$s\' does not exist"), template_lxc);
-        return SECURITY_DRIVER_DISABLE;
-    }
-
     return SECURITY_DRIVER_ENABLE;
 }
 
-- 
2.46.0
Re: [PATCH] apparmor: Don't check for existence of templates upfront
Posted by Daniel P. Berrangé 2 weeks, 5 days ago 
On Mon, Sep 16, 2024 at 04:55:55PM +0200, Andrea Bolognani wrote:
> Currently, if either template is missing AppArmor support is
> completely disabled. This means that uninstalling the LXC
> driver from a system results in QEMU domains being started
> without AppArmor confinement, which obviously doesn't make any
> sense.
> 
> The problematic scenario was impossible to hit in Debian until
> very recently, because all AppArmor files were shipped as part
> of the same package; now that the Debian package is much closer
> to the Fedora one, and specifically ships the AppArmor files
> together with the corresponding driver, it becomes trivial to
> trigger it.
> 
> Drop the checks entirely. virt-aa-helper, which is responsible
> for creating the per-domain profiles starting from the
> driver-specific template, already fails if the latter is not
> present, so they were always redundant.
> 
> https://bugs.debian.org/1081396
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/security/security_apparmor.c | 18 ------------------
>  1 file changed, 18 deletions(-)
> 
> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> index 27184aef7f..a62ec1b10d 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -318,27 +318,9 @@ AppArmorSetSecurityHostLabel(virSCSIVHostDevice *dev G_GNUC_UNUSED,
>  static virSecurityDriverStatus
>  AppArmorSecurityManagerProbe(const char *virtDriver G_GNUC_UNUSED)

We're passing the virt driver name ("QEMU" or "LXC") in here and not using
it.....

>  {
> -    g_autofree char *template_qemu = NULL;
> -    g_autofree char *template_lxc = NULL;
> -
>      if (use_apparmor() < 0)
>          return SECURITY_DRIVER_DISABLE;
>  
> -    /* see if template file exists */
> -    template_qemu = g_strdup_printf("%s/TEMPLATE.qemu", APPARMOR_DIR "/libvirt");
> -    template_lxc = g_strdup_printf("%s/TEMPLATE.lxc", APPARMOR_DIR "/libvirt");
> -
> -    if (!virFileExists(template_qemu)) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("template \'%1$s\' does not exist"), template_qemu);
> -        return SECURITY_DRIVER_DISABLE;
> -    }
> -    if (!virFileExists(template_lxc)) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("template \'%1$s\' does not exist"), template_lxc);
> -        return SECURITY_DRIVER_DISABLE;
> -    }

...rather than delete these, pick the right check to perform based
on 'virtDriver' value.

eg approximately like this

   g_autofree char *template_name = g_strdup(virtDriver);
   for (i = 0; template_name[i]; i++)
       template_name[i] = tolower(template_name[i])
   template = g_strdup_printf("%s/TEMPLATE.%s", APPARMOR_DIR "/libvirt", template_name)


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH] apparmor: Don't check for existence of templates upfront
Posted by Andrea Bolognani 2 weeks, 5 days ago 
On Mon, Sep 16, 2024 at 04:04:40PM GMT, Daniel P. Berrangé wrote:
> On Mon, Sep 16, 2024 at 04:55:55PM +0200, Andrea Bolognani wrote:
> >  static virSecurityDriverStatus
> >  AppArmorSecurityManagerProbe(const char *virtDriver G_GNUC_UNUSED)
>
> We're passing the virt driver name ("QEMU" or "LXC") in here and not using
> it.....
>
> ...rather than delete these, pick the right check to perform based
> on 'virtDriver' value.
>
> eg approximately like this
>
>    g_autofree char *template_name = g_strdup(virtDriver);
>    for (i = 0; template_name[i]; i++)
>        template_name[i] = tolower(template_name[i])
>    template = g_strdup_printf("%s/TEMPLATE.%s", APPARMOR_DIR "/libvirt", template_name)

I can give it a shot, but it still seems pointless to check whether
the files are available ahead of time when virt-aa-helper will do
that at the time when they're actually going to be used. What do we
gain by doing that?

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] apparmor: Don't check for existence of templates upfront
Posted by Daniel P. Berrangé 2 weeks, 5 days ago 
On Tue, Sep 17, 2024 at 12:12:05AM +0900, Andrea Bolognani wrote:
> On Mon, Sep 16, 2024 at 04:04:40PM GMT, Daniel P. Berrangé wrote:
> > On Mon, Sep 16, 2024 at 04:55:55PM +0200, Andrea Bolognani wrote:
> > >  static virSecurityDriverStatus
> > >  AppArmorSecurityManagerProbe(const char *virtDriver G_GNUC_UNUSED)
> >
> > We're passing the virt driver name ("QEMU" or "LXC") in here and not using
> > it.....
> >
> > ...rather than delete these, pick the right check to perform based
> > on 'virtDriver' value.
> >
> > eg approximately like this
> >
> >    g_autofree char *template_name = g_strdup(virtDriver);
> >    for (i = 0; template_name[i]; i++)
> >        template_name[i] = tolower(template_name[i])
> >    template = g_strdup_printf("%s/TEMPLATE.%s", APPARMOR_DIR "/libvirt", template_name)
> 
> I can give it a shot, but it still seems pointless to check whether
> the files are available ahead of time when virt-aa-helper will do
> that at the time when they're actually going to be used. What do we
> gain by doing that?

Do we still get a clear error message back to the user if virt-aa-helper
fails due to the missing files ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH] apparmor: Don't check for existence of templates upfront
Posted by Andrea Bolognani 2 weeks, 5 days ago 
On Mon, Sep 16, 2024 at 04:13:03PM GMT, Daniel P. Berrangé wrote:
> On Tue, Sep 17, 2024 at 12:12:05AM +0900, Andrea Bolognani wrote:
> > On Mon, Sep 16, 2024 at 04:04:40PM GMT, Daniel P. Berrangé wrote:
> > > On Mon, Sep 16, 2024 at 04:55:55PM +0200, Andrea Bolognani wrote:
> > > >  static virSecurityDriverStatus
> > > >  AppArmorSecurityManagerProbe(const char *virtDriver G_GNUC_UNUSED)
> > >
> > > We're passing the virt driver name ("QEMU" or "LXC") in here and not using
> > > it.....
> > >
> > > ...rather than delete these, pick the right check to perform based
> > > on 'virtDriver' value.
> > >
> > > eg approximately like this
> > >
> > >    g_autofree char *template_name = g_strdup(virtDriver);
> > >    for (i = 0; template_name[i]; i++)
> > >        template_name[i] = tolower(template_name[i])
> > >    template = g_strdup_printf("%s/TEMPLATE.%s", APPARMOR_DIR "/libvirt", template_name)
> >
> > I can give it a shot, but it still seems pointless to check whether
> > the files are available ahead of time when virt-aa-helper will do
> > that at the time when they're actually going to be used. What do we
> > gain by doing that?
>
> Do we still get a clear error message back to the user if virt-aa-helper
> fails due to the missing files ?

Clear-ish.

This is what the failure will look like to the user:

  $ virsh start cirros
  error: Failed to start domain 'cirros'
  error: internal error: cannot load AppArmor profile
'libvirt-70a00233-4e17-4038-9e03-f84d35ba28e9'

The journal will contain additional information:

  internal error: Child process (LIBVIRT_LOG_OUTPUTS=3:stderr
/usr/lib/libvirt/virt-aa-helper -c -u
libvirt-70a00233-4e17-4038-9e03-f84d35ba28e9)
                  unexpected exit status 1: virt-aa-helper: error:
template does not exist
                  virt-aa-helper: error: could not create profile
  internal error: cannot load AppArmor profile
'libvirt-70a00233-4e17-4038-9e03-f84d35ba28e9'

Note that, even before this change, the actual issue was only
reported in the journal as:

  internal error: template '/etc/apparmor.d/libvirt/TEMPLATE.qemu'
does not exist

I could perhaps enhance the error reported by virt-aa-helper so that
the path of the missing template is included, but let's keep in mind
that this failure will only be reported if the deployment is severely
borked (i.e. the package containing the driver is installed but some
of its files are missing for whatever reason) so I think it's
appropriate to have the details reported in the journal only. The
admin is the only one who can act upon them anyway.

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] apparmor: Don't check for existence of templates upfront
Posted by Daniel P. Berrangé 2 weeks, 5 days ago 
On Mon, Sep 16, 2024 at 04:13:03PM +0100, Daniel P. Berrangé wrote:
> On Tue, Sep 17, 2024 at 12:12:05AM +0900, Andrea Bolognani wrote:
> > On Mon, Sep 16, 2024 at 04:04:40PM GMT, Daniel P. Berrangé wrote:
> > > On Mon, Sep 16, 2024 at 04:55:55PM +0200, Andrea Bolognani wrote:
> > > >  static virSecurityDriverStatus
> > > >  AppArmorSecurityManagerProbe(const char *virtDriver G_GNUC_UNUSED)
> > >
> > > We're passing the virt driver name ("QEMU" or "LXC") in here and not using
> > > it.....
> > >
> > > ...rather than delete these, pick the right check to perform based
> > > on 'virtDriver' value.
> > >
> > > eg approximately like this
> > >
> > >    g_autofree char *template_name = g_strdup(virtDriver);
> > >    for (i = 0; template_name[i]; i++)
> > >        template_name[i] = tolower(template_name[i])
> > >    template = g_strdup_printf("%s/TEMPLATE.%s", APPARMOR_DIR "/libvirt", template_name)
> > 
> > I can give it a shot, but it still seems pointless to check whether
> > the files are available ahead of time when virt-aa-helper will do
> > that at the time when they're actually going to be used. What do we
> > gain by doing that?
> 
> Do we still get a clear error message back to the user if virt-aa-helper
> fails due to the missing files ?

A difference is that this Probe check will presumably report the error
during daemon startup, while the virt-aa-helper check will delay the
report until a VM is started. A failure to start the daemon is arguably
more likely to be noticed & fixed at time of host deployment.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH] apparmor: Don't check for existence of templates upfront
Posted by Andrea Bolognani 2 weeks, 5 days ago 
On Mon, Sep 16, 2024 at 04:15:58PM GMT, Daniel P. Berrangé wrote:
> A difference is that this Probe check will presumably report the error
> during daemon startup, while the virt-aa-helper check will delay the
> report until a VM is started. A failure to start the daemon is arguably
> more likely to be noticed & fixed at time of host deployment.

The problem is that you won't get a daemon startup failure: libvirtd
will happily come up, just with AppArmor containment disabled. QEMU
domains will also start up just fine, except they'll be uncontained.

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] apparmor: Don't check for existence of templates upfront
Posted by Daniel P. Berrangé 2 weeks, 5 days ago 
On Tue, Sep 17, 2024 at 12:32:46AM +0900, Andrea Bolognani wrote:
> On Mon, Sep 16, 2024 at 04:15:58PM GMT, Daniel P. Berrangé wrote:
> > A difference is that this Probe check will presumably report the error
> > during daemon startup, while the virt-aa-helper check will delay the
> > report until a VM is started. A failure to start the daemon is arguably
> > more likely to be noticed & fixed at time of host deployment.
> 
> The problem is that you won't get a daemon startup failure: libvirtd
> will happily come up, just with AppArmor containment disabled. QEMU
> domains will also start up just fine, except they'll be uncontained.

Oh right, becuase we probe for the driver and decide it isn't
available. Yes, lets kill this check. For your original patch

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.