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

Fixes #325 -- add support for Ed25519 keys #972

Merged
merged 14 commits into from Jun 6, 2017

Conversation

Projects
None yet
8 participants
@alex
Contributor

alex commented May 27, 2017

@alex

This comment has been minimized.

Contributor

alex commented May 27, 2017

I haven't done any integration testing, besides the tests themselves. Can you suggest where and how to add more tests for this?

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented May 27, 2017

looks like a unit test could go in tests/test_pkey.py and an integration test could go in tests/test_client.py

keyfiles.append((keytype, full_path))
# look in ~/ssh/ for windows users:
full_path = os.path.expanduser("~/ssh/id_%s" % path)

This comment has been minimized.

@ploxiln

ploxiln May 27, 2017

Contributor

you could parameterize a bit more e.g.

for ssh_dir in ["~/.ssh", "~/ssh"]:
    full_path = os.path.expanduser("%s/id_%s" % (ssh_dir, path))
    ...
@mithrandi

The file format stuff all looks correct to me.

def unpad(data):
# At the moment, this is only used for unpadding private keys on disk, and
# only unencrypted ones at that. In the future, if either of those changes,
# this really ought to be made constant time.

This comment has been minimized.

@mithrandi

mithrandi May 27, 2017

I don't think this comment is accurate, unpad seems to be used for encrypted keys too. Although, I'm not sure how anyone able to observe the timing of this code would not have access to the ciphertext as well, which would appear to render a timing attack irrelevant.

from paramiko.ssh_exception import SSHException, PasswordRequiredException
OPENSSH_AUTH_MAGIC = b"openssh-key-v1\x00"

This comment has been minimized.

@mithrandi

mithrandi May 27, 2017

Minor nitpick: You seem to flipflop between 'strings' and "strings" in this code, maybe stick to one? 😼

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented May 27, 2017

very nice 👍

@bitprophet

This comment has been minimized.

Member

bitprophet commented May 27, 2017

OMG, I love you @alex! Will take a crack at testing & merging this early next week.

setup.py Outdated
'cryptography>=1.1',
'pynacl',

This comment has been minimized.

@bitprophet

bitprophet Jun 2, 2017

Member

Would love at least some pinning here; latest pynacl seems to be 1.1.2. What's good - pynacl>=1.0, >=1.1, other?

This comment has been minimized.

@alex

alex Jun 3, 2017

Contributor

@reaperhulk opinions on an appropriate minimum version?

This comment has been minimized.

@reaperhulk

reaperhulk Jun 3, 2017

Contributor

=1.0 (or 1.0.1) would be fine. That's the cffi 1.0 out-of-line version.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 2, 2017

Testing this out...

  • Localhost:
    • OS X Sierra 10.12.5
    • pyenv-compiled Python 2.7.12
    • stock OS X OpenSSH ssh 7.4p1, LibreSSL 2.5.0 (...huh.)
  • Target:
    • Debian 8
    • stock OpenSSH sshd (6.7p1)
    • which appears to have 3x host keys installed and enabled (in order: ED25519, RSA, ECDSA)

Sanity check with ssh, Fabric 2 and Paramiko master (massaging my known_hosts in between, as necessary, to avoid complaints):

  • With all 3 host keys enabled:
    • ssh/sshd agree on the ED25519 key, everything works great
    • Paramiko/sshd agree on the RSA key, everything works great
  • With the RSA and ECDSA keys disabled, leaving only the ED25519 key:
    • ssh/sshd still happy as clams
    • Paramiko, as expected, goes kaput because no host key could be agreed upon.

Unfortunately, when I switch from Paramiko master to an integration branch based on this PR (still with only the ED25519 key enabled on the target) Paramiko dies with Signature verification (ssh-ed25519) failed:

