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

[Gavin Panella]

Permit use of ECDSA and ED25519 SSH keys.

[Mike Pontillo (mpontillo)]

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)]

>     (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)]

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)]

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

[Mike Pontillo (mpontillo)]

(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)]

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)]

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)]

> 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)]

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)]

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?

[Gavin Panella (allenap)]

> 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.

[Gavin Panella (allenap)]

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

[Mike Pontillo (mpontillo)]

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)]

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.

[Gavin Panella (allenap)]

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)]

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)]

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

=== modified file 'src/maasserver/models/sshkey.py'
--- src/maasserver/models/sshkey.py	2016-04-11 16:23:26 +0000
+++ src/maasserver/models/sshkey.py	2016-06-14 15:28:23 +0000
@@ -18,13 +18,10 @@
     TextField,
 )
 from django.utils.safestring import mark_safe
-from maasserver import (
-    DefaultMeta,
-    logger,
-)
+from maasserver import DefaultMeta
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.timestampedmodel import TimestampedModel
-from twisted.conch.ssh.keys import Key
+from provisioningserver.utils.sshkey import normalise_openssh_public_key
 
 
 class SSHKeyManager(Manager):
@@ -38,18 +35,9 @@
 def validate_ssh_public_key(value):
     """Validate that the given value contains a valid SSH public key."""
     try:
-        key = Key.fromString(value.encode("utf-8"))
-    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).")
+        return normalise_openssh_public_key(value)
+    except Exception as error:
+        raise ValidationError("Invalid SSH public key: " + str(error))
 
 
 HELLIPSIS = '…'
 
