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.