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

[Colin Watson]

Add support for SSH ECDSA keys.

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.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/registry/interfaces/ssh.py'
--- lib/lp/registry/interfaces/ssh.py	2018-06-25 15:14:38 +0000
+++ lib/lp/registry/interfaces/ssh.py	2018-07-02 14:38:50 +0000
@@ -8,7 +8,6 @@
 __all__ = [
     'ISSHKey',
     'ISSHKeySet',
-    'SSH_KEY_TYPE_TO_TEXT',
     'SSH_TEXT_TO_KEY_TYPE',
     'SSHKeyAdditionError',
     'SSHKeyType',
@@ -39,7 +38,7 @@
 class SSHKeyType(DBEnumeratedType):
     """SSH key type
 
-    SSH (version 2) can use RSA or DSA keys for authentication. See
+    SSH (version 2) can use RSA, DSA, or ECDSA keys for authentication.  See
     OpenSSH's ssh-keygen(1) man page for details.
     """
 
@@ -55,14 +54,20 @@
         DSA
         """)
 
-
-SSH_KEY_TYPE_TO_TEXT = {
-    SSHKeyType.RSA: "ssh-rsa",
-    SSHKeyType.DSA: "ssh-dss",
-}
-
-
-SSH_TEXT_TO_KEY_TYPE = {v: k for k, v in SSH_KEY_TYPE_TO_TEXT.items()}
+    ECDSA = DBItem(3, """
+        ECDSA
+
+        ECDSA
+        """)
+
+
+SSH_TEXT_TO_KEY_TYPE = {
+    "ssh-rsa": SSHKeyType.RSA,
+    "ssh-dss": SSHKeyType.DSA,
+    "ecdsa-sha2-nistp256": SSHKeyType.ECDSA,
+    "ecdsa-sha2-nistp384": SSHKeyType.ECDSA,
+    "ecdsa-sha2-nistp521": SSHKeyType.ECDSA,
+    }
 
 
 class ISSHKey(Interface):
@@ -139,6 +144,11 @@
         if 'kind' in kwargs:
             kind = kwargs.pop('kind')
             msg = "Invalid SSH key type: '%s'" % kind
+        if 'type_mismatch' in kwargs:
+            keytype, keydatatype = kwargs.pop('type_mismatch')
+            msg = (
+                "Invalid SSH key data: key type '%s' does not match key data "
+                "type '%s'" % (keytype, keydatatype))
         if 'exception' in kwargs:
             exception = kwargs.pop('exception')
             try:

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2018-06-19 14:46:05 +0000
+++ lib/lp/registry/model/person.py	2018-07-02 14:38:50 +0000
@@ -29,6 +29,7 @@
     'WikiNameSet',
     ]
 
+import base64
 from datetime import (
     datetime,
     timedelta,
@@ -81,6 +82,7 @@
     Store,
     )
 import transaction
+from twisted.conch.ssh.common import getNS
 from twisted.conch.ssh.keys import Key
 from zope.component import (
     adapter,
@@ -205,7 +207,6 @@
 from lp.registry.interfaces.ssh import (
     ISSHKey,
     ISSHKeySet,
-    SSH_KEY_TYPE_TO_TEXT,
     SSH_TEXT_TO_KEY_TYPE,
     SSHKeyAdditionError,
     SSHKeyType,
@@ -4122,8 +4123,23 @@
         super(SSHKey, self).destroySelf()
 
     def getFullKeyText(self):
-        return "%s %s %s" % (
-            SSH_KEY_TYPE_TO_TEXT[self.keytype], self.keytext, self.comment)
+        try:
+            ssh_keytype = getNS(base64.b64decode(self.keytext))[0]
+        except Exception as e:
+            # We didn't always validate keys, so there might be some that
+            # can't be loaded this way.
+            if self.keytype == SSHKeyType.RSA:
+                ssh_keytype = "ssh-rsa"
+            elif self.keytype == SSHKeyType.DSA:
+                ssh_keytype = "ssh-dss"
+            else:
+                # There's no single ssh_keytype for ECDSA keys (it depends
+                # on the curve), and we've always validated these so this
+                # shouldn't happen.
+                raise ValueError(
+                    "SSH key of type %s has invalid data '%s'" %
+                    (self.keytype, self.keytext))
+        return "%s %s %s" % (ssh_keytype, self.keytext, self.comment)
 
 
 @implementer(ISSHKeySet)
@@ -4131,13 +4147,16 @@
 
     def new(self, person, sshkey, check_key=True, send_notification=True,
             dry_run=False):
-        keytype, keytext, comment = self._extract_ssh_key_components(sshkey)
+        kind, keytype, keytext, comment = self._extract_ssh_key_components(
+            sshkey)
 
         if check_key:
             try:
-                Key.fromString(sshkey)
+                key = Key.fromString(sshkey)
             except Exception as e:
                 raise SSHKeyAdditionError(key=sshkey, exception=e)
+            if kind != key.sshType():
+                raise SSHKeyAdditionError(type_mismatch=(kind, key.sshType()))
 
         if send_notification:
             person.security_field_changed(
@@ -4161,7 +4180,7 @@
             """ % sqlvalues([person.id for person in people]))
 
     def getByPersonAndKeyText(self, person, sshkey):
-        keytype, keytext, comment = self._extract_ssh_key_components(sshkey)
+        _, keytype, keytext, comment = self._extract_ssh_key_components(sshkey)
         return IStore(SSHKey).find(
             SSHKey,
             person=person, keytype=keytype, keytext=keytext, comment=comment)
@@ -4178,7 +4197,7 @@
         keytype = SSH_TEXT_TO_KEY_TYPE.get(kind)
         if keytype is None:
             raise SSHKeyAdditionError(kind=kind)
-        return keytype, keytext, comment
+        return kind, keytype, keytext, comment
 
 
 @implementer(IWikiName)

=== modified file 'lib/lp/registry/scripts/listteammembers.py'
--- lib/lp/registry/scripts/listteammembers.py	2016-05-23 03:17:16 +0000
+++ lib/lp/registry/scripts/listteammembers.py	2018-07-02 14:38:50 +0000
@@ -11,7 +11,6 @@
 from zope.component import getUtility
 
 from lp.registry.interfaces.person import IPersonSet
-from lp.registry.interfaces.ssh import SSH_KEY_TYPE_TO_TEXT
 
 
 OUTPUT_TEMPLATES = {
@@ -30,11 +29,8 @@
 bad_ssh_pattern = re.compile('[\r\n\f]')
 
 
-def make_sshkey_params(member, type_name, key):
-    sshkey = "%s %s %s" % (
-        type_name,
-        bad_ssh_pattern.sub('', key.keytext),
-        bad_ssh_pattern.sub('', key.comment).strip())
+def make_sshkey_params(member, key):
+    sshkey = bad_ssh_pattern.sub('', key.getFullKeyText()).strip()
     return dict(name=member.name, sshkey=sshkey)
 
 
@@ -62,8 +58,7 @@
         sshkey = '--none--'
         if display_option == 'sshkeys':
             for key in member.sshkeys:
-                type_name = SSH_KEY_TYPE_TO_TEXT[key.keytype]
-                params = make_sshkey_params(member, type_name, key)
+                params = make_sshkey_params(member, key)
                 output.append(template % params)
         # Ubuntite
         ubuntite = "no"

=== modified file 'lib/lp/registry/stories/person/xx-add-sshkey.txt'
--- lib/lp/registry/stories/person/xx-add-sshkey.txt	2017-01-11 18:45:55 +0000
+++ lib/lp/registry/stories/person/xx-add-sshkey.txt	2018-07-02 14:38:50 +0000
@@ -57,8 +57,9 @@
     Change your SSH keys...
 
 Any key must be of the form "keytype keytext comment", where keytype must be
-either ssh-rsa or ssh-dss.  If the key doesn't match the expected format, an
-error message will be shown.
+one of ssh-rsa, ssh-dss, ecdsa-sha2-nistp256, ecdsa-sha2-nistp284, or
+ecdsa-sha2-nistp521.  If the key doesn't match the expected format, an error
+message will be shown.
 
     >>> sshkey = "ssh-rsa   "
     >>> browser.getControl(name='sshkey').value = sshkey
@@ -82,7 +83,7 @@
     Invalid public key
 
 
-Now, Salgado will upload one RSA and one DSA keys, matching the expected
+Now, Salgado will upload one of each type of key, matching the expected
 format.
 
     >>> sshkey = "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA6VVQrIoBhxSB7duD69PRzYfdJz3QNUky5lSOpl6a9hEP9iAU72RK3fr4uaYiEEjr70EDAROCimi/rtkBuWCRmPJbQDpzBoZ7PDW/jF5tWAuC4+5z/fy05HOhHRH8WGzeEuWn5HBflcx1QasMD95oDiiEuQbF/kGxBM5/no/4FeJU3fgc+1XQNH7tMDQIcaqoHarc2kefGC8/sbRwbzajhg9yeqskgs6o6y+7931/bcZSLZ/wU53m5nB7eVkkVihk7KD+sf9jKG91LnaRW1IjBgo8AAbXl+e556XkwIwVoieKNYW2Fvw8ybcW5rCTvJ1e/3Cvo2hw8ZsDMRofSifiKw== salgado@canario"
@@ -101,6 +102,30 @@
     ...     print tag.renderContents()
     SSH public key added.
 
+    >>> sshkey = "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBJseCUmxVG7D6qh4JmhLp0Du4kScScJ9PtZ0LGHYHaURnRw9tbX1wwURAio8og6dbnT75CQ3TbUE/xJhxI0aFXE= salgado@canario"
+    >>> browser.getControl(name='sshkey').value = sshkey
+    >>> browser.getControl('Import Public Key').click()
+    >>> soup = find_main_content(browser.contents)
+    >>> for tag in soup('p', 'informational message'):
+    ...     print tag.renderContents()
+    SSH public key added.
+
+    >>> sshkey = "ecdsa-sha2-nistp384 AAAAE2VjZHNhLXNoYTItbmlzdHAzODQAAAAIbmlzdHAzODQAAABhBDUR0E0zCHRHJER6uzjfE/o0HAHFLcq/n8lp0duThpeIPsmo+wr3vHHuAAyOddOgkuQC8Lj8FzHlrOEYgXL6qa7FvpviE9YWUgmqVDa/yJbL/m6Mg8fvSIXlDJKmvOSv6g== salgado@canario"
+    >>> browser.getControl(name='sshkey').value = sshkey
+    >>> browser.getControl('Import Public Key').click()
+    >>> soup = find_main_content(browser.contents)
+    >>> for tag in soup('p', 'informational message'):
+    ...     print tag.renderContents()
+    SSH public key added.
+
+    >>> sshkey = "ecdsa-sha2-nistp521 AAAAE2VjZHNhLXNoYTItbmlzdHA1MjEAAAAIbmlzdHA1MjEAAACFBAB3rpD+Ozb/kwUOqCZUXSiruAkIx6sNZLJyjJ0zxVTZSannaysCLxMQ/IiVxCd59+U2NaLduMzd93JcYDRlX3M5+AApY+3JjfSPo01Sb17HTLNSYU3RZWx0A3XJxm/YN+x/iuYZ3IziuAKeYMsNsdfHlO4/IWjw4Ruy0enW+QhWaY2qAQ== salgado@canario"
+    >>> browser.getControl(name='sshkey').value = sshkey
+    >>> browser.getControl('Import Public Key').click()
+    >>> soup = find_main_content(browser.contents)
+    >>> for tag in soup('p', 'informational message'):
+    ...     print tag.renderContents()
+    SSH public key added.
+
 Launchpad administrators are not allowed to poke at other user's ssh keys.
 
     >>> login(ANONYMOUS)
@@ -118,11 +143,13 @@
     >>> browser.open('http://launchpad.dev/~salgado')
     >>> print browser.title
     Guilherme Salgado in Launchpad
-    >>> print extract_text(
-    ...     find_tag_by_id(browser.contents, 'sshkeys'))
+    >>> print extract_text(find_tag_by_id(browser.contents, 'sshkeys'))
     SSH keys: Update SSH keys
     salgado@canario
     salgado@canario
+    salgado@canario
+    salgado@canario
+    salgado@canario
     >>> browser.getLink('Update SSH keys').click()
     >>> print browser.title
     Change your SSH keys...

=== modified file 'lib/lp/registry/templates/person-editsshkeys.pt'
--- lib/lp/registry/templates/person-editsshkeys.pt	2012-05-31 02:20:41 +0000
+++ lib/lp/registry/templates/person-editsshkeys.pt	2018-07-02 14:38:50 +0000
@@ -47,7 +47,8 @@
         <label>Public key line</label>
         <div class="formHelp">
           Insert the contents of your public key (usually
-          <code>~/.ssh/id_dsa.pub</code> or <code>~/.ssh/id_rsa.pub</code>).
+          <code>~/.ssh/id_rsa.pub</code>, <code>~/.ssh/id_dsa.pub</code>, or
+          <code>~/.ssh/id_ecdsa.pub</code>).
           Only SSH v2 keys are supported.
           <a href="https://help.launchpad.net/YourAccount/CreatingAnSSHKeyPair">
             How do I create a public key?

