Merge lp:~wgrant/launchpad/getFullKeyText-crash into lp:launchpad

Proposed by William Grant
Status: Merged
Merged at revision: 18812
Proposed branch: lp:~wgrant/launchpad/getFullKeyText-crash
Merge into: lp:launchpad
Diff against target: 44 lines (+15/-1)
2 files modified
lib/lp/registry/model/person.py (+2/-1)
lib/lp/registry/tests/test_ssh.py (+13/-0)
To merge this branch: bzr merge lp:~wgrant/launchpad/getFullKeyText-crash
Reviewer Review Type Date Requested Status
Colin Watson Approve
Review via email: mp+358417@code.launchpad.net

Commit message

Fix SSHKey.getFullKeyText to not crash on some corrupt keys.

Description of the change

It already handled cases where the base64 or netstring were bad, but could crash
if the netstring contained non-ASCII garbage.

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/model/person.py'
2--- lib/lp/registry/model/person.py 2018-10-03 09:14:06 +0000
3+++ lib/lp/registry/model/person.py 2018-11-07 10:43:45 +0000
4@@ -4129,7 +4129,8 @@
5
6 def getFullKeyText(self):
7 try:
8- ssh_keytype = getNS(base64.b64decode(self.keytext))[0]
9+ ssh_keytype = getNS(base64.b64decode(self.keytext))[0].decode(
10+ 'ascii')
11 except Exception as e:
12 # We didn't always validate keys, so there might be some that
13 # can't be loaded this way.
14
15=== modified file 'lib/lp/registry/tests/test_ssh.py'
16--- lib/lp/registry/tests/test_ssh.py 2018-07-02 14:08:22 +0000
17+++ lib/lp/registry/tests/test_ssh.py 2018-11-07 10:43:45 +0000
18@@ -7,6 +7,7 @@
19
20 from testtools.matchers import StartsWith
21 from zope.component import getUtility
22+from zope.security.proxy import removeSecurityProxy
23
24 from lp.registry.interfaces.ssh import (
25 ISSHKeySet,
26@@ -62,6 +63,18 @@
27 expected = "ecdsa-sha2-nistp521 %s %s" % (key.keytext, key.comment)
28 self.assertEqual(expected, key.getFullKeyText())
29
30+ def test_getFullKeyText_for_corrupt_key(self):
31+ # If the key text is corrupt, the type from the database is used
32+ # instead of the one decoded from the text.
33+ person = self.factory.makePerson()
34+ with person_logged_in(person):
35+ key = self.factory.makeSSHKey(person, "ssh-rsa")
36+ # The base64 has a valid netstring, but the contents are garbage so
37+ # can't be a valid key type.
38+ removeSecurityProxy(key).keytext = 'AAAAB3NzaC1012EAAAA='
39+ expected = "ssh-rsa %s %s" % (key.keytext, key.comment)
40+ self.assertEqual(expected, key.getFullKeyText())
41+
42 def test_destroySelf_sends_notification_by_default(self):
43 person = self.factory.makePerson()
44 with person_logged_in(person):