launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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

Commit message:
Skip compromised SSH key check if ssh-vulnkey is absent (removed in openssh 1:6.5p1-1).

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/tolerate-missing-ssh-vulnkey/+merge/314557

We could also remove this entirely, of course, but I'm more comfortable with it staying in place for as long as we have the necessary OS support, and we might well want the exception again in future.

Also, I'm sure I've wanted find_in_path in Launchpad in the past, so this is a good excuse to add it.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/tolerate-missing-ssh-vulnkey into lp:launchpad.

=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py	2016-05-25 16:41:05 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py	2017-01-11 18:56:29 +0000
@@ -1,8 +1,10 @@
-# 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).
 
 __metaclass__ = type
 
+from unittest import skipUnless
+
 from storm.store import Store
 from zope.component import getUtility
 from zope.security.management import endInteraction
@@ -20,6 +22,7 @@
     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 (
@@ -559,6 +562,7 @@
             "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()
@@ -569,6 +573,7 @@
             "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()

=== modified file 'lib/lp/registry/browser/tests/test_sshkey.py'
--- lib/lp/registry/browser/tests/test_sshkey.py	2015-10-26 14:54:43 +0000
+++ lib/lp/registry/browser/tests/test_sshkey.py	2017-01-11 18:56:29 +0000
@@ -1,13 +1,16 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for GPG key on the web."""
 
 __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,
@@ -18,6 +21,7 @@
 from lp.testing.pages import (
     extract_text,
     find_tags_by_class,
+    get_feedback_messages,
     setupBrowserFreshLogin,
     )
 from lp.testing.views import create_initialized_view
@@ -73,3 +77,33 @@
         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/model/person.py'
--- lib/lp/registry/model/person.py	2016-09-19 12:33:59 +0000
+++ lib/lp/registry/model/person.py	2017-01-11 18:56:29 +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).
 
 """Implementation classes for a Person."""
@@ -205,11 +205,11 @@
 from lp.registry.interfaces.ssh import (
     ISSHKey,
     ISSHKeySet,
+    SSH_KEY_TYPE_TO_TEXT,
+    SSH_TEXT_TO_KEY_TYPE,
     SSHKeyAdditionError,
     SSHKeyCompromisedError,
     SSHKeyType,
-    SSH_KEY_TYPE_TO_TEXT,
-    SSH_TEXT_TO_KEY_TYPE,
     )
 from lp.registry.interfaces.teammembership import (
     IJoinTeamEvent,
@@ -294,6 +294,7 @@
     )
 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,
@@ -4097,13 +4098,15 @@
     def new(self, person, sshkey, send_notification=True, dry_run=False):
         keytype, keytext, comment = self._extract_ssh_key_components(sshkey)
 
-        process = subprocess.Popen(
-            '/usr/bin/ssh-vulnkey -', shell=True, 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 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(

=== modified file 'lib/lp/registry/stories/person/xx-add-sshkey.txt'
--- lib/lp/registry/stories/person/xx-add-sshkey.txt	2013-09-27 04:13:23 +0000
+++ lib/lp/registry/stories/person/xx-add-sshkey.txt	2017-01-11 18:56:29 +0000
@@ -173,29 +173,13 @@
     >>> assert payload.startswith('The SSH Key')
 
 
-== Blacklisted keys ==
-
-For quite some time there has been a bug on Debian-based distros that
-caused ssh keys generated on these systems to be guessable through
-brute-force attacks, so we don't accept any of these keys.
-
-    >>> compromised_key = 'ssh-dss AAAAB3NzaC1kc3MAAACBAMDMAwIgYxgquosN4grBbVJCuyLXODSkY2x4/jYxUPuj0iUwVl/nTdZ2hitv7DE5dshFGUNm4sizXZoX7/u2Y68av2VHwlIkbQ52qM3ltiPXvS7uP4RjKUEZr+6l6BjohEmnlhhnLdbNy4kj4xTQizE9QSS999PBvQ3csxkOSZNXAAAAFQDZpXHMZqsqy8s0JxTQPg256XEjtwAAAIEAszRf/KwyKHGGTdbQUjOxdfyngk2Fol/1fYRtSGpkooAOMTxfWyOZiEigv6Zqt4VAmXuFpSM/DU0tNrbBPvzKVMkIXwoOfnWimf3ozGuoIxYYLao5pgGqS0dNADOOIaXo6YiVkIYi2YL/7ISq3WLdCeqy9mSZr9z8esdNfW5SiWsAAACAdPqgY1eCyEfxWCEH+Nz4bsig1DkgdZX27QMzW27xJdN03GPUABA5HSRHY/QwvpgD+2PlwNf44ceiWEgcPToyWd/7koPoDga8im/B+ui5j2PQr++prQCa849UMk6Ol9kZWjvNMvk1gM9Rw732DK0FSj0qzk83iK4eqfrZIk3u1a4= maddog39@ubuntu'
-    >>> browser.open('http://launchpad.dev/~salgado/+editsshkeys')
-    >>> browser.getControl(name='sshkey').value = compromised_key
-    >>> browser.getControl('Import Public Key').click()
-    >>> print_feedback_messages(browser.contents)
-    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.
-
-
 == Keys containing non-ASCII comments ==
 
 These keys can be imported just like the others which have ASCII
 comments.
 
     >>> key = 'ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAzSc+OzlZaURcX8NK9Hy1VoA1SXXuxFAvLw9ljz6xXEFgodmkSgSE/Pc+nR2fO/hl0rnWi//8oOwkHlwLVPQpor2cjQlceVs9rKaQcrcwRm6Jmi8CYKlEIBq82kpaLwXwK/x5ZsDfFtYUq558C5IKZOnDozthC7REPYK0cQ8gZ4bLf+5hmJ6QO4sSRZcXTZuPvgUixhlazFo6w6GqWbynf29Wp+WkLFGxGF2UE/dI8HyQy2j7ddaLnW50mGfB00B/pYtO246s84097BRUE8XoBC3SvzsZx6IbI3hOd2e834lq6kOj6QI0wu6+GINRCZf5UyNlpJv6X809XBvq68SCgw== St\xc3\xa9phane'
+    >>> browser.open('http://launchpad.dev/~salgado/+editsshkeys')
     >>> browser.getControl(name='sshkey').value = key
     >>> browser.getControl('Import Public Key').click()
     >>> print_feedback_messages(browser.contents)

=== modified file 'lib/lp/registry/tests/test_ssh.py'
--- lib/lp/registry/tests/test_ssh.py	2016-06-02 04:39:33 +0000
+++ lib/lp/registry/tests/test_ssh.py	2017-01-11 18:56:29 +0000
@@ -1,10 +1,12 @@
-# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for SSHKey."""
 
 __metaclass__ = type
 
+from unittest import skipUnless
+
 from testtools.matchers import StartsWith
 from zope.component import getUtility
 
@@ -15,6 +17,7 @@
     SSHKeyCompromisedError,
     SSHKeyType,
     )
