Fix #2275 nested netgroups do not work in IPA provider

[Petr Cech]

Hi,

reverting this commit "5e9bc89b28f1ac3ce573ecdece74fe9623580c28" fixed 
the problem for me. So is the original commit no longer valid?

Regards,

Petr

[Lukas Slebodnik]

On (02/09/15 18:06), Petr Cech wrote:
> Hi,
>
> reverting this commit "5e9bc89b28f1ac3ce573ecdece74fe9623580c28" fixed the
> problem for me. So is the original commit no longer valid?

I'm little bit worried about reverting this patch.
Did you test the bug which was fixed by this commit.
@see https://fedorahosted.org/sssd/ticket/1519

LS

[Jakub Hrozek]

On Thu, Sep 03, 2015 at 06:15:24AM +0200, Lukas Slebodnik wrote:
> On (02/09/15 18:06), Petr Cech wrote:
> > Hi,
> >
> > reverting this commit "5e9bc89b28f1ac3ce573ecdece74fe9623580c28" fixed the
> > problem for me. So is the original commit no longer valid?
>
> I'm little bit worried about reverting this patch.
> Did you test the bug which was fixed by this commit.
> @see https://fedorahosted.org/sssd/ticket/1519

The author of the patch could help, too :-)

[Petr Cech]

On 09/03/2015 08:18 AM, Jakub Hrozek wrote:
> On Thu, Sep 03, 2015 at 06:15:24AM +0200, Lukas Slebodnik wrote:
> > On (02/09/15 18:06), Petr Cech wrote:
> > > Hi,
> > >
> > > reverting this commit "5e9bc89b28f1ac3ce573ecdece74fe9623580c28" fixed the
> > > problem for me. So is the original commit no longer valid?
> >
> > I'm little bit worried about reverting this patch.
> > Did you test the bug which was fixed by this commit.
> > @see https://fedorahosted.org/sssd/ticket/1519

Thanks.

Tested. We need both patches (because user groups are in memberOf and 
host groups are in orig_memberOf).
Simple, I will do it.

Is it OK that freeIPA use two kind of memberOf?
> The author of the patch could help, too :-)
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Thu, Sep 03, 2015 at 09:31:07AM +0200, Petr Cech wrote:
> On 09/03/2015 08:18 AM, Jakub Hrozek wrote:
> > On Thu, Sep 03, 2015 at 06:15:24AM +0200, Lukas Slebodnik wrote:
> > > On (02/09/15 18:06), Petr Cech wrote:
> > > > Hi,
> > > >
> > > > reverting this commit "5e9bc89b28f1ac3ce573ecdece74fe9623580c28" fixed the
> > > > problem for me. So is the original commit no longer valid?
> > >
> > > I'm little bit worried about reverting this patch.
> > > Did you test the bug which was fixed by this commit.
> > > @see https://fedorahosted.org/sssd/ticket/1519
>
> Thanks.
>
> Tested. We need both patches (because user groups are in memberOf and host
> groups are in orig_memberOf).
> Simple, I will do it.
>
> Is it OK that freeIPA use two kind of memberOf?

It does not. In FreeIPA LDAP there should only be memberOf (check it
out with openldap). What is happening is that we internally store IPA's
memberof value as originalMemberOf and our memberof points to cached
objects.

[Sumit Bose]

On Thu, Sep 03, 2015 at 09:54:51AM +0200, Jakub Hrozek wrote:
> On Thu, Sep 03, 2015 at 09:31:07AM +0200, Petr Cech wrote:
> > On 09/03/2015 08:18 AM, Jakub Hrozek wrote:
> > > On Thu, Sep 03, 2015 at 06:15:24AM +0200, Lukas Slebodnik wrote:
> > > > On (02/09/15 18:06), Petr Cech wrote:
> > > > > Hi,
> > > > >
> > > > > reverting this commit "5e9bc89b28f1ac3ce573ecdece74fe9623580c28" fixed the
> > > > > problem for me. So is the original commit no longer valid?
> > > >
> > > > I'm little bit worried about reverting this patch.
> > > > Did you test the bug which was fixed by this commit.
> > > > @see https://fedorahosted.org/sssd/ticket/1519
> >
> > Thanks.
> >
> > Tested. We need both patches (because user groups are in memberOf and host
> > groups are in orig_memberOf).
> > Simple, I will do it.
> >
> > Is it OK that freeIPA use two kind of memberOf?
>
> It does not. In FreeIPA LDAP there should only be memberOf (check it
> out with openldap). What is happening is that we internally store IPA's
> memberof value as originalMemberOf and our memberof points to cached
> objects.

