[SSSD] [PATCHES] PAC responder and client

Simo Sorce simo at redhat.com
Sun Jun 17 22:47:05 UTC 2012


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.

0. Can you add more comments on what the various functions are supposed
to do ?

1. Patch 0002 is *really* big, I would split it in smaller pieces.

2. In patch 0002 leave the gecos empty if there is no full name.

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.

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 ?

5. SSS_PAC_ADD_PAC_USER     = 0x00E9,
  Make this 0x0101


Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list