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 kerberos support to authentication pip (when supported) #4854

Closed
wants to merge 11 commits into from

Conversation

cryvate
Copy link

@cryvate cryvate commented Nov 11, 2017

Closes #6708

Kerberos has seen widespread adoption in many academic institutions, corporations and other organisations. By:

  • vendoring requests_kerberos
  • detecting whether (requests_)kerberos is working by doing a try ... except ImportError
  • creating a chain of authenticators based on both prompting (opposite no-input flag) and kerberos flag

I have a server running that has kerberos authentication running (with/without possibility of interactive password authentication) and there is pypi.org that has no authentication (for downloads). Consider the matrix of options:

  • py/winkerberos installed (or not)
  • kerberos credentials present (or not) (user: testing, password: pip10pip10)
  • --no-input option passed (or not)

The possible outcomes are as follows:

  • If py/winkerberos is not installed, the behaviour is the same as before
  • If kerberos credentials are not present, the apparent behaviour is the same as before, and no network requests involving kerberos will be send (as pykerberos fails before that happens). These errors only show with --verbose
  • If kerberos credentials are present, and negotiation fails, it will fall-through to another option (prompting or failure). The errors are only shown with verbose.
  • If kerberos credentials are present, and negotiation succeeds, the connection will work with kerberos!

@pradyunsg pradyunsg added the type: feature request Request for a new feature label Nov 11, 2017
@pradyunsg
Copy link
Member

Hi @cryvate! Thanks for this PR! :)

I personally haven't used Kerberos so, I genuinely can't review the code in this PR. The rest looks fine to me.

Could you add documentation regarding this new support -- in some appropriate file in the docs/ directory? There's a tox job for generating the docs so, tox -e docs would build the documentation for you.

@cryvate
Copy link
Author

cryvate commented Nov 14, 2017

Having a reference to --no-input in the docs is helpful, until now I think you could only find it by looking at the source.

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Mar 31, 2018
@suic86
Copy link

suic86 commented Aug 14, 2018

Any chance to have this in the next pip version? Thanks

@cryvate
Copy link
Author

cryvate commented Aug 14, 2018

I don't see why not, I will have look at rebasing/merging and adding the docs

@forsberg
Copy link

Let me cheerfully encourage you to do the rebase/merge job! :-) This would be very useful for us in working with our internal nexus-based pypi repo!

@cryvate
Copy link
Author

cryvate commented Sep 26, 2018

OK, let's do this then. Better late than never!

@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 26, 2018
Kerberos Authentication
++++++++++++++++++++++++++++

Starting with v10.0, pip supports using a Kerberos ticket to authenticate
Copy link
Author

Choose a reason for hiding this comment

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

This version number should be changed to the release it will actually be in.

@suic86
Copy link

suic86 commented Nov 8, 2018

Any updates on this? Thanks

@pradyunsg
Copy link
Member

None yet @suic86.

@suic86
Copy link

suic86 commented Jan 22, 2019

@cryvate and @pradyunsg - can I somehow help to move this forward?

@pradyunsg
Copy link
Member

I think #5948 is likely a better way to go about this.

@cryvate
Copy link
Author

cryvate commented Jan 22, 2019 via email

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Jan 22, 2019
@suic86
Copy link

suic86 commented Jan 23, 2019

IMO kerberos support and keyring aren't the same thing.

Set logging configuration for vendored kerberos using dictConfig instead
of by hand.
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jan 23, 2019
@dstufft
Copy link
Member

dstufft commented Jan 23, 2019

Keyring is effectively a way to have pluggable authentication backends for HTTP requests, so it should be possible to write a keyring backend for Kerberos.

@cryvate
Copy link
Author

cryvate commented Jan 23, 2019

@dstufft I am not sure I agree. Keyring seems to fundamentally serve as a username-and-password middleware that allows writing and retrieving to various backends for keyrings (stores of username and password), however this is not sufficient to support Kerberos:

  • Kerberos is uncommonly used with username and password to access services, instead using username and password to retrieve a token that is then used for further authentication. This token could somehow be retrieved by a different backend as you suggest but it will not work the same in terms of HTTP requests as a username and password still requiring changes in pip to make it work.
  • Acquiring a service under a Kerberos ticket is a multi-authentication (including service to client authentication), multi-round-trip process that is not implemented in pip, its vendored dependencies or keyring.

Fundamentally, Kerberos is not a username-and-password system (though it uses them at one stage) and thus not a HTTP basic authentication protocol we can support without changing our Auth classes.

