[PATCH] ad: store group in correct tree on initgroups via tokenGroups

[Pavel Březina]

https://fedorahosted.org/sssd/ticket/2066

Please note, that these patches has to be applied atop
0004-util-add-find_subdomain_by_sid.patch from my simple provider
patches. You can also pull it from branch ad-groups at my fedorapeople repo.

I still don't know why AD returns Domain Users from root domain in 
tokenGroups, even though subaduser is not member of DU(a)ad.pb (AD bug 
perhaps?), but with these patches we store it correctly.

This should be also a step forward to #2064.

[Jakub Hrozek]

On Fri, Sep 06, 2013 at 02:30:54PM +0200, Pavel Březina wrote:
>  https://fedorahosted.org/sssd/ticket/2066
>  
>  Please note, that these patches has to be applied atop
>  0004-util-add-find_subdomain_by_sid.patch from my simple provider
>  patches. You can also pull it from branch ad-groups at my fedorapeople repo.
>  
>  I still don't know why AD returns Domain Users from root domain in
>  tokenGroups, even though subaduser is not member of DU(a)ad.pb (AD bug
>  perhaps?), but with these patches we store it correctly.
>  
>  This should be also a step forward to #2064. 
Sorry for the long delay in reviews. Can you rebase patch #4 on your
latest simple access provider patches?

[Jakub Hrozek]

On Fri, Sep 06, 2013 at 02:30:54PM +0200, Pavel Březina wrote:
>  https://fedorahosted.org/sssd/ticket/2066
>  
>  Please note, that these patches has to be applied atop
>  0004-util-add-find_subdomain_by_sid.patch from my simple provider
>  patches. You can also pull it from branch ad-groups at my fedorapeople repo.
>  
>  I still don't know why AD returns Domain Users from root domain in
>  tokenGroups, even though subaduser is not member of DU(a)ad.pb (AD bug
>  perhaps?), but with these patches we store it correctly.
>   
Did you check if the SID of root domain's Domain Users is also returned
in the PAC?

>  This should be also a step forward to #2064. 
>  From 3b07596669cac3ec57e7a20e210b6ccd1fc5fa3b Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  Date: Fri, 6 Sep 2013 11:08:56 +0200
>  Subject: [PATCH 1/4] util: add get_domains_head()
>  
>  This function will return head of the domain list.
>  
>  Resolves:
>  https://fedorahosted.org/sssd/ticket/2066
>  ---
>   src/util/domain_info_utils.c | 13 +++++++++++++
>   src/util/util.h              |  2 ++
>   2 files changed, 15 insertions(+)
>  
>  diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c
>  index f9d9057a811f3e08c451d7f6b44bc14202559962..65f3c21f0ad3de2d560347260d317fa21b3e69b3
> 100644
>  --- a/src/util/domain_info_utils.c
>  +++ b/src/util/domain_info_utils.c
>  @@ -27,6 +27,19 @@
>   /* the directory domain - realm mappings are written to */
>   #define KRB5_MAPPING_DIR PUBCONF_PATH"/krb5.include.d"
>   
>  +struct sss_domain_info *get_domains_head(struct sss_domain_info *domain)
>  +{
>  +    struct sss_domain_info *dom = NULL;
>  +
>  +    /* get to the top level domain */
>  +    for (dom = domain; dom->parent != NULL; dom = dom->parent);
>  +
>  +    /* proceed to the list head */
>  +    for (; dom->prev != NULL; dom = dom->prev); 
Can we currently have a setup with multiple top level domains?

>  +
>  +    return dom;
>  +}
>  + 
>  From caf817e5b64cd09c2f953d48a5761f33735ea95e Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  Date: Fri, 6 Sep 2013 13:13:04 +0200
>  Subject: [PATCH 2/4] sysdb: get_sysdb_grouplist() can return either names or
>   dn
>  
>  We need to work with distinguish names when processing
>  cross-domain membership, because groups and users may
>  be stored in different sysdb tree.
>  
>  Resolves:
>  https://fedorahosted.org/sssd/ticket/2066
>  --- 
The code is fine but I would prefer if you created a think wrapper like
you did in patch #3 for sysdb_update_members_dn(). The true/false
boolean values in the code look unreadable to me.

>  From 786b665d4906283a70ddf2e7c040a97f43a57aad Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  Date: Fri, 6 Sep 2013 13:44:46 +0200
>  Subject: [PATCH 3/4] sysdb: sysdb_update_members can take either name or dn
>  
>  We need to work with distinguish names when processing
>  cross-domain membership, because groups and users may
>  be stored in different sysdb tree.
>  
>  Resolves:
>  https://fedorahosted.org/sssd/ticket/2066 
ACK