yes and since we (so far) only store POSIX groups (user groups) in the
SSSD cache memberOf will only point to user groups. But as Jakub said
originalMemberOf should contain all memberOf attributres from the
related IPA LDAP object. Hence I would expect that originalMemberOf will
have a complete list of memberships with both user and host groups.

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Petr Cech]

On 09/03/2015 10:08 AM, Sumit Bose wrote:
> On Thu, Sep 03, 2015 at 09:54:51AM +0200, Jakub Hrozek wrote:
> > On Thu, Sep 03, 2015 at 09:31:07AM +0200, Petr Cech wrote:
> > > On 09/03/2015 08:18 AM, Jakub Hrozek wrote:
> > > > On Thu, Sep 03, 2015 at 06:15:24AM +0200, Lukas Slebodnik wrote:
> > > > > On (02/09/15 18:06), Petr Cech wrote:
> > > > > > Hi,
> > > > > >
> > > > > > reverting this commit "5e9bc89b28f1ac3ce573ecdece74fe9623580c28" fixed the
> > > > > > problem for me. So is the original commit no longer valid?
> > > > >
> > > > > I'm little bit worried about reverting this patch.
> > > > > Did you test the bug which was fixed by this commit.
> > > > > @see https://fedorahosted.org/sssd/ticket/1519
> > >
> > > Thanks.
> > >
> > > Tested. We need both patches (because user groups are in memberOf and host
> > > groups are in orig_memberOf).
> > > Simple, I will do it.
> > >
> > > Is it OK that freeIPA use two kind of memberOf?
> >
> > It does not. In FreeIPA LDAP there should only be memberOf (check it
> > out with openldap). What is happening is that we internally store IPA's
> > memberof value as originalMemberOf and our memberof points to cached
> > objects.
>
> yes and since we (so far) only store POSIX groups (user groups) in the
> SSSD cache memberOf will only point to user groups. But as Jakub said
> originalMemberOf should contain all memberOf attributres from the
> related IPA LDAP object. Hence I would expect that originalMemberOf will
> have a complete list of memberships with both user and host groups.
>
> bye,
> Sumit

I tried both case. I used only originalMemberOf and I had right 
hostgroups, no user groups. Then I used only memberOf and I had no 
hostgroups, right user groups.

So I did little hack, we could use both memberOf. The patch is attached 
and it works for me.

Petr
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

On Thu, Sep 03, 2015 at 01:50:35PM +0200, Petr Cech wrote:
> On 09/03/2015 10:08 AM, Sumit Bose wrote:
> > On Thu, Sep 03, 2015 at 09:54:51AM +0200, Jakub Hrozek wrote:
> > > On Thu, Sep 03, 2015 at 09:31:07AM +0200, Petr Cech wrote:
> > > > On 09/03/2015 08:18 AM, Jakub Hrozek wrote:
> > > > > On Thu, Sep 03, 2015 at 06:15:24AM +0200, Lukas Slebodnik wrote:
> > > > > > On (02/09/15 18:06), Petr Cech wrote:
> > > > > > >Hi,
> > > > > > >
> > > > > > >reverting this commit "5e9bc89b28f1ac3ce573ecdece74fe9623580c28" fixed the
> > > > > > >problem for me. So is the original commit no longer valid?
> > > > > > >
> > > > > > I'm little bit worried about reverting this patch.
> > > > > > Did you test the bug which was fixed by this commit.
> > > > > > @see https://fedorahosted.org/sssd/ticket/1519
> > > >
> > > > Thanks.
> > > >
> > > > Tested. We need both patches (because user groups are in memberOf and host
> > > > groups are in orig_memberOf).
> > > > Simple, I will do it.
> > > >
> > > > Is it OK that freeIPA use two kind of memberOf?
> > >
> > > It does not. In FreeIPA LDAP there should only be memberOf (check it
> > > out with openldap). What is happening is that we internally store IPA's
> > > memberof value as originalMemberOf and our memberof points to cached
> > > objects.
> >
> > yes and since we (so far) only store POSIX groups (user groups) in the
> > SSSD cache memberOf will only point to user groups. But as Jakub said
> > originalMemberOf should contain all memberOf attributres from the
> > related IPA LDAP object. Hence I would expect that originalMemberOf will
> > have a complete list of memberships with both user and host groups.
> >
> > bye,
> > Sumit
>
> I tried both case. I used only originalMemberOf and I had right hostgroups,
> no user groups. Then I used only memberOf and I had no hostgroups, right
> user groups.
>
> So I did little hack, we could use both memberOf. The patch is attached and
> it works for me.

