[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