opened
tjstansell
username_0
I was reading #20711 and happened to notice that commit 18d24ea9 changed host_keys from doing a read() to a readline().strip(), which means it only reads the first line of each file now.  I'm not sure if this was intentional, but it prevents being able to read multi-line private keys (should someone actually want to).  As it stands today, ssh.host_keys simply shows things like this:
```
host01:
    ----------
    dsa:
        -----BEGIN DSA PRIVATE KEY-----
    dsa.pub:
        ssh-dss AAAAB3Nza...
    key:
        SSH PRIVATE KEY FILE FORMAT 1.1
    key.pub.pub:
        2048 35 280958967...
    rsa:
        -----BEGIN RSA PRIVATE KEY-----
    rsa.pub:
        ssh-rsa AAAAB3Nza...
```
salt.modules.ssh.host_keys only reads first line
issue
created
basepi
username_1
Are you saying this changed recently, or you just happened to notice it now? That commit is from 2012, so that's been the behavior for a long time.

I didn't actually know that you could have multi-line private keys.... what does that look like?
comment
created
tjstansell
username_0
Sorry, I only recently noticed, not that it's changed recently ... so yeah, i think it's been this way since 2012.  Here's an example private key on one of my hosts:

```
-----BEGIN RSA PRIVATE KEY-----
MIIEowIBAAKCAQEAuCD2Pq5FoWQcUbPIZv2//V/JcFgLDrKyJ4igVcGmyFe9//xT
559cS0DPpOVMuXqhOhm9LdbBPF4hgcpDnqDxhWUGVK5Hwzz3WE8N/nsEeSciABfo
ac9Zx7/5Hm82pnmOpnseOHM9VTnqb5z4RU6OtDLM1yNtp8DDJ4ER937rb3H1YHZs
9GUlSNbpB5LT11eXfchrpv2r38y1yW+Cl+ohjW1Lf3cwNFjtTddPR1+1J+42VuiQ
TNzbpi8MClufPEstMbi33JdkEqZ/jU9WLr/ihB/R+RL8Zem+TnaJZsm4nO9jeOzj
J+d+0V7qavZfnraoa5YL6e7tsJr4CB5J88VNIQIDAQABAoIBAAXvwiC5O7N9Z9BW
....
-----END RSA PRIVATE KEY-----
```
comment
created
basepi
username_1
Thanks for the report.
comment
created
bechtoldt
username_2
@username_0 this should be fixed with #21878.

A short test is appreciated. ;)
comment
created
tjstansell
username_0
To add more heartache to this discussion, I manually changed this on my test box, and ran `ssh.host_keys` to see the result.  One of the private keys on my test host is actually in binary format... 

```
/etc/ssh/ssh_host_dsa_key:     ASCII text
/etc/ssh/ssh_host_dsa_key.pub: ASCII text, with very long lines
/etc/ssh/ssh_host_key:         data
/etc/ssh/ssh_host_key.pub:     ASCII text, with very long lines
/etc/ssh/ssh_host_rsa_key:     ASCII text
/etc/ssh/ssh_host_rsa_key.pub: ASCII text, with very long lines
```
so you get all kinds of garbled output for that key.  The private keys have header lines like this:
```
# head -1 /etc/ssh/ssh_host_*key
==> /etc/ssh/ssh_host_dsa_key <==
-----BEGIN DSA PRIVATE KEY-----

==> /etc/ssh/ssh_host_key <==
SSH PRIVATE KEY FILE FORMAT 1.1

==> /etc/ssh/ssh_host_rsa_key <==
-----BEGIN RSA PRIVATE KEY-----
```
So maybe that was the original reason for only reading the first line (to get rid of all the binary junk)?
comment
created
bechtoldt
username_2
@username_0 what system are you running? Could it be reasonable to skip binary files?

```
# file /etc/ssh/ssh_host*
/etc/ssh/ssh_host_dsa_key:       PEM DSA private key
/etc/ssh/ssh_host_dsa_key.pub:   OpenSSH DSA public key
/etc/ssh/ssh_host_ecdsa_key:     ASCII text
/etc/ssh/ssh_host_ecdsa_key.pub: ASCII text
/etc/ssh/ssh_host_rsa_key:       PEM RSA private key
/etc/ssh/ssh_host_rsa_key.pub:   OpenSSH RSA public key
```

```
# lsb_release -a
No LSB modules are available.
Distributor ID:	Debian
Description:	Debian GNU/Linux 7.8 (wheezy)
Release:	7.8
Codename:	wheezy
```
comment
created
tjstansell
username_0
This happens to be a CentOS 6 box that was a pretty generic kickstart install.  Not sure why it's creating a key file that's using such an old format...

I looked through the openssh code and in `sshkey.c`, it has this:
```
/* Version identification string for SSH v1 identity files. */
#define LEGACY_BEGIN            "SSH PRIVATE KEY FILE FORMAT 1.1\n"
```
So, I think it's safe to only read the first line if it contains this string, otherwise read the whole file.
comment
closed
thatch45
username_3
issue
created
tjstansell
username_0
I don't think the change that was committed is the correct fix.  It changes the behavior to no longer strip the private keys, which is the "backward compatibility" issue that was discussed in the pull request.  After researching the SSH key format more, I believe the following fix would actually do what we want:

```diff
diff --git a/salt/modules/ssh.py b/salt/modules/ssh.py
index 8da33bf..245a16e 100644
--- a/salt/modules/ssh.py
+++ b/salt/modules/ssh.py
@@ -255,7 +255,10 @@ def host_keys(keydir=None):
                 kname += '.{0}'.format(top[1])
             try:
                 with salt.utils.fopen(os.path.join(keydir, fn_), 'r') as _fh:
-                    keys[kname] = _fh.readline().strip()
+                    keys[kname] = _fh.readline()
+                    if keys[kname] != "SSH PRIVATE KEY FILE FORMAT 1.1\n":
+                        keys[kname] += _fh.read()
+                    keys[kname] = keys[kname].strip()
             except (IOError, OSError):
                 keys[kname] = ''
     return keys
```

This reads the first line, if it's the legacy binary header, stop there, otherwise, read the rest of the file and append it.  Then, strip the result regardless (to stick with a stripped output).  I can submit a pull request if needed, but I think the 8f5380b commit should be reverted.
comment
created
tjstansell
username_0
Also, I noticed that the pull request change referenced issue #20708, which talks about stripping trailing newlines for *user* keys.  I think that led to some confusion in the pull request discussion because user keys were not being stripped, but host keys were.  The goal is to strip them all, which is more reason why 8f5380b should get reverted (since it no longer strips the private key).
comment
created
tjstansell
username_0
Regarding my previous comments, I was confused by the way github is referencing commit 8f5380b, but in reality the merge included that plus the other commit in the pull request... so I didn't realize that the entire pull request had been merged.  I'll file a new issue about not reading the legacy 1.1 binary private key.
comment