Merge lp:~allenap/maas/other-ssh-key-types--bug-1590081 into lp:maas/trunk

Proposed by Gavin Panella on 2016-06-10
Status: Merged
Approved by: Gavin Panella on 2016-06-14
Approved revision: 5105
Merged at revision: 5118
Proposed branch: lp:~allenap/maas/other-ssh-key-types--bug-1590081
Merge into: lp:maas/trunk
Diff against target: 463 lines (+354/-24)
8 files modified
src/maasserver/models/sshkey.py (+5/-17)
src/maasserver/models/tests/test_sshkey.py (+28/-7)
src/maasserver/tests/data/test_ecdsa256.pub (+1/-0)
src/maasserver/tests/data/test_ecdsa384.pub (+1/-0)
src/maasserver/tests/data/test_ecdsa521.pub (+1/-0)
src/maasserver/tests/data/test_ed25519.pub (+1/-0)
src/provisioningserver/utils/sshkey.py (+161/-0)
src/provisioningserver/utils/tests/test_sshkey.py (+156/-0)
To merge this branch: bzr merge lp:~allenap/maas/other-ssh-key-types--bug-1590081
Reviewer Review Type Date Requested Status
Mike Pontillo (community) 2016-06-10 Approve on 2016-06-14
Review via email: mp+297093@code.launchpad.net

Commit message

Permit use of ECDSA and ED25519 SSH keys.

To post a comment you must log in.
Mike Pontillo (mpontillo) wrote :

I would rather see this fixed upstream than hack around it in MAAS. I checked the twisted code[1] and saw that it was decoding the base64 and parsing the expected ASN1, which I agree is probably too much work for us to take on at this time.

But I'm not convinced that calling Twisted to validate the key for us was the right thing to do in the first place. After all, SSH would be better at telling us whether or not we have a valid key. We should do this instead:

    (1) Write key to <tempfile
    (2) Check the result of: ssh-keygen -l -f <tempfile>

[1]:
https://github.com/twisted/twisted/blob/trunk/twisted/conch/ssh/keys.py

review: Needs Fixing
Gavin Panella (allenap) wrote :

>     (1) Write key to <tempfile
>     (2) Check the result of: ssh-keygen -l -f <tempfile>

I like this idea, but I'm not sure that's going to reproduce the right
behaviour. The conch code is useful because it does check that the key
is sane *and* it's a public key. The last bit prevents users from saving
private keys, which would be a bit of a security disaster, and wouldn't
work for MAAS's purposes anyway. Even the hacky approach in this branch
can distinguish between public and private keys.

If we can figure out a way to get an answer to "is this a valid looking
public key" out of ssh-keygen or another OpenSSH program (library?) then
I'm in full agreement that we should use it.

Perhaps the way to do it is to first check, in Python, that the key
starts with {ssh-{rsa,dsa,ed25519},ecdsa-sha2-nistp{256,384,521}} and
then run it through ssh-keygen -l as well.

Mike Pontillo (mpontillo) wrote :

Good catch. A couple of options I can think of:

(1) Manually validate that the user didn't enter a private key (because they should know they just did something horribly wrong) - could check for PEM headers, like if the key starts with "-", contains the text "PRIVATE", etc.

(2) Use ssh-keygen to export and then re-import the key, thus normalizing it. (if they pasted in their private key, it would be converted to a public key) For example:

keygen -e -f id_rsa.pub > keyfile && ssh-keygen -i -f keyfle

Only problem is, this seems to lose the comment at the end.

Mike Pontillo (mpontillo) wrote :

I meant ssh-keygen for the example command under (2), by the way. (and I ran it from ~/.ssh)

Mike Pontillo (mpontillo) wrote :

