Initial support for authGSSServer#20
Conversation
|
Neat! I'll take a closer look soon. |
behackett
left a comment
There was a problem hiding this comment.
Thanks for sending this pull request. I apologize for how long it's taken me to review it. I have a few questions about the API.
| PyObject* resultobj = NULL; | ||
| INT result = 0; | ||
| static SEC_CHAR* keywords[] = { | ||
| "service", "principal", "gssflags", "user", "domain", "password", "mech_oid", NULL }; |
There was a problem hiding this comment.
This project mimics the API of https://github.com/apple/ccs-pykerberos. It looks like the only parameter you want here is "service". I'm not sure what principal or gssflags is used for on the server side. On the bright side that means a huge amount of code can just go away.
Note that, though PyKerberos supports "DELEGATE" as a special service name, I'm not sure Winkerberos can actually support it because it doesn't seem like SSPI supports it.
https://github.com/apple/ccs-pykerberos/blob/PyKerberos-1.3.0/src/kerberos.c#L522
There was a problem hiding this comment.
It's actually just a straight copy of what's found in sspi_client_init. So you're right a lot of the code can go away.
| /* Buff */ | ||
| &inbuf, | ||
| /* Flags */ | ||
| ASC_REQ_INTEGRITY | ASC_REQ_SEQUENCE_DETECT | ASC_REQ_REPLAY_DETECT | ASC_REQ_CONFIDENTIALITY | state->flags, |
There was a problem hiding this comment.
PyKerberos sets this to NULL. I'm not sure what the implications of setting these flags is. It seems like the client can set them to require integrity, confidentiality, etc. without the server having them set. How did you settle on this list?
https://github.com/apple/ccs-pykerberos/blob/PyKerberos-1.3.0/src/kerberosgss.c#L725
Remove "principal", "gssflags", "user", "domain" and "password" parameters. Remove flags since PyKerberos sets them to NULL.
behackett
left a comment
There was a problem hiding this comment.
Thanks again for working on this, and for being patient with my entirely too slow code reviews. I've added a couple more comments about things that we don't want in the API, mostly because ccs-pykerberos doesn't support them.
If you can make the changes I'll merge in the results and do any further cleanup required.
| "O|O", | ||
| keywords, | ||
| &serviceobj, | ||
| &mechoidobj)) { |
There was a problem hiding this comment.
We don't want mechoid on the server side. WinKerberos only supports doing Kerberos (GSSAPI) authentication, not NTLM, so configuring it for "Negotiate" isn't useful. ccs-pykerberos also doesn't support this.
There was a problem hiding this comment.
I believe Negotiate means the client can fallback to NTLM if Kerberos is unavailable. However using only Kerberos for SPNEGO resulted in AcquireCredentialsHandleW returning SEC_E_INSUFFICIENT_MEMORY.
There was a problem hiding this comment.
I believe Negotiate means the client can fallback to NTLM
That is what it means, but WinKerberos doesn't support NTLM.
However using only Kerberos for SPNEGO resulted in AcquireCredentialsHandleW returning SEC_E_INSUFFICIENT_MEMORY
You mean when the client uses "Negotiate" WinKerberos errors in AcquireCredentialsHandle?
There was a problem hiding this comment.
You mean when the client uses "Negotiate" WinKerberos errors in AcquireCredentialsHandle?
Yes, when the client used SPNEGO/Negotiate WinKerberos errors out in AcquireCredentialsHandle. There seem to be some fundamental difference between the two modes, even though Negotiate uses Kerberos in my case (I made sure I wasn't doing NTLM using wireshark).
There was a problem hiding this comment.
Then it sounds like we should hardcode L"Negotiate" into the AcquireCredentialsHandle call on the server side instead of supporting mechoid? If passing winkerberos.GSS_MECH_OID_KRB5 causes AcquireCredentialsHandle to always fail it doesn't seem very useful. :-)
There was a problem hiding this comment.
Perhaps using GSS_MECH_OID_KRB5 both on the client and server would make AcquireCredentialsHandle happy. Nothing I have tried but I believe AcquireCredentialsHandle would work correctly in that case. I'm unsure if such an implementation is useful though.
There was a problem hiding this comment.
Yeah, probably not useful, since the server side won't know in advance. It's weird that you get SEC_E_INSUFFICIENT_MEMORY. I can't imagine why that would happen or not based on choice of security package.
| PyObject* pyctx = NULL; | ||
| PyObject* serviceobj; | ||
| PyObject* mechoidobj = Py_None; | ||
| WCHAR *service = NULL, *principal = NULL; |
There was a problem hiding this comment.
It looks like principal is unused.
| status = AcquireCredentialsHandleW(/* Principal */ | ||
| NULL, | ||
| /* Security package name */ | ||
| mechoid, |
There was a problem hiding this comment.
See my comment about mechoid on sspi_server_init. This should just be L"Kerberos".
| SecPkgContext_Bindings* sec_pkg_context_bindings = NULL; | ||
| static char *kwlist[] = { "state", "challenge", "channel_bindings", NULL }; | ||
|
|
||
| if (!PyArg_ParseTupleAndKeywords(args, keywds, "Os|O", kwlist, &pyctx, &challenge, &pychan_bindings)) { |
There was a problem hiding this comment.
ccs-pykerberos doesn't support channel bindings on the server side, and I'm not sure how they work in that direction anyway. Let's simplify the implementation and leave channel bindings out.
| inbuf.pBuffers = inBufs; | ||
| inbuf.cBuffers = 0; | ||
|
|
||
| if (sec_pkg_context_bindings != NULL) { |
There was a problem hiding this comment.
See my comments sspi_server_step about channel bindings. I don't think we want / need any of this.
| METH_VARARGS, sspi_client_response_conf_doc}, | ||
| {"authGSSClientUsername", sspi_client_username, | ||
| METH_VARARGS, sspi_client_username_doc}, | ||
| { "authGSSServerUsername", sspi_server_username, |
There was a problem hiding this comment.
It was pointed out in #25 that the function name on the client side should be authGSSClientUserName (Name is capitalized). This should be authGSSServerUserName. See https://github.com/apple/ccs-pykerberos/blob/PyKerberos-1.3.0/pysrc/kerberos.py#L413
| inbuf.cBuffers++; | ||
|
|
||
| /* Get cbMaxToken size for the output buffer */ | ||
| PSecPkgInfo pkgInfo; |
There was a problem hiding this comment.
After what seems like forever I'm finally back working on this. I think the reason you're getting a memory error might be related to this chunk of code. If you don't tell AcceptSecurityContext to allocate buffers for you then you have to allocate the out buffer yourself. The flag to use is ASC_REQ_ALLOCATE_MEMORY (rather than ISC_REQ_ALLOCATE_MEMORY in InitializeSecurityContext). Microsoft doesn't make this super easy to find. I'm working on fixing this up myself.
https://docs.microsoft.com/en-us/windows/desktop/api/sspi/nf-sspi-acceptsecuritycontext
|
I've merged your changes to master and added a few patches on top. Thanks again for the hard work on this. It's really appreciated. Please try master out in your environment. I don't currently have an environment set up to test server side Kerberos, so any testing you can do would be really helpful. |
I've added authGSSServer support for a project I worked on that needed it. It would be nice if it would be accepted and ultimately accessible from pip. It's a pretty rough implementation, though I'm happy to work on it further incase it isn't good enough for a merge.