LDAP/AD: resolve domain local groups for remote users

[sumit-bose]

This set of patches should solve https://fedorahosted.org/sssd/ticket/3206. To
test this you have to join SSSD into an AD forest with at least two domains.
Assuming the domain SSSD is joined to is called A and there is another domain
in the forest called B you have to create a domain local group in domain A,
e.g. A\dom_loc. Then create e.g. a universal group in domain B, e.g. B\uni and
make B\uni a member of A\dom_loc. Any user from domain B which is a member of
B\uni should now be a member of A\dom_loc as well because SSSD is joined to
domain A and any domain local group from domain A is valid in this case.

If SSSD is joined to domain B A\dom_loc is not a valid group in this domain and
any user which is a member of B\uni should not be a member of A\dom_loc.

The lookup to the domain local groups is added as a second step after the
initial initgroups lookup and should work with and without the tokenGroups
request.

There is one issue I currently do not have a solution for. It is possible to
add the user from domain B to A\dom_loc directly. It this user is an indirect
member of A\dom_loc as well, e.g. via B\uni, and this direct membership is
removed the initgroups request will not return A\dom_loc. This is due to the
nature of SSSD's cache. The memberOf attribute will be removed from the user
entry during the initial lookup because this lookup is not aware of the domain
local groups and will remove the missing direct parent. During the domain local
lookup no changes are detected, because the direct membership is already
removed and there are no changes in the objects related to the indirect
membership. Even adding all nested memberships again won't help because the
sysdb code will detect that the membership is already present and skip it. And
unconditionally removing and adding all nested membership again sounds too
costly to me for this special case especially since the plan to use the PAC to
determine the memberships in the next release even for user which are not
already logged in.

[jhrozek]

Hmm, looks like github ate my mail, so let's paste the comment again (and sorry if it arrives twice). Coverity detected some warnings:

Error: COMPILER_WARNING:
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c:1554:12: warning: unused variable 'd' [-Wunused-variable]
#     size_t d;
#            ^
# 1552|       int ret;
# 1553|       size_t c;
# 1554|->     size_t d;
# 1555|       char **groupnamelist = NULL;
# 1556|       struct sysdb_attrs *groups[1];

Error: COMPILER_WARNING:
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c:1562:25: warning: unused variable 'msg' [-Wunused-variable]
#     struct ldb_message *msg;
#                         ^
# 1560|       const char *class;
# 1561|       struct sss_domain_info *obj_dom;
# 1562|->     struct ldb_message *msg;
# 1563|       struct ldb_message_element *el;
# 1564|       const char *obj_attrs[] = {SYSDB_NAME, SYSDB_MEMBEROF, NULL};

Error: COMPILER_WARNING:
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c:1563:33: warning: unused variable 'el' [-Wunused-variable]
#     struct ldb_message_element *el;
#                                 ^
# 1561|       struct sss_domain_info *obj_dom;
# 1562|       struct ldb_message *msg;
# 1563|->     struct ldb_message_element *el;
# 1564|       const char *obj_attrs[] = {SYSDB_NAME, SYSDB_MEMBEROF, NULL};
# 1565|       char *local_groups_base_dn;

Error: COMPILER_WARNING:
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c:1564:17: warning: unused variable 'obj_attrs' [-Wunused-variable]
#     const char *obj_attrs[] = {SYSDB_NAME, SYSDB_MEMBEROF, NULL};
#                 ^
# 1562|       struct ldb_message *msg;
# 1563|       struct ldb_message_element *el;
# 1564|->     const char *obj_attrs[] = {SYSDB_NAME, SYSDB_MEMBEROF, NULL};
# 1565|       char *local_groups_base_dn;
# 1566|       uint8_t *obj_base_dn;

Error: COMPILER_WARNING:
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c:1566:14: warning: unused variable 'obj_base_dn' [-Wunused-variable]
#     uint8_t *obj_base_dn;
#              ^
# 1564|       const char *obj_attrs[] = {SYSDB_NAME, SYSDB_MEMBEROF, NULL};
# 1565|       char *local_groups_base_dn;
# 1566|->     uint8_t *obj_base_dn;
# 1567|       char **cached_local_parents = NULL;
# 1568|       uint8_t *name_start;

Error: COMPILER_WARNING:
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c: scope_hint: In function 'sdap_ad_get_domain_local_groups_parse_parents'
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c:1568:14: warning: unused variable 'name_start' [-Wunused-variable]
#     uint8_t *name_start;
#              ^
# 1566|       uint8_t *obj_base_dn;
# 1567|       char **cached_local_parents = NULL;
# 1568|->     uint8_t *name_start;
# 1569|       char **add_list = NULL;
# 1570|       char **del_list = NULL;

[jhrozek]

I wonder if this check is safe for the future. We already have a request to support group names with @ in them, so I think this check could cause some false positives in the future. I wonder if it was safer to always parse the name and also pass in the "expected" domain and compare if the parsed and expected domain match. I think if the function only performed a check and didn't return anything except a true/false boolean, we could even avoid memory allocation, so the performance impact would be minimal.