(and also, it outputs the same thing for both my id_rsa and id_rsa.pub, in case I wasn't clear.)

Mike Pontillo (mpontillo) wrote :

Oh, and I guess you could run the key through "ssh-keygen -l -f <keyfile>" first to grab the comment. You'd have to parse the output; it seems to append the key type in parentheses after the comment, and spaces are allowed in the comment. So you'd just drop the first, second, and last fields, and keep everything in the middle.

Gavin Panella (allenap) wrote :

I've read and read today and I think we should do what we can to ensure
we have a public key first (and not try to extract a public key from a
private key) and then pump it through ssh-keygen to ensure it's valid.

sshd(8) has a section describing the format of ~/.ssh/authorized_keys:

  Each line of the file contains one key (empty lines and lines starting
  with a ‘#’ are ignored as comments). Protocol 1 public keys consist of
  the following space-separated fields: options, bits, exponent,
  modulus, comment. Protocol 2 public key consist of: options, keytype,
  base64-encoded key, comment. The options field is optional; [...]. The
  bits, exponent, modulus, and comment fields give the RSA key for
  protocol version 1; the comment field is not used for anything (but
  may be convenient for the user to identify the key). For protocol
  version 2 the keytype is “ecdsa-sha2-nistp256”, “ecdsa-sha2-nistp384”,
  “ecdsa-sha2-nistp521”, “ssh-ed25519”, “ssh-dss” or “ssh-rsa”.

ssh-keygen(1) explicitly recommends appending public key files to
~/.ssh/authorized_keys:

  The contents ... should be added to ~/.ssh/authorized_keys on all
  machines where the user wishes to log in using public key
  authentication.

Marrying the two we have official documentation for the format of public
key files!

We should ignore protocol 1 keys. I can't even create an rsa1 key on
Xenial:

  $ ssh-keygen -t rsa1
  Generating public/private rsa1 key pair.
  Enter file in which to save the key (.../.ssh/identity):
  Enter passphrase (empty for no passphrase):
  Enter same passphrase again:
  Saving key ".../.ssh/identity" failed: unknown or unsupported key type

Although ~/.ssh/authorized_keys can contain options, we should assume
that the public keys pasted into MAAS do not have options.

Given all that I propose the following:

1. Check there are 2 or 3 fields: keytype base64-encoded-key [comment]

2. Run through `setsid -w ssh-keygen -e -f $keyfile > $intermediate <&-`.

3. Run through `setsid -w ssh-keygen -i -f $intermediate > $pubkey <&-`.

Note: setsid and <&- ensures ssh-keygen doesn't using the caller's TTY.
This'll be implemented in Python with no recourse to a shell, so the
commands won't look like those above, but behaviour will be preserved.

4. $pubkey should contain two fields: keytype, base64-encoded key.

5. Reunite $pubkey with comment, if there was one.

Errors from ssh-keygen at any point should be reported *with the error
message*. Currently all errors relating to SSH keys are coalesced into
the same static message.

Gavin Panella (allenap) wrote :

> Oh, and I guess you could run the key through "ssh-keygen -l -f
> <keyfile>" first to grab the comment. You'd have to parse the output;
> it seems to append the key type in parentheses after the comment, and
> spaces are allowed in the comment. So you'd just drop the first,
> second, and last fields, and keep everything in the middle.

There's no official documentation for the output of `ssh-keygen -l` so
I'd rather parse the given key because we do have some kind of
documentation for what that ought to look like.

Gavin Panella (allenap) wrote :

I forgot something:

> Given all that I propose the following:
>
> 1. Check there are 2 or 3 fields: keytype base64-encoded-key [comment]

1a. Check that keytype is one of “ecdsa-sha2-nistp256”,
“ecdsa-sha2-nistp384”, “ecdsa-sha2-nistp521”, “ssh-ed25519”, “ssh-dss”
or “ssh-rsa”.

Mike Pontillo (mpontillo) wrote :

I think all that sounds good. But again, I would rather not hard-code the expected 'keytype' values in our Python code. Don't you think that if the export/import cycle succeeds, that's a strong indication that `keytype` is valid?

5100. By Gavin Panella on 2016-06-13

New function normalise_openssh_public_key and happy-path testing for it.

5101. By Gavin Panella on 2016-06-13

Test error paths.

5102. By Gavin Panella on 2016-06-13

Use normalise_openssh_public_key in the Django validator.

Gavin Panella (allenap) wrote :

> I think all that sounds good. But again, I would rather not hard-code
> the expected 'keytype' values in our Python code. Don't you think that
> if the export/import cycle succeeds, that's a strong indication that
> `keytype` is valid?

I would agree with that but for one problem: ssh-keygen gives terrible
error messages. For example, given a valid key but with a bogus key
type, ssh-keygen prints:

  Load key "/path/to/key": incorrect passphrase supplied to decrypt
  private key

This message is also given for many other issues. Initially I did want
to expose ssh-keygen's error messages in the exception raised, and
ultimately in the UI, but so far they seem to be universally awful.

My preference would be to stick with the set of key types so that we can
produce better error messages. Key types are a slow moving target and we
can respond quickly with a small update to OPENSSH_PROTOCOL2_KEY_TYPES
and some test data.

Including recognised key types in the error message also means the user
can help him/herself by choosing a different key. They can see that the
deficiency is in MAAS and not their key, so they're more likely to file
a bug report rather than bad-mouth MAAS for not working.

5103. By Gavin Panella on 2016-06-14

Fix typo.

5104. By Gavin Panella on 2016-06-14

Be cautious with %-formatting errors.

Gavin Panella (allenap) wrote :

BTW, I've implemented it with ssh-keygen now and it's ready to be reviewed again.

Mike Pontillo (mpontillo) wrote :

Gavin, I have tested out the new code and it's working great.

Just one regression to fix before it can land: spaces are supported in SSH key comments.

Mike Pontillo (mpontillo) wrote :

Another nice-to-have validation would be to reject duplicate keys based on the (type, value) ignoring the comment. (right now it seems we do an exact match, which means I can add the same key twice with a different comment.) But I guess that's out of scope for this branch.

5105. By Gavin Panella on 2016-06-14

Deal with comments containing whitespace.

Gavin Panella (allenap) wrote :

Thanks Mike. I've fixed the comments-with-whitespace issue, but I agree that de-duplicating on keytype+key is out of scope here.

Mike Pontillo (mpontillo) wrote :

Looks great. Thanks for all the fixes.

One thing I won't block you on: it looks like you're missing a test for comments-with-spaces? Unless I just missed it.

review: Approve
Gavin Panella (allenap) wrote :

Yep, as discussed, I sneaked one in as test_normalises_mixed_whitespace_in_comments. Thanks again!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/models/sshkey.py'
2--- src/maasserver/models/sshkey.py 2016-04-11 16:23:26 +0000
3+++ src/maasserver/models/sshkey.py 2016-06-14 15:28:23 +0000
4@@ -18,13 +18,10 @@
5 TextField,
6 )
7 from django.utils.safestring import mark_safe
8-from maasserver import (
9- DefaultMeta,
10- logger,
11-)
12+from maasserver import DefaultMeta
13 from maasserver.models.cleansave import CleanSave
14 from maasserver.models.timestampedmodel import TimestampedModel
15-from twisted.conch.ssh.keys import Key
16+from provisioningserver.utils.sshkey import normalise_openssh_public_key
17
18
19 class SSHKeyManager(Manager):
20@@ -38,18 +35,9 @@
21 def validate_ssh_public_key(value):
22 """Validate that the given value contains a valid SSH public key."""
23 try:
24- key = Key.fromString(value.encode("utf-8"))
25- except Exception:
26- # twisted.conch.ssh.keys.Key.fromString raises all sorts of exceptions.
27- # Here, we catch them all and return a ValidationError since this
28- # method only aims at validating keys and not return the exact cause of
29- # the failure.
30- logger.exception("Invalid SSH public key")
31- raise ValidationError("Invalid SSH public key.")
32- else:
33- if not key.isPublic():
34- raise ValidationError(
35- "Invalid SSH public key (this key is a private key).")
36+ return normalise_openssh_public_key(value)
37+ except Exception as error:
38+ raise ValidationError("Invalid SSH public key: " + str(error))
39
40
41 HELLIPSIS = '&hellip;'
42
43=== modified file 'src/maasserver/models/tests/test_sshkey.py'
44--- src/maasserver/models/tests/test_sshkey.py 2016-05-12 19:07:37 +0000
45+++ src/maasserver/models/tests/test_sshkey.py 2016-06-14 15:28:23 +0000
46@@ -1,4 +1,4 @@
47-# Copyright 2012-2015 Canonical Ltd. This software is licensed under the
48+# Copyright 2012-2016 Canonical Ltd. This software is licensed under the
49 # GNU Affero General Public License version 3 (see the file LICENSE).
50
51 """Tests for the SSHKey model."""
52@@ -6,16 +6,17 @@
53 __all__ = []
54
55 import random
56-from unittest.mock import Mock
57
58 from django.core.exceptions import ValidationError
59 from django.db import IntegrityError
60 from django.utils.safestring import SafeString
61-from maasserver.models import SSHKey
62+from maasserver.models import (
63+ sshkey,
64+ SSHKey,
65+)
66 from maasserver.models.sshkey import (
67 get_html_display_for_key,
68 HELLIPSIS,
69- Key,
70 validate_ssh_public_key,
71 )
72 from maasserver.testing import get_data
73@@ -36,6 +37,26 @@
74 validate_ssh_public_key(key_string)
75 # No ValidationError.
76
77+ def test_validates_ecdsa_curve256_public_key(self):
78+ key_string = get_data('data/test_ecdsa256.pub')
79+ validate_ssh_public_key(key_string)
80+ # No ValidationError.
81+
82+ def test_validates_ecdsa_curve384_public_key(self):
83+ key_string = get_data('data/test_ecdsa384.pub')
84+ validate_ssh_public_key(key_string)
85+ # No ValidationError.
86+
87+ def test_validates_ecdsa_curve521_public_key(self):
88+ key_string = get_data('data/test_ecdsa521.pub')
89+ validate_ssh_public_key(key_string)
90+ # No ValidationError.
91+
92+ def test_validates_ed25519_public_key(self):
93+ key_string = get_data('data/test_ed25519.pub')
94+ validate_ssh_public_key(key_string)
95+ # No ValidationError.
96+
97 def test_does_not_validate_random_data(self):
98 key_string = factory.make_string()
99 self.assertRaises(
100@@ -57,9 +78,9 @@
101 ValidationError, validate_ssh_public_key, key_string)
102
103 def test_does_not_validate_wrong_key(self):
104- # If twisted.conch.ssh.keys.Key raises an exception, the validation
105- # fails.
106- self.patch(Key, 'fromString', Mock(side_effect=Exception()))
107+ # Validation fails if normalise_openssh_public_key crashes.
108+ norm = self.patch(sshkey, "normalise_openssh_public_key")
109+ norm.side_effect = factory.make_exception_type()
110 self.assertRaises(
111 ValidationError, validate_ssh_public_key, factory.make_name('key'))
112
113
114=== added file 'src/maasserver/tests/data/test_ecdsa256.pub'
115--- src/maasserver/tests/data/test_ecdsa256.pub 1970-01-01 00:00:00 +0000
116+++ src/maasserver/tests/data/test_ecdsa256.pub 2016-06-14 15:28:23 +0000
117@@ -0,0 +1,1 @@
118+ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBEqp6hJ9qj6dD1Y1AfsbauzjAaoIQhvTCdg+otLRklg5ZWr8KoS98K50s0eVwcOD7iLltCeS7W0y8c7wlsADVh0= foo@bar
119
120=== added file 'src/maasserver/tests/data/test_ecdsa384.pub'
121--- src/maasserver/tests/data/test_ecdsa384.pub 1970-01-01 00:00:00 +0000
122+++ src/maasserver/tests/data/test_ecdsa384.pub 2016-06-14 15:28:23 +0000
123@@ -0,0 +1,1 @@
124+ecdsa-sha2-nistp384 AAAAE2VjZHNhLXNoYTItbmlzdHAzODQAAAAIbmlzdHAzODQAAABhBFnB+h79/2MeUR4FoDuKJDyjLEswi8I50NuwIoRbHOwPkPDSDXk6EKfBY0GEwAGyr7h9OjVlmA1KKWUE01KJKf4/iJOh+9zsaL4iQzP9Q9phiUAmxkvegefGwqEXeAvk1Q== foo@bar
125
126=== added file 'src/maasserver/tests/data/test_ecdsa521.pub'
127--- src/maasserver/tests/data/test_ecdsa521.pub 1970-01-01 00:00:00 +0000
128+++ src/maasserver/tests/data/test_ecdsa521.pub 2016-06-14 15:28:23 +0000
129@@ -0,0 +1,1 @@
130+ecdsa-sha2-nistp521 AAAAE2VjZHNhLXNoYTItbmlzdHA1MjEAAAAIbmlzdHA1MjEAAACFBAFid8WJ6720Z8xJ/Fnsz9eZmUxdbcVNzBeML380gMeBMP9zPXWz629cahQT0HncnKsLsbRB7MMxdaBdsAQ8pteGXQEHVdnr6IkOrVbCHtVaVbjN4gpRICseMnDHrryrOjsvBIU7GGpmmHZka9alvSZlbB1lCx1BxqZZj8AHjJq2KpUh+A== foo@bar
131
132=== added file 'src/maasserver/tests/data/test_ed25519.pub'
133--- src/maasserver/tests/data/test_ed25519.pub 1970-01-01 00:00:00 +0000
134+++ src/maasserver/tests/data/test_ed25519.pub 2016-06-14 15:28:23 +0000
135@@ -0,0 +1,1 @@
136+ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIBEqkw2AgmkjqNjCFuiKXeUgLNmRbgVr8W2TlAvFybJv foo@bar
137
138=== added file 'src/provisioningserver/utils/sshkey.py'
139--- src/provisioningserver/utils/sshkey.py 1970-01-01 00:00:00 +0000
140+++ src/provisioningserver/utils/sshkey.py 2016-06-14 15:28:23 +0000
141@@ -0,0 +1,161 @@
142+# Copyright 2016 Canonical Ltd. This software is licensed under the
143+# GNU Affero General Public License version 3 (see the file LICENSE).
144+
145+"""Utilities for working with OpenSSH keys."""
146+
147+__all__ = [
148+ "normalise_openssh_public_key",
149+ "OpenSSHKeyError",
150+]
151+
152+from itertools import chain
153+import os
154+from pathlib import Path
155+import pipes
156+from subprocess import (
157+ CalledProcessError,
158+ check_output,
159+ PIPE,
160+)
161+from tempfile import TemporaryDirectory
162+
163+from provisioningserver.utils.shell import select_c_utf8_locale
164+
165+
166+OPENSSH_PROTOCOL2_KEY_TYPES = frozenset((
167+ "ecdsa-sha2-nistp256",
168+ "ecdsa-sha2-nistp384",
169+ "ecdsa-sha2-nistp521",
170+ "ssh-dss",
171+ "ssh-ed25519",
172+ "ssh-rsa",
173+))
174+
175+
176+class OpenSSHKeyError(ValueError):
177+ """The given key was not recognised or was corrupt."""
178+
179+
180+def normalise_openssh_public_key(keytext):
181+ """Validate and normalise an OpenSSH public key.
182+
183+ Essentially: ensure we have a public key first (and not try to extract a
184+ public key from a private key) and then pump it through an ssh-keygen(1)
185+ pipeline to ensure it's valid.
186+
187+ sshd(8) has a section describing the format of ~/.ssh/authorized_keys:
188+
189+ Each line of the file contains one key (empty lines and lines starting
190+ with a ‘#’ are ignored as comments). Protocol 1 public keys consist of
191+ the following space-separated fields: options, bits, exponent, modulus,
192+ comment. Protocol 2 public key consist of: options, keytype,
193+ base64-encoded key, comment. The options field is optional; [...]. The
194+ bits, exponent, modulus, and comment fields give the RSA key for
195+ protocol version 1; the comment field is not used for anything (but may
196+ be convenient for the user to identify the key). For protocol version 2
197+ the keytype is “ecdsa-sha2-nistp256”, “ecdsa-sha2-nistp384”,
198+ “ecdsa-sha2-nistp521”, “ssh-ed25519”, “ssh-dss” or “ssh-rsa”.
199+
200+ ssh-keygen(1) explicitly recommends appending public key files to
201+ ~/.ssh/authorized_keys:
202+
203+ The contents ... should be added to ~/.ssh/authorized_keys on all
204+ machines where the user wishes to log in using public key
205+ authentication.
206+
207+ Marrying the two we have official documentation for the format of public
208+ key files!
209+
210+ We should ignore protocol 1 keys. It does not even appear to be possible
211+ to create an rsa1 key on Xenial:
212+
213+ $ ssh-keygen -t rsa1
214+ Generating public/private rsa1 key pair.
215+ Enter file in which to save the key (.../.ssh/identity):
216+ Enter passphrase (empty for no passphrase):
217+ Enter same passphrase again:
218+ Saving key ".../.ssh/identity" failed: unknown or unsupported key type
219+
220+ Although ~/.ssh/authorized_keys can contain options, we should assume that
221+ the public keys pasted into MAAS do not have options. Public key files
222+ generated by ssh-keygen(1) will not contain options.
223+
224+ Given all that, this function does the following:
225+
226+ 1. Checks there are 2 or more fields: keytype base64-encoded-key [comment]
227+ (the comment can contain whitespace).
228+
229+ 2. Checks that keytype is one of “ssh-dss”, “ssh-rsa”, “ssh-ed25519”,
230+ “ecdsa-sha2-nistp256”, “ecdsa-sha2-nistp384”, or “ecdsa-sha2-nistp521”,
231+
232+ 2. Run through `setsid -w ssh-keygen -e -f $keyfile > $intermediate <&-`.
233+
234+ 3. Run through `setsid -w ssh-keygen -i -f $intermediate > $pubkey <&-`.
235+
236+ Note: setsid and <&- ensures ssh-keygen doesn't use the caller's TTY. This
237+ is Python, and no recourse to a shell is being taken, but it has similar
238+ behaviour.
239+
240+ 4. $pubkey should contain two fields: keytype, base64-encoded key.
241+
242+ 5. Reunite $pubkey with comment, if there was one.
243+
244+ Errors from ssh-keygen(1) at any point should be reported *with the error
245+ message*. Previously all errors relating to SSH keys were coalesced into
246+ the same static message.
247+
248+ """
249+ parts = keytext.split()
250+ if len(parts) >= 2:
251+ keytype, key, *comments = parts
252+ else:
253+ raise OpenSSHKeyError(
254+ "Key should contain 2 or more space separated parts (key type, "
255+ "base64-encoded key, optional comments), not %d: %s" % (
256+ len(parts), " ".join(map(pipes.quote, parts))))
257+
258+ if keytype not in OPENSSH_PROTOCOL2_KEY_TYPES:
259+ raise OpenSSHKeyError(
260+ "Key type %s not recognised; it should be one of: %s" % (
261+ pipes.quote(keytype), " ".join(
262+ sorted(OPENSSH_PROTOCOL2_KEY_TYPES))))
263+
264+ env = select_c_utf8_locale()
265+ # Request OpenSSH to use /bin/true when prompting for passwords. We also
266+ # have to redirect stdin from, say, /dev/null so that it doesn't use the
267+ # terminal (when this is executed from a terminal).
268+ env["SSH_ASKPASS"] = "/bin/true"
269+
270+ with TemporaryDirectory(prefix="maas") as tempdir:
271+ keypath = Path(tempdir).joinpath("intermediate")
272+ # Ensure that this file is locked-down.
273+ keypath.touch()
274+ keypath.chmod(0o600)
275+ # Convert given key to RFC4716 form.
276+ keypath.write_text("%s %s" % (keytype, key), "utf-8")
277+ try:
278+ with open(os.devnull, "r") as devnull:
279+ rfc4716key = check_output(
280+ ("setsid", "-w", "ssh-keygen", "-e", "-f", keypath.path),
281+ stdin=devnull, stderr=PIPE, env=env)
282+ except CalledProcessError:
283+ raise OpenSSHKeyError(
284+ "Key could not be converted to RFC4716 form.")
285+ # Convert RFC4716 back to OpenSSH format public key.
286+ keypath.write_bytes(rfc4716key)
287+ try:
288+ with open(os.devnull, "r") as devnull:
289+ opensshkey = check_output(
290+ ("setsid", "-w", "ssh-keygen", "-i", "-f", keypath.path),
291+ stdin=devnull, stderr=PIPE, env=env)
292+ except CalledProcessError:
293+ # If this happens it /might/ be an OpenSSH bug. If we've managed
294+ # to convert to RFC4716 form then it seems reasonable to assume
295+ # that OpenSSH has already given this key its blessing.
296+ raise OpenSSHKeyError(
297+ "Key could not be converted from RFC4716 form to "
298+ "OpenSSH public key form.")
299+ else:
300+ keytype, key = opensshkey.decode("utf-8").split()
301+
302+ return " ".join(chain((keytype, key), comments))
303
304=== added file 'src/provisioningserver/utils/tests/test_sshkey.py'
305--- src/provisioningserver/utils/tests/test_sshkey.py 1970-01-01 00:00:00 +0000
306+++ src/provisioningserver/utils/tests/test_sshkey.py 2016-06-14 15:28:23 +0000
307@@ -0,0 +1,156 @@
308+# Copyright 2016 Canonical Ltd. This software is licensed under the
309+# GNU Affero General Public License version 3 (see the file LICENSE).
310+
311+"""Tests for `provisioningserver.utils.sshkey`."""
312+
313+__all__ = []
314+
315+import re
316+
317+from maastesting.factory import factory
318+from maastesting.matchers import DocTestMatches
319+from maastesting.testcase import MAASTestCase
320+from provisioningserver.utils.sshkey import (
321+ normalise_openssh_public_key,
322+ OpenSSHKeyError,
323+)
324+from testtools.matchers import Equals
325+
326+
327+example_openssh_public_keys = {
328+ "ecdsa256": (
329+ "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAA"
330+ "ABBBEqp6hJ9qj6dD1Y1AfsbauzjAaoIQhvTCdg+otLRklg5ZWr8KoS98K50s0eVwcOD7i"
331+ "LltCeS7W0y8c7wlsADVh0= ec2@bar"
332+ ),
333+ "ecdsa384": (
334+ "ecdsa-sha2-nistp384 AAAAE2VjZHNhLXNoYTItbmlzdHAzODQAAAAIbmlzdHAzODQAA"
335+ "ABhBFnB+h79/2MeUR4FoDuKJDyjLEswi8I50NuwIoRbHOwPkPDSDXk6EKfBY0GEwAGyr7"
336+ "h9OjVlmA1KKWUE01KJKf4/iJOh+9zsaL4iQzP9Q9phiUAmxkvegefGwqEXeAvk1Q== "
337+ "ec3@bar"
338+ ),
339+ "ecdsa521": (
340+ "ecdsa-sha2-nistp521 AAAAE2VjZHNhLXNoYTItbmlzdHA1MjEAAAAIbmlzdHA1MjEAA"
341+ "ACFBAFid8WJ6720Z8xJ/Fnsz9eZmUxdbcVNzBeML380gMeBMP9zPXWz629cahQT0HncnK"
342+ "sLsbRB7MMxdaBdsAQ8pteGXQEHVdnr6IkOrVbCHtVaVbjN4gpRICseMnDHrryrOjsvBIU"
343+ "7GGpmmHZka9alvSZlbB1lCx1BxqZZj8AHjJq2KpUh+A== ec5@bar"
344+ ),
345+ "dsa": (
346+ "ssh-dss AAAAB3NzaC1kc3MAAACBALl8PCMaSa3pCCGJaJr4kH0QPlrgyG3Lka+/y4xx1"
347+ "dOuJhpsLe2V9+CKX7Sz1yphCs26KqMFe/ebYGAUDhTdVlE4/TgpAP4GiTjdO1FGXTYdgQ"
348+ "yJpfp50bTUW0zKIP/dwHs5dCLn4XYAxXzSsvORGVQGbM6P6vh3lieTkeVETGZDAAAAFQC"
349+ "AaBKUmPvRqI37VRj1PE9B2rnkfQAAAIEApWYMF0IU+BYUtFuwRRUE9wEGxDEjTtuoWYCW"
350+ "ML7Zn+cFOvK+C0x8YItQ3xIiI3a/0DCoDPIZPvImXDMrs0zUunegndS9g7J0gCHFY9dd+"
351+ "rgYShUHwCI+hy/D9Dp1ukNnGD0bb3x5vEoSK6whrJWBM6is7TW4R5fvz/xDhrtIcxgAAA"
352+ "CBAJbZsmuuWN2kb7lD27IzKcOgd07esoHPWZnv4qg7xhS1GdVr485v73OW1rfpWU6Pdoh"
353+ "ckXLg9ZaoWtVTwNKTfHxS3iug9/pseBWTHdpmxCM5ClsZJii6T4frR5NTOCGKLxOamTs/"
354+ "//OXopZr5u3vT20NFlzFE95J86tGtxYPPivx ubuntu@server-7476"
355+ ),
356+ "ed25519": (
357+ "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIBEqkw2AgmkjqNjCFuiKXeUgLNmRbgVr8"
358+ "W2TlAvFybJv ed255@bar"
359+ ),
360+ "rsa": (
361+ "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDdrzzDZNwyMVBvBTT6kBnrfPZv/AUbk"
362+ "xj7G5CaMTdw6xkKthV22EntD3lxaQxRKzQTfCc2d/CC1K4ushCcRs1S6SQ2zJ2jDq1UmO"
363+ "UkDMgvNh4JVhJYSKc6mu8i3s7oGSmBado5wvtlpSzMrscOpf8Qe/wmT5fH12KB9ipJqoF"
364+ "NQMVbVcVarE/v6wpn3GZC62YRb5iaz9/M+t92Qhu50W2u+KfouqtKB2lwIDDKZMww38Ex"
365+ "tdMouh2FZpxaoh4Uey5bRp3tM3JgnWcX6fyUOp2gxJRPIlD9rrZhX5IkEkZM8MQbdPTQL"
366+ "gIf98oFph5RG6w1t02BvI9nJKM7KkKEfBHt ubuntu@test_rsa0"
367+ ),
368+}
369+
370+
371+def remove_comment(key):
372+ """Remove the comment field from an OpenSSH public key.
373+
374+ Preserves leading and intermediate whitespace where reasonable.
375+ """
376+ match = re.match(r"\s*\S+\s+\S+", key)
377+ assert match is not None, "Could not find keytype and key in %r" % (key,)
378+ return match.group(0)
379+
380+
381+class TestNormaliseOpenSSHPublicKeyBasics(MAASTestCase):
382+ """Tests for `normalise_openssh_public_key`."""
383+
384+ def test_rejects_keys_with_fewer_than_2_parts(self):
385+ example_key = factory.make_name("key")
386+ error = self.assertRaises(
387+ OpenSSHKeyError, normalise_openssh_public_key, example_key)
388+ self.assertThat(str(error), Equals(
389+ "Key should contain 2 or more space separated parts (key type, "
390+ "base64-encoded key, optional comments), not 1: " + example_key))
391+
392+
393+class _TestNormaliseOpenSSHPublicKeyCommon:
394+ """Mix-in tests for `normalise_openssh_public_key`.
395+
396+ Providing tests that are common to keys with and without comments.
397+ """
398+
399+ def test_roundtrip(self):
400+ self.assertThat(
401+ normalise_openssh_public_key(self.key),
402+ Equals(self.key))
403+
404+ def test_rejects_keys_of_unrecognised_type(self):
405+ _, rest = self.key.split(None, 1)
406+ example_type = factory.make_name("type")
407+ example_key = example_type + " " + rest
408+ error = self.assertRaises(
409+ OpenSSHKeyError, normalise_openssh_public_key, example_key)
410+ self.assertThat(str(error), DocTestMatches(
411+ "Key type " + example_type + " not recognised; it should be "
412+ "one of: ... ssh-dss ..."))
413+
414+ def test_rejects_corrupt_keys(self):
415+ parts = self.key.split()
416+ parts[1] = parts[1][:-1] # Remove one character from the key.
417+ example_key = " ".join(parts)
418+ error = self.assertRaises(
419+ OpenSSHKeyError, normalise_openssh_public_key, example_key)
420+ self.assertThat(str(error), Equals(
421+ "Key could not be converted to RFC4716 form."))
422+
423+
424+class TestNormaliseOpenSSHPublicKeyWithComments(
425+ _TestNormaliseOpenSSHPublicKeyCommon, MAASTestCase):
426+ """Tests for `normalise_openssh_public_key` for keys with comments."""
427+
428+ scenarios = sorted(
429+ (name, dict(key=key)) for name, key in
430+ example_openssh_public_keys.items()
431+ )
432+
433+ def test_normalises_mixed_whitespace(self):
434+ parts = self.key.split()
435+ example_key = " %s \t %s\n %s\r\n" % tuple(parts)
436+ self.assertThat(
437+ normalise_openssh_public_key(example_key),
438+ Equals(self.key))
439+
440+ def test_normalises_mixed_whitespace_in_comments(self):
441+ extra_comments = factory.make_name("foo"), factory.make_name("bar")
442+ example_key = self.key + " \t " + " \n\r ".join(extra_comments) + "\n"
443+ expected_key = self.key + " " + " ".join(extra_comments)
444+ self.assertThat(
445+ normalise_openssh_public_key(example_key),
446+ Equals(expected_key))
447+
448+
449+class TestNormaliseOpenSSHPublicKeyWithoutComments(
450+ _TestNormaliseOpenSSHPublicKeyCommon, MAASTestCase):
451+ """Tests for `normalise_openssh_public_key` for keys without comments."""
452+
453+ scenarios = sorted(
454+ (name, dict(key=remove_comment(key))) for name, key in
455+ example_openssh_public_keys.items()
456+ )
457+
458+ def test_normalises_mixed_whitespace(self):
459+ parts = self.key.split()
460+ example_key = " %s \t %s\r\n" % tuple(parts)
461+ self.assertThat(
462+ normalise_openssh_public_key(example_key),
463+ Equals(self.key))