Merge lp:~cjwatson/launchpad/ssh-ecdsa-keys into lp:launchpad

Proposed by Colin Watson on 2018-07-02
Status: Merged
Merged at revision: 18721
Proposed branch: lp:~cjwatson/launchpad/ssh-ecdsa-keys
Merge into: lp:launchpad
Prerequisite: lp:~cjwatson/launchpad/lazr.sshserver-0.1.8
Diff against target: 449 lines (+152/-56)
7 files modified
lib/lp/registry/interfaces/ssh.py (+20/-10)
lib/lp/registry/model/person.py (+26/-7)
lib/lp/registry/scripts/listteammembers.py (+3/-8)
lib/lp/registry/stories/person/xx-add-sshkey.txt (+32/-5)
lib/lp/registry/templates/person-editsshkeys.pt (+2/-1)
lib/lp/registry/tests/test_ssh.py (+32/-4)
lib/lp/testing/factory.py (+37/-21)
To merge this branch: bzr merge lp:~cjwatson/launchpad/ssh-ecdsa-keys
Reviewer Review Type Date Requested Status
William Grant code 2018-07-02 Approve on 2018-07-02
Review via email: mp+348848@code.launchpad.net

Commit message

Add support for SSH ECDSA keys.

Description of the change

The main difficulty is that there are multiple public key algorithm names for ECDSA, so we need to stop having data structures laid out on the assumption that there's a bijection between public key algorithm names and SSHKeyType enumeration items. (The alternative was to have one SSHKeyType item for each curve size, but that seemed a bit silly.)

The prerequisite branch and the corresponding branches of txpkgupload and turnip must all be deployed to production before this is landed, to avoid the situation where people can upload keys they can't use.

