Improve Secret Service interoperability

The current implementation of Secret Service keyring client assumes that
the last component of an item path is integer, which is not true for some
Secret Service server implementations (e.g. KeePassXC). Besides,
the Secret Service API documents advises against recording object path
(not to mentioning parsing it in any way), recommending using lookup attributes
instead[1].

This commit fixes the code to behave in more interoperable way.

- The item path (called "keyid" in code) is no longer parsed and stored anywhere.
- The secret item is looked up in the Secret Service using hvuri and machine
  uuid attributes.
- /console-password with (username, keyid) is removed from GSettings
  storage. Instead, only username is stored in /console-username.

[1] https://specifications.freedesktop.org/secret-service/latest/ch03.html

Resolves: #237

3 files changed, 45 insertions, 46 deletions

diff --git a/data/org.virt-manager.virt-manager.gschema.xml b/data/org.virt-manager.virt-manager.gschema.xml
index d4ed2973..dadbb5b7 100644
--- a/data/org.virt-manager.virt-manager.gschema.xml
+++ b/data/org.virt-manager.virt-manager.gschema.xml
@@ -14,10 +14,10 @@
       <description>When to scale the VM graphical console. -1 = global default, 0 = never, 1 = only when in full screen mode, 2 = Always</description>
     </key>
 
-    <key name="console-password" type="(si)">
-      <default>("", -1)</default>
-      <summary>Username and secrets ID for graphical password</summary>
-      <description>Username and secrets ID for graphical password</description>
+    <key name="console-username" type="s">
+      <default>""</default>
+      <summary>Username for graphical password</summary>
+      <description>Username for graphical password</description>
     </key>
 
     <key name="resize-guest" type="i">
diff --git a/virtManager/lib/keyring.py b/virtManager/lib/keyring.py
index c0f50142..1f956513 100644
--- a/virtManager/lib/keyring.py
+++ b/virtManager/lib/keyring.py
@@ -61,8 +61,19 @@ class vmmKeyring(vmmGObject):
     def _cleanup(self):
         pass  # pragma: no cover
 
+    def _find_secret_item_path(self, uuid, hvuri):
+        attributes = {
+            "uuid": uuid,
+            "hvuri": hvuri,
+        }
+        unlocked, locked = self._service.SearchItems("(a{ss})", attributes)
+        if not unlocked:
+            if locked:
+                log.warning("Item found, but it's locked")
+            return None
+        return unlocked[0]
+
     def _add_secret(self, secret):
-        ret = None
         try:
             props = {
                 "org.freedesktop.Secret.Item.Label": GLib.Variant("s", secret.get_name()),
@@ -73,17 +84,17 @@ class vmmKeyring(vmmGObject):
                       "text/plain; charset=utf8")
             replace = True
 
-            _id = self._collection.CreateItem("(a{sv}(oayays)b)",
-                                              props, params, replace)[0]
-            ret = int(_id.rsplit("/")[-1])
+            self._collection.CreateItem("(a{sv}(oayays)b)",
+                                              props, params, replace)
         except Exception:  # pragma: no cover
             log.exception("Failed to add keyring secret")
 
-        return ret
-
-    def _del_secret(self, _id):
+    def _del_secret(self, uuid, hvuri):
         try:
-            path = self._collection.get_object_path() + "/" + str(_id)
+            path = self._find_secret_item_path(uuid, hvuri)
+            if path is None:
+                return None
+
             iface = Gio.DBusProxy.new_sync(self._dbus, 0, None,
                                            "org.freedesktop.secrets", path,
                                            "org.freedesktop.Secret.Item", None)
@@ -96,10 +107,13 @@ class vmmKeyring(vmmGObject):
         except Exception:
             log.exception("Failed to delete keyring secret")
 
-    def _get_secret(self, _id):
+    def _get_secret(self, uuid, hvuri):
         ret = None
         try:
-            path = self._collection.get_object_path() + "/" + str(_id)
+            path = self._find_secret_item_path(uuid, hvuri)
+            if path is None:
+                return None
+
             iface = Gio.DBusProxy.new_sync(self._dbus, 0, None,
                                     "org.freedesktop.secrets", path,
                                     "org.freedesktop.Secret.Item", None)
@@ -118,7 +132,7 @@ class vmmKeyring(vmmGObject):
 
             ret = _vmmSecret(label, secret, attrs)
         except Exception:  # pragma: no cover
-            log.exception("Failed to get keyring secret id=%s", _id)
+            log.exception("Failed to get keyring secret uuid=%r hvuri=%r", uuid, hvuri)
 
         return ret
 
@@ -137,41 +151,26 @@ class vmmKeyring(vmmGObject):
         if not self.is_available():
             return ("", "")  # pragma: no cover
 
-        username, keyid = vm.get_console_password()
-
-        if keyid == -1:
-            return ("", "")
-
-        secret = self._get_secret(keyid)
-        if secret is None or secret.get_name() != self._get_secret_name(vm):
-            return ("", "")  # pragma: no cover
-
-        if (secret.attributes.get("hvuri", None) != vm.conn.get_uri() or
-            secret.attributes.get("uuid", None) != vm.get_uuid()):
+        secret = self._get_secret(vm.get_uuid(), vm.conn.get_uri())
+        if secret is None:
             return ("", "")  # pragma: no cover
 
-        return (secret.get_secret(), username or "")
+        return (secret.get_secret(), vm.get_console_username() or "")
 
     def set_console_password(self, vm, password, username=""):
         if not self.is_available():
             return  # pragma: no cover
 
+
         secret = _vmmSecret(self._get_secret_name(vm), password,
                            {"uuid": vm.get_uuid(),
                             "hvuri": vm.conn.get_uri()})
-        keyid = self._add_secret(secret)
-        if keyid is None:
-            return  # pragma: no cover
-
-        vm.set_console_password(username, keyid)
+        vm.set_console_username(username)
+        self._add_secret(secret)
 
     def del_console_password(self, vm):
         if not self.is_available():
             return  # pragma: no cover
 
-        ignore, keyid = vm.get_console_password()
-        if keyid == -1:
-            return
-
-        self._del_secret(keyid)
-        vm.del_console_password()
+        self._del_secret(vm.get_uuid(), vm.conn.get_uri())
+        vm.del_console_username()
diff --git a/virtManager/object/domain.py b/virtManager/object/domain.py
index cc2f506d..805e5576 100644
--- a/virtManager/object/domain.py
+++ b/virtManager/object/domain.py
@@ -1597,14 +1597,14 @@ class vmmDomain(vmmLibvirtObject):
         ret = self.config.get_pervm(self.get_uuid(), "/vm-window-size")
         return ret
 
-    def get_console_password(self):
-        return self.config.get_pervm(self.get_uuid(), "/console-password")
-    def set_console_password(self, username, keyid):
-        return self.config.set_pervm(self.get_uuid(), "/console-password",
-                                     (username, keyid))
-    def del_console_password(self):
-        return self.config.set_pervm(self.get_uuid(), "/console-password",
-                                     ("", -1))
+    def get_console_username(self):
+        return self.config.get_pervm(self.get_uuid(), "/console-username")
+    def set_console_username(self, username):
+        return self.config.set_pervm(self.get_uuid(), "/console-username",
+                                     username)
+    def del_console_username(self):
+        return self.config.set_pervm(self.get_uuid(), "/console-username",
+                                     "")
 
     def get_cache_dir(self):
         ret = os.path.join(self.conn.get_cache_dir(), self.get_uuid())