=== modified file 'lib/lp/registry/tests/test_ssh.py'
--- lib/lp/registry/tests/test_ssh.py	2018-06-25 15:14:38 +0000
+++ lib/lp/registry/tests/test_ssh.py	2018-07-02 14:38:50 +0000
@@ -12,7 +12,6 @@
     ISSHKeySet,
     SSH_TEXT_TO_KEY_TYPE,
     SSHKeyAdditionError,
-    SSHKeyType,
     )
 from lp.testing import (
     admin_logged_in,
@@ -31,17 +30,38 @@
     def test_getFullKeyText_for_rsa_key(self):
         person = self.factory.makePerson()
         with person_logged_in(person):
-            key = self.factory.makeSSHKey(person, SSHKeyType.RSA)
+            key = self.factory.makeSSHKey(person, "ssh-rsa")
         expected = "ssh-rsa %s %s" % (key.keytext, key.comment)
         self.assertEqual(expected, key.getFullKeyText())
 
     def test_getFullKeyText_for_dsa_key(self):
         person = self.factory.makePerson()
         with person_logged_in(person):
-            key = self.factory.makeSSHKey(person, SSHKeyType.DSA)
+            key = self.factory.makeSSHKey(person, "ssh-dss")
         expected = "ssh-dss %s %s" % (key.keytext, key.comment)
         self.assertEqual(expected, key.getFullKeyText())
 
+    def test_getFullKeyText_for_ecdsa_nistp256_key(self):
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            key = self.factory.makeSSHKey(person, "ecdsa-sha2-nistp256")
+        expected = "ecdsa-sha2-nistp256 %s %s" % (key.keytext, key.comment)
+        self.assertEqual(expected, key.getFullKeyText())
+
+    def test_getFullKeyText_for_ecdsa_nistp384_key(self):
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            key = self.factory.makeSSHKey(person, "ecdsa-sha2-nistp384")
+        expected = "ecdsa-sha2-nistp384 %s %s" % (key.keytext, key.comment)
+        self.assertEqual(expected, key.getFullKeyText())
+
+    def test_getFullKeyText_for_ecdsa_nistp521_key(self):
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            key = self.factory.makeSSHKey(person, "ecdsa-sha2-nistp521")
+        expected = "ecdsa-sha2-nistp521 %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):
@@ -58,7 +78,7 @@
                     % key.comment)
             )
 
