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(a)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?