[SSSD] [PATCH] Add experimental support to receive PAC via GSSAPI

Jan Zeleny jzeleny at redhat.com
Fri Jul 1 22:20:25 UTC 2011


Sumit Bose <sbose at redhat.com> wrote:
> On Tue, Jun 07, 2011 at 02:14:56PM +0200, Jan Zelený wrote:
> > > Hi,
> > > 
> > > this series of patches adds support to receive a windows PAC via GSSAPI
> > > and to create a user based on the data in the PAC. This is useful
> > > because in an environment with lots of trust relationships between AD
> > > server it might be quite time consuming to find out about all the group
> > > memberships of a domain user by querying the domain controllers. But
> > > the PAC contains all information about group memberships of the
> > > corresponding user.
> > > 
> > > The general idea is to add the user, if it doesn't exist in the cache,
> > > to the cache of the corresponding domain (see thread about sub-domains,
> > > currently this patch add the user to the local domain for simplicity)
> > > and to add all group memberships (currently not implemented). If one of
> > > the groups cannot be found in the cache a dummy entry with all data
> > > needed to resolve this group quickly is added to the cache.
> > > 
> > > Currently there are a couple of loose end, e.g.
> > > - groups and group memberships are not handled
> > > - PAC is not validated
> > > - missing sub-domains
> > > - no real SID to uid/gid mapping
> > > 
> > > etc, but I like to start the discussion about the code and the general
> > > direction as soon as possible. Currently sssd with these patches can
> > > only be build on rawhide, because of the dependencies to the samba4
> > > package (libndr-krb5pac).
> > > 
> > > Patch 0007 contains a little example that demonstrates that the pac
> > > responder can also be used to add user and groups based on other input,
> > > e.g. it can be used as a backend for the sss_* utilities. This would
> > > allow a much better control about which user is allowed to do what kind
> > > of operation. Currently only the root user can add and modify user and
> > > group entries with the sss_* tools.
> > > 
> > > I have used 'pac' as a part of names here because this was the original
> > > target, but I would be happy to change it to a more generic keyword if
> > > anyone has a good suggestion.
> > > 
> > > bye,
> > > Sumit
> > 
> > Hi,
> > 
> > I'm sending some comments based on code review (I didn't actually try the
> > code). I skipped all autotools parts, since it's not my area of expertise
> > and it still makes my eyes bleed.
> > 
> > 0002
> > - pac_get_config() seems redundant
> 
> removed, we can easily add it if we start using config options
> 
> > - what is the pac_ctx structure for? Is it there for some future
> > 
> >   extensibility? So far it seems to be redundant
> 
> yes, there is a similar context in the pam and nss responders and I
> expect that we will have more data in the context in future.
> 
> > 0004
> > - would it be possible to wrap those unbelievably long and lines in
> > 
> >   pac_add_pac_user() in some kind of macro or function?
> > 
> > - the block on lines 149-156 doesn't make any sense (should the last line
> > 
> >   be deleted)
> 
> I have restructured this file a bit and hope it makes the intention more
> clear now.
> 
> > 0005
> > - as we discussed on IRC, functions sssdpac_externalize() and
> > 
> >   sssdpac_internalize() might be redundant. If they are mandatory, please
> >   try to find out if it's ok just to place a return in them, it would be
> >   better I think
> > 
> > - the sssdpac_size() function is mandatory?
> 
> all three are mandatory and have to return to data, because all three
> are used as a substitute is the copy call, which is optional, is not
> available. See src/lib/krb5/krb/authdata.c in the MIT sources starting
> at line 960 for details.
> 
> > - can you just briefly explain, what does the flag AD_USAGE_TGS_REQ in
> > 
> >   sssdpac_flags() mean. It's little confusing for me
> 
> ah, thanks, AD_USAGE_TGS_REQ is wrong here. I have changed it to
> 'AD_USAGE_KDC_ISSUED | AD_INFORMATIONAL' which means that we are
> interested in data issued by the KDC and the caller must not fail if an
> error occurs. With AD_USAGE_TGS_REQ the client can add data in the TGS
> request. See http://k5wiki.kerberos.org/wiki/Projects/VerifyAuthData for
> some more information.
> 
> > 0007
> > - as I pointed out before, I'm against implementing functions which won't
> > be
> > 
> >   used
> > 
> > - considering we wanted to push the patch anyway, what is the purpose of
> > 
> >   pac_cmd_handler() function? I mean why don't we call pac_putpwent() and
> >   other similar functions directly?
> 
> I dropped the patch completely for now but I keep it in my tree. If
> someone finds a good use case I can update it and send it again.
> 
> > Thanks
> > Jan
> 
> Thank you very much for the review.
> 
> bye,
> Sumit


All patches look much better now (the only issue I found was in patch 4 on 
lines 103 and 104 - you have two returns there), although I didn't have time 
review them very closely nor test them. If noone does the close review in the 
next two weeks, could you please sent me some tips how to set up testing 
environment? I'd like to do the full-scale review as soon as I'm back from 
vacation.

Thanks
Jan



More information about the sssd-devel mailing list