URL: https://github.com/SSSD/sssd/pull/554
Author: jhrozek
Title: #554: Several fixes for the files provider
Action: opened
PR body:
"""
This PR contains several fixes for the files provider, mainly a performance
bug which is really embarassing - I have no idea why was the code #if-ed
out. I think I must have been experimenting with the provider update
and then forgot to remove the preprocessor macros. The rest is mostly code
cleanup and minor fixes.
btw initially I wanted to also include a fix to avoid removing cachedPassword
on updates, but I realized the current way where the files provider throws
away everything and then updates everything would force us to maintain
all the sssd-added attributes on our own. And because especially with 2FA
it might not be just cachedPassword, but also the factor length or offline
lockout counter etc, we might as well devise a better way to update the
cache than just throw away everything and recreate the entries, so I'll work
on that separately.
"""
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/554/head:pr554
git checkout pr554
From e6a8757fe90ca3c614c241b736bd2a3973898fae Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Wed, 4 Apr 2018 14:18:10 +0200
Subject: [PATCH 1/5] FILES: Do not overwrite and actually remove
files_ctx.{pwd,grp}_watch
The snotify_ctx structures were unused, are completely opaque (their
only value is that if they are freed, the watches disappear which
the files provider never does).
And moreover, since the patches to support multiple files, the watches
were overwritten with subsequent assignments.
---
src/providers/files/files_ops.c | 35 +++++++++++++++++++++++------------
1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/src/providers/files/files_ops.c b/src/providers/files/files_ops.c
index a2a2798d3..b71064d24 100644
--- a/src/providers/files/files_ops.c
+++ b/src/providers/files/files_ops.c
@@ -36,9 +36,6 @@
#define GRP_MAXSIZE 2048
struct files_ctx {
- struct snotify_ctx *pwd_watch;
- struct snotify_ctx *grp_watch;
-
struct files_ops_ctx *ops;
};
@@ -957,6 +954,7 @@ struct files_ctx *sf_init(TALLOC_CTX *mem_ctx,
struct files_ctx *fctx;
struct tevent_immediate *imm;
int i;
+ struct snotify_ctx *snctx;
fctx = talloc(mem_ctx, struct files_ctx);
if (fctx == NULL) {
@@ -964,18 +962,31 @@ struct files_ctx *sf_init(TALLOC_CTX *mem_ctx,
}
for (i = 0; passwd_files[i]; i++) {
- fctx->pwd_watch = sf_setup_watch(fctx, ev, passwd_files[i],
- sf_passwd_cb, id_ctx);
+ snctx = sf_setup_watch(fctx, ev, passwd_files[i],
+ sf_passwd_cb, id_ctx);
+ if (snctx == NULL) {
+ DEBUG(SSSDBG_FATAL_FAILURE,
+ "Cannot set watch for passwd file %s\n", passwd_files[i]);
+ /* Rather than reporting incomplete or inconsistent information
+ * in case e.g. group memberships span multiple files, just abort
+ */
+ talloc_free(fctx);
+ return NULL;
}
-
- for (i = 0; group_files[i]; i++) {
- fctx->grp_watch = sf_setup_watch(fctx, ev, group_files[i],
- sf_group_cb, id_ctx);
}
- if (fctx->pwd_watch == NULL || fctx->grp_watch == NULL) {
- talloc_free(fctx);
- return NULL;
+ for (i = 0; group_files[i]; i++) {
+ snctx = sf_setup_watch(fctx, ev, group_files[i],
+ sf_group_cb, id_ctx);
+ if (snctx == NULL) {
+ DEBUG(SSSDBG_FATAL_FAILURE,
+ "Cannot set watch for group file %s\n", group_files[i]);
+ /* Rather than reporting incomplete or inconsistent information
+ * in case e.g. group memberships span multiple files, just abort
+ */
+ talloc_free(fctx);
+ return NULL;
+ }
}
/* Enumerate users and groups on startup to process any changes when
From 7ec10888ed90fc52782ece74e453b26e46725b40 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Tue, 17 Apr 2018 14:22:39 +0200
Subject: [PATCH 2/5] FILES: Reduce code duplication
---
src/providers/files/files_ops.c | 213 +++++++++++++++-------------------------
1 file changed, 81 insertions(+), 132 deletions(-)
diff --git a/src/providers/files/files_ops.c b/src/providers/files/files_ops.c
index b71064d24..6ac824df0 100644
--- a/src/providers/files/files_ops.c
+++ b/src/providers/files/files_ops.c
@@ -35,6 +35,10 @@
#define PWD_MAXSIZE 1024
#define GRP_MAXSIZE 2048
+#define SF_UPDATE_PASSWD 1<<0
+#define SF_UPDATE_GROUP 1<<1
+#define SF_UPDATE_BOTH (SF_UPDATE_PASSWD | SF_UPDATE_GROUP)
+
struct files_ctx {
struct files_ops_ctx *ops;
};
@@ -708,6 +712,70 @@ static errno_t sf_enum_groups(struct files_id_ctx *id_ctx,
return ret;
}
+static errno_t sf_enum_files(struct files_id_ctx *id_ctx,
+ uint8_t flags)
+{
+ errno_t ret;
+ errno_t tret;
+ bool in_transaction = false;
+
+ ret = sysdb_transaction_start(id_ctx->domain->sysdb);
+ if (ret != EOK) {
+ goto done;
+ }
+ in_transaction = true;
+
+ if (flags & SF_UPDATE_PASSWD) {
+ ret = delete_all_users(id_ctx->domain);
+ if (ret != EOK) {
+ goto done;
+ }
+
+ /* All users were deleted, therefore we need to enumerate each file again */
+ for (size_t i = 0; id_ctx->passwd_files[i] != NULL; i++) {
+ ret = sf_enum_users(id_ctx, id_ctx->passwd_files[i]);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, "Cannot enumerate users\n");
+ goto done;
+ }
+ }
+ }
+
+ if (flags & SF_UPDATE_GROUP) {
+ ret = delete_all_groups(id_ctx->domain);
+ if (ret != EOK) {
+ goto done;
+ }
+
+ /* All groups were deleted, therefore we need to enumerate each file again */
+ for (size_t i = 0; id_ctx->group_files[i] != NULL; i++) {
+ ret = sf_enum_groups(id_ctx, id_ctx->group_files[i]);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, "Cannot enumerate groups\n");
+ goto done;
+ }
+ }
+ }
+
+ ret = sysdb_transaction_commit(id_ctx->domain->sysdb);
+ if (ret != EOK) {
+ goto done;
+ }
+ in_transaction = false;
+
+ ret = EOK;
+done:
+ if (in_transaction) {
+ tret = sysdb_transaction_cancel(id_ctx->domain->sysdb);
+ if (tret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Cannot cancel transaction: %d\n", ret);
+ }
+ }
+
+ return ret;
+}
+
static void sf_cb_done(struct files_id_ctx *id_ctx)
{
/* Only activate a domain when both callbacks are done */
@@ -722,8 +790,6 @@ static int sf_passwd_cb(const char *filename, uint32_t flags, void *pvt)
{
struct files_id_ctx *id_ctx;
errno_t ret;
- errno_t tret;
- bool in_transaction = false;
id_ctx = talloc_get_type(pvt, struct files_id_ctx);
if (id_ctx == NULL) {
@@ -740,49 +806,17 @@ static int sf_passwd_cb(const char *filename, uint32_t flags, void *pvt)
dp_sbus_reset_users_memcache(id_ctx->be->provider);
dp_sbus_reset_initgr_memcache(id_ctx->be->provider);
- ret = sysdb_transaction_start(id_ctx->domain->sysdb);
- if (ret != EOK) {
- goto done;
- }
- in_transaction = true;
-
- ret = delete_all_users(id_ctx->domain);
- if (ret != EOK) {
- goto done;
- }
-
- /* All users were deleted, therefore we need to enumerate each file again */
- for (size_t i = 0; id_ctx->passwd_files[i] != NULL; i++) {
- ret = sf_enum_users(id_ctx, id_ctx->passwd_files[i]);
- if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE, "Cannot enumerate users\n");
- goto done;
- }
- }
-
- /* Covers the case when someone edits /etc/group, adds a group member and
+ /* Using SF_UDPATE_BOTH here the case when someone edits /etc/group, adds a group member and
* only then edits passwd and adds the user. The reverse is not needed,
* because member/memberof links are established when groups are saved.
*/
- ret = delete_all_groups(id_ctx->domain);
- if (ret != EOK) {
- goto done;
- }
-
- /* All groups were deleted, therefore we need to enumerate each file again */
- for (size_t i = 0; id_ctx->group_files[i] != NULL; i++) {
- ret = sf_enum_groups(id_ctx, id_ctx->group_files[i]);
- if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE, "Cannot enumerate groups\n");
- goto done;
- }
- }
-
- ret = sysdb_transaction_commit(id_ctx->domain->sysdb);
+ ret = sf_enum_files(id_ctx, SF_UPDATE_BOTH);
if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Could not update files: [%d]: %s\n",
+ ret, sss_strerror(ret));
goto done;
}
- in_transaction = false;
id_ctx->updating_passwd = false;
sf_cb_done(id_ctx);
@@ -790,14 +824,6 @@ static int sf_passwd_cb(const char *filename, uint32_t flags, void *pvt)
ret = EOK;
done:
- if (in_transaction) {
- tret = sysdb_transaction_cancel(id_ctx->domain->sysdb);
- if (tret != EOK) {
- DEBUG(SSSDBG_CRIT_FAILURE,
- "Cannot cancel transaction: %d\n", ret);
- }
- }
-
return ret;
}
@@ -805,8 +831,6 @@ static int sf_group_cb(const char *filename, uint32_t flags, void *pvt)
{
struct files_id_ctx *id_ctx;
errno_t ret;
- errno_t tret;
- bool in_transaction = false;
id_ctx = talloc_get_type(pvt, struct files_id_ctx);
if (id_ctx == NULL) {
@@ -823,47 +847,20 @@ static int sf_group_cb(const char *filename, uint32_t flags, void *pvt)
dp_sbus_reset_groups_memcache(id_ctx->be->provider);
dp_sbus_reset_initgr_memcache(id_ctx->be->provider);
- ret = sysdb_transaction_start(id_ctx->domain->sysdb);
- if (ret != EOK) {
- goto done;
- }
- in_transaction = true;
-
- ret = delete_all_groups(id_ctx->domain);
- if (ret != EOK) {
- goto done;
- }
-
- /* All groups were deleted, therefore we need to enumerate each file again */
- for (size_t i = 0; id_ctx->group_files[i] != NULL; i++) {
- ret = sf_enum_groups(id_ctx, id_ctx->group_files[i]);
- if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE, "Cannot enumerate groups\n");
- goto done;
- }
- }
-
- ret = sysdb_transaction_commit(id_ctx->domain->sysdb);
+ ret = sf_enum_files(id_ctx, SF_UPDATE_GROUP);
if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Could not update files: [%d]: %s\n",
+ ret, sss_strerror(ret));
goto done;
}
- in_transaction = false;
id_ctx->updating_groups = false;
sf_cb_done(id_ctx);
files_account_info_finished(id_ctx, BE_REQ_GROUP, ret);
ret = EOK;
-
done:
- if (in_transaction) {
- tret = sysdb_transaction_cancel(id_ctx->domain->sysdb);
- if (tret != EOK) {
- DEBUG(SSSDBG_CRIT_FAILURE,
- "Cannot cancel transaction: %d\n", ret);
- }
- }
-
return ret;
}
@@ -873,62 +870,14 @@ static void startup_enum_files(struct tevent_context *ev,
{
struct files_id_ctx *id_ctx = talloc_get_type(pvt, struct files_id_ctx);
errno_t ret;
- errno_t tret;
- bool in_transaction = false;
talloc_zfree(imm);
- ret = sysdb_transaction_start(id_ctx->domain->sysdb);
- if (ret != EOK) {
- goto done;
- }
- in_transaction = true;
-
- ret = delete_all_users(id_ctx->domain);
- if (ret != EOK) {
- goto done;
- }
-
- ret = delete_all_groups(id_ctx->domain);
+ ret = sf_enum_files(id_ctx, SF_UPDATE_BOTH);
if (ret != EOK) {
- goto done;
- }
-
- for (size_t i = 0; id_ctx->passwd_files[i] != NULL; i++) {
- DEBUG(SSSDBG_TRACE_FUNC,
- "Startup user enumeration of [%s]\n", id_ctx->passwd_files[i]);
- ret = sf_enum_users(id_ctx, id_ctx->passwd_files[i]);
- if (ret != EOK) {
- DEBUG(SSSDBG_CRIT_FAILURE,
- "Enumerating users failed, data might be inconsistent!\n");
- goto done;
- }
- }
-
- for (size_t i = 0; id_ctx->group_files[i] != NULL; i++) {
- DEBUG(SSSDBG_TRACE_FUNC,
- "Startup group enumeration of [%s]\n", id_ctx->group_files[i]);
- ret = sf_enum_groups(id_ctx, id_ctx->group_files[i]);
- if (ret != EOK) {
- DEBUG(SSSDBG_CRIT_FAILURE,
- "Enumerating groups failed, data might be inconsistent!\n");
- goto done;
- }
- }
-
- ret = sysdb_transaction_commit(id_ctx->domain->sysdb);
- if (ret != EOK) {
- goto done;
- }
- in_transaction = false;
-
-done:
- if (in_transaction) {
- tret = sysdb_transaction_cancel(id_ctx->domain->sysdb);
- if (tret != EOK) {
- DEBUG(SSSDBG_CRIT_FAILURE,
- "Cannot cancel transaction: %d\n", ret);
- }
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Could not update files after startup: [%d]: %s\n",
+ ret, sss_strerror(ret));
}
}
From e7631c9ed69b547f6c9edab36ca4edf2e6d9a8f9 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Tue, 17 Apr 2018 14:38:03 +0200
Subject: [PATCH 3/5] FILES: Reset the domain status back even on errors
The block that resets the domain status was only called on success, so
on error, the domain would have been permanently stuck in an
inconsistent state.
---
src/providers/files/files_ops.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/providers/files/files_ops.c b/src/providers/files/files_ops.c
index 6ac824df0..b21ada559 100644
--- a/src/providers/files/files_ops.c
+++ b/src/providers/files/files_ops.c
@@ -793,8 +793,7 @@ static int sf_passwd_cb(const char *filename, uint32_t flags, void *pvt)
id_ctx = talloc_get_type(pvt, struct files_id_ctx);
if (id_ctx == NULL) {
- ret = EINVAL;
- goto done;
+ return EINVAL;
}
DEBUG(SSSDBG_TRACE_FUNC, "passwd notification\n");
@@ -818,12 +817,11 @@ static int sf_passwd_cb(const char *filename, uint32_t flags, void *pvt)
goto done;
}
+ ret = EOK;
+done:
id_ctx->updating_passwd = false;
sf_cb_done(id_ctx);
files_account_info_finished(id_ctx, BE_REQ_USER, ret);
-
- ret = EOK;
-done:
return ret;
}
@@ -834,8 +832,7 @@ static int sf_group_cb(const char *filename, uint32_t flags, void *pvt)
id_ctx = talloc_get_type(pvt, struct files_id_ctx);
if (id_ctx == NULL) {
- ret = EINVAL;
- goto done;
+ return EINVAL;
}
DEBUG(SSSDBG_TRACE_FUNC, "group notification\n");
@@ -855,12 +852,11 @@ static int sf_group_cb(const char *filename, uint32_t flags, void *pvt)
goto done;
}
+ ret = EOK;
+done:
id_ctx->updating_groups = false;
sf_cb_done(id_ctx);
files_account_info_finished(id_ctx, BE_REQ_GROUP, ret);
-
- ret = EOK;
-done:
return ret;
}
From 5aa8d677c7f482db3e0e286794b536ac00f615de Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Wed, 4 Apr 2018 14:13:56 +0200
Subject: [PATCH 4/5] FILES: Skip files that are not created yet
In order to avoid complex ordering logic, even if one file is updated,
we flush all the entries. In theory, we could only flush the individual
file and all the files preceding it, but it's safer to just create a
complete mirror every time.
And this can be problematic if one of the files we try to update is not
created yet during the update. This can happen e.g. when a file is not
created during early boot.
To solve this, try to be very defensive and always flush the whole
database, ignore ENOENT errors, but abort on all other errors.
---
src/providers/files/files_ops.c | 22 ++++++++++---
src/tests/intg/test_files_provider.py | 58 +++++++++++++++++++++++++++++++++++
2 files changed, 76 insertions(+), 4 deletions(-)
diff --git a/src/providers/files/files_ops.c b/src/providers/files/files_ops.c
index b21ada559..b08ca7745 100644
--- a/src/providers/files/files_ops.c
+++ b/src/providers/files/files_ops.c
@@ -734,8 +734,15 @@ static errno_t sf_enum_files(struct files_id_ctx *id_ctx,
/* All users were deleted, therefore we need to enumerate each file again */
for (size_t i = 0; id_ctx->passwd_files[i] != NULL; i++) {
ret = sf_enum_users(id_ctx, id_ctx->passwd_files[i]);
- if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE, "Cannot enumerate users\n");
+ if (ret == ENOENT) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "The file %s does not exist (yet), skipping\n",
+ id_ctx->passwd_files[i]);
+ continue;
+ } else if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Cannot enumerate users from %s, aborting\n",
+ id_ctx->passwd_files[i]);
goto done;
}
}
@@ -750,8 +757,15 @@ static errno_t sf_enum_files(struct files_id_ctx *id_ctx,
/* All groups were deleted, therefore we need to enumerate each file again */
for (size_t i = 0; id_ctx->group_files[i] != NULL; i++) {
ret = sf_enum_groups(id_ctx, id_ctx->group_files[i]);
- if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE, "Cannot enumerate groups\n");
+ if (ret == ENOENT) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "The file %s does not exist (yet), skipping\n",
+ id_ctx->group_files[i]);
+ continue;
+ } else if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Cannot enumerate groups from %s, aborting\n",
+ id_ctx->group_files[i]);
goto done;
}
}
diff --git a/src/tests/intg/test_files_provider.py b/src/tests/intg/test_files_provider.py
index ce5c7b774..4b4b4373e 100644
--- a/src/tests/intg/test_files_provider.py
+++ b/src/tests/intg/test_files_provider.py
@@ -187,6 +187,39 @@ def files_multiple_sources(request):
return alt_pwops, alt_grops
+@pytest.fixture
+def files_multiple_sources_nocreate(request):
+ """
+ Sets up SSSD with multiple sources, but does not actually create
+ the files.
+ """
+ alt_passwd_path = tempfile.mktemp(prefix='altpasswd')
+ request.addfinalizer(lambda: os.unlink(alt_passwd_path))
+
+ alt_group_path = tempfile.mktemp(prefix='altgroup')
+ request.addfinalizer(lambda: os.unlink(alt_group_path))
+
+ passwd_list = ",".join([os.environ["NSS_WRAPPER_PASSWD"], alt_passwd_path])
+ group_list = ",".join([os.environ["NSS_WRAPPER_GROUP"], alt_group_path])
+
+ conf = unindent("""\
+ [sssd]
+ domains = files
+ services = nss
+
+ [nss]
+ debug_level = 10
+
+ [domain/files]
+ id_provider = files
+ passwd_files = {passwd_list}
+ group_files = {group_list}
+ debug_level = 10
+ """).format(**locals())
+ create_conf_fixture(request, conf)
+ create_sssd_fixture(request)
+ return alt_passwd_path, alt_group_path
+
@pytest.fixture
def proxy_to_files_domain_only(request):
conf = unindent("""\
@@ -1113,3 +1146,28 @@ def test_multiple_passwd_group_files(add_user_with_canary,
check_group(GROUP1)
check_group(ALT_GROUP1)
+
+def test_multiple_files_created_after_startup(add_user_with_canary,
+ add_group_with_canary,
+ files_multiple_sources_nocreate):
+ """
+ Test that users and groups can be mirrored from multiple files,
+ but those files are not created when SSSD starts, only afterwards.
+ """
+ alt_passwd_path, alt_group_path = files_multiple_sources_nocreate
+
+ check_user(USER1)
+ check_group(GROUP1)
+
+ # touch the files
+ for fpath in (alt_passwd_path, alt_group_path):
+ with open(fpath, "w") as f:
+ pass
+
+ alt_pwops = PasswdOps(alt_passwd_path)
+ alt_grops = GroupOps(alt_group_path)
+ alt_pwops.useradd(**ALT_USER1)
+ alt_grops.groupadd(**ALT_GROUP1)
+
+ check_user(ALT_USER1)
+ check_group(ALT_GROUP1)
From 032856c4d608f85c80b3e2862019468375369eb8 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Tue, 17 Apr 2018 12:32:11 +0200
Subject: [PATCH 5/5] FILES: Only send the request for update if the files
domain is inconsistent
Resolves:
https://pagure.io/SSSD/sssd/issue/3520
The code was probably commented out as a mistake..
---
src/responder/common/responder_dp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/responder/common/responder_dp.c b/src/responder/common/responder_dp.c
index 8cc734813..9669b5fee 100644
--- a/src/responder/common/responder_dp.c
+++ b/src/responder/common/responder_dp.c
@@ -598,11 +598,11 @@ static int sss_dp_account_files_params(struct sss_domain_info *dom,
enum sss_dp_acct_type *_type_out,
const char **_opt_name_out)
{
-#if 0
if (sss_domain_get_state(dom) != DOM_INCONSISTENT) {
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ "The entries in the files domain are up-to-date\n");
return EOK;
}
-#endif
DEBUG(SSSDBG_TRACE_INTERNAL,
"Domain files is not consistent, issuing update\n");
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
