Merge lp:~silhusk/software-properties/lp1656100-remove-signing-keys into lp:software-properties

[Carlo Vanini]

Fix removal of signing keys in the Authentication tab always failing.

When clicking on "Remove" we were using the first 8 characters of the display string as a 32 bit (short) key ID. But what we get is the first half of a 64 bit ID, which is not a short ID. This fix uses all the 16 characters in the display string.

Move calls to KeysModified() from class SoftwareProperties to SoftwarePropertiesDBus, where it is defined. Otherwise, in SoftwarePropertiesQt it would raise AttributeError because the function is not defined, and make remove_key() fail even though it did not (the "report this bug" dialog shows up). The same for update_keys and add_key.

[Sebastien Bacher (seb128)]

Thank you for the work there, I'm not really familiar with that code but it has no dedicated maintainer so I'm trying to help with review, some comments/questions

* you wrote 'Remove calls to KeysModified which doesnt's exist and causes all actions on
  keys to look like they failed.'

Where does it look like they failed? What actions are you talking about? (unsure why there is no error/traceback printed about those calls)

* shouldn't those calls be moved to the Dbus backend rather than removed, like it was done in http://launchpadlibrarian.net/91837493/software-properties_0.82.3_0.82.4.diff.gz

* apt-key list is listing 8 digits keys, should we do the same on the UI? (but the parsing code doing [:8] takes the wrong half it seems, did that ever worked? [8:16] looks like what we want or just using :16 should be good enough?

(it doesn't help that "apt-key rm <garbage>" always returns "OK" (known as https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=799342)

review: Needs Information

[Carlo Vanini (silhusk)]

Hi Sebastien, thank you for looking into it.

> * you wrote 'Remove calls to KeysModified which doesnt's exist and causes all
> actions on
> keys to look like they failed.'

Let's consider remove_key() defined at softwareproperties/SoftwareProperties.py:838, for instance. The call to self.KeysModified is wrapped in a try-except that catches the AttributeError exception (self doesn't have any KeysModified), and make remove_key always return False.
The same holds for add_key() and update_keys().

> * shouldn't those calls be moved to the Dbus backend

Yes, definitely (imho). But that's a different story. Please, see https://bugs.launchpad.net/ubuntu/+source/software-properties/+bug/1804887 The patch there is just a proof of concept. Since then I ported the whole application to DBus, but did't publish the code yet.

> * apt-key list is listing 8 digits keys, should we do the same on the UI? (but
> the parsing code doing [:8] takes the wrong half it seems, did that ever
> worked? [8:16] looks like what we want or just using :16 should be good
> enough?

I guess at some point the UI was showing 8 digits keys, therefore :8 was used. Now it shows 16. It is now advised to use the full key fingerprint (40 digits) in order to avoid collision attacks, but that's really long (we should change the UI).
Using :16 was just the smallest change to make it work, with the added benefit of using 64 bits key id.
A way to check fingerprints like in Dolphin's file properties for md5/sha1/sha256 would be cool, where you paste some text and it tells you if it does match.

[Sebastien Bacher (seb128)]

> Let's consider remove_key() defined at softwareproperties/SoftwareProperties.py:838, for instance.
> The call to self.KeysModified is wrapped in a try-except that catches the AttributeError exception
> (self doesn't have any KeysModified), and make remove_key always return False.

Right, I did try to remove the try/except and still don't see an error though, maybe I did test that wrongly, I'm going to give it another try

>> * shouldn't those calls be moved to the Dbus backend
> Yes, definitely (imho). But that's a different story.

Well, I was just speaking about moving the self.KeysModified() lines instead of removing them (same that was done in http://launchpadlibrarian.net/91837493/software-properties_0.82.3_0.82.4.diff.gz). Why do you consider it a "different story"? e.g RemoveKey() in SoftwarePropertiesDbus.py line 312 could have that line added no, that way the change signal keeps being emited? or do you think that's not required for some reason and if so could give some details on why you think those calls are not useful/can be purely removed?

> Using :16 was just the smallest change to make it work, with the added benefit of using 64 bits key id.

Right, that makes sense, that's fine with me. I would like to see the KeysModified question resolved before we merge the changes though.

review: Needs Information

[lp:~silhusk/software-properties/lp1656100-remove-signing-keys]

1060. By Carlo <carlo@mela> on 2019-01-12

Emit KeysModified signal when keys are modified

[Carlo Vanini (silhusk)]

> Well, I was just speaking about moving the self.KeysModified() lines instead
> of removing them (same that was done in http://launchpadlibrarian.net/91837493
> /software-properties_0.82.3_0.82.4.diff.gz).

I didn't understand correctly what you meant, sorry. You are right! The branch is now updated.
In fact the exception is only raised in SoftwarePropertiesQt. In the dbus service KeysModified is defined in class SoftwarePropertiesDBus, although it is called from parent class SoftwareProperties. After the changes both the calls and the definition are in the same class SoftwarePropertiesDBus.

s-p-qt is not using dbus, and is reloading keys anyway.

[Sebastien Bacher (seb128)]

That looks good now, thanks!

review: Approve

=== modified file 'softwareproperties/SoftwareProperties.py'
--- softwareproperties/SoftwareProperties.py	2018-04-09 19:29:09 +0000
+++ softwareproperties/SoftwareProperties.py	2019-01-12 13:07:33 +0000
@@ -844,7 +844,6 @@
         return False
     try:
         res = self.apt_key.add(path)
-        self.KeysModified()
         return res
     except:
         return False
@@ -860,7 +859,6 @@
     """Remove a gnupg key from the list of trusted software vendors"""
     try:
         self.apt_key.rm(keyid)
-        self.KeysModified()
         return True
     except:
         return False
@@ -869,7 +867,6 @@
     """ Run apt-key update """
     try:
       self.apt_key.update()
-      self.KeysModified()
       return True
     except:
       return False

=== modified file 'softwareproperties/dbus/SoftwarePropertiesDBus.py'
--- softwareproperties/dbus/SoftwarePropertiesDBus.py	2017-08-30 18:57:54 +0000
+++ softwareproperties/dbus/SoftwarePropertiesDBus.py	2019-01-12 13:07:33 +0000
@@ -293,7 +293,10 @@
     def AddKey(self, path, sender=None, conn=None):
         self._check_policykit_privilege(
             sender, conn, "com.ubuntu.softwareproperties.applychanges")
-        return self.add_key(path)
+        res = self.add_key(path)
+        if res:
+            self.KeysModified()
+        return res
 
     @dbus.service.method(DBUS_INTERFACE_NAME,
                          sender_keyword="sender", connection_keyword="conn",
@@ -301,7 +304,10 @@
     def AddKeyFromData(self, keyData, sender=None, conn=None):
         self._check_policykit_privilege(
             sender, conn, "com.ubuntu.softwareproperties.applychanges")
-        return self.add_key_from_data(keyData)
+        res = self.add_key_from_data(keyData)
+        if res:
+            self.KeysModified()
+        return res
 
     @dbus.service.method(DBUS_INTERFACE_NAME,
                          sender_keyword="sender", connection_keyword="conn",
@@ -309,7 +315,10 @@
     def RemoveKey(self, keyid, sender=None, conn=None):
         self._check_policykit_privilege(
             sender, conn, "com.ubuntu.softwareproperties.applychanges")
-        return self.remove_key(keyid)
+        res = self.remove_key(keyid)
+        if res:
+            self.KeysModified()
+        return res
 
     @dbus.service.method(DBUS_INTERFACE_NAME,
                          sender_keyword="sender", connection_keyword="conn",
@@ -317,7 +326,10 @@
     def UpdateKeys(self, sender=None, conn=None):
         self._check_policykit_privilege(
             sender, conn, "com.ubuntu.softwareproperties.applychanges")
-        return self.update_keys()
+        res = self.update_keys()
+        if res:
+            self.KeysModified()
+        return res
 
     # LivePatch
     @dbus.service.method(DBUS_INTERFACE_NAME,

=== modified file 'softwareproperties/gtk/SoftwarePropertiesGtk.py'
--- softwareproperties/gtk/SoftwarePropertiesGtk.py	2018-12-11 22:35:38 +0000
+++ softwareproperties/gtk/SoftwarePropertiesGtk.py	2019-01-12 13:07:33 +0000
@@ -1015,7 +1015,7 @@
             return
         key = model.get_value(a_iter, 0)
         try:
-            if not self.backend.RemoveKey(key[:8]):
+            if not self.backend.RemoveKey(key[:16]):
                 error(self.main,
                       _("Error removing the key"),
                       _("The key you selected could not be removed. "

=== modified file 'softwareproperties/qt/SoftwarePropertiesQt.py'
--- softwareproperties/qt/SoftwarePropertiesQt.py	2018-07-14 10:45:16 +0000
+++ softwareproperties/qt/SoftwarePropertiesQt.py	2019-01-12 13:07:33 +0000
@@ -677,7 +677,7 @@
     if item == None:
         return
     key = item.text(0)
-    if not self.remove_key(key[:8]):
+    if not self.remove_key(key[:16]):
       title = _("Error removing the key")
       text = _("The key you selected could not be removed. "
                "Please report this as a bug.")