[SSSD] [PATCH] sdap: properly handle binary objectGuid attribute

Jakub Hrozek jhrozek at redhat.com
Fri Mar 20 10:48:28 UTC 2015


On Tue, Mar 17, 2015 at 04:46:22PM +0100, Sumit Bose wrote:
> On Tue, Mar 10, 2015 at 12:31:59PM +0100, Sumit Bose wrote:
> > On Mon, Mar 09, 2015 at 10:06:26AM +0100, Sumit Bose wrote:
> > > On Tue, Feb 17, 2015 at 05:05:37PM +0100, Sumit Bose wrote:
> > > > Hi,
> > > > 
> > > > this patch tries to resolve https://fedorahosted.org/sssd/ticket/2588 by
> > > > properly handling binary objectGUID values. In general there shouldn't
> > > > be an issue because this ID is not used for AD user. But if the binary
> > > > value starts with 0x00 there might be issues saving the user object to
> > > > the cache.
> > > > 
> > > > bye,
> > > > Sumit
> > > 
> > > Please find attached a new version which covers groups as well.
> > > 
> > 
> > I found another place which needed fixing sdap_add_incomplete_groups().
> > 
> > New version attached.
> > 
> 
> Another new version which fixes a typo.
> 
> bye,
> Sumit

> From e6a8fe45447ba7410b09c76ceb7d928045aa7c02 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Tue, 17 Feb 2015 04:41:21 +0100
> Subject: [PATCH] sdap: properly handle binary objectGuid attribute
> 
> Although in the initial processing SSSD treats the binary value right at
> some point it mainly assumes that it is a string. Depending on the value
> this might end up with the correct binary value stored in the cache but
> in most cases there will be only a broken entry in the cache.
> 
> This patch converts the binary value into a string representation which
> is described in [MS-DTYP] and stores the result in the cache.
> 
> Resolves https://fedorahosted.org/sssd/ticket/2588
> ---
>  Makefile.am                                |  16 ++++
>  src/db/sysdb.h                             |   6 ++
>  src/db/sysdb_ops.c                         |  52 +++++++++++
>  src/providers/ldap/sdap_async_groups.c     |  25 ++----
>  src/providers/ldap/sdap_async_initgroups.c |   7 +-
>  src/providers/ldap/sdap_async_users.c      |  23 ++---
>  src/tests/cmocka/test_string_utils.c       |  59 +++++++++++++
>  src/tests/cmocka/test_sysdb_utils.c        | 134 +++++++++++++++++++++++++++++
>  src/tests/cmocka/test_utils.c              |   1 +
>  src/tests/cmocka/test_utils.h              |   1 +
>  src/tests/cwrap/Makefile.am                |   2 +
>  src/util/string_utils.c                    |  25 ++++++
>  src/util/util.h                            |   6 ++
>  13 files changed, 323 insertions(+), 34 deletions(-)
>  create mode 100644 src/tests/cmocka/test_sysdb_utils.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index ba24343c3a2be9edc94ec817d3709a0c78599847..0f7725b06e0330bf17c38eb7b1829a8093fb1d70 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -214,6 +214,7 @@ if HAVE_CMOCKA
>          test_search_bases \
>          sdap-tests \
>          test_sysdb_views \
> +        test_sysdb_utils \
>          test_be_ptask \
>          test_copy_ccache \
>          test_copy_keytab \
> @@ -2129,6 +2130,21 @@ test_sysdb_views_LDADD = \
>      libsss_test_common.la \
>      $(NULL)
>  
> +test_sysdb_utils_SOURCES = \
> +    src/tests/cmocka/test_sysdb_utils.c \
> +    $(NULL)
> +test_sysdb_utils_CFLAGS = \
> +    $(AM_CFLAGS) \
> +    $(NULL)
> +test_sysdb_utils_LDADD = \
> +    $(CMOCKA_LIBS) \
> +    $(LDB_LIBS) \
> +    $(POPT_LIBS) \
> +    $(TALLOC_LIBS) \
> +    $(SSSD_INTERNAL_LTLIBS) \
> +    libsss_test_common.la \
> +    $(NULL)
> +
>  test_be_ptask_SOURCES = \
>      src/tests/cmocka/test_be_ptask.c \
>      src/providers/dp_ptask.c \
> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> index 84c84a49f1080fc7c3e8b5a129677598ee6760d8..2a3a2df98fa1e51cf024b7bd57347e8b02449eff 100644
> --- a/src/db/sysdb.h
> +++ b/src/db/sysdb.h
> @@ -1113,4 +1113,10 @@ errno_t sysdb_get_sids_of_members(TALLOC_CTX *mem_ctx,
>                                    const char ***_sids,
>                                    const char ***_dns,
>                                    size_t *_n);
> +
> +errno_t sysdb_handle_original_uuid(const char *orig_name,
> +                                   struct sysdb_attrs *src_attrs,
> +                                   const char *src_name,
> +                                   struct sysdb_attrs *dest_attrs,
> +                                   const char *dest_name);
>  #endif /* __SYS_DB_H__ */
> diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
> index 4babc76cf50facaa1291de13a60f9312f16863ef..e73d025a2a20992dc47d6b212e2169d825b614e4 100644
> --- a/src/db/sysdb_ops.c
> +++ b/src/db/sysdb_ops.c
> @@ -3702,3 +3702,55 @@ done:
>      talloc_free(tmp_ctx);
>      return ret;
>  }
> +
> +errno_t sysdb_handle_original_uuid(const char *orig_name,
> +                                   struct sysdb_attrs *src_attrs,
> +                                   const char *src_name,
> +                                   struct sysdb_attrs *dest_attrs,
> +                                   const char *dest_name)
> +{
> +    int ret;
> +    struct ldb_message_element *el;
> +    char guid_str_buf[GUID_STR_BUF_SIZE];
> +
> +    if (orig_name == NULL || src_attrs == NULL || src_name == NULL
> +            || dest_attrs == NULL || dest_name == NULL) {
> +        return EINVAL;
> +    }
> +
> +    ret = sysdb_attrs_get_el_ext(src_attrs, src_name, false, &el);
> +    if (ret != EOK) {
> +        if (ret != ENOENT) {
> +            DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_get_el failed.\n");
> +        }
> +        return ret;
> +    }
> +
> +    if (el->num_values != 1) {
> +        DEBUG(SSSDBG_MINOR_FAILURE,
> +              "Found more than one UUID value, using the first.\n");
> +    }
> +
> +    /* Check if we got a binary AD objectGUID */
> +    if (el->values[0].length == 16

My only nitpick would be that we should either add a comment or define a
const int in the function with a descriptive name to avoid "magical"
constants.

Otherwise ACK. Thank you for the unit tests.

Given that our QE engineers reproduced the problem as well and there was
a user who reported a similar issue on sssd-users, I would like to push
the patch to sssd-1-12 and also the downstreams as well if either of them
reports this patch fixes the problem. Do you agree?



More information about the sssd-devel mailing list