>  From 7cdd86441a937f9a12a9a5a1a5d2decff927809c Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  Date: Fri, 6 Sep 2013 10:43:35 +0200
>  Subject: [PATCH 4/4] ad: store group in correct tree on initgroups via
>   tokenGroups
>  
>  If tokenGroups contains group from different domain than user's,
>  we stored it under the user's domain tree in sysdb. This patch
>  changes it so we store it under group's domain tree.
>  
>  Resolves:
>  https://fedorahosted.org/sssd/ticket/2066
>  ---
>   src/providers/ldap/sdap_async_initgroups_ad.c | 50 +++++++++++++++++++++------
>   1 file changed, 40 insertions(+), 10 deletions(-)
>  
>  diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c
> b/src/providers/ldap/sdap_async_initgroups_ad.c
>  index 08d0e7dad8e870480b72fbd4179576d445df396b..05f2336ad01391a6427e332bcb9367ee4e745f9b
> 100644
>  --- a/src/providers/ldap/sdap_async_initgroups_ad.c
>  +++ b/src/providers/ldap/sdap_async_initgroups_ad.c
>  @@ -364,9 +364,12 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req
> *subreq)
>       char *sid_str;
>       gid_t gid;
>       time_t now;
>  +    struct sss_domain_info *group_domain;
>       struct sysdb_attrs **users;
>       struct ldb_message_element *el;
>       struct ldb_message *msg;
>  +    struct ldb_dn *group_ldb_dn;
>  +    const char *group_str_dn;
>       char **ldap_grouplist;
>       char **sysdb_grouplist;
>       char **add_groups;
>  @@ -471,13 +474,21 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req
> *subreq)
>               continue;
>           }
>   
>  +        group_domain = find_subdomain_by_sid(get_domains_head(state->domain),
>  +                                                              sid_str);
>  +        if (group_domain == NULL) {
>  +            DEBUG(SSSDBG_MINOR_FAILURE, ("Domain not found for SID %s\n",
>  +                                         sid_str));
>  +            continue;
>  +        }
>  +
>           DEBUG(SSSDBG_TRACE_LIBS,
>                 ("Processing membership GID [%lu]\n",
>                  gid));
>   
>           /* Check whether this GID already exists in the sysdb */
>  -        ret = sysdb_search_group_by_gid(tmp_ctx, state->sysdb, state->domain,
>  -                                        gid, attrs, &msg);
>  +        ret = sysdb_search_group_by_gid(tmp_ctx, group_domain->sysdb,
>  +                                        group_domain, gid, attrs, &msg);
>           if (ret == EOK) {
>               group_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
>               if (!group_name) {
>  @@ -492,9 +503,10 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req
> *subreq)
>                * the group or its GID occurs, it will replace this
>                * temporary entry.
>                */
>  +
>               group_name = sid_str;
>  -            ret = sysdb_add_incomplete_group(state->sysdb,
>  -                                             state->domain,
>  +            ret = sysdb_add_incomplete_group(group_domain->sysdb,
>  +                                             group_domain,
>                                                group_name, gid,
>                                                NULL, sid_str, false, now);
>               if (ret != EOK) {
>  @@ -511,13 +523,31 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req
> *subreq)
>               goto done;
>           }
>   
>  +        group_ldb_dn = sysdb_group_dn(group_domain->sysdb, tmp_ctx,
>  +                                      group_domain, group_name);
>  +        if (group_ldb_dn == NULL) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE, ("sysdb_group_dn() failed\n"));
>  +            ret = ENOMEM;
>  +            goto done;
>  +        }
>  +
>  +        group_str_dn = ldb_dn_get_linearized(group_ldb_dn);
>  +        if (group_str_dn == NULL) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE, ("ldb_dn_get_linearized() failed\n"));
>  +            ret = EINVAL;
>  +            goto done;
>  +        }
>  +
>           ldap_grouplist[group_count] =
>  -                talloc_strdup(ldap_grouplist, group_name);
>  +                talloc_strdup(ldap_grouplist, group_str_dn);
>           if (!ldap_grouplist[group_count]) {
>               ret = ENOMEM;
>               goto done;
>           }
>   
>  +        talloc_zfree(group_ldb_dn);
>  +        group_str_dn = NULL; 
Why do you NULL the group_str_dn pointer here?

>  +
>           group_count++;
>       }
>       ldap_grouplist[group_count] = NULL;
>  @@ -526,7 +556,7 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req
> *subreq)
>        * so we can update it.
>        */
>       ret = get_sysdb_grouplist(state, state->sysdb, state->domain,
>  -                              state->username, &sysdb_grouplist, false);
>  +                              state->username, &sysdb_grouplist, true);
>       if (ret != EOK) {
>           DEBUG(SSSDBG_MINOR_FAILURE,
>                 ("Could not get the list of groups for [%s] in the sysdb: "

[Pavel Březina]

On 09/21/2013 07:41 PM, Jakub Hrozek wrote:
>  On Fri, Sep 06, 2013 at 02:30:54PM +0200, Pavel Březina wrote:
> > https://fedorahosted.org/sssd/ticket/2066
> >
> > Please note, that these patches has to be applied atop
> > 0004-util-add-find_subdomain_by_sid.patch from my simple provider
> > patches. You can also pull it from branch ad-groups at my fedorapeople repo.
> >
> > I still don't know why AD returns Domain Users from root domain in
> > tokenGroups, even though subaduser is not member of DU(a)ad.pb (AD bug
> > perhaps?), but with these patches we store it correctly.
> >
>
>  Did you check if the SID of root domain's Domain Users is also returned
>  in the PAC?
>
> > This should be also a step forward to #2064.
>
> >  From 3b07596669cac3ec57e7a20e210b6ccd1fc5fa3b Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> > Date: Fri, 6 Sep 2013 11:08:56 +0200
> > Subject: [PATCH 1/4] util: add get_domains_head()
> >
> > This function will return head of the domain list.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2066
> > ---
> >   src/util/domain_info_utils.c | 13 +++++++++++++
> >   src/util/util.h              |  2 ++
> >   2 files changed, 15 insertions(+)
> >
> > diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c
> > index
> f9d9057a811f3e08c451d7f6b44bc14202559962..65f3c21f0ad3de2d560347260d317fa21b3e69b3 100644
> > --- a/src/util/domain_info_utils.c
> > +++ b/src/util/domain_info_utils.c
> > @@ -27,6 +27,19 @@
> >   /* the directory domain - realm mappings are written to */
> >   #define KRB5_MAPPING_DIR PUBCONF_PATH"/krb5.include.d"
> >
> > +struct sss_domain_info *get_domains_head(struct sss_domain_info *domain)
> > +{
> > +    struct sss_domain_info *dom = NULL;
> > +
> > +    /* get to the top level domain */
> > +    for (dom = domain; dom->parent != NULL; dom = dom->parent);
> > +
> > +    /* proceed to the list head */
> > +    for (; dom->prev != NULL; dom = dom->prev);
>
>  Can we currently have a setup with multiple top level domains? 
No. We create separate sssd_be process for each top level domain and I 
just checked that be_ctx->domain contains only one top level domain in 
multi-domain environment.

> > +
> > +    return dom;
> > +}
> > +
>
> >  From caf817e5b64cd09c2f953d48a5761f33735ea95e Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> > Date: Fri, 6 Sep 2013 13:13:04 +0200
> > Subject: [PATCH 2/4] sysdb: get_sysdb_grouplist() can return either names or
> >   dn
> >
> > We need to work with distinguish names when processing
> > cross-domain membership, because groups and users may
> > be stored in different sysdb tree.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2066
> > ---
>
>  The code is fine but I would prefer if you created a think wrapper like
>  you did in patch #3 for sysdb_update_members_dn(). The true/false
>  boolean values in the code look unreadable to me. 
Done.

> >  From 786b665d4906283a70ddf2e7c040a97f43a57aad Mon Sep 17
> 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> > Date: Fri, 6 Sep 2013 13:44:46 +0200
> > Subject: [PATCH 3/4] sysdb: sysdb_update_members can take either name or dn
> >
> > We need to work with distinguish names when processing
> > cross-domain membership, because groups and users may
> > be stored in different sysdb tree.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2066
>
>  ACK
>
> >  From 7cdd86441a937f9a12a9a5a1a5d2decff927809c Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> > Date: Fri, 6 Sep 2013 10:43:35 +0200
> > Subject: [PATCH 4/4] ad: store group in correct tree on initgroups via
> >   tokenGroups
> >
> > If tokenGroups contains group from different domain than user's,
> > we stored it under the user's domain tree in sysdb. This patch
> > changes it so we store it under group's domain tree.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2066
> > ---
> >   src/providers/ldap/sdap_async_initgroups_ad.c | 50 +++++++++++++++++++++------
> >   1 file changed, 40 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c
> b/src/providers/ldap/sdap_async_initgroups_ad.c
> > index
> 08d0e7dad8e870480b72fbd4179576d445df396b..05f2336ad01391a6427e332bcb9367ee4e745f9b 100644
> > --- a/src/providers/ldap/sdap_async_initgroups_ad.c
> > +++ b/src/providers/ldap/sdap_async_initgroups_ad.c
> > @@ -364,9 +364,12 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req
> *subreq)
> >       char *sid_str;
> >       gid_t gid;
> >       time_t now;
> > +    struct sss_domain_info *group_domain;
> >       struct sysdb_attrs **users;
> >       struct ldb_message_element *el;
> >       struct ldb_message *msg;
> > +    struct ldb_dn *group_ldb_dn;
> > +    const char *group_str_dn;
> >       char **ldap_grouplist;
> >       char **sysdb_grouplist;
> >       char **add_groups;
> > @@ -471,13 +474,21 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct
> tevent_req *subreq)
> >               continue;
> >           }
> >
> > +        group_domain = find_subdomain_by_sid(get_domains_head(state->domain),
> > +                                                              sid_str);
> > +        if (group_domain == NULL) {
> > +            DEBUG(SSSDBG_MINOR_FAILURE, ("Domain not found for SID %s\n",
> > +                                         sid_str));
> > +            continue;
> > +        }
> > +
> >           DEBUG(SSSDBG_TRACE_LIBS,
> >                 ("Processing membership GID [%lu]\n",
> >                  gid));
> >
> >           /* Check whether this GID already exists in the sysdb */
> > -        ret = sysdb_search_group_by_gid(tmp_ctx, state->sysdb, state->domain,
> > -                                        gid, attrs, &msg);
> > +        ret = sysdb_search_group_by_gid(tmp_ctx, group_domain->sysdb,
> > +                                        group_domain, gid, attrs, &msg);
> >           if (ret == EOK) {
> >               group_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
> >               if (!group_name) {
> > @@ -492,9 +503,10 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req
> *subreq)
> >                * the group or its GID occurs, it will replace this
> >                * temporary entry.
> >                */
> > +
> >               group_name = sid_str;
> > -            ret = sysdb_add_incomplete_group(state->sysdb,
> > -                                             state->domain,
> > +            ret = sysdb_add_incomplete_group(group_domain->sysdb,
> > +                                             group_domain,
> >                                                group_name, gid,
> >                                                NULL, sid_str, false, now);
> >               if (ret != EOK) {
> > @@ -511,13 +523,31 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct
> tevent_req *subreq)
> >               goto done;
> >           }
> >
> > +        group_ldb_dn = sysdb_group_dn(group_domain->sysdb, tmp_ctx,
> > +                                      group_domain, group_name);
> > +        if (group_ldb_dn == NULL) {
> > +            DEBUG(SSSDBG_CRIT_FAILURE, ("sysdb_group_dn() failed\n"));
> > +            ret = ENOMEM;
> > +            goto done;
> > +        }
> > +
> > +        group_str_dn = ldb_dn_get_linearized(group_ldb_dn);
> > +        if (group_str_dn == NULL) {
> > +            DEBUG(SSSDBG_CRIT_FAILURE, ("ldb_dn_get_linearized()
> failed\n"));
> > +            ret = EINVAL;
> > +            goto done;
> > +        }
> > +
> >           ldap_grouplist[group_count] =
> > -                talloc_strdup(ldap_grouplist, group_name);
> > +                talloc_strdup(ldap_grouplist, group_str_dn);
> >           if (!ldap_grouplist[group_count]) {
> >               ret = ENOMEM;
> >               goto done;
> >           }
> >
> > +        talloc_zfree(group_ldb_dn);
> > +        group_str_dn = NULL;
>
>  Why do you NULL the group_str_dn pointer here? 
I want to explicitly reset iteration data before we continue with next 
iteration. But more important is that group_str_dn is freed with 
talloc_zfree(group_ldb_dn) so it would be a dangling pointer.

I put a comment there.

> > +
> >           group_count++;
> >       }
> >       ldap_grouplist[group_count] = NULL;
> > @@ -526,7 +556,7 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req
> *subreq)
> >        * so we can update it.
> >        */
> >       ret = get_sysdb_grouplist(state, state->sysdb, state->domain,
> > -                              state->username, &sysdb_grouplist, false);
> > +                              state->username, &sysdb_grouplist, true);
> >       if (ret != EOK) {
> >           DEBUG(SSSDBG_MINOR_FAILURE,
> >                 ("Could not get the list of groups for [%s] in the sysdb: "

[Jakub Hrozek]

On Tue, Sep 24, 2013 at 03:14:59PM +0200, Pavel Březina wrote:
>  On 09/21/2013 07:41 PM, Jakub Hrozek wrote:
>  >On Fri, Sep 06, 2013 at 02:30:54PM +0200, Pavel Březina wrote:
>  >>https://fedorahosted.org/sssd/ticket/2066
>  >>
>  >>Please note, that these patches has to be applied atop
>  >>0004-util-add-find_subdomain_by_sid.patch from my simple provider
>  >>patches. You can also pull it from branch ad-groups at my fedorapeople repo.
>  >>
>  >>I still don't know why AD returns Domain Users from root domain in
>  >>tokenGroups, even though subaduser is not member of DU(a)ad.pb (AD bug
>  >>perhaps?), but with these patches we store it correctly.
>  >>
>  >
>  >Did you check if the SID of root domain's Domain Users is also returned
>  >in the PAC?
>  >
>  >>This should be also a step forward to #2064.
>  >
>  >> From 3b07596669cac3ec57e7a20e210b6ccd1fc5fa3b Mon Sep 17 00:00:00 2001
>  >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  >>Date: Fri, 6 Sep 2013 11:08:56 +0200
>  >>Subject: [PATCH 1/4] util: add get_domains_head()
>  >>
>  >>This function will return head of the domain list.
>  >>
>  >>Resolves:
>  >>https://fedorahosted.org/sssd/ticket/2066
>  >>---
>  >>  src/util/domain_info_utils.c | 13 +++++++++++++
>  >>  src/util/util.h              |  2 ++
>  >>  2 files changed, 15 insertions(+)
>  >>
>  >>diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c
>  >>index
> f9d9057a811f3e08c451d7f6b44bc14202559962..65f3c21f0ad3de2d560347260d317fa21b3e69b3 100644
>  >>--- a/src/util/domain_info_utils.c
>  >>+++ b/src/util/domain_info_utils.c
>  >>@@ -27,6 +27,19 @@
>  >>  /* the directory domain - realm mappings are written to */
>  >>  #define KRB5_MAPPING_DIR PUBCONF_PATH"/krb5.include.d"
>  >>
>  >>+struct sss_domain_info *get_domains_head(struct sss_domain_info *domain)
>  >>+{
>  >>+    struct sss_domain_info *dom = NULL;
>  >>+
>  >>+    /* get to the top level domain */
>  >>+    for (dom = domain; dom->parent != NULL; dom = dom->parent);
>  >>+
>  >>+    /* proceed to the list head */
>  >>+    for (; dom->prev != NULL; dom = dom->prev);
>  >
>  >Can we currently have a setup with multiple top level domains?
>  
>  No. We create separate sssd_be process for each top level domain and
>  I just checked that be_ctx->domain contains only one top level
>  domain in multi-domain environment. 
OK, then what is the point of the loop that proceeds to the list head if
there can only be one domain?

>  
>  >>+
>  >>+    return dom;
>  >>+}
>  >>+ 
Otherwise the patches work for me, so ACK.

But I'm wondering whether (in a future patch, maybe during 1.12/1.13)
change updating members to primarily work on DNs, not names?

[Pavel Březina]

On 09/25/2013 01:07 PM, Jakub Hrozek wrote:
>  On Tue, Sep 24, 2013 at 03:14:59PM +0200, Pavel Březina wrote:
> > On 09/21/2013 07:41 PM, Jakub Hrozek wrote:
> >> On Fri, Sep 06, 2013 at 02:30:54PM +0200, Pavel Březina wrote:
> >>> https://fedorahosted.org/sssd/ticket/2066
> >>>
> >>> Please note, that these patches has to be applied atop
> >>> 0004-util-add-find_subdomain_by_sid.patch from my simple
> >>> provider patches. You can also pull it from branch ad-groups at
> >>> my fedorapeople repo.
> >>>
> >>> I still don't know why AD returns Domain Users from root domain
> >>> in tokenGroups, even though subaduser is not member of DU(a)ad.pb
> >>> (AD bug perhaps?), but with these patches we store it
> >>> correctly.
> >>>
> >>
> >> Did you check if the SID of root domain's Domain Users is also
> >> returned in the PAC?
> >>
> >>> This should be also a step forward to #2064.
> >>
> >>> From 3b07596669cac3ec57e7a20e210b6ccd1fc5fa3b Mon Sep 17
> >>> 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
> >>> <pbrezina(a)redhat.com&gt; Date: Fri, 6 Sep 2013 11:08:56 +0200
> >>> Subject: [PATCH 1/4] util: add get_domains_head()
> >>>
> >>> This function will return head of the domain list.
> >>>
> >>> Resolves: https://fedorahosted.org/sssd/ticket/2066 ---
> >>> src/util/domain_info_utils.c | 13 +++++++++++++ src/util/util.h
> >>> |  2 ++ 2 files changed, 15 insertions(+)
> >>>
> >>> diff --git a/src/util/domain_info_utils.c
> >>> b/src/util/domain_info_utils.c index
> >>>
> f9d9057a811f3e08c451d7f6b44bc14202559962..65f3c21f0ad3de2d560347260d317fa21b3e69b3
> >>> 100644 --- a/src/util/domain_info_utils.c +++
> >>> b/src/util/domain_info_utils.c @@ -27,6 +27,19 @@ /* the
> >>> directory domain - realm mappings are written to */ #define
> >>> KRB5_MAPPING_DIR PUBCONF_PATH"/krb5.include.d"
> >>>
> >>> +struct sss_domain_info *get_domains_head(struct
> >>> sss_domain_info *domain) +{ +    struct sss_domain_info *dom =
> >>> NULL; + +    /* get to the top level domain */ +    for (dom =
> >>> domain; dom->parent != NULL; dom = dom->parent); + +    /*
> >>> proceed to the list head */ +    for (; dom->prev != NULL; dom
> >>> = dom->prev);
> >>
> >> Can we currently have a setup with multiple top level domains?
> >
> > No. We create separate sssd_be process for each top level domain
> > and I just checked that be_ctx->domain contains only one top level
> > domain in multi-domain environment.
>
>  OK, then what is the point of the loop that proceeds to the list head
>  if there can only be one domain? 
state->domain is not necessarily equal to be_ctx->domain in this case. I
would have pass it to _send as parameter and create state->be_domain for
example.

I'm not quite sure why I've gone this path. I remember that I've
actually created state->be_domain at first but then switched to
get_domains_head().

I'm also quite surprised that get_domains_head() is called at only one
place. I thought I used it more :-)

> >
> >>> + +    return dom; +} +
>
>  Otherwise the patches work for me, so ACK.
>
>  But I'm wondering whether (in a future patch, maybe during
>  1.12/1.13) change updating members to primarily work on DNs, not
>  names? 
Dunno. It may be a good idea. To be more general, we should completely
unify how we deal with objects from main domain and subdomain. At the
moment we have lots of code that goes like if (subdomain) then do this
else if (maindomain) then do this. My idea is to see main domain in the
same light as sub domain.

Also at the moment, subdomains are basically handled on sdap level. I'd
like to bring them more to back-end level.

It is something I'd like to talk about next time, when all devs are face
to face.

[Jakub Hrozek]

On Wed, Sep 25, 2013 at 02:55:22PM +0200, Pavel Březina wrote:
>  On 09/25/2013 01:07 PM, Jakub Hrozek wrote:
>  >On Tue, Sep 24, 2013 at 03:14:59PM +0200, Pavel Březina wrote:
>  >>On 09/21/2013 07:41 PM, Jakub Hrozek wrote:
>  >>>On Fri, Sep 06, 2013 at 02:30:54PM +0200, Pavel Březina wrote:
>  >>>>https://fedorahosted.org/sssd/ticket/2066
>  >>>>
>  >>>>Please note, that these patches has to be applied atop
>  >>>>0004-util-add-find_subdomain_by_sid.patch from my simple
>  >>>>provider patches. You can also pull it from branch ad-groups at
>  >>>>my fedorapeople repo.
>  >>>>
>  >>>>I still don't know why AD returns Domain Users from root domain
>  >>>>in tokenGroups, even though subaduser is not member of DU(a)ad.pb
>  >>>>(AD bug perhaps?), but with these patches we store it
>  >>>>correctly.
>  >>>>
>  >>>
>  >>>Did you check if the SID of root domain's Domain Users is also
>  >>>returned in the PAC?
>  >>>
>  >>>>This should be also a step forward to #2064.
>  >>>
>  >>>>From 3b07596669cac3ec57e7a20e210b6ccd1fc5fa3b Mon Sep 17
>  >>>>00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
>  >>>><pbrezina(a)redhat.com> Date: Fri, 6 Sep 2013 11:08:56 +0200
>  >>>>Subject: [PATCH 1/4] util: add get_domains_head()
>  >>>>
>  >>>>This function will return head of the domain list.
>  >>>>
>  >>>>Resolves: https://fedorahosted.org/sssd/ticket/2066 ---
>  >>>>src/util/domain_info_utils.c | 13 +++++++++++++ src/util/util.h
>  >>>>|  2 ++ 2 files changed, 15 insertions(+)
>  >>>>
>  >>>>diff --git a/src/util/domain_info_utils.c
>  >>>>b/src/util/domain_info_utils.c index
>
> >>>>f9d9057a811f3e08c451d7f6b44bc14202559962..65f3c21f0ad3de2d560347260d317fa21b3e69b3
>  >>>>100644 --- a/src/util/domain_info_utils.c +++
>  >>>>b/src/util/domain_info_utils.c @@ -27,6 +27,19 @@ /* the
>  >>>>directory domain - realm mappings are written to */ #define
>  >>>>KRB5_MAPPING_DIR PUBCONF_PATH"/krb5.include.d"
>  >>>>
>  >>>>+struct sss_domain_info *get_domains_head(struct
>  >>>>sss_domain_info *domain) +{ +    struct sss_domain_info *dom =
>  >>>>NULL; + +    /* get to the top level domain */ +    for (dom =
>  >>>>domain; dom->parent != NULL; dom = dom->parent); + +    /*
>  >>>>proceed to the list head */ +    for (; dom->prev != NULL; dom
>  >>>>= dom->prev);
>  >>>
>  >>>Can we currently have a setup with multiple top level domains?
>  >>
>  >>No. We create separate sssd_be process for each top level domain
>  >>and I just checked that be_ctx->domain contains only one top level
>  >>domain in multi-domain environment.
>  >
>  >OK, then what is the point of the loop that proceeds to the list head
>  >if there can only be one domain?
>  
>  state->domain is not necessarily equal to be_ctx->domain in this case. I
>  would have pass it to _send as parameter and create state->be_domain for
>  example.
>  
>  I'm not quite sure why I've gone this path. I remember that I've
>  actually created state->be_domain at first but then switched to
>  get_domains_head().
>  
>  I'm also quite surprised that get_domains_head() is called at only one
>  place. I thought I used it more :-)
>  
>  >>
>  >>>>+ +    return dom; +} +
>  >
>  >Otherwise the patches work for me, so ACK.
>  >
>  >But I'm wondering whether (in a future patch, maybe during
>  >1.12/1.13) change updating members to primarily work on DNs, not
>  >names?
>  
>  Dunno. It may be a good idea. To be more general, we should completely
>  unify how we deal with objects from main domain and subdomain. At the
>  moment we have lots of code that goes like if (subdomain) then do this
>  else if (maindomain) then do this. My idea is to see main domain in the
>  same light as sub domain.
>  
>  Also at the moment, subdomains are basically handled on sdap level. I'd
>  like to bring them more to back-end level.
>  
>  It is something I'd like to talk about next time, when all devs are face
>  to face. 
I agree completely. I think we should start by always using FQDNs for
all users and storing the plain name separately. But I'm afraid a change
like this is outside the scope of even 1.12.

[Jakub Hrozek]

On Wed, Sep 25, 2013 at 05:21:30PM +0200, Jakub Hrozek wrote:
>  On Wed, Sep 25, 2013 at 02:55:22PM +0200, Pavel Březina wrote:
>  > On 09/25/2013 01:07 PM, Jakub Hrozek wrote:
>  > >On Tue, Sep 24, 2013 at 03:14:59PM +0200, Pavel Březina wrote:
>  > >>On 09/21/2013 07:41 PM, Jakub Hrozek wrote:
>  > >>>On Fri, Sep 06, 2013 at 02:30:54PM +0200, Pavel Březina wrote:
>  > >>>>https://fedorahosted.org/sssd/ticket/2066
>  > >>>>
>  > >>>>Please note, that these patches has to be applied atop
>  > >>>>0004-util-add-find_subdomain_by_sid.patch from my simple
>  > >>>>provider patches. You can also pull it from branch ad-groups at
>  > >>>>my fedorapeople repo.
>  > >>>>
>  > >>>>I still don't know why AD returns Domain Users from root domain
>  > >>>>in tokenGroups, even though subaduser is not member of DU(a)ad.pb
>  > >>>>(AD bug perhaps?), but with these patches we store it
>  > >>>>correctly.
>  > >>>>
>  > >>>
>  > >>>Did you check if the SID of root domain's Domain Users is also
>  > >>>returned in the PAC?
>  > >>>
>  > >>>>This should be also a step forward to #2064.
>  > >>>
>  > >>>>From 3b07596669cac3ec57e7a20e210b6ccd1fc5fa3b Mon Sep 17
>  > >>>>00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
>  > >>>><pbrezina(a)redhat.com> Date: Fri, 6 Sep 2013 11:08:56 +0200
>  > >>>>Subject: [PATCH 1/4] util: add get_domains_head()
>  > >>>>
>  > >>>>This function will return head of the domain list.
>  > >>>>
>  > >>>>Resolves: https://fedorahosted.org/sssd/ticket/2066 ---
>  > >>>>src/util/domain_info_utils.c | 13 +++++++++++++ src/util/util.h
>  > >>>>|  2 ++ 2 files changed, 15 insertions(+)
>  > >>>>
>  > >>>>diff --git a/src/util/domain_info_utils.c
>  > >>>>b/src/util/domain_info_utils.c index
>  >
> >>>>f9d9057a811f3e08c451d7f6b44bc14202559962..65f3c21f0ad3de2d560347260d317fa21b3e69b3
>  > >>>>100644 --- a/src/util/domain_info_utils.c +++
>  > >>>>b/src/util/domain_info_utils.c @@ -27,6 +27,19 @@ /* the
>  > >>>>directory domain - realm mappings are written to */ #define
>  > >>>>KRB5_MAPPING_DIR PUBCONF_PATH"/krb5.include.d"
>  > >>>>
>  > >>>>+struct sss_domain_info *get_domains_head(struct
>  > >>>>sss_domain_info *domain) +{ +    struct sss_domain_info *dom =
>  > >>>>NULL; + +    /* get to the top level domain */ +    for (dom =
>  > >>>>domain; dom->parent != NULL; dom = dom->parent); + +    /*
>  > >>>>proceed to the list head */ +    for (; dom->prev != NULL; dom
>  > >>>>= dom->prev);
>  > >>>
>  > >>>Can we currently have a setup with multiple top level domains?
>  > >>
>  > >>No. We create separate sssd_be process for each top level domain
>  > >>and I just checked that be_ctx->domain contains only one top level
>  > >>domain in multi-domain environment.
>  > >
>  > >OK, then what is the point of the loop that proceeds to the list head
>  > >if there can only be one domain?
>  > 
>  > state->domain is not necessarily equal to be_ctx->domain in this case. I
>  > would have pass it to _send as parameter and create state->be_domain for
>  > example.
>  > 
>  > I'm not quite sure why I've gone this path. I remember that I've
>  > actually created state->be_domain at first but then switched to
>  > get_domains_head().
>  > 
>  > I'm also quite surprised that get_domains_head() is called at only one
>  > place. I thought I used it more :-) 
Pavel, please file a ticket, assign it to yourself and check the code
out once again during the 1.11.2 time. I don't see a problem with the
current code that could cause a bug, but we should keep the incoming
code clean.

Ack to the current patches on condition you'd check the the logic again.

[Jakub Hrozek]

On Thu, Sep 26, 2013 at 09:11:24PM +0200, Jakub Hrozek wrote:
>  On Wed, Sep 25, 2013 at 05:21:30PM +0200, Jakub Hrozek wrote:
>  > On Wed, Sep 25, 2013 at 02:55:22PM +0200, Pavel Březina wrote:
>  > > On 09/25/2013 01:07 PM, Jakub Hrozek wrote:
>  > > >On Tue, Sep 24, 2013 at 03:14:59PM +0200, Pavel Březina wrote:
>  > > >>On 09/21/2013 07:41 PM, Jakub Hrozek wrote:
>  > > >>>On Fri, Sep 06, 2013 at 02:30:54PM +0200, Pavel Březina wrote:
>  > > >>>>https://fedorahosted.org/sssd/ticket/2066
>  > > >>>>
>  > > >>>>Please note, that these patches has to be applied atop
>  > > >>>>0004-util-add-find_subdomain_by_sid.patch from my simple
>  > > >>>>provider patches. You can also pull it from branch ad-groups
> at
>  > > >>>>my fedorapeople repo.
>  > > >>>>
>  > > >>>>I still don't know why AD returns Domain Users from root
> domain
>  > > >>>>in tokenGroups, even though subaduser is not member of
> DU(a)ad.pb
>  > > >>>>(AD bug perhaps?), but with these patches we store it
>  > > >>>>correctly.
>  > > >>>>
>  > > >>>
>  > > >>>Did you check if the SID of root domain's Domain Users is also
>  > > >>>returned in the PAC?
>  > > >>>
>  > > >>>>This should be also a step forward to #2064.
>  > > >>>
>  > > >>>>From 3b07596669cac3ec57e7a20e210b6ccd1fc5fa3b Mon Sep 17
>  > > >>>>00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
>  > > >>>><pbrezina(a)redhat.com> Date: Fri, 6 Sep 2013 11:08:56
> +0200
>  > > >>>>Subject: [PATCH 1/4] util: add get_domains_head()
>  > > >>>>
>  > > >>>>This function will return head of the domain list.
>  > > >>>>
>  > > >>>>Resolves: https://fedorahosted.org/sssd/ticket/2066 ---
>  > > >>>>src/util/domain_info_utils.c | 13 +++++++++++++
> src/util/util.h
>  > > >>>>|  2 ++ 2 files changed, 15 insertions(+)
>  > > >>>>
>  > > >>>>diff --git a/src/util/domain_info_utils.c
>  > > >>>>b/src/util/domain_info_utils.c index
>  > >
> >>>>f9d9057a811f3e08c451d7f6b44bc14202559962..65f3c21f0ad3de2d560347260d317fa21b3e69b3
>  > > >>>>100644 --- a/src/util/domain_info_utils.c +++
>  > > >>>>b/src/util/domain_info_utils.c @@ -27,6 +27,19 @@ /* the
>  > > >>>>directory domain - realm mappings are written to */ #define
>  > > >>>>KRB5_MAPPING_DIR PUBCONF_PATH"/krb5.include.d"
>  > > >>>>
>  > > >>>>+struct sss_domain_info *get_domains_head(struct
>  > > >>>>sss_domain_info *domain) +{ +    struct sss_domain_info *dom =
>  > > >>>>NULL; + +    /* get to the top level domain */ +    for (dom =
>  > > >>>>domain; dom->parent != NULL; dom = dom->parent); + +   
> /*
>  > > >>>>proceed to the list head */ +    for (; dom->prev != NULL;
> dom
>  > > >>>>= dom->prev);
>  > > >>>
>  > > >>>Can we currently have a setup with multiple top level domains?
>  > > >>
>  > > >>No. We create separate sssd_be process for each top level domain
>  > > >>and I just checked that be_ctx->domain contains only one top level
>  > > >>domain in multi-domain environment.
>  > > >
>  > > >OK, then what is the point of the loop that proceeds to the list head
>  > > >if there can only be one domain?
>  > > 
>  > > state->domain is not necessarily equal to be_ctx->domain in this case. I
>  > > would have pass it to _send as parameter and create state->be_domain for
>  > > example.
>  > > 
>  > > I'm not quite sure why I've gone this path. I remember that I've
>  > > actually created state->be_domain at first but then switched to
>  > > get_domains_head().
>  > > 
>  > > I'm also quite surprised that get_domains_head() is called at only one
>  > > place. I thought I used it more :-)
>  
>  Pavel, please file a ticket, assign it to yourself and check the code
>  out once again during the 1.11.2 time. I don't see a problem with the
>  current code that could cause a bug, but we should keep the incoming
>  code clean.
>  
>  Ack to the current patches on condition you'd check the the logic again. 
Pushed to master and sssd-1-11

