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

Support for ECDH Key Exchange #951

Merged
merged 4 commits into from Jun 6, 2017

Conversation

Projects
None yet
5 participants
@shashankv02
Contributor

shashankv02 commented Apr 30, 2017

Paramiko doesn't support any of the ecdh-sha2-* family of elliptic curve based DH key exchange algorithms. Some hardened servers and the few servers operating in FIPS mode only support ecdh key exchange algorithms. FIPS standard is going to remove dh-group14-sha1 from approved algorithms soon which will break lot of existing infrastructure using paramiko. The implementation follows RFC 5656.

I have tested this with servers only supporting ECDH kex and also client only supporting ECDH kex with paramiko as server.

@coveralls

This comment has been minimized.

coveralls commented Apr 30, 2017

Coverage Status

Changes Unknown when pulling c46e31e on shashankv02:ecdh_key_exchange into ** on paramiko:master**.

@coveralls

This comment has been minimized.

coveralls commented Apr 30, 2017

Coverage Status

Changes Unknown when pulling a785ad0 on shashankv02:ecdh_key_exchange into ** on paramiko:master**.

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented Apr 30, 2017

Nice!

Minor question: why not include KexNistp384 and KexNistp521 in the same source file as KexNistp256? At 4 lines for each additional variant, my humble opinion is that they could all go in a single file named something like "kex_ecdh_nistp.py"

@shashankv02

This comment has been minimized.

Contributor

shashankv02 commented Apr 30, 2017

Hi @ploxiln,

Yup. That's how I have in my local copy but I refactored it into different files before pushing to follow the existing convention ~ see kex_group1.py and kex_group14.py.

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented Apr 30, 2017

True, but then again, see kex_gex.py. Probably best to wait for the maintainer's opinion.

@bitprophet

This comment has been minimized.

Member

bitprophet commented May 1, 2017

When we're talking near single digit number of lines in each chunk, for chunks of code which are strongly thematically related, I'm definitely all for minimizing number of files. So I agree with @ploxiln's original assessment.

Sadly, this codebase is nearly 15 years old now (and my personal involvement [I'm a picky bastard] has only been the last ~5, and that at times minimal) so...following existing style doesn't count for quite as much as in some other projects 😁

Thanks for this!

@bitprophet bitprophet added this to the 2.2 milestone May 1, 2017

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented May 4, 2017

Looks good. (One sub-test spuriously failed, and another hit some travis-ci scheduling bug and never started, but I'm pretty sure those events are not related to this branch.)

@shashankv02

This comment has been minimized.

Contributor

shashankv02 commented May 5, 2017

@ploxiln @bitprophet Any way to restart the CI build without pushing a commit?

@SethMichaelLarson

This comment has been minimized.

SethMichaelLarson commented May 5, 2017

@shashankv02 Close and reopen.

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented May 5, 2017

you can trivially amend your commit message with git commit --amend, then force-push it

@shashankv02 shashankv02 closed this May 6, 2017

@shashankv02 shashankv02 reopened this May 6, 2017

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented May 6, 2017

I tested this - modifying _preferred_kex was the most convenient way for me to cause these kex algorithms to be chosen in my setup. It seems to work :)

'gss-gex-sha1-toWM5Slw5Ew8Mqkay+al2g==': KexGSSGex,
'ecdh-sha2-nistp256': KexNistp256,
'ecdh-sha2-nistp384': KexNistp384,
'ecdh-sha2-nistp521': KexNistp521

This comment has been minimized.

@ploxiln

ploxiln May 6, 2017

Contributor

minor nit: it's a bit better to include the comma after the last item, to reduce the diff when adding another item later

This comment has been minimized.

@shashankv02

shashankv02 May 6, 2017

Contributor

Thanks for reviewing. Good idea about the trailing comma. I'll incorporate all the review suggestions in a single go if there are more. :)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 6, 2017

Integration test:

  • Debian 8 target (OS X workstation), stock opensshd, KexAlgorithms set to hardened default of <ed25519>,<ecdh-sha2-family>,<dh-group-exchange-sha256>
  • OpenSSH client selects ed25519, is happy, by default.
  • Stripping ed25519 out of the server's KexAlgorithms results in agreement on ecdh-sha2-nistp521.
    • This is due to my local ~/.ssh/config; the default client KexAlgorithms has the keys ordered smallest-to-largest, so ecdh-sha2-nistp256 would normally have won.
  • Master paramiko, as expected, agrees upon diffie-hellman-group-exchange-sha256
  • Integration branch (this + latest master) now agrees upon ecdh-sha2-nistp256
@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 6, 2017

Merged & gussied up w/ changelog, trailing comma, etc. Thanks @shashankv02 !

@ploxiln I figure like with the changes from yesterday, we should strongly consider moving the new kex algos above the older DH ones, so that we match OpenSSH's default ordering - thoughts? As with the host key changes, since this doesn't impact user key selection it feels relatively safe, compatibility wise. (Especially as it is going into 2.2 and not 2.0/2.1.)

@bitprophet bitprophet merged commit 8eb2775 into paramiko:master Jun 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

bitprophet added a commit that referenced this pull request Jun 6, 2017

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 6, 2017

(For reference, #983's tl;dr is "yes, we should move the new algorithms above the old ones", and we did, and then everybody rejoiced)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment