Merge ~aixtools/cloud-init:ssh_update into cloud-init:master

[Michael Felt]

Update the list of valid ssh keys.

update ssh_util.py with latest keys (from openssh-7.3p1/sshkeys.c)
and remove extinct keys ending with "-<email address hidden>"

Added keys:
  rsa-sha2-256,
  rsa-sha2-512,
  ed25519,
  ssh-ed25519,
  <email address hidden>

Removed both of the double entries for the keys
  <email address hidden>
  <email address hidden>

Update the list of valid ssh keys (from openssh-7.3p1 ssh_keys.c)

** Do not know if this is a "known bug"

[Scott Moser (smoser)]

Michael,

The content of the patch is fine and I can pull it, but there are a few minor/review things.
I do realize these are all nit picks and I am willing to just fix them up myself, so really this is just helping to get you familiar with the review process.

If you'd rather me just do these things, just say so.

Thanks!

a.) set the 'commit message' (Set commit message) to something.
   what you have in Description is fine, except, please just mention what things we are adding.

b.) please go ahead and break VALID_KEY_TYPES into one entry per line and sort it. Ie:
VALID_KEY_TYPES = (
    "dsa",
    "ecdsa",
    ...
    "<email address hidden>",
    "<email address hidden>",
    "<email address hidden>",
)

that will make updating in the future more obvious.

c.) the comment above VALID_KEY_TYPES is > 80 chars now, so 'tox -e flake8' will complain.

review: Needs Fixing

[Michael Felt (aixtools)]

On 19-Dec-16 20:59, Scott Moser wrote:
> Review: Needs Fixing
>
> Michael,
>
> The content of the patch is fine and I can pull it, but there are a few minor/review things.
> I do realize these are all nit picks and I am willing to just fix them up myself, so really this is just helping to get you familiar with the review process.
>
> If you'd rather me just do these things, just say so.
Thanks for the chance to learn. I shall run tox first - ran it on
something else and only saw a lot of tests that failed - not having
valid input (I'll try and fix that in time).

Will reply again later this evening, or tomorrow at the latest.

>
> Thanks!
>
> a.) set the 'commit message' (Set commit message) to something.
> what you have in Description is fine, except, please just mention what things we are adding.
>
> b.) please go ahead and break VALID_KEY_TYPES into one entry per line and sort it. Ie:
> VALID_KEY_TYPES = (
> "dsa",
> "ecdsa",
> ...
> "<email address hidden>",
> "<email address hidden>",
> "<email address hidden>",
> )
>
> that will make updating in the future more obvious.
>
> c.) the comment above VALID_KEY_TYPES is > 80 chars now, so 'tox -e flake8' will complain.
>

[Scott Moser (smoser)]

On Mon, 19 Dec 2016, Michael Felt wrote:

> On 19-Dec-16 20:59, Scott Moser wrote:
> > Review: Needs Fixing
> >
> > Michael,
> >
> > The content of the patch is fine and I can pull it, but there are a few minor/review things.
> > I do realize these are all nit picks and I am willing to just fix them up myself, so really this is just helping to get you familiar with the review process.
> >
> > If you'd rather me just do these things, just say so.
> Thanks for the chance to learn. I shall run tox first - ran it on
> something else and only saw a lot of tests that failed - not having
> valid input (I'll try and fix that in time).

OK, its quite possible some of the failures are due to you running it on
AIX, and things we'd have to fix in that regard. I don't have easy access
to an aix system, but I'm willing to help out if you could get me access
somewhere.

The thing that would fail here is just:

tox -e flake8

[Michael Felt (aixtools)]

On 19-Dec-16 20:59, Scott Moser wrote:
> Review: Needs Fixing
>
> Michael,
>
> The content of the patch is fine and I can pull it, but there are a few minor/review things.
> I do realize these are all nit picks and I am willing to just fix them up myself, so really this is just helping to get you familiar with the review process.
>
> If you'd rather me just do these things, just say so.
>
> Thanks!
>
> a.) set the 'commit message' (Set commit message) to something.
> what you have in Description is fine, except, please just mention what things we are adding.
* what is the command to edit an existing log entry? - and as this is
two changes now, once I commit.

* This is what I would add: do you agree with the removal of the keys
(double entry!) currently
containing <email address hidden>?

ADDED keys:
rsa-sha2-256,
rsa-sha2-512,
ed25519,
ssh-ed25519,
<email address hidden>

REMOVED both of the double entries for the keys (containing v00@)
"<email address hidden>",
"<email address hidden>",

>
> b.) please go ahead and break VALID_KEY_TYPES into one entry per line and sort it. Ie:
> VALID_KEY_TYPES = (
> "dsa",
> "ecdsa",
> ...
> "<email address hidden>",
> "<email address hidden>",
> "<email address hidden>",
> )
# See: man sshd_config
DEF_SSHD_CFG = "/etc/ssh/sshd_config"