[sumit-bose]

I removed this patch and modified the caller rfc2307bis_nested_groups_step() to check if sdap_get_group_primary_name() must be called or not.

[jhrozek]

This comment is not related to this patch per se, but I was recently looking at performance issues where the user was a member of a huge number of groups (thousands) and just issuing the requests one-by-one took a long time there. I was wondering what do you think in general about issuing a number of parallel requests up to a certain point (maybe up to 5 or 10 parallel lookups at the time..)?

[sumit-bose]

In general yes, but I think we should do it on a case-by-case basis and we have to take care not to overrun the server-side because I think there are limits how many parallel requests can be send from a single connection. An alternative to parallel request might be to make the search filter longer so that the server can return multiple results in a single request.

That said I think rfc2307bis_nested_groups_send() might not be the most important one to optimize because this path is not used with the default tokenGroups based lookup and with the current patch only by the AD provider for user from different domains. And since we try to add the 24U2Self/PAC based lookups for the next release this might be gone soon in the default setup as well.

But in the request where we lookup groups based on SIDs we got from the PAC or the tokenGroups request this kind of improvement would make sense.

[jhrozek]

Same comment about detecting a qualified name as I had before applies also here.

[sumit-bose]

I removed the check and added a comment that at this point we process data from LDAP and we unconditionally can expand the name to an internal fully-qualified name.

[jhrozek]

I think we can make this optimization even a bit more aggressive by also checking if either list only contains a NULL. In one of my basic tests (with no domain-local groups so far..) I'm seeing:

(gdb) p del_list
$4 = (char **) 0x14421e0
(gdb) p del_list[0]
$5 = 0x0

It's not a big deal because sysdb_update_members would then just loop through the list, but at least we would avoid the (now potentially confusing) debug message later and avoid starting and then commiting an empty transaction in sysdb_update_members.

[jhrozek]

so far more cases need to be tested, both manually and using tests, but maybe we can at least discuss the code for now

[lslebodn]

retest this please

[jhrozek]

Thanks for the changes, at least with the previous version all the tests with the domain-local groups were working for me and the internal ad_forest test didn't catch any new regressions (some tests are failing, but those are failing even with the old version). I haven't had the chance to run the IPA-AD tests yet.

[lslebodn]

On (21/10/16 01:22), Jakub Hrozek wrote:

Hmm, looks like github ate my mail, so let's paste the comment again (and sorry if it arrives twice). Coverity detected some warnings:
It has nothing to do with coverity.
All warnings are reported by gcc.

covscan != coverity

LS

[sumit-bose]

On Fri, Oct 21, 2016 at 01:22:08AM -0700, Jakub Hrozek wrote:

Hmm, looks like github ate my mail, so let's paste the comment again (and sorry if it arrives twice). Coverity detected some warnings:

Error: COMPILER_WARNING:
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c:1554:12: warning: unused variable 'd' [-Wunused-variable]
#     size_t d;
#            ^
....

hm, I wonder why gcc 4.9.2 didn't show those warnings for
sdap_async_initgroups_ad.c but shows it for other files?

Nevertheless, I can see the warnings with newer versions of gcc and
updated the patch.

bye,
Sumit

[jhrozek]

On Wed, Oct 19, 2016 at 02:45:02AM -0700, sumit-bose wrote:

This set of patches should solve https://fedorahosted.org/sssd/ticket/3206. To
test this you have to join SSSD into an AD forest with at least two domains.
Assuming the domain SSSD is joined to is called A and there is another domain
in the forest called B you have to create a domain local group in domain A,
e.g. A\dom_loc. Then create e.g. a universal group in domain B, e.g. B\uni and
make B\uni a member of A\dom_loc. Any user from domain B which is a member of
B\uni should now be a member of A\dom_loc as well because SSSD is joined to
domain A and any domain local group from domain A is valid in this case.

If SSSD is joined to domain B A\dom_loc is not a valid group in this domain and
any user which is a member of B\uni should not be a member of A\dom_loc.

The lookup to the domain local groups is added as a second step after the
initial initgroups lookup and should work with and without the tokenGroups
request.

There is one issue I currently do not have a solution for. It is possible to
add the user from domain B to A\dom_loc directly. It this user is an indirect
member of A\dom_loc as well, e.g. via B\uni, and this direct membership is
removed the initgroups request will not return A\dom_loc. This is due to the
nature of SSSD's cache. The memberOf attribute will be removed from the user
entry during the initial lookup because this lookup is not aware of the domain
local groups and will remove the missing direct parent. During the domain local
lookup no changes are detected, because the direct membership is already
removed and there are no changes in the objects related to the indirect
membership. Even adding all nested memberships again won't help because the
sysdb code will detect that the membership is already present and skip it. And
unconditionally removing and adding all nested membership again sounds too
costly to me for this special case especially since the plan to use the PAC to
determine the memberships in the next release even for user which are not
already logged in.
You can view, comment on, or merge this pull request online at:

