On Thu, Oct 18, 2012 at 05:47:19PM -0400, Simo Sorce wrote:
On Thu, 2012-10-18 at 17:12 -0400, Simo Sorce wrote:
> On Thu, 2012-10-18 at 22:36 +0200, Sumit Bose wrote:
> > Hi,
> >
> > I'm sorry but this patch series is a bit longer than I originally
> > expected. While testing password authentication I came across some
> > issues and inconsistencies, some of them were also already seen by
> > Steeve during his testings. And since they may prevent successful
> > password authentication I put the patches in the series as well. I also
> > tried to make the patches as small and self-contained as possible to
> > make the review easier.
> >
> > [PATCH 01/14] subdomain-id: Generate homedir only for users
> > - this is the only patch not strictly related to password
> > authentication, it remove a useless operation for group requests
> >
> > [PATCH 02/14] s2n: save user principal name of sub-domain user
> > - if the user is looked up via the extdom exop and the IPA server the
> > user principal name is generated and store in the cache so that it
> > can be used for password authentication
> >
> > [PATCH 03/14] pac responder: fix copy-and-paste error
> > - this error prevent proper id-mapping in the PAC responder
> >
> > [PATCH 04/14] sysdb: look for ranges in the parent tree
> > - always use the right sub-tree in the cache to search for ranges
> >
> > [PATCH 05/14] pac responder: use only lower case user name
> > - since the extdom exop uses winbind to do the SID to name mapping and
> > winbind only returns names lower case we have to lower case the names
> > in the PAC responder as well to avoid inconsistent behaviour
> >
> > [PATCH 06/14] pac responder: add user principal to cached user
> > - same as patch 0002 but for the case the user is stored by the PAC
> > responder
> >
> > [PATCH 07/14] Accept be_req instead if be_ctx in krb5 auth provider
> > - this patch was written by Jan some time ago and makes the same
> > changes to the auth provider which were already done for other
> > provider to better handle the needed data for different sub-domains
> >
> > [PATCH 08/14] sysdb: add sysdb_base_dn()
> > - this just adds a helper which is needed by patch 9
> >
> > [PATCH 09/14] check_ccache_files: search sub-domains as well
> > - this fixes a todo in Jan patch, now the renewal task checks the
> > ccache for sub-domain users as well
> >
> > [PATCH 10/14] Detect subdomain request in IPA auth provider
> > - checks if the auth request is for a subdomain and creates that
> > creates the needed structs to access the cached data for the
> > sub-domain
> >
> > [PATCH 11/14] Add replacement for krb5_find_authdata()
> > - add a replacement for a call only available in MIT Kerberos 1.10,
> > currently it returns only and error. Is it worth to implement some
> > basic functionality for platforms with older MIT versions?
> >
> > [PATCH 12/14] krb5_auth: check if principal belongs to a different
> > - add a flag if the principal used for authentication does not belong
> > to our realm
> >
> > [PATCH 13/14] krb5_auth: send different_realm flag to krb5_child
> > - send the flag from patch 12 to the krb5-child
> >
> > [PATCH 14/14] krb5_child: send PAC to PAC responder
> > - look for a PAC and send it to the PAC responder if the principal does
> > not belong to our realm
>
> Sumit you have nice comments here, and none in the git commits,
> Can you please rebase the patchset and add nice comments in the actual
> commits.
> They will come handy much after this review has been done and buried in
> the archives when someone is looking for info on why a particular change
> has been done and looks into git log/git blame.
Also nack to patch 0007, as far as I can understand reading the code and
discussing on IRC it seem the only reason you are changing to use be_req
is to use a different sysdb struct from what is available in be_ctx.
In this case though creating a whole fake be_req makes it completely
opaque, it took me asking in person why that was done. This is bad,
because proliferating fake/half filled functions is a recipe for issues.
So NACK.
Please add a 'subdom_sysdb' argument to the function instead (you can
also replace be_ctx with a list of explicit arguments needed), and do
not create a fake struct be_req.
Also if this kind of scheme has been used elsewhere I would ask you to
consider fixing those places as well.
Creating fake structures is confusing, makes reading the code hard,
makes using those structures dangerous as you do not know if they are
real or fake, and generally masks what arguments are really needed by a
function.
I am actually regretting allowing to put the domain struct in sysdb
which is the real reason all this dance is being made. I never like it
as it is a bit of a layer violation, however it would be a major effort
to change that, so I will just end the rant here and now :)
Simo.
Thank you for your comments. I added the comments to the commits and
changed the approach used in patch 0007 which also made patch 0010
obsolete. All other patches are unchanged, but I had to add a few more.
The new patch 0014 just fixes a minor memory hierarchy issue. Patch
15-18 make sure we have a principle with the right case in the cache for
AD users.
bye,
Sumit