-    def test_destroySelf_notications_can_be_supressed(self):
+    def test_destroySelf_notifications_can_be_suppressed(self):
         person = self.factory.makePerson()
         with person_logged_in(person):
             key = self.factory.makeSSHKey(person, send_notification=False)
@@ -179,6 +199,14 @@
         )
         self.assertRaisesWithContent(
             SSHKeyAdditionError,
+            "Invalid SSH key data: key type 'ssh-rsa' does not match key "
+            "data type 'ssh-dss'",
+            keyset.new,
+            person,
+            'ssh-rsa ' + self.factory.makeSSHKeyText(key_type='ssh-dss')[8:]
+        )
+        self.assertRaisesWithContent(
+            SSHKeyAdditionError,
             "Invalid SSH key data: 'None'",
             keyset.new,
             person, None

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2018-06-30 16:09:44 +0000
+++ lib/lp/testing/factory.py	2018-07-02 14:38:50 +0000
@@ -20,6 +20,7 @@
     ]
 
 import base64
+from cryptography.utils import int_to_bytes
 from datetime import (
     datetime,
     timedelta,
@@ -228,11 +229,7 @@
     SourcePackageUrgency,
     )
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
-from lp.registry.interfaces.ssh import (
-    ISSHKeySet,
-    SSH_KEY_TYPE_TO_TEXT,
-    SSHKeyType,
-    )
+from lp.registry.interfaces.ssh import ISSHKeySet
 from lp.registry.model.commercialsubscription import CommercialSubscription
 from lp.registry.model.karma import KarmaTotalCache
 from lp.registry.model.milestone import Milestone
