[SSSD] [PATCHES] PAC responder and client

Jan Zelený jzeleny at redhat.com
Thu Jun 21 14:55:58 UTC 2012


Dne pondělí 18 června 2012 16:34:23, Sumit Bose napsal(a):
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120621/ee67b46e/attachment.sig>


More information about the sssd-devel mailing list