# taken from openssh source openssh-7.3p1/sshkey.c:
# static const struct keytype keytypes[] = { ... }
VALID_KEY_TYPES = (
     "dsa",
     "ecdsa",
     "<email address hidden>",
     "<email address hidden>",
     "<email address hidden>",
     "ed25519",
     "rsa",
     "rsa-sha2-256",
     "rsa-sha2-512",
     "ssh-dss",
     "<email address hidden>",
     "ssh-ed25519",
     "<email address hidden>",
     "ssh-rsa",
     "<email address hidden>"
     )

p.s. something for the future - DEF_SSHD_CFG should come from the distro
and/or from the CONFIG_FILE, rather than hard-coded here.
My port of openssh (for AIX) puts the file at
/var/openssh/etc/sshd_config. Mine is minor of course, but does serve as
an example
of how a distro could have a different location (as in my case, to
support multiple versions of ssh(d) installed on a system).

The nit picky part - " )" to close the keytypes, or appended to the
end of the last key?
> that will make updating in the future more obvious.
>
> c.) the comment above VALID_KEY_TYPES is > 80 chars now, so 'tox -e flake8' will complain.
Fixed - now in two lines.

[Scott Moser (smoser)]

> On 19-Dec-16 20:59, Scott Moser wrote:
> > Review: Needs Fixing
> >
> > a.) set the 'commit message' (Set commit message) to something.
> > what you have in Description is fine, except, please just mention what
> things we are adding.
> * what is the command to edit an existing log entry? - and as this is
> two changes now, once I commit.

Well, the merge proposal here has a 'Set commit message'. You an set that to a nice:
  Summary Line
  <blank line>
  Detailed Description

With regard to your git branch, there are 2 ways to do this. Feel free to take either way..
 1.) "squash" commits into one and 'git push --force'
     In this, you'll end up using:
       git commit --ammend -a
     or
       git commit --amend <path>/to/file

 2.) commit each response to merge comments (or other improvements) indivivudally, and I will "squash" them before merging.
     This is probably simpler to wrap your head around, and then I'll just squash your commits into a single commit when I pull the proposal.

To read more about squashing, you can google for 'git rebase'. Once you get familiar with it, its pretty wonderful magic, but it can be a bit scary.

> * This is what I would add: do you agree with the removal of the keys
> (double entry!) currently

I agree, didn't realize we had a double entry. yes, clean it up.

Looks fine, just 'Set Commit message' above.

> "ssh-rsa",
> "<email address hidden>",
> )
>
> p.s. something for the future - DEF_SSHD_CFG should come from the distro
> and/or from the CONFIG_FILE, rather than hard-coded here.
> My port of openssh (for AIX) puts the file at
> /var/openssh/etc/sshd_config. Mine is minor of course, but does serve as
> an example
> of how a distro could have a different location (as in my case, to
> support multiple versions of ssh(d) installed on a system).

Sure, I can see that for sure.

>
> The nit picky part - " )" to close the keytypes, or appended to the
> end of the last key?

The thing that best updates in the future (imo) is to do:
FOO = (
  "e1",
  "e2",
  ...
  "eN",
  "eLast",
)

Ie, you can leave a trailing comma on the last element. But yeah, we're well into nit-picky :)

> > that will make updating in the future more obvious.
> >
> > c.) the comment above VALID_KEY_TYPES is > 80 chars now, so 'tox -e flake8'
> will complain.
> Fixed - now in two lines.

Great.

[Michael Felt (aixtools)]

Hope all is in order now - if not, please be nit picky - I need to learn!

[Scott Moser (smoser)]

Looks great.
I pulled, I made some minor changes to the commit message, but just some that aren't worth discussing.

Thanks.

[Michael Felt (aixtools)]

On 20-Dec-16 20:14, Scott Moser wrote:
> Looks great.
> I pulled, I made some minor changes to the commit message, but just some that aren't worth discussing.
>
> Thanks.

Your welcome. Now on to something bigger :)

diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py
index c74a7ae..0d595b0 100644
--- a/cloudinit/ssh_util.py
+++ b/cloudinit/ssh_util.py
@@ -30,16 +30,25 @@ LOG = logging.getLogger(__name__)
 # See: man sshd_config
 DEF_SSHD_CFG = "/etc/ssh/sshd_config"
 
-# taken from openssh source key.c/key_type_from_name
+# taken from openssh source openssh-7.3p1/sshkey.c:
+# static const struct keytype keytypes[] = { ... }
 VALID_KEY_TYPES = (
-    "rsa", "dsa", "ssh-rsa", "ssh-dss", "ecdsa",
-    "[e-mail]", "[e-mail]",
-    "[e-mail]", "[e-mail]",
-    "[e-mail]", "[e-mail]",
+    "dsa",
+    "ecdsa",
     "[e-mail]",
     "[e-mail]",
-    "[e-mail]")
-
+    "[e-mail]",
+    "ed25519",
+    "rsa",
+    "rsa-sha2-256",
+    "rsa-sha2-512",
+    "ssh-dss",
+    "[e-mail]",
+    "ssh-ed25519",
+    "[e-mail]",
+    "ssh-rsa",
+    "[e-mail]",
+)
 
 class AuthKeyLine(object):
     def __init__(self, source, keytype=None, base64=None,