[PATCHES] filter domain local groups and fix cross-domain groupmembership

[Sumit Bose]

Hi,

the follwing patch set contains two different fixes but since they are
related and depend on each other I thought it might be easier to just
send them together. The first three patches should fix
https://fedorahosted.org/sssd/ticket/2178 while the last one should fix
cross-domain group-memberships which was broken by a previous patch by
me.

bye,
Sumit

[Jakub Hrozek]

On Wed, Dec 18, 2013 at 04:48:44PM +0100, Sumit Bose wrote:
>  Hi,
>  
>  the follwing patch set contains two different fixes but since they are
>  related and depend on each other I thought it might be easier to just
>  send them together. The first three patches should fix
>  https://fedorahosted.org/sssd/ticket/2178 while the last one should fix
>  cross-domain group-memberships which was broken by a previous patch by
>  me.
>  
>  bye,
>  Sumit 
I'm still reviewing the last patch, but I wanted to give some comments
so we can work in parallel:

>  From f410f0869d090048f4ab5aec5190bae0c82d5b2f Mon Sep 17 00:00:00
> 2001
>  From: Sumit Bose <sbose(a)redhat.com&gt;
>  Date: Mon, 9 Dec 2013 12:17:43 +0100
>  Subject: [PATCH 1/4] Add new option ldap_group_type
>  
>  index 72586fb..82dcbba 100644
>  --- a/src/man/sssd-ldap.5.xml
>  +++ b/src/man/sssd-ldap.5.xml
>  @@ -849,6 +849,27 @@
>                   </varlistentry>
>   
>                   <varlistentry>
>  +                    <term>ldap_group_type (integer)</term>
>  +                    <listitem>
>  +                        <para>
>  +                            The LDAP attribute that contains an integer value
>  +                            indicating the type of the group and maybe other
>  +                            flags.
>  +                        </para>
>  +                        <para>
>  +                            This attribute is currently only used by the AD
>  +                            privder to determine if a group is a domain local 
                              ^^^^^
                              typo
>  +                            groups and has to be filtered out for
> trusted
>  +                            domains.
>  +                        </para>
>  +                        <para>
>  +                            Default: groupType in the AD provider, othewise not
>  +                            set
>  +                        </para>
>  +                    </listitem>
>  +                </varlistentry>
>  +
>  +                <varlistentry>
>                       <term>ldap_group_nesting_level (integer)</term>
>                       <listitem>
>                           <para> 
Otherwise ACK

>  From 717c1f39324e0d718021873f20f31154d7bf58f5 Mon Sep 17 00:00:00
> 2001
>  From: Sumit Bose <sbose(a)redhat.com&gt;
>  Date: Tue, 10 Dec 2013 10:14:02 +0100
>  Subject: [PATCH 2/4] Add sysdb_attrs_get_int32_t 
>  --- a/src/db/sysdb.h
>  +++ b/src/db/sysdb.h
>  @@ -294,6 +294,10 @@ errno_t sysdb_attrs_get_bool(struct sysdb_attrs *attrs, const char
> *name,
>                                bool *value);
>   int sysdb_attrs_get_uint16_t(struct sysdb_attrs *attrs, const char *name,
>                                uint16_t *value);
>  +int sysdb_attrs_get_string(struct sysdb_attrs *attrs, const char *name,
>  +                           const char **string); 
This ^^ addition is not needed, sysdb_attrs_get_string() was already
declared in sysdb.h earlier.