starting thread (client mode): 0x7ab8990L
Local version/idstring: SSH-2.0-paramiko_2.1.2
Remote version/idstring: SSH-2.0-OpenSSH_6.7p1 Debian-5+deb8u1
Connected (version 2.0, client OpenSSH_6.7p1)
kex algos:[u'curve25519-sha256@libssh.org', u'ecdh-sha2-nistp521', u'ecdh-sha2-nistp384', u'ecdh-sha2-nistp256', u'diffie-hellman-group-exchange-sha256'] server key:[u'ssh-ed25519'] client encrypt:[u'chacha20-poly1305@openssh.com', u'aes256-gcm@openssh.com', u'aes128-gcm@openssh.com', u'aes256-ctr', u'aes192-ctr', u'aes128-ctr'] server encrypt:[u'chacha20-poly1305@openssh.com', u'aes256-gcm@openssh.com', u'aes128-gcm@openssh.com', u'aes256-ctr', u'aes192-ctr', u'aes128-ctr'] client mac:[u'hmac-sha2-512-etm@openssh.com', u'hmac-sha2-256-etm@openssh.com', u'umac-128-etm@openssh.com', u'hmac-sha2-512', u'hmac-sha2-256', u'umac-128@openssh.com'] server mac:[u'hmac-sha2-512-etm@openssh.com', u'hmac-sha2-256-etm@openssh.com', u'umac-128-etm@openssh.com', u'hmac-sha2-512', u'hmac-sha2-256', u'umac-128@openssh.com'] client compress:[u'none', u'zlib@openssh.com'] server compress:[u'none', u'zlib@openssh.com'] client lang:[u''] server lang:[u''] kex follows?False
Kex agreed: diffie-hellman-group-exchange-sha256
Cipher agreed: aes128-ctr
MAC agreed: hmac-sha2-256
Compression agreed: none
Got server p (2048 bits)
Exception: Signature verification (ssh-ed25519) failed.
Traceback (most recent call last):
  File "/Users/jforcier/Code/oss/paramiko/paramiko/transport.py", line 1851, in run
    self.kex_engine.parse_next(ptype, m)
  File "/Users/jforcier/Code/oss/paramiko/paramiko/kex_gex.py", line 91, in parse_next
    return self._parse_kexdh_gex_reply(m)
  File "/Users/jforcier/Code/oss/paramiko/paramiko/kex_gex.py", line 260, in _parse_kexdh_gex_reply
    self.transport._verify_key(host_key, sig)
  File "/Users/jforcier/Code/oss/paramiko/paramiko/transport.py", line 1697, in _verify_key
    raise SSHException('Signature verification (%s) failed.' % self.host_key_type) # noqa
SSHException: Signature verification (ssh-ed25519) failed.

When I dig, it's due to a BadSignatureException, whose traceback is thusly:

Traceback (most recent call last):
  File "/Users/jforcier/Code/oss/paramiko/paramiko/transport.py", line 1851, in run
    self.kex_engine.parse_next(ptype, m)
  File "/Users/jforcier/Code/oss/paramiko/paramiko/kex_gex.py", line 91, in parse_next
    return self._parse_kexdh_gex_reply(m)
  File "/Users/jforcier/Code/oss/paramiko/paramiko/kex_gex.py", line 260, in _parse_kexdh_gex_reply
    self.transport._verify_key(host_key, sig)
  File "/Users/jforcier/Code/oss/paramiko/paramiko/transport.py", line 1696, in _verify_key
    if not key.verify_ssh_sig(self.H, Message(sig)):
  File "/Users/jforcier/Code/oss/paramiko/paramiko/ed25519key.py", line 190, in verify_ssh_sig
    self._verifying_key.verify(data, msg.get_binary())
  File "/Users/jforcier/.virtualenvs/fabric2/lib/python2.7/site-packages/nacl/signing.py", line 115, in verify
    return nacl.bindings.crypto_sign_open(smessage, self._key)
  File "/Users/jforcier/.virtualenvs/fabric2/lib/python2.7/site-packages/nacl/bindings/crypto_sign.py", line 109, in crypto_sign_open
    raise exc.BadSignatureError("Signature was forged or corrupt")

