Add ecdsa-sha2-nistp384 and ecdsa-sha2-nistp521 key authentication #611

Closed
wants to merge 8 commits into
from

Projects

None yet

3 participants

@mchlt
mchlt commented Nov 5, 2015

Added ecdsa-sha2-nistp384 and ecdsa-sha2-nistp521 key authentication support.
Didn't have to change too much. The ecdsa module already supported them, so just needed a little work to get the right ssh parameters into ECDSAKey and having it use ecdsa's features for identifying which key size is being used.
Added bits parameter to ecdsa generate function to make it compatible with RSAKey and DSSKey. Because of this, I could easily add ecdsa to demo_keygen.py.

Tested client login against all key sizes, added ecdsa t demo.py.
Generating ecdsa 256, 384 and 521 size keys works with demo_keygen.py
Existing tests for ecdsa-sha2-nistp256 all still work fine without modification.

@bitprophet
Member

Thanks for this. Seems like it includes the changes from #610 too? If so, you can close #610, I don't actually mind "multi-change" PRs if the 2nd change is just in the demo folder :)

@bitprophet bitprophet added this to the 1.17 milestone Nov 6, 2015
@mchlt mchlt referenced this pull request Nov 6, 2015
Closed

added ecdsa to demo.py #610

@mchlt mchlt fix to GSS key exchange selection being hardcoded elsewhere, and ther…
…efore lacked the new diffie-hellman-group-exchange-sha256 kex
d68fb72
@mchlt
mchlt commented Nov 22, 2015

I've added a fix to transport.py where in the case of GSS, the key exchange algorithm list was replaced further on in the code, which has caused a later update which added diffie-hellman-group-exchange-sha256to be forgotten in that location. Moved the GSS preferred kex list to the top for consistency

mchlt added some commits Nov 22, 2015
@bitprophet
Member

We're planning to merge #394 into master soon and that'll make it hard to merge this as-is. Pretty sure it'd be possible to take the general approach in here and apply it to the updated ecdsakey.py afterwards; I'm going to bump this to a 2.1 milestone with that in mind. (If @mchlt finds time to make this #394-friendly before 2.0 releases, I can make it part of 2.0, doesn't matter a ton to me either way.)

@bitprophet bitprophet modified the milestone: 2.1, 1.17 Apr 24, 2016
@CrazyCasta
Contributor

I'm going to have a version of my pull request done soon. Should I link that here or what?

@CrazyCasta
Contributor
CrazyCasta commented Apr 25, 2016 edited

Ok, I've updated my PR, now #731.

A quick overview of how mine differs from mchlt's:

  • I have some wrappers around my definitions of the various curves, should make it easier to add more in the future if we ever want that.
  • Added tests to test all the new stuff.
  • I didn't bother with the demo stuff. I just want this in by 2.0 so I can stop using my own branch of paramiko.
  • I'm not familiar enough with GSSAPI/Kerberos to know what I'm doing wrt making the gss_ keys, so I just left that out for now.
@bitprophet
Member
bitprophet commented Apr 26, 2016 edited

Close to merging #731. I'd ideally like the demos to get updated too but it can wait for a bugfix or similar update once someone else has the time :)

EDIT: re: the GSS key stuff, from my reading that was simply a cleanup/format change on @mchlt's part and actually orthogonal to the ECDSA functionality update. So again, not a blocker AFAICT.

@bitprophet bitprophet removed this from the 2.1 milestone Apr 26, 2016
@bitprophet bitprophet closed this Apr 26, 2016
@bitprophet bitprophet added a commit that referenced this pull request Apr 29, 2016
@bitprophet bitprophet Changelog entry re #731, re #611 474b566
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment