launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/remove-ssh-vulnkey into lp:launchpad.

Commit message:
Remove compromised SSH key checking.

Now that all the appservers are on xenial, ssh-vulnkey is no longer
available.  We can reintroduce the basic mechanism later if we need it, but
right now it's just dead code.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/remove-ssh-vulnkey/+merge/324935
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/remove-ssh-vulnkey into lp:launchpad.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2017-04-22 13:13:22 +0000
+++ lib/lp/registry/browser/person.py	2017-06-01 13:21:22 +0000
@@ -180,7 +180,6 @@
 from lp.registry.interfaces.ssh import (
     ISSHKeySet,
     SSHKeyAdditionError,
-    SSHKeyCompromisedError,
     )
 from lp.registry.interfaces.teammembership import (
     ITeamMembershipSet,
@@ -2450,14 +2449,6 @@
             getUtility(ISSHKeySet).new(self.user, sshkey)
         except SSHKeyAdditionError:
             self.error_message = structured('Invalid public key')
-        except SSHKeyCompromisedError:
-            self.error_message = structured(
-                'This key is known to be compromised due to a security flaw '
-                'in the software used to generate it, so it will not be '
-                'accepted by Launchpad. See the full '
-                '<a href="http://www.ubuntu.com/usn/usn-612-2";>Security '
-                'Notice</a> for further information and instructions on how '
-                'to generate another key.')
         else:
             self.info_message = structured('SSH public key added.')
 

=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py	2017-01-11 18:45:55 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py	2017-06-01 13:21:22 +0000
@@ -3,8 +3,6 @@
 
 __metaclass__ = type
 
-from unittest import skipUnless
-
 from storm.store import Store
 from zope.component import getUtility
 from zope.security.management import endInteraction
@@ -16,13 +14,11 @@
     )
 from lp.registry.interfaces.ssh import SSHKeyType
 from lp.registry.interfaces.teammembership import ITeamMembershipSet
-from lp.registry.tests.test_ssh import VULNERABLE_RSA_KEY
 from lp.services.identity.interfaces.account import (
     AccountStatus,
     IAccountSet,
     )
 from lp.services.openid.model.openididentifier import OpenIdIdentifier
-from lp.services.osutils import find_on_path
 from lp.services.webapp import snapshot
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.testing import (
@@ -562,28 +558,6 @@
             "Invalid SSH key type: 'foo'",
             response.body)
 
-    @skipUnless(find_on_path("ssh-vulnkey"), "requires ssh-vulnkey")
-    def test_addSSHKeyFromSSO_rejects_vulnerable_keys(self):
-        with admin_logged_in():
-            person = self.factory.makePerson()
-            openid_id = person.account.openid_identifiers.any().identifier
-        response = self.addSSHKeyForPerson(openid_id, VULNERABLE_RSA_KEY)
-        self.assertEqual(400, response.status)
-        self.assertEqual(
-            "This key cannot be added as it is known to be compromised.",
-            response.body)
-
-    @skipUnless(find_on_path("ssh-vulnkey"), "requires ssh-vulnkey")
-    def test_addSSHKeyFromSSO_rejects_vulnerable_keys_dry_run(self):
-        with admin_logged_in():
-            person = self.factory.makePerson()
-            openid_id = person.account.openid_identifiers.any().identifier
-        response = self.addSSHKeyForPerson(openid_id, VULNERABLE_RSA_KEY, True)
-        self.assertEqual(400, response.status)
-        self.assertEqual(
-            "This key cannot be added as it is known to be compromised.",
-            response.body)
-
     def test_addSSHKeyFromSSO_works(self):
         with admin_logged_in():
             person = removeSecurityProxy(self.factory.makePerson())

=== modified file 'lib/lp/registry/browser/tests/test_sshkey.py'
--- lib/lp/registry/browser/tests/test_sshkey.py	2017-01-11 18:45:55 +0000
+++ lib/lp/registry/browser/tests/test_sshkey.py	2017-06-01 13:21:22 +0000
@@ -5,12 +5,9 @@
 
 __metaclass__ = type
 
-from unittest import skipUnless
-
 from zope.component import getUtility
 
 from lp.registry.interfaces.ssh import ISSHKeySet
