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

GSS-API / SSPI authenticated Diffie-Hellman Key Exchange and user authentication with Python 3 support #267

Merged
merged 20 commits into from Sep 9, 2014

Conversation

@SebastianDeiss
Copy link
Contributor

@SebastianDeiss SebastianDeiss commented Feb 11, 2014

Add Python 3 support for the GSS-API / SSPI authenticated Diffie-Hellman
Key Exchange and user authentication.
This patch supersedes pull request #250.

Sebastian Deiss added 2 commits Feb 11, 2014
authentication with Python 3 support

Add Python 3 support for the GSS-API / SSPI authenticated Diffie-Hellman
Key Exchange and user authentication. This patch supersedes pull request
#250.
@lndbrg
Copy link
Contributor

@lndbrg lndbrg commented Feb 11, 2014

Wow, this is really awesome!

@SebastianDeiss
Copy link
Contributor Author

@SebastianDeiss SebastianDeiss commented Feb 11, 2014

Thanks.
Unfortunately, the python-gssapi module (https://github.com/sigmaris/python-gssapi) is not able to run on py3 yet.
So currently you can't run paramiko with kerberos support on py3 on unix systems.

UPDATE:
Since version 0.5.1 python-gssapi runs on py3, but building the module is still a bit tricky. See https://github.com/sigmaris/python-gssapi/blob/master/README.md
This patch is now complete and ready to merge.

Sebastian Deiss added 3 commits Feb 17, 2014
Previously an attribute error occurred or a SSHException was thrown if
the GSS-API authentication failed.
If GSS-API authentication fails now or the remote host does not support
GSS-API, paramiko tries other authentication methods.
If an GSS-API / SSPI error occurs you get a status code and an error
message, but you may also want the name of the remote host.
That's what this patch adds.
Previously GSS-API authentication (gssapi-with-mic) failed if the server
sent a userauth-banner.
@lndbrg lndbrg added the Reviewed label Mar 4, 2014
Sebastian Deiss added 5 commits Mar 17, 2014
Conflicts:
	.gitignore
	README
	demos/demo_simple.py
	dev-requirements.txt
	paramiko/__init__.py
	paramiko/_winapi.py
	paramiko/agent.py
	paramiko/auth_handler.py
	paramiko/ber.py
	paramiko/buffered_pipe.py
	paramiko/channel.py
	paramiko/client.py
	paramiko/common.py
	paramiko/dsskey.py
	paramiko/ecdsakey.py
	paramiko/file.py
	paramiko/hostkeys.py
	paramiko/kex_gex.py
	paramiko/kex_group1.py
	paramiko/message.py
	paramiko/packet.py
	paramiko/pkey.py
	paramiko/primes.py
	paramiko/proxy.py
	paramiko/py3compat.py
	paramiko/server.py
	paramiko/sftp_client.py
	paramiko/transport.py
	paramiko/util.py
	paramiko/win_pageant.py
	setup.py
	sites/shared_conf.py
	sites/www/changelog.rst
	sites/www/conf.py
	sites/www/index.rst
	sites/www/installing.rst
	test.py
	tests/loop.py
	tests/stub_sftp.py
	tests/test_auth.py
	tests/test_client.py
	tests/test_file.py
	tests/test_hostkeys.py
	tests/test_kex.py
	tests/test_message.py
	tests/test_packetizer.py
	tests/test_pkey.py
	tests/test_sftp.py
	tests/test_sftp_big.py
	tests/test_transport.py
	tests/test_util.py
@bitprophet bitprophet mentioned this pull request Apr 2, 2014
akruis and others added 3 commits Apr 7, 2014
This change fixes a paramiko crash in case the package 'gssapi' is installed but can't load  libgssapi_krb5.so.
Conflicts:
	dev-requirements.txt
Conflicts:
	sites/www/changelog.rst
	test.py
@lukecyca
Copy link

@lukecyca lukecyca commented Apr 28, 2014

I tried this out today and it worked perfectly. I connected from client 1 to client 2...

Client 1: Scientific Linux 6.1, Python 2.7.3 (from source), MIT Kerberos 1.10.3 (distro)
Client 2: Scientific Linux 6.3, Python 2.6.6 (distro), MIT Kerberos 1.10.3 (distro)
Kerberos Server: Heimdal 1.5.1apple1, OpenDirectory running on a Mac Mini Server with OS X 10.9.2

@wbashir
Copy link

@wbashir wbashir commented Apr 29, 2014

Please merge this !

@xp-lol-er
Copy link

@xp-lol-er xp-lol-er commented Apr 29, 2014

Ops are breathing down my neck to change fabric for capistrano!!!! the only reason they have is the kerberos support. Please merge it quickly :).

# client mode
mic_status = self._gss_ctxt.verify_mic(self._session_id,
mic_token)
return mic_status

This comment has been minimized.

@sigmaris

sigmaris May 2, 2014
Contributor

The return value from gssapi.AcceptContext.verify_mic is actually the qop_state value returned from the underlying gss_verify_mic call, it indicates the cryptographic algorithm used in generating the MIC.

