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

Add support for GSSAPI authentication. #235

Closed
wants to merge 1 commit into from

Conversation

adelton
Copy link
Contributor

@adelton adelton commented Jan 9, 2018

No description provided.

@simo5
Copy link
Member

simo5 commented Jan 9, 2018

can you add proper deps on request_gssapi and a test ?

@adelton
Copy link
Contributor Author

adelton commented Jan 9, 2018

Would you prefer deps, or maybe weakening the import and failing in runtime if the modules is not available?

As for tests, I'm not really sure how we would test it, given we'd likely need KDC for that ...

Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • you need to modify .travis.yml to install gssapi lib and development files, libkrb5-dev should do the trick
  • please add a test for the new method.
  • please add documentation

@@ -133,6 +133,11 @@ def timeout(arg):
help='PEM encoded key file (if not given, key is read from certfile)'
)

# Use Negotiate / GSSAPI
main_parser.add_argument(
Copy link
Member

@tiran tiran Jan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -8,6 +8,7 @@
from jwcrypto.jwk import JWK

import requests
from requests_gssapi import HTTPSPNEGOAuth
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the fact that all users have to install requests_gssapi. Please make the import optional and add an optional dependency to setup.py (gssapi extras require).

@@ -84,6 +85,9 @@ def set_client_cert(self, certfile, keyfile=None):
else:
self.session.cert = (certfile, keyfile)

def set_gssapi_auth(self):
Copy link
Member

@tiran tiran Jan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_gssapi_auth should support passing arguments to HTTPSPNEGOAuth(). Just take **kwargs and pass them along.

@frozencemetery
Copy link
Member

Thanks for using requests_gssapi! If tests are needed, it may be possible to use k5test.

That said, I'm not sure there's a lot of value in setting up a full test with KDC here, since bugs in that would be bugs in a lower component - perhaps it'd be better to check that it starts a GSSAPI handshake when prompted?

@tiran
Copy link
Member

tiran commented Jan 10, 2018

I don't think we need a full integration test or functional test here. A unit test is good enough, just check that the function sets the object.

@raildo
Copy link

raildo commented Jan 10, 2018

Can you also rebase that patch? So, it would be also in a good shape passing the tests.

@adelton
Copy link
Contributor Author

adelton commented Jan 17, 2018

Closing in favour of #238.

@adelton adelton closed this Jan 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants