Merge lp:~rvb/maas/wrong-key-bug-1298788 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Catch all the exceptions raised by twisted.conch.ssh.keys.Key.fromString when validating a SSH key.

I know that catching all exceptions is generally an anti-pattern but in this instance, I think it's preferable to playing hide-and-seek with twisted.conch.ssh.keys.Key.fromString() which clearly can throw many types of exceptions. I just noticed that calling it on 'ssh-r' triggers an IndexError (http://paste.ubuntu.com/7168866/).

[Jeroen T. Vermeulen (jtv)]

It's a nasty problem. For the record, I don't think it's wrong to catch Exception per se, just to suppress potentially important error information.

In that spirit, perhaps you could log the Exception that you catch, for debugging purposes?

review: Approve

[Julian Edwards (julian-edwards)]

On 29/03/14 01:41, Jeroen T. Vermeulen wrote:
> Review: Approve
>
> It's a nasty problem. For the record, I don't think it's wrong to catch Exception per se, just to suppress potentially important error information.
>
> In that spirit, perhaps you could log the Exception that you catch, for debugging purposes?
>

In addition, always comment in the code why you are using bare
Exceptions. Thanks.

[Raphaël Badin (rvb)]

> It's a nasty problem. For the record, I don't think it's wrong to catch
> Exception per se, just to suppress potentially important error information.
>
> In that spirit, perhaps you could log the Exception that you catch, for
> debugging purposes?

Good point, done.

[Raphaël Badin (rvb)]

> In addition, always comment in the code why you are using bare
> Exceptions. Thanks.

done.

=== modified file 'src/maasserver/models/sshkey.py'
--- src/maasserver/models/sshkey.py	2014-01-17 08:45:11 +0000
+++ src/maasserver/models/sshkey.py	2014-03-31 07:34:52 +0000
@@ -17,7 +17,6 @@
     ]
 
 
-import binascii
 from cgi import escape
 
 from django.contrib.auth.models import User
@@ -28,13 +27,13 @@
     TextField,
     )
 from django.utils.safestring import mark_safe
-from maasserver import DefaultMeta
+from maasserver import (
+    DefaultMeta,
+    logger,
+    )
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.timestampedmodel import TimestampedModel
-from twisted.conch.ssh.keys import (
-    BadKeyError,
-    Key,
-    )
+from twisted.conch.ssh.keys import Key
 
 
 class SSHKeyManager(Manager):
@@ -49,11 +48,17 @@
     """Validate that the given value contains a valid SSH public key."""
     try:
         key = Key.fromString(value)
+    except Exception:
+        # twisted.conch.ssh.keys.Key.fromString raises all sorts of exceptions.
+        # Here, we catch them all and return a ValidationError since this
+        # method only aims at validating keys and not return the exact cause of
+        # the failure.
+        logger.exception("Invalid SSH public key")
+        raise ValidationError("Invalid SSH public key.")
+    else:
         if not key.isPublic():
             raise ValidationError(
                 "Invalid SSH public key (this key is a private key).")
-    except (BadKeyError, binascii.Error, UnicodeEncodeError):
-        raise ValidationError("Invalid SSH public key.")
 
 
 HELLIPSIS = '…'

=== modified file 'src/maasserver/models/tests/test_sshkey.py'
--- src/maasserver/models/tests/test_sshkey.py	2014-01-17 08:45:01 +0000
+++ src/maasserver/models/tests/test_sshkey.py	2014-03-31 07:34:52 +0000
@@ -23,11 +23,13 @@
 from maasserver.models.sshkey import (
     get_html_display_for_key,
     HELLIPSIS,
+    Key,
     validate_ssh_public_key,
     )
 from maasserver.testing import get_data
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from mock import Mock
 from testtools.matchers import EndsWith
 
 
@@ -63,6 +65,13 @@
         self.assertRaises(
             ValidationError, validate_ssh_public_key, key_string)
 
+    def test_does_not_validate_wrong_key(self):
+        # If twisted.conch.ssh.keys.Key raises an exception, the validation
+        # fails.
+        self.patch(Key, 'fromString', Mock(side_effect=Exception()))
+        self.assertRaises(
+            ValidationError, validate_ssh_public_key, factory.make_name('key'))
+
     def test_does_not_validate_rsa_private_key(self):
         key_string = get_data('data/test_rsa')
         self.assertRaises(