launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/maas-ssh-keys into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/maas-ssh-keys/+merge/99697

This branch adds a validator to src/maasserver/models.py:SSHKey.key to make sure that only valid public keys can be stored.  It uses twisted.conch.ssh.keys to perform the actual key validation.
-- 
https://code.launchpad.net/~rvb/maas/maas-ssh-keys/+merge/99697
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-ssh-keys into lp:maas.

=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py	2012-03-28 06:21:37 +0000
+++ src/maasserver/models.py	2012-03-28 10:48:00 +0000
@@ -40,6 +40,7 @@
 from django.contrib import admin
 from django.contrib.auth.backends import ModelBackend
 from django.contrib.auth.models import User
+from django.core.exceptions import ValidationError
 from django.core.files.base import ContentFile
 from django.core.files.storage import FileSystemStorage
 from django.db import models
@@ -62,6 +63,10 @@
     POWER_TYPE,
     POWER_TYPE_CHOICES,
     )
+from twisted.conch.ssh.keys import (
+    BadKeyError,
+    Key,
+    )
 
 # Special users internal to MAAS.
 SYSTEM_USERS = [
@@ -683,6 +688,17 @@
         return SSHKey.objects.filter(user=user).values_list('key', flat=True)
 
 
+def validate_ssh_public_key(value):
+    """Validate that the given value contains a valid SSH public key."""
+    try:
+        key = Key.fromString(value)
+        if not key.isPublic():
+            raise ValidationError(
+                "Invalid SSH public key (this key contains a private key).")
+    except BadKeyError:
+        raise ValidationError("Invalid SSH public key.")
+
+
 class SSHKey(CommonInfo):
     """A `SSHKey` represents a user public SSH key.
 
@@ -698,7 +714,8 @@
 
     user = models.ForeignKey(User, null=False, editable=False)
 
-    key = models.TextField(null=False, editable=True)
+    key = models.TextField(
+        null=False, editable=True, validators=[validate_ssh_public_key])
 
     def __unicode__(self):
         return self.key

=== added directory 'src/maasserver/tests/data'
=== added file 'src/maasserver/tests/data/test_dsa'
--- src/maasserver/tests/data/test_dsa	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/data/test_dsa	2012-03-28 10:48:00 +0000
@@ -0,0 +1,12 @@
+-----BEGIN DSA PRIVATE KEY-----
+MIIBvAIBAAKBgQC5fDwjGkmt6QghiWia+JB9ED5a4Mhty5Gvv8uMcdXTriYabC3t
+lffgil+0s9cqYQrNuiqjBXv3m2BgFA4U3VZROP04KQD+Bok43TtRRl02HYEMiaX6
+edG01FtMyiD/3cB7OXQi5+F2AMV80rLzkRlUBmzOj+r4d5Ynk5HlRExmQwIVAIBo
+EpSY+9GojftVGPU8T0HaueR9AoGBAKVmDBdCFPgWFLRbsEUVBPcBBsQxI07bqFmA
+ljC+2Z/nBTryvgtMfGCLUN8SIiN2v9AwqAzyGT7yJlwzK7NM1Lp3oJ3UvYOydIAh
+xWPXXfq4GEoVB8AiPocvw/Q6dbpDZxg9G298ebxKEiusIayVgTOorO01uEeX78/8
+Q4a7SHMYAoGBAJbZsmuuWN2kb7lD27IzKcOgd07esoHPWZnv4qg7xhS1GdVr485v
+73OW1rfpWU6PdohckXLg9ZaoWtVTwNKTfHxS3iug9/pseBWTHdpmxCM5ClsZJii6
+T4frR5NTOCGKLxOamTs///OXopZr5u3vT20NFlzFE95J86tGtxYPPivxAhQByRHQ
+RXk6Jpjwa5kX+bYX1J3FIg==
+-----END DSA PRIVATE KEY-----

=== added file 'src/maasserver/tests/data/test_dsa.pub'
--- src/maasserver/tests/data/test_dsa.pub	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/data/test_dsa.pub	2012-03-28 10:48:00 +0000
@@ -0,0 +1,1 @@
+ssh-dss AAAAB3NzaC1kc3MAAACBALl8PCMaSa3pCCGJaJr4kH0QPlrgyG3Lka+/y4xx1dOuJhpsLe2V9+CKX7Sz1yphCs26KqMFe/ebYGAUDhTdVlE4/TgpAP4GiTjdO1FGXTYdgQyJpfp50bTUW0zKIP/dwHs5dCLn4XYAxXzSsvORGVQGbM6P6vh3lieTkeVETGZDAAAAFQCAaBKUmPvRqI37VRj1PE9B2rnkfQAAAIEApWYMF0IU+BYUtFuwRRUE9wEGxDEjTtuoWYCWML7Zn+cFOvK+C0x8YItQ3xIiI3a/0DCoDPIZPvImXDMrs0zUunegndS9g7J0gCHFY9dd+rgYShUHwCI+hy/D9Dp1ukNnGD0bb3x5vEoSK6whrJWBM6is7TW4R5fvz/xDhrtIcxgAAACBAJbZsmuuWN2kb7lD27IzKcOgd07esoHPWZnv4qg7xhS1GdVr485v73OW1rfpWU6PdohckXLg9ZaoWtVTwNKTfHxS3iug9/pseBWTHdpmxCM5ClsZJii6T4frR5NTOCGKLxOamTs///OXopZr5u3vT20NFlzFE95J86tGtxYPPivx ubuntu@server-7476

=== added file 'src/maasserver/tests/data/test_rsa'
--- src/maasserver/tests/data/test_rsa	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/data/test_rsa	2012-03-28 10:48:00 +0000
@@ -0,0 +1,27 @@
+-----BEGIN RSA PRIVATE KEY-----
+MIIEpAIBAAKCAQEA3a88w2TcMjFQbwU0+pAZ63z2b/wFG5MY+xuQmjE3cOsZCrYV
+dthJ7Q95cWkMUSs0E3wnNnfwgtSuLrIQnEbNUukkNsydow6tVJjlJAzILzYeCVYS
+WEinOprvIt7O6BkpgWnaOcL7ZaUszK7HDqX/EHv8Jk+Xx9digfYqSaqBTUDFW1XF
+WqxP7+sKZ9xmQutmEW+Yms/fzPrfdkIbudFtrvin6LqrSgdpcCAwymTMMN/BMbXT
+KLodhWacWqIeFHsuW0ad7TNyYJ1nF+n8lDqdoMSUTyJQ/a62YV+SJBJGTPDEG3T0
+0C4CH/fKBaYeURusNbdNgbyPZySjOypChHwR7QIDAQABAoIBAE5/DH8LqcTEHX0S
+VO4cNHFkMEb68DwRXBkea5eNsdn0BUv7qaIJeDPO9OupjMj5CVmU7rWkxq8s6/hw
+6NzNXUrsbvxQe8kPG2UHNqwLMp81BHG93oUQRNbFocOxLYaV0lKWzsUBO8+EK1bW
+1HllYenOXTybll0W8TSfm9212E8n5YFYoJDFH9nEDiUqDcYLOKJc5njceZUr0KQ2
+eKTpaNEtS78crNAwvdu4UzfC/I4ezfoWreXao0epr3anfTXXYf2TrdthgG7FclNP
+aOrCsQFXMrqVIc/FVXmbPKfhZOnOGglKVEUr0OmG3mfKan60636bGoiJJ2D6snKw
+YCAk8mkCgYEA8TcYZAG7gsi/1yTDrIfcrm5zGGBQ371vNOsTOVXGD28mepwUUl3B
+SucD7dAXlAyN/ikHly/wX/tVw3tF7N8nJx1wJ+KwGYgTVCgRBXexjiwz6NdyMbQX
+F7kb6LQjL6P40ygYTjVuFgee2KXd11NuT5pW1wleF3cY3tKWx0TWgEcCgYEA60Wv
+qz5yMg9+9A85Qis3VAw2DgJ3j12RYfSLhL0zkgZ6M1caZqMBuP7Q3HLlrEVJ+PJY
+aIiidIz9VLARa9wetAfJUVDsi/aijVXknLdm18Rujymsf97Wa4xj+006VmjdEeXV
+RoZGJ3l+j7yo2yfHxO1hHLLiXRXRQwOmUIGgSisCgYEAzYoI+o6PXS36ajUll0pd
+vTTYVhkcUMp2jD0TMHPqRRSNUUTV/Clvn4eiTW5X6QuZos0LbsSmquLbfar5NpIg
+JrBq9VGwhNDyx28sseAAKAl6YhnTcI7oboqJQYzdvqaWTDeKHnpgx9zOegU8N1Mc
+WDBHdwzAZHZTduszF7GMpdkCgYAQ7SuNS2nV1i2RC4NYElnhrxs4eM73PokWHgzn
+mOEb8WFbTjn1Bmc6UwLdyVpiwX1n7q+TnbjqX7ZeIGiwdN60nxbJxeOu0iixuGtB
+JyS8A0LdA+eIL5UHmcsbqlu3GcZF4l4su75SWrhTSQRw9/S0Y0uoT+pfPhGXG60c
+f6bzjwKBgQDe+l7pa+qkE0a8B11CgqKzmHOa/PRool7+0//WA5/H6jx0iAH5Ms2f
+8bpVbWugyXOFXkFN+DOfzJgar5EQmAtpipx6OKx1DXQqabmUlm75HOkr1dxOshou
+w39I3JOc1yHjlPJHjVyJqJcVOHYAFbEu5w+Sk0YELajvgU7FkfkonA==
+-----END RSA PRIVATE KEY-----

=== added file 'src/maasserver/tests/data/test_rsa.pub'
--- src/maasserver/tests/data/test_rsa.pub	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/data/test_rsa.pub	2012-03-28 10:48:00 +0000
@@ -0,0 +1,1 @@
+ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDdrzzDZNwyMVBvBTT6kBnrfPZv/AUbkxj7G5CaMTdw6xkKthV22EntD3lxaQxRKzQTfCc2d/CC1K4ushCcRs1S6SQ2zJ2jDq1UmOUkDMgvNh4JVhJYSKc6mu8i3s7oGSmBado5wvtlpSzMrscOpf8Qe/wmT5fH12KB9ipJqoFNQMVbVcVarE/v6wpn3GZC62YRb5iaz9/M+t92Qhu50W2u+KfouqtKB2lwIDDKZMww38ExtdMouh2FZpxaoh4Uey5bRp3tM3JgnWcX6fyUOp2gxJRPIlD9rrZhX5IkEkZM8MQbdPTQLgIf98oFph5RG6w1t02BvI9nJKM7KkKEfBHt ubuntu@server-7476

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	2012-03-27 10:40:40 +0000
+++ src/maasserver/tests/test_models.py	2012-03-28 10:48:00 +0000
@@ -40,6 +40,7 @@
     SSHKey,
     SYSTEM_USERS,
     UserProfile,
+    validate_ssh_public_key,
     )
 from maasserver.provisioning import get_provisioning_api_proxy
 from maasserver.testing.enum import map_enum
@@ -520,6 +521,44 @@
         self.assertTrue(set(SYSTEM_USERS).isdisjoint(usernames))
 
 
+
+class SSHKeyValidatorTest(TestCase):
+
+    def get_data(self, filename):
+        """Utility method to read the content of files in
+        src/maasserver/tests.
+
+        Usually used to read files in src/maasserver/tests/data."""
+        path = os.path.join(
+            os.path.dirname(os.path.abspath(__file__)), filename)
+        return file(path).read()
+
+    def test_validates_rsa_public_key(self):
+        key_string = self.get_data('data/test_rsa.pub')
+        validate_ssh_public_key(key_string)
+        # No ValidationError.
+
+    def test_validates_dsa_public_key(self):
+        key_string = self.get_data('data/test_dsa.pub')
+        validate_ssh_public_key(key_string)
+        # No ValidationError.
+
+    def test_does_not_validate_random_data(self):
+        key_string = factory.getRandomString()
+        self.assertRaises(
+            ValidationError, validate_ssh_public_key, key_string)
+
+    def test_does_not_validate_rsa_private_key(self):
+        key_string = self.get_data('data/test_rsa')
+        self.assertRaises(
+            ValidationError, validate_ssh_public_key, key_string)
+
+    def test_does_not_validate_dsa_private_key(self):
+        key_string = self.get_data('data/test_dsa')
+        self.assertRaises(
+            ValidationError, validate_ssh_public_key, key_string)
+
+
 class SSHKeyManagerTest(TestCase):
     """Testing for the :class `SSHKeyManager` model manager."""