#57

-- Commit Summary --

• sysdb: add parent_dom to sysdb_get_direct_parents()
• sdap: sdap_get_primary_fqdn() check if name is already fully-qualified
• sdap: make some nested group related calls public
• LDAP/AD: resolve domain local groups for remote users

-- File Changes --

M src/db/sysdb.h (22)
M src/db/sysdb_search.c (7)
M src/providers/ldap/sdap.c (12)
M src/providers/ldap/sdap_async_initgroups.c (162)
M src/providers/ldap/sdap_async_initgroups_ad.c (409)
M src/providers/ldap/sdap_async_private.h (26)

-- Patch Links --

https://github.com/SSSD/sssd/pull/57.patch
https://github.com/SSSD/sssd/pull/57.diff

As first part of the review, I ran Coverity and it found some warnings:

rror: COMPILER_WARNING:
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c:1554:12:
warning: unused variable 'd' [-Wunused-variable]
#     size_t d;
#            ^
# 1552|       int ret;
# 1553|       size_t c;
# 1554|->     size_t d;
# 1555|       char **groupnamelist = NULL;
# 1556|       struct sysdb_attrs *groups[1];

Error: COMPILER_WARNING:
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c:1562:25:
warning: unused variable 'msg' [-Wunused-variable]
#     struct ldb_message *msg;
#                         ^
# 1560|       const char *class;
# 1561|       struct sss_domain_info *obj_dom;
# 1562|->     struct ldb_message *msg;
# 1563|       struct ldb_message_element *el;
# 1564|       const char *obj_attrs[] = {SYSDB_NAME, SYSDB_MEMBEROF,
# NULL};

Error: COMPILER_WARNING:
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c:1563:33:
warning: unused variable 'el' [-Wunused-variable]
#     struct ldb_message_element *el;
#                                 ^
# 1561|       struct sss_domain_info *obj_dom;
# 1562|       struct ldb_message *msg;
# 1563|->     struct ldb_message_element *el;
# 1564|       const char *obj_attrs[] = {SYSDB_NAME, SYSDB_MEMBEROF,
# NULL};
# 1565|       char *local_groups_base_dn;

Error: COMPILER_WARNING:
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c:1564:17:
warning: unused variable 'obj_attrs' [-Wunused-variable]
#     const char *obj_attrs[] = {SYSDB_NAME, SYSDB_MEMBEROF, NULL};
#                 ^
# 1562|       struct ldb_message *msg;
# 1563|       struct ldb_message_element *el;
# 1564|->     const char *obj_attrs[] = {SYSDB_NAME, SYSDB_MEMBEROF,
# NULL};
# 1565|       char *local_groups_base_dn;
# 1566|       uint8_t *obj_base_dn;

Error: COMPILER_WARNING:
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c:1566:14:
warning: unused variable 'obj_base_dn' [-Wunused-variable]
#     uint8_t *obj_base_dn;
#              ^
# 1564|       const char *obj_attrs[] = {SYSDB_NAME, SYSDB_MEMBEROF,
# NULL};
# 1565|       char *local_groups_base_dn;
# 1566|->     uint8_t *obj_base_dn;
# 1567|       char **cached_local_parents = NULL;
# 1568|       uint8_t *name_start;

Error: COMPILER_WARNING:
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c: scope_hint:
In function 'sdap_ad_get_domain_local_groups_parse_parents'
sssd-1.14.90/src/providers/ldap/sdap_async_initgroups_ad.c:1568:14:
warning: unused variable 'name_start' [-Wunused-variable]
#     uint8_t *name_start;
#              ^
# 1566|       uint8_t *obj_base_dn;
# 1567|       char **cached_local_parents = NULL;
# 1568|->     uint8_t *name_start;
# 1569|       char **add_list = NULL;
# 1570|       char **del_list = NULL;

Next, I'm running downstream tests to see if any functionality
regressed..(so I haven't started looking at the code yet..)

[jhrozek]

Thank you, this version reads better to me and still works fine (adding and removing the global group as well..)

So far ACK, I will push the patch once I'm able to re-run the downstream tests with this version.

[jhrozek]

CI: http://sssd-ci.duckdns.org/logs/job/56/05/summary.html

Coverity also didn't find any errors. For some reason, re-running just the tests that failed previously made them pass, but they fail when I run the whole test suite. I will take another look at the logs, but I don't think that a failure in test that tries the ad_access_filter option can be relevant to this patch.

So final ACK

[jhrozek]

• master:
  • cbee11e..2569984

[jhrozek]

sssd-1-4: ec1829d..c1f3b29

[jhrozek]

Umm sorry, the ranges are not so nice, let's list the commits

• master:
  • 2569984
  • 49d3f0a
  • 3dd4c3e
• sssd-1-14:
  • c1f3b29
  • f38c62f
  • 9a243dc