To post a comment you must log in.
William Grant (wgrant) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/interfaces/ssh.py'
2--- lib/lp/registry/interfaces/ssh.py 2018-06-25 15:14:38 +0000
3+++ lib/lp/registry/interfaces/ssh.py 2018-07-02 14:38:50 +0000
4@@ -8,7 +8,6 @@
5 __all__ = [
6 'ISSHKey',
7 'ISSHKeySet',
8- 'SSH_KEY_TYPE_TO_TEXT',
9 'SSH_TEXT_TO_KEY_TYPE',
10 'SSHKeyAdditionError',
11 'SSHKeyType',
12@@ -39,7 +38,7 @@
13 class SSHKeyType(DBEnumeratedType):
14 """SSH key type
15
16- SSH (version 2) can use RSA or DSA keys for authentication. See
17+ SSH (version 2) can use RSA, DSA, or ECDSA keys for authentication. See
18 OpenSSH's ssh-keygen(1) man page for details.
19 """
20
21@@ -55,14 +54,20 @@
22 DSA
23 """)
24
25-
26-SSH_KEY_TYPE_TO_TEXT = {
27- SSHKeyType.RSA: "ssh-rsa",
28- SSHKeyType.DSA: "ssh-dss",
29-}
30-
31-
32-SSH_TEXT_TO_KEY_TYPE = {v: k for k, v in SSH_KEY_TYPE_TO_TEXT.items()}
33+ ECDSA = DBItem(3, """
34+ ECDSA
35+
36+ ECDSA
37+ """)
38+
39+
40+SSH_TEXT_TO_KEY_TYPE = {
41+ "ssh-rsa": SSHKeyType.RSA,
42+ "ssh-dss": SSHKeyType.DSA,
43+ "ecdsa-sha2-nistp256": SSHKeyType.ECDSA,
44+ "ecdsa-sha2-nistp384": SSHKeyType.ECDSA,
45+ "ecdsa-sha2-nistp521": SSHKeyType.ECDSA,
46+ }
47
48
49 class ISSHKey(Interface):
50@@ -139,6 +144,11 @@
51 if 'kind' in kwargs:
52 kind = kwargs.pop('kind')
53 msg = "Invalid SSH key type: '%s'" % kind
54+ if 'type_mismatch' in kwargs:
55+ keytype, keydatatype = kwargs.pop('type_mismatch')
56+ msg = (
57+ "Invalid SSH key data: key type '%s' does not match key data "
58+ "type '%s'" % (keytype, keydatatype))
59 if 'exception' in kwargs:
60 exception = kwargs.pop('exception')
61 try:
62
63=== modified file 'lib/lp/registry/model/person.py'
64--- lib/lp/registry/model/person.py 2018-06-19 14:46:05 +0000
65+++ lib/lp/registry/model/person.py 2018-07-02 14:38:50 +0000
66@@ -29,6 +29,7 @@
67 'WikiNameSet',
68 ]
69
70+import base64
71 from datetime import (
72 datetime,
73 timedelta,
74@@ -81,6 +82,7 @@
75 Store,
76 )
77 import transaction
78+from twisted.conch.ssh.common import getNS
79 from twisted.conch.ssh.keys import Key
80 from zope.component import (
81 adapter,
82@@ -205,7 +207,6 @@
83 from lp.registry.interfaces.ssh import (
84 ISSHKey,
85 ISSHKeySet,
86- SSH_KEY_TYPE_TO_TEXT,
87 SSH_TEXT_TO_KEY_TYPE,
88 SSHKeyAdditionError,
89 SSHKeyType,
90@@ -4122,8 +4123,23 @@
91 super(SSHKey, self).destroySelf()
92
93 def getFullKeyText(self):
94- return "%s %s %s" % (
95- SSH_KEY_TYPE_TO_TEXT[self.keytype], self.keytext, self.comment)
96+ try:
97+ ssh_keytype = getNS(base64.b64decode(self.keytext))[0]
98+ except Exception as e:
99+ # We didn't always validate keys, so there might be some that
100+ # can't be loaded this way.
101+ if self.keytype == SSHKeyType.RSA:
102+ ssh_keytype = "ssh-rsa"
103+ elif self.keytype == SSHKeyType.DSA:
104+ ssh_keytype = "ssh-dss"
105+ else:
106+ # There's no single ssh_keytype for ECDSA keys (it depends
107+ # on the curve), and we've always validated these so this
108+ # shouldn't happen.
109+ raise ValueError(
110+ "SSH key of type %s has invalid data '%s'" %
111+ (self.keytype, self.keytext))
112+ return "%s %s %s" % (ssh_keytype, self.keytext, self.comment)
113
114
115 @implementer(ISSHKeySet)
116@@ -4131,13 +4147,16 @@
117
118 def new(self, person, sshkey, check_key=True, send_notification=True,
119 dry_run=False):
120- keytype, keytext, comment = self._extract_ssh_key_components(sshkey)
121+ kind, keytype, keytext, comment = self._extract_ssh_key_components(
122+ sshkey)
123
124 if check_key:
125 try:
126- Key.fromString(sshkey)
127+ key = Key.fromString(sshkey)
128 except Exception as e:
129 raise SSHKeyAdditionError(key=sshkey, exception=e)
130+ if kind != key.sshType():
131+ raise SSHKeyAdditionError(type_mismatch=(kind, key.sshType()))
132
133 if send_notification:
134 person.security_field_changed(
135@@ -4161,7 +4180,7 @@
136 """ % sqlvalues([person.id for person in people]))
137
138 def getByPersonAndKeyText(self, person, sshkey):
139- keytype, keytext, comment = self._extract_ssh_key_components(sshkey)
140+ _, keytype, keytext, comment = self._extract_ssh_key_components(sshkey)
141 return IStore(SSHKey).find(
142 SSHKey,
143 person=person, keytype=keytype, keytext=keytext, comment=comment)
144@@ -4178,7 +4197,7 @@
145 keytype = SSH_TEXT_TO_KEY_TYPE.get(kind)
146 if keytype is None:
147 raise SSHKeyAdditionError(kind=kind)
148- return keytype, keytext, comment
149+ return kind, keytype, keytext, comment
150
151
152 @implementer(IWikiName)
153
154=== modified file 'lib/lp/registry/scripts/listteammembers.py'
155--- lib/lp/registry/scripts/listteammembers.py 2016-05-23 03:17:16 +0000
156+++ lib/lp/registry/scripts/listteammembers.py 2018-07-02 14:38:50 +0000
157@@ -11,7 +11,6 @@
158 from zope.component import getUtility
159
160 from lp.registry.interfaces.person import IPersonSet
161-from lp.registry.interfaces.ssh import SSH_KEY_TYPE_TO_TEXT
162
163
164 OUTPUT_TEMPLATES = {
165@@ -30,11 +29,8 @@
166 bad_ssh_pattern = re.compile('[\r\n\f]')
167
168
169-def make_sshkey_params(member, type_name, key):
170- sshkey = "%s %s %s" % (
171- type_name,
172- bad_ssh_pattern.sub('', key.keytext),
173- bad_ssh_pattern.sub('', key.comment).strip())
174+def make_sshkey_params(member, key):
175+ sshkey = bad_ssh_pattern.sub('', key.getFullKeyText()).strip()
176 return dict(name=member.name, sshkey=sshkey)
177
178
179@@ -62,8 +58,7 @@
180 sshkey = '--none--'
181 if display_option == 'sshkeys':
182 for key in member.sshkeys:
183- type_name = SSH_KEY_TYPE_TO_TEXT[key.keytype]
184- params = make_sshkey_params(member, type_name, key)
185+ params = make_sshkey_params(member, key)
186 output.append(template % params)
187 # Ubuntite
188 ubuntite = "no"
189
190=== modified file 'lib/lp/registry/stories/person/xx-add-sshkey.txt'
191--- lib/lp/registry/stories/person/xx-add-sshkey.txt 2017-01-11 18:45:55 +0000
192+++ lib/lp/registry/stories/person/xx-add-sshkey.txt 2018-07-02 14:38:50 +0000
193@@ -57,8 +57,9 @@
194 Change your SSH keys...
195
196 Any key must be of the form "keytype keytext comment", where keytype must be
197-either ssh-rsa or ssh-dss. If the key doesn't match the expected format, an
198-error message will be shown.
199+one of ssh-rsa, ssh-dss, ecdsa-sha2-nistp256, ecdsa-sha2-nistp284, or
200+ecdsa-sha2-nistp521. If the key doesn't match the expected format, an error
201+message will be shown.
202
203 >>> sshkey = "ssh-rsa "
204 >>> browser.getControl(name='sshkey').value = sshkey
205@@ -82,7 +83,7 @@
206 Invalid public key
207
208
209-Now, Salgado will upload one RSA and one DSA keys, matching the expected
210+Now, Salgado will upload one of each type of key, matching the expected
211 format.
212
213 >>> sshkey = "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA6VVQrIoBhxSB7duD69PRzYfdJz3QNUky5lSOpl6a9hEP9iAU72RK3fr4uaYiEEjr70EDAROCimi/rtkBuWCRmPJbQDpzBoZ7PDW/jF5tWAuC4+5z/fy05HOhHRH8WGzeEuWn5HBflcx1QasMD95oDiiEuQbF/kGxBM5/no/4FeJU3fgc+1XQNH7tMDQIcaqoHarc2kefGC8/sbRwbzajhg9yeqskgs6o6y+7931/bcZSLZ/wU53m5nB7eVkkVihk7KD+sf9jKG91LnaRW1IjBgo8AAbXl+e556XkwIwVoieKNYW2Fvw8ybcW5rCTvJ1e/3Cvo2hw8ZsDMRofSifiKw== salgado@canario"
214@@ -101,6 +102,30 @@
215 ... print tag.renderContents()
216 SSH public key added.
217
218+ >>> sshkey = "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBJseCUmxVG7D6qh4JmhLp0Du4kScScJ9PtZ0LGHYHaURnRw9tbX1wwURAio8og6dbnT75CQ3TbUE/xJhxI0aFXE= salgado@canario"
219+ >>> browser.getControl(name='sshkey').value = sshkey
220+ >>> browser.getControl('Import Public Key').click()
221+ >>> soup = find_main_content(browser.contents)
222+ >>> for tag in soup('p', 'informational message'):
223+ ... print tag.renderContents()
224+ SSH public key added.
225+
226+ >>> sshkey = "ecdsa-sha2-nistp384 AAAAE2VjZHNhLXNoYTItbmlzdHAzODQAAAAIbmlzdHAzODQAAABhBDUR0E0zCHRHJER6uzjfE/o0HAHFLcq/n8lp0duThpeIPsmo+wr3vHHuAAyOddOgkuQC8Lj8FzHlrOEYgXL6qa7FvpviE9YWUgmqVDa/yJbL/m6Mg8fvSIXlDJKmvOSv6g== salgado@canario"
227+ >>> browser.getControl(name='sshkey').value = sshkey
228+ >>> browser.getControl('Import Public Key').click()
229+ >>> soup = find_main_content(browser.contents)
230+ >>> for tag in soup('p', 'informational message'):
231+ ... print tag.renderContents()
232+ SSH public key added.
233+
234+ >>> sshkey = "ecdsa-sha2-nistp521 AAAAE2VjZHNhLXNoYTItbmlzdHA1MjEAAAAIbmlzdHA1MjEAAACFBAB3rpD+Ozb/kwUOqCZUXSiruAkIx6sNZLJyjJ0zxVTZSannaysCLxMQ/IiVxCd59+U2NaLduMzd93JcYDRlX3M5+AApY+3JjfSPo01Sb17HTLNSYU3RZWx0A3XJxm/YN+x/iuYZ3IziuAKeYMsNsdfHlO4/IWjw4Ruy0enW+QhWaY2qAQ== salgado@canario"
235+ >>> browser.getControl(name='sshkey').value = sshkey
236+ >>> browser.getControl('Import Public Key').click()
237+ >>> soup = find_main_content(browser.contents)
238+ >>> for tag in soup('p', 'informational message'):
239+ ... print tag.renderContents()
240+ SSH public key added.
241+
242 Launchpad administrators are not allowed to poke at other user's ssh keys.
243
244 >>> login(ANONYMOUS)
245@@ -118,11 +143,13 @@
246 >>> browser.open('http://launchpad.dev/~salgado')
247 >>> print browser.title
248 Guilherme Salgado in Launchpad
249- >>> print extract_text(
250- ... find_tag_by_id(browser.contents, 'sshkeys'))
251+ >>> print extract_text(find_tag_by_id(browser.contents, 'sshkeys'))
252 SSH keys: Update SSH keys
253 salgado@canario
254 salgado@canario
255+ salgado@canario
256+ salgado@canario
257+ salgado@canario
258 >>> browser.getLink('Update SSH keys').click()
259 >>> print browser.title
260 Change your SSH keys...
261
262=== modified file 'lib/lp/registry/templates/person-editsshkeys.pt'
263--- lib/lp/registry/templates/person-editsshkeys.pt 2012-05-31 02:20:41 +0000
264+++ lib/lp/registry/templates/person-editsshkeys.pt 2018-07-02 14:38:50 +0000
265@@ -47,7 +47,8 @@
266 <label>Public key line</label>
267 <div class="formHelp">
268 Insert the contents of your public key (usually
269- <code>~/.ssh/id_dsa.pub</code> or <code>~/.ssh/id_rsa.pub</code>).
270+ <code>~/.ssh/id_rsa.pub</code>, <code>~/.ssh/id_dsa.pub</code>, or
271+ <code>~/.ssh/id_ecdsa.pub</code>).
272 Only SSH v2 keys are supported.
273 <a href="https://help.launchpad.net/YourAccount/CreatingAnSSHKeyPair">
274 How do I create a public key?
275
276=== modified file 'lib/lp/registry/tests/test_ssh.py'
277--- lib/lp/registry/tests/test_ssh.py 2018-06-25 15:14:38 +0000
278+++ lib/lp/registry/tests/test_ssh.py 2018-07-02 14:38:50 +0000
279@@ -12,7 +12,6 @@
280 ISSHKeySet,
281 SSH_TEXT_TO_KEY_TYPE,
282 SSHKeyAdditionError,
283- SSHKeyType,
284 )
285 from lp.testing import (
286 admin_logged_in,
287@@ -31,17 +30,38 @@
288 def test_getFullKeyText_for_rsa_key(self):
289 person = self.factory.makePerson()
290 with person_logged_in(person):
291- key = self.factory.makeSSHKey(person, SSHKeyType.RSA)
292+ key = self.factory.makeSSHKey(person, "ssh-rsa")
293 expected = "ssh-rsa %s %s" % (key.keytext, key.comment)
294 self.assertEqual(expected, key.getFullKeyText())
295
296 def test_getFullKeyText_for_dsa_key(self):
297 person = self.factory.makePerson()
298 with person_logged_in(person):
299- key = self.factory.makeSSHKey(person, SSHKeyType.DSA)
300+ key = self.factory.makeSSHKey(person, "ssh-dss")
301 expected = "ssh-dss %s %s" % (key.keytext, key.comment)
302 self.assertEqual(expected, key.getFullKeyText())
303
304+ def test_getFullKeyText_for_ecdsa_nistp256_key(self):
305+ person = self.factory.makePerson()
306+ with person_logged_in(person):
307+ key = self.factory.makeSSHKey(person, "ecdsa-sha2-nistp256")
308+ expected = "ecdsa-sha2-nistp256 %s %s" % (key.keytext, key.comment)
309+ self.assertEqual(expected, key.getFullKeyText())
310+
311+ def test_getFullKeyText_for_ecdsa_nistp384_key(self):
312+ person = self.factory.makePerson()
313+ with person_logged_in(person):
314+ key = self.factory.makeSSHKey(person, "ecdsa-sha2-nistp384")
315+ expected = "ecdsa-sha2-nistp384 %s %s" % (key.keytext, key.comment)
316+ self.assertEqual(expected, key.getFullKeyText())
317+
318+ def test_getFullKeyText_for_ecdsa_nistp521_key(self):
319+ person = self.factory.makePerson()
320+ with person_logged_in(person):
321+ key = self.factory.makeSSHKey(person, "ecdsa-sha2-nistp521")
322+ expected = "ecdsa-sha2-nistp521 %s %s" % (key.keytext, key.comment)
323+ self.assertEqual(expected, key.getFullKeyText())
324+
325 def test_destroySelf_sends_notification_by_default(self):
326 person = self.factory.makePerson()
327 with person_logged_in(person):
328@@ -58,7 +78,7 @@
329 % key.comment)
330 )
331
332- def test_destroySelf_notications_can_be_supressed(self):
333+ def test_destroySelf_notifications_can_be_suppressed(self):
334 person = self.factory.makePerson()
335 with person_logged_in(person):
336 key = self.factory.makeSSHKey(person, send_notification=False)
337@@ -179,6 +199,14 @@
338 )
339 self.assertRaisesWithContent(
340 SSHKeyAdditionError,
341+ "Invalid SSH key data: key type 'ssh-rsa' does not match key "
342+ "data type 'ssh-dss'",
343+ keyset.new,
344+ person,
345+ 'ssh-rsa ' + self.factory.makeSSHKeyText(key_type='ssh-dss')[8:]
346+ )
347+ self.assertRaisesWithContent(
348+ SSHKeyAdditionError,
349 "Invalid SSH key data: 'None'",
350 keyset.new,
351 person, None
352
353=== modified file 'lib/lp/testing/factory.py'
354--- lib/lp/testing/factory.py 2018-06-30 16:09:44 +0000
355+++ lib/lp/testing/factory.py 2018-07-02 14:38:50 +0000
356@@ -20,6 +20,7 @@
357 ]
358
359 import base64
360+from cryptography.utils import int_to_bytes
361 from datetime import (
362 datetime,
363 timedelta,
364@@ -228,11 +229,7 @@
365 SourcePackageUrgency,
366 )
367 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
368-from lp.registry.interfaces.ssh import (
369- ISSHKeySet,
370- SSH_KEY_TYPE_TO_TEXT,
371- SSHKeyType,
372- )
373+from lp.registry.interfaces.ssh import ISSHKeySet
374 from lp.registry.model.commercialsubscription import CommercialSubscription
375 from lp.registry.model.karma import KarmaTotalCache
376 from lp.registry.model.milestone import Milestone
377@@ -4311,37 +4308,56 @@
378 return getUtility(IHWSubmissionDeviceSet).create(
379 device_driver_link, submission, parent, hal_device_id)
380
381- def makeSSHKeyText(self, key_type=SSHKeyType.RSA, comment=None):
382+ def makeSSHKeyText(self, key_type="ssh-rsa", comment=None):
383 """Create new SSH public key text.
384
385- :param key_type: If specified, the type of SSH key to generate. Must be
386- a member of SSHKeyType. If unspecified, SSHKeyType.RSA is used.
387+ :param key_type: If specified, the type of SSH key to generate, as a
388+ public key algorithm name
389+ (https://www.iana.org/assignments/ssh-parameters/). Must be a
390+ member of SSH_TEXT_TO_KEY_TYPE. If unspecified, "ssh-rsa" is
391+ used.
392 """
393- key_type_string = SSH_KEY_TYPE_TO_TEXT.get(key_type)
394- if key_type is None:
395- raise AssertionError(
396- "key_type must be a member of SSHKeyType, not %r" % key_type)
397- if key_type == SSHKeyType.RSA:
398+ parameters = None
399+ if key_type == "ssh-rsa":
400 parameters = [MP(keydata.RSAData[param]) for param in ("e", "n")]
401- elif key_type == SSHKeyType.DSA:
402+ elif key_type == "ssh-dss":
403 parameters = [
404 MP(keydata.DSAData[param]) for param in ("p", "q", "g", "y")]
405- else:
406+ elif key_type.startswith("ecdsa-sha2-"):
407+ curve = key_type[len("ecdsa-sha2-"):]
408+ key_size, curve_data = {
409+ "nistp256": (256, keydata.ECDatanistp256),
410+ "nistp384": (384, keydata.ECDatanistp384),
411+ "nistp521": (521, keydata.ECDatanistp521),
412+ }.get(curve, (None, None))
413+ if curve_data is not None:
414+ key_byte_length = (key_size + 7) // 8
415+ parameters = [
416+ NS(curve_data["curve"][-8:]),
417+ NS(b"\x04" +
418+ int_to_bytes(curve_data["x"], key_byte_length) +
419+ int_to_bytes(curve_data["y"], key_byte_length)),
420+ ]
421+ if parameters is None:
422 raise AssertionError(
423- "key_type must be a member of SSHKeyType, not %r" % key_type)
424- key_text = base64.b64encode(NS(key_type_string) + b"".join(parameters))
425+ "key_type must be a member of SSH_TEXT_TO_KEY_TYPE, not %r" %
426+ key_type)
427+ key_text = base64.b64encode(NS(key_type) + b"".join(parameters))
428 if comment is None:
429 comment = self.getUniqueString()
430- return "%s %s %s" % (key_type_string, key_text, comment)
431+ return "%s %s %s" % (key_type, key_text, comment)
432
433- def makeSSHKey(self, person=None, key_type=SSHKeyType.RSA,
434+ def makeSSHKey(self, person=None, key_type="ssh-rsa",
435 send_notification=True):
436 """Create a new SSHKey.
437
438 :param person: If specified, the person to attach the key to. If
439 unspecified, a person is created.
440- :param key_type: If specified, the type of SSH key to generate. Must be
441- a member of SSHKeyType. If unspecified, SSHKeyType.RSA is used.
442+ :param key_type: If specified, the type of SSH key to generate, as a
443+ public key algorithm name
444+ (https://www.iana.org/assignments/ssh-parameters/). Must be a
445+ member of SSH_TEXT_TO_KEY_TYPE. If unspecified, "ssh-rsa" is
446+ used.
447 """
448 if person is None:
449 person = self.makePerson()