Add tentative support for ECDSA #152

Merged
merged 3 commits into from Sep 28, 2013

Projects

None yet

5 participants

@glasserc
Contributor

This adds (incomplete) support for ECDSA using the library at https://github.com/warner/python-ecdsa . Key negotiation still needs some work -- if the server offers us RSA and ECDSA keys, we ask for the RSA key, without any consideration of what we actually have in the known_hosts file. Still, it worked for me when I turned off RSA and DSA keys in my sshd_config.

@glasserc
Contributor

I forgot to mention that this helps with, but doesn't entirely resolve, #88 and #67.

glasserc added some commits Mar 24, 2013
@glasserc glasserc Introduce ECDSA
This just adds tests; hooking this up with paramiko comes in the next
commit.
632129c
@glasserc glasserc Hook up ECDSA to hostkeys
More sophisticated key negotiation is still necessary in the case
where we have an ECDSA key for the server and it offers us both RSA
and ECDSA. In this case, we will pick RSA and fail because we don't
have it. Instead, we should pick ECDSA. Still, this works if you tell
your server to only offer ECDSA keys :)
ebdbfae
@bitprophet
Member

Thanks for this!

I agree we should probably change the logic such that if only ECDSA host keys are found and the server is offering it, we use it. I assume similar logic is in place for RSA/DSA but haven't checked yet - certainly we should follow existing behavior if possible.

We also need a changelog entry in NEWS under the 1.11 section :) including a bolded callout about the fact that there is now a new dependency.


I'm strained for time & have to triage other tickets, but if @glasserc or @ctheune can tackle the remaining bits (and if possible give me some quick examples of a known target OS that uses ECDSA by default so I can spin up a Rackspace Cloud or EC2 instance to test against) I will gladly merge.

@glasserc
Contributor

Key negotiation is done in transport.py. I think "existing behavior" is pretty naive right now -- line 1790 just says "self.host_key_type = agreed_keys[0]" without looking at what host keys we have for the server. I don't really know how SSH key negotiation works, so maybe I'm misreading it or not using it correctly, but it seemed like changing it was outside the scope of this diff.

Ubuntu 12.10 is what I'm running on my laptop and it seems to have generated an ECDSA key by default.

It looks like you merged branch 1.10. Should I rebase against current master, or what?

I'll happily add the NEWS entry, hopefully tonight.

@glasserc
Contributor

Here's a NEWS entry. I didn't rebase against anything.

@mgorny
mgorny commented Aug 22, 2013

Ping. Is anything happening here? We need that feature bad.

@ctheune
ctheune commented Aug 25, 2013

I've gotta admit that I pulled paramiko out of batou and started using execnet's remote control model. So, I was interested in this before, but I don't have any stakes in this any longer.

@bitprophet
Member

@mgorny Gah, sorry for dropping this. Dayjob has been slaying me. Will poke at this ASAP and merge. Spinning up a Quetzal VM now.

@bitprophet
Member

@ctheune Curious how that worked out for you, by the way - I did see that batou got a release (or at least a writeup; something crossed my desk in the last couple weeks) so congrats on that!

@bitprophet
Member

Sweet deal, confirmed working:

  • Commented out RSA/DSA host keys in sshd config, restarted sshd
  • Even my system's builtin ssh can't talk to the server now; neither can Fabric using Paramiko master
  • Switch to this branch, ecdsa module missing
  • pip install ecdsa
  • Fabric connects, works apparently fine.

I'll need to modify the ecdsa import so it gives a useful message - would rather not make it a required dependency which is another approach. Will do that and merge. Thanks again! & sorry for the long delay.

@bitprophet
Member

Ah, I see the PR does actually add it to setup.py (I run with a pip -e install so don't usually rerun that step.) Hrm. Would rather not add a new dependency for what is still a small subset of users (given, as I saw, a default recent Ubuntu box still provides RSA and DSA keys). Think I will still go ahead w/ the optional import.

@infowolfe

Considering ecdsa's the only preferred key I give out nowadays, any chance we could be given the directions to "force" it?

@bitprophet
Member

@infowolfe What exactly do you mean by "force"? Force Paramiko to ignore supplied DSA/RSA keys in favor of an ECDSA one (and bail if one isn't found)?

@bitprophet
Member

Looks like making one of the ECDSAKey imports lazy is going to take more work than just shunting the import itself around. Going to poke a bit to see just how widespread ECDSA really is/is becoming; given the lib is pure-Python it may in fact be smarter to roll with it & make it required.

@infowolfe

@bitprophet any chance you'd be willing to post your dependency patch in the meanwhile? ecdsa isn't really an optional for me, so I was asking if there was a way that (if you made it an optional import) I could specify that it shouldn't be optional in my deployments. I'm kinda stuck between a rock and a hard place. I've been working on an xchat-python alternative to a bash script that I used for years to output ssh to my irc client. Sadly, due to the $#%& containers dictated by the mac app store, I can't access anything outside the container, meaning i must bring my keys into the container in order to use them... and i'm prevented from being able to do things like use my ssh agent. Being that most of the machines i'd access via my paramiko+xchat-python script use ecdsa keys now, I wouldn't be able to use paramiko for this purpose without ecdsa support.

@bitprophet
Member

Oh I didn't mean I was going to spend a long time doing that! I still want to release tonight either way. Did just poke around on Google a bit and not finding a slew of evidence that it's crazy widespread. So I'll spend a couple minutes seeing if I can lazily import it in that one trouble spot, without going overboard. If that fails I will probably just make it required as per above.

@bitprophet
Member

Yea, unfortunately Transport._get_keys and ._set_keys are implemented such that trying to make the new ECDSA key/value pair lazy just doesn't make sense. It could be done with some janky metaprogramming BS but at this point I think it probably is worth the new dependency.

@bitprophet bitprophet merged commit a733c42 into paramiko:master Sep 28, 2013
@bitprophet
Member

@infowolfe If having support out (I will release Paramiko 1.12 to PyPI shortly) isn't sufficient for your needs, please let me know & we can make a new ticket (if one doesn't already exist along the same lines.) Thanks!

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