Hi Petr,

thank you for the patch I haven't tested it yet. But I think I now
understand the issue better. Currently we store the originalMemberOf
attribute for users and hosts but not for POSIX/user groups (we do not
even read it from LDAP). So an alternative fix might be to add memberOf
attribute to the list of attribute read from LDAP for POSIX groups and
save the result in originalMemberOf in the cache. The using only
originalMemberOf should be sufficient for the netgroups lookup.

Would you mind to try this? For a test is shoult de sufficient to add a
line like

{ "ldap_group_member_of", "memberOf", SYSDB_MEMBEROF, NULL }

to all 'struct sdap_attr_map *_group_map[]' lists and a corresponding
entry to 'enum sdap_group_attrs'.

bye,
Sumit
> Petr
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Petr Cech]

On 09/03/2015 03:45 PM, Sumit Bose wrote:
> > I tried both case. I used only originalMemberOf and I had right hostgroups,
> > > no user groups. Then I used only memberOf and I had no hostgroups, right
> > > user groups.
> > >
> > > So I did little hack, we could use both memberOf. The patch is attached and
> > > it works for me.
>
> Hi Petr,
>
> thank you for the patch I haven't tested it yet. But I think I now
> understand the issue better. Currently we store the originalMemberOf
> attribute for users and hosts but not for POSIX/user groups (we do not
> even read it from LDAP). So an alternative fix might be to add memberOf
> attribute to the list of attribute read from LDAP for POSIX groups and
> save the result in originalMemberOf in the cache. The using only
> originalMemberOf should be sufficient for the netgroups lookup.
>
> Would you mind to try this? For a test is shoult de sufficient to add a
> line like
>  { "ldap_group_member_of", "memberOf", SYSDB_MEMBEROF, NULL }
>
> to all 'struct sdap_attr_map *_group_map[]' lists and a corresponding
> entry to 'enum sdap_group_attrs'.
>
> bye,
> Sumit

Hello Sumit,

I tried your alternative way (thanks for it). Patch is attached.
I added some lines like:
#  { "ldap_user_member_of", "memberOf", SYSDB_ORIG_MEMBEROF, NULL }
and it works for me.

I hope that meaning of this patch is saving user/POSIX group memberOf 
attribute to originalMemberOf attribute.

Regards,

Petr

[Petr Cech]

On 09/04/2015 03:24 PM, Petr Cech wrote:
> On 09/03/2015 03:45 PM, Sumit Bose wrote:
> > > I tried both case. I used only originalMemberOf and I had right
> > > hostgroups,
> > > > no user groups. Then I used only memberOf and I had no hostgroups,
> > >
> > > right
> > > > user groups.
> > > >
> > > > So I did little hack, we could use both memberOf. The patch is
> > >
> > > attached and
> > > > it works for me.
> >
> > Hi Petr,
> >
> > thank you for the patch I haven't tested it yet. But I think I now
> > understand the issue better. Currently we store the originalMemberOf
> > attribute for users and hosts but not for POSIX/user groups (we do not
> > even read it from LDAP). So an alternative fix might be to add memberOf
> > attribute to the list of attribute read from LDAP for POSIX groups and
> > save the result in originalMemberOf in the cache. The using only
> > originalMemberOf should be sufficient for the netgroups lookup.
> >
> > Would you mind to try this? For a test is shoult de sufficient to add a
> > line like
> >  { "ldap_group_member_of", "memberOf", SYSDB_MEMBEROF, NULL }
> >
> > to all 'struct sdap_attr_map *_group_map[]' lists and a corresponding
> > entry to 'enum sdap_group_attrs'.
> >
> > bye,
> > Sumit
>
> Hello Sumit,
>
> I tried your alternative way (thanks for it). Patch is attached.
> I added some lines like:
> #  { "ldap_user_member_of", "memberOf", SYSDB_ORIG_MEMBEROF, NULL }
> and it works for me.
>
> I hope that meaning of this patch is saving user/POSIX group memberOf
> attribute to originalMemberOf attribute.
>
> Regards,
>
> Petr

