On Sun, 2013-01-06 at 00:37 -0500, Simo Sorce wrote:
> While looking at some code my eye fell on the fact that sdap_reinit.c
> was including sysdb_private.h
>
> That's a no-no on its own, you don't get to use private headers
> liberally, or I wouldn't have marked them "private" in the first place!
>
> However besides the abuse of the private headers I found also that the
> function was broken because it wasn't doing what it was trying to do
> (limit cleanups to users, groups and services).
>
> Instead it would search the whole tree (3 times) and later remove all
> entries w/o a USN.
>
> I think this could cause the code to remove *everything* not directly
> downloaded from the IPA tree (for example subdomain users) that lacks
> the SYSDB_USN attribute for example.
>
> I haven't tested the patch yet tbh, but I do not have the setup right
> now, if someone has a 2 servers setup ready and can force sssd to
> reconnect to the second and step through the cleanup to make sure it
> runs as it should I would be grateful.
I am not sure how this function ever worked at all now, I found another
bug, state->sysdb where never assinged, so sysdb was NULl in some
calls ...
Attached rebased patch that adds this fix.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
>From 7d12feff5cff6f7c4565857a1f55de1b77a1dbb1 Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Sun, 6 Jan 2013 00:10:33 -0500
Subject: [PATCH] Fix sdap reinit.
This set of functions had a few important issues:
1. the base_dn was always NULL, as the base array was never actually used
to construct any DN. This means each function searched the whole database
multiple times.
It would try to remove SYSDB_USN from all database entries 3 times.
Then it would try to find non updated entries another 3 times and delete
them, arguably find empty results the last 2 times.
2. Remove use of sysdb_private.h, that header is *PRIVATE* which means it
should not be used anywhere but within sysdb. Do this by using existing
functions instead of using ldb calls directly. This is important to keep
sysdb as conistent and self-contained as possible.
---
src/db/sysdb_services.c | 57 +++++++++++++
src/db/sysdb_services.h | 7 ++
src/providers/ldap/sdap_reinit.c | 167 ++++++++++++++++++++-------------------
3 files changed, 151 insertions(+), 80 deletions(-)
diff --git a/src/db/sysdb_services.c b/src/db/sysdb_services.c
index f3ed48ed8f433d083d40ca3bc2ccc47d039c653a..fb25d4a859674a3433dbf6a98f07eb5564bf1ff3 100644
--- a/src/db/sysdb_services.c
+++ b/src/db/sysdb_services.c
@@ -802,3 +802,60 @@ done:
talloc_free(tmp_ctx);
return ret;
}
+
+errno_t sysdb_search_services(TALLOC_CTX *mem_ctx,
+ struct sysdb_ctx *sysdb,
+ const char *sub_filter,
+ const char **attrs,
+ size_t *msgs_count,
+ struct ldb_message ***msgs)
+{
+ TALLOC_CTX *tmp_ctx;
+ struct ldb_dn *basedn;
+ char *filter;
+ int ret;
+
+ tmp_ctx = talloc_new(NULL);
+ if (!tmp_ctx) {
+ return ENOMEM;
+ }
+
+ basedn = ldb_dn_new_fmt(tmp_ctx, sysdb->ldb,
+ SYSDB_TMPL_SVC_BASE, sysdb->domain->name);
+ if (!basedn) {
+ DEBUG(2, ("Failed to build base dn\n"));
+ ret = ENOMEM;
+ goto fail;
+ }
+
+ filter = talloc_asprintf(tmp_ctx, "(&(%s)%s)", SYSDB_SC, sub_filter);
+ if (!filter) {
+ DEBUG(2, ("Failed to build filter\n"));
+ ret = ENOMEM;
+ goto fail;
+ }
+
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ ("Search services with filter: %s\n", filter));
+
+ ret = sysdb_search_entry(mem_ctx, sysdb, basedn,
+ LDB_SCOPE_SUBTREE, filter, attrs,
+ msgs_count, msgs);
+ if (ret) {
+ goto fail;
+ }
+
+ talloc_zfree(tmp_ctx);
+ return EOK;
+
+fail:
+ if (ret == ENOENT) {
+ DEBUG(SSSDBG_TRACE_INTERNAL, ("No such entry\n"));
+ }
+ else if (ret) {
+ DEBUG(SSSDBG_MINOR_FAILURE, ("Error: %d (%s)\n", ret, strerror(ret)));
+ }
+ talloc_zfree(tmp_ctx);
+ return ret;
+}
+
diff --git a/src/db/sysdb_services.h b/src/db/sysdb_services.h
index 29d531ed847027ae96f44170ed1f98b7e41693a3..351a89f5bb13ef064f48db64dc374d00bf1e420a 100644
--- a/src/db/sysdb_services.h
+++ b/src/db/sysdb_services.h
@@ -91,4 +91,11 @@ sysdb_set_service_attr(struct sysdb_ctx *sysdb,
struct sysdb_attrs *attrs,
int mod_op);
+errno_t sysdb_search_services(TALLOC_CTX *mem_ctx,
+ struct sysdb_ctx *sysdb,
+ const char *sub_filter,
+ const char **attrs,
+ size_t *msgs_count,
+ struct ldb_message ***msgs);
+
#endif /* SYSDB_SERVICES_H_ */
diff --git a/src/providers/ldap/sdap_reinit.c b/src/providers/ldap/sdap_reinit.c
index 0200de0a2a9ebcad2e3b375b4c600e43697fdf17..792a659186e942473419401216d2daea6100d5b4 100644
--- a/src/providers/ldap/sdap_reinit.c
+++ b/src/providers/ldap/sdap_reinit.c
@@ -27,7 +27,6 @@
#include "providers/ldap/ldap_common.h"
#include "db/sysdb.h"
#include "db/sysdb_services.h"
-#include "db/sysdb_private.h"
struct sdap_reinit_cleanup_state {
struct sysdb_ctx *sysdb;
@@ -61,13 +60,15 @@ struct tevent_req* sdap_reinit_cleanup_send(TALLOC_CTX *mem_ctx,
return NULL;
}
+ state->sysdb = be_ctx->domain->sysdb;
+
if (!be_ctx->domain->enumerate) {
/* enumeration is disabled, this whole process is meaningless */
ret = EOK;
goto immediately;
}
- ret = sdap_reinit_clear_usn(be_ctx->domain->sysdb);
+ ret = sdap_reinit_clear_usn(state->sysdb);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to clear USN attributes [%d]: %s\n",
ret, strerror(ret)));
@@ -96,23 +97,32 @@ immediately:
return req;
}
+static void sdap_delete_msgs_usn(struct sysdb_ctx *sysdb,
+ struct ldb_message **msgs,
+ size_t msgs_num)
+{
+ struct ldb_message_element el = { 0, SYSDB_USN, 0, NULL };
+ struct sysdb_attrs usn_el = { 1, &el };
+ errno_t ret;
+ int i;
+
+ for (i = 0; i < msgs_num; i++) {
+ ret = sysdb_set_entry_attr(sysdb, msgs[i]->dn, &usn_el, SYSDB_MOD_DEL);
+ if (ret) {
+ DEBUG(SSSDBG_TRACE_FUNC, ("Failed to clean USN on entry: [%s]\n",
+ ldb_dn_get_linearized(msgs[i]->dn)));
+ }
+ }
+}
+
static errno_t sdap_reinit_clear_usn(struct sysdb_ctx *sysdb)
{
TALLOC_CTX *tmp_ctx = NULL;
bool in_transaction = false;
- struct ldb_result *result = NULL;
- struct ldb_message **messages = NULL;
- struct ldb_message *msg = NULL;
- int messages_num = 0;
- struct ldb_dn *base_dn = NULL;
- const char *base[] = { SYSDB_TMPL_USER_BASE,
- SYSDB_TMPL_GROUP_BASE,
- SYSDB_TMPL_SVC_BASE,
- NULL };
+ struct ldb_message **msgs = NULL;
+ size_t msgs_num = 0;
const char *attrs[] = { "dn", NULL };
- int i, j;
int sret;
- int lret;
errno_t ret;
tmp_ctx = talloc_new(NULL);
@@ -121,56 +131,35 @@ static errno_t sdap_reinit_clear_usn(struct sysdb_ctx *sysdb)
return ENOMEM;
}
- for (i = 0; base[i] != NULL; i++) {
- lret = ldb_search(sysdb->ldb, tmp_ctx, &result, base_dn,
- LDB_SCOPE_SUBTREE, attrs, NULL);
- if (lret != LDB_SUCCESS) {
- ret = sysdb_error_to_errno(lret);
- goto done;
- }
-
- if (result->count == 0) {
- talloc_zfree(result);
- continue;
- }
-
- messages = talloc_realloc(tmp_ctx, messages, struct ldb_message*,
- messages_num + result->count);
-
- for (j = 0; j < result->count; j++) {
- msg = ldb_msg_new(messages);
- if (msg == NULL) {
- ret = ENOMEM;
- goto done;
- }
- msg->dn = talloc_move(tmp_ctx, &result->msgs[j]->dn);
-
- lret = ldb_msg_add_empty(msg, SYSDB_USN, LDB_FLAG_MOD_DELETE, NULL);
- if (lret != LDB_SUCCESS) {
- ret = sysdb_error_to_errno(lret);
- goto done;
- }
-
- messages[messages_num + j] = msg;
- }
-
- messages_num += result->count;
- talloc_zfree(result);
- }
-
ret = sysdb_transaction_start(sysdb);
if (ret != EOK) {
goto done;
}
in_transaction = true;
- for (i = 0; i < messages_num; i++) {
- lret = ldb_modify(sysdb->ldb, messages[i]);
- if (lret != LDB_SUCCESS) {
- ret = sysdb_error_to_errno(lret);
- goto done;
- }
+ /* reset users' usn */
+ ret = sysdb_search_users(tmp_ctx, sysdb, "", attrs, &msgs_num, &msgs);
+ if (ret != EOK) {
+ goto done;
}
+ sdap_delete_msgs_usn(sysdb, msgs, msgs_num);
+ talloc_zfree(msgs);
+ msgs_num = 0;
+
+ /* reset groups' usn */
+ ret = sysdb_search_groups(tmp_ctx, sysdb, "", attrs, &msgs_num, &msgs);
+ if (ret != EOK) {
+ goto done;
+ }
+ sdap_delete_msgs_usn(sysdb, msgs, msgs_num);
+ talloc_zfree(msgs);
+ msgs_num = 0;
+
+ /* reset services' usn */
+ ret = sysdb_search_services(tmp_ctx, sysdb, "", attrs, &msgs_num, &msgs);
+ sdap_delete_msgs_usn(sysdb, msgs, msgs_num);
+ talloc_zfree(msgs);
+ msgs_num = 0;
ret = sysdb_transaction_commit(sysdb);
if (ret == EOK) {
@@ -234,20 +223,30 @@ fail:
tevent_req_error(req, ret);
}
+static void sdap_delete_msgs_dn(struct sysdb_ctx *sysdb,
+ struct ldb_message **msgs,
+ size_t msgs_num)
+{
+ errno_t ret;
+ int i;
+
+ for (i = 0; i < msgs_num; i++) {
+ ret = sysdb_delete_entry(sysdb, msgs[i]->dn, true);
+ if (ret) {
+ DEBUG(SSSDBG_TRACE_FUNC, ("Failed to delete entry: [%s]\n",
+ ldb_dn_get_linearized(msgs[i]->dn)));
+ }
+ }
+}
+
static errno_t sdap_reinit_delete_records(struct sysdb_ctx *sysdb)
{
TALLOC_CTX *tmp_ctx = NULL;
bool in_transaction = false;
- struct ldb_result *result = NULL;
- struct ldb_dn *base_dn = NULL;
- const char *base[] = { SYSDB_TMPL_USER_BASE,
- SYSDB_TMPL_GROUP_BASE,
- SYSDB_TMPL_SVC_BASE,
- NULL };
+ struct ldb_message **msgs = NULL;
+ size_t msgs_num = 0;
const char *attrs[] = { "dn", NULL };
- int i, j;
int sret;
- int lret;
errno_t ret;
tmp_ctx = talloc_new(NULL);
@@ -262,24 +261,32 @@ static errno_t sdap_reinit_delete_records(struct sysdb_ctx *sysdb)
}
in_transaction = true;
- for (i = 0; base[i] != NULL; i++) {
- lret = ldb_search(sysdb->ldb, tmp_ctx, &result, base_dn,
- LDB_SCOPE_SUBTREE, attrs, "(!("SYSDB_USN"=*))");
- if (lret != LDB_SUCCESS) {
- ret = sysdb_error_to_errno(lret);
- goto done;
- }
-
- for (j = 0; j < result->count; j++) {
- ret = ldb_delete(sysdb->ldb, result->msgs[i]->dn);
- if (ret != LDB_SUCCESS) {
- ret = sysdb_error_to_errno(ret);
- goto done;
- }
- }
+ /* purge untouched users */
+ ret = sysdb_search_users(tmp_ctx, sysdb, "(!("SYSDB_USN"=*))",
+ attrs, &msgs_num, &msgs);
+ if (ret != EOK) {
+ goto done;
+ }
+ sdap_delete_msgs_dn(sysdb, msgs, msgs_num);
+ talloc_zfree(msgs);
+ msgs_num = 0;
- talloc_zfree(result);
+ /* purge untouched groups */
+ ret = sysdb_search_groups(tmp_ctx, sysdb, "(!("SYSDB_USN"=*))",
+ attrs, &msgs_num, &msgs);
+ if (ret != EOK) {
+ goto done;
}
+ sdap_delete_msgs_dn(sysdb, msgs, msgs_num);
+ talloc_zfree(msgs);
+ msgs_num = 0;
+
+ /* purge untouched services */
+ ret = sysdb_search_services(tmp_ctx, sysdb, "(!("SYSDB_USN"=*))",
+ attrs, &msgs_num, &msgs);
+ sdap_delete_msgs_dn(sysdb, msgs, msgs_num);
+ talloc_zfree(msgs);
+ msgs_num = 0;
ret = sysdb_transaction_commit(sysdb);
if (ret == EOK) {
--
1.8.0.1
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
