Merge ~paelzer/ubuntu/+source/libvirt:lp-1927519-corrupted-1948880-swtpm-JAMMY into ubuntu/+source/libvirt:ubuntu/jammy-devel

[Christian Ehrhardt ]

[Christian Ehrhardt  (paelzer)]

PPA: https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/4710/+packages

[Christian Ehrhardt  (paelzer)]

Testing of swtpm finished, see comment 8-12 in https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1948880

The profile handling change matches upstream was created by Ioanna.

Ready for review & upload now.

[Sergio Durigan Junior (sergiodj)]

Thanks for the MP, Christian.

I was going to start reviewing it, but I noticed that the arm64 build has failed on the PPA:

Summary of Failures:

 90/167 virdrivermoduletest FAIL 0.17s exit status 1
134/167 storagepoolxml2argvtest FAIL 0.12s exit status 1
135/167 storagepoolxml2xmltest FAIL 0.14s exit status 1
138/167 virstoragetest FAIL 0.16s exit status 1

I couldn't find any mentions to this failure in the bug, so I'm thinking this is unexpected. Either way, I took the liberty to retry the build; let's see if it succeeds now. If it does (and if I'm still working at the time), I can review the MP.

Thanks.

[Sergio Durigan Junior (sergiodj)]

Ah, the rebuild worked :-). I had the impression that the failures were flaky, and apparently they are. Also, I noticed that you've faced the same failures on s390x. Interesting...

Anyway, here's the review I promised.

I'm OK with the patch to address the apparmor issue.

As for the swtpm thing, I read the entire bug and I am also a bit uncomfortable with the scenario #2 (upgrade) that you described there, specifically about the problem with permissions and the implications this will have for SRUs. But as you said there, this looks fine for Jammy and I don't really have anything bad to say for this specific MP.

I found a bunch of small nits here and there; I'm leaving inline comments for them.

Otherwise, this LGTM with those fixed, so I'm approving it in advance.

Thanks!

review: Approve

[Christian Ehrhardt  (paelzer)]

The arm issue was https://bugs.launchpad.net/ubuntu/+source/glusterfs/+bug/1950777 which apparently seemed to take longer to publish on arm and therefore appeared to be flaky.

Thanks for the review, going through inline comments now ...

[Christian Ehrhardt  (paelzer)]

Thank you so much for all the little Hints Sergio.
All resolved now, uploading ...

[Christian Ehrhardt  (paelzer)]

Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading libvirt_7.6.0-0ubuntu2.dsc: done.
  Uploading libvirt_7.6.0-0ubuntu2.debian.tar.xz: done.
  Uploading libvirt_7.6.0-0ubuntu2_source.buildinfo: done.
  Uploading libvirt_7.6.0-0ubuntu2_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/changelog b/debian/changelog