-from lp.services.osutils import find_on_path
 from lp.services.webapp import canonical_url
 from lp.testing import (
     login_person,
@@ -21,7 +18,6 @@
 from lp.testing.pages import (
     extract_text,
     find_tags_by_class,
-    get_feedback_messages,
     setupBrowserFreshLogin,
     )
 from lp.testing.views import create_initialized_view
@@ -77,33 +73,3 @@
         expected_url = (
             '%s/+editsshkeys/+login?reauth=1' % canonical_url(person))
         self.assertEqual(expected_url, response.getHeader('location'))
-
-    @skipUnless(find_on_path("ssh-vulnkey"), "requires ssh-vulnkey")
-    def test_blacklisted_keys(self):
-        """+editsshkeys refuses keys known to be compromised."""
-        person = self.factory.makePerson()
-        url = '%s/+editsshkeys' % canonical_url(person)
-        compromised_key = (
-            'ssh-dss AAAAB3NzaC1kc3MAAACBAMDMAwIgYxgquosN4grBbVJCuyLXODSkY2x4/'
-            'jYxUPuj0iUwVl/nTdZ2hitv7DE5dshFGUNm4sizXZoX7/u2Y68av2VHwlIkbQ52qM'
-            '3ltiPXvS7uP4RjKUEZr+6l6BjohEmnlhhnLdbNy4kj4xTQizE9QSS999PBvQ3csxk'
-            'OSZNXAAAAFQDZpXHMZqsqy8s0JxTQPg256XEjtwAAAIEAszRf/KwyKHGGTdbQUjOx'
-            'dfyngk2Fol/1fYRtSGpkooAOMTxfWyOZiEigv6Zqt4VAmXuFpSM/DU0tNrbBPvzKV'
-            'MkIXwoOfnWimf3ozGuoIxYYLao5pgGqS0dNADOOIaXo6YiVkIYi2YL/7ISq3WLdCe'
-            'qy9mSZr9z8esdNfW5SiWsAAACAdPqgY1eCyEfxWCEH+Nz4bsig1DkgdZX27QMzW27'
-            'xJdN03GPUABA5HSRHY/QwvpgD+2PlwNf44ceiWEgcPToyWd/7koPoDga8im/B+ui5'
-            'j2PQr++prQCa849UMk6Ol9kZWjvNMvk1gM9Rw732DK0FSj0qzk83iK4eqfrZIk3u1'
-            'a4= maddog39@ubuntu')
-        with person_logged_in(person):
-            browser = setupBrowserFreshLogin(person)
-            browser.open(url)
-            browser.getControl(name='sshkey').value = compromised_key
-            browser.getControl('Import Public Key').click()
-            expected_message = (
-                'This key is known to be compromised due to a security flaw '
-                'in the software used to generate it, so it will not be '
-                'accepted by Launchpad. See the full Security Notice for '
-                'further information and instructions on how to generate '
-                'another key.')
-            self.assertEqual(
-                [expected_message], get_feedback_messages(browser.contents))

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2017-03-21 07:35:08 +0000
+++ lib/lp/registry/interfaces/person.py	2017-06-01 13:21:22 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Person interfaces."""
@@ -2311,8 +2311,6 @@
         :raises NoSuchAccount: If the openid_identifier specified does not
             match any account.
         :raises SSHKeyAdditionError: If the ssh key_text is invalid.
-        :raises SSHKeyCompromisedError: If the ssh key_text is a known
-            compromised key.
         """
 
     @call_with(user=REQUEST_USER)

=== modified file 'lib/lp/registry/interfaces/ssh.py'
--- lib/lp/registry/interfaces/ssh.py	2016-05-25 05:20:30 +0000
+++ lib/lp/registry/interfaces/ssh.py	2017-06-01 13:21:22 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """SSH key interfaces."""
@@ -11,11 +11,11 @@
     'SSH_KEY_TYPE_TO_TEXT',
     'SSH_TEXT_TO_KEY_TYPE',
     'SSHKeyAdditionError',
-    'SSHKeyCompromisedError',
     'SSHKeyType',
     ]
 
 import httplib
+
 from lazr.enum import (
     DBEnumeratedType,
     DBItem,
@@ -122,8 +122,3 @@
 @error_status(httplib.BAD_REQUEST)
 class SSHKeyAdditionError(Exception):
     """Raised when the SSH public key is invalid."""
-
-
-@error_status(httplib.BAD_REQUEST)
-class SSHKeyCompromisedError(Exception):
-    """Raised when the SSH public key is known to be easily compromisable."""

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2017-04-22 13:09:01 +0000
+++ lib/lp/registry/model/person.py	2017-06-01 13:21:22 +0000
@@ -36,7 +36,6 @@
 from operator import attrgetter
 import random
 import re
-import subprocess
 import weakref
 
 from lazr.delegates import delegate_to
@@ -208,7 +207,6 @@
     SSH_KEY_TYPE_TO_TEXT,
     SSH_TEXT_TO_KEY_TYPE,
     SSHKeyAdditionError,
-    SSHKeyCompromisedError,
     SSHKeyType,
     )
 from lp.registry.interfaces.teammembership import (
@@ -294,7 +292,6 @@
     )
 from lp.services.openid.adapters.openid import CurrentOpenIDEndPoint
 from lp.services.openid.model.openididentifier import OpenIdIdentifier
