On Wed, Jun 05, 2013 at 09:51:04AM +0200, Lukas Slebodnik wrote:
> On (04/06/13 22:37), Jakub Hrozek wrote:
> >On Tue, Jun 04, 2013 at 08:46:44PM +0200, Sumit Bose wrote:
> >> On Tue, Jun 04, 2013 at 03:25:12PM +0200, Jakub Hrozek wrote:
> >> > On Mon, Jun 03, 2013 at 11:58:02AM +0200, Jakub Hrozek wrote:
> >> > > On Fri, May 31, 2013 at 04:01:51PM +0200, Jakub Hrozek wrote:
> >> > > > On Fri, May 31, 2013 at 03:47:32PM +0200, Jakub Hrozek
wrote:
> >> > > > > On Fri, May 31, 2013 at 10:59:21AM +0200, Lukas
Slebodnik wrote:
> >> > > > > > On (29/05/13 18:39), Jakub Hrozek wrote:
> >> > > > > > >Hi,
> >> > > > > > >
> >> > > > > > >the attached patches implement lookups in
Global Catalog and AD
> >> > > > > > >subdomains. Unfortunately they turned out to be
quite complex and I'd like
> >> > > > > > >to make them shorter and remove some of the
complexity during the review
> >> > > > > > >if possible.
> >> > > > > > >
> >> > > > > > >The patches implement identity lookups,
authentication. There will be
> >> > > > > > >additional patch that implements SRV discovery,
but I wanted to get
> >> > > > > > >the patches reviewed so that I don't waste
time if the approach was not
> >> > > > > > >correct. Authentication currently requires the
subdomain to be present in
> >> > > > > > >krb5.conf otherwise the Kerberos libraries
would search for _kerberos-master,
> >> > > > > > >not _kerberos and fail. Sumit, who kindly
helped me debug this problem
> >> > > > > > >might already have a MIT bug handy.
> >> > > > > > >
> >> > > > > > >There is couple of LDAP provider patches that
instead of using a single
> >> > > > > > >connection towards LDAP allows for a linked
list in the sdap_id_ctx.
> >> > > > > > >Then the caller of the LDAP ID function can
choose himself what
> >> > > > > > >connection he would like to use for the
particular lookup. For instance,
> >> > > > > > >for trusted AD users, GC lookups are always
used, but for native AD
> >> > > > > > >users, LDAP can be used (and is actually
preferred).
> >> > > > > > >
> >> > > > > > >One part of the complexity comes from the fact
that initially I wanted
> >> > > > > > >the ID code to be flexible and allow
"failover" between the connection.
> >> > > > > > >In other words, the ID code could try GC lookup
first and optionally
> >> > > > > > >fail over to LDAP lookup using the same context
if the entry was not
> >> > > > > > >found using the first connection.
> >> > > > > > >
> >> > > > > > >But after testing the identities I'm not
sure if that is actually needed. The
> >> > > > > > >trusted users and groups have to be looked up
from GC anyway and the local
> >> > > > > > >users and groups would be available in LDAP.
Maybe we could conserve
> >> > > > > > >some resources by attempting to look up local
users from GC and only
> >> > > > > > >keep one connection open when possible, but
I'm not quite sure it is
> >> > > > > > >worth it. Removing the connection failover
would get rid of having to
> >> > > > > > >report the LDAP return code to AD identity
functions and maybe we could
> >> > > > > > >even tie the connection with sdap_domain, too.
> >> > > > > > >
> >> > > > > > >So can anyone think of a lookup where we would
like to try one
> >> > > > > > >connection and then the other? Maybe groups
that contain members from
> >> > > > > > >both primary and trusted domains might be such
a case, but there we
> >> > > > > > >could handle the failover in the group
request.
> >> > > > > > >
> >> > > > > > >[PATCH 01/15] Do not obfuscate calls with
booleans
> >> > > > > > >This is purely a personal preference, but I
found it hard to follow what
> >> > > > > > >the various booleans meant when reading
function calls. Maybe macros
> >> > > > > > >would make the code more readable, but I'm
OK with not applying this
> >> > > > > > >patch if others disagree.
> >> > > > > > >
> >> > > > > > >
> >> > > > > > >
> >> > > > > > >[PATCH 02/15] LDAP: sdap_id_ctx might contain
several connections
> >> > > > > > >With some LDAP server implementations, one
server might provide different
> >> > > > > > >"views" of the identites on different
ports. One example is the Active
> >> > > > > > >Directory Global catalog. The provider would
contact different view
> >> > > > > > >depending on which operation it is performing
and against which SSSD domain.
> >> > > > > > >
> >> > > > > > >At the same time, these views run on the same
server, which means the same
> >> > > > > > >server options, enumeration, cleanup or
Kerberos service should be used.
> >> > > > > > >So instead of using several different failover
ports or several instances
> >> > > > > > >of sdap_id_ctx, this patch introduces a new
"struct sdap_id_conn_ctx" that
> >> > > > > > >contains the connection cache to the particular
view and an instance of
> >> > > > > > >"struct sdap_options" that contains
the URI.
> >> > > > > > >
> >> > > > > > >No functional changes are present in this
patch, currently all providers
> >> > > > > > >use a single connection. Multiple connections
will be used later in the
> >> > > > > > >upcoming patches.
> >> > > > > > >
> >> > > > > > >
> >> > > > > > >
> >> > > > > >
> >> > > > > > src/providers/ldap/ldap_init.c: In function
'sssm_ldap_id_init':
> >> > > > > > src/providers/ldap/ldap_init.c:188:21: warning:
'ctx' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >> > > > > >
> >> > > > > > >[PATCH 03/15] LDAP: Refactor account info
handler into a tevent request
> >> > > > > > >The sdap account handler was a function with
its own private callback
> >> > > > > > >that directly called the back end handlers.
This patch refactors the
> >> > > > > > >handler into a new tevent request that the
current sdap handler calls.
> >> > > > > > >
> >> > > > > > >This refactoring would allow the caller to
specify a custom sdap
> >> > > > > > >connection for use by the handler and
optionally retry the same request
> >> > > > > > >with another connection inside a single
per-provider handler.
> >> > > > > > >
> >> > > > > > >No functional changes are present in this
patch.
> >> > > > > > >
> >> > > > > >
> >> > > > > > src/providers/ldap/ldap_id.c: In function
'sdap_handle_acct_req_done':
> >> > > > > > src/providers/ldap/ldap_id.c:1206:9: error:
implicit declaration of function 'sdap_get_user_and_group_recv'
[-Werror=implicit-function-declaration]
> >> > > > >
> >> > > > > [snip]
> >> > > > >
> >> > > > > Thank you I will work on a respin, but it should be
noted that these
> >> > > > > warnings and even errors occur only when compiling
patches individually.
> >> > > > > Applying the whole set would make the patches compilable
and testable.
> >> > > >
> >> > > > Attached are rebased patches on top of the current master. I
will work
> >> > > > on making the patches compilable individually but the
attached gzipped
> >> > > > patches should unblock the review now.
> >> > >
> >> > > Attached is another rebase on top of the current master. I also
> >> > > confirmed that using krb5-libs-1.11.2-10.fc19.x86_64 I no longer
needed
> >> > > to define the subdomain's master_kdc in krb5.conf.
> >> >
> >> > Attached is the same set of patches, just fixed to address Lukas'
> >> > comments. The patches can now be compiled separately.
> >>
> >> did you attach the right tar ball?
> >
> >Not sure, I thought I did.
> >
> >> I still can see the two issues in
> >> ldap_id?
> >>
> >
> >I fixed one -Wmaybe-uninitialized warning, do you see some other?
> >
> >I pushed the latest patches into my
fedorapeople.org branch:
> >git://fedorapeople.org/home/fedora/jhrozek/public_git/sssd.git
> >
> >The branch is called "gc". A tarball is also attached to be able to
> >access the archived versions of work-in-progress.
>
> I wrote it in my previous mail. There is one error, even in your branch
> from fedorapeople git repo.
>
> src/providers/ldap/ldap_id.c: In function 'sdap_handle_acct_req_done':
> src/providers/ldap/ldap_id.c:1280:9: error: implicit declaration of function
'sdap_get_user_and_group_recv' [-Werror=implicit-function-declaration]
> cc1: some warnings being treated as errors
>
> Solution is very simple and I don't think it is stop-blocker for review
> process.
It seems that problem is with fedora 19 (gcc, libtool or ...)
I tried to recompile srpm in mock.
mock --root fedora-19-x86_64 --rebuild --resultdir f19/
sssd-1.9.93-0.20130605.1034.gitd09ea20.fc18.src.rpm
And packages were built successfully.
But in fedora 18 process failed.
yes, and if it is fixed in both cases I still see
../src/providers/ldap/ldap_id.c: In function 'sdap_handle_acct_req_send':
../src/providers/ldap/ldap_id.c:1241:20: warning: 'be_ctx' may be used
uninitialized in this function [-Wuninitialized]
../src/providers/ldap/ldap_id.c:1075:20: note: 'be_ctx' was declared here
bye,
Sumit
>
> diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
> index b29ab99..5824567 100644
> --- a/src/providers/ldap/ldap_id.c
> +++ b/src/providers/ldap/ldap_id.c
> @@ -1242,6 +1242,9 @@ done:
> return req;
> }
>
> +errno_t sdap_get_user_and_group_recv(struct tevent_req *req,
> + int *dp_error_out, int *sdap_ret);
> +
> static void
> sdap_handle_acct_req_done(struct tevent_req *subreq)
> {
>
> LS