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