And there is version with ticket number.
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

On Fri, Sep 04, 2015 at 03:28:09PM +0200, Petr Cech wrote:
> On 09/04/2015 03:24 PM, Petr Cech wrote:
> > On 09/03/2015 03:45 PM, Sumit Bose wrote:
> > > > I tried both case. I used only originalMemberOf and I had right
> > > > hostgroups,
> > > > > no user groups. Then I used only memberOf and I had no hostgroups,
> > > >
> > > > right
> > > > > user groups.
> > > > >
> > > > > So I did little hack, we could use both memberOf. The patch is
> > > >
> > > > attached and
> > > > > it works for me.
> > >
> > > Hi Petr,
> > >
> > > thank you for the patch I haven't tested it yet. But I think I now
> > > understand the issue better. Currently we store the originalMemberOf
> > > attribute for users and hosts but not for POSIX/user groups (we do not
> > > even read it from LDAP). So an alternative fix might be to add memberOf
> > > attribute to the list of attribute read from LDAP for POSIX groups and
> > > save the result in originalMemberOf in the cache. The using only
> > > originalMemberOf should be sufficient for the netgroups lookup.
> > >
> > > Would you mind to try this? For a test is shoult de sufficient to add a
> > > line like
> > > { "ldap_group_member_of", "memberOf", SYSDB_MEMBEROF, NULL }
> > >
> > > to all 'struct sdap_attr_map *_group_map[]' lists and a corresponding
> > > entry to 'enum sdap_group_attrs'.
> > >
> > > bye,
> > > Sumit
> >
> > Hello Sumit,
> >
> > I tried your alternative way (thanks for it). Patch is attached.
> > I added some lines like:
> > #  { "ldap_user_member_of", "memberOf", SYSDB_ORIG_MEMBEROF, NULL }
> > and it works for me.
> >
> > I hope that meaning of this patch is saving user/POSIX group memberOf
> > attribute to originalMemberOf attribute.
> >
> > Regards,
> >
> > Petr
>
> And there is version with ticket number.
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Hi Petr,

thank you for your patience. Both versions of your patches work as
expected and fixes the issues with group and nested group memberships.

After testing a reading the current sources I think the best solution
might be a slight variant of your first patch where you check for both
memberOf and originalMemberOf but instead of always checking both I
would suggest to pass down the attribute to check by adding a new member
to struct extract_state and set it with the help of a new attribute to
extract_members(). Since extract_members() is called individually for
users and hosts in ipa_netgr_process_all() you can always pass the right
attribute. Btw. instead of using SYSDB_ORIG_MEMBEROF and
SYSDB_MEMBEROF explicitly you might want to use the configured mapping
which e.g. is available in ipa_netgr_process_all() as

state->ipa_opts->id->user_map[SDAP_AT_USER_MEMBEROF].sys_name

bye,
Sumit

[Petr Cech]

On 09/14/2015 01:53 PM, Sumit Bose wrote:
> Hi Petr,
>
> thank you for your patience. Both versions of your patches work as
> expected and fixes the issues with group and nested group memberships.
>
> After testing a reading the current sources I think the best solution
> might be a slight variant of your first patch where you check for both
> memberOf and originalMemberOf but instead of always checking both I
> would suggest to pass down the attribute to check by adding a new member
> to struct extract_state and set it with the help of a new attribute to
> extract_members(). Since extract_members() is called individually for
> users and hosts in ipa_netgr_process_all() you can always pass the right
> attribute.Btw. instead of using SYSDB_ORIG_MEMBEROF and
> SYSDB_MEMBEROF explicitly you might want to use the configured mapping
> which e.g. is available in ipa_netgr_process_all() as
>
> state->ipa_opts->id->user_map[SDAP_AT_USER_MEMBEROF].sys_name
>
> bye,
> Sumit

Hello Sumit,

there is next version of patch addresing your comments.

