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 Windows native authentication support #34

Merged
merged 2 commits into from
May 4, 2015
Merged

Add Windows native authentication support #34

merged 2 commits into from
May 4, 2015

Conversation

gentoo90
Copy link
Contributor

@gentoo90 gentoo90 commented Jul 2, 2014

Add kerberos-sspi as an alternative backend.
kerberos-sspi package is an API level equivalent with kerberos
package but uses windows sspi (through pywin32)

@sigmavirus24
Copy link
Contributor

Before merging this, we need to review if anything here is necessary for this to be a real alternative backend.

@gentoo90
Copy link
Contributor Author

gentoo90 commented Jul 3, 2014

The methods:

  • changePassword
  • getServerPrincipalDetails

not used by requests-kerberos.

The flags:

  • GSS_C_ANON_FLAG
  • GSS_C_PROT_READY_FLAG
  • GSS_C_TRANS_FLAG

requests-kerberos uses default value of gssflags parameter of authGSSClientInit method.

Any way this seems to be the only kerberos implementation on Windows.
And this commit changes nothing for other platforms.

@sigmavirus24
Copy link
Contributor

It seems there is a bug fix in master that has yet to be released (in kerberos-sspi). I would expect anyone who runs headlong into it to complain upstream. I'm +1 on this as I suspect it's useful. I'll defer to @Lukasa and @mkomitee

@mkomitee
Copy link
Collaborator

mkomitee commented Jul 3, 2014

This is a great idea:

  1. Will this apply cleanly against pull request Brought rrig's implementation of kerberos delegation up to date with master #32? It optionally modifies the gssflags to support delegation.
  2. With kerberos a requirement on the package the import should always succeed. How would kerberos_sspi ever be imported?

@sigmavirus24
Copy link
Contributor

@mkomitee

  1. looks like it will apply cleanly.
  2. Good question, I suspect someone will pip uninstall kerberos && pip install kerberos-sspi if they know they need it.

@mkomitee
Copy link
Collaborator

mkomitee commented Jul 4, 2014

That's kind of ugly -- Users are expected to install requests_kerberos, which will attempt to (and maybe fail to) install kerberos. Then they're expected to uninstall kerberos and install kerberos-sspi?

Is there a way w/ setuptools to mark two packages as alternatives, and allow either one to fulfill the dependency?

If not, should we remove our dependency on kerberos and update the code to try kerberos, then try kerberos-sspi, then raise a custom exception that instructs them to install one of them?

btw, i'm +1 on merging this as is if it helps people, but we should come up with a better way to handle the kerberos/kerberos-sspi dependency.

@gentoo90
Copy link
Contributor Author

gentoo90 commented Sep 6, 2014

Fixed if statement as @sigmavirus24 said. Commit amended

@tardyp
Copy link

tardyp commented Jan 27, 2015

👍 This PR fixes my windows kerberos authentication issues in a clean fashion. Please merge it!

@e-nouri
Copy link

e-nouri commented Jan 27, 2015

👍 Kerberos SSPI fixed the issue on Windows, pealse merge it!

@mkomitee
Copy link
Collaborator

If it's not already clear from before, I'm +1.

@sigmavirus24
Copy link
Contributor

@gentoo90 can you rebase this? It isn't mergeable in its present state

kerberos-sspi package is an API level equivalent with kerberos
package but uses windows sspi (through pywin32)
@gentoo90
Copy link
Contributor Author

Done. Sorry for the delay.

@carsonyl
Copy link
Collaborator

@sigmavirus24 I'm in favour of this, and the changes look good to me. Right now I'm accomplishing the same result by renaming kerberos_sspi.py to kerberos.py.

sigmavirus24 added a commit that referenced this pull request May 4, 2015
Add Windows native authentication support
@sigmavirus24 sigmavirus24 merged commit 05bc753 into requests:master May 4, 2015
@sigmavirus24
Copy link
Contributor

Thanks for the 👀 @rbcarson

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.

6 participants