POC for Kerberos on Windows with SSPI #20
Conversation
Merge from Valeri Karpov.
This adds support for ServiceHost on a GSSAPI authentication.
| } | ||
|
|
||
| // Make sure we've cleaned up all the buffers we malloced when we're sure we don't need em anymore | ||
| if ss.authComplete || ss.errored { |
There was a problem hiding this comment.
If this is being closed, why would buffers not be freed? Close should mean "stop doing everything you are doing and free all resources".
There was a problem hiding this comment.
Got some weird errors because Close gets called after every step. Figured this would be a reasonable way to not dive down the windows rabbit hole while avoiding leaking memory.
There was a problem hiding this comment.
I don't think I understand the issue. Close cannot be called after a step unless the saslSession is not going to be touched anymore, and if that's the case its resources should be released or we do have leaks. If there are lurking errors, we should understand them as there's likely a fundamental issue in the way we're using the API.
|
|
||
| // Call the function | ||
| return (*pfn_queryContextAttributes)( | ||
| phContext, ulAttribute, pBuffer |
There was a problem hiding this comment.
We can have it all in a single line.
|
Thanks for the work, Valeri. A few initial comments above. |
|
Sorry, wrong button.. didn't mean to close it. |
|
In general, I'd rather not make too many changes to |
|
I'm all for having less code that we need to maintain, but the proper way to do that is to use an external library, which is the process I have followed to implement the current support in Linux, and which hopefully actually works in Windows too (I never got a proper response to whether it does work there or not, and why not, given that the library does mention support for Windows). I'd be quite surprised if there's no proper library that abstract that exact API for multiple operating systems. That said, if you prefer implementing it that way, and supporting it too, that's okay too. But if we are going down the rabbit role of importing external code, we'll need to maintain that code sane and working for us. That includes file headers, fixing things such as the loose error handling, adding tests, and on the bright side getting rid of code that we're in fact not depending upon. Looks like we can get rid of quite a bit of it. |
|
I don't know the exact reasoning why Cyrus SASL doesn't work for Kerberos on Windows. Probably due to the fact that Windows-flavored Kerberos enables you to get a TGT ticket in the process itself without going through Given the sorry state of go's package management, I think its a better idea to keep the code in there. I'll go in and make kerberos_sspi conform to some reasonable stylistic standards. |
|
Having experience with other lands, I don't find Go's package management to be in a sorry state, but let's have that conversation in a different forum. I'm happy to support you in merging this work nevertheless. Let's just try to polish it to a state where we're both happy to maintain it in-tree. |
|
Thanks. Let me know if you have any more comments on the most recent iteration, I did what I could to clean up the kerberos_sspi files. |
This cuts down the timing further to 22.5ns/op, or 1/300th of the original.
|
Going over the comments above, I still see a number of uncovered points. One easy way to ensure we're on the same page is by responding to the actual comments, even if it's just a "Done." This gives a chance for both of us to tell whether things were completed, or whether there was a disagreement, and what was it. |
|
Also, can we please drop the "_" prefix from all of the C function names? These are just normal functions in our own namespace. |
This prevents blocks of 512k from being allocated to hold the 256k data field plus metadata. Fixes MGO-28.
Introduces a few stubs for non-database tests to pass in Windows.
I added 5 new files:
kerberos_sspi.*: these are thin wrappers around windows system calls that help with loading DLLs, they're copied exactly from the node driver's SSPI implementationsasl_sspi.c,sasl_sspi.h: some glue to make it easier to use the windows system calls from Go. Mostly, its just a C adaptation of Eric Milkie's implementation for the mongo shellsasl_sspi.go: exposes the same API assasl.gobut uses Windows SSPI instead of SASL.Still a couple outstanding issues, most notably that 1) I haven't added tests yet (see #12, among other limitations which we can discuss), and 2) only ANSI support for now, no unicode, because for some reason my testing setup is blowing up when I try to use unicode.