On Sun, Jun 17, 2012 at 06:47:05PM -0400, Simo Sorce wrote:
> On Sun, 2012-06-17 at 11:38 +0200, Sumit Bose wrote:
> > On Thu, Jun 14, 2012 at 04:00:32PM +0200, Jan Zelený wrote:
> > > 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.
> >
> > Thank you for the review. Please find new patches attached. I added two
> > now patches to support the new ID range objects on the IPA server. I
> > didn't squash them into the other patches to hopefully make review
> > easier.
> >
> > bye
> > Sumit
> >
> > > 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)
> >
> > Corrected the message. But I would like to keep the current scheme of
> > detecting the MIT Kerberos version, because the used plugin interface is
> > currently not public (maybe in 1.11) and it should be carefully checked
> > if everything is working with new versions of MIT Kerberos.
> >
> > > 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()
> >
> > renamed
> >
> > > > > 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.
> >
> > added
> >
> > > Patch #0005:
> > > Ack
> > >
> > > Patch #0006:
> > > Ack
> > >
> > > Patch #0007:
> > > Are there any changes that should be done in spec file for this?
> >
> > no, MIT Kerberos libraries are all that is needed
> >
> > > Nitpick: the "if" statement on line 361 of the patch is missing
curly
> > > braces. The same on line 425.
> >
> > added braces
>
> Just giving a quick look.
thank you.
I forgot to attach the very first patch, so patch number no differ by 1.
> 0. Can you add more comments on what the various functions are supposed
> to do ?
comments added
> 1. Patch 0002 is *really* big, I would split it in smaller pieces.
I moved all the utility calls into 'PAC responder: add some utility
functions' and also into the pacsrv_utils.c file.
> 2. In patch 0002 leave the gecos empty if there is no full name.
done
> 3. In pac_add_pac_user you seem to be trusting the logon_domain w/o any
> check. I would rather we verify it is valid wrt the realm of the
> principal that owns the PAC. Otherwise technically one trust domain
> could try to attack another by giving us a same named user and injecting
> a domain name of a different trusted domain.
we talked about this on irc and agreed that this check should be done by
the KDC before resigning the PAC. I opened ticket
https://fedorahosted.org/freeipa/ticket/2849 to track this.
> 4. In pac_add_pac_user() you are returning pac_cmd_done() whether that
> whole funciton completed or had to fork off async requests, is that
> intentional ?
yes, as done in other responders as well pac_cmd_done() returns to the
main loop when called with EAGAIN as second parameter.
> 5. SSS_PAC_ADD_PAC_USER = 0x00E9,
>
> Make this 0x0101
done
> Simo.
new versions attached.
All issues have been addressed. If noone else has any objections, ACK.
Jan