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