launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/getFullKeyText-crash into lp:launchpad.

Commit message:
Fix SSHKey.getFullKeyText to not crash on some corrupt keys.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1798046 in Launchpad itself: "oops when displaying sshkeys page (presumable related to encoding)"
  https://bugs.launchpad.net/launchpad/+bug/1798046

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/getFullKeyText-crash/+merge/358417

It already handled cases where the base64 or netstring were bad, but could crash
if the netstring contained non-ASCII garbage.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/getFullKeyText-crash into lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2018-10-03 09:14:06 +0000
+++ lib/lp/registry/model/person.py	2018-11-07 04:11:23 +0000
@@ -4129,7 +4129,8 @@
 
     def getFullKeyText(self):
         try:
-            ssh_keytype = getNS(base64.b64decode(self.keytext))[0]
+            ssh_keytype = getNS(base64.b64decode(self.keytext))[0].decode(
+                'ascii')
         except Exception as e:
             # We didn't always validate keys, so there might be some that
             # can't be loaded this way.

=== modified file 'lib/lp/registry/tests/test_ssh.py'
--- lib/lp/registry/tests/test_ssh.py	2018-07-02 14:08:22 +0000
+++ lib/lp/registry/tests/test_ssh.py	2018-11-07 04:11:23 +0000
@@ -7,6 +7,7 @@
 
 from testtools.matchers import StartsWith
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from lp.registry.interfaces.ssh import (
     ISSHKeySet,
@@ -62,6 +63,18 @@
         expected = "ecdsa-sha2-nistp521 %s %s" % (key.keytext, key.comment)
         self.assertEqual(expected, key.getFullKeyText())
 
+    def test_getFullKeyText_for_corrupt_key(self):
+	# If the key text is corrupt, the type from the database is used
+	# instead of the one decoded from the text.
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            key = self.factory.makeSSHKey(person, "ssh-rsa")
+	    # The base64 has a valid netstring, but the contents are garbage so
+	    # can't be a valid key type.
+            removeSecurityProxy(key).keytext = 'AAAAB3NzaC1012EAAAA='
+        expected = "ssh-rsa %s %s" % (key.keytext, key.comment)
+        self.assertEqual(expected, key.getFullKeyText())
+
     def test_destroySelf_sends_notification_by_default(self):
         person = self.factory.makePerson()
         with person_logged_in(person):