Checking that return value isn't the correct thing to do to check if the MIC verify succeeded or failed; this code needs to check if verify_mic raises GSSException, which indicates that the verification failed. Sorry if this wasn't clear in the python-gssapi docs.

In the next version of python-gssapi I'm working on at the moment, there'll be subclasses of GSSException for each error status defined in the GSSAPI, so you can do:

try:
    gss_ctxt.verify_mic(message, mic)
except gssapi.BadSignature:
    # The MIC does not match, indicating the message may have been altered

... and catch the specific exception that it'll throw if the MIC verification fails. But for current versions, it'll just have to catch any gssapi.GSSException.

This comment has been minimized.

@SebastianDeiss

SebastianDeiss May 6, 2014
Author Contributor

Thanks for letting me know.
I will fix that when the next version of python-gssapi is available. In the meantime, the GSSException thrown by gssapi.AcceptContext.verify_mic will also lead to an auth failure.

This comment has been minimized.

@sigmaris

sigmaris May 6, 2014
Contributor

I've released python-gssapi version 0.6.1 a couple of days ago, which contains the new subclasses of GSSException for BadSignature, etc. So I'd recommend requiring at least that version. That'll fix the problems with installation on Py3 as well, as that was fixed in 0.6.0.

This comment has been minimized.

@SebastianDeiss

SebastianDeiss Jun 11, 2014
Author Contributor

I've created a new branch to fix that issue, but i haven't had much time to test it.
See https://github.com/SebastianDeiss/paramiko/tree/python-gssapi-0.6.1

akruis and others added 2 commits May 27, 2014
Conflicts:
	dev-requirements.txt
	sites/www/changelog.rst
@coveralls
Copy link

@coveralls coveralls commented May 28, 2014

Coverage Status

Coverage decreased (-6.2%) when pulling 0be471c on SebastianDeiss:gssapi-py3-support into e811e71 on paramiko:master.

@wbashir
Copy link

@wbashir wbashir commented Jun 9, 2014

Any updates to this ?

@jjwon0
Copy link

@jjwon0 jjwon0 commented Jun 19, 2014

@lukecyca you were able to connect using @SebastianDeiss 's branch, right? can you show what the code you used to instantiate the connection was? i'm getting a bizarre error and am a little confused — SebastianDeiss#6

@lukecyca
Copy link

@lukecyca lukecyca commented Jun 19, 2014

@jjwon: Yes, I made a file in demos/ with the following:

from paramiko.client import SSHClient
import interactive

HOSTNAME = 'myserver.example.com'

client = SSHClient()

# The key for my server is in the system-wide known-hosts file...
client.load_system_host_keys('/etc/ssh/ssh_known_hosts')

client.connect(HOSTNAME, gss_host=HOSTNAME, gss_auth=True, gss_kex=True)
chan = client.invoke_shell()

print(repr(client.get_transport()))
print('*** Here we go!\n')
interactive.interactive_shell(chan)
chan.close()
client.close()

You can describe your issue here and maybe we can help.

@jjwon0
Copy link

@jjwon0 jjwon0 commented Jun 19, 2014

Sure, here's the output of trying to run that demo. It's the same error in the issue I posted. I installed the latest branch of paramiko with gssapi support that I found. The log after running your demo can be found here.

$ ./test.py              
No handlers could be found for logger "paramiko.transport"
Traceback (most recent call last):
  File "./process_args.py", line 203, in <module>
    main()
  File "./process_args.py", line 192, in main
    client.connect(HOSTNAME, gss_host=HOSTNAME, gss_auth=True, gss_kex=True)
  File "build/bdist.macosx-10.9-intel/egg/paramiko/client.py", line 253, in connect
  File "build/bdist.macosx-10.9-intel/egg/paramiko/transport.py", line 380, in start_client
AttributeError: 'Transport' object has no attribute 'rng'

According to the log, the AttributeError comes from transport.py when KexGSSGroup tries to parse a group packet and calls self._generate_x(), which looks for a self.transport.rng. Not sure if this is because Transport used to have an rng attribute?

Edits: removed my confused comments, added log, added what I found.

@coveralls
Copy link

@coveralls coveralls commented Jul 15, 2014

Coverage Status

Coverage decreased (-6.3%) when pulling 739b409 on SebastianDeiss:gssapi-py3-support into e811e71 on paramiko:master.

1 similar comment
@coveralls
Copy link

@coveralls coveralls commented Jul 15, 2014

Coverage Status

Coverage decreased (-6.3%) when pulling 739b409 on SebastianDeiss:gssapi-py3-support into e811e71 on paramiko:master.

@@ -92,7 +95,7 @@ class Transport (threading.Thread):
'aes256-cbc', '3des-cbc', 'arcfour128', 'arcfour256')
_preferred_macs = ('hmac-sha1', 'hmac-md5', 'hmac-sha1-96', 'hmac-md5-96')
_preferred_keys = ('ssh-rsa', 'ssh-dss', 'ecdsa-sha2-nistp256')
_preferred_kex = ('diffie-hellman-group1-sha1', 'diffie-hellman-group-exchange-sha1')
_preferred_kex = ( 'diffie-hellman-group1-sha1', 'diffie-hellman-group14-sha1', 'diffie-hellman-group-exchange-sha1' )

