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

Channel bindings #17

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
2 participants
@jborean93
Contributor

jborean93 commented May 29, 2017

This PR brings in support for Channel Binding Tokens through RFC5929 #16. I've manually tested it against a WinRM endpoint with the following value set.

Set-Item -Path "WSMan:\localhost\Service\Auth\CbtHardeningLevel" -Value Strict

Happy to change anything that is concerning with how it is implemented but it is designed to be as cross compatible with apple/ccs-pykerberos#55.

@behackett

Thanks for the patch! Here's my initial set of comments.

Show outdated Hide outdated README.rst
@@ -64,6 +64,13 @@ following RFC-4752, section 3.1:
import winkerberos as kerberos
def get_channel_binding_struct(response):
# This is 'tls-server-end-point:' + certificate hash of the server
application_data = b'tls-server-end-point:D01402E0F16F30ED71B02B655AD71C7B0ADA73DE5FBD8134021A794FFA1EECE8'

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

I haven't tested this yet, but I assume the value for tls-server-end-point can be a hash of the value returned by SSLSocket.getpeercert(binary_form=True)? The RFC says you have to use a SHA-256 hash if the signing method was MD5 or SHA-1, otherwise whatever hash function was used to sign the peer cert. It looks like the only way you can get the signature algorithm used is through the cryptography module, since getpeercert's dict format doesn't return that.

Can we provide more guidance here? The same information would be equally useful in pykerberos' docs.

@behackett

behackett May 31, 2017

Collaborator

I haven't tested this yet, but I assume the value for tls-server-end-point can be a hash of the value returned by SSLSocket.getpeercert(binary_form=True)? The RFC says you have to use a SHA-256 hash if the signing method was MD5 or SHA-1, otherwise whatever hash function was used to sign the peer cert. It looks like the only way you can get the signature algorithm used is through the cryptography module, since getpeercert's dict format doesn't return that.

Can we provide more guidance here? The same information would be equally useful in pykerberos' docs.

This comment has been minimized.

@jborean93

jborean93 May 31, 2017

Contributor

When testing this I just used a SHA256 hash as my server endpoint matched the RFC for this. I have done some extra work in requests-ntlm and requests-kerberos to support hashed > SHA256 by manually parsing the DER of the cert as it is just an ASN1 structure. An example of this can be found at https://github.com/jborean93/requests-kerberos/blob/3a3ef105e01952a61c993ac275555d45d8ad8101/requests_kerberos/kerberos_.py#L96-L168

I can definitely add some more info to the README for this though and give some more concrete examples.

@jborean93

jborean93 May 31, 2017

Contributor

When testing this I just used a SHA256 hash as my server endpoint matched the RFC for this. I have done some extra work in requests-ntlm and requests-kerberos to support hashed > SHA256 by manually parsing the DER of the cert as it is just an ASN1 structure. An example of this can be found at https://github.com/jborean93/requests-kerberos/blob/3a3ef105e01952a61c993ac275555d45d8ad8101/requests_kerberos/kerberos_.py#L96-L168

I can definitely add some more info to the README for this though and give some more concrete examples.

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

get_cbt_application_data is awesome. Too bad Python's ssl module doesn't provide a way to get that information.

@behackett

behackett May 31, 2017

Collaborator

get_cbt_application_data is awesome. Too bad Python's ssl module doesn't provide a way to get that information.

@@ -64,6 +64,13 @@ following RFC-4752, section 3.1:
import winkerberos as kerberos
def get_channel_binding_struct(response):

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

This feels like advanced usage, rather than simple tutorial. It might be better to document this in the docstring for buildChannelBindingsStruct.

@behackett

behackett May 31, 2017

Collaborator

This feels like advanced usage, rather than simple tutorial. It might be better to document this in the docstring for buildChannelBindingsStruct.

This comment has been minimized.

@jborean93

jborean93 May 31, 2017

Contributor

Is there a practice as to how large this docstring should be? I can add the standard tls-server-end-point: one and links to the RFC if needed.

@jborean93

jborean93 May 31, 2017

Contributor

Is there a practice as to how large this docstring should be? I can add the standard tls-server-end-point: one and links to the RFC if needed.

