5:20 p.m.
Sumit Bose <sbose(a)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