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