[Pavel Březina]

On 09/26/2013 09:11 PM, Jakub Hrozek wrote:
>  On Wed, Sep 25, 2013 at 05:21:30PM +0200, Jakub Hrozek wrote:
> > On Wed, Sep 25, 2013 at 02:55:22PM +0200, Pavel Březina wrote:
> >> On 09/25/2013 01:07 PM, Jakub Hrozek wrote:
> >>> On Tue, Sep 24, 2013 at 03:14:59PM +0200, Pavel Březina wrote:
> >>>> On 09/21/2013 07:41 PM, Jakub Hrozek wrote:
> >>>>> On Fri, Sep 06, 2013 at 02:30:54PM +0200, Pavel Březina wrote:
> >>>>>> https://fedorahosted.org/sssd/ticket/2066
> >>>>>>
> >>>>>> Please note, that these patches has to be applied atop
> >>>>>> 0004-util-add-find_subdomain_by_sid.patch from my simple
> >>>>>> provider patches. You can also pull it from branch ad-groups at
> >>>>>> my fedorapeople repo.
> >>>>>>
> >>>>>> I still don't know why AD returns Domain Users from root
> domain
> >>>>>> in tokenGroups, even though subaduser is not member of DU(a)ad.pb
> >>>>>> (AD bug perhaps?), but with these patches we store it
> >>>>>> correctly.
> >>>>>>
> >>>>>
> >>>>> Did you check if the SID of root domain's Domain Users is also
> >>>>> returned in the PAC?
> >>>>>
> >>>>>> This should be also a step forward to #2064.
> >>>>>
> >>>>> >From 3b07596669cac3ec57e7a20e210b6ccd1fc5fa3b Mon Sep 17
> >>>>>> 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
> >>>>>> <pbrezina(a)redhat.com&gt; Date: Fri, 6 Sep 2013 11:08:56 +0200
> >>>>>> Subject: [PATCH 1/4] util: add get_domains_head()
> >>>>>>
> >>>>>> This function will return head of the domain list.
> >>>>>>
> >>>>>> Resolves: https://fedorahosted.org/sssd/ticket/2066 ---
> >>>>>> src/util/domain_info_utils.c | 13 +++++++++++++ src/util/util.h
> >>>>>> |  2 ++ 2 files changed, 15 insertions(+)
> >>>>>>
> >>>>>> diff --git a/src/util/domain_info_utils.c
> >>>>>> b/src/util/domain_info_utils.c index
> >>>>>>
> f9d9057a811f3e08c451d7f6b44bc14202559962..65f3c21f0ad3de2d560347260d317fa21b3e69b3
> >>>>>> 100644 --- a/src/util/domain_info_utils.c +++
> >>>>>> b/src/util/domain_info_utils.c @@ -27,6 +27,19 @@ /* the
> >>>>>> directory domain - realm mappings are written to */ #define
> >>>>>> KRB5_MAPPING_DIR PUBCONF_PATH"/krb5.include.d"
> >>>>>>
> >>>>>> +struct sss_domain_info *get_domains_head(struct
> >>>>>> sss_domain_info *domain) +{ +    struct sss_domain_info *dom =
> >>>>>> NULL; + +    /* get to the top level domain */ +    for (dom =
> >>>>>> domain; dom->parent != NULL; dom = dom->parent); + +    /*
> >>>>>> proceed to the list head */ +    for (; dom->prev != NULL;
> dom
> >>>>>> = dom->prev);
> >>>>>
> >>>>> Can we currently have a setup with multiple top level domains?
> >>>>
> >>>> No. We create separate sssd_be process for each top level domain
> >>>> and I just checked that be_ctx->domain contains only one top level
> >>>> domain in multi-domain environment.
> >>>
> >>> OK, then what is the point of the loop that proceeds to the list head
> >>> if there can only be one domain?
> >>
> >> state->domain is not necessarily equal to be_ctx->domain in this case. I
> >> would have pass it to _send as parameter and create state->be_domain for
> >> example.
> >>
> >> I'm not quite sure why I've gone this path. I remember that I've
> >> actually created state->be_domain at first but then switched to
> >> get_domains_head().
> >>
> >> I'm also quite surprised that get_domains_head() is called at only one
> >> place. I thought I used it more :-)
>
>  Pavel, please file a ticket, assign it to yourself and check the code
>  out once again during the 1.11.2 time. I don't see a problem with the
>  current code that could cause a bug, but we should keep the incoming
>  code clean.
>
>  Ack to the current patches on condition you'd check the the logic again. 
https://fedorahosted.org/sssd/ticket/2103