I didn't use statement like
# state->ipa_opts->id->user_map[SDAP_AT_USER_MEMBEROF].sys_name
because we don't have any for originalMemberOf. I'm not sure if I added 
originalMemberOf version of statement above wheter it is not more like 
the second version of patch. If I understood the problem right, we 
wouldn't have saved both kind of memberOf in our DB.

Regards
Petr

[Sumit Bose]

On Fri, Sep 18, 2015 at 02:33:03PM +0200, Petr Cech wrote:
> On 09/14/2015 01:53 PM, Sumit Bose wrote:
> > Hi Petr,
> >
> > thank you for your patience. Both versions of your patches work as
> > expected and fixes the issues with group and nested group memberships.
> >
> > After testing a reading the current sources I think the best solution
> > might be a slight variant of your first patch where you check for both
> > memberOf and originalMemberOf but instead of always checking both I
> > would suggest to pass down the attribute to check by adding a new member
> > to struct extract_state and set it with the help of a new attribute to
> > extract_members(). Since extract_members() is called individually for
> > users and hosts in ipa_netgr_process_all() you can always pass the right
> > attribute.Btw. instead of using SYSDB_ORIG_MEMBEROF and
> > SYSDB_MEMBEROF explicitly you might want to use the configured mapping
> > which e.g. is available in ipa_netgr_process_all() as
> >
> > state->ipa_opts->id->user_map[SDAP_AT_USER_MEMBEROF].sys_name
> >
> > bye,
> > Sumit
>
> Hello Sumit,
>
> there is next version of patch addresing your comments.
>
> I didn't use statement like
> # state->ipa_opts->id->user_map[SDAP_AT_USER_MEMBEROF].sys_name
> because we don't have any for originalMemberOf. I'm not sure if I added

we have,

state->ipa_opts->host_map[IPA_AT_HOST_MEMBER_OF].sys_name
> originalMemberOf version of statement above wheter it is not more like the
> second version of patch. If I understood the problem right, we wouldn't have
> saved both kind of memberOf in our DB.

yes, we do not save it in the cache, but I think I was confused myself
when I mentioned the cache earlier in the thread. I thought we used this
data from the cache to create the netgroup data, but the IPA provider
already saves netgroup triples to the cache which makes sense because
otherwise the responder must have knowledge about how netgroups are
stored on the server. So no kind of cached memberOf data is used to
create the netgroup entries, sorry for guiding you in the wrong
direction here.

However, the server side names aren't used as well. Since the most
common use case for the LDAP data we read from the server is to write
them into the cache and to make further processing more easy the
attribute names are translated automatically into the internal names
based on the various struct sdap_attr_map maps. Based on those
translated names the netgroups evaluation is done. As you can see in
e.g. src/providers/ipa/ipa_opts.h ""memberOf" of a user entry is
translated into "SYSDB_MEMBEROF" and ""memberOf" of a host entry is
translated into "SYSDB_ORIG_MEMBEROF". I hope this helps to make it more
clear.

Nevertheless the patch looks good and works as expected. I only found an
issues when there are duplicated entries dues to memberships in multiple
groups. I had to add