>  +int sysdb_attrs_get_int32_t(struct sysdb_attrs *attrs, const char
> *name,
>  +                             int32_t *value);
>   int sysdb_attrs_get_uint32_t(struct sysdb_attrs *attrs, const char *name,
>                                uint32_t *value); 
>  From 4b82b6439042550fd63023b23e876ba8ba35490e Mon Sep 17 00:00:00
> 2001
>  From: Sumit Bose <sbose(a)redhat.com&gt;
>  Date: Tue, 10 Dec 2013 10:14:28 +0100
>  Subject: [PATCH 3/4] AD: filter domain local groups for trusted/sub domains
>  
>  In Active Directory groups with a domain local scope should only be used
>  inside of the specific domain. Since SSSD read the group memberships
>  from LDAP server of the user's domain the domain local groups are
>  included in the LDAP result. Those groups should be filtered out if the
>  domain is a sub/trusted domain, i.e. is not the domain the client
>  running SSSD is joined to.
>  
>  The groups will still be in the cache but marked as non-POSIX groups and
>  no GID will be assigned.
>  
>  Fixes https://fedorahosted.org/sssd/ticket/2178 
ACK, the code looks good and seems to work fine.

[Sumit Bose]

On Thu, Dec 19, 2013 at 10:56:47AM +0100, Jakub Hrozek wrote:
>  On Wed, Dec 18, 2013 at 04:48:44PM +0100, Sumit Bose wrote:
>  > Hi,
>  > 
>  > the follwing patch set contains two different fixes but since they are
>  > related and depend on each other I thought it might be easier to just
>  > send them together. The first three patches should fix
>  > https://fedorahosted.org/sssd/ticket/2178 while the last one should fix
>  > cross-domain group-memberships which was broken by a previous patch by
>  > me.
>  > 
>  > bye,
>  > Sumit
>  
>  I'm still reviewing the last patch, but I wanted to give some comments
>  so we can work in parallel:
>  
>  > From f410f0869d090048f4ab5aec5190bae0c82d5b2f Mon Sep 17 00:00:00 2001
>  > From: Sumit Bose <sbose(a)redhat.com&gt;
>  > Date: Mon, 9 Dec 2013 12:17:43 +0100
>  > Subject: [PATCH 1/4] Add new option ldap_group_type
>  > 
>  > index 72586fb..82dcbba 100644
>  > --- a/src/man/sssd-ldap.5.xml
>  > +++ b/src/man/sssd-ldap.5.xml
>  > @@ -849,6 +849,27 @@
>  >                  </varlistentry>
>  >  
>  >                  <varlistentry>
>  > +                    <term>ldap_group_type (integer)</term>
>  > +                    <listitem>
>  > +                        <para>
>  > +                            The LDAP attribute that contains an integer value
>  > +                            indicating the type of the group and maybe other
>  > +                            flags.
>  > +                        </para>
>  > +                        <para>
>  > +                            This attribute is currently only used by the AD
>  > +                            privder to determine if a group is a domain local
>  
>                                ^^^^^
>                                typo
>  > +                            groups and has to be filtered out for trusted
>  > +                            domains.
>  > +                        </para>
>  > +                        <para>
>  > +                            Default: groupType in the AD provider, othewise not
>  > +                            set
>  > +                        </para>
>  > +                    </listitem>
>  > +                </varlistentry>
>  > +
>  > +                <varlistentry>
>  >                      <term>ldap_group_nesting_level (integer)</term>
>  >                      <listitem>
>  >                          <para>
>  
>  Otherwise ACK
>  
>  > From 717c1f39324e0d718021873f20f31154d7bf58f5 Mon Sep 17 00:00:00 2001
>  > From: Sumit Bose <sbose(a)redhat.com&gt;
>  > Date: Tue, 10 Dec 2013 10:14:02 +0100
>  > Subject: [PATCH 2/4] Add sysdb_attrs_get_int32_t
>  
>  > --- a/src/db/sysdb.h
>  > +++ b/src/db/sysdb.h
>  > @@ -294,6 +294,10 @@ errno_t sysdb_attrs_get_bool(struct sysdb_attrs *attrs, const
> char *name,
>  >                               bool *value);
>  >  int sysdb_attrs_get_uint16_t(struct sysdb_attrs *attrs, const char *name,
>  >                               uint16_t *value);
>  > +int sysdb_attrs_get_string(struct sysdb_attrs *attrs, const char *name,
>  > +                           const char **string);
>  
>  This ^^ addition is not needed, sysdb_attrs_get_string() was already
>  declared in sysdb.h earlier.
>  
>  > +int sysdb_attrs_get_int32_t(struct sysdb_attrs *attrs, const char *name,
>  > +                             int32_t *value);
>  >  int sysdb_attrs_get_uint32_t(struct sysdb_attrs *attrs, const char *name,
>  >                               uint32_t *value);
>  
>  > From 4b82b6439042550fd63023b23e876ba8ba35490e Mon Sep 17 00:00:00 2001
>  > From: Sumit Bose <sbose(a)redhat.com&gt;
>  > Date: Tue, 10 Dec 2013 10:14:28 +0100
>  > Subject: [PATCH 3/4] AD: filter domain local groups for trusted/sub domains
>  > 
>  > In Active Directory groups with a domain local scope should only be used
>  > inside of the specific domain. Since SSSD read the group memberships
>  > from LDAP server of the user's domain the domain local groups are
>  > included in the LDAP result. Those groups should be filtered out if the
>  > domain is a sub/trusted domain, i.e. is not the domain the client
>  > running SSSD is joined to.
>  > 
>  > The groups will still be in the cache but marked as non-POSIX groups and
>  > no GID will be assigned.
>  > 
>  > Fixes https://fedorahosted.org/sssd/ticket/2178
>  
>  ACK, the code looks good and seems to work fine. 
Thank you for the review, new versions attached.

