-
Notifications
You must be signed in to change notification settings - Fork 65
Conversation
I couldn't get an instance of CalendarServer up and running and interacting with LDAP so I did the next best thing and just create a site backed by Apache and mod_auth_kerb. Here is the testing setup info and details if you wish to test it manually yourself. Kerberos Setup/etc/krb5.confThis is my Kerberos config setup and configured to connect to the
SPNI have manually set the SPN KeytabThis is my keytab setup at Apache SetupApache Kerberos ConfigMy Kerberos config at
I also just have a basic HTML site configured on port 80 at `/etc/httpd/sites-available/centos01.jordan.local.conf
Test ScriptThis is the script I used to test out all 4 existing tests with the Kerberos backed website. It tests it with both Python 2.7.13 and Python 3.6.1. I had to modify the existing tests to support both Python 2 and 3 (committed) and modify the principal to work outside of CalendarServer (just done through a sed line replacement)
Test OutputThis is the test output from the script
Hopefully this manual test and my test with a CBT endpoint as shown in the first post helps to alleviate any concerns about backwards compatibility. If you have anything else you want me to try or do differently just ask and I should be able to do it. |
@dreness I hope you were the person I was talking to in IRC, just hoping the above information is good enough to cover the testing side of things. Not sure what you would like me to do next. |
@jborean93 I am. Your testing efforts have been substantial and are well received. I will dig into this and get some comments from others, and can hopefully merge this soon. |
Thanks for that, I might spend some time on another PR soon to get integration with travis to make this easier in the future but it all depends on how much time I have. |
This looks good -- one question: for this malloc... input_chan_bindings = (struct gss_channel_bindings_struct *) malloc(sizeof(struct gss_channel_bindings_struct)); Who is responsible for freeing it? Is it handled by Python object reference counting? |
I'm not sure on that (my inexperience shining through) but I suppose after the library using this get's a successful authentication it should free it. Is there a way to do this in python or do I need to pass in the object to another method on the C side? |
I am not familiar with the Python/CObjects API, but I do see in the docs that PyCObject_FromVoidPtr( ) can take a destructor function which gets called with the python object is reclaimed. Perhaps it needs to be passed a function that does the free? We need to get someone more familiar with this to chime in. |
I'll do some research and will see what I can find. |
I'm trying to get this loaded up in xcode so I can point the clang static analyzer at it, which should help identify lifecycle issues. |
So I think you want to pass "free" to PyCObject_FromVoidPtr, e.g.: pychan_bindings = PyCObject_FromVoidPtr(input_chan_bindings, free); |
I received a hot tip from another frog right around the time that a similar conclusion was reached by m0rgen: If "free" is passed instead of NULL, that should free the C-side object. Doing some testing on that... |
So if I pass free to that Python will automatically free the memory or do I still have to call that somewhere else when ready? |
The python object is reclaimed automatically, and this "destr" argument is a facility for allowing that reclamation to flow back to the C-side, by the C. sure. (cough). The argument itself is the function that is called (with one argument: a pointer to the thing being freed) when the python object is reclaimed, so to do the moral equivalent on the C-side, we can just pass 'free'. |
Made those changes plus I bumped the version. I checked with my original test and the CBT stuff still works after the test. |
src/kerberos.c
Outdated
} | ||
|
||
input_chan_bindings = (struct gss_channel_bindings_struct *) malloc(sizeof(struct gss_channel_bindings_struct)); | ||
pychan_bindings = PyCObject_FromVoidPtr(input_chan_bindings, free); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Python 3.x, PyCObject_FromVoidPtr is an alias for PyCapsule_New (see line 40). The destructor passed as the second argument here will end up being called on the PyCapsule itself, not the gss_channel_bindings_struct. That's almost certainly not what you want.
https://docs.python.org/3.1/c-api/capsule.html#PyCapsule_New
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So do I need to do something like line 109 at https://github.com/02strich/pykerberos/blob/master/src/kerberos.c where I free the struct in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example that deals with both python 2 and 3:
https://github.com/02strich/pykerberos/blob/v1.1.9/src/kerberos.c#L102-L116
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I'll give it a shot and will hopefully get it working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, just to say the words out loud for my own benefit: the differences between python 2 and 3 for instantiating python objects are abstracted by the macros near the top of src/kerberos.c, and the destructor function is conditionally defined based on the python version.
It would be really helpful if you could update the docstring for authGSSClientStep with the new parameter and an example of how to use it here: https://github.com/apple/ccs-pykerberos/blob/master/pysrc/kerberos.py |
src/kerberos.c
Outdated
static char *kwlist[] = {"initiator_addrtype", "initiator_address", "accept_addrtype", | ||
"acceptor_address", "application_data", NULL}; | ||
|
||
if (!PyArg_ParseTupleAndKeywords(args, keywds, "|is#is#s#", kwlist, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the PyCObject / PyCapsule used to hold the binding struct may outlive the Python strings used to pass in the addresses, I think you need to use es# here, and call PyMem_Free on each string in your destructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I need to free all the char* that can possibly be used in the structure in what I have just added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, specifically using PyMem_Free, since PyArg_ParseTuple allocates the storage for you. The thing that worries me about using s# is that PyArg_ParseTuple just gives you a pointer to the internal storage of the Python string / unicode object. If that object's ref count falls to zero while the application is still using the binding object you'll have a problem.
The C API docs for python 3.6 make this clear:
In general, when a format sets a pointer to a buffer, the buffer is managed by the corresponding Python object, and the buffer shares the lifetime of this object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to use et#
instead as we are actually passing in a byte string not a unicode string. Hopefully the changes I've put through are correct I'm going into uncharted territory for me.
https://docs.oracle.com/cd/E19455-01/806-3814/overview-52/index.html | ||
|
||
@param initiator_addrtype: Optional integer used to set the | ||
initiator_addrtype, defaults to GSS_C_AF_UNSPEC if not set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'll also want to add module constants for GSS_C_AF_UNSPEC and whatever other values are valid for *addrtype, similar to the definitions for things like AUTH_GSS_COMPLETE, GSS_C_DELEG_FLAG, etc. here:
https://github.com/apple/ccs-pykerberos/blob/PyKerberos-1.2.5/src/kerberos.c#L802-L835
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, there are a lot of options...
http://www.shrubbery.net/solaris9ab/SUNWdev/GSSAPIPG/p27.html#REFERENCE-9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I was thinking about that when doing the docs for this, will update it accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in the latest commit.
Looking good! @jborean93 - do you still have the test environment available? If so, would it be possible for you to run through the testing and validation again with all the recent changes applied? |
Yep I just reran the original tests and the WinRM test and here is the output. Also is there a way of having a collapsable block of text in Github?
|
In buildChannelBindingsStruct the "et#" format is not being used properly. This causes warnings when compiling: src/kerberos.c:272:51: warning: incompatible pointer to integer conversion assigning to 'size_t' (aka 'unsigned long') from 'int *' [-Wint-conversion] I've attached a diff to correct this. Please run all your tests again after this fix. diff --git a/src/kerberos.c b/src/kerberos.c
|
Ugh! My diff got messed up. Let's try again:
|
Some other things:
Slightly better than having to always append "[1]" to the channelBindings result to get the py object. |
@cyrusdaboo thanks for the feedback, I've made the changes you have requested and commit them. I also reran the tests and it still works fine. Please let me know if you wish for any more changes. |
Merged in the latest changes from the master branch and pushed them to this branch. Looks like the travis build is working fine. Please let me know if there is anything else you need from me. |
@dreness @cyrusdaboo any chance you can have another look, I am hoping to get a consensus with this PR and the Windows equivalent mongodb/winkerberos#17 and I am nearing the end of that PR. |
@jborean93 I think we're looking good here. I'm going to ping one more possibly interested stakeholder, but personally I'm satisfied. |
Thanks again @jborean93 - very nice job on this PR :) |
Thanks @dreness and others who helped with this PR. |
Added support for creating an input channel bindings value and passing that into the authGSSClientStep as an optional argument. Will continue on with the default behavior if this isn't specified.
I'm fairly new to C so happy to take any pointers and ideas on ways this could be better improved. I've tested this manually on an IIS and WinRM endpoint where the channel bindings token is set to required and verified it worked.
Test Result:
This is the environment set up I had to verify the test results, please ask me any questions around it or if you know of a better way to add automated tests.
Windows Configuration
Run this script on a Server 2008 or newer host that is hooked up to a domain https://github.com/ansible/ansible/blob/devel/examples/scripts/ConfigureRemotingForAnsible.ps1. This will set up a WinRM listener over HTTPS with a self signed cert and open and firewall rules
After running this run the following command in Powershell to set the CBT policy to Strict
You can verify the setting by running this in powershell
Python Host Config
On the *nix server you are using for testing run the following command to install the relevant libraries and set everything up
Create the python script below
Run the following commands to get a kerberos ticket and test out the script
You get the Windows IP configuration returned
If you are using the stock standard kerberos and requests-kerberos library you would get the following error returned