Bug 2617 - sign_and_send_pubkey: no separate private key for certificate
Summary: sign_and_send_pubkey: no separate private key for certificate
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: ssh (show other bugs)
Version: 7.3p1
Hardware: All All
: P5 normal
Assignee: Damien Miller
URL:
Keywords:
Depends on:
Blocks: V_7_5
  Show dependency treegraph
 
Reported: 2016-09-27 04:34 AEST by Peter
Modified: 2018-04-06 12:26 AEST (History)
4 users (show)

See Also:


Attachments
probable fix (319 bytes, patch)
2016-10-28 15:01 AEDT, Damien Miller
no flags Details | Diff
revised fix (520 bytes, patch)
2016-10-28 18:15 AEDT, Damien Miller
dtucker: ok+
Details | Diff
try to support IdentityFile w/ no key.pub with CertificateFile (1.60 KB, patch)
2016-12-02 16:51 AEDT, Damien Miller
no flags Details | Diff
Allow for id->key being NULL before being passed to identity_sign() (1.82 KB, patch)
2016-12-06 23:33 AEDT, Adam Eijdenberg
no flags Details | Diff
Tests (3.17 KB, patch)
2016-12-06 23:57 AEDT, Adam Eijdenberg
no flags Details | Diff
Load key files for matching cert names (1.88 KB, patch)
2016-12-07 00:05 AEDT, Adam Eijdenberg
no flags Details | Diff
Tests (fixed patch format) (3.25 KB, application/octet-stream)
2016-12-07 00:09 AEDT, Adam Eijdenberg
no flags Details
Tests (3.25 KB, patch)
2016-12-07 00:11 AEDT, Adam Eijdenberg
no flags Details | Diff
Allow for id->key being NULL before being passed to identity_sign() (1.89 KB, patch)
2016-12-07 00:14 AEDT, Adam Eijdenberg
no flags Details | Diff
consolidated and tweaked patches (8.46 KB, patch)
2016-12-09 13:49 AEDT, Damien Miller
dtucker: ok+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter 2016-09-27 04:34:21 AEST
This worked back in openssh 6.  I'd just recently updated to OSX 10.12 and it stopped right after.  Openssh 7.2+ seems to be a point in which I know it has changed.  I have since tested this on Ubuntu 16.04 with openssh 7.2 with same results, so it's not a platform issue.  I also updated ssh through homebrew on the mac to 7.3p1.  

