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";
+ return;
+ }
+
+ DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
return;
}
@@ -748,9 +756,7 @@ static void sdap_sudo_refresh_done(struct tevent_req *subreq)
DEBUG(SSSDBG_TRACE_FUNC, "Sudoers is successfuly stored in cache\n");
/* remember new usn */
- if (usn != NULL) {
- sdap_sudo_set_usn(state->srv_opts, usn);
- }
+ sdap_sudo_set_usn(state->srv_opts, usn);
ret = EOK;
state->num_rules = rules_count;
But unfortunatelly it does not work.
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_sudo_load_sudoers_done] (0x0400):
Receiving sudo rules with base [dc=example,dc=com]
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_id_op_done] (0x4000): releasing
operation connection
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_sudo_refresh_done] (0x0400): Received 0
rules
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sysdb_sudo_purge_byfilter] (0x0400): No rules
matched
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [ldb] (0x4000): commit ldb transaction
(nesting: 0)
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_sudo_refresh_done] (0x0400): Sudoers is
successfuly stored in cache
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_sudo_set_usn] (0x0400): Bug: srv_opts is
NULL
Result is that max_sudo_value was not stored
and smart refresh was not executed.
(Mon Dec 14 07:23:11 2015) [sssd[be[LDAP]]] [be_ptask_execute] (0x0400): Task [SUDO Smart
Refresh]: executing task, timeout 1 seconds
(Mon Dec 14 07:23:11 2015) [sssd[be[LDAP]]] [sdap_sudo_smart_refresh_send] (0x0400): USN
value is unknown, waiting for full refresh!
(Mon Dec 14 07:23:11 2015) [sssd[be[LDAP]]] [be_ptask_done] (0x0040): Task [SUDO Smart
Refresh]: failed with [22]: Invalid argument
(Mon Dec 14 07:23:11 2015) [sssd[be[LDAP]]] [be_ptask_schedule] (0x0400): Task [SUDO Smart
Refresh]: scheduling task 1 seconds from now [1450095792]
LS