[PATCH] Resolve IPA external group members during getgr* requests

[Jakub Hrozek]

Hi,

the attached patches implement https://fedorahosted.org/sssd/ticket/2522

Here is what I tested:
    1) topgr -> bottomgr -> extgr -> administrator(a)win.trust.test
        - this is a simple case to test a member of an external group

    2) ipa_uni -> ipa_uni_ext -> unigroup(a)win.trust.test
                                    |
                                    +-- userfromchild(a)child.win.trust.test
                                     \- anotheruser(a)win.trust.test

        - this is to test that a universal group with members from two
          domains resolves correctly

    3) idm_admin -> idm_admin_ext -> extgroup(a)win.trust.test
                                         \- puser(a)win.trust.test
                                  -> someuser(a)win.trust.test
        - tests that a combination of user and group members work

    4) toc_admin -> toc_admin_ext -> someuser(a)win.trust.test
                                  -> someuser(a)child.win.trust.test
        - tests that two users with the same name from different domains
          work correctly

I also tested adding and removing group members and found some bugs in
our groups processing (#2940, #2952)

Please feel free to suggest more tests. If the patches are accepted,
I'll also add unit tests for the nested groups code, but currently we
need to make the patches available to upstream.

I also tested a combination of user lookups and group lookups,
especially with someuser to test that the initgroups and group lookups
play well together. And that's where I found one issue that I think we
should fix in master -- the initgroups code adds the external members as
direct members of the POSIX group, so I did the same in the group
lookup code. This is safer, but doesn't allow us to remove the hack we
have (6fac5e5f0c54a0f92872ce1450606cfcb577a920) to retain external
members.

I think in 1.14 it would be more systematic to store the group
membership as:
    gr -> ext_gr -> ad_member
rather than:
    gr -> ext_gr
       -> ad_member

The reason we can't remove those hacks is that if the external group is
up-to-date and only the top group is resolved, we would only write the
group memberships as stored in LDAP, effectively removing the AD
memberships. This of course depends on fixing the issues in memberof
plugin, but I still think it's worth it. The current code works well,
though, so I guess we can accept it for 1.13.

Our initgroups code should be well equipped to handle that and we could
remove the hacks we have at the moment.

[Jakub Hrozek]

On Mon, Feb 22, 2016 at 06:04:07PM +0100, Jakub Hrozek wrote:
>  Hi,
>  
>  the attached patches implement https://fedorahosted.org/sssd/ticket/2522
>  
>  Here is what I tested:
>      1) topgr -> bottomgr -> extgr -> administrator(a)win.trust.test
>          - this is a simple case to test a member of an external group
>  
>      2) ipa_uni -> ipa_uni_ext -> unigroup(a)win.trust.test
>                                      |
>                                      +-- userfromchild(a)child.win.trust.test
>                                       \- anotheruser(a)win.trust.test
>  
>          - this is to test that a universal group with members from two
>            domains resolves correctly
>  
>      3) idm_admin -> idm_admin_ext -> extgroup(a)win.trust.test
>                                           \- puser(a)win.trust.test
>                                    -> someuser(a)win.trust.test
>          - tests that a combination of user and group members work
>  
>      4) toc_admin -> toc_admin_ext -> someuser(a)win.trust.test
>                                    -> someuser(a)child.win.trust.test
>          - tests that two users with the same name from different domains
>            work correctly
>  
>  I also tested adding and removing group members and found some bugs in
>  our groups processing (#2940, #2952)
>  
>  Please feel free to suggest more tests. If the patches are accepted,
>  I'll also add unit tests for the nested groups code, but currently we
>  need to make the patches available to upstream.
>  
>  I also tested a combination of user lookups and group lookups,
>  especially with someuser to test that the initgroups and group lookups
>  play well together. And that's where I found one issue that I think we
>  should fix in master -- the initgroups code adds the external members as
>  direct members of the POSIX group, so I did the same in the group
>  lookup code. This is safer, but doesn't allow us to remove the hack we
>  have (6fac5e5f0c54a0f92872ce1450606cfcb577a920) to retain external
>  members.
>  
>  I think in 1.14 it would be more systematic to store the group
>  membership as:
>      gr -> ext_gr -> ad_member
>  rather than:
>      gr -> ext_gr
>         -> ad_member
>  
>  The reason we can't remove those hacks is that if the external group is
>  up-to-date and only the top group is resolved, we would only write the
>  group memberships as stored in LDAP, effectively removing the AD
>  memberships. This of course depends on fixing the issues in memberof
>  plugin, but I still think it's worth it. The current code works well,
>  though, so I guess we can accept it for 1.13.
>  
>  Our initgroups code should be well equipped to handle that and we could
>  remove the hacks we have at the moment. 
The attached patches fix issues in the reallocation logic that Sumit
found during his review. Only the third patch had changed.

[Sumit Bose]

On Tue, Feb 23, 2016 at 09:37:57PM +0100, Jakub Hrozek wrote:
>  On Mon, Feb 22, 2016 at 06:04:07PM +0100, Jakub Hrozek wrote:
>  > Hi,
>  > 
>  > the attached patches implement https://fedorahosted.org/sssd/ticket/2522
>  > 
>  > Here is what I tested:
>  >     1) topgr -> bottomgr -> extgr -> administrator(a)win.trust.test
>  >         - this is a simple case to test a member of an external group
>  > 
>  >     2) ipa_uni -> ipa_uni_ext -> unigroup(a)win.trust.test
>  >                                     |
>  >                                     +-- userfromchild(a)child.win.trust.test
>  >                                      \- anotheruser(a)win.trust.test
>  > 
>  >         - this is to test that a universal group with members from two
>  >           domains resolves correctly
>  > 
>  >     3) idm_admin -> idm_admin_ext -> extgroup(a)win.trust.test
>  >                                          \- puser(a)win.trust.test
>  >                                   -> someuser(a)win.trust.test
>  >         - tests that a combination of user and group members work
>  > 
>  >     4) toc_admin -> toc_admin_ext -> someuser(a)win.trust.test
>  >                                   -> someuser(a)child.win.trust.test
>  >         - tests that two users with the same name from different domains
>  >           work correctly
>  > 
>  > I also tested adding and removing group members and found some bugs in
>  > our groups processing (#2940, #2952)
>  > 
>  > Please feel free to suggest more tests. If the patches are accepted,
>  > I'll also add unit tests for the nested groups code, but currently we
>  > need to make the patches available to upstream.
>  > 
>  > I also tested a combination of user lookups and group lookups,
>  > especially with someuser to test that the initgroups and group lookups
>  > play well together. And that's where I found one issue that I think we
>  > should fix in master -- the initgroups code adds the external members as
>  > direct members of the POSIX group, so I did the same in the group
>  > lookup code. This is safer, but doesn't allow us to remove the hack we
>  > have (6fac5e5f0c54a0f92872ce1450606cfcb577a920) to retain external
>  > members.
>  > 
>  > I think in 1.14 it would be more systematic to store the group
>  > membership as:
>  >     gr -> ext_gr -> ad_member
>  > rather than:
>  >     gr -> ext_gr
>  >        -> ad_member
>  > 
>  > The reason we can't remove those hacks is that if the external group is
>  > up-to-date and only the top group is resolved, we would only write the
>  > group memberships as stored in LDAP, effectively removing the AD
>  > memberships. This of course depends on fixing the issues in memberof
>  > plugin, but I still think it's worth it. The current code works well,
>  > though, so I guess we can accept it for 1.13.
>  > 
>  > Our initgroups code should be well equipped to handle that and we could
>  > remove the hacks we have at the moment.
>  
>  The attached patches fix issues in the reallocation logic that Sumit
>  found during his review. Only the third patch had changed. 
Thank you. I do not have additional comments on the code and the patches
work well in my test. I tested with nested groups on the AD and IPA side
as well to put even more groups between the group with them members in
AD and the group looked up on the IPA side but all worked well. CI
passed as well http://sssd-ci.duckdns.org/logs/job/37/87/summary.html so

ACK.

Btw, about the change you suggested to store the AD members in the
external groups. All PAC processing does not know about the external
group and can only lookup up and add the AD members to the real group.
So I'm not sure if this change is really needed.

bye,
Sumit

[Jakub Hrozek]

On Wed, Feb 24, 2016 at 01:17:27PM +0100, Sumit Bose wrote:
>  On Tue, Feb 23, 2016 at 09:37:57PM +0100, Jakub Hrozek wrote:
>  > On Mon, Feb 22, 2016 at 06:04:07PM +0100, Jakub Hrozek wrote:
>  > > Hi,
>  > > 
>  > > the attached patches implement https://fedorahosted.org/sssd/ticket/2522
>  > > 
>  > > Here is what I tested:
>  > >     1) topgr -> bottomgr -> extgr -> administrator(a)win.trust.test
>  > >         - this is a simple case to test a member of an external group
>  > > 
>  > >     2) ipa_uni -> ipa_uni_ext -> unigroup(a)win.trust.test
>  > >                                     |
>  > >                                     +-- userfromchild(a)child.win.trust.test
>  > >                                      \- anotheruser(a)win.trust.test
>  > > 
>  > >         - this is to test that a universal group with members from two
>  > >           domains resolves correctly
>  > > 
>  > >     3) idm_admin -> idm_admin_ext -> extgroup(a)win.trust.test
>  > >                                          \- puser(a)win.trust.test
>  > >                                   -> someuser(a)win.trust.test
>  > >         - tests that a combination of user and group members work
>  > > 
>  > >     4) toc_admin -> toc_admin_ext -> someuser(a)win.trust.test
>  > >                                   -> someuser(a)child.win.trust.test
>  > >         - tests that two users with the same name from different domains
>  > >           work correctly
>  > > 
>  > > I also tested adding and removing group members and found some bugs in
>  > > our groups processing (#2940, #2952)
>  > > 
>  > > Please feel free to suggest more tests. If the patches are accepted,
>  > > I'll also add unit tests for the nested groups code, but currently we
>  > > need to make the patches available to upstream.
>  > > 
>  > > I also tested a combination of user lookups and group lookups,
>  > > especially with someuser to test that the initgroups and group lookups
>  > > play well together. And that's where I found one issue that I think we
>  > > should fix in master -- the initgroups code adds the external members as
>  > > direct members of the POSIX group, so I did the same in the group
>  > > lookup code. This is safer, but doesn't allow us to remove the hack we
>  > > have (6fac5e5f0c54a0f92872ce1450606cfcb577a920) to retain external
>  > > members.
>  > > 
>  > > I think in 1.14 it would be more systematic to store the group
>  > > membership as:
>  > >     gr -> ext_gr -> ad_member
>  > > rather than:
>  > >     gr -> ext_gr
>  > >        -> ad_member
>  > > 
>  > > The reason we can't remove those hacks is that if the external group is
>  > > up-to-date and only the top group is resolved, we would only write the
>  > > group memberships as stored in LDAP, effectively removing the AD
>  > > memberships. This of course depends on fixing the issues in memberof
>  > > plugin, but I still think it's worth it. The current code works well,
>  > > though, so I guess we can accept it for 1.13.
>  > > 
>  > > Our initgroups code should be well equipped to handle that and we could
>  > > remove the hacks we have at the moment.
>  > 
>  > The attached patches fix issues in the reallocation logic that Sumit
>  > found during his review. Only the third patch had changed.
>  
>  Thank you. I do not have additional comments on the code and the patches
>  work well in my test. I tested with nested groups on the AD and IPA side
>  as well to put even more groups between the group with them members in
>  AD and the group looked up on the IPA side but all worked well. CI
>  passed as well http://sssd-ci.duckdns.org/logs/job/37/87/summary.html so
>  
>  ACK.
>  
>  Btw, about the change you suggested to store the AD members in the
>  external groups. All PAC processing does not know about the external
>  group and can only lookup up and add the AD members to the real group.
>  So I'm not sure if this change is really needed.
>  
>  bye,
>  Sumit 
Thank you for the review:
    master:
        3cf7fdfcaedb986f42a6640e26aa057007b64045
        e2d96566aeb881bd89e5c9236d663f6a9a88019a
        c32266e79f9d4bebd0c31eaa8d6fa26050e7fb3e 
    sssd-1-13:
        7db3bdfd6b1b845866c1ff062d25de5804141e89
        00ee45423f0712b83926c6f8b354a1a18ff741c8
        19194cb18a1cc20f02423861dd831aa5bc3a1003