[SSSD] [PATCHES] LDAP: Fix group handling with AD schema

Jakub Hrozek jhrozek at redhat.com
Wed Jan 29 19:12:54 UTC 2014


On Wed, Jan 29, 2014 at 05:40:56PM +0100, Lukas Slebodnik wrote:
> On (28/01/14 23:10), Jakub Hrozek wrote:
> >On Mon, Jan 27, 2014 at 09:46:26AM +0100, Lukas Slebodnik wrote:
> >> ehlo,
> >> 
> >> two patches are attached.
> >> The 1st one is almost he same like patch:
> >> commit 16b27fcceebcbbaeefaf5b9bdf2dec3065adba4a
> >>     LDAP: Don't fail if subdomain cannot be found by sid
> >> 
> >> I didn't notice that similar change was done in two separeted patches.
> >> 
> >> 
> >> I am not sure if 2nd is right solution, because some conditions are litle bit
> >> confusing for me.
> >> 
> >> LS
> >
> >> From 6fb4a41e7340f48565d5d81f079e9861b5b3c71e Mon Sep 17 00:00:00 2001
> >> From: Lukas Slebodnik <lslebodn at redhat.com>
> >> Date: Fri, 24 Jan 2014 17:03:27 +0100
> >> Subject: [PATCH 1/2] LDAP: store group if subdomain cannot be found by sid
> >> 
> >> Domain needn't contain sid if id_provider is ldap.
> >> With enabled id mapping, group couldn't be stored, because domain
> >> couldn't be found by sid.
> >> 
> >> Resolves:
> >> https://fedorahosted.org/sssd/ticket/2172
> >> ---
> >>  src/providers/ldap/sdap_async_groups.c | 10 ++++++----
> >>  1 file changed, 6 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c
> >> index 4ae772636863acf4c1fd59a20a20d1ad3a28ace0..55813c0fe4ffe864e1725959ad2a6ff51b1ffc65 100644
> >> --- a/src/providers/ldap/sdap_async_groups.c
> >> +++ b/src/providers/ldap/sdap_async_groups.c
> >> @@ -450,6 +450,7 @@ static int sdap_save_group(TALLOC_CTX *memctx,
> >>      bool posix_group;
> >>      bool use_id_mapping;
> >>      char *sid_str;
> >> +    struct sss_domain_info *subdomain;
> >
> >I know that the function is called 'find_subdomain_by_sid' but if I read
> >its code right, it can return also the main domain and is actually used
> >this way on a couple of places. So I suggest we rename the function in a
> >separate patch. Similarly, this variable shouldn't be called
> >'subdomain', but maybe 'sid_dom' or 'group_dom' ?
> >
> >(the same misnomer seems to be in save_user)
> >
> >>      int32_t ad_group_type;
> >>  
> >>      tmpctx = talloc_new(NULL);
> >> @@ -488,11 +489,12 @@ static int sdap_save_group(TALLOC_CTX *memctx,
> >>      /* If this object has a SID available, we will determine the correct
> >>       * domain by its SID. */
> >>      if (sid_str != NULL) {
> >> -        dom = find_subdomain_by_sid(get_domains_head(dom), sid_str);
> >> -        if (dom == NULL) {
> >> -            DEBUG(SSSDBG_OP_FAILURE, ("SID %s does not belong to any known "
> >> +        subdomain = find_subdomain_by_sid(get_domains_head(dom), sid_str);
> >> +        if (subdomain) {
> >> +            dom = subdomain;
> >> +        } else {
> >> +            DEBUG(SSSDBG_TRACE_FUNC, ("SID %s does not belong to any known "
> >>                                        "domain\n", sid_str));
> >
> >This change is correct. But I checked the other calls to
> >find_subdomain_by_sid() and there are some other places where a similar
> >change might be needed:
> >    src/providers/simple/simple_access_check.c:614
> >and several places in src/providers/ldap/sdap_async_initgroups_ad.c
> >
> >But I admit I only checked the code visually and didn't do any testing.
> >
> >Because there is a deadline coming up, we can commit this patch, but
> >then we should file a ticket to inspect the other usages
> >
> >And one related idea -- some code in the LDAP provider really depends on
> >having data like domain SID available. I wonder if we could add a new
> >LDAP schema (SDAP_SCHEMA_AD_BE) that would only be used by the AD
> >provider and code that depends on having the AD data be only called if
> >this schema is selected. The current AD schema could be still used for
> >cases where the LDAP ID provider is used.
> >
> >An alternative might be to split the code better during the upcoming
> >refactoring of the LDAP provider.
> >
> I would prefer to file a ticket, because proposed change can break something.
> 
> >> -            return ERR_DOMAIN_NOT_FOUND;
> >>          }
> >>      }
> >>  
> >> -- 
> >> 1.8.5.3
> >> 
> >
> >> From 8ee3656c5a24abf5520ea5c636935ff9b0c5fb2d Mon Sep 17 00:00:00 2001
> >> From: Lukas Slebodnik <lslebodn at redhat.com>
> >> Date: Fri, 24 Jan 2014 19:11:57 +0100
> >> Subject: [PATCH 2/2] LDAP: Fix group handling with AD schema
> >> 
> >> regression introduced in commit 8280c5213094a72fcaa499dda2f8647246185d45
> >
> >Could we set the ldap_group_type in the gen_ad2008r2_group_map instead?
> >Something like:
> >    -    { "ldap_group_type", NULL, SYSDB_GROUP_TYPE, NULL },
> >    +    { "ldap_group_type", "groupType", SYSDB_GROUP_TYPE, NULL },
> 
> This change is sufficient and patch look nicer :-)
> 
> Updated patches are attached.
> 
> LS

These patches work for me and resolve the issue. Retrieving groups also
still works fine with AD backend. So ACK to the code.

I still think there are issues to address, at least we should check the
usage of find_domain_by_XXX. I think that searching for a domain should
be only used in the generic LDAP code if there's no other way:
    https://fedorahosted.org/sssd/ticket/2216



More information about the sssd-devel mailing list