Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kex: support curve25519-sha256@libssh.org #1258

Closed
wants to merge 5 commits into from

Conversation

@fuhry
Copy link

@fuhry fuhry commented Aug 2, 2018

This commit introduces support for the curve25519-sha256@libssh.org key exchange method, which is the currently (August 2018) recommended method for all modern SSH servers and clients.

Client mode, server mode and unit tests are included and have been tested with the OpenSSH client and server.

Closes #532

@fuhry
Copy link
Author

@fuhry fuhry commented Aug 2, 2018

I've got the build passing on everything except PyPy 5.6.0, and that's not something I have a lot of expertise in. Could use some help from someone who knows PyPy.

The build log suggests that this might actually be caused by an outdated version of OpenSSL. This seems to be confirmed by @reaperhulk's comment on pyca/cryptography#3859, so maybe this is an environment issue.

My implementation is pretty straightforward as it just sits on top of the pyca-cryptography x25519 primitives API, but I'd welcome reviews from anyone with crypto knowledge (particularly more EC knowledge than me) to validate it anyway.

@bitprophet - would appreciate a cursory glance over this when you have a moment. I'd also welcome any suggestions you have for fixing the PyPy 5.6.0 build. This will greatly improve the security of paramiko so it would be really nice to get it included. Thanks!

@fuhry
Copy link
Author

@fuhry fuhry commented Aug 2, 2018

FYI: I've merged #1232 into this PR to attempt to fix the PyPy issues... will back that change out if it doesn't work.

It didn't work, backed out.

@fuhry
Copy link
Author

@fuhry fuhry commented Aug 3, 2018

For anyone who would like an unofficial build with both curve25519 and @edgsousa's EtM patch, I've put those together into a branch here: https://github.com/fuhry/paramiko/tree/curve25519%2Betm

@reaperhulk
Copy link
Contributor

@reaperhulk reaperhulk commented Aug 3, 2018

PyPy has no stable ABI so we can't ship wheels (for cryptography) for it on PyPI. This means that it will build against system OpenSSL, which will be 1.0.1 on trusty (which the Travis builders are currently using). I'd suggest writing something to do capability detection for x25519 and skip the tests if unsupported.

@fuhry fuhry force-pushed the issue-532-curve25519-kex branch 3 times, most recently from 56cb215 to 45e51b9 Aug 3, 2018
@fuhry
Copy link
Author

@fuhry fuhry commented Aug 3, 2018

@reaperhulk Thanks for the advice - I implemented your suggestion and this is working nicely.

Builds are now passing.

This commit introduces support for the curve25519-sha256@libssh.org key
exchange method, which is the currently recommended method for all
modern SSH servers and clients.

Client mode, server mode and unit tests are included.

Logic is included to conditionally enable this support based on the
state of support for x25519 in the underlying pyca-cryptography and
openssl libraries. If support for x25519 is not available, this kex
method and the associated unit tests will not be available in paramiko.
@fuhry fuhry force-pushed the issue-532-curve25519-kex branch from 45e51b9 to 482a3fe Aug 3, 2018
@bitprophet bitprophet added this to the 2.5.0 milestone Aug 3, 2018
@mboersma
Copy link

@mboersma mboersma commented Aug 31, 2018

Could this get merged soon? With a recent Ubuntu security update, ssh access with paramiko is broken for all new VMs our service is creating in Azure. This would fix it.

Copy link

@Conan-Kudo Conan-Kudo left a comment

This PR requires raising the minimum version of cryptography to 2.0, since that's the version that introduces x25519 support.

@fuhry
Copy link
Author

@fuhry fuhry commented Jan 3, 2019

Is this still on the table for being merged into the next paramiko release?

@crawfxrd
Copy link

@crawfxrd crawfxrd commented Mar 8, 2019

Using this produces a deprecation notice from cryptography.

/root/.local/share/virtualenvs/system-2ifbupYJ/lib/python3.6/site-packages/cryptography/hazmat/backends/openssl/x25519.py:35: CryptographyDeprecationWarning: public_bytes now requires encoding and format arguments. Support for calling without arguments will be removed in cryptography 2.7

- test/text_kex: X25519._from_private_bytes -> from_private_bytes
- kex_curve25519: pass encoding and format parameters to public_bytes()
- update python-cryptography dependency to >= 2.5
@fuhry fuhry force-pushed the issue-532-curve25519-kex branch from 50b67ab to f412c21 Mar 11, 2019
@fuhry
Copy link
Author

@fuhry fuhry commented Mar 11, 2019

@crawfxrd Latest commit should resolve that deprecation notice.

@Conan-Kudo
Copy link

@Conan-Kudo Conan-Kudo commented Mar 11, 2019

This latest change addresses remaining issue I had noted earlier.

@mikeln
Copy link

@mikeln mikeln commented Apr 25, 2019

Really could use this PR merged. Dealing with a number of servers that only support this kex algorithm.

@sylr
Copy link

@sylr sylr commented May 16, 2019

I'd also really need this PR to be merged if the implementation is all right.

@sylr
Copy link

@sylr sylr commented May 16, 2019

@reaperhulk
Copy link
Contributor

@reaperhulk reaperhulk commented May 18, 2019

I would love for this PR to get merged, but have no access to this repository.

@swills
Copy link

@swills swills commented May 23, 2019

Another vote to merge this, and hopefully include in the next release.

@fuhry
Copy link
Author

@fuhry fuhry commented May 28, 2019

Note - @ploxiln had some good suggestions in the paramiko-ng PR which has since been merged (ploxiln#18) and improved somewhat (ploxiln#19). I'm incorporating those changes into this PR.

@fuhry
Copy link
Author

@fuhry fuhry commented May 29, 2019

@bitprophet @reaperhulk This is once again ready to go; please let me know if you would like it squashed (5 commits currently).

bitprophet added a commit that referenced this issue Jun 8, 2019
Needed to change the test/mock pattern re: ensuring static private key value
to be compared against, but seems to work out.
@bitprophet bitprophet closed this in a606567 Jun 8, 2019
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 8, 2019

I Frankenstein'd Alex's slightly tidier implementation with your tests (&, as it turns out, your on-spec use of the otherwise undocumented hash_algo attribute! 😬✌️) so this is good to go now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

10 participants