I might be misunderstanding, but I suggest reading the outline of the Kerberos protocol on Wikipedia before thinking it is 'just another username-and-password or username-and-token system.'

@suic86
Copy link

suic86 commented Feb 11, 2019

@dstufft and @pradyunsg: Is there any chance that this is going to be merged soon? Thanks

@gowdy
Copy link

gowdy commented Feb 13, 2019

This would be very useful for me too.

@ailurid
Copy link

ailurid commented Apr 9, 2019

I would also find this helpful

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label May 11, 2019
@chrahunt chrahunt mentioned this pull request Jul 14, 2019
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Aug 26, 2019
try:
from pip._vendor.requests_kerberos import HTTPKerberosAuth
from pip._vendor.requests_kerberos import kerberos_ as ik
_KERBEROS_AVAILABLE = True
Copy link
Member

Choose a reason for hiding this comment

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

Is there some reason other that pip._vendor.requests_kerberos might result in ImportErrors, outside of them being missing?

If not, please import these unconditionally at the top of the file, since pip does vendoring to ensure that vendored packages are always available. Basically, in not-broken installations of pip, this variable will always be True, which makes it redundant.

Copy link
Author

Choose a reason for hiding this comment

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

"If py/winkerberos is not installed, the behaviour is the same as before": requests_kerberos on its own will not make kerberos work, one has to have either py/winkerberos installed (which we cannot vendor because they are compiled/platform dependent?). requests_kerberos in that case will fail with an ImportError.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

There's a checked in a venv directory here. That's going to have to be removed from the history of this PR, for it to be eligible for merging.

@cryvate please lemme know if you want any sort of help with that. :)

@pradyunsg pradyunsg dismissed their stale review August 26, 2019 13:08

Ah, and the relevant change was commited and pushed in the same minute. :)

@pradyunsg
Copy link
Member

I spent some time thinking about this.

I don't think that there's anyone on the pip team, who has expertise in Kerberos. In this situation, it's not reasonable to believe that we would be maintaining this support for Kerberos. I, personally, did not know it existed till this PR came along.

However, there's clearly a user base for this feature. So, here's my proposal:

  • Put the kerberos support behind a --i-want-kerberos-support flag (please suggest better names).
    • Due to how pip's configuration handling works, enabling this on a system, is a matter of running pip config set global.i-want-kerberos-support true (or manually modifying the configuration file).
    • I don't want this to affect users who don't care about this in any way, unless the user explicitly opts-in.
  • Clearly document that kerberos support is not directly maintained by pip's maintainers and is "community maintained".

If other @pypa/pip-committers and @pypa/pip-helpers are OK with this, let's go down this route.


I'll also say this: If requests_kerberos gets significantly bigger, pip will de-vendor it and make it a conditional dependency (like we do for PyOpenSSL). At ~400 lines, right now it's not that big so I'm OK with vendoring it. If it's bigger, it's extra bytes for all users of pip, and AFAICT only a fairly small fraction of our users benefit from this support. Thus, requiring a bit of additional work on the end users who do need this support, is a trade-off I'm willing to make. (how big pip gets does affect how much bandwidth PyPI uses, so it is a concern as well here)

Related: In case it becomes a conditional dependency, and a user passes --i-want-kerberos-support, pip would complain that install requests_kerberos and abort.

@cryvate
Copy link
Author

cryvate commented Aug 26, 2019

(full quote of previous post)

That all sounds reasonable, I will rework the PR for this.

@pradyunsg
Copy link
Member

That all sounds reasonable, I will rework the PR for this.

Great! Thanks! ^>^

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 21, 2019
@pradyunsg
Copy link
Member

Gentle nudge to see if there's interest in moving this forward.

If no one expresses interest to take this forward in the coming week, I'll close this PR when I come around to it.

I'll note that my comment above still stands even if I close this PR.

@pradyunsg pradyunsg added the S: awaiting response Waiting for a response/more information label Oct 18, 2019
@chrahunt
Copy link
Member

chrahunt commented Nov 2, 2019

I'll close this given @pradyunsg's comment above. If there's any interest in picking this back up (this applies to anyone interested), please feel free to open a new PR.

@chrahunt chrahunt closed this Nov 2, 2019
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Dec 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation needs rebase or merge PR has conflicts with current master S: awaiting response Waiting for a response/more information type: feature request Request for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kerberos support
10 participants