Show outdated Hide outdated src/winkerberos.c
char **acceptor_address = NULL;
char **application_data = NULL;
if (!PyArg_ParseTupleAndKeywords(args, keywds, "|iet#iet#et#", kwlist,

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

This situation here is different from pykerberos. Since you have to copy each string into channel_bindings anyway, might as well just use s#, rather than et#, and save a memory copy for each string. You won't have to call PyMem_Free below either.

@behackett

behackett May 31, 2017

Collaborator

This situation here is different from pykerberos. Since you have to copy each string into channel_bindings anyway, might as well just use s#, rather than et#, and save a memory copy for each string. You won't have to call PyMem_Free below either.

This comment has been minimized.

@jborean93

jborean93 May 31, 2017

Contributor

Ok, thanks for the info.

@jborean93

jborean93 May 31, 2017

Contributor

Ok, thanks for the info.

Show outdated Hide outdated src/winkerberos.c
SecPkgContext_Bindings* context_bindings;
PyObject *pychan_bindings = NULL;
int result = 0;
int initiator_addrtype = 0;

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

Since everything in SEC_CHANNEL_BINDINGS is type unsigned long, use the 'l' format specifier, then check the value and raise ValueError if it is less than 0.

@behackett

behackett May 31, 2017

Collaborator

Since everything in SEC_CHANNEL_BINDINGS is type unsigned long, use the 'l' format specifier, then check the value and raise ValueError if it is less than 0.

Show outdated Hide outdated src/winkerberos.c
PyObject *pychan_bindings = NULL;
int result = 0;
int initiator_addrtype = 0;
int acceptor_addrtype = 0;

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

Same here.

@behackett

behackett May 31, 2017

Collaborator

Same here.

Show outdated Hide outdated src/winkerberos.c
if (application_data != NULL) {
channel_bindings->dwApplicationDataOffset = offset;
offset_p = &((unsigned char*) channel_bindings)[offset];
memcpy(&offset_p[0], application_data, application_length);

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

memcpy_s

@behackett

behackett May 31, 2017

Collaborator

memcpy_s

Show outdated Hide outdated src/winkerberos.c
PyErr_SetString(PyExc_TypeError, "Expected a SecPkgContext_Bindings object");
return NULL;
}
sec_pkg_context_bindings = (SecPkgContext_Bindings *)PyCObject_AsVoidPtr(pychan_bindings);

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

Check for NULL.

@behackett

behackett May 31, 2017

Collaborator

Check for NULL.

This comment has been minimized.

@jborean93

jborean93 May 31, 2017

Contributor

Do we have to check for NULL, if pychan_bindings isn't set through the kwargs it will send NULl anyway to auth_sspi_client_step. If I do check for NULL would I be checking pychan_bindings or the result of this pointer cast?

@jborean93

jborean93 May 31, 2017

Contributor

Do we have to check for NULL, if pychan_bindings isn't set through the kwargs it will send NULl anyway to auth_sspi_client_step. If I do check for NULL would I be checking pychan_bindings or the result of this pointer cast?

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

The call to PyCapsule_GetPointer can fail, returning NULL:

https://docs.python.org/3.1/c-api/capsule.html#PyCapsule_GetPointer

It's generally good practice to always check return values in C.

@behackett

behackett May 31, 2017

Collaborator

The call to PyCapsule_GetPointer can fail, returning NULL:

https://docs.python.org/3.1/c-api/capsule.html#PyCapsule_GetPointer

It's generally good practice to always check return values in C.

channel_bindings->dwApplicationDataOffset = 0;
}
pychan_bindings = PyCObject_FromVoidPtr(context_bindings, &destruct_channel_bindings_struct);

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

PyCapsule_New, which is used here in Python 3.x, can return NULL on error. Though the docs for PyCObject_FromVoidPtr are not explicit, checking for NULL certainly doesn't hurt there either.

@behackett

behackett May 31, 2017

Collaborator

PyCapsule_New, which is used here in Python 3.x, can return NULL on error. Though the docs for PyCObject_FromVoidPtr are not explicit, checking for NULL certainly doesn't hurt there either.

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

Note that if this fails you will have to free channel_bindings and context_bindings before returning NULL.

@behackett

behackett May 31, 2017

Collaborator

Note that if this fails you will have to free channel_bindings and context_bindings before returning NULL.

Show outdated Hide outdated src/winkerberos.c
int initiator_addrtype = 0;
int acceptor_addrtype = 0;
int offset = 0;
int data_length = 0;

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

Since you're adding three ints together you need more than an int to avoid overflow. Since this value is going to passed to malloc, use size_t.

@behackett

behackett May 31, 2017

Collaborator

Since you're adding three ints together you need more than an int to avoid overflow. Since this value is going to passed to malloc, use size_t.

Show outdated Hide outdated src/winkerberos.c
context_bindings = PyCapsule_GetPointer(o, NULL);
#else
void destruct_channel_bindings_struct(void* o) {
SecPkgContext_Bindings* context_bindings;

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

You can define this variable and set it on the same line.

@behackett

behackett May 31, 2017

Collaborator

You can define this variable and set it on the same line.

This comment has been minimized.

@jborean93

jborean93 May 31, 2017

Contributor

Thanks will do this.

@jborean93

jborean93 May 31, 2017

Contributor

Thanks will do this.

Show outdated Hide outdated src/winkerberos.c
data_length = (int)initiator_length + (int)acceptor_length + (int)application_length;
offset = sizeof(SEC_CHANNEL_BINDINGS);
context_bindings = (SecPkgContext_Bindings *) malloc(sizeof(SecPkgContext_Bindings));

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

context_bindings could be NULL if malloc failed. In that case you should call PyErr_NoMemory and return NULL.

@behackett

behackett May 31, 2017

Collaborator

context_bindings could be NULL if malloc failed. In that case you should call PyErr_NoMemory and return NULL.

Show outdated Hide outdated src/winkerberos.c
context_bindings = (SecPkgContext_Bindings *) malloc(sizeof(SecPkgContext_Bindings));
context_bindings->BindingsLength = sizeof(SEC_CHANNEL_BINDINGS) + data_length;
channel_bindings = (SEC_CHANNEL_BINDINGS*) malloc(context_bindings->BindingsLength);

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

Need to check for NULL here as well. Note that you will have to free context_bindings before returning NULL.

@behackett

behackett May 31, 2017

Collaborator

Need to check for NULL here as well. Note that you will have to free context_bindings before returning NULL.

channel_bindings->dwApplicationDataOffset = 0;
}
pychan_bindings = PyCObject_FromVoidPtr(context_bindings, &destruct_channel_bindings_struct);

This comment has been minimized.

@behackett

behackett May 31, 2017

Collaborator

Note that if this fails you will have to free channel_bindings and context_bindings before returning NULL.

@behackett

behackett May 31, 2017

Collaborator

Note that if this fails you will have to free channel_bindings and context_bindings before returning NULL.

@jborean93

This comment has been minimized.

Show comment
Hide comment
@jborean93

jborean93 Jun 12, 2017

Contributor

Hey @behackett I've made the changes requested, please let me know if I've missed anything as I probably have. I changed the name of the methods and kwargs for the step method based on what was requested in the ccs-pykerberos PR.

Contributor

jborean93 commented Jun 12, 2017

Hey @behackett I've made the changes requested, please let me know if I've missed anything as I probably have. I changed the name of the methods and kwargs for the step method based on what was requested in the ccs-pykerberos PR.

@behackett

This is getting really close. The docstring for channelBindings is fantastic.

Show outdated Hide outdated doc/changelog.rst
- Added optional support for passing in Channel Binding Tokens (RFC 5929) into
:func:`winkerberos.authGSSClientStep`. The binding token structure can be
built using :func:`winkerberos.buildChannelBindingsStruct` (see the example

This comment has been minimized.

@behackett

behackett Jun 15, 2017

Collaborator

winkerberos.channelBindings

@behackett

behackett Jun 15, 2017

Collaborator

winkerberos.channelBindings

Show outdated Hide outdated doc/winkerberos.rst
@@ -12,6 +12,7 @@
.. autofunction:: authGSSClientUnwrap
.. autofunction:: authGSSClientWrap
.. autofunction:: authGSSClientClean
.. autofunction:: buildChannelBindingsStruct

This comment has been minimized.

@behackett

behackett Jun 15, 2017

Collaborator

channelBindings

@behackett

behackett Jun 15, 2017

Collaborator

channelBindings

Show outdated Hide outdated src/winkerberos.c
"\n"
"If using an endpoint over TLS like IIS with Extended Protection or WinRM\n"
"with the CbtHardeningLevel set to Strict then you can use this method to\n"
"build the channel bindings structured required for a successful\n"

This comment has been minimized.

@behackett

behackett Jun 15, 2017

Collaborator

structured -> structure

@behackett

behackett Jun 15, 2017

Collaborator

structured -> structure

Show outdated Hide outdated src/winkerberos.c
" 'tls-server-endpoint:{cert-hash}' where {cert-hash} is the hash of the\n"
" server's certificate.\n"
"\n"
":Returns: SecPkgContext_Bindings* where SecPkgContext_Bindings is the\n"

This comment has been minimized.

@behackett

behackett Jun 15, 2017

Collaborator

I would just say something like...

"Returns: an opaque value to be passed to the channel_bindings parameter of authGSSClientStep"

That language is similar to how we describe the context object returned by authGSSClientInit.

@behackett

behackett Jun 15, 2017

Collaborator

I would just say something like...

"Returns: an opaque value to be passed to the channel_bindings parameter of authGSSClientStep"

That language is similar to how we describe the context object returned by authGSSClientInit.

Show outdated Hide outdated src/kerberos_sspi.c
@@ -338,8 +355,8 @@ auth_sspi_client_step(sspi_client_state* state, SEC_CHAR* challenge) {
status = AUTH_GSS_CONTINUE;
}
done:
if (inBufs[0].pvBuffer) {
free(inBufs[0].pvBuffer);
if (inBufs[1].pvBuffer) {

This comment has been minimized.

@behackett

behackett Jun 15, 2017

Collaborator

If the caller doesn't provide channel bindings, the data to free will be at inBufs[0].

@behackett

behackett Jun 15, 2017

Collaborator

If the caller doesn't provide channel bindings, the data to free will be at inBufs[0].

Show outdated Hide outdated src/winkerberos.c
if (pychan_bindings == NULL) {
free(channel_bindings);
free(context_bindings);
PyErr_SetString(PyExc_RuntimeError, "NULL result returned when creating PyCObject of a SecPkgContext_Bindings struct");

This comment has been minimized.

@behackett

behackett Jun 15, 2017

Collaborator

If PyCapsule_New or PyCObject_FromVoidPtr fail an exception with more detail will automatically be set. You shouldn't set RuntimeError here, just return NULL.

@behackett

behackett Jun 15, 2017

Collaborator

If PyCapsule_New or PyCObject_FromVoidPtr fail an exception with more detail will automatically be set. You shouldn't set RuntimeError here, just return NULL.

Show outdated Hide outdated src/winkerberos.c
}
sec_pkg_context_bindings = (SecPkgContext_Bindings *)PyCObject_AsVoidPtr(pychan_bindings);
if (sec_pkg_context_bindings == NULL) {
PyErr_SetString(PyExc_RuntimeError, "NULL result returned when getting a SecPkgContext_Bindings struct from a PyCObject");

This comment has been minimized.

@behackett

behackett Jun 15, 2017

Collaborator

If PyCObject_AsVoidPtr (or the equivalent PyCapsule function) fails an exception will automatically be set. You shouldn't raise RuntimeError here. Just return NULL.

@behackett

behackett Jun 15, 2017

Collaborator

If PyCObject_AsVoidPtr (or the equivalent PyCapsule function) fails an exception will automatically be set. You shouldn't raise RuntimeError here. Just return NULL.

Show outdated Hide outdated src/winkerberos.c
result = auth_sspi_client_step(state, challenge);
if (pychan_bindings != NULL) {
if (!PyCObject_Check(pychan_bindings)) {
PyErr_SetString(PyExc_TypeError, "Expected a SecPkgContext_Bindings object");

This comment has been minimized.

@behackett

behackett Jun 15, 2017

Collaborator

Just say "Expected a channel bindings object"

@behackett

behackett Jun 15, 2017

Collaborator

Just say "Expected a channel bindings object"

Show outdated Hide outdated src/winkerberos.c
channel_bindings->dwInitiatorOffset = offset;
offset_p = &((unsigned char*) channel_bindings)[offset];
memcpy_s(&offset_p[0], initiator_length, initiator_address, initiator_length);

This comment has been minimized.

@behackett

behackett Jun 15, 2017

Collaborator

The idea behind memcpy_s is to ensure you have enough buffer space to copy the data on each call. Instead of passing initiator_length as the second param you want to pass data_length. Then, in the next call, data_length - initiator_length, then in the next call data_length - initiate_length - acceptor_length, etc. The function can only protect you if you tell it how much buffer space is actually left.

See https://msdn.microsoft.com/en-us/library/wes2t00f.aspx

@behackett

behackett Jun 15, 2017

Collaborator

The idea behind memcpy_s is to ensure you have enough buffer space to copy the data on each call. Instead of passing initiator_length as the second param you want to pass data_length. Then, in the next call, data_length - initiator_length, then in the next call data_length - initiate_length - acceptor_length, etc. The function can only protect you if you tell it how much buffer space is actually left.

See https://msdn.microsoft.com/en-us/library/wes2t00f.aspx

This comment has been minimized.

@jborean93

jborean93 Jun 16, 2017

Contributor

So the 2nd parameter is the amount of memory that is currently available in the buffer which memcpy_s verifies is enough for the copy process?

@jborean93

jborean93 Jun 16, 2017

Contributor

So the 2nd parameter is the amount of memory that is currently available in the buffer which memcpy_s verifies is enough for the copy process?

@jborean93

This comment has been minimized.

Show comment
Hide comment
@jborean93

jborean93 Jun 15, 2017

Contributor

Thanks very much for the feedback appreciate the time and I'll try and make the changes shortly. I am hoping to get a consensus with this PR and apple/ccs-pykerberos#55 before merging either but hopefully the other one is nearing completion.

Contributor

jborean93 commented Jun 15, 2017

Thanks very much for the feedback appreciate the time and I'll try and make the changes shortly. I am hoping to get a consensus with this PR and apple/ccs-pykerberos#55 before merging either but hopefully the other one is nearing completion.

@jborean93

This comment has been minimized.

Show comment
Hide comment
@jborean93

jborean93 Jun 16, 2017

Contributor

@behackett changes have been made.

Contributor

jborean93 commented Jun 16, 2017

@behackett changes have been made.

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Jun 16, 2017

Collaborator

Wonderful. I'll run it through coverity, fix anything coverity complains about, and merge it. Thanks again!

Collaborator

behackett commented Jun 16, 2017

Wonderful. I'll run it through coverity, fix anything coverity complains about, and merge it. Thanks again!

@jborean93 jborean93 referenced this pull request Jun 19, 2017

Merged

Cbt support #55

@jborean93

This comment has been minimized.

Show comment
Hide comment
@jborean93

jborean93 Jun 27, 2017

Contributor

@behackett how did the coverity report go, is this a scan I can run myself or is it some propriety product you have access to?

Contributor

jborean93 commented Jun 27, 2017

@behackett how did the coverity report go, is this a scan I can run myself or is it some propriety product you have access to?

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Jun 30, 2017

Collaborator

Sorry for the late response. What version of Python did you use to develop the patch set? It won't build for me under Python 2.6 and 2.7, but does build under 3.3+. I'm guessing the issue has something to do with the Visual Studio 2008 compiler (necessary for Python 2.6 and 2.7) vs. the Visual Studio 2010 compiler (Python 3.3 and 3.4) or Visual Studio 2015 compiler (Python 3.5+).

PS C:\10gen\winkerberos> C:\Python27\python.exe .\setup.py build_ext -i
running build_ext
building 'winkerberos' extension
creating build
creating build\temp.win-amd64-2.7
creating build\temp.win-amd64-2.7\Release
creating build\temp.win-amd64-2.7\Release\src
c:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\BIN\amd64\cl.exe /c /nologo /Ox /MD /W3 /GS- /DNDEBUG -IC:\Python2
7\include -IC:\Python27\PC /Tcsrc/winkerberos.c /Fobuild\temp.win-amd64-2.7\Release\src/winkerberos.obj
winkerberos.c
c:\10gen\winkerberos\src\kerberos_sspi.h(57) : error C2143: syntax error : missing ')' before '*'
c:\10gen\winkerberos\src\kerberos_sspi.h(57) : error C2081: 'SecPkgContext_Bindings' : name in formal parameter list ill
egal
c:\10gen\winkerberos\src\kerberos_sspi.h(57) : error C2143: syntax error : missing '{' before '*'
c:\10gen\winkerberos\src\kerberos_sspi.h(57) : error C2059: syntax error : ')'
src/winkerberos.c(445) : error C2065: 'SecPkgContext_Bindings' : undeclared identifier
src/winkerberos.c(445) : error C2065: 'context_bindings' : undeclared identifier
src/winkerberos.c(445) : error C2065: 'SecPkgContext_Bindings' : undeclared identifier
src/winkerberos.c(445) : error C2059: syntax error : ')'
src/winkerberos.c(447) : error C2065: 'context_bindings' : undeclared identifier
src/winkerberos.c(447) : warning C4047: '!=' : 'int' differs in levels of indirection from 'void *'
src/winkerberos.c(448) : error C2065: 'context_bindings' : undeclared identifier
src/winkerberos.c(448) : error C2223: left of '->Bindings' must point to struct/union
src/winkerberos.c(448) : error C2198: 'free' : too few arguments for call
src/winkerberos.c(449) : error C2065: 'context_bindings' : undeclared identifier
src/winkerberos.c(449) : warning C4022: 'free' : pointer mismatch for actual parameter 1
...more...
Collaborator

behackett commented Jun 30, 2017

Sorry for the late response. What version of Python did you use to develop the patch set? It won't build for me under Python 2.6 and 2.7, but does build under 3.3+. I'm guessing the issue has something to do with the Visual Studio 2008 compiler (necessary for Python 2.6 and 2.7) vs. the Visual Studio 2010 compiler (Python 3.3 and 3.4) or Visual Studio 2015 compiler (Python 3.5+).

PS C:\10gen\winkerberos> C:\Python27\python.exe .\setup.py build_ext -i
running build_ext
building 'winkerberos' extension
creating build
creating build\temp.win-amd64-2.7
creating build\temp.win-amd64-2.7\Release
creating build\temp.win-amd64-2.7\Release\src
c:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\BIN\amd64\cl.exe /c /nologo /Ox /MD /W3 /GS- /DNDEBUG -IC:\Python2
7\include -IC:\Python27\PC /Tcsrc/winkerberos.c /Fobuild\temp.win-amd64-2.7\Release\src/winkerberos.obj
winkerberos.c
c:\10gen\winkerberos\src\kerberos_sspi.h(57) : error C2143: syntax error : missing ')' before '*'
c:\10gen\winkerberos\src\kerberos_sspi.h(57) : error C2081: 'SecPkgContext_Bindings' : name in formal parameter list ill
egal
c:\10gen\winkerberos\src\kerberos_sspi.h(57) : error C2143: syntax error : missing '{' before '*'
c:\10gen\winkerberos\src\kerberos_sspi.h(57) : error C2059: syntax error : ')'
src/winkerberos.c(445) : error C2065: 'SecPkgContext_Bindings' : undeclared identifier
src/winkerberos.c(445) : error C2065: 'context_bindings' : undeclared identifier
src/winkerberos.c(445) : error C2065: 'SecPkgContext_Bindings' : undeclared identifier
src/winkerberos.c(445) : error C2059: syntax error : ')'
src/winkerberos.c(447) : error C2065: 'context_bindings' : undeclared identifier
src/winkerberos.c(447) : warning C4047: '!=' : 'int' differs in levels of indirection from 'void *'
src/winkerberos.c(448) : error C2065: 'context_bindings' : undeclared identifier
src/winkerberos.c(448) : error C2223: left of '->Bindings' must point to struct/union
src/winkerberos.c(448) : error C2198: 'free' : too few arguments for call
src/winkerberos.c(449) : error C2065: 'context_bindings' : undeclared identifier
src/winkerberos.c(449) : warning C4022: 'free' : pointer mismatch for actual parameter 1
...more...
@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Jun 30, 2017

Collaborator

Before you ask, I'm building on Windows 7, which msdn claims is the minimum required Windows version.

https://msdn.microsoft.com/en-us/library/windows/desktop/dd919960(v=vs.85).aspx

Collaborator

behackett commented Jun 30, 2017

Before you ask, I'm building on Windows 7, which msdn claims is the minimum required Windows version.

https://msdn.microsoft.com/en-us/library/windows/desktop/dd919960(v=vs.85).aspx

@jborean93

This comment has been minimized.

Show comment
Hide comment
@jborean93

jborean93 Jun 30, 2017

Contributor

I swear I tested from python 2.6-3.6 manually but I'll get back to you on that as I don't have access to the original environment right now, shouldn't take too long to rebuild though.

Contributor

jborean93 commented Jun 30, 2017

I swear I tested from python 2.6-3.6 manually but I'll get back to you on that as I don't have access to the original environment right now, shouldn't take too long to rebuild though.

@jborean93

This comment has been minimized.

Show comment
Hide comment
@jborean93

jborean93 Jun 30, 2017

Contributor

@behackett I've been able to compile it on all those Python versions but I'm getting some other errors which I will follow up on. I thought I tested it but it seems I didn't on the latest changes.

Contributor

jborean93 commented Jun 30, 2017

@behackett I've been able to compile it on all those Python versions but I'm getting some other errors which I will follow up on. I thought I tested it but it seems I didn't on the latest changes.

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Jun 30, 2017

Collaborator

Which version of Windows are you using, and are you using this compiler?

Collaborator

behackett commented Jun 30, 2017

Which version of Windows are you using, and are you using this compiler?

@jborean93

This comment has been minimized.

Show comment
Hide comment
@jborean93

jborean93 Jun 30, 2017

Contributor

For 2.6 and 2.7 yes that is the one. I used this page https://wiki.python.org/moin/WindowsCompilers to see what other compilers to use and was able to compile 3.3-6 as well with their respective ones.

This is the Windows details I have

C:\Users\WINDOWS7>systeminfo | findstr /B /C:"OS Name" /C:"OS Version"
OS Name: Microsoft Windows 7 Professional
OS Version: 6.1.7601 Service Pack 1 Build 7601

This is a brand new Windows 7 image that has all the latest updates installed and setuptools/pip are all up to date. I am using a virtualenv though unlike your example where it seems like you are using the default installed Python.

Contributor

jborean93 commented Jun 30, 2017

For 2.6 and 2.7 yes that is the one. I used this page https://wiki.python.org/moin/WindowsCompilers to see what other compilers to use and was able to compile 3.3-6 as well with their respective ones.

This is the Windows details I have

C:\Users\WINDOWS7>systeminfo | findstr /B /C:"OS Name" /C:"OS Version"
OS Name: Microsoft Windows 7 Professional
OS Version: 6.1.7601 Service Pack 1 Build 7601

This is a brand new Windows 7 image that has all the latest updates installed and setuptools/pip are all up to date. I am using a virtualenv though unlike your example where it seems like you are using the default installed Python.

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Jun 30, 2017

Collaborator

Thanks. I'm using the compilers from Visual Studio 2008 (the actual IDE). I'll try again with the Python specific package and see what happens.

Collaborator

behackett commented Jun 30, 2017

Thanks. I'm using the compilers from Visual Studio 2008 (the actual IDE). I'll try again with the Python specific package and see what happens.

@jborean93

This comment has been minimized.

Show comment
Hide comment
@jborean93

jborean93 Jul 1, 2017

Contributor

I'm coming across a really weird issue (probably due to my faulty code). I can get it working on all Python versions by having the following diff

diff --git a/src/kerberos_sspi.c b/src/kerberos_sspi.c
index 948ea99..09ba2c3 100644
--- a/src/kerberos_sspi.c
+++ b/src/kerberos_sspi.c
@@ -357,7 +357,8 @@ auth_sspi_client_step(sspi_client_state* state, SEC_CHAR* challenge, SecPkgConte
         status = AUTH_GSS_CONTINUE;
     }
 done:
-    if (inBufs[tokenBufferIndex].pvBuffer) {
+    if (inBufs[tokenBufferIndex].pvBuffer && inBufs[tokenBufferIndex].BufferType != SECBUFFER_EMPTY) {
+        printf("This is weirdly required\n");
         free(inBufs[tokenBufferIndex].pvBuffer);
     }
     if (outBufs[0].pvBuffer) **{**

For whatever reason it will fail with a 0xc0000374 STATUS_HEAP_CORRUPTION fault if the printf statement is not there but works with it in. This to me is an indication I've done something wrong somewhere but I cannot figure it out. Do you have any ideas?

Contributor

jborean93 commented Jul 1, 2017

I'm coming across a really weird issue (probably due to my faulty code). I can get it working on all Python versions by having the following diff

diff --git a/src/kerberos_sspi.c b/src/kerberos_sspi.c
index 948ea99..09ba2c3 100644
--- a/src/kerberos_sspi.c
+++ b/src/kerberos_sspi.c
@@ -357,7 +357,8 @@ auth_sspi_client_step(sspi_client_state* state, SEC_CHAR* challenge, SecPkgConte
         status = AUTH_GSS_CONTINUE;
     }
 done:
-    if (inBufs[tokenBufferIndex].pvBuffer) {
+    if (inBufs[tokenBufferIndex].pvBuffer && inBufs[tokenBufferIndex].BufferType != SECBUFFER_EMPTY) {
+        printf("This is weirdly required\n");
         free(inBufs[tokenBufferIndex].pvBuffer);
     }
     if (outBufs[0].pvBuffer) **{**

For whatever reason it will fail with a 0xc0000374 STATUS_HEAP_CORRUPTION fault if the printf statement is not there but works with it in. This to me is an indication I've done something wrong somewhere but I cannot figure it out. Do you have any ideas?

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Jul 11, 2017

Collaborator

I spun up a new Windows 7 VM using the Microsoft Python 2.7 compiler instead of Visual Studio 2008 and can build the project again. I'll update the build docs to require the right compiler after we get this merged. I assume at this point that the Python compiler just bundles a newer set of SSPI headers than Visual Studio 2008.

Sorry this is taking so long. A lot of other things at MongoDB are drawing my attention right now. I hope to be able to investigate that heap corruption later tonight.

Collaborator

behackett commented Jul 11, 2017

I spun up a new Windows 7 VM using the Microsoft Python 2.7 compiler instead of Visual Studio 2008 and can build the project again. I'll update the build docs to require the right compiler after we get this merged. I assume at this point that the Python compiler just bundles a newer set of SSPI headers than Visual Studio 2008.

Sorry this is taking so long. A lot of other things at MongoDB are drawing my attention right now. I hope to be able to investigate that heap corruption later tonight.

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Jul 13, 2017

Collaborator

What options are you using when you get STATUS_HEAP_CORRUPTION? I don't have an environment to test channel bindings, and the existing test suite runs without issue with your patch.

Collaborator

behackett commented Jul 13, 2017

What options are you using when you get STATUS_HEAP_CORRUPTION? I don't have an environment to test channel bindings, and the existing test suite runs without issue with your patch.

@jborean93

This comment has been minimized.

Show comment
Hide comment
@jborean93

jborean93 Jul 13, 2017

Contributor

I'm installing pywinrm and my forked version of requests-kerberos https://github.com/jborean93/requests-kerberos/tree/add-cbt and running the following script

from winrm.protocol import Protocol

p = Protocol(
    endpoint='https://SERVER.domain.local:5986/wsman',
    transport='kerberos',
    server_cert_validation='ignore')
shell_id = p.open_shell()
command_id = p.run_command(shell_id, 'ipconfig', ['/all'])
std_out, std_err, status_code = p.get_command_output(shell_id, command_id)
p.cleanup_command(shell_id, command_id)
p.close_shell(shell_id)

print("STDOUT")
print(std_out)
print("STDERR")
print(std_err)
print("RC")
print(status_code)

You will need a WinRM endpoint currently setup for this to work, if you want a quick and easy one this can do it for you on a host https://github.com/ansible/ansible/blob/devel/examples/scripts/ConfigureRemotingForAnsible.ps1

In it's current form this work with Python 2.6, 2.7 and 3.6, 3.3-3.5 are returning the STATUS_HEAP_CORRUPTION error.

Contributor

jborean93 commented Jul 13, 2017

I'm installing pywinrm and my forked version of requests-kerberos https://github.com/jborean93/requests-kerberos/tree/add-cbt and running the following script

from winrm.protocol import Protocol

p = Protocol(
    endpoint='https://SERVER.domain.local:5986/wsman',
    transport='kerberos',
    server_cert_validation='ignore')
shell_id = p.open_shell()
command_id = p.run_command(shell_id, 'ipconfig', ['/all'])
std_out, std_err, status_code = p.get_command_output(shell_id, command_id)
p.cleanup_command(shell_id, command_id)
p.close_shell(shell_id)

print("STDOUT")
print(std_out)
print("STDERR")
print(std_err)
print("RC")
print(status_code)

You will need a WinRM endpoint currently setup for this to work, if you want a quick and easy one this can do it for you on a host https://github.com/ansible/ansible/blob/devel/examples/scripts/ConfigureRemotingForAnsible.ps1

In it's current form this work with Python 2.6, 2.7 and 3.6, 3.3-3.5 are returning the STATUS_HEAP_CORRUPTION error.

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Aug 4, 2017

Collaborator

I think I understand what the problem is. Your test program is always passing two SecBuffers to InitializeSecurityContext, even on the first call when there is no token from the server yet. In that first call the second SecBuffer is set to type SECBUFFER_EMPTY. According to the docs on msdn, that is apparently a placeholder type for output buffers, rather than an input (read only) type. That may or may not contribute to the problem. The real problem, I think, is that you are also incrementing inbuf.cBuffers in that case, so InitializeSecurityContext is trying to access the data in that "empty" SecBuffer. I bet you can solve the problem by not incrementing cBuffers for the empty buffer.

Also, the docs for InitializeSecurityContext claim that pInput must be NULL on first call, but you're passing channel bindings on first call. Does authentication fail if you don't pass channel bindings on the first call? It wouldn't surprise me if the msdn docs are misleading or flat out wrong.

Collaborator

behackett commented Aug 4, 2017

I think I understand what the problem is. Your test program is always passing two SecBuffers to InitializeSecurityContext, even on the first call when there is no token from the server yet. In that first call the second SecBuffer is set to type SECBUFFER_EMPTY. According to the docs on msdn, that is apparently a placeholder type for output buffers, rather than an input (read only) type. That may or may not contribute to the problem. The real problem, I think, is that you are also incrementing inbuf.cBuffers in that case, so InitializeSecurityContext is trying to access the data in that "empty" SecBuffer. I bet you can solve the problem by not incrementing cBuffers for the empty buffer.

Also, the docs for InitializeSecurityContext claim that pInput must be NULL on first call, but you're passing channel bindings on first call. Does authentication fail if you don't pass channel bindings on the first call? It wouldn't surprise me if the msdn docs are misleading or flat out wrong.

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Aug 4, 2017

Collaborator

Can you try this patch on top of your existing changes?

diff --git a/src/kerberos_sspi.c b/src/kerberos_sspi.c
index 948ea99..0ce4e4c 100644
--- a/src/kerberos_sspi.c
+++ b/src/kerberos_sspi.c
@@ -249,8 +249,8 @@ auth_sspi_client_step(sspi_client_state* state, SEC_CHAR* challenge, SecPkgConte
     ULONG ignored;
     SECURITY_STATUS status = AUTH_GSS_CONTINUE;
     DWORD len;
-    int secBufferCount = 0;
-    int tokenBufferIndex = 0;
+    BOOL haveToken = FALSE;
+    INT tokenBufferIndex = 0;

     if (state->response != NULL) {
         free(state->response);
@@ -258,35 +258,27 @@ auth_sspi_client_step(sspi_client_state* state, SEC_CHAR* challenge, SecPkgConte
     }

     inbuf.ulVersion = SECBUFFER_VERSION;
+    inbuf.cBuffers = 0;
     inbuf.pBuffers = inBufs;

     if (sec_pkg_context_bindings != NULL) {
-        inBufs[secBufferCount].BufferType = SECBUFFER_CHANNEL_BINDINGS;
-        inBufs[secBufferCount].pvBuffer = sec_pkg_context_bindings->Bindings;
-        inBufs[secBufferCount].cbBuffer = sec_pkg_context_bindings->BindingsLength;
-
-        secBufferCount++;
+        inBufs[inbuf.cBuffers].BufferType = SECBUFFER_CHANNEL_BINDINGS;
+        inBufs[inbuf.cBuffers].pvBuffer = sec_pkg_context_bindings->Bindings;
+        inBufs[inbuf.cBuffers].cbBuffer = sec_pkg_context_bindings->BindingsLength;
+        inbuf.cBuffers++;
     }

-    tokenBufferIndex = secBufferCount;
+    tokenBufferIndex = inbuf.cBuffers;
     if (state->haveCtx) {
-        inBufs[secBufferCount].BufferType = SECBUFFER_TOKEN;
-        inBufs[secBufferCount].pvBuffer = base64_decode(challenge, &len);
-        if (!inBufs[secBufferCount].pvBuffer) {
+        haveToken = TRUE;
+        inBufs[tokenBufferIndex].BufferType = SECBUFFER_TOKEN;
+        inBufs[tokenBufferIndex].pvBuffer = base64_decode(challenge, &len);
+        if (!inBufs[tokenBufferIndex].pvBuffer) {
             return AUTH_GSS_ERROR;
         }
-        inBufs[secBufferCount].cbBuffer = len;
-
-        secBufferCount++;
-    } else if (!state->haveCtx && secBufferCount > 0) {
-        // Set the buffer to SECBUFFER_EMPTY if context bindings are set but we have no Context
-        inBufs[secBufferCount].BufferType = SECBUFFER_EMPTY;
-        inBufs[secBufferCount].pvBuffer = NULL;
-        inBufs[secBufferCount].cbBuffer = 0;
-
-        secBufferCount++;
+        inBufs[tokenBufferIndex].cbBuffer = len;
+        inbuf.cBuffers++;
     }
-    inbuf.cBuffers = secBufferCount;

     outbuf.ulVersion = SECBUFFER_VERSION;
     outbuf.cBuffers = 1;
@@ -357,7 +349,7 @@ auth_sspi_client_step(sspi_client_state* state, SEC_CHAR* challenge, SecPkgConte
         status = AUTH_GSS_CONTINUE;
     }
 done:
-    if (inBufs[tokenBufferIndex].pvBuffer) {
+    if (haveToken) {
         free(inBufs[tokenBufferIndex].pvBuffer);
     }
     if (outBufs[0].pvBuffer) {
Collaborator

behackett commented Aug 4, 2017

Can you try this patch on top of your existing changes?

diff --git a/src/kerberos_sspi.c b/src/kerberos_sspi.c
index 948ea99..0ce4e4c 100644
--- a/src/kerberos_sspi.c
+++ b/src/kerberos_sspi.c
@@ -249,8 +249,8 @@ auth_sspi_client_step(sspi_client_state* state, SEC_CHAR* challenge, SecPkgConte
     ULONG ignored;
     SECURITY_STATUS status = AUTH_GSS_CONTINUE;
     DWORD len;
-    int secBufferCount = 0;
-    int tokenBufferIndex = 0;
+    BOOL haveToken = FALSE;
+    INT tokenBufferIndex = 0;

     if (state->response != NULL) {
         free(state->response);
@@ -258,35 +258,27 @@ auth_sspi_client_step(sspi_client_state* state, SEC_CHAR* challenge, SecPkgConte
     }

     inbuf.ulVersion = SECBUFFER_VERSION;
+    inbuf.cBuffers = 0;
     inbuf.pBuffers = inBufs;

     if (sec_pkg_context_bindings != NULL) {
-        inBufs[secBufferCount].BufferType = SECBUFFER_CHANNEL_BINDINGS;
-        inBufs[secBufferCount].pvBuffer = sec_pkg_context_bindings->Bindings;
-        inBufs[secBufferCount].cbBuffer = sec_pkg_context_bindings->BindingsLength;
-
-        secBufferCount++;
+        inBufs[inbuf.cBuffers].BufferType = SECBUFFER_CHANNEL_BINDINGS;
+        inBufs[inbuf.cBuffers].pvBuffer = sec_pkg_context_bindings->Bindings;
+        inBufs[inbuf.cBuffers].cbBuffer = sec_pkg_context_bindings->BindingsLength;
+        inbuf.cBuffers++;
     }

-    tokenBufferIndex = secBufferCount;
+    tokenBufferIndex = inbuf.cBuffers;
     if (state->haveCtx) {
-        inBufs[secBufferCount].BufferType = SECBUFFER_TOKEN;
-        inBufs[secBufferCount].pvBuffer = base64_decode(challenge, &len);
-        if (!inBufs[secBufferCount].pvBuffer) {
+        haveToken = TRUE;
+        inBufs[tokenBufferIndex].BufferType = SECBUFFER_TOKEN;
+        inBufs[tokenBufferIndex].pvBuffer = base64_decode(challenge, &len);
+        if (!inBufs[tokenBufferIndex].pvBuffer) {
             return AUTH_GSS_ERROR;
         }
-        inBufs[secBufferCount].cbBuffer = len;
-
-        secBufferCount++;
-    } else if (!state->haveCtx && secBufferCount > 0) {
-        // Set the buffer to SECBUFFER_EMPTY if context bindings are set but we have no Context
-        inBufs[secBufferCount].BufferType = SECBUFFER_EMPTY;
-        inBufs[secBufferCount].pvBuffer = NULL;
-        inBufs[secBufferCount].cbBuffer = 0;
-
-        secBufferCount++;
+        inBufs[tokenBufferIndex].cbBuffer = len;
+        inbuf.cBuffers++;
     }
-    inbuf.cBuffers = secBufferCount;

     outbuf.ulVersion = SECBUFFER_VERSION;
     outbuf.cBuffers = 1;
@@ -357,7 +349,7 @@ auth_sspi_client_step(sspi_client_state* state, SEC_CHAR* challenge, SecPkgConte
         status = AUTH_GSS_CONTINUE;
     }
 done:
-    if (inBufs[tokenBufferIndex].pvBuffer) {
+    if (haveToken) {
         free(inBufs[tokenBufferIndex].pvBuffer);
     }
     if (outBufs[0].pvBuffer) {
@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Aug 5, 2017

Collaborator

Assuming that patch fixes the memory corruption issue you're having, I can move this along quickly. I've developed some patches that clean up compiler warnings and fix a few documentation issues (including documenting the compiler you need for Python 2.6 and 2.7).

Collaborator

behackett commented Aug 5, 2017

Assuming that patch fixes the memory corruption issue you're having, I can move this along quickly. I've developed some patches that clean up compiler warnings and fix a few documentation issues (including documenting the compiler you need for Python 2.6 and 2.7).

@jborean93

This comment has been minimized.

Show comment
Hide comment
@jborean93

jborean93 Aug 5, 2017

Contributor

Thanks @behackett for your suggestions, I've applied them and tested it locally and it works for me now. As for pInput being null on the first call, I would say it is a docs error as the credentials are rejected if I don't pass in the channel bindings structure everytime to that method.

Let me know if there is anything else you wish for me to change but I'm fairly happy things are working locally.

Contributor

jborean93 commented Aug 5, 2017

Thanks @behackett for your suggestions, I've applied them and tested it locally and it works for me now. As for pInput being null on the first call, I would say it is a docs error as the credentials are rejected if I don't pass in the channel bindings structure everytime to that method.

Let me know if there is anything else you wish for me to change but I'm fairly happy things are working locally.

@behackett

This comment has been minimized.

Show comment
Hide comment
@behackett

behackett Aug 5, 2017

Collaborator

Awesome. I've merged this and added a few follow up commits. Let me know if you have any problems.

Collaborator

behackett commented Aug 5, 2017

Awesome. I've merged this and added a few follow up commits. Let me know if you have any problems.

@jborean93

This comment has been minimized.

Show comment
Hide comment
@jborean93

jborean93 Aug 6, 2017

Contributor

Thanks for all of this, really appreciate you helping me with some of the issues I had.

Contributor

jborean93 commented Aug 6, 2017

Thanks for all of this, really appreciate you helping me with some of the issues I had.

@jborean93 jborean93 deleted the jborean93:channel-bindings branch Aug 6, 2017

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