URL: https://github.com/SSSD/sssd/pull/583 Author: fidencio Title: #583: sudo/sysdb regressions Action: opened
PR body: """ This patch set consists in 3 patches: - sudo_ldap: fix sudoHost=defaults -> cn=defaults: this is a typo that caused https://pagure.io/SSSD/sssd/issue/3742 - Revert "sysdb custom: completely replace old object instead of merging it": this one caused https://pagure.io/SSSD/sssd/issue/3733 - sysdb_sudo: completely replace old object instead of merging it: As far as I understand, the idea behind cd4590de was to never merged sudo rules. Instead, delete the old one and add the new one. However, doing this all over place caused the regression mentioned above. I've checked the other patches that leaded to this one and seems that keeping the "delete the old one and add the new one" approach may be the cleaner possible way. @pbrezina, may I ask you to (re)test https://pagure.io/SSSD/sssd/issue/3558 in order to be sure that I won't be adding regressions while trying to fix regressions? Also, I do believe this approach is cleaner than adding a new boolean flag in sysdb_store_custom(), do you agree? """ To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/583/head:pr583 git checkout pr583
From 01c0c911d9487b01cf5978d31e170da2b82b3977 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Tue, 29 May 2018 13:04:43 +0200 Subject: [PATCH 1/3] sudo_ldap: fix sudoHost=defaults -> cn=defaults in the filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a typo introduced as part of 47ad0778. Resolves: https://pagure.io/SSSD/sssd/issue/3742 Related: https://pagure.io/SSSD/sssd/issue/3558 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/providers/ldap/sdap_async_sudo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c index 3da76256e..c20132a01 100644 --- a/src/providers/ldap/sdap_async_sudo.c +++ b/src/providers/ldap/sdap_async_sudo.c @@ -161,7 +161,7 @@ static char *sdap_sudo_build_host_filter(TALLOC_CTX *mem_ctx, /* sudoHost is not specified and it is a cn=defaults rule */ filter = talloc_asprintf_append_buffer(filter, "(&(!(%s=*))(%s=defaults))", map[SDAP_AT_SUDO_HOST].name, - map[SDAP_AT_SUDO_HOST].name); + map[SDAP_AT_SUDO_NAME].name); if (filter == NULL) { goto done; } From 290861fb7f628a6240499468174fdeb2943863da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Tue, 29 May 2018 23:22:06 +0200 Subject: [PATCH 2/3] Revert "sysdb custom: completely replace old object instead of merging it" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit cd4590de2a84b8143a6c75b5198f5e1b3c0a6d63, as the commit introduced a regression on known_hosts. Resolves: https://pagure.io/SSSD/sssd/issue/3733 Related: https://pagure.io/SSSD/sssd/issue/3558 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/db/sysdb_ops.c | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index 7e0a16542..699cb6645 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -3430,7 +3430,12 @@ int sysdb_store_custom(struct sss_domain_info *domain, struct sysdb_attrs *attrs) { TALLOC_CTX *tmp_ctx; + const char *search_attrs[] = { "*", NULL }; + size_t resp_count = 0; + struct ldb_message **resp; struct ldb_message *msg; + struct ldb_message_element *el; + bool add_object = false; int ret; int i; @@ -3449,12 +3454,17 @@ int sysdb_store_custom(struct sss_domain_info *domain, goto done; } - /* Always add a new object. */ - ret = sysdb_delete_custom(domain, object_name, subtree_name); - if (ret != EOK) { + ret = sysdb_search_custom_by_name(tmp_ctx, domain, + object_name, subtree_name, + search_attrs, &resp_count, &resp); + if (ret != EOK && ret != ENOENT) { goto done; } + if (ret == ENOENT) { + add_object = true; + } + msg = ldb_msg_new(tmp_ctx); if (msg == NULL) { ret = ENOMEM; @@ -3476,11 +3486,24 @@ int sysdb_store_custom(struct sss_domain_info *domain, for (i = 0; i < attrs->num; i++) { msg->elements[i] = attrs->a[i]; - msg->elements[i].flags = LDB_FLAG_MOD_ADD; + if (add_object) { + msg->elements[i].flags = LDB_FLAG_MOD_ADD; + } else { + el = ldb_msg_find_element(resp[0], attrs->a[i].name); + if (el == NULL) { + msg->elements[i].flags = LDB_FLAG_MOD_ADD; + } else { + msg->elements[i].flags = LDB_FLAG_MOD_REPLACE; + } + } } msg->num_elements = attrs->num; - ret = ldb_add(domain->sysdb->ldb, msg); + if (add_object) { + ret = ldb_add(domain->sysdb->ldb, msg); + } else { + ret = ldb_modify(domain->sysdb->ldb, msg); + } if (ret != LDB_SUCCESS) { DEBUG(SSSDBG_CRIT_FAILURE, "Failed to store custom entry: %s(%d)[%s]\n", ldb_strerror(ret), ret, ldb_errstring(domain->sysdb->ldb)); From 57e9a1928451ac3d8261beb48c2cdcd504ab67cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Tue, 29 May 2018 23:30:03 +0200 Subject: [PATCH 3/3] sysdb_sudo: completely replace old object instead of merging it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's make sure that we do not merge two record in sysdb_sudo. 1) If there are two rules with the same cn (possible with multiple search bases or organizational units) we would end up merging those two rules instead of choosing one of them. 2) Also smart refresh would merge the diff insteand of removing the attributes that are no longer present in ldap. Since 1) is a rare use case and it is a misconfiguration we completely replace the old rule with new one. It is simpler to implement and it solves both issues. Resolves: https://pagure.io/SSSD/sssd/issue/3558 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/db/sysdb_sudo.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/db/sysdb_sudo.c b/src/db/sysdb_sudo.c index ff8c95105..3ad462d8f 100644 --- a/src/db/sysdb_sudo.c +++ b/src/db/sysdb_sudo.c @@ -956,6 +956,14 @@ sysdb_sudo_store_rule(struct sss_domain_info *domain, return ret; } + /* Always delete the old rule and add a new one */ + ret = sysdb_delete_custom(domain, name, SUDORULE_SUBDIR); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "Unable to delete the old rule %s [%d]: %s\n", + name, ret, strerror(ret)); + return ret; + } + ret = sysdb_store_custom(domain, name, SUDORULE_SUBDIR, rule); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "Unable to store rule %s [%d]: %s\n",
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/sssd-devel@lists.fedorahosted.org/message/HATBNUEJRPSRCIM4SUUK32RXNQFVC2FN/
