ssh-keygen-to-Paramiko change breaks third-party tools

[Corey Wright]

Changing ssh key generation from OpenSSH's ssh-keygen to the Paramiko library [1][2] changed (unintentionally?) the ASN.1 encoding format of SSH private keys from DER to BER. (DER is a strict subset of BER, so anything that can read BER can read DER, but not necessarily the other way around.)

Some third-party tools only support DER and this has created at least one issue [3] (specifically because Go's standard library only supports DER).

I have provided Paramiko with a small change that makes its SSH private key output equal to OpenSSH's ssh-keygen output (and presumably DER formatted) [4].

Providing a change to Paramiko is just one method of addressing this backwards-incompatibility and interoperability issue. Should the Paramiko change be accepted the unit test output vectors will need to be changed, but should it not, is a reversion of or modification to Nova acceptable to maintain backwards-compatibility and interoperability?

[1] https://review.openstack.org/157931
[2] http://git.openstack.org/cgit/openstack/nova/commit/?id=3f3f9bf22efd2fb209d2a2fe0246f4857cd2d21a
[3] https://github.com/mitchellh/packer/issues/2526
[4] https://github.com/paramiko/paramiko/pull/572

[Markus Zoeller (markus_z) (mzoeller)]

Note: The backport of this change to stable/kilo got stopped [1].

[1] https://review.openstack.org/#/c/191206/

[Markus Zoeller (markus_z) (mzoeller)]

The review [1] doesn't waste a word about the encoding, so I would assume that this was unintentional. I struggle with myself to decide if the private SSH key of a keypair should be seen as an interface which Nova exposes for consumption for third party tools. From a security point of view, the stricter DER seems to fit better our needs, so I would assume that a rollback of [1] would be justifiable. Unfortunately I'm no expert here and I need another opinion.

[1] https://review.openstack.org/157931

[Corey Wright (coreywright)]

Paramiko update: Once https://github.com/paramiko/paramiko/pull/394 is merged (switching Paramiko from PyCrypto to Cryptography which uses OpenSSL) and a new release is made (planned for September), then the private key output will be DER.

[Stanislaw Pitucha (stanislaw-pitucha)]

I just noticed the update, so not sure if this is still an open problem, but since paramiko still hasn't merged that pull, there's an easy workaround on nova's side. By using pyasn1 (already in global requirements), you nova should be able to just do:

asn1 = pyasn1.codec.ber.decode(berdata)[0]
derdata = pyasn1.codec.der.encode(ans1)

(with proper wrapping/unwrapping for base64 and other of course)

This will be compatible, even after paramiko is fixed.

[Sean Dague (sdague)]

I do feel like private key format is not part of the nova contract. I'm sorry Go tools are so limitted with what they support. The right path is working with upstream paramiko on this.

[Marco Voelz (marco-voelz)]

Paramiko 2.0 is released where the referenced PR is merged. Could we get a bump in paramiko to bring DER encoding back to nova-generated keys? That would be awesome!

[Corey Wright (coreywright)]

@marco-voelz

as the current nova requirements on the "master" branch are "paramiko>=1.16.0" (see http://git.openstack.org/cgit/openstack/nova/tree/requirements.txt?id=5bafd5fba508174f557acfeddbf85de0438c51d7#n24) i believe paramiko 2.0.0 will be pulled in for all future releases (though this also means that unit tests will break / are breaking as they were changed to ber with the original commit (that prompted this "bug" report) and need to change back to the original der.

@all

for the record my PR (see https://github.com/paramiko/paramiko/pull/572) was not merged, but the larger pycrypto-to-cryptography PR (see https://github.com/paramiko/paramiko/pull/394), which obviated my PR, was merged and present in paramiko 2.0.0.

[Corey Wright (coreywright)]

@marco-voelz

nevermind, i'm an idiot: i forgot about the upper-constraints.txt referenced in tox.ini, which currently contains "paramiko===1.16.0" (see https://git.openstack.org/cgit/openstack/requirements/tree/upper-constraints.txt?id=5c1dcc21174c8fdc5e4e0fbde37ae9fb96dcdeb6#n236) and therefor needs a bump to 2.0.0 (as you requested) to resolve this bug.

[Marco Voelz (marco-voelz)]

@coreywright thanks for the hint to the constraints file. I'm not sure on how to proceed now; should I be the one who bumps paramiko in that file? I haven't done a single change in OpenStack, so not sure on how to proceed here.

Also: Could we move this bug from "Won't Fix" (because @sdague wanted to take this upstream to paramiko, which now happened) back to "open"?

[Corey Wright (coreywright)]

The attached patch allows nova ("master" branch as of May 3 / HEAD commit 8185dcb57e55f7579b60040649fcd0588177d714) to pass unit tests with either Paramiko 1.x or 2.x.

Nova will probably not support both Paramiko 1.x and 2.x simultaneously (due to upper-constraints.txt pinning Paramiko to a specific release; currently it is "1.16.0", but that will probably be changed to "2.0.0" or such), so my patch also includes comments on how to remove the Paramiko 1.x work-around and simply and directly use the appropriate and original Paramiko interface (ie revert change If88beeb3983705621fe736995939ac20b2daf1f3 / commit 1fd0f4f69b21cbd20c0eb0e2f8f4506061f4a211).

[Corey Wright (coreywright)]

@marco-voelz

I'm not familiar with how OpenStack manages the upper-constraint.txt file (as that file is doubtfully specific to Nova, but merely used by it).

The best course of action is probably to ping sdague on IRC and ask him the best course of action (as he was the last Nova core reviewer to touch this bug), but any Nova core reviewer should suffice:

1. Open a new bug?

2. Merge the attached patch so that Nova supports both 1.x and 2.x independent of when Paramiko's constraint is upgraded?

3. Prepare a patch that is specific to only Paramiko 2.x (ignoring Paramiko 1.x) and will be merged immediately following the Paramiko constraint being "upgraded" from 1.x to 2.x?

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/314592

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/314639

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/314592
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=c05b338f163e0bafbe564c6c7c593b819f2f2eac
Submitter: Jenkins
Branch: master

commit c05b338f163e0bafbe564c6c7c593b819f2f2eac
Author: Corey Wright <email address hidden>
Date: Tue May 3 23:13:24 2016 -0500

    crypto: Add support for Paramiko 2.x

    Only use PyCrypto/PyCryptodome work-around with Paramiko 1.x and use
    straight-forward Paramiko interface with 2.x.

    TODO: Revert this and PyCrypto/PyCryptodome work-around when Paramiko
    is upgraded to 2.x (ie replace `generate_keys(bits)` call with
    `paramiko.RSAKey.generate(bits)`).

    Change If88beeb3983705621fe736995939ac20b2daf1f3 added a work-around
    for the partially-PyCrypto-compatible PyCryptodome causing Paramiko,
    which has a dependency on PyCrypto, to break. This work-around
    entails implementing Paramiko internals (ie how to generate a key) in
    Nova in a way compatible with both PyCrypto and PyCryptodom.

    This work-around is itself a source of failure with Paramiko 2 which
    has replaced the PyCrypto requirement with the cryptography Python
    package. As Paramiko no longer depends on PyCrypto, Nova doesn't have
    an explicit PyCrypto requirement, and there's no implicit dependency
    on PyCrypto, when Nova tries to import PyCrypto it fails. Even if
    PyCrypto was installed, the work-around would still fail because the
    Paramiko interface that Nova is using as part of the work-around
    changed with the major version change (ie 1.x => 2.x).

    Change-Id: I5d6543e690a3b4495476027fd8a4894ff8c42bf6
    Related-Bug: #1483132

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/314639
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=6b1293fd6f5bcb35f317f36c540f543b1192928c
Submitter: Jenkins
Branch: master

commit 6b1293fd6f5bcb35f317f36c540f543b1192928c
Author: Sean Dague <email address hidden>
Date: Tue May 10 11:39:11 2016 -0400

    Drop paramiko < 2 compat code

    This drops the paramiko < 2 compatibility code so we only need to
    support one major version.

    Depends-On: I2369638282b4fefccd8484a5039fcfa9795069a7
    (global requirements change)

    Change-Id: Ife4df9e64299e1182d77d568d1deed5ec3b608b3
    Closes-Bug: #1483132

[Davanum Srinivas (DIMS) (dims-v)]

This issue was fixed in the openstack/nova 14.0.0.0b1 development milestone.