byem
Sumit
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Thu, Dec 19, 2013 at 03:17:36PM +0100, Sumit Bose wrote:
>  On Thu, Dec 19, 2013 at 10:56:47AM +0100, Jakub Hrozek wrote:
>  > On Wed, Dec 18, 2013 at 04:48:44PM +0100, Sumit Bose wrote:
>  > > Hi,
>  > > 
>  > > the follwing patch set contains two different fixes but since they are
>  > > related and depend on each other I thought it might be easier to just
>  > > send them together. The first three patches should fix
>  > > https://fedorahosted.org/sssd/ticket/2178 while the last one should fix
>  > > cross-domain group-memberships which was broken by a previous patch by
>  > > me.
>  > > 
>  > > bye,
>  > > Sumit
>  > 
>  > I'm still reviewing the last patch, but I wanted to give some comments
>  > so we can work in parallel:
>  > 
>  > > From f410f0869d090048f4ab5aec5190bae0c82d5b2f Mon Sep 17 00:00:00 2001
>  > > From: Sumit Bose <sbose(a)redhat.com&gt;
>  > > Date: Mon, 9 Dec 2013 12:17:43 +0100
>  > > Subject: [PATCH 1/4] Add new option ldap_group_type
>  > > 
>  > > index 72586fb..82dcbba 100644
>  > > --- a/src/man/sssd-ldap.5.xml
>  > > +++ b/src/man/sssd-ldap.5.xml
>  > > @@ -849,6 +849,27 @@
>  > >                  </varlistentry>
>  > >  
>  > >                  <varlistentry>
>  > > +                    <term>ldap_group_type (integer)</term>
>  > > +                    <listitem>
>  > > +                        <para>
>  > > +                            The LDAP attribute that contains an integer value
>  > > +                            indicating the type of the group and maybe other
>  > > +                            flags.
>  > > +                        </para>
>  > > +                        <para>
>  > > +                            This attribute is currently only used by the AD
>  > > +                            privder to determine if a group is a domain local
>  > 
>  >                               ^^^^^
>  >                               typo
>  > > +                            groups and has to be filtered out for trusted
>  > > +                            domains.
>  > > +                        </para>
>  > > +                        <para>
>  > > +                            Default: groupType in the AD provider, othewise
> not
>  > > +                            set
>  > > +                        </para>
>  > > +                    </listitem>
>  > > +                </varlistentry>
>  > > +
>  > > +                <varlistentry>
>  > >                      <term>ldap_group_nesting_level
> (integer)</term>
>  > >                      <listitem>
>  > >                          <para>
>  > 
>  > Otherwise ACK
>  > 
>  > > From 717c1f39324e0d718021873f20f31154d7bf58f5 Mon Sep 17 00:00:00 2001
>  > > From: Sumit Bose <sbose(a)redhat.com&gt;
>  > > Date: Tue, 10 Dec 2013 10:14:02 +0100
>  > > Subject: [PATCH 2/4] Add sysdb_attrs_get_int32_t
>  > 
>  > > --- a/src/db/sysdb.h
>  > > +++ b/src/db/sysdb.h
>  > > @@ -294,6 +294,10 @@ errno_t sysdb_attrs_get_bool(struct sysdb_attrs *attrs,
> const char *name,
>  > >                               bool *value);
>  > >  int sysdb_attrs_get_uint16_t(struct sysdb_attrs *attrs, const char *name,
>  > >                               uint16_t *value);
>  > > +int sysdb_attrs_get_string(struct sysdb_attrs *attrs, const char *name,
>  > > +                           const char **string);
>  > 
>  > This ^^ addition is not needed, sysdb_attrs_get_string() was already
>  > declared in sysdb.h earlier.
>  > 
>  > > +int sysdb_attrs_get_int32_t(struct sysdb_attrs *attrs, const char *name,
>  > > +                             int32_t *value);
>  > >  int sysdb_attrs_get_uint32_t(struct sysdb_attrs *attrs, const char *name,
>  > >                               uint32_t *value);
>  > 
>  > > From 4b82b6439042550fd63023b23e876ba8ba35490e Mon Sep 17 00:00:00 2001
>  > > From: Sumit Bose <sbose(a)redhat.com&gt;
>  > > Date: Tue, 10 Dec 2013 10:14:28 +0100
>  > > Subject: [PATCH 3/4] AD: filter domain local groups for trusted/sub domains
>  > > 
>  > > In Active Directory groups with a domain local scope should only be used
>  > > inside of the specific domain. Since SSSD read the group memberships
>  > > from LDAP server of the user's domain the domain local groups are
>  > > included in the LDAP result. Those groups should be filtered out if the
>  > > domain is a sub/trusted domain, i.e. is not the domain the client
>  > > running SSSD is joined to.
>  > > 
>  > > The groups will still be in the cache but marked as non-POSIX groups and
>  > > no GID will be assigned.
>  > > 
>  > > Fixes https://fedorahosted.org/sssd/ticket/2178
>  > 
>  > ACK, the code looks good and seems to work fine.
>  
>  Thank you for the review, new versions attached.
>  
>  byem
>  Sumit 
ACK to these patches.

