Merge lp:~maxiberta/launchpad/sshkeyadditionerror-msg-format into lp:launchpad

[Maximiliano Bertacchini]

Fix crash while adding an ssh key with unknown type.

Fix encoding crash while adding an ssh key with unknown type. E.g. on an attempt to add this key in SSO /ssh-keys:

    ssh-rsa asdfasdf comment

LP raises an encoding error and crashes. See https://sentry.ols.canonical.com/canonical/sso/issues/669/.

Also, add a warning to try to prevent a mismatch with SSO regex matching.

[Colin Watson (cjwatson)]

review: Approve (code)

=== modified file 'lib/lp/registry/interfaces/ssh.py'
--- lib/lp/registry/interfaces/ssh.py	2018-02-24 09:11:39 +0000
+++ lib/lp/registry/interfaces/ssh.py	2018-06-20 14:48:20 +0000
@@ -25,6 +25,7 @@
     export_as_webservice_entry,
     exported,
     )
+import six
 from zope.interface import Interface
 from zope.schema import (
     Choice,
@@ -124,4 +125,27 @@
 
 @error_status(httplib.BAD_REQUEST)
 class SSHKeyAdditionError(Exception):
-    """Raised when the SSH public key is invalid."""
+    """Raised when the SSH public key is invalid.
+
+    WARNING: Be careful when changing the message format as
+    SSO uses a regex to recognize it.
+    """
+
+    def __init__(self, *args, **kwargs):
+        msg = ""
+        if 'key' in kwargs:
+            key = kwargs.pop('key')
+            msg = "Invalid SSH key data: '%s'" % key
+        if 'kind' in kwargs:
+            kind = kwargs.pop('kind')
+            msg = "Invalid SSH key type: '%s'" % kind
+        if 'exception' in kwargs:
+            exception = kwargs.pop('exception')
+            try:
+                exception_text = six.text_type(exception)
+            except UnicodeDecodeError:
+                # On Python 2, Key.fromString can raise exceptions with
+                # non-UTF-8 messages.
+                exception_text = bytes(exception).decode('unicode_escape')
+            msg = "%s (%s)" % (msg, exception_text)
+        super(SSHKeyAdditionError, self).__init__(msg, *args, **kwargs)

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2018-05-14 10:59:35 +0000
+++ lib/lp/registry/model/person.py	2018-06-20 14:48:20 +0000
@@ -4137,8 +4137,7 @@
             try:
                 Key.fromString(sshkey)
             except Exception as e:
-                raise SSHKeyAdditionError(
-                    "Invalid SSH key data: '%s' (%s)" % (sshkey, e))
+                raise SSHKeyAdditionError(key=sshkey, exception=e)
 
         if send_notification:
             person.security_field_changed(
@@ -4171,15 +4170,14 @@
         try:
             kind, keytext, comment = sshkey.split(' ', 2)
         except (ValueError, AttributeError):
-            raise SSHKeyAdditionError("Invalid SSH key data: '%s'" % sshkey)
+            raise SSHKeyAdditionError(key=sshkey)
 
         if not (kind and keytext and comment):
-            raise SSHKeyAdditionError("Invalid SSH key data: '%s'" % sshkey)
+            raise SSHKeyAdditionError(key=sshkey)
 
         keytype = SSH_TEXT_TO_KEY_TYPE.get(kind)
         if keytype is None:
-            raise SSHKeyAdditionError(
-                "Invalid SSH key type: '%s'" % kind)
+            raise SSHKeyAdditionError(kind=kind)
         return keytype, keytext, comment
 
 

=== modified file 'lib/lp/registry/tests/test_ssh.py'
--- lib/lp/registry/tests/test_ssh.py	2018-02-24 09:11:39 +0000
+++ lib/lp/registry/tests/test_ssh.py	2018-06-20 14:48:20 +0000
@@ -172,6 +172,13 @@
         )
         self.assertRaisesWithContent(
             SSHKeyAdditionError,
+            "Invalid SSH key data: 'ssh-rsa asdfasdf comment' "
+            u"(unknown blob type: \xc7_)",
+            keyset.new,
+            person, 'ssh-rsa asdfasdf comment'
+        )
+        self.assertRaisesWithContent(
+            SSHKeyAdditionError,
             "Invalid SSH key data: 'None'",
             keyset.new,
             person, None

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2018-06-04 14:02:57 +0000
+++ lib/lp/testing/__init__.py	2018-06-20 14:48:20 +0000
@@ -95,6 +95,7 @@
 import pytz
 import scandir
 import simplejson
+import six
 from storm.store import Store
 import subunit
 import testtools
@@ -651,7 +652,7 @@
         match what was raised an AssertionError is raised.
         """
         err = self.assertRaises(exception, func, *args, **kwargs)
-        self.assertEqual(exception_content, str(err))
+        self.assertEqual(exception_content, six.text_type(err))
 
     def assertBetween(self, lower_bound, variable, upper_bound):
         """Assert that 'variable' is strictly between two boundaries."""