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
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.
bye, Sumit
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.
bye, Sumit
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
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@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?
On Fri, Mar 20, 2015 at 11:48:28AM +0100, Jakub Hrozek wrote:
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:
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.
new version with GUID_BIN_LENGTH attached.
bye, Sumit
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? _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Fri, Mar 20, 2015 at 12:02:55PM +0100, Sumit Bose wrote:
On Fri, Mar 20, 2015 at 11:48:28AM +0100, Jakub Hrozek wrote:
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:
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.
new version with GUID_BIN_LENGTH attached.
bye, Sumit
ACK
On Fri, Mar 20, 2015 at 12:26:15PM +0100, Jakub Hrozek wrote:
On Fri, Mar 20, 2015 at 12:02:55PM +0100, Sumit Bose wrote:
On Fri, Mar 20, 2015 at 11:48:28AM +0100, Jakub Hrozek wrote:
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:
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.
new version with GUID_BIN_LENGTH attached.
bye, Sumit
ACK
* master: 1d93029624d708119bbf803e6647a2cbb271f001
In order to give out test builds I backported the patch to 1-12: https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=review-1...
The only conflict was in src/tests/cmocka/test_utils.c
On Fri, Mar 20, 2015 at 01:32:05PM +0100, Jakub Hrozek wrote:
On Fri, Mar 20, 2015 at 12:26:15PM +0100, Jakub Hrozek wrote:
On Fri, Mar 20, 2015 at 12:02:55PM +0100, Sumit Bose wrote:
On Fri, Mar 20, 2015 at 11:48:28AM +0100, Jakub Hrozek wrote:
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:
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.
new version with GUID_BIN_LENGTH attached.
bye, Sumit
ACK
- master: 1d93029624d708119bbf803e6647a2cbb271f001
In order to give out test builds I backported the patch to 1-12: https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=review-1...
The only conflict was in src/tests/cmocka/test_utils.c
Sorry to spam the list but I realized I forgot to send the CI link prior to pushing. Here it is: http://sssd-ci.duckdns.org/logs/job/11/28/summary.html
On Fri, Mar 20, 2015 at 03:32:24PM +0100, Jakub Hrozek wrote:
On Fri, Mar 20, 2015 at 01:32:05PM +0100, Jakub Hrozek wrote:
On Fri, Mar 20, 2015 at 12:26:15PM +0100, Jakub Hrozek wrote:
On Fri, Mar 20, 2015 at 12:02:55PM +0100, Sumit Bose wrote:
On Fri, Mar 20, 2015 at 11:48:28AM +0100, Jakub Hrozek wrote:
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:
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.
new version with GUID_BIN_LENGTH attached.
bye, Sumit
ACK
- master: 1d93029624d708119bbf803e6647a2cbb271f001
In order to give out test builds I backported the patch to 1-12: https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=review-1...
The only conflict was in src/tests/cmocka/test_utils.c
Sorry to spam the list but I realized I forgot to send the CI link prior to pushing. Here it is: http://sssd-ci.duckdns.org/logs/job/11/28/summary.html
I also pushed the patch to sssd-1-12: 4619742836ec22edf8f9d274d928bc896c5b0883
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Sumit Bose