This comment has been minimized.

@lndbrg

lndbrg Aug 21, 2014
Contributor

When merging this we should probably reorder so we have diffie-hellman-group14-sha1 as preferred kex. The default one now: diffie-hellman-group1-sha1 is no longer considered secure. It is required to support it according to the rfc, but the order is not stated. I think we should reorder them as such:

    _preferred_kex = ( 'diffie-hellman-group14-sha1', 'diffie-hellman-group-exchange-sha1' , 'diffie-hellman-group1-sha1')

This comment has been minimized.

@SebastianDeiss

SebastianDeiss Aug 22, 2014
Author Contributor

I agree that dh-group14-sha1 should be the prefered kex. We should also consider implementing dh-gex-sha256, because sha1 is considered insecure too.

This comment has been minimized.

@lndbrg

lndbrg Aug 22, 2014
Contributor

We have a pull for dh-gex-sha256 here: #356

This comment has been minimized.

@SebastianDeiss

SebastianDeiss Aug 22, 2014
Author Contributor

+1

@coveralls
Copy link

@coveralls coveralls commented Aug 25, 2014

Coverage Status

Coverage decreased (-6.26%) when pulling b076c10 on SebastianDeiss:gssapi-py3-support into e941d56 on paramiko:master.

gss_authenticated=paramiko.AUTH_FAILED,
cc_file=None):
"""
@note: We are just checking in L{AuthHandler} that the given user is

This comment has been minimized.

@lndbrg

lndbrg Sep 8, 2014
Contributor

Low prio, but this docstring probably should get sphinxified.

This comment has been minimized.

@bitprophet

bitprophet Sep 8, 2014
Member

This ticket is still on my milestone list for 1.15 and I've been trying to catch this sort of thing as I go. Good looking out tho.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Sep 8, 2014

Merging this now, my temp todo list:

  • Reconcile README updates w/ www site's installation page, probably belongs there instead/additionally
  • Ensure the changelog links to those install docs since they mention optional dependencies
  • Ensure docstrings updated with .. versionadded::/changed
  • General formatting pass
  • Tests pass, including fabric's test suites
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Sep 9, 2014

A bit miffed at how much off-style stuff I had to edit here 😿 but it's my own fault for not having time to review & give feedback before this release sprint. Next time :)

Made a bunch of cleanup commits, they're primarily in this diff here: 752507a...e71f4e5

@bitprophet bitprophet merged commit b076c10 into paramiko:master Sep 9, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
bitprophet added a commit that referenced this pull request Sep 9, 2014
bitprophet added a commit that referenced this pull request Sep 9, 2014
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Sep 9, 2014

@SebastianDeiss Final note coming out of previous comment, this specific commit is probably the only one I'd like you to semi-sign-off for: 6b580b9 - its commit message lays out the rationale for the change.

If you have strong feelings about the hyperlinks to your organization, etc (again note that the copyright data in the headers has not been removed) let me know and we can work out how to add it back, probably into the public documentation.

Thanks again for all the effort on this!

@akruis
Copy link
Contributor

@akruis akruis commented Sep 9, 2014

If you have strong feelings about the hyperlinks to your organization, etc (again note that the copyright data in the headers has not been removed) let me know and we can work out how to add it back, probably into the public documentation.

Thanks again for all the effort on this!

@bitprophet I'm the manager in charge for this pull request at science + computing ag. Many thanks for merging Sebastian's work. About the hyperlinks etc: it is OK as it is, no need to change anything.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Sep 9, 2014

Great, thanks! Appreciate the feedback.

@@ -28,6 +28,7 @@
import sys
import os
import socket
from paramiko.py3compat import b

This comment has been minimized.

@lndbrg

lndbrg Sep 18, 2014
Contributor

unused?

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Sep 18, 2014

Was under the mistaken impression the tests could be run under Travis but it looks like we need an actual factual Kerberos database & such set up? So I guess we'll continue running the suite w/o enabling the Kerberos tests for the time being. A new PR adding some hooks to the travis.yml that set up a dummy Kerberos install, would be rad though.

@akruis

This comment has been minimized.

Copy link
Contributor

@akruis akruis commented on paramiko/ssh_gss.py in de0d528 Feb 9, 2015

Why doe you catch gssapi.BadSignature and raise the very unspecific Exception("GSS-API MIC check failed.")?
Woldn't it be better completely remove the try...except clause? At least you should use an more specific exception class.

@akruis

This comment has been minimized.

Copy link
Contributor

@akruis akruis commented on paramiko/auth_handler.py in de0d528 Feb 9, 2015

Ouch!! The variable retval is used in line 525 and is now undefined!

@akruis

This comment has been minimized.

Copy link
Contributor

@akruis akruis commented on paramiko/auth_handler.py in de0d528 Feb 9, 2015

Again, retval is used at line 551 and is undefined!

@akruis

This comment has been minimized.

Copy link
Contributor

@akruis akruis commented on paramiko/auth_handler.py in c8391cb Feb 9, 2015

MSG_NAMES is missing!!

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

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.