-from lp.services.osutils import find_on_path
 from lp.services.propertycache import (
     cachedproperty,
     get_property_cache,
@@ -4099,16 +4096,6 @@
     def new(self, person, sshkey, send_notification=True, dry_run=False):
         keytype, keytext, comment = self._extract_ssh_key_components(sshkey)
 
-        if find_on_path('ssh-vulnkey'):
-            process = subprocess.Popen(
-                ['ssh-vulnkey', '-'], stdin=subprocess.PIPE,
-                stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-            (out, err) = process.communicate(sshkey.encode('utf-8'))
-            if 'compromised' in out.lower():
-                raise SSHKeyCompromisedError(
-                    "This key cannot be added as it is known to be "
-                    "compromised.")
-
         if send_notification:
             person.security_field_changed(
                 "New SSH key added to your account.",

=== modified file 'lib/lp/registry/tests/test_ssh.py'
--- lib/lp/registry/tests/test_ssh.py	2017-01-11 18:45:55 +0000
+++ lib/lp/registry/tests/test_ssh.py	2017-06-01 13:21:22 +0000
@@ -5,8 +5,6 @@
 
 __metaclass__ = type
 
-from unittest import skipUnless
-
 from testtools.matchers import StartsWith
 from zope.component import getUtility
 
@@ -14,10 +12,8 @@
     ISSHKeySet,
     SSH_TEXT_TO_KEY_TYPE,
     SSHKeyAdditionError,
-    SSHKeyCompromisedError,
     SSHKeyType,
     )
-from lp.services.osutils import find_on_path
 from lp.testing import (
     admin_logged_in,
     person_logged_in,
@@ -27,29 +23,6 @@
 from lp.testing.mail_helpers import pop_notifications
 
 
-VULNERABLE_RSA_KEY = (
-    "ssh-rsa "
-    "AAAAB3NzaC1yc2EAAAABIwAAAQEArPO3VWegIxlZsd9zAAKQ9x3TNXzU0AtM2QF3y92wIDgwX"
-    "DuNcchze3SnTyGDO4l4cIqpvMV8XLZlRUbNDr5hoSGUNPqZxI8ycdfcGyKXLlkMsSKJddnDX7"
-    "+n6Di2KPiluw9IVOmmOk1x80qV0Shrgqoespx+C1ra5omIK/RN2Raf7K5LgaoYOqGHmviacNP"
-    "kFSXOKIOz4cAmWoorEdlHmoWzHN64qvj4Qfh666TuM5pkoxXjWwt1nEn9kxsTd8kB9+Hf7ouM"
-    "54TqNDtlhdLI6xaO9aCXpXg7A+2B0iK1bFtOq8vaQY7NCDQvPnvxEFsNJ4lyogu4SbCDEJejX"
-    "BxOgQ== "
-    "Comment goes here")
-
-VULNERABLE_DSA_KEY = (
-    "ssh-dss "
-    "AAAAB3NzaC1kc3MAAACBANy0bpKtBS+iTLKSMYGmQCxruTuThyn/RyNT/B3LoNNeQmS/LBoQK"
-    "HzQmJDMKOr5TdvooOB3i5wZ0gA868U88WjHJmSQklodOEDeV3xPQmh0hdJm6pd+DrLCeqxhuJ"
-    "LjDuNUahAqVPD8GJWuL88nX3fkjRTsNVUl6O7MbFHf8XyBAAAAFQDpRTNzw5oudxJtW3K8nvh"
-    "COLvZ8wAAAIAVyC46KCN2f5HzMHQceuvNRo+tbGe/SWuX5qwFvyMYjIAuVeW75f+Xi24VgeYZ"
-    "Wn2Da6ieDk0px8ossDx4Qb6AECTPfbVge/D/DtqYxJFaj6zFekCKJXs7TogxIdLHCwcL9M8a0"
-    "X/FVgnPj/DKlHvMkzj4u3IfXKf7bOlBntm0rgAAAIBANST6hbRklJCGPZdC1j24LsDiDmWh+M"
-    "wIhjGj0bjtqjs/B1a7NOqi3ZOjdU2aoyrY2oAo1nF4VDE9uPMj8+LUZV1sw9vfwUTGnzBxFgy"
-    "rV32YhdWvV/mwEMCWiy6+rgCOZlswJ4nfuxF4llnYTsc6c7h1s7jYIMI+K6dZgATAEw== "
-    "Comment goes here")
-
-
 class TestSSHKey(TestCaseWithFactory):
     """Test `ISSHKey`."""
 
@@ -120,15 +93,6 @@
                        send_notification=False)
             self.assertEqual([], pop_notifications())
 
-    @skipUnless(find_on_path("ssh-vulnkey"), "requires ssh-vulnkey")
-    def test_raises_on_vulnerable_keys(self):
-        person = self.factory.makePerson()
-        with person_logged_in(person):
-            keyset = getUtility(ISSHKeySet)
-            for key in (VULNERABLE_DSA_KEY, VULNERABLE_RSA_KEY):
-                self.assertRaises(SSHKeyCompromisedError,
-                                  keyset.new, person, key,)
-
     def test_getByPersonAndKeyText_retrieves_target_key(self):
         person = self.factory.makePerson()
         with person_logged_in(person):