[SSSD] [PATCHES] SDAP: Refactor sdap_nested_group_hash_group

Sumit Bose sbose at redhat.com
Tue Dec 2 10:39:22 UTC 2014


On Mon, Nov 03, 2014 at 12:20:12PM +0100, Pavel Reichl wrote:
> Hello,
> 
> please see attached patches.
> 
> 1st patch adds unit test for sdap_nested_group_hash_group()
> 2nd patch refactors  sdap_nested_group_hash_group()
> 
> While working on the test I found what I think may be a bug in
> sdap_nested_group_hash_group()
> Part of function definition consists of following:
> int32_t ad_group_type;
> ...
> ret = sysdb_attrs_get_int32_t(group, SYSDB_GROUP_TYPE, &ad_group_type)
> 
> ad_group_type is bitmask for AD group types and may contain
> 2147483648 (0x80000000)
> (http://msdn.microsoft.com/en-us/library/ms675935%28v=vs.85%29.aspx) which
> can't be stored in 32 bit integer.

Hi Pavel,
this is all correct, but AD itself treats the groupType attribute as
(signed) int32_t:

[root at ipa20-devel ~]# ldapsearch -H ldap://win-kps47i803u9.ad18.ipa18.devel -b 'dc=ad18,dc=ipa18,dc=devel' -Y GSSAPI 'cn=domain users'
SASL/GSSAPI authentication started
SASL username: admin at IPA20.DEVEL
SASL SSF: 56
SASL data security layer installed.
# extended LDIF
#
# LDAPv3
# base <dc=ad18,dc=ipa18,dc=devel> with scope subtree
# filter: cn=domain users
# requesting: ALL
#

# Domain Users, Users, ad18.ipa18.devel
dn: CN=Domain Users,CN=Users,DC=ad18,DC=ipa18,DC=devel
objectClass: top
objectClass: group
cn: Domain Users
description: All domain users
distinguishedName: CN=Domain Users,CN=Users,DC=ad18,DC=ipa18,DC=devel
instanceType: 4
whenCreated: 20130131215919.0Z
whenChanged: 20130813161856.0Z
uSNCreated: 12348
memberOf: CN=Users,CN=Builtin,DC=ad18,DC=ipa18,DC=devel
uSNChanged: 55150
name: Domain Users
objectGUID:: WCy1KCRGAUyg3Clr97qTeA==
objectSid:: AQUAAAAAAAUVAAAATRk6uN2ymZwhkGbKAQIAAA==
sAMAccountName: Domain Users
sAMAccountType: 268435456
groupType: -2147483646
objectCategory:
CN=Group,CN=Schema,CN=Configuration,DC=ad18,DC=ipa18,DC=devel
isCriticalSystemObject: TRUE
dSCorePropagationData: 16010101000000.0Z


According to man strtoull "if there was a leading minus sign, the
negation of the result of the conversion represented as an unsigned
value". So it looks like it might work but the negation is done on a
unsigned long long basis:

(gdb) n
56	    ret = strtoull(nptr, endptr, base);
(gdb) p ret
$15 = 0
(gdb) n
58	    if (ret > UINT32_MAX) {
(gdb) p ret
$16 = 18446744071562067968
(gdb) p nptr
$17 = 0x8074390 "-2147483648"
(gdb)

So I guess we have to treat it as signed int32_t. Maybe this is worth a
comment when reading SYSDB_GROUP_TYPE.

> 
> In second patch I changed type of ad_group_type to uint32_t. But without
> this change tests will fail because sysdb_attrs_get_int32_t() will return
> ERANGE.
> 
> Thanks!

> From 94280752bef650e99368e64e13152f418c009152 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Mon, 20 Oct 2014 09:09:21 +0100
> Subject: [PATCH 1/2] TESTS: unit tests for sdap_nested_group_hash_group
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2417
> ---
>  Makefile.am                                   |   1 +
>  src/providers/ldap/sdap_async_nested_groups.c |  15 +-
>  src/providers/ldap/sdap_async_private.h       |  16 ++
>  src/tests/cmocka/test_nested_groups.c         |  28 ++-
>  src/tests/cmocka/test_nested_groups_funcs.c   | 310 ++++++++++++++++++++++++++
>  src/tests/cmocka/test_nested_groups_private.h |  44 ++++
>  6 files changed, 388 insertions(+), 26 deletions(-)
>  create mode 100644 src/tests/cmocka/test_nested_groups_funcs.c
>  create mode 100644 src/tests/cmocka/test_nested_groups_private.h
> 
> diff --git a/Makefile.am b/Makefile.am
> index 6a8124b5ad30f9e54a0325dc574a96c91ec4e805..8b0c2285dd1e4a03afecd58d0a1aff6acbe02dbc 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1818,6 +1818,7 @@ nestedgroups_tests_SOURCES = \
>      $(TEST_MOCK_PROVIDER_OBJ) \
>      src/providers/ldap/sdap_idmap.c \
>      src/tests/cmocka/test_nested_groups.c \
> +    src/tests/cmocka/test_nested_groups_funcs.c \
>      src/providers/ldap/sdap_async_nested_groups.c \
>      $(NULL)
>  nestedgroups_tests_CFLAGS = \
> diff --git a/src/providers/ldap/sdap_async_nested_groups.c b/src/providers/ldap/sdap_async_nested_groups.c
> index 1eba35ae8ac90acac8a2d46e8cc5f2b57e3a9256..a482239f4d601ce9e0054242bb0da250569b52de 100644
> --- a/src/providers/ldap/sdap_async_nested_groups.c
> +++ b/src/providers/ldap/sdap_async_nested_groups.c
> @@ -55,19 +55,6 @@ struct sdap_nested_group_member {
>      const char *group_filter;
>  };
>  
> -struct sdap_nested_group_ctx {
> -    struct sss_domain_info *domain;
> -    struct sdap_options *opts;
> -    struct sdap_search_base **user_search_bases;
> -    struct sdap_search_base **group_search_bases;
> -    struct sdap_handle *sh;
> -    hash_table_t *users;
> -    hash_table_t *groups;
> -    bool try_deref;
> -    int deref_treshold;
> -    int max_nesting_level;
> -};

What about adding a public init call for sdap_nested_group_ctx() instead
of moving the definition to sdap_async_private.h? This would make the
test initialisation more easy and you will automatically test the
initialisation of sdap_nested_group_ctx in the main code as well.

> -
>  static struct tevent_req *
>  sdap_nested_group_process_send(TALLOC_CTX *mem_ctx,
>                               struct tevent_context *ev,
> @@ -234,7 +221,7 @@ sdap_nested_group_hash_user(struct sdap_nested_group_ctx *group_ctx,
>      return sdap_nested_group_hash_entry(group_ctx->users, user, "users");
>  }
>  
> -static errno_t
> +errno_t
>  sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx,
>                               struct sysdb_attrs *group)
>  {
> diff --git a/src/providers/ldap/sdap_async_private.h b/src/providers/ldap/sdap_async_private.h
> index e689394c5db8a3385c333e6b98372c6f6d34366c..f449717e9f3f99a227e4e080cdd1c301742e79b1 100644
> --- a/src/providers/ldap/sdap_async_private.h
> +++ b/src/providers/ldap/sdap_async_private.h
> @@ -35,6 +35,19 @@ struct dn_item {
>      struct dn_item *prev;
>  };
>  
> +struct sdap_nested_group_ctx {
> +    struct sss_domain_info *domain;
> +    struct sdap_options *opts;
> +    struct sdap_search_base **user_search_bases;
> +    struct sdap_search_base **group_search_bases;
> +    struct sdap_handle *sh;
> +    hash_table_t *users;
> +    hash_table_t *groups;
> +    bool try_deref;
> +    int deref_treshold;
> +    int max_nesting_level;
> +};
> +
>  bool is_dn(const char *str);
>  errno_t update_dn_list(struct dn_item *dn_list,
>                         const size_t count,
> @@ -132,4 +145,7 @@ errno_t sdap_nested_group_recv(TALLOC_CTX *mem_ctx,
>                                 unsigned long *_num_groups,
>                                 struct sysdb_attrs ***_groups);
>  
> +errno_t sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx,
> +                                     struct sysdb_attrs *group);
> +
>  #endif /* _SDAP_ASYNC_PRIVATE_H_ */
> diff --git a/src/tests/cmocka/test_nested_groups.c b/src/tests/cmocka/test_nested_groups.c
> index b64d67d3cc802d9bb1e585a63da04b68ecc460c4..2843166d52e0300f46786d0b1801c8527ebc267f 100644
> --- a/src/tests/cmocka/test_nested_groups.c
> +++ b/src/tests/cmocka/test_nested_groups.c
> @@ -26,28 +26,17 @@
>  #include "tests/cmocka/common_mock.h"
>  #include "tests/cmocka/common_mock_sdap.h"
>  #include "tests/cmocka/common_mock_sysdb_objects.h"
> +#include "tests/cmocka/test_nested_groups_private.h"
>  #include "providers/ldap/ldap_common.h"
>  #include "providers/ldap/sdap.h"
>  #include "providers/ldap/sdap_idmap.h"
>  #include "providers/ldap/sdap_async_private.h"
>  
> -#define TESTS_PATH "tests_ldap_nested_groups"
> -#define TEST_CONF_DB "test_ldap_nested_groups_conf.ldb"
> -#define TEST_DOM_NAME "ldap_nested_groups_test"
> -#define TEST_SYSDB_FILE "cache_"TEST_DOM_NAME".ldb"
> -#define TEST_ID_PROVIDER "ldap"
> -
>  #define new_test(test) \
>      unit_test_setup_teardown(nested_groups_test_ ## test, \
>                               nested_groups_test_setup, \
>                               nested_groups_test_teardown)
>  
> -/* put users and groups under the same container so we can easily run the
> - * same tests cases for several search base scenarios */
> -#define OBJECT_BASE_DN "cn=objects,dc=test,dc=com"
> -#define GROUP_BASE_DN "cn=groups," OBJECT_BASE_DN
> -#define USER_BASE_DN "cn=users," OBJECT_BASE_DN
> -
>  #define N_ELEMENTS(arr) \
>      (sizeof(arr) / sizeof(arr[0]))
>  
> @@ -466,6 +455,21 @@ int main(int argc, const char *argv[])
>          new_test(one_group_dup_users),
>          new_test(one_group_unique_group_members),
>          new_test(one_group_dup_group_members),
> +        unit_test_setup_teardown(sdap_nested_group_hash_group_test_non_posix,
> +                                 sdap_nested_group_hash_group_setup_rfc2307bis,
> +                                 nested_groups_test_teardown),
> +        unit_test_setup_teardown(sdap_nested_group_hash_group_test_posix,
> +                                 sdap_nested_group_hash_group_setup_rfc2307bis,
> +                                 nested_groups_test_teardown),
> +        unit_test_setup_teardown(sdap_nested_group_hash_group_test_non_posix_AD_enoent,
> +                                 sdap_nested_group_hash_group_setup_AD,
> +                                 nested_groups_test_teardown),
> +        unit_test_setup_teardown(sdap_nested_group_hash_group_test_non_posix_AD_buildin,
> +                                 sdap_nested_group_hash_group_setup_AD,
> +                                 nested_groups_test_teardown),
> +        unit_test_setup_teardown(sdap_nested_group_hash_group_test_non_posix_AD_security,
> +                                 sdap_nested_group_hash_group_setup_AD,
> +                                 nested_groups_test_teardown),

If I see it correctly the only thing you reuse from test_nested_groups.c
is nested_groups_test_teardown, which is just talloc_zfree(). Since you
already created new files for the tests I think you can just create a
new test binary for the tests.

>      };
>  
>      /* Set debug level to invalid value so we can deside if -d 0 was used. */
> diff --git a/src/tests/cmocka/test_nested_groups_funcs.c b/src/tests/cmocka/test_nested_groups_funcs.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..e1c2bda24a9f03aa4c1bc0727a6af3e8a0615e56
> --- /dev/null
> +++ b/src/tests/cmocka/test_nested_groups_funcs.c
> @@ -0,0 +1,310 @@
> +/*
> +    Authors:
> +        Pavel Reichl <preichl at redhat.com>
> +
> +    Copyright (C) 2014 Red Hat
> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 3 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> +    You should have received a copy of the GNU General Public License
> +    along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#include <talloc.h>
> +#include <tevent.h>
> +#include <errno.h>
> +#include <popt.h>
> +

...

> +/* AD, gid = 0 */
> +void sdap_nested_group_hash_group_test_non_posix_AD_security(void **state)
> +{
> +    errno_t ret;
> +    struct test_ctx *tc;
> +    uint32_t gid;
> +    bool is_posix;
> +
> +    tc = talloc_get_type_abort(*state, struct test_ctx);
> +
> +    /* attrs */
> +    ret = sysdb_attrs_add_string(tc->group, SYSDB_ORIG_DN,
> +                                 "cn=testgroup" GROUP_BASE_DN);
> +    assert_int_equal(ret, EOK);
> +
> +    ret = sysdb_attrs_add_long(tc->group, SYSDB_GROUP_TYPE,
> +                                 SDAP_AD_GROUP_TYPE_SECURITY);

this test fails for me on 32bit because "-2147483648" is stored in
attrs and later on strtoull() fails  to convert it to the expected
unsigned value (see above). This might be different on 64bits where long
is 8byte long and can hold the unsigned value of
SDAP_AD_GROUP_TYPE_SECURITY.


> +    assert_int_equal(ret, EOK);
> +
> +    ret = sdap_nested_group_hash_group(tc->group_ctx, tc->group);
> +    assert_int_equal(ret, EOK);
> +
> +    ret = sysdb_attrs_get_uint32_t(tc->group,
> +                                   tc->group_ctx->opts->group_map[SDAP_AT_GROUP_GID].sys_name,
> +                                   &gid);
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(gid, 0);
> +
> +    ret = sysdb_attrs_get_bool(tc->group,
> +                               SYSDB_POSIX,
> +                               &is_posix);
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(is_posix, 0);
> +}

...

> From 2878bb0ad9fad836f85f31e3142f2319e483ba9e Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Mon, 3 Nov 2014 10:40:05 +0000
> Subject: [PATCH 2/2] SDAP: Refactor sdap_nested_group_hash_group
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2417
> ---
>  src/providers/ldap/sdap_async_nested_groups.c | 152 +++++++++++++++++---------
>  1 file changed, 100 insertions(+), 52 deletions(-)
> 
> diff --git a/src/providers/ldap/sdap_async_nested_groups.c b/src/providers/ldap/sdap_async_nested_groups.c
> index a482239f4d601ce9e0054242bb0da250569b52de..368c339d03f47c3bfcfea719e7f4e54ff6dd2b16 100644
> --- a/src/providers/ldap/sdap_async_nested_groups.c
> +++ b/src/providers/ldap/sdap_async_nested_groups.c
> @@ -221,74 +221,122 @@ sdap_nested_group_hash_user(struct sdap_nested_group_ctx *group_ctx,
>      return sdap_nested_group_hash_entry(group_ctx->users, user, "users");
>  }
>  
> +static errno_t
> +is_ad_group_posix(struct sysdb_attrs *group,
> +                 struct sss_domain_info *domain,
> +                 bool *posix_group)
> +{
> +    uint32_t ad_group_type;
> +    errno_t ret;
> +
> +    ret = sysdb_attrs_get_uint32_t(group, SYSDB_GROUP_TYPE, &ad_group_type);

see above

> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_get_int32_t failed.\n");
> +        return ret;
> +    }
> +
> +    DEBUG(SSSDBG_TRACE_ALL, "AD group has type flags %#x.\n", ad_group_type);
> +
> +    /* Only security groups from AD are considered for POSIX groups.
> +     * Additionally only global and universal group are taken to account
> +     * for trusted domains. */
> +    bool is_security = ad_group_type & SDAP_AD_GROUP_TYPE_SECURITY;
> +    bool is_global = ad_group_type & SDAP_AD_GROUP_TYPE_GLOBAL;
> +    bool is_universal = ad_group_type & SDAP_AD_GROUP_TYPE_UNIVERSAL;

please declare the variables at the beginning of the function.

> +
> +    *posix_group = is_security &&
> +        (!IS_SUBDOMAIN(domain) || (is_global || is_universal));
> +
> +    ret = EOK;
> +    return ret;
> +}
> +
> +static errno_t set_dfl_non_posix_group(struct sysdb_attrs *group,
> +                                       const char *gid_label)
> +{
> +    errno_t ret;
> +
> +    DEBUG(SSSDBG_TRACE_INTERNAL,
> +          "Marking group as non-posix and setting GID=0!\n");
> +
> +    ret = sysdb_attrs_add_uint32(group, gid_label, 0);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE,
> +              "Failed to add a GID to non-posix group!\n");
> +        goto done;
> +    }
> +
> +    ret = sysdb_attrs_add_bool(group, SYSDB_POSIX, false);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "Error: Failed to mark group as non-posix!\n");
> +        goto done;
> +    }
> +
> +done:
> +    return ret;
> +}
> +
> +static errno_t handle_groups_with_gid(struct sysdb_attrs *group,
> +                                      const char *gid_label)
> +{
> +    errno_t ret;
> +    gid_t gid;
> +
> +    ret = sysdb_attrs_get_uint32_t(group, gid_label, &gid);
> +    if ((ret == EOK && gid == 0) || ret == ENOENT) {
> +        DEBUG(SSSDBG_TRACE_ALL,
> +              "The group's gid was %s\n",
> +              ret == ENOENT ? "missing" : "zero");
> +
> +        ret = set_dfl_non_posix_group(group, gid_label);
> +        if (ret != EOK) {
> +            goto done;
> +        }
> +    } else if (ret != EOK) {
> +        goto done;
> +    }
> +    ret = EOK;
> +
> +done:
> +    return ret;
> +}
> +
>  errno_t
>  sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx,
>                               struct sysdb_attrs *group)
>  {
> -    struct sdap_attr_map *map = group_ctx->opts->group_map;
> -    gid_t gid;
>      errno_t ret = ENOENT;
> -    int32_t ad_group_type;
>      bool posix_group = true;
>      bool use_id_mapping;
> -    bool can_find_gid;
> -
> -    if (group_ctx->opts->schema_type == SDAP_SCHEMA_AD) {
> -        ret = sysdb_attrs_get_int32_t(group, SYSDB_GROUP_TYPE, &ad_group_type);
> -        if (ret != EOK) {
> -            DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_get_int32_t failed.\n");
> -            return ret;
> -        }
> -
> -        DEBUG(SSSDBG_TRACE_ALL, "AD group has type flags %#x.\n",
> -                                 ad_group_type);
> -        /* Only security groups from AD are considered for POSIX groups.
> -         * Additionally only global and universal group are taken to account
> -         * for trusted domains. */
> -        if (!(ad_group_type & SDAP_AD_GROUP_TYPE_SECURITY)
> -                || (IS_SUBDOMAIN(group_ctx->domain)
> -                    && (!((ad_group_type & SDAP_AD_GROUP_TYPE_GLOBAL)
> -                        || (ad_group_type & SDAP_AD_GROUP_TYPE_UNIVERSAL))))) {
> -            posix_group = false;
> -            gid = 0;
> -            DEBUG(SSSDBG_TRACE_FUNC, "Filtering AD group.\n");
> -        }
> -    }
> +    const char *gid_label;
>  
> +    gid_label = group_ctx->opts->group_map[SDAP_AT_GROUP_GID].sys_name;
>      use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(
>                                                            group_ctx->opts->idmap_ctx,
>                                                            group_ctx->domain->name,
>                                                            group_ctx->domain->domain_id);
>  
> -    can_find_gid = posix_group && !use_id_mapping;
> -    if (can_find_gid) {
> -        ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name,
> -                                       &gid);
> -    }
> -    if (!can_find_gid || ret == ENOENT || (ret == EOK && gid == 0)) {
> -        DEBUG(SSSDBG_TRACE_ALL,
> -             "The group's gid was %s\n", ret == ENOENT ? "missing" : "zero");
> -        DEBUG(SSSDBG_TRACE_INTERNAL,
> -             "Marking group as non-posix and setting GID=0!\n");
> -
> -        if (ret == ENOENT || !posix_group) {
> -            ret = sysdb_attrs_add_uint32(group,
> -                                         map[SDAP_AT_GROUP_GID].sys_name, 0);
> -            if (ret != EOK) {
> -                DEBUG(SSSDBG_CRIT_FAILURE,
> -                      "Failed to add a GID to non-posix group!\n");
> -                return ret;
> -            }
> -        }
> -
> -        ret = sysdb_attrs_add_bool(group, SYSDB_POSIX, false);
> +    /* handle specific group types for AD case */
> +    if (group_ctx->opts->schema_type == SDAP_SCHEMA_AD) {
> +        ret = is_ad_group_posix(group, group_ctx->domain, &posix_group);
>          if (ret != EOK) {
> -            DEBUG(SSSDBG_OP_FAILURE,
> -                  "Error: Failed to mark group as non-posix!\n");
> +            DEBUG(SSSDBG_OP_FAILURE, "is_ad_user_posix failed.\n");
>              return ret;
>          }
> -    } else if (ret != EOK) {
> -        return ret;
> +
> +        if (!posix_group) {
> +            DEBUG(SSSDBG_TRACE_FUNC, "Filtering AD group.\n");
> +        }
> +    }
> +
> +    if (posix_group && use_id_mapping == false) {

You should stick to either use plain bool and !bool or ==true and
==false but not mix them

> +        /* gid is supposed to be in attributes */
> +        ret = handle_groups_with_gid(group, gid_label);
> +       if (ret != EOK) return ret;
> +    } else {
> +        ret = set_dfl_non_posix_group(group, gid_label);
> +        if (ret != EOK) return ret;
>      }
>  
>      return sdap_nested_group_hash_entry(group_ctx->groups, group, "groups");
> -- 
> 1.9.3
> 

bye,
Sumit



More information about the sssd-devel mailing list