I'm waiting for Coverity scan to finish before pushing them. I'll be
crafting the release notes in the meantime.

[Jakub Hrozek]

On Thu, Dec 19, 2013 at 04:40:54PM +0100, Jakub Hrozek wrote:
>  > Thank you for the review, new versions attached.
>  > 
>  > byem
>  > Sumit
>  
>  ACK to these patches.
>  
>  I'm waiting for Coverity scan to finish before pushing them. I'll be
>  crafting the release notes in the meantime. 
Unfortunately our Coverity server is completely overwhelmed today. I
don't want to block the release so I'll push the patches and we'll fix
Coverity warnings (if any) later.

[Jakub Hrozek]

On Thu, Dec 19, 2013 at 05:32:12PM +0100, Jakub Hrozek wrote:
>  On Thu, Dec 19, 2013 at 04:40:54PM +0100, Jakub Hrozek wrote:
>  > > Thank you for the review, new versions attached.
>  > > 
>  > > byem
>  > > Sumit
>  > 
>  > ACK to these patches.
>  > 
>  > I'm waiting for Coverity scan to finish before pushing them. I'll be
>  > crafting the release notes in the meantime.
>  
>  Unfortunately our Coverity server is completely overwhelmed today. I
>  don't want to block the release so I'll push the patches and we'll fix
>  Coverity warnings (if any) later. 
Pushed all patches to master and sssd-1-11