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:
> >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
> >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.
> >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
> >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"
> >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
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.