diff --git a/src/providers/ipa/ipa_netgroups.c
b/src/providers/ipa/ipa_netgroups.c
index 2c39257..bc89006 100644
--- a/src/providers/ipa/ipa_netgroups.c
+++ b/src/providers/ipa/ipa_netgroups.c
@@ -121,9 +121,9 @@ static errno_t ipa_save_netgroup(TALLOC_CTX
*mem_ctx,
         }
     } else {
         for(c = 0; c < el->num_values; c++) {
-            ret = sysdb_attrs_add_string(netgroup_attrs,
-                                         SYSDB_NETGROUP_TRIPLE,
-                                         (const
                                          char*)el->values[c].data);
+            ret = sysdb_attrs_add_string_safe(netgroup_attrs,
+                                              SYSDB_NETGROUP_TRIPLE,
+                                              (const char*)el->values[c].data);
             if (ret) {
                 goto fail;
             }

to make it work in my tests.

bye,
Sumit
> Regards
> Petr

[Petr Cech]

On 09/21/2015 01:27 PM, Sumit Bose wrote:
> > Hello Sumit,
> > > there is next version of patch addresing your comments.
> > >
> > > I didn't use statement like
> > > # state->ipa_opts->id->user_map[SDAP_AT_USER_MEMBEROF].sys_name
> > > because we don't have any for originalMemberOf. I'm not sure if I added
>
> we have,
>
> state->ipa_opts->host_map[IPA_AT_HOST_MEMBER_OF].sys_name
> > > originalMemberOf version of statement above wheter it is not more like the
> > > second version of patch. If I understood the problem right, we wouldn't have
> > > saved both kind of memberOf in our DB.
>
> yes, we do not save it in the cache, but I think I was confused myself
> when I mentioned the cache earlier in the thread. I thought we used this
> data from the cache to create the netgroup data, but the IPA provider
> already saves netgroup triples to the cache which makes sense because
> otherwise the responder must have knowledge about how netgroups are
> stored on the server. So no kind of cached memberOf data is used to
> create the netgroup entries, sorry for guiding you in the wrong
> direction here.
>
> However, the server side names aren't used as well. Since the most
> common use case for the LDAP data we read from the server is to write
> them into the cache and to make further processing more easy the
> attribute names are translated automatically into the internal names
> based on the various struct sdap_attr_map maps. Based on those
> translated names the netgroups evaluation is done. As you can see in
> e.g. src/providers/ipa/ipa_opts.h ""memberOf" of a user entry is
> translated into "SYSDB_MEMBEROF" and ""memberOf" of a host entry is
> translated into "SYSDB_ORIG_MEMBEROF". I hope this helps to make it more
> clear.

Hello Sumit,

thank you for your patience. There is fixed patch attached. I used the 
predefined mapping.
> Nevertheless the patch looks good and works as expected. I only found an
> issues when there are duplicated entries dues to memberships in multiple
> groups. I had to add
>
> diff --git a/src/providers/ipa/ipa_netgroups.c
> b/src/providers/ipa/ipa_netgroups.c
> index 2c39257..bc89006 100644
> --- a/src/providers/ipa/ipa_netgroups.c
> +++ b/src/providers/ipa/ipa_netgroups.c
> @@ -121,9 +121,9 @@ static errno_t ipa_save_netgroup(TALLOC_CTX
> *mem_ctx,
>           }
>       } else {
>           for(c = 0; c < el->num_values; c++) {
>        ret = sysdb_attrs_add_string(netgroup_attrs,
>                                     SYSDB_NETGROUP_TRIPLE,
>                                     (const
>                                      char*)el->values[c].data);

I saw another 2 occurrences of calling 'sysdb_attrs_add_string()' in for 
loop. Maybe there should be 'safe' variant too.

Regards
Petr
>        ret = sysdb_attrs_add_string_safe(netgroup_attrs,
>                                          SYSDB_NETGROUP_TRIPLE,
>                                          (const char*)el->values[c].data);
>         if (ret) {
>             goto fail;
>         }
>
> to make it work in my tests.
>
> bye,
> Sumit
> > > Regards
> > > Petr

[Sumit Bose]

On Mon, Sep 21, 2015 at 02:49:53PM +0200, Petr Cech wrote:
> On 09/21/2015 01:27 PM, Sumit Bose wrote:
> > > Hello Sumit,
> > > > there is next version of patch addresing your comments.
> > > >
> > > > I didn't use statement like
> > > > # state->ipa_opts->id->user_map[SDAP_AT_USER_MEMBEROF].sys_name
> > > > because we don't have any for originalMemberOf. I'm not sure if I added
> >
> > we have,
> >
> > state->ipa_opts->host_map[IPA_AT_HOST_MEMBER_OF].sys_name
> > > > originalMemberOf version of statement above wheter it is not more like the
> > > > second version of patch. If I understood the problem right, we wouldn't have
> > > > saved both kind of memberOf in our DB.
> >
> > yes, we do not save it in the cache, but I think I was confused myself
> > when I mentioned the cache earlier in the thread. I thought we used this
> > data from the cache to create the netgroup data, but the IPA provider
> > already saves netgroup triples to the cache which makes sense because
> > otherwise the responder must have knowledge about how netgroups are
> > stored on the server. So no kind of cached memberOf data is used to
> > create the netgroup entries, sorry for guiding you in the wrong
> > direction here.
> >
> > However, the server side names aren't used as well. Since the most
> > common use case for the LDAP data we read from the server is to write
> > them into the cache and to make further processing more easy the
> > attribute names are translated automatically into the internal names
> > based on the various struct sdap_attr_map maps. Based on those
> > translated names the netgroups evaluation is done. As you can see in
> > e.g. src/providers/ipa/ipa_opts.h ""memberOf" of a user entry is
> > translated into "SYSDB_MEMBEROF" and ""memberOf" of a host entry is
> > translated into "SYSDB_ORIG_MEMBEROF". I hope this helps to make it more
> > clear.
>
> Hello Sumit,
>
> thank you for your patience. There is fixed patch attached. I used the
> predefined mapping.
> > Nevertheless the patch looks good and works as expected. I only found an
> > issues when there are duplicated entries dues to memberships in multiple
> > groups. I had to add
> >
> > diff --git a/src/providers/ipa/ipa_netgroups.c
> > b/src/providers/ipa/ipa_netgroups.c
> > index 2c39257..bc89006 100644
> > --- a/src/providers/ipa/ipa_netgroups.c
> > +++ b/src/providers/ipa/ipa_netgroups.c
> > @@ -121,9 +121,9 @@ static errno_t ipa_save_netgroup(TALLOC_CTX
> > *mem_ctx,
> >          }
> >      } else {
> >          for(c = 0; c < el->num_values; c++) {
> >        ret = sysdb_attrs_add_string(netgroup_attrs,
> >                                     SYSDB_NETGROUP_TRIPLE,
> >                                     (const
> >                                     char*)el->values[c].data);
>
> I saw another 2 occurrences of calling 'sysdb_attrs_add_string()' in for
> loop. Maybe there should be 'safe' variant too.

The 'safe' variant comes with a cost and should be only used when really
needed. I think the other cases are safe (famous last words :-).

The patches are looking good and fixed the netgroups issues with group
and nested group memberships. CI passes
http://sssd-ci.duckdns.org/logs/job/27/43/summary.html except the debian
test but this is an unrelated known issue, so ACK.

Thank you for your patience as well.

bye,
Sumit
> Regards
> Petr
> >        ret = sysdb_attrs_add_string_safe(netgroup_attrs,
> >                                          SYSDB_NETGROUP_TRIPLE,
> >                                          (const char*)el->values[c].data);
> >        if (ret) {
> >            goto fail;
> >        }
> >
> > to make it work in my tests.
> >
> > bye,
> > Sumit
> > > > Regards
> > > > Petr

[Jakub Hrozek]

On Tue, Sep 22, 2015 at 11:40:46AM +0200, Sumit Bose wrote:
> The 'safe' variant comes with a cost and should be only used when really
> needed. I think the other cases are safe (famous last words :-).
>
> The patches are looking good and fixed the netgroups issues with group
> and nested group memberships. CI passes
> http://sssd-ci.duckdns.org/logs/job/27/43/summary.html except the debian
> test but this is an unrelated known issue, so ACK.
>
> Thank you for your patience as well.
>
> bye,
> Sumit

* master: e6595222c41af84288d303e8d464ce45b1408ed3

[Jakub Hrozek]

On Thu, Sep 03, 2015 at 09:54:51AM +0200, Jakub Hrozek wrote:
> On Thu, Sep 03, 2015 at 09:31:07AM +0200, Petr Cech wrote:
> > On 09/03/2015 08:18 AM, Jakub Hrozek wrote:
> > > On Thu, Sep 03, 2015 at 06:15:24AM +0200, Lukas Slebodnik wrote:
> > > > On (02/09/15 18:06), Petr Cech wrote:
> > > > > Hi,
> > > > >
> > > > > reverting this commit "5e9bc89b28f1ac3ce573ecdece74fe9623580c28" fixed the
> > > > > problem for me. So is the original commit no longer valid?
> > > >
> > > > I'm little bit worried about reverting this patch.
> > > > Did you test the bug which was fixed by this commit.
> > > > @see https://fedorahosted.org/sssd/ticket/1519
> >
> > Thanks.
> >
> > Tested. We need both patches (because user groups are in memberOf and host
> > groups are in orig_memberOf).
> > Simple, I will do it.
> >
> > Is it OK that freeIPA use two kind of memberOf?
>
> It does not. In FreeIPA LDAP there should only be memberOf (check it
> out with openldap). What is happening is that we internally store IPA's

~~~~~~~~
        sorry I meany ldapsearch here.
> memberof value as originalMemberOf and our memberof points to cached
> objects.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel