fix(virtsecrets): a secret in use is not deleted, and a password is stored as typed

- A secret is in use when a VM names it in a disk, a host device or its
  vTPM, or a pool in its source, by UUID or by its usage within the usage
  type, as they run and as saved. The vTPM, host devices and pools' saved
  definitions were not checked before.
- A Ceph key is taken in base64, as `ceph auth get-key` prints it; any other
  value is a password or passphrase stored as typed, spaces included.
- A new secret takes its value at once; a value refused leaves no secret.
- TLS and vTPM secrets can be created; the usage has no length limit.
- Every POST redirects; the list is read once and only with its users; a
  secret, VM or pool gone meanwhile is left out.
- domain_trees() is shared by the in-use checks of secrets, network filters,
  networks and pools; the storage page's Ceph secret picker reads the same
  listing.

virtsecrets/tests/test_audit_rows.py

@@ -27,11 +27,11 @@ class SecretRowTests(TestCase):
         patcher = patch("virtsecrets.views.wvmSecrets")
         self.conn = patcher.start().return_value
         self.addCleanup(patcher.stop)
-        self.conn.get_secrets.return_value = []
+        self.conn.get_secrets_info.return_value = []
 
     def test_secret_actions(self):
-        self.client.post(self.url, {"create": "", "ephemeral": "no", "private": "yes", "usage_type": "ceph",
-                                    "data": "client.admin"})
+        self.client.post(self.url, {"create": "", "private": "on", "usage_type": "ceph", "usage": "client.admin",
+                                    "value": VALUE})
         self.client.post(self.url, {"set_value": "", "uuid": UUID, "value": VALUE})
         self.client.post(self.url, {"delete": "", "uuid": UUID})
         self.assertEqual(rows(), [
@@ -42,9 +42,12 @@ class SecretRowTests(TestCase):
         self.assertEqual(set(Logs.objects.values_list("host", flat=True)), {"c1"})
 
     def test_a_secret_value_is_never_logged(self):
+        # an unexpected error naming the value is raised, its row names only the error's class
         self.conn.set_secret_value.side_effect = ValueError(f"bad value {VALUE}")
-        self.client.post(self.url, {"set_value": "", "uuid": UUID, "value": VALUE})
+        with self.assertRaises(ValueError):
+            self.client.post(self.url, {"set_value": "", "uuid": UUID, "value": VALUE})
         self.assertEqual(rows(), [(f"Secret {UUID}: set value: ValueError", False)])
+        self.conn.set_secret_value.side_effect = OperationError("A Ceph key is base64 text")
         with patch.object(Logs.objects, "create", side_effect=RuntimeError("db down")), \
                 self.assertLogs("logs.audit", "WARNING") as logged:
             self.client.post(self.url, {"set_value": "", "uuid": UUID, "value": VALUE})

virtsecrets/tests/test_create_secret.py

@@ -1,55 +1,91 @@
-"""A secret's usage value goes into its XML as text, whatever it holds: a
-volume path, an iSCSI IQN or a Ceph name, and characters XML gives a meaning."""
+"""A secret's usage goes into its XML as text, whatever it holds. Its value
+is a Ceph key in base64, as `ceph auth get-key` prints it, or a password or
+passphrase as typed; a new secret takes its value at once, or none, and a
+value refused leaves no secret behind."""
 
 from unittest.mock import patch
 
+import libvirt
 from computes.models import Compute
 from django.contrib.auth import get_user_model
 from django.test import SimpleTestCase, TestCase
 from django.urls import reverse
 from lxml import etree, html
 from virtsecrets.forms import AddSecret
+from vrtManager.util import OperationError
 from vrtManager.virtsecrets import wvmSecrets
 
 
-class CreateSecretTests(SimpleTestCase):
-    def defined_xml(self, usage_type, data):
-        with patch("vrtManager.connection.connection_manager.get_connection") as get:
-            wvmSecrets("host", "login", "pw", 1).create_secret("no", "yes", usage_type, data)
-        return etree.fromstring(get.return_value.secretDefineXML.call_args.args[0])
+def connection():
+    with patch("vrtManager.connection.connection_manager.get_connection") as get:
+        conn = wvmSecrets("host", "login", "pw", 1)
+    return conn, get.return_value
 
-    def test_the_usage_value_is_text_of_its_element(self):
-        for usage_type, element, data in (
+
+class CreateSecretTests(SimpleTestCase):
+    def test_the_usage_is_text_of_its_element(self):
+        for usage_type, element, usage in (
             ("volume", "volume", "/var/lib/libvirt/images/vm-1.qcow2"),
             ("iscsi", "target", "iqn.2026-10.test.wvc:lun"),
             ("ceph", "name", "client.libvirt secret"),
+            ("tls", "name", "migration"),
+            ("vtpm", "name", "VM web-1's vTPM"),
             ("volume", "volume", "/a'b<c>&d\""),
         ):
-            with self.subTest(usage_type=usage_type, data=data):
-                secret = self.defined_xml(usage_type, data)
+            with self.subTest(usage_type=usage_type, usage=usage):
+                conn, wvm = connection()
+                conn.create_secret(usage_type, usage, private=True)
+                secret = etree.fromstring(wvm.secretDefineXML.call_args.args[0])
                 self.assertEqual((secret.get("ephemeral"), secret.get("private")), ("no", "yes"))
                 self.assertEqual(secret.find("usage").get("type"), usage_type)
-                self.assertEqual(secret.findtext(f"usage/{element}"), data)
+                self.assertEqual(secret.findtext(f"usage/{element}"), usage)
+                wvm.secretDefineXML.return_value.setValue.assert_not_called()
+
+    def test_a_new_secret_takes_its_value_at_once(self):
+        for usage_type, value, stored in (("ceph", "cHcA/w==", b"pw\x00\xff"), ("iscsi", " pass word ", b" pass word ")):
+            with self.subTest(usage_type=usage_type):
+                conn, wvm = connection()
+                conn.create_secret(usage_type, "lun", value)
+                wvm.secretDefineXML.return_value.setValue.assert_called_once_with(stored)
+
+    def test_a_value_refused_leaves_no_secret(self):
+        conn, wvm = connection()
+        with self.assertRaisesRegex(OperationError, "base64"):
+            conn.create_secret("ceph", "client.libvirt", "not base64!")
+        wvm.secretDefineXML.assert_not_called()
+        wvm.secretDefineXML.return_value.setValue.side_effect = libvirt.libvirtError("refused")
+        with self.assertRaises(libvirt.libvirtError):
+            conn.create_secret("iscsi", "lun", "pw")
+        wvm.secretDefineXML.return_value.undefine.assert_called_once_with()
 
 
 class SecretValueTests(SimpleTestCase):
-    def secrets(self):
-        with patch("vrtManager.connection.connection_manager.get_connection") as get:
-            conn = wvmSecrets("host", "login", "pw", 1)
-        secret = get.return_value.secretLookupByUUIDString.return_value
-        return conn, secret
+    def set_value(self, usage_type, value):
+        conn, wvm = connection()
+        secret = wvm.secretLookupByUUIDString.return_value
+        secret.XMLDesc.return_value = f"<secret><usage type='{usage_type}'><name>x</name></usage></secret>"
+        conn.set_secret_value("uuid", value)
+        return secret.setValue
 
-    def test_a_base64_value_is_stored_as_its_bytes(self):
-        conn, secret = self.secrets()
-        conn.set_secret_value("uuid", "cHcA/w==")
-        secret.setValue.assert_called_once_with(b"pw\x00\xff")
+    def test_a_ceph_key_is_stored_as_its_bytes_a_password_as_typed(self):
+        self.set_value("ceph", "cHcA/w==").assert_called_once_with(b"pw\x00\xff")
+        # a password that happens to be base64 is not decoded
+        self.set_value("iscsi", "cHcA/w==").assert_called_once_with(b"cHcA/w==")
+        self.set_value("vtpm", " pässword ").assert_called_once_with(" pässword ".encode())
 
-    def test_a_value_that_is_not_base64_is_refused(self):
-        conn, secret = self.secrets()
-        for value in ("b'cHc='", "not base64!"):
-            with self.subTest(value=value), self.assertRaises(ValueError):
-                conn.set_secret_value("uuid", value)
-        secret.setValue.assert_not_called()
+    def test_a_ceph_key_that_is_not_base64_is_refused_without_the_value(self):
+        for value in ("b'cHc='", "not base64!", "kéy"):
+            with self.subTest(value=value), self.assertRaises(OperationError) as refused:
+                self.set_value("ceph", value)
+            self.assertNotIn(value, str(refused.exception))
+
+
+class CreateSecretFormTests(SimpleTestCase):
+    def test_a_password_keeps_its_spaces_and_the_flags_are_checkboxes(self):
+        form = AddSecret({"usage_type": "iscsi", "usage": "iqn.2026-10.test.wvc:lun", "value": " pw ", "private": "on"})
+        self.assertTrue(form.is_valid(), form.errors)
+        self.assertEqual((form.cleaned_data["value"], form.cleaned_data["ephemeral"], form.cleaned_data["private"]),
+                         (" pw ", False, True))
 
 
 class CreateSecretDialogTests(TestCase):
@@ -57,7 +93,8 @@ class CreateSecretDialogTests(TestCase):
         compute = Compute.objects.create(name="secret-usage", hostname="localhost", login="", password="", type=4)
         self.client.force_login(get_user_model().objects.create_superuser("usage", "usage@example.com", "pw"))
         with patch("virtsecrets.views.wvmSecrets") as conn:
-            conn.return_value.get_secrets.return_value = []
+            conn.return_value.get_secrets_info.return_value = []
+            conn.return_value.get_users.return_value = {}
             response = self.client.get(reverse("virtsecrets", args=[compute.id]))
         offered = html.fromstring(response.content).xpath("//select[@id='secret_usage_type']/option/@value")
         self.assertEqual(offered, [value for value, _ in AddSecret.base_fields["usage_type"].choices])

virtsecrets/tests/test_live_secrets.py

@@ -46,7 +46,7 @@ class LiveSecretsTestCase(TestCase):
             with self.subTest(name):
                 response = self.client.post(
                     reverse("virtsecrets", args=[self.compute.id]),
-                    {"create": "", "ephemeral": "no", "private": "no", "usage_type": name, "data": value},
+                    {"create": "", "usage_type": name, "usage": value},
                 )
                 self.assertEqual(response.status_code, 302)
                 self.assertEqual(self.conn.secretLookupByUsage(usage_type, value).usageID(), value)

virtsecrets/tests/test_secret_in_use.py

@@ -1,9 +1,12 @@
-"""A secret a VM disk or a pool needs is not deleted: without it that VM or
-pool no longer starts. A disk or pool names its secret by UUID or by usage.
-libvirt's test driver has no secrets, so the connection is a mock."""
+"""A secret a VM or a pool needs is not deleted: without it that VM or pool
+no longer starts. A VM names its secret in a disk, a host device or its vTPM,
+a pool in its source, by UUID or by usage within the usage type; as they run
+and as saved. libvirt's test driver has no secrets, so the connection is a
+mock."""
 
 from unittest.mock import MagicMock, patch
 
+import libvirt
 from computes.models import Compute
 from django.contrib.auth import get_user_model
 from django.contrib.messages import get_messages
@@ -12,53 +15,139 @@ from django.urls import reverse
 from vrtManager.util import OperationError
 from vrtManager.virtsecrets import wvmSecrets
 
-UUID = "8f2c0000-0000-4000-8000-000000000001"
+CEPH = "8f2c0000-0000-4000-8000-000000000001"
+ISCSI = "8f2c0000-0000-4000-8000-000000000002"
+VTPM = "8f2c0000-0000-4000-8000-000000000003"
 
 
-def libvirt_object(name, xml):
-    obj = MagicMock()
-    obj.name.return_value = name
-    obj.XMLDesc.return_value = xml
-    return obj
+def secret(uuid, usage_type, usage):
+    return MagicMock(**{"UUIDString.return_value": uuid,
+                        "XMLDesc.return_value": f"<secret><usage type='{usage_type}'><name>{usage}</name></usage></secret>"})
 
 
-def disk_vm(name, secret):
-    return libvirt_object(name, f"<domain><devices><disk type='network'><source protocol='rbd' name='rbd/{name}'/>"
-                                f"<auth username='libvirt'>{secret}</auth><target dev='vdb'/></disk></devices></domain>")
+def owner(name, xml, saved=None, active=False, persistent=True):
+    """A domain or a pool: xml as it runs, saved as defined for its next start."""
+    return MagicMock(**{"name.return_value": name, "XMLDesc.side_effect": lambda flags=0: (saved or xml) if flags else xml,
+                        "isActive.return_value": active, "isPersistent.return_value": persistent})
 
 
-class SecretInUseTests(SimpleTestCase):
-    def delete(self, domains=(), pools=()):
-        with patch("vrtManager.connection.connection_manager.get_connection") as get:
-            wvm = get.return_value
-            wvm.listAllDomains.return_value = list(domains)
-            wvm.listAllStoragePools.return_value = list(pools)
-            secret = wvm.secretLookupByUUIDString.return_value
-            secret.usageID.return_value = "client.libvirt secret"
-            wvmSecrets("host", "login", "pw", 1).delete_secret(UUID)
-        return secret
+def vm(name, devices, **kwargs):
+    return owner(name, f"<domain><devices>{devices}</devices></domain>", **kwargs)
 
-    def test_a_secret_a_vm_disk_or_a_pool_names_is_not_deleted(self):
-        pool = libvirt_object("ceph-rbd", "<pool type='rbd'><source><auth type='ceph' username='libvirt'>"
-                                          "<secret usage='client.libvirt secret'/></auth></source></pool>")
-        with self.assertRaises(OperationError) as refused:
-            self.delete([disk_vm("db-2", f"<secret type='ceph' uuid='{UUID}'/>")], [pool])
-        self.assertIn("VMs db-2", str(refused.exception))
-        self.assertIn("pools ceph-rbd", str(refused.exception))
+
+def pool(name, auth, **kwargs):
+    return owner(name, f"<pool type='rbd'><source>{auth}</source></pool>", **kwargs)
+
+
+def gone(code):
+    err = libvirt.libvirtError("gone")
+    err.get_error_code = lambda: code
+    return MagicMock(**{"name.return_value": "gone", "XMLDesc.side_effect": err})
+
+
+# the same usage, of two types: a reference by usage names the one of its type
+SECRETS = [secret(CEPH, "ceph", "client.libvirt"), secret(ISCSI, "iscsi", "client.libvirt"), secret(VTPM, "vtpm", "tpm")]
+
+
+def connection(domains=(), pools=(), secrets=SECRETS):
+    with patch("vrtManager.connection.connection_manager.get_connection") as get:
+        conn = wvmSecrets("host", "login", "pw", 1)
+    get.return_value.configure_mock(**{"listAllDomains.return_value": list(domains),
+                                       "listAllStoragePools.return_value": list(pools),
+                                       "listAllSecrets.return_value": list(secrets)})
+    return conn
+
+
+def users(**kwargs):
+    conn = connection(**kwargs)
+    return conn.get_users(conn.get_secrets_info())
+
+
+class UsersTests(SimpleTestCase):
+    def test_vms_name_secrets_in_disks_host_devices_and_vtpm(self):
+        found = users(domains=[
+            vm("rbd-1", f"<disk type='network'><auth username='libvirt'><secret type='ceph' uuid='{CEPH}'/></auth></disk>"),
+            vm("lun-1", "<hostdev mode='subsystem' type='scsi'><source protocol='iscsi'><auth username='u'>"
+                        "<secret type='iscsi' usage='client.libvirt'/></auth></source></hostdev>"),
+            vm("tpm-1", f"<tpm model='tpm-crb'><backend type='emulator'><encryption secret='{VTPM}'/></backend></tpm>"),
+        ])
+        self.assertEqual(found, {CEPH: {"vms": ["rbd-1"], "pools": []}, ISCSI: {"vms": ["lun-1"], "pools": []},
+                                 VTPM: {"vms": ["tpm-1"], "pools": []}})
+
+    def test_a_pool_names_its_secret_by_the_auth_type_and_as_saved(self):
+        found = users(pools=[
+            pool("ceph-rbd", "<auth type='ceph' username='libvirt'><secret usage='client.libvirt'/></auth>"),
+            # chap is iSCSI; named only for its next start
+            pool("lun", "", saved="<pool><source><auth type='chap' username='u'><secret usage='client.libvirt'/>"
+                                  "</auth></source></pool>", active=False),
+        ])
+        self.assertEqual(found, {CEPH: {"vms": [], "pools": ["ceph-rbd"]}, ISCSI: {"vms": [], "pools": ["lun"]}})
+
+    def test_a_vm_uses_a_secret_as_it_runs_and_as_saved(self):
+        disk = "<disk><auth username='libvirt'><secret type='ceph' usage='client.libvirt'/></auth></disk>"
+        # saved only: read whether it runs or not, as it may stop between the reads
+        for kwargs in ({"active": True, "saved": f"<domain><devices>{disk}</devices></domain>"},
+                       {"active": False, "saved": f"<domain><devices>{disk}</devices></domain>"},
+                       {"active": False}, {"active": True, "persistent": False}):
+            devices = "" if "saved" in kwargs else disk
+            with self.subTest(**kwargs):
+                self.assertEqual(users(domains=[vm("db-1", devices, **kwargs)]), {CEPH: {"vms": ["db-1"], "pools": []}})
+
+    def test_what_is_gone_meanwhile_is_left_out_other_errors_are_raised(self):
+        found = users(domains=[gone(libvirt.VIR_ERR_NO_DOMAIN)], pools=[gone(libvirt.VIR_ERR_NO_STORAGE_POOL)],
+                      secrets=[gone(libvirt.VIR_ERR_NO_SECRET)])
+        self.assertEqual(found, {})
+        with self.assertRaises(libvirt.libvirtError):
+            users(domains=[gone(libvirt.VIR_ERR_OPERATION_FAILED)])
+
+
+class NoValueReadTests(SimpleTestCase):
+    def test_the_list_and_its_users_read_no_value(self):
+        for listed in SECRETS:
+            listed.value.side_effect = AssertionError("a secret's value was read")
+        conn = connection(domains=[vm("db-1", f"<disk><auth><secret type='ceph' uuid='{CEPH}'/></auth></disk>")])
+        conn.get_users(conn.get_secrets_info())
+        conn.wvm.secretLookupByUUIDString.assert_not_called()
+
+
+class DeleteTests(SimpleTestCase):
+    def test_a_secret_a_vm_or_a_pool_names_is_not_deleted(self):
+        conn = connection(domains=[vm("db-2", f"<disk><auth><secret type='ceph' uuid='{CEPH}'/></auth></disk>")],
+                          pools=[pool("ceph-rbd", "<auth type='ceph'><secret usage='client.libvirt'/></auth>")])
+        with self.assertRaisesRegex(OperationError, f"Secret {CEPH} is used by VMs db-2; pools ceph-rbd"):
+            conn.delete_secret(CEPH)
+        conn.wvm.secretLookupByUUIDString.return_value.undefine.assert_not_called()
 
     def test_an_unused_secret_is_deleted(self):
-        other = disk_vm("web-1", "<secret type='ceph' uuid='8f2c0000-0000-4000-8000-000000000002'/>")
-        self.delete([other]).undefine.assert_called_once_with()
+        conn = connection(domains=[vm("web-1", f"<disk><auth><secret type='ceph' uuid='{CEPH}'/></auth></disk>")])
+        conn.delete_secret(ISCSI)
+        conn.wvm.secretLookupByUUIDString.assert_called_once_with(ISCSI)
+        conn.wvm.secretLookupByUUIDString.return_value.undefine.assert_called_once_with()
 
 
 class SecretInUsePageTests(TestCase):
-    def test_the_page_says_who_needs_it(self):
+    def setUp(self):
         compute = Compute.objects.create(name="secret-use", hostname="localhost", login="", password="", type=4)
         self.client.force_login(get_user_model().objects.create_superuser("secret-use", "su@example.com", "pw"))
-        url = reverse("virtsecrets", args=[compute.id])
+        self.url = reverse("virtsecrets", args=[compute.id])
+
+    def test_the_page_says_who_needs_it(self):
         with patch("virtsecrets.views.wvmSecrets") as conn:
-            conn.return_value.get_secrets.return_value = []
-            conn.return_value.delete_secret.side_effect = OperationError(f"Secret {UUID} is used by VMs db-2")
-            response = self.client.post(url, {"delete": "", "uuid": UUID})
-        self.assertRedirects(response, url, fetch_redirect_response=False)
-        self.assertEqual([str(m) for m in get_messages(response.wsgi_request)], [f"Secret {UUID} is used by VMs db-2"])
+            conn.return_value.delete_secret.side_effect = OperationError(f"Secret {CEPH} is used by VMs db-2")
+            response = self.client.post(self.url, {"delete": "", "uuid": CEPH})
+        self.assertRedirects(response, self.url, fetch_redirect_response=False)
+        self.assertEqual([str(m) for m in get_messages(response.wsgi_request)], [f"Secret {CEPH} is used by VMs db-2"])
+
+    def test_a_post_to_a_host_not_reached_is_redirected(self):
+        with patch("virtsecrets.views.wvmSecrets", side_effect=libvirt.libvirtError("unable to connect")):
+            response = self.client.post(self.url, {"delete": "", "uuid": CEPH})
+        self.assertRedirects(response, self.url, fetch_redirect_response=False)
+
+    def test_the_list_is_not_shown_without_its_users(self):
+        # it would offer to delete the secrets in use
+        with patch("virtsecrets.views.wvmSecrets") as conn:
+            conn.return_value.get_secrets_info.return_value = [{"uuid": CEPH, "usage_type": "ceph", "usage": "client.libvirt",
+                                                                "ephemeral": False, "private": False}]
+            conn.return_value.get_users.side_effect = libvirt.libvirtError("connection reset")
+            response = self.client.get(self.url)
+        self.assertEqual(response.context["secrets_all"], [])

virtsecrets/tests/test_secrets_page.py

@@ -7,7 +7,6 @@ from computes.models import Compute
 from django.contrib.auth import get_user_model
 from django.test import TestCase
 from django.urls import reverse
-from libvirt import VIR_SECRET_USAGE_TYPE_CEPH, VIR_SECRET_USAGE_TYPE_VTPM
 from lxml import html
 
 
@@ -16,23 +15,21 @@ class SecretsPageTests(TestCase):
         compute = Compute.objects.create(name="secrets-page", hostname="localhost", login="", password="", type=4)
         self.client.force_login(get_user_model().objects.create_superuser("page", "page@example.com", "pw"))
         with patch("virtsecrets.views.wvmSecrets") as conn:
-            conn.return_value.get_secrets.return_value = ["0e5b1f0a-8f3e-4c1a-9d2b-5c6d7e8f9a0b"]
-            secret = conn.return_value.get_secret.return_value
-            secret.usageID.return_value = "client.libvirt"
-            secret.UUIDString.return_value = "0e5b1f0a-8f3e-4c1a-9d2b-5c6d7e8f9a0b"
-            secret.usageType.return_value = usage_type
-            secret.value.return_value = b"cephx key"
-            conn.return_value.get_secret_value.return_value = "Y2VwaHgga2V5"
+            conn.return_value.get_secrets_info.return_value = [
+                {"uuid": "0e5b1f0a-8f3e-4c1a-9d2b-5c6d7e8f9a0b", "usage_type": usage_type, "usage": "client.libvirt",
+                 "ephemeral": False, "private": False}]
+            conn.return_value.get_users.return_value = {}
+            conn.return_value.wvm.secretLookupByUUIDString.return_value.value.return_value = b"cephx key"
             response = self.client.get(reverse("virtsecrets", args=[compute.id]))
         self.assertEqual(response.status_code, 200)
         return response
 
     def test_a_vtpm_secret_is_listed(self):
-        self.assertContains(self.page(VIR_SECRET_USAGE_TYPE_VTPM), "vtpm")
+        self.assertContains(self.page("vtpm"), "vtpm")
 
     def test_the_page_holds_no_secret_value(self):
-        response = self.page(VIR_SECRET_USAGE_TYPE_CEPH)
-        self.assertFalse(b"Y2VwaHgga2V5" in response.content, "the secret's value is on the page")
+        response = self.page("ceph")
+        self.assertFalse(b"cephx key" in response.content, "the secret's value is on the page")
         field = html.fromstring(response.content).get_element_by_id("secret_value_0e5b1f0a-8f3e-4c1a-9d2b-5c6d7e8f9a0b")
         self.assertIsNone(field.get("value"))
         self.assertEqual(field.get("type"), "password")