Update the list of valid ssh keys.

[omBratteng]

Update ssh_util.py with latest list of keys (from openssh-8.3p1/sshkey.c),

Added keys:
sk-ecdsa-sha2-nistp256-cert-v01@openssh.com
sk-ecdsa-sha2-nistp256@openssh.com
sk-ssh-ed25519-cert-v01@openssh.com
sk-ssh-ed25519@openssh.com
ssh-xmss-cert-v01@openssh.com
ssh-xmss@openssh.com

LP: #1877869

[omBratteng]

I tried testing locally with tox, worked fine. But not sure what the best way to test the newly added keys are.

[OddBloke]

Hi @omBratteng, thanks for the submission! The actual change here LGTM, it just needs tests adding to https://github.com/canonical/cloud-init/blob/master/tests/unittests/test_sshutil.py. (The last time this was done was in 853df0a which should give you a rough idea of how to proceed. 👍)

[omBratteng]

Thanks @OddBloke, I'll try to get some tests. I'm a bit unfamiliar with tox, is there a way I can run the specific test https://github.com/canonical/cloud-init/blob/master/tests/unittests/test_sshutil.py ?

[OddBloke]

Thanks for the update @omBratteng! You've presumably figured this out, but tox -e py3 -- path/to/test/file.py will run all the tests in a file, or tox -e py3 -- path/to/test/file.py::SpecificTestClass will run a single class in a file. (More generally, anything after the -- is passed to pytest.)

I notice that the tests only include new cases for a subset of the key types you're adding; is it feasible to expand that list out further, to cover all the newly-added types?

[OddBloke]

(Oh and as a note to my future self: I have confirmed you've signed the CLA.)

[omBratteng]

I just spun up a faster VM for running the tests, but I'll try it later.
Yeah, I can try to add tests for the -cert variants too, I just have never used ssh certs. I can also add tests for the rest of the valid SSH keys, as I see it's just a subset of all the valid ones.

[OddBloke]

On Tue, Jul 14, 2020 at 05:57:01AM -0700, Ole-Martin Bratteng wrote: I just spun up a faster VM for running the tests, but I'll try it later.

Fair enough!

Yeah, I can try to add tests for the -cert variants too, I just have never used ssh certs. I can also add tests for the rest of the valid SSH keys, as I see it's just a subset of all the valid ones.

Thanks, that would be much appreciated!

[omBratteng]

@OddBloke added more tests now, and removed some key types which @djmdjm pointed out for me which actually are signatures.
Plus removed dsa, rsa, ecdsa and ed25519, as they're not valid public keys, as they're the short names used for human interactions, e.g. "ssh-keygen -t rsa" and they should never appear in public keys

[OddBloke]

Plus removed dsa, rsa, ecdsa and ed25519, as they're not valid public keys, as they're the short names used for human interactions, e.g. "ssh-keygen -t rsa" and they should never appear in public keys

Older versions of OpenSSH (I tested with 7.2, the version in Ubuntu 16.04 which we still actively backport cloud-init to) will allow you to SSH in if an authorized_keys line starts with "rsa", so removing these is premature.

(I'm running to a meeting, so haven't reviewed anything more than this.)

[omBratteng]

Gotcha, I can either revert the last three commits, or just f77a723 which is the one that removes dsa, rsa, ecdsa and ed25519 from the valid key types, the other two are just testing.

[OddBloke]

I'd opt for reverting all three commits, if you're happy doing that?

[omBratteng]

Commits reverted

[OddBloke]

Thanks for the update! Determining the list of public key types is more involved than I was expecting, so I've asked for an update to an explanatory comment inline (and also have a minor test improvement Q).

[omBratteng]

I've added some comments, both in the source and docs, about where the list comes from. Let me know if you want me to change to wording of it, writing documentation for this sized project isn't something I have done before.

[paride]

Related PR: #506.

[omBratteng]

@paride I cherry picked @tsanghan's commit into my branch, updated the _is_printable_key list.
And added a pointer in both cc_ssh_authkey_fingerprints and ssh_util, both pointing to eachother, so the next person knows to update all the lists.

[OddBloke]

One question about the additional commit that was added here.

[blackboxsw]

+1

Review comments were addressed sufficiently by the author. So, I'm dismissing this review as "addressed" by the author