@@ -4311,37 +4308,56 @@
         return getUtility(IHWSubmissionDeviceSet).create(
             device_driver_link, submission, parent, hal_device_id)
 
-    def makeSSHKeyText(self, key_type=SSHKeyType.RSA, comment=None):
+    def makeSSHKeyText(self, key_type="ssh-rsa", comment=None):
         """Create new SSH public key text.
 
-        :param key_type: If specified, the type of SSH key to generate. Must be
-            a member of SSHKeyType. If unspecified, SSHKeyType.RSA is used.
+        :param key_type: If specified, the type of SSH key to generate, as a
+            public key algorithm name
+            (https://www.iana.org/assignments/ssh-parameters/).  Must be a
+            member of SSH_TEXT_TO_KEY_TYPE.  If unspecified, "ssh-rsa" is
+            used.
         """
-        key_type_string = SSH_KEY_TYPE_TO_TEXT.get(key_type)
-        if key_type is None:
-            raise AssertionError(
-                "key_type must be a member of SSHKeyType, not %r" % key_type)
-        if key_type == SSHKeyType.RSA:
+        parameters = None
+        if key_type == "ssh-rsa":
             parameters = [MP(keydata.RSAData[param]) for param in ("e", "n")]
-        elif key_type == SSHKeyType.DSA:
+        elif key_type == "ssh-dss":
             parameters = [
                 MP(keydata.DSAData[param]) for param in ("p", "q", "g", "y")]
-        else:
+        elif key_type.startswith("ecdsa-sha2-"):
+            curve = key_type[len("ecdsa-sha2-"):]
+            key_size, curve_data = {
+                "nistp256": (256, keydata.ECDatanistp256),
+                "nistp384": (384, keydata.ECDatanistp384),
+                "nistp521": (521, keydata.ECDatanistp521),
+                }.get(curve, (None, None))
+            if curve_data is not None:
+                key_byte_length = (key_size + 7) // 8
+                parameters = [
+                    NS(curve_data["curve"][-8:]),
+                    NS(b"\x04" +
+                       int_to_bytes(curve_data["x"], key_byte_length) +
+                       int_to_bytes(curve_data["y"], key_byte_length)),
+                    ]
+        if parameters is None:
             raise AssertionError(
-                "key_type must be a member of SSHKeyType, not %r" % key_type)
-        key_text = base64.b64encode(NS(key_type_string) + b"".join(parameters))
+                "key_type must be a member of SSH_TEXT_TO_KEY_TYPE, not %r" %
+                key_type)
+        key_text = base64.b64encode(NS(key_type) + b"".join(parameters))
         if comment is None:
             comment = self.getUniqueString()
-        return "%s %s %s" % (key_type_string, key_text, comment)
+        return "%s %s %s" % (key_type, key_text, comment)
 
-    def makeSSHKey(self, person=None, key_type=SSHKeyType.RSA,
+    def makeSSHKey(self, person=None, key_type="ssh-rsa",
                    send_notification=True):
         """Create a new SSHKey.
 
         :param person: If specified, the person to attach the key to. If
             unspecified, a person is created.
-        :param key_type: If specified, the type of SSH key to generate. Must be
-            a member of SSHKeyType. If unspecified, SSHKeyType.RSA is used.
+        :param key_type: If specified, the type of SSH key to generate, as a
+            public key algorithm name
+            (https://www.iana.org/assignments/ssh-parameters/).  Must be a
+            member of SSH_TEXT_TO_KEY_TYPE.  If unspecified, "ssh-rsa" is
+            used.
         """
         if person is None:
             person = self.makePerson()