[PATCH] apparmor: Don't check for existence of templates upfront
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
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 :|
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
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 :|
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 :|
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
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 :|
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
participants (2)
-
Andrea Bolognani -
Daniel P. Berrangé