index 01f0460..1dee69f 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,16 @@
+libvirt (7.6.0-0ubuntu2) jammy; urgency=medium
+
+  * d/p/u/lp-1927519-virt-aa-helper-Purge-profile-if-corrupted.patch: avoid
+    issues due to corrupted apparmor profiles (LP: #1927519)
+  * libvirt should not use user/group tss for swtpm (LP: #1948880)
+    - d/libvirt-daemon-system.postinst: own swtpm logdir by user swtpm
+    - d/p/u/swtpm-by-swtpm-user.patch: change default spawned swtpm processes
+      to user swtpm
+    - d/p/u/swtpm-by-swtpm-user.patch: adapt expected self test results
+    - d/control: suggest swtpm-tools
+
+ -- Christian Ehrhardt <[e-mail]>  Thu, 11 Nov 2021 12:11:38 +0100
+
 libvirt (7.6.0-0ubuntu1) impish; urgency=medium
 
   * Merge v7.6.0 from upstream and unreleased changes from Debian git.
diff --git a/debian/control b/debian/control
index a95e480..7fa21dd 100644
--- a/debian/control
+++ b/debian/control
@@ -355,6 +355,7 @@ Suggests:
  systemd,
  systemtap,
  zfsutils,
+ swtpm-tools,
 Breaks:
  systemd-sysv (<< 224-1~),
 Description: Libvirt daemon configuration files
diff --git a/debian/libvirt-daemon-system.postinst b/debian/libvirt-daemon-system.postinst
index ad55c75..71fe46e 100644
--- a/debian/libvirt-daemon-system.postinst
+++ b/debian/libvirt-daemon-system.postinst
@@ -172,6 +172,8 @@ add_statoverrides()
 
     QEMU_CONF="/etc/libvirt/qemu.conf"
 
+    SWTPM_DIR="/var/log/swtpm/libvirt/qemu"
+
     for dir in ${ROOT_DIRS}; do
         if ! dpkg-statoverride --list "${dir}" >/dev/null 2>&1; then
             [ ! -e "${dir}" ] || chown root:root "${dir}"
@@ -195,6 +197,12 @@ add_statoverrides()
         [ ! -e "${QEMU_CONF}" ] || chown root:root "${QEMU_CONF}"
         [ ! -e "${QEMU_CONF}" ] || chmod 0600 "${QEMU_CONF}"
     fi
+
+    # swtpm shall use user swtpm (LP: #1948880)
+    if ! dpkg-statoverride --list "${SWTPM_DIR}" >/dev/null 2>&1; then
+        [ ! -e "${SWTPM_DIR}" ] || chown swtpm:swtpm "${SWTPM_DIR}"
+        [ ! -e "${SWTPM_DIR}" ] || chmod 0700 "${SWTPM_DIR}"
+    fi
 }
 
 case "$1" in
diff --git a/debian/patches/series b/debian/patches/series
index 38bb8ca..3c60875 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -27,3 +27,5 @@ ubuntu-aa/0032-apparmor-libvirt-qemu-Allow-reading-charm-specific-c.patch
 ubuntu-aa/0033-UBUNTU-only-apparmor-for-kvm.powerpc-LP-1680384.patch
 ubuntu-aa/0034-apparmor-virt-aa-helper-access-for-snapped-nova.patch
 ubuntu-aa/lp-1815910-allow-vhost-hotplug.patch
+ubuntu/lp-1927519-virt-aa-helper-Purge-profile-if-corrupted.patch
+ubuntu/swtpm-by-swtpm-user.patch
diff --git a/debian/patches/ubuntu/lp-1927519-virt-aa-helper-Purge-profile-if-corrupted.patch b/debian/patches/ubuntu/lp-1927519-virt-aa-helper-Purge-profile-if-corrupted.patch
 new file mode 100644
index 0000000..7094217
--- /dev/null
+++ b/debian/patches/ubuntu/lp-1927519-virt-aa-helper-Purge-profile-if-corrupted.patch
@@ -0,0 +1,76 @@
+From 4ab33415db31d0e77015bb852cab4a08dd0efd40 Mon Sep 17 00:00:00 2001
+From: Ioanna Alifieraki <[e-mail]>
+Date: Tue, 2 Nov 2021 16:04:45 +0200
+Subject: [PATCH] virt-aa-helper: Purge profile if corrupted
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+This commit aims to address the bug reported in [1] and [2].
+If the profile is corrupted (0-size) the VM cannot be launched.
+To overcome this, check if the profile exists and if it has 0 size
+remove it.
+
+[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890084
+[2] https://bugs.launchpad.net/bugs/1927519
+
+Signed-off-by: Ioanna Alifieraki <[e-mail]>
+Reviewed-by: Christian Ehrhardt <[e-mail]>
+Reviewed-by: Ján Tomko <[e-mail]>
+
+Origin: upstream, https://libvirt.org/git/?p=libvirt.git;a=commit;h=4ab33415db
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1927519
+Last-Update: 2021-11-11
+
+---
+ src/security/virt-aa-helper.c | 20 +++++++++++++++++++-
+ 1 file changed, 19 insertions(+), 1 deletion(-)
+
+diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
+index 7c21ab9515..218e07bfb0 100644
+--- a/src/security/virt-aa-helper.c
++++ b/src/security/virt-aa-helper.c
+@@ -1437,6 +1437,8 @@ main(int argc, char **argv)
+     int rc = -1;
+     char *profile = NULL;
+     char *include_file = NULL;
++    off_t size;
++    bool purged = 0;
+ 
+     if (virGettextInitialize() < 0 ||
+         virErrorInitialize() < 0) {
+@@ -1484,6 +1486,22 @@ main(int argc, char **argv)
+         if (ctl->cmd == 'c' && virFileExists(profile))
+             vah_error(ctl, 1, _("profile exists"));
+ 
++        /*
++         * Rare cases can leave corrupted empty files behind breaking
++         * the guest. An empty file is never correct as virt-aa-helper
++         * would at least add the basic rules, therefore clean this up
++         * for a proper refresh.
++         */
++        if (virFileExists(profile)) {
++                size = virFileLength(profile, -1);
++                if (size == 0) {
++                        vah_warning(_("Profile of 0 size detected, will attempt to remove it"));
++                        if ((rc = parserRemove(ctl->uuid) != 0))
++                                vah_error(ctl, 1, _("could not remove profile"));
++                        unlink(profile);
++                        purged = true;
++                }
++        }
+         if (ctl->append && ctl->newfile) {
+             if (vah_add_file(&buf, ctl->newfile, "rwk") != 0)
+                 goto cleanup;
+@@ -1523,7 +1541,7 @@ main(int argc, char **argv)
+ 
+ 
+         /* create the profile from TEMPLATE */
+-        if (ctl->cmd == 'c') {
++        if (ctl->cmd == 'c' || purged) {
+             char *tmp = NULL;
+             tmp = g_strdup_printf("  #include <libvirt/%s.files>\n", ctl->uuid);
+ 
+-- 
+2.33.1
+
diff --git a/debian/patches/ubuntu/swtpm-by-swtpm-user.patch b/debian/patches/ubuntu/swtpm-by-swtpm-user.patch
 new file mode 100644
index 0000000..8644a28
--- /dev/null
+++ b/debian/patches/ubuntu/swtpm-by-swtpm-user.patch
@@ -0,0 +1,40 @@
+Description: Have swtpm use the swtpm user by default
+ User 'tss' has more permissions than required and since tpm in some sense
+ is guest/host interface it shall be run under a more restrictive user.
+Forwarded: no-needed
+X-Not-Forwarded-Reason: swtpm user is ubuntu specific
+Author: Christian Ehrhardt <[e-mail]>
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1948880
+Last-Update: 2021-11-11
+--- a/src/qemu/qemu.conf
++++ b/src/qemu/qemu.conf
+@@ -922,11 +922,14 @@
+ 
+ # User for the swtpm TPM Emulator
+ #
+-# Default is 'tss'; this is the same user that tcsd (TrouSerS) installs
+-# and uses; alternative is 'root'
++# Default is 'swtpm' as established by the swtpm-tools package.
+ #
+-#swtpm_user = "tss"
+-#swtpm_group = "tss"
++# In the past this was 'tss' and that still would be the built-in default
++# if nothing was configured here, but the 'tss' user also has TPM device
++# access in the host which isn't needed for swtpm.
++#
++swtpm_user = "swtpm"
++swtpm_group = "swtpm"
+ 
+ # For debugging and testing purposes it's sometimes useful to be able to disable
+ # libvirt behaviour based on the capabilities of the qemu process. This option
+--- a/src/qemu/test_libvirtd_qemu.aug.in
++++ b/src/qemu/test_libvirtd_qemu.aug.in
+@@ -111,8 +111,6 @@ module Test_libvirtd_qemu =
+ { "pr_helper" = "/usr/bin/qemu-pr-helper" }
+ { "slirp_helper" = "/usr/bin/slirp-helper" }
+ { "dbus_daemon" = "/usr/bin/dbus-daemon" }
+-{ "swtpm_user" = "tss" }
+-{ "swtpm_group" = "tss" }
+ { "capability_filters"
+     { "1" = "capname" }
+ }