On (15/12/15 11:21), Pavel Březina wrote: >On 12/14/2015 04:09 PM, Lukas Slebodnik wrote: >>On (14/12/15 13:34), Lukas Slebodnik wrote: >>>On (14/12/15 12:38), Pavel Březina wrote: >>>>On 12/14/2015 12:05 PM, Lukas Slebodnik wrote: >>>>>On (11/12/15 15:56), Pavel Březina wrote: >>>>>>On 12/11/2015 03:07 PM, Jakub Hrozek wrote: >>>>>>>> From 2479352011f96a8f64c403d1278b791c73df703a Mon Sep 17 00:00:00 2001 >>>>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> >>>>>>>>Date: Fri, 6 Nov 2015 13:00:47 +0100 >>>>>>>>Subject: [PATCH 01/35] SUDO: convert periodical refreshes to be_ptask >>>>>>> >>>>>>>LGTM, not tested yet. One question inline... >>>>>>> >>>>>>>> >>>>>>>>This removes old sudo timer and simplyfies code a lot. It also >>>>>>>>allows to manage offline/online state. >>>>>>>> >>>>>>>>- Full and smart refresh are disabled when offline. >>>>>>>>- Full refresh is run immediately when sssd is back online. >>>>>>>>- Smart refresh is scheduled normally when sssd is back online. >>>>>>>> >>>>>>>>Resolves: >>>>>>>>https://fedorahosted.org/sssd/ticket/1943 >>>>>>> >>>>>>>I agree with the logic, but it's different from the original code, so is >>>>>>>the change intended? >>>>>> >>>>>>Yes, the functionality remains the same but we handle online/offline >>>>>>transition much better here. >>>>>> >>>>>>>> From d17b4d1c63f1b85a14a6fe8b2400e2c29158cd43 Mon Sep 17 00:00:00 2001 >>>>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> >>>>>>>>Date: Fri, 6 Nov 2015 13:32:34 +0100 >>>>>>>>Subject: [PATCH 02/35] SUDO: move refreshes from sdap_sudo.c to >>>>>>>> sdap_sudo_refresh.c >>>>>>> >>>>>>>LGTM, I wonder why git doesn't show the changes as move since such a >>>>>>>substantial part of sdap_sudo.c changed? >>>>>> >>>>>>Dunno. >>>>>> >>>>>>>> From 633d0de6e3edab4d5b988506426800cf35845702 Mon Sep 17 00:00:00 2001 >>>>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> >>>>>>>>Date: Mon, 9 Nov 2015 12:26:07 +0100 >>>>>>>>Subject: [PATCH 03/35] SUDO: move offline check to handler >>>>>>>> >>>>>>>>We let sdap_id_op decide if we are offline or not here but we >>>>>>>>should not get to this code since ptask is disabled and we will >>>>>>>>not get through sudo handler if offline. >>>>>>> >>>>>>>LGTM, the connection error would still take us offline, which is what we >>>>>>>want. >>>>>>> >>>>>>>> From 3dc1bbb6fa4353f0bb2cc2b9417900a6d4695240 Mon Sep 17 00:00:00 2001 >>>>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> >>>>>>>>Date: Mon, 9 Nov 2015 12:44:13 +0100 >>>>>>>>Subject: [PATCH 04/35] SUDO: simplify error handling >>>>>>> >>>>>>>LGTM >>>>>>> >>>>>>>> From 2e85798a9d1f856b549cb733b6a7f353bec05e6e Mon Sep 17 00:00:00 2001 >>>>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> >>>>>>>>Date: Mon, 9 Nov 2015 12:52:35 +0100 >>>>>>>>Subject: [PATCH 05/35] SUDO: fix sdap_id_op logic >>>>>>> >>>>>>>LGTM, the fail/immediate confusion in sdap_sudo_load_sudoers_send is >>>>>>>fixed in another patch. If you end up changing this patchset, it might >>>>>>>also be better to fix the "i" iterators to use size_t instead of int, >>>>>>>but if you don't change them, it's fine. >>>>>> >>>>>>Ok, new patch is attached to the patch set. Thanks. >>>>>> >>>>>>> >>>>>>>> From 03bcc4d6b5c1324644bab7b98a10bfacaf9096c7 Mon Sep 17 00:00:00 2001 >>>>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> >>>>>>>>Date: Mon, 9 Nov 2015 13:34:45 +0100 >>>>>>>>Subject: [PATCH 07/35] SUDO: fix sdap_sudo_smart_refresh_recv() >>>>>>>> >>>>>>>>This fix huge violation of tevent coding style. >>>>>>> >>>>>>>LGTM >>>>>>> >>>>>>>> From 0093a630eac9a745512c643b1389506c3839a56e Mon Sep 17 00:00:00 2001 >>>>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> >>>>>>>>Date: Mon, 9 Nov 2015 13:59:48 +0100 >>>>>>>>Subject: [PATCH 08/35] SUDO: sdap_sudo_load_sudoers improve iterator >>>>>>>> >>>>>>>>The old search base iterator was difficult to read since its logic >>>>>>>>spread through all functions. This patch also shorten names. >>>>>>> >>>>>>>LGTM >>>>>>> >>>>>>>> From a7d3e06ac32b74e2586d87f9671b5a49eda7cafe Mon Sep 17 00:00:00 2001 >>>>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> >>>>>>>>Date: Tue, 10 Nov 2015 10:34:41 +0100 >>>>>>>>Subject: [PATCH 09/35] SUDO: set USN inside sdap_sudo_refresh request >>>>>>>> >>>>>>>>Reduce code duplication. >>>>>>>>--- >>>>>>>> src/providers/ldap/sdap_async_sudo.c | 48 >>>>>>>> +++++++++++++++++++++------ >>>>>>>> src/providers/ldap/sdap_sudo.h | 2 +- >>>>>>>> src/providers/ldap/sdap_sudo_refresh.c | 59 >>>>>>>> ++++++---------------------------- >>>>>>>> 3 files changed, 49 insertions(+), 60 deletions(-) >>>>>>>> >>>>>>>>diff --git a/src/providers/ldap/sdap_async_sudo.c >>>>>>>>b/src/providers/ldap/sdap_async_sudo.c >>>>>>>>index >>>>>>>>71e1b3322a557beaf82b51316967b3147fb61f76..7a90ccde9cbd2efc0d0509f21e072cd745c57844 >>>>>>>> 100644 >>>>>>>>--- a/src/providers/ldap/sdap_async_sudo.c >>>>>>>>+++ b/src/providers/ldap/sdap_async_sudo.c >>>>>>>>@@ -283,6 +283,7 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX >>>>>>>>*mem_ctx, >>>>>>>> >>>>>>>> /* Empty sudoers? Done. */ >>>>>>>> if (rules_count == 0 || rules == NULL) { >>>>>>>>+ *_usn = NULL; >>>>>>>> return EOK; >>>>>>>> } >>>>>>>> >>>>>>>>@@ -299,8 +300,37 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX >>>>>>>>*mem_ctx, >>>>>>>> return EOK; >>>>>>>> } >>>>>>>> >>>>>>>>+static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char >>>>>>>>*usn) >>>>>>>>+{ >>>>>>>>+ unsigned int usn_number; >>>>>>>>+ char *endptr = NULL; >>>>>>>>+ >>>>>>>>+ if (usn == NULL) { >>>>>>>>+ DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n"); >>>>>>>>+ return; >>>>>>>>+ } >>>>>>>>+ >>>>>>>>+ if (srv_opts == NULL) { >>>>>>>>+ DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n"); >>>>>>>>+ return; >>>>>>>>+ } >>>>>>>>+ >>>>>>>>+ talloc_zfree(srv_opts->max_sudo_value); >>>>>>>>+ srv_opts->max_sudo_value = talloc_steal(srv_opts, usn); >>>>>>>>+ >>>>>>>>+ usn_number = strtoul(usn, &endptr, 10); >>>>>>>>+ if ((endptr == NULL || (*endptr == '\0' && endptr != usn)) >>>>>>>>+ && (usn_number > srv_opts->last_usn)) { >>>>>>>>+ srv_opts->last_usn = usn_number; >>>>>>>>+ } >>>>>>>>+ >>>>>>>>+ DEBUG(SSSDBG_FUNC_DATA, "SUDO higher USN value: [%s]\n", >>>>>>>>+ srv_opts->max_sudo_value); >>>>>>>>+} >>>>>>>>+ >>>>>>> >>>>>>>What is the logic behind this function? Why do we store the USN both as >>>>>>>a string unconditionally and as a number conditionally? Wouldn't it make >>>>>>>sense to just store the data in one format? >>>>>> >>>>>>max_sudo_value is internal to sudo, it is max value found in sudo subtree. >>>>>>It's string since it is not needed to be a number. last_usn is the largers >>>>>>value on the whole server, it needs to be compared. >>>>>> >>>>>>max_sudo_value can be a number also, but it would be inconsistent with >>>>>>other >>>>>>providers. >>>>>> >>>>>>> >>>>>>>> From 1ca17bd4110960f3d9b6834446cec83dfe9a1ca8 Mon Sep 17 00:00:00 2001 >>>>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> >>>>>>>>Date: Tue, 10 Nov 2015 11:31:50 +0100 >>>>>>>>Subject: [PATCH 10/35] SUDO: built host filter inside sdap_sudo_refresh >>>>>>>> request >>>>>>>> >>>>>>>>Preparation for: >>>>>>>>https://fedorahosted.org/sssd/ticket/2672 >>>>>>> >>>>>>>LGTM >>>>>>> >>>>>>>> From ebe26c922ad72c97f53d5ca0f5eeef1034c4a470 Mon Sep 17 00:00:00 2001 >>>>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> >>>>>>>>Date: Tue, 10 Nov 2015 11:34:14 +0100 >>>>>>>>Subject: [PATCH 11/35] SUDO: fail if usn is unknown in smart refresh >>>>>>>> >>>>>>>>USN should never be unknown when we run smart refresh so this is >>>>>>>>more a safety condition for a code bug. Nevertheless, we should >>>>>>>>not immitate full refresh here under those conditions. >>>>>>> >>>>>>>LGTM >>>>>>> >>>>>>>> From aa06528d2a1fdd6b5df5de629d2da5297cbcc90d Mon Sep 17 00:00:00 2001 >>>>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> >>>>>>>>Date: Tue, 10 Nov 2015 12:34:11 +0100 >>>>>>>>Subject: [PATCH 12/35] SUDO: fix potential memory leak in sdap_sudo_init >>>>>>> >>>>>>>LGTM >>>>>>> >>>>>>>> From 897f68a3379f8fab70eae106f5fa71b4fe4d809a Mon Sep 17 00:00:00 2001 >>>>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> >>>>>>>>Date: Tue, 10 Nov 2015 12:32:16 +0100 >>>>>>>>Subject: [PATCH 13/35] SUDO: obtain host information when going online >>>>>>>> >>>>>>>>Resolves: >>>>>>>>https://fedorahosted.org/sssd/ticket/2672 >>>>>>> >>>>>>>[...] >>>>>>> >>>>>>>>+static void sdap_sudo_refresh_hostinfo_done(struct tevent_req *subreq) >>>>>>>>+{ >>>>>>>>+ struct sdap_sudo_ctx *sudo_ctx; >>>>>>>>+ struct sdap_sudo_refresh_state *state; >>>>>>>>+ struct tevent_req *req; >>>>>>>>+ errno_t ret; >>>>>>>>+ >>>>>>>>+ req = tevent_req_callback_data(subreq, struct tevent_req); >>>>>>>>+ state = tevent_req_data(req, struct sdap_sudo_refresh_state); >>>>>>>>+ >>>>>>>>+ sudo_ctx = state->sudo_ctx; >>>>>>>>+ >>>>>>>>+ ret = sdap_sudo_get_hostinfo_recv(sudo_ctx, subreq, >>>>>>>>&sudo_ctx->hostnames, >>>>>>>>+ &sudo_ctx->ip_addr); >>>>>>>>+ talloc_zfree(subreq); >>>>>>>>+ if (ret != EOK) { >>>>>>>>+ DEBUG(SSSDBG_OP_FAILURE, "Unable to retrieve host information, >>>>>>>>" >>>>>>>>+ "host filter will be disabled [%d]: >>>>>>>>%s\n", >>>>>>>>+ ret, sss_strerror(ret)); >>>>>>>>+ sudo_ctx->use_host_filter = false; >>>>>>>>+ } >>>>>>> >>>>>>>I think you forgot an else here.. >>>>>>> >>>>>> >>>>>>Yes. thanks. >>>>>> >>>>> >>>>>We had a discussion on IRC with Pavel and Jakub >>>>>and patch set caused a bug. The smart refresh was not executed >>>>>if USN was NULL. (full refresh did not found any sudo rules) >>>>> >>>>>e.g. >>>>>* install freeIPA (sssd is started) >>>>>* add sudo rules >>>>>* sudo rules will be downloaded after full refresh >>>>> (which is 1 day by default or after restarting sssd) >>>>> >>>>>LS >>>> >>>>I have changed the patch 0009 in a way so we do not imitate full refresh in >>>>smart refresh but still be able to run smart refresh if no rules were found. >>>> >>>Here is a diff: >>>diff --git a/src/providers/ldap/sdap_async_sudo.c >>>b/src/providers/ldap/sdap_async_sudo.c >>>index 9f66d6c..13824c5 100644 >>>--- a/src/providers/ldap/sdap_async_sudo.c >>>+++ b/src/providers/ldap/sdap_async_sudo.c >>>@@ -305,13 +305,21 @@ static void sdap_sudo_set_usn(struct sdap_server_opts >>>*srv_opts, char *usn) >>> unsigned int usn_number; >>> char *endptr = NULL; >>> >>>- if (usn == NULL) { >>>- DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n"); >>>+ if (srv_opts == NULL) { >>>+ DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n"); >>> return; >>> } >>> >>>- if (srv_opts == NULL) { >>>- DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n"); >>>+ if (usn == NULL) { >>>+ /* If the USN value is unknown and we don't have max_sudo_value set >>>+ * (possibly first full refresh which did not find any rule) we will >>>+ * set zero so smart refresh can pick up. */ >>>+ if (srv_opts->max_sudo_value == NULL) { >>>+ srv_opts->max_sudo_value = "0"; >>I'm sorry that I didn't noticed it earlier. >>You assign static allocated constant here. >>But there is zfree :-) It was not visible from diff >> >>313 if (usn == NULL) { >>314 /* If the USN value is unknown and we don't have max_sudo_value >>set >>315 * (possibly first full refresh which did not find any rule) we >>will >>316 * set zero so smart refresh can pick up. */ >>317 if (srv_opts->max_sudo_value == NULL) { >>318 srv_opts->max_sudo_value = "0"; >>319 return; >>320 } >>321 >>322 DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n"); >>323 return; >>324 } >>325 >>326 talloc_zfree(srv_opts->max_sudo_value); >>327 srv_opts->max_sudo_value = talloc_steal(srv_opts, usn); >> >> >> >>==35602== Process terminating with default action of signal 11 (SIGSEGV) >>==35602== General Protection Fault >>==35602== at 0x3EB8447E2C: vfprintf (vfprintf.c:1641) >>==35602== by 0x3EB84FFB0F: __vsnprintf_chk (vsnprintf_chk.c:65) >>==35602== by 0x3EBE405402: talloc_vasprintf (stdio2.h:78) >>==35602== by 0x3EBE402BDC: talloc_log (talloc.c:288) >>==35602== by 0x3EBE402B13: _talloc_free (talloc.c:356) >>==35602== by 0xD038168: sdap_sudo_refresh_done (sdap_async_sudo.c:326) >>==35602== by 0xD037BCE: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:195) >>==35602== by 0xD005516: generic_ext_search_handler (sdap_async.c:1677) >>==35602== by 0xD009AA2: sdap_get_generic_op_finished (sdap_async.c:1603) >>==35602== by 0xD00AD0E: sdap_process_result (sdap_async.c:352) >>==35602== by 0x3EC1007C90: tevent_common_loop_timer_delay >>(tevent_timed.c:341) >>==35602== by 0x3EC1008CBA: epoll_event_loop_once (tevent_epoll.c:916) >>--35602-- Discarding syms at 0xd8c41f0-0xd8cc648 in >>/lib64/libnss_files-2.12.so due to munmap() >>--35602-- Discarding syms at 0xdad1000-0xdad4388 in /lib64/libnss_dns-2.12.so >>due to munmap() >>--35602-- Discarding syms at 0xf06c760-0xf0717f8 in /lib64/libnss_sss.so.2 >>due to munmap() >> >>LS >Thanks. I should pay more attention when doing a quick fix, I was already >overcoded though. I hope this patch set is the one :-) > ACK to patch set
http://sssd-ci.duckdns.org/logs/job/34/76/summary.html LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