+from lp.services.osutils import find_on_path
 from lp.testing import (
     admin_logged_in,
     person_logged_in,
@@ -117,6 +120,7 @@
                        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):

=== modified file 'lib/lp/services/osutils.py'
--- lib/lp/services/osutils.py	2016-02-05 16:51:12 +0000
+++ lib/lp/services/osutils.py	2017-01-11 18:56:29 +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).
 
 """Utilities for doing the sort of thing the os module does."""
@@ -6,6 +6,7 @@
 __metaclass__ = type
 __all__ = [
     'ensure_directory_exists',
+    'find_on_path',
     'get_pid_from_file',
     'kill_by_pidfile',
     'get_pid_from_file',
@@ -179,3 +180,17 @@
 def write_file(path, content):
     with open_for_writing(path, 'w') as f:
         f.write(content)
+
+
+def find_on_path(command):
+    """Is 'command' on the executable search path?"""
+    if "PATH" not in os.environ:
+        return False
+    path = os.environ["PATH"]
+    for element in path.split(os.pathsep):
+        if not element:
+            continue
+        filename = os.path.join(element, command)
+        if os.path.isfile(filename) and os.access(filename, os.X_OK):
+            return True
+    return False

=== modified file 'lib/lp/services/tests/test_osutils.py'
--- lib/lp/services/tests/test_osutils.py	2015-10-14 15:22:01 +0000
+++ lib/lp/services/tests/test_osutils.py	2017-01-11 18:56:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 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).
 
 """Tests for lp.services.osutils."""
@@ -12,6 +12,7 @@
 
 from lp.services.osutils import (
     ensure_directory_exists,
+    find_on_path,
     open_for_writing,
     remove_tree,
     write_file,
@@ -103,3 +104,25 @@
         content = self.getUniqueString()
         write_file(filename, content)
         self.assertThat(filename, FileContains(content))
+
+
+class TestFindOnPath(TestCase):
+
+    def test_missing_environment(self):
+        os.environ.pop("PATH", None)
+        self.assertFalse(find_on_path("ls"))
+
+    def test_present_executable(self):
+        temp_dir = self.makeTemporaryDirectory()
+        bin_dir = os.path.join(temp_dir, "bin")
+        program = os.path.join(bin_dir, "program")
+        write_file(program, "")
+        os.chmod(program, 0o755)
+        os.environ["PATH"] = bin_dir
+        self.assertTrue(find_on_path("program"))
+
+    def test_present_not_executable(self):
+        bin_dir = os.path.join(self.temp_dir, "bin")
+        write_file(os.path.join(bin_dir, "program"), "")
+        os.environ["PATH"] = bin_dir
+        self.assertFalse(find_on_path("program"))