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

Support of kerberos.GSS_MECH_OID_SPNEGO #15

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
@veklov
Contributor

veklov commented Jan 18, 2017

Hello Bernie (@behackett),

This change allows implemention of Spnego/Negotiate authentication in requests_kerberos (and potentially in urllib_kerberos after switching to winkerberos):

result, self.context[host] = kerberos.authGSSClientInit(kerb_spn, gssflags=gssflags, principal=self.principal, mech_oid=kerberos.GSS_MECH_OID_SPNEGO)

If you are fine with the change and merge it, I am going to add HttpSpnegoAuth to requests_kerberos and urllib_kerberos (migrating it to winkerberos).

Kind regrads,
Alexey Veklov

Support of kerberos.GSS_MECH_OID_SPNEGO
This change allows implemention of Spnego/Negotiate authentication in
requests_kerberos (and potentially in urllib_kerberos after switching to
winkerberos):
            result, self.context[host] =
	    kerberos.authGSSClientInit(kerb_spn,
	                    gssflags=gssflags, principal=self.principal,
			    mech_oid=kerberos.GSS_MECH_OID_SPNEGO)
@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Jan 18, 2017

Collaborator

Neat! I had thought about adding this myself. Thanks for the patch. I'll review and merge it soon.

Collaborator

behackett commented Jan 18, 2017

Neat! I had thought about adding this myself. Thanks for the patch. I'll review and merge it soon.

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Jan 18, 2017

Collaborator

Though it's not strictly necessary, maybe we should use PyCObject / PyCapsule for the OIDs, rather than strings, to match the API of PyKerberos?

https://github.com/apple/ccs-pykerberos/blob/PyKerberos-1.2.5/src/kerberos.c#L836-L841

Collaborator

behackett commented Jan 18, 2017

Though it's not strictly necessary, maybe we should use PyCObject / PyCapsule for the OIDs, rather than strings, to match the API of PyKerberos?

https://github.com/apple/ccs-pykerberos/blob/PyKerberos-1.2.5/src/kerberos.c#L836-L841

@behackett behackett self-requested a review Jan 18, 2017

@behackett behackett self-assigned this Jan 18, 2017

@behackett behackett added this to the 0.6.0 milestone Jan 18, 2017

@veklov

This comment has been minimized.

Show comment
Hide comment
@veklov

veklov Jan 18, 2017

Contributor

To tell the truth I am new to Python, feel free to do whatever changes you think are necessary. If you do not have time, give me a hint and I will try to implement and update the pull request.

Contributor

veklov commented Jan 18, 2017

To tell the truth I am new to Python, feel free to do whatever changes you think are necessary. If you do not have time, give me a hint and I will try to implement and update the pull request.

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Jan 18, 2017

Collaborator

No worries. I can merge this and add patches after. Thanks again for the patch. This is great work.

Collaborator

behackett commented Jan 18, 2017

No worries. I can merge this and add patches after. Thanks again for the patch. This is great work.

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Jan 19, 2017

Collaborator

@veklov - to add support for this option to requests-kerberos, you'll also have to get requests-kerberos to switch to https://github.com/apple/ccs-pykerberos (see requests/requests-kerberos#63), or get this feature added to https://github.com/02strich/pykerberos.

Collaborator

behackett commented Jan 19, 2017

@veklov - to add support for this option to requests-kerberos, you'll also have to get requests-kerberos to switch to https://github.com/apple/ccs-pykerberos (see requests/requests-kerberos#63), or get this feature added to https://github.com/02strich/pykerberos.

@veklov

This comment has been minimized.

Show comment
Hide comment
@veklov

veklov Jan 19, 2017

Contributor

@behackett - I have changed the code to use capsules.

Good catch about 02strich/pykerberos vs apple/ccs-pykerberos
I tried several libraries and it looks like I finally ended up testing with packages after
pip install requests-kerberos
pip install kerberos
I presume that Apple's implementation was used...
I will try to create a patch for pykerberos as well.

Meanwhile, could you take a look at my attempt to integrate this into requests-kerberos:
https://github.com/veklov/requests-kerberos

Contributor

veklov commented Jan 19, 2017

@behackett - I have changed the code to use capsules.

Good catch about 02strich/pykerberos vs apple/ccs-pykerberos
I tried several libraries and it looks like I finally ended up testing with packages after
pip install requests-kerberos
pip install kerberos
I presume that Apple's implementation was used...
I will try to create a patch for pykerberos as well.

Meanwhile, could you take a look at my attempt to integrate this into requests-kerberos:
https://github.com/veklov/requests-kerberos

@behackett behackett self-requested a review Jan 20, 2017

@behackett

Looks pretty good so far. The only issue is that we have to support Python versions that don't provide PyCapsule. I've added comments explaining what to do instead.

Show outdated Hide outdated src/winkerberos.c
@@ -775,6 +785,12 @@ initwinkerberos(VOID)
"GSS_C_INTEG_FLAG",
PyInt_FromLong(ISC_REQ_INTEGRITY)) ||
PyModule_AddObject(module,
"GSS_MECH_OID_KRB5",
PyCapsule_New(GSS_MECH_OID_KRB5, "winkerberos.GSS_MECH_OID_KRB5", NULL)) ||

This comment has been minimized.

@behackett

behackett Jan 20, 2017

Collaborator

We have to support Python versions that don't provide PyCapsule. I've aliased the necessary PyCObject* functions to the PyCapsule equivalents at the top of this file. Change this to:

PyCObject_FromVoidPtr(GSS_MECH_OID_KRB5, NULL)

@behackett

behackett Jan 20, 2017

Collaborator

We have to support Python versions that don't provide PyCapsule. I've aliased the necessary PyCObject* functions to the PyCapsule equivalents at the top of this file. Change this to:

PyCObject_FromVoidPtr(GSS_MECH_OID_KRB5, NULL)

Show outdated Hide outdated src/winkerberos.c
PyCapsule_New(GSS_MECH_OID_KRB5, "winkerberos.GSS_MECH_OID_KRB5", NULL)) ||
PyModule_AddObject(module,
"GSS_MECH_OID_SPNEGO",
PyCapsule_New(GSS_MECH_OID_SPNEGO, "winkerberos.GSS_MECH_OID_SPNEGO", NULL)) ||

This comment has been minimized.

@behackett

behackett Jan 20, 2017

Collaborator

Do the same here, but with GSS_MECH_OID_SPENGO.

@behackett

behackett Jan 20, 2017

Collaborator

Do the same here, but with GSS_MECH_OID_SPENGO.

Show outdated Hide outdated src/winkerberos.c
@@ -359,6 +364,11 @@ sspi_client_init(PyObject* self, PyObject* args, PyObject* kw) {
ulen = wcslen(user);
}
if (mechoidobj != NULL && PyCapsule_CheckExact(mechoidobj)) {

This comment has been minimized.

@behackett

behackett Jan 20, 2017

Collaborator

We have to use PyCObject functions (or the aliases defined in this file). This should be something like:

if (mechoidobj != NULL) {
    if (!PyCObject_Check(mechoidobj)) {
        PyErr_SetString(PyExc_TypeError, "Invalid type for mech_oid");
        goto done;
    } else {
        mechoid = (WCHAR*)PyCObject_AsVoidPtr(mechoidobj);
        if (mechoid == NULL) {
            goto done;
        }
    }
}
@behackett

behackett Jan 20, 2017

Collaborator

We have to use PyCObject functions (or the aliases defined in this file). This should be something like:

if (mechoidobj != NULL) {
    if (!PyCObject_Check(mechoidobj)) {
        PyErr_SetString(PyExc_TypeError, "Invalid type for mech_oid");
        goto done;
    } else {
        mechoid = (WCHAR*)PyCObject_AsVoidPtr(mechoidobj);
        if (mechoid == NULL) {
            goto done;
        }
    }
}
Show outdated Hide outdated setup.py
@@ -84,7 +84,7 @@ def run(self):
setup(
name="winkerberos",
version="0.5.0",
version="0.6.0",

This comment has been minimized.

@behackett

behackett Jan 20, 2017

Collaborator

Following PEP-440, until we do a release this should be 0.6.0.dev0.

@behackett

behackett Jan 20, 2017

Collaborator

Following PEP-440, until we do a release this should be 0.6.0.dev0.

@veklov

This comment has been minimized.

Show comment
Hide comment
@veklov

veklov Jan 21, 2017

Contributor

@behackett - Hi. I will update the code as you suggested. I adapted PyCapsule implementation used in Apple's code, and I did the same while merging Apple's fix into 02strich/pykerberos (02strich/pykerberos#25). I literally copied it. Could you take a look at that pull request and suggest whether it makes sense to use similar macroses there?

Contributor

veklov commented Jan 21, 2017

@behackett - Hi. I will update the code as you suggested. I adapted PyCapsule implementation used in Apple's code, and I did the same while merging Apple's fix into 02strich/pykerberos (02strich/pykerberos#25). I literally copied it. Could you take a look at that pull request and suggest whether it makes sense to use similar macroses there?

@veklov

This comment has been minimized.

Show comment
Hide comment
@veklov

veklov Jan 23, 2017

Contributor

@behackett - Hi, I have changed the code as you suggested.

Contributor

veklov commented Jan 23, 2017

@behackett - Hi, I have changed the code as you suggested.

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Jan 23, 2017

Collaborator

Great work. I'll do some testing and merge this soon.

The issue with PyCapsule is that Python 2.6 doesn't provide it. PyKerberos currently works on 2.6, and provides aliases similar to WinKerberos. It appears that Apple's project broke support for Python 2.6.

Note that requests (and therefore requests-kerberos) claims to support Python 2.6.

Collaborator

behackett commented Jan 23, 2017

Great work. I'll do some testing and merge this soon.

The issue with PyCapsule is that Python 2.6 doesn't provide it. PyKerberos currently works on 2.6, and provides aliases similar to WinKerberos. It appears that Apple's project broke support for Python 2.6.

Note that requests (and therefore requests-kerberos) claims to support Python 2.6.

@veklov

This comment has been minimized.

Show comment
Hide comment
@veklov

veklov Jan 23, 2017

Contributor

ok. It turned out that PyKerberos automatically runs tests for pull requests. Mine passed https://travis-ci.org/02strich/pykerberos/builds/193902068 (including Python 2.6 build). By the way, I have not got any response for 02strich/pykerberos#25 . Maybe 02strich is out. Do you know who else can review it?

Contributor

veklov commented Jan 23, 2017

ok. It turned out that PyKerberos automatically runs tests for pull requests. Mine passed https://travis-ci.org/02strich/pykerberos/builds/193902068 (including Python 2.6 build). By the way, I have not got any response for 02strich/pykerberos#25 . Maybe 02strich is out. Do you know who else can review it?

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Jan 23, 2017

Collaborator

Looking at the Travis test output I'm not sure what it actually tests. Python 2.6 definitely doesn't provide PyCapsule. PyKerberos provides PyNew, PyCheck, and PyGet aliases for this purpose:

https://github.com/02strich/pykerberos/blob/v1.1.9/src/kerberos.c#L26-L28

Collaborator

behackett commented Jan 23, 2017

Looking at the Travis test output I'm not sure what it actually tests. Python 2.6 definitely doesn't provide PyCapsule. PyKerberos provides PyNew, PyCheck, and PyGet aliases for this purpose:

https://github.com/02strich/pykerberos/blob/v1.1.9/src/kerberos.c#L26-L28

@veklov

This comment has been minimized.

Show comment
Hide comment
@veklov

veklov Jan 23, 2017

Contributor

I will update that pull request too :)

Contributor

veklov commented Jan 23, 2017

I will update that pull request too :)

@veklov

This comment has been minimized.

Show comment
Hide comment
@veklov

veklov Feb 6, 2017

Contributor

Hi, Bernie. My pull request for PyKerberos (02strich/pykerberos#25) has been merged and released. Could you merge this one, so I could proceed with the fix for requests_kerberos and urllib_kerberos.

Contributor

veklov commented Feb 6, 2017

Hi, Bernie. My pull request for PyKerberos (02strich/pykerberos#25) has been merged and released. Could you merge this one, so I could proceed with the fix for requests_kerberos and urllib_kerberos.

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Feb 7, 2017

Collaborator

Thanks for your patience. I've merged this, applied a small fix, updated the docs, and given you credit in the changelog.

Thanks again! This is a great addition to the library.

Collaborator

behackett commented Feb 7, 2017

Thanks for your patience. I've merged this, applied a small fix, updated the docs, and given you credit in the changelog.

Thanks again! This is a great addition to the library.

@behackett behackett closed this Feb 7, 2017

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Feb 7, 2017

Collaborator

I'm going to run a build through Coverity scan. Assuming there are no issues, I'll do a release after.

Collaborator

behackett commented Feb 7, 2017

I'm going to run a build through Coverity scan. Assuming there are no issues, I'll do a release after.

@veklov

This comment has been minimized.

Show comment
Hide comment
@veklov

veklov Feb 7, 2017

Contributor

Thank you Bernie

Contributor

veklov commented Feb 7, 2017

Thank you Bernie

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Feb 14, 2017

Collaborator

I've released WinKerberos 0.6.0 - https://pypi.python.org/pypi/winkerberos/0.6.0. Thanks again for the patch!

Collaborator

behackett commented Feb 14, 2017

I've released WinKerberos 0.6.0 - https://pypi.python.org/pypi/winkerberos/0.6.0. Thanks again for the patch!

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