First look on bugzilla, I thought it was related to the 2550 bug (https://bugzilla.mindrot.org/show_bug.cgi?id=2550), but that was fixed in 7.3p1.

The process using ssh certificate authentication through an SSH proxy host.  The private key is in the downloaded certificate.  Openssh is now looking for a separate ssh private key file.

Via 7.3 failure:
ssh -vvv -o 'ProxyCommand ssh -i ~/.ssh/bastion_key my.name@<BASTIONHOST> -W %h:%p' ec2-user@<EC2HOST> -i ~/.ssh/bastion_key
OpenSSH_7.3p1, LibreSSL 2.4.2
debug1: Reading configuration data /Users/user/.ssh/config
debug1: /Users/user/.ssh/config line 27: Applying options for 10.*
debug1: Reading configuration data /usr/local/etc/ssh/ssh_config
debug1: Executing proxy command: exec ssh -i ~/.ssh/bastion_key my.name@<BASTIONHOST> -W <EC2HOST>:22
debug1: permanently_drop_suid: ######
debug1: key_load_public: No such file or directory
debug1: identity file /Users/user/.ssh/bastion_key type -1
debug1: identity file /Users/user/.ssh/bastion_key-cert type 5
debug1: Enabling compatibility mode for protocol 2.0
debug1: Local version string SSH-2.0-OpenSSH_7.3
no such identity: /Users/user/.ssh/bastion_key-cert: No such file or directory
Permission denied (publickey).
ssh_exchange_identification: Connection closed by remote host

When I check out the bastion file, I get the following:
$ ls -l ~/.ssh/bastion_key*
-rw------- 1 user group 1675 Sep 26 14:09 /Users/user/.ssh/bastion_key
-rw-r--r-- 1 user group 1539 Sep 26 14:09 /Users/user/.ssh/bastion_key-cert.pub


Docker container with OpenSSH 6.6 works(docker is why its all as root):
[root@18be76b35451 ~]# ssh -vvv -o 'ProxyCommand ssh -i ~/.ssh/bastion_key my.name@<BASTIONHOST> -W %h:%p' ec2-user@<EC2HOST> -i ~/.ssh/bastion_key
OpenSSH_6.6.1, OpenSSL 1.0.1e-fips 11 Feb 2013
debug1: Reading configuration data /etc/ssh/ssh_config
debug1: /etc/ssh/ssh_config line 56: Applying options for *
debug1: Executing proxy command: exec ssh -i ~/.ssh/bastion_key my.name@<BASTIONHOST> -W <EC2HOST>:22
debug1: permanently_set_uid: 0/0
debug1: permanently_drop_suid: 0
debug3: Incorrect RSA1 identifier
debug3: Could not load "/root/.ssh/bastion_key" as a RSA1 public key
debug1: identity file /root/.ssh/bastion_key type -1
debug1: ssh_rsa_verify: signature correct
debug1: identity file /root/.ssh/bastion_key-cert type 5
debug1: Enabling compatibility mode for protocol 2.0
debug1: Local version string SSH-2.0-OpenSSH_6.6.1
debug1: Remote protocol version 2.0, remote software version OpenSSH_6.6.1
debug1: match: OpenSSH_6.6.1 pat OpenSSH_6.6.1* compat 0x04000000
debug2: fd 5 setting O_NONBLOCK
debug2: fd 4 setting O_NONBLOCK
debug3: load_hostkeys: loading entries for host "<EC2HOST>" from file "/root/.ssh/known_hosts"
debug3: load_hostkeys: found key type ECDSA in file /root/.ssh/known_hosts:2
debug3: load_hostkeys: loaded 1 keys
....

[root@18be76b35451 ~]# ls -l ~/.ssh/bastion_key*
-rw------- 1 root root 1679 Sep 26 18:25 /root/.ssh/bastion_key
-rw-r--r-- 1 root root 1539 Sep 26 18:25 /root/.ssh/bastion_key-cert.pub

Let me know if more logs are needed.  I can do more debugging also if this isn't the right data.
Comment 1 Damien Miller 2016-10-28 15:01:37 AEDT
Created attachment 2884 [details]
probable fix

I think this patch should fix the problem. Could you please test it?
Comment 2 Damien Miller 2016-10-28 18:15:09 AEDT
Created attachment 2886 [details]
revised fix

Previous fix had a problem, please try this one
Comment 3 Peter 2016-10-29 01:00:05 AEDT
I was able to test and confirm this resolved the issue.  

Thanks for the fix.  Do you have an ideas when either p2 or 7.4 will be released?

Thanks again.
Comment 4 Adam Eijdenberg 2016-11-01 11:36:15 AEDT
I found this bug after preparing a similar patch (including tests).

Although the patch provided here is simpler, it fails when using the new CertificateFile configuration line (which was introduced in the commit that broke the old behaviour).

e.g. the following config:

CertificateFile /Users/aeijdenberg/.ssh/id_androgogic_shortlived_rsa-cert.pub
IdentityFile /Users/aeijdenberg/.ssh/id_androgogic_shortlived_rsa

debug1: Offering RSA-CERT public key: /Users/aeijdenberg/.ssh/id_androgogic_shortlived_rsa-cert.pub
debug1: Server accepts key: pkalg ssh-rsa-cert-v01@openssh.com blen 1540
debug1: sign_and_send_pubkey: no separate private key for certificate "/Users/aeijdenberg/.ssh/id_androgogic_shortlived_rsa-cert.pub"
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@         WARNING: UNPROTECTED PRIVATE KEY FILE!          @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
Permissions 0644 for '/Users/aeijdenberg/.ssh/id_androgogic_shortlived_rsa-cert.pub' are too open.
It is required that your private key files are NOT accessible by others.
This private key will be ignored.
Load key "/Users/aeijdenberg/.ssh/id_androgogic_shortlived_rsa-cert.pub": bad permissions
debug1: Trying private key: /Users/aeijdenberg/.ssh/id_androgogic_shortlived_rsa
debug1: Authentications that can continue: publickey,password
debug1: No more authentication methods to try.
Permission denied (publickey,password).

(and just changing the permissions didn't seem to help, it instead prompted me for a password for the cert file, which doesn't need one)

Commenting out the explicit reference in config to CertificateFile makes it work again.

Here is the alternate patch I had put together - it includes tests, and also addresses a few other somewhat related issues:
https://github.com/openssh/openssh-portable/pull/53
Comment 5 Damien Miller 2016-12-02 14:28:08 AEDT
(In reply to Adam Eijdenberg from comment #4)
> I found this bug after preparing a similar patch (including tests).
> 
> Although the patch provided here is simpler, it fails when using the
> new CertificateFile configuration line (which was introduced in the
> commit that broke the old behaviour).

I think your pull request goes a bit beyond what's going on here, by removing the restrictions that CertificateFile-loaded keys must have a corresponding plain public key. IMO that's a fine goal, but it's not strictly a regression like this is.
Comment 6 Damien Miller 2016-12-02 16:51:15 AEDT
Created attachment 2899 [details]
try to support IdentityFile w/ no key.pub with CertificateFile

This attempts to make CertificateFile work when a key provided by IdentityFile has no public copy on disk by considering IdentityFile keys that did not load a public half if the filename matches, possibly without .pub/-cert.pub
Comment 7 Damien Miller 2016-12-06 18:49:15 AEDT
Patch for the IdentityFile case has been committed and will be in OpenSSH 7.4.

If anyone could test the 2nd patch for CertificateFile it would be greatly appreciated; the window for the 7.4 release is rapidly closing...
Comment 8 Adam Eijdenberg 2016-12-06 23:29:03 AEDT
Hi Damien,

I've tested your patch against the same tests I included in my original PR (https://github.com/openssh/openssh-portable/pull/53) however I'm seeing the same segfaults that I encountered when I tried to make mine. :)

The problem (I think) is that identity_sign() calls identity_sign_encode() before doing anything, and identity_sign_encode() attempts to dereference id->key->type which is problematic since id->key is NULL.

I'll attach a patch that addresses the segfaults, and separately a patch with the tests that I'd put in the original PR (even though this patch doesn't address all of them).

Cheers, Adam
Comment 9 Adam Eijdenberg 2016-12-06 23:33:00 AEDT
Created attachment 2901 [details]
Allow for id->key being NULL before being passed to identity_sign()

Allow for id->key being NULL before being passed to identity_sign()
Comment 10 Adam Eijdenberg 2016-12-06 23:57:33 AEDT
Created attachment 2902 [details]
Tests

This attachment is a patch to add the tests that I had in the original PR.

To run:

cd regress/
PATH=`pwd`/..:$PATH:. TEST_SHELL=/bin/sh sh test-exec.sh `pwd` cert-file.sh
Comment 11 Adam Eijdenberg 2016-12-07 00:05:20 AEDT
Created attachment 2903 [details]
Load key files for matching cert names

This patch adds to the previous ones to make all the tests actually pass.

It mirrors the logic for loading keys (which check for matching certs filenames, if none are explicitly specified), and looks for keys which match cert filenames if no keys are explicitly specified.

It also disables use of id_rsa (and other defaults) when an explicit CertificateFile is specified (similar to when an IdentityFile is specified) and also when IdentitiesOnly is specified (and that is likely worth discussion as to whether that's the right thing to do or not).
Comment 12 Adam Eijdenberg 2016-12-07 00:09:32 AEDT
Created attachment 2904 [details]
Tests (fixed patch format)
Comment 13 Adam Eijdenberg 2016-12-07 00:11:30 AEDT
Created attachment 2905 [details]
Tests

(third time lucky formatting the attachment correctly, sorry about the spam)
Comment 14 Adam Eijdenberg 2016-12-07 00:14:02 AEDT
Created attachment 2906 [details]
Allow for id->key being NULL before being passed to identity_sign()

(fixed patch attachment format)
Comment 15 Damien Miller 2016-12-09 13:49:59 AEDT
Created attachment 2909 [details]
consolidated and tweaked patches

Thanks indeed for taking the time to write regression tests.

I've merged most of the patches to this one. It does not include your changes to load keys specified via CertificateFile but not IdentityFile - I want to think about those a bit more and I'd like to get the rest of it in before release if possible.
Comment 16 Damien Miller 2016-12-09 13:52:52 AEDT
Comment on attachment 2909 [details]
consolidated and tweaked patches

Note to Darren: the changes in identity_sign(), etc are necessary because we'll now let identities with id->key == NULL in for the case where a certificate doesn't have a .pub file that corresponds to the private file.
Comment 17 Darren Tucker 2016-12-12 13:22:38 AEDT
Comment on attachment 2909 [details]
consolidated and tweaked patches

however I'm not all that familiar with this code, so you might want to also get Markus to take a look
Comment 18 Damien Miller 2016-12-16 14:31:20 AEDT
OpenSSH 7.4 release is closing; punt the bugs to 7.5
Comment 19 Damien Miller 2017-03-12 10:51:39 AEDT
Patch is applied, this will be in OpenSSH 7.5
Comment 20 Damien Miller 2018-04-06 12:26:55 AEST
Close all resolved bugs after release of OpenSSH 7.7.