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

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

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

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.

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

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

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.

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.

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

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

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

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

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.

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.

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

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.

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