[SSSD] [PATCHES] PAC responder and client

Jan Zelený jzeleny at redhat.com
Thu Jun 14 14:00:32 UTC 2012


First I'd like to point out that I could not try full functionality since 
there are some pieces missing on the server side. That said, I have couple 
minor comments for some patches.

Patch #0001:                                                                                         
In pac_responder.m4 you are checking for the right version of kerberos. Would 
it be possible to make the check more generic, like "if the version is 1.9 or 
higher"? Otherwise we would need to update this every time new Kerberos is 
out. The error message should be corrected accordingly (right now it only 
specifies Kerberos 1.9 is necessary)                                                                            

idmap_talloc() and idmap_free(): I just noticed that we use practically the 
same functions in ldap provider (sdap_idmap.c). How about moving both 
functions to util/ and utilize them both in ldap provider and pac responder?                                                                                 


Patch #0002:
Ack


Patch #0003:
Nitpick: pac_store_memberships() should be more accuratelly called 
pac_store_membership()

> > You are not freeing pr_ctx in some error cases. The only place pr_ctx is
> > free'd is pac_save_memberships_done(), i.e. only when everything went ok.
> > 
> > Please add some talloc_free() calls where necessary. My first guess is
> > that pac_get_domains_done() and pac_add_pac_user() are the only two
> > places where it is necessary.
> 
> I have added a talloc_free() at both places. But since pr_ctx is a child
> of the client context which is freed shortly afterwards one might argue
> if the talloc_free() is even needed in pac_save_memberships_done(). But
> I would agree with you that it is a good idea to remove pr_ctx when it
> is not needed anymore.

Just for the record, I don't think the cctx context is freed before responder 
starts shutting down. That's why I asked you to free those pr_ctx contexts.


Patch #0004:
Please add build dependencies (conditionally of course) to our example spec 
file.

Patch #0005:
Ack

Patch #0006:
Ack

Patch #0007:
Are there any changes that should be done in spec file for this?

Nitpick: the "if" statement on line 361 of the patch is missing curly braces. 
The same on line 425.

Thanks
Jan



More information about the sssd-devel mailing list