=== modified file 'src/maasserver/models/tests/test_sshkey.py'
--- src/maasserver/models/tests/test_sshkey.py	2016-05-12 19:07:37 +0000
+++ src/maasserver/models/tests/test_sshkey.py	2016-06-14 15:28:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the SSHKey model."""
@@ -6,16 +6,17 @@
 __all__ = []
 
 import random
-from unittest.mock import Mock
 
 from django.core.exceptions import ValidationError
 from django.db import IntegrityError
 from django.utils.safestring import SafeString
-from maasserver.models import SSHKey
+from maasserver.models import (
+    sshkey,
+    SSHKey,
+)
 from maasserver.models.sshkey import (
     get_html_display_for_key,
     HELLIPSIS,
-    Key,
     validate_ssh_public_key,
 )
 from maasserver.testing import get_data
@@ -36,6 +37,26 @@
         validate_ssh_public_key(key_string)
         # No ValidationError.
 
+    def test_validates_ecdsa_curve256_public_key(self):
+        key_string = get_data('data/test_ecdsa256.pub')
+        validate_ssh_public_key(key_string)
+        # No ValidationError.
+
+    def test_validates_ecdsa_curve384_public_key(self):
+        key_string = get_data('data/test_ecdsa384.pub')
+        validate_ssh_public_key(key_string)
+        # No ValidationError.
+
+    def test_validates_ecdsa_curve521_public_key(self):
+        key_string = get_data('data/test_ecdsa521.pub')
+        validate_ssh_public_key(key_string)
+        # No ValidationError.
+
+    def test_validates_ed25519_public_key(self):
+        key_string = get_data('data/test_ed25519.pub')
+        validate_ssh_public_key(key_string)
+        # No ValidationError.
+
     def test_does_not_validate_random_data(self):
         key_string = factory.make_string()
         self.assertRaises(
@@ -57,9 +78,9 @@
             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()))
+        # Validation fails if normalise_openssh_public_key crashes.
+        norm = self.patch(sshkey, "normalise_openssh_public_key")
+        norm.side_effect = factory.make_exception_type()
         self.assertRaises(
             ValidationError, validate_ssh_public_key, factory.make_name('key'))
 
 
=== added file 'src/maasserver/tests/data/test_ecdsa256.pub'
--- src/maasserver/tests/data/test_ecdsa256.pub	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/data/test_ecdsa256.pub	2016-06-14 15:28:23 +0000
@@ -0,0 +1,1 @@
+ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBEqp6hJ9qj6dD1Y1AfsbauzjAaoIQhvTCdg+otLRklg5ZWr8KoS98K50s0eVwcOD7iLltCeS7W0y8c7wlsADVh0= foo@bar
 
=== added file 'src/maasserver/tests/data/test_ecdsa384.pub'
--- src/maasserver/tests/data/test_ecdsa384.pub	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/data/test_ecdsa384.pub	2016-06-14 15:28:23 +0000
@@ -0,0 +1,1 @@
+ecdsa-sha2-nistp384 AAAAE2VjZHNhLXNoYTItbmlzdHAzODQAAAAIbmlzdHAzODQAAABhBFnB+h79/2MeUR4FoDuKJDyjLEswi8I50NuwIoRbHOwPkPDSDXk6EKfBY0GEwAGyr7h9OjVlmA1KKWUE01KJKf4/iJOh+9zsaL4iQzP9Q9phiUAmxkvegefGwqEXeAvk1Q== foo@bar
 
=== added file 'src/maasserver/tests/data/test_ecdsa521.pub'
--- src/maasserver/tests/data/test_ecdsa521.pub	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/data/test_ecdsa521.pub	2016-06-14 15:28:23 +0000
@@ -0,0 +1,1 @@
+ecdsa-sha2-nistp521 AAAAE2VjZHNhLXNoYTItbmlzdHA1MjEAAAAIbmlzdHA1MjEAAACFBAFid8WJ6720Z8xJ/Fnsz9eZmUxdbcVNzBeML380gMeBMP9zPXWz629cahQT0HncnKsLsbRB7MMxdaBdsAQ8pteGXQEHVdnr6IkOrVbCHtVaVbjN4gpRICseMnDHrryrOjsvBIU7GGpmmHZka9alvSZlbB1lCx1BxqZZj8AHjJq2KpUh+A== foo@bar
 
=== added file 'src/maasserver/tests/data/test_ed25519.pub'
--- src/maasserver/tests/data/test_ed25519.pub	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/data/test_ed25519.pub	2016-06-14 15:28:23 +0000
@@ -0,0 +1,1 @@
+ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIBEqkw2AgmkjqNjCFuiKXeUgLNmRbgVr8W2TlAvFybJv foo@bar
 
=== added file 'src/provisioningserver/utils/sshkey.py'
--- src/provisioningserver/utils/sshkey.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/utils/sshkey.py	2016-06-14 15:28:23 +0000
@@ -0,0 +1,161 @@
+# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Utilities for working with OpenSSH keys."""
+
+__all__ = [
+    "normalise_openssh_public_key",
+    "OpenSSHKeyError",
+]
+
+from itertools import chain
+import os
+from pathlib import Path
+import pipes
+from subprocess import (
+    CalledProcessError,
+    check_output,
+    PIPE,
+)
+from tempfile import TemporaryDirectory
+
+from provisioningserver.utils.shell import select_c_utf8_locale
+
+
+OPENSSH_PROTOCOL2_KEY_TYPES = frozenset((
+    "ecdsa-sha2-nistp256",
+    "ecdsa-sha2-nistp384",
+    "ecdsa-sha2-nistp521",
+    "ssh-dss",
+    "ssh-ed25519",
+    "ssh-rsa",
+))
+
+
+class OpenSSHKeyError(ValueError):
+    """The given key was not recognised or was corrupt."""
+
+
+def normalise_openssh_public_key(keytext):
+    """Validate and normalise an OpenSSH public key.
+
+    Essentially: ensure we have a public key first (and not try to extract a
+    public key from a private key) and then pump it through an ssh-keygen(1)
+    pipeline 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. It does not even appear to be possible
+    to 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. Public key files
+    generated by ssh-keygen(1) will not contain options.
+
+    Given all that, this function does the following:
+
+    1. Checks there are 2 or more fields: keytype base64-encoded-key [comment]
+    (the comment can contain whitespace).
+
+    2. Checks that keytype is one of “ssh-dss”, “ssh-rsa”, “ssh-ed25519”,
+    “ecdsa-sha2-nistp256”, “ecdsa-sha2-nistp384”, or “ecdsa-sha2-nistp521”,
+
+    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 use the caller's TTY. This
+    is Python, and no recourse to a shell is being taken, but it has similar
+    behaviour.
+
+    4. $pubkey should contain two fields: keytype, base64-encoded key.
+
+    5. Reunite $pubkey with comment, if there was one.
+
+    Errors from ssh-keygen(1) at any point should be reported *with the error
+    message*. Previously all errors relating to SSH keys were coalesced into
+    the same static message.
+
+    """
+    parts = keytext.split()
+    if len(parts) >= 2:
+        keytype, key, *comments = parts
+    else:
+        raise OpenSSHKeyError(
+            "Key should contain 2 or more space separated parts (key type, "
+            "base64-encoded key, optional comments), not %d: %s" % (
+                len(parts), " ".join(map(pipes.quote, parts))))
+
+    if keytype not in OPENSSH_PROTOCOL2_KEY_TYPES:
+        raise OpenSSHKeyError(
+            "Key type %s not recognised; it should be one of: %s" % (
+                pipes.quote(keytype), " ".join(
+                    sorted(OPENSSH_PROTOCOL2_KEY_TYPES))))
+
+    env = select_c_utf8_locale()
+    # Request OpenSSH to use /bin/true when prompting for passwords. We also
+    # have to redirect stdin from, say, /dev/null so that it doesn't use the
+    # terminal (when this is executed from a terminal).
+    env["SSH_ASKPASS"] = "/bin/true"
+
+    with TemporaryDirectory(prefix="maas") as tempdir:
+        keypath = Path(tempdir).joinpath("intermediate")
+        # Ensure that this file is locked-down.
+        keypath.touch()
+        keypath.chmod(0o600)
+        # Convert given key to RFC4716 form.
+        keypath.write_text("%s %s" % (keytype, key), "utf-8")
+        try:
+            with open(os.devnull, "r") as devnull:
+                rfc4716key = check_output(
+                    ("setsid", "-w", "ssh-keygen", "-e", "-f", keypath.path),
+                    stdin=devnull, stderr=PIPE, env=env)
+        except CalledProcessError:
+            raise OpenSSHKeyError(
+                "Key could not be converted to RFC4716 form.")
+        # Convert RFC4716 back to OpenSSH format public key.
+        keypath.write_bytes(rfc4716key)
+        try:
+            with open(os.devnull, "r") as devnull:
+                opensshkey = check_output(
+                    ("setsid", "-w", "ssh-keygen", "-i", "-f", keypath.path),
+                    stdin=devnull, stderr=PIPE, env=env)
+        except CalledProcessError:
+            # If this happens it /might/ be an OpenSSH bug. If we've managed
+            # to convert to RFC4716 form then it seems reasonable to assume
+            # that OpenSSH has already given this key its blessing.
+            raise OpenSSHKeyError(
+                "Key could not be converted from RFC4716 form to "
+                "OpenSSH public key form.")
+        else:
+            keytype, key = opensshkey.decode("utf-8").split()
+
+    return " ".join(chain((keytype, key), comments))
 
=== added file 'src/provisioningserver/utils/tests/test_sshkey.py'
--- src/provisioningserver/utils/tests/test_sshkey.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/utils/tests/test_sshkey.py	2016-06-14 15:28:23 +0000
@@ -0,0 +1,156 @@
+# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `provisioningserver.utils.sshkey`."""
+
+__all__ = []
+
+import re
+
+from maastesting.factory import factory
+from maastesting.matchers import DocTestMatches
+from maastesting.testcase import MAASTestCase
+from provisioningserver.utils.sshkey import (
+    normalise_openssh_public_key,
+    OpenSSHKeyError,
+)
+from testtools.matchers import Equals
+
+
+example_openssh_public_keys = {
+    "ecdsa256": (
+        "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAA"
+        "ABBBEqp6hJ9qj6dD1Y1AfsbauzjAaoIQhvTCdg+otLRklg5ZWr8KoS98K50s0eVwcOD7i"
+        "LltCeS7W0y8c7wlsADVh0= ec2@bar"
+    ),
+    "ecdsa384": (
+        "ecdsa-sha2-nistp384 AAAAE2VjZHNhLXNoYTItbmlzdHAzODQAAAAIbmlzdHAzODQAA"
+        "ABhBFnB+h79/2MeUR4FoDuKJDyjLEswi8I50NuwIoRbHOwPkPDSDXk6EKfBY0GEwAGyr7"
+        "h9OjVlmA1KKWUE01KJKf4/iJOh+9zsaL4iQzP9Q9phiUAmxkvegefGwqEXeAvk1Q== "
+        "ec3@bar"
+    ),
+    "ecdsa521": (
+        "ecdsa-sha2-nistp521 AAAAE2VjZHNhLXNoYTItbmlzdHA1MjEAAAAIbmlzdHA1MjEAA"
+        "ACFBAFid8WJ6720Z8xJ/Fnsz9eZmUxdbcVNzBeML380gMeBMP9zPXWz629cahQT0HncnK"
+        "sLsbRB7MMxdaBdsAQ8pteGXQEHVdnr6IkOrVbCHtVaVbjN4gpRICseMnDHrryrOjsvBIU"
+        "7GGpmmHZka9alvSZlbB1lCx1BxqZZj8AHjJq2KpUh+A== ec5@bar"
+    ),
+    "dsa": (
+        "ssh-dss AAAAB3NzaC1kc3MAAACBALl8PCMaSa3pCCGJaJr4kH0QPlrgyG3Lka+/y4xx1"
+        "dOuJhpsLe2V9+CKX7Sz1yphCs26KqMFe/ebYGAUDhTdVlE4/TgpAP4GiTjdO1FGXTYdgQ"
+        "yJpfp50bTUW0zKIP/dwHs5dCLn4XYAxXzSsvORGVQGbM6P6vh3lieTkeVETGZDAAAAFQC"
+        "AaBKUmPvRqI37VRj1PE9B2rnkfQAAAIEApWYMF0IU+BYUtFuwRRUE9wEGxDEjTtuoWYCW"
+        "ML7Zn+cFOvK+C0x8YItQ3xIiI3a/0DCoDPIZPvImXDMrs0zUunegndS9g7J0gCHFY9dd+"
+        "rgYShUHwCI+hy/D9Dp1ukNnGD0bb3x5vEoSK6whrJWBM6is7TW4R5fvz/xDhrtIcxgAAA"
+        "CBAJbZsmuuWN2kb7lD27IzKcOgd07esoHPWZnv4qg7xhS1GdVr485v73OW1rfpWU6Pdoh"
+        "ckXLg9ZaoWtVTwNKTfHxS3iug9/pseBWTHdpmxCM5ClsZJii6T4frR5NTOCGKLxOamTs/"
+        "//OXopZr5u3vT20NFlzFE95J86tGtxYPPivx ubuntu@server-7476"
+    ),
+    "ed25519": (
+        "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIBEqkw2AgmkjqNjCFuiKXeUgLNmRbgVr8"
+        "W2TlAvFybJv ed255@bar"
+    ),
+    "rsa": (
+        "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDdrzzDZNwyMVBvBTT6kBnrfPZv/AUbk"
+        "xj7G5CaMTdw6xkKthV22EntD3lxaQxRKzQTfCc2d/CC1K4ushCcRs1S6SQ2zJ2jDq1UmO"
+        "UkDMgvNh4JVhJYSKc6mu8i3s7oGSmBado5wvtlpSzMrscOpf8Qe/wmT5fH12KB9ipJqoF"
+        "NQMVbVcVarE/v6wpn3GZC62YRb5iaz9/M+t92Qhu50W2u+KfouqtKB2lwIDDKZMww38Ex"
+        "tdMouh2FZpxaoh4Uey5bRp3tM3JgnWcX6fyUOp2gxJRPIlD9rrZhX5IkEkZM8MQbdPTQL"
+        "gIf98oFph5RG6w1t02BvI9nJKM7KkKEfBHt ubuntu@test_rsa0"
+    ),
+}
+
+
+def remove_comment(key):
+    """Remove the comment field from an OpenSSH public key.
+
+    Preserves leading and intermediate whitespace where reasonable.
+    """
+    match = re.match(r"\s*\S+\s+\S+", key)
+    assert match is not None, "Could not find keytype and key in %r" % (key,)
+    return match.group(0)
+
+
+class TestNormaliseOpenSSHPublicKeyBasics(MAASTestCase):
+    """Tests for `normalise_openssh_public_key`."""
+
+    def test_rejects_keys_with_fewer_than_2_parts(self):
+        example_key = factory.make_name("key")
+        error = self.assertRaises(
+            OpenSSHKeyError, normalise_openssh_public_key, example_key)
+        self.assertThat(str(error), Equals(
+            "Key should contain 2 or more space separated parts (key type, "
+            "base64-encoded key, optional comments), not 1: " + example_key))
+
+
+class _TestNormaliseOpenSSHPublicKeyCommon:
+    """Mix-in tests for `normalise_openssh_public_key`.
+
+    Providing tests that are common to keys with and without comments.
+    """
+
+    def test_roundtrip(self):
+        self.assertThat(
+            normalise_openssh_public_key(self.key),
+            Equals(self.key))
+
+    def test_rejects_keys_of_unrecognised_type(self):
+        _, rest = self.key.split(None, 1)
+        example_type = factory.make_name("type")
+        example_key = example_type + " " + rest
+        error = self.assertRaises(
+            OpenSSHKeyError, normalise_openssh_public_key, example_key)
+        self.assertThat(str(error), DocTestMatches(
+            "Key type " + example_type + " not recognised; it should be "
+            "one of: ... ssh-dss ..."))
+
+    def test_rejects_corrupt_keys(self):
+        parts = self.key.split()
+        parts[1] = parts[1][:-1]  # Remove one character from the key.
+        example_key = " ".join(parts)
+        error = self.assertRaises(
+            OpenSSHKeyError, normalise_openssh_public_key, example_key)
+        self.assertThat(str(error), Equals(
+            "Key could not be converted to RFC4716 form."))
+
+
+class TestNormaliseOpenSSHPublicKeyWithComments(
+        _TestNormaliseOpenSSHPublicKeyCommon, MAASTestCase):
+    """Tests for `normalise_openssh_public_key` for keys with comments."""
+
+    scenarios = sorted(
+        (name, dict(key=key)) for name, key in
+        example_openssh_public_keys.items()
+    )
+
+    def test_normalises_mixed_whitespace(self):
+        parts = self.key.split()
+        example_key = "  %s \t %s\n  %s\r\n" % tuple(parts)
+        self.assertThat(
+            normalise_openssh_public_key(example_key),
+            Equals(self.key))
+
+    def test_normalises_mixed_whitespace_in_comments(self):
+        extra_comments = factory.make_name("foo"), factory.make_name("bar")
+        example_key = self.key + " \t " + " \n\r ".join(extra_comments) + "\n"
+        expected_key = self.key + " " + " ".join(extra_comments)
+        self.assertThat(
+            normalise_openssh_public_key(example_key),
+            Equals(expected_key))
+
+
+class TestNormaliseOpenSSHPublicKeyWithoutComments(
+        _TestNormaliseOpenSSHPublicKeyCommon, MAASTestCase):
+    """Tests for `normalise_openssh_public_key` for keys without comments."""
+
+    scenarios = sorted(
+        (name, dict(key=remove_comment(key))) for name, key in
+        example_openssh_public_keys.items()
+    )
+
+    def test_normalises_mixed_whitespace(self):
+        parts = self.key.split()
+        example_key = "  %s \t %s\r\n" % tuple(parts)
+        self.assertThat(
+            normalise_openssh_public_key(example_key),
+            Equals(self.key))