I haven't looked any deeper yet; key exchange stuff isn't something I've had to muck with much before. Left to my own devices I'll dive in and educate myself and see if something obvious jumps out, otherwise, direction is welcome.

@alex

This comment has been minimized.

Contributor

alex commented Jun 3, 2017

Grumble, this is what I get for not doing the integration testing myself. Will try to debug this weekend.

@alex

This comment has been minimized.

Contributor

alex commented Jun 3, 2017

@bitprophet Ok, should be good now.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 6, 2017

Thanks! Glad it wasn't me doing something silly. Confirmed my test case works once I get the recent commits

@alex

This comment has been minimized.

Contributor

alex commented Jun 6, 2017

Nope, I was the one doing something silly.

@bitprophet bitprophet merged commit 0ddb28f into paramiko:master Jun 6, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

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

@alex alex deleted the alex:ed25519-support branch Jun 6, 2017

@007

This comment has been minimized.

007 commented on b03ebb2 Jun 8, 2017

👍 many times over!

@zhangysh1995

This comment has been minimized.

zhangysh1995 commented Jul 28, 2017

In version 2.2.1, I still have to manually pip3 install pynacl

The error:

root@194c224d0b6a:~/scripts# python3 tools/ssh.py 
Traceback (most recent call last):
  File "tools/ssh.py", line 2, in <module>
    from paramiko import SSHClient
  File "/usr/local/lib/python3.4/dist-packages/paramiko/__init__.py", line 31, in <module>
    from paramiko.transport import SecurityOptions, Transport
  File "/usr/local/lib/python3.4/dist-packages/paramiko/transport.py", line 57, in <module>
    from paramiko.ed25519key import Ed25519Key
  File "/usr/local/lib/python3.4/dist-packages/paramiko/ed25519key.py", line 22, in <module>
    import nacl.signing
ImportError: No module named 'nacl'

Openssl information:

root@194c224d0b6a:~/scripts# sudo apt show openssl   
Package: openssl
Priority: standard
Section: utils
Installed-Size: 929 kB
Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com>
Original-Maintainer: Debian OpenSSL Team <pkg-openssl-devel@lists.alioth.debian.org>
Version: 1.0.1f-1ubuntu2.22
Depends: libc6 (>= 2.15), libssl1.0.0 (>= 1.0.1)
Suggests: ca-certificates
Download-Size: 489 kB
Bugs: https://bugs.launchpad.net/ubuntu/+filebug
Origin: Ubuntu
Supported: 5y
Task: standard, kubuntu-active, kubuntu-active, mythbuntu-frontend, mythbuntu-frontend, mythbuntu-desktop, mythbuntu-backend-slave, mythbuntu-backend-slave, mythbuntu-backend-master, mythbuntu-backend-master
APT-Manual-Installed: yes
APT-Sources: http://mirrors.163.com/ubuntu/ trusty-security/main amd64 Packages
Description: Secure Sockets Layer toolkit - cryptographic utility

N: There is 1 additional record. Please use the '-a' switch to see it
@ploxiln

This comment has been minimized.

Contributor

ploxiln commented Jul 28, 2017

"pynacl" is an explicit requirement in setup.py - it is up to the method you used to install paramiko 2.2.1 to install pynacl at the same time. If you didn't use pip, you should probably use pip.

@vaygr

This comment has been minimized.

vaygr commented Oct 21, 2017

Another day I was wondering if there's a way to reuse cryptography built-in support for this instead of introducing a train of libsodium dependencies, unless I'm missing something very obvious here.

Can't paramiko (and thus, something else, like Ansible) rely on the one crypto library and/or its backend, or at least have another one as optional for specific key support?

@alex

This comment has been minimized.

Contributor

alex commented Oct 21, 2017

@vaygr

This comment has been minimized.

vaygr commented Oct 21, 2017

Ah, I see, it's my misunderstanding then.

So it's more like a temporary workaround than a solid permanent solution, waiting until the unified part "is ready".

Looking forward to it!

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