From fae3fecc1e01d973109f16a13afeb9f5c44359c8 Mon Sep 17 00:00:00 2001 From: WGH Date: Sun, 21 Mar 2021 20:44:04 +0300 Subject: [PATCH] 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 --- .../org.virt-manager.virt-manager.gschema.xml | 8 +-- virtManager/lib/keyring.py | 67 +++++++++---------- virtManager/object/domain.py | 16 ++--- 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 d4ed2973c..dadbb5b7e 100644 --- a/data/org.virt-manager.virt-manager.gschema.xml +++ b/data/org.virt-manager.virt-manager.gschema.xml @@ -14,10 +14,10 @@ When to scale the VM graphical console. -1 = global default, 0 = never, 1 = only when in full screen mode, 2 = Always - - ("", -1) - Username and secrets ID for graphical password - Username and secrets ID for graphical password + + "" + Username for graphical password + Username for graphical password diff --git a/virtManager/lib/keyring.py b/virtManager/lib/keyring.py index c0f501429..1f9565132 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): + secret = self._get_secret(vm.get_uuid(), vm.conn.get_uri()) + if secret is None: return ("", "") # pragma: no cover - if (secret.attributes.get("hvuri", None) != vm.conn.get_uri() or - secret.attributes.get("uuid", None) != vm.get_uuid()): - 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 cc2f506d4..805e5576c 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())