On 09/10/2012 10:48 AM, Sumit Bose wrote:
On Mon, Sep 10, 2012 at 09:35:25AM +0200, Pavel Březina wrote:
On 09/07/2012 11:31 AM, Sumit Bose wrote:
On Wed, Sep 05, 2012 at 03:05:49PM +0200, Pavel Březina wrote:
0001 fixes https://fedorahosted.org/sssd/ticket/1458
0002 fixes a memory leak when be_process_init() fails. I think it
should be fixed, even though the backend is in this case immediately
terminated
Hi,
the patches compile without errors and work as expected. Nevertheless I
have a few comments:
0001:
- I think it make sense to skip the initialization of the sudo target
if there is no responder configured. But to avoid surprises on the
user side I would recommend to do the initialization if it is
configured explicitly in sssd.conf, i.e sudo_provider = ldap, and
give a warning in the logs that the corresponding responder will not
be running.
I discussed this with Jakub before writing the patch. sudo_provider
currently defaults to id_provider. If I get it right, you are
suggesting to make the default value "none".
No, sorry for not being clear here. I think the id_provider default is
right. I just wanted to say that if for whatever reasons the
sudo_provider parameter is used explicitly in sssd.conf the
sudo_provider should be initialization even if sudo is not listed in
services.
bye,
Sumit
We agreed with Jakub that this makes more pressure on the user to
understand the configuration so we chose not to do it. Although the
configurations I've been seeing so far are mostly generated by
authconfig or ipa-client-install (where are all providers set
explicitly) so I don't think it is a problem after all. What do you say
Jakub?
If we decide to do it this way, it will also require changes in man page.
+ ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
+ CONFDB_MONITOR_ACTIVE_SERVICES, NULL, &services);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_FATAL_FAILURE,
+ ("Unable to read from confdb [%d]: %s\n", ret, strerror(ret)));
+ return ret;
+ }
+
- I would prefer to use confdb_get_string_as_list() and use strcmp()
later on, to be on the safe side.
0002: ACK
bye,
Sumit
OK, that makes sense. Patches are attached.
I moved the code into separate function.
From 94d868cb427a6073835ef62f28ddabb9ff4745ea Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
Date: Mon, 10 Sep 2012 13:12:55 +0200
Subject: [PATCH 1/2] be_process_init(): free ctx on error
---
src/providers/data_provider_be.c | 36 +++++++++++++++++++++---------------
1 files changed, 21 insertions(+), 15 deletions(-)
diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index dcce69ca42fe4b8f216a69a6877e0aeaf20872cc..b979ccc640c7cedb68e8d19493d04a07d68032af 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -2096,21 +2096,22 @@ int be_process_init(TALLOC_CTX *mem_ctx,
ctx->conf_path = talloc_asprintf(ctx, CONFDB_DOMAIN_PATH_TMPL, be_domain);
if (!ctx->identity || !ctx->conf_path) {
DEBUG(0, ("Out of memory!?\n"));
- return ENOMEM;
+ ret = ENOMEM;
+ goto fail;
}
ret = be_init_failover(ctx);
if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE,
("fatal error initializing failover context\n"));
- return ret;
+ goto fail;
}
ret = sysdb_init_domain_and_sysdb(ctx, cdb, be_domain, DB_PATH,
&ctx->domain, &ctx->sysdb);
if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE, ("fatal error opening cache database\n"));
- return ret;
+ goto fail;
}
ret = sss_monitor_init(ctx, ctx->ev, &monitor_be_interface,
@@ -2119,13 +2120,13 @@ int be_process_init(TALLOC_CTX *mem_ctx,
if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE,
("fatal error setting up monitor bus\n"));
- return ret;
+ goto fail;
}
ret = be_srv_init(ctx);
if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE, ("fatal error setting up server bus\n"));
- return ret;
+ goto fail;
}
ret = load_backend_module(ctx, BET_ID,
@@ -2133,7 +2134,7 @@ int be_process_init(TALLOC_CTX *mem_ctx,
if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE,
("fatal error initializing data providers\n"));
- return ret;
+ goto fail;
}
DEBUG(SSSDBG_TRACE_INTERNAL,
("ID backend target successfully loaded from provider [%s].\n",
@@ -2146,7 +2147,7 @@ int be_process_init(TALLOC_CTX *mem_ctx,
if (ret != ENOENT) {
DEBUG(SSSDBG_FATAL_FAILURE,
("fatal error initializing data providers\n"));
- return ret;
+ goto fail;
}
DEBUG(SSSDBG_MINOR_FAILURE,
("No authentication module provided for [%s] !!\n",
@@ -2162,7 +2163,7 @@ int be_process_init(TALLOC_CTX *mem_ctx,
if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE,
("Failed to setup ACCESS backend.\n"));
- return ret;
+ goto fail;
}
DEBUG(SSSDBG_TRACE_INTERNAL,
("ACCESS backend target successfully loaded "
@@ -2175,7 +2176,7 @@ int be_process_init(TALLOC_CTX *mem_ctx,
if (ret != ENOENT) {
DEBUG(SSSDBG_FATAL_FAILURE,
("fatal error initializing data providers\n"));
- return ret;
+ goto fail;
}
DEBUG(SSSDBG_MINOR_FAILURE,
("No change password module provided for [%s] !!\n",
@@ -2193,7 +2194,7 @@ int be_process_init(TALLOC_CTX *mem_ctx,
if (ret != ENOENT) {
DEBUG(SSSDBG_FATAL_FAILURE,
("fatal error initializing data providers\n"));
- return ret;
+ goto fail;
}
DEBUG(SSSDBG_MINOR_FAILURE,
("No SUDO module provided for [%s] !!\n", be_domain));
@@ -2210,7 +2211,7 @@ int be_process_init(TALLOC_CTX *mem_ctx,
if (ret != ENOENT) {
DEBUG(SSSDBG_FATAL_FAILURE,
("fatal error initializing data providers\n"));
- return ret;
+ goto fail;
}
DEBUG(SSSDBG_MINOR_FAILURE,
("No autofs module provided for [%s] !!\n", be_domain));
@@ -2226,7 +2227,7 @@ int be_process_init(TALLOC_CTX *mem_ctx,
if (ret != EOK) {
if (ret != ENOENT) {
DEBUG(SSSDBG_FATAL_FAILURE, ("fatal error initializing data providers\n"));
- return ret;
+ goto fail;
}
DEBUG(SSSDBG_CRIT_FAILURE, ("No selinux module provided for [%s] !!\n",
be_domain));
@@ -2242,7 +2243,7 @@ int be_process_init(TALLOC_CTX *mem_ctx,
if (ret != ENOENT) {
DEBUG(SSSDBG_FATAL_FAILURE,
("fatal error initializing data providers\n"));
- return ret;
+ goto fail;
}
DEBUG(SSSDBG_CRIT_FAILURE,
("No host info module provided for [%s] !!\n", be_domain));
@@ -2267,7 +2268,8 @@ int be_process_init(TALLOC_CTX *mem_ctx,
tes = tevent_add_signal(ctx->ev, ctx, SIGUSR1, 0,
signal_be_offline, ctx);
if (tes == NULL) {
- return EIO;
+ ret = EIO;
+ goto fail;
}
ret = sss_sigchld_init(ctx, ctx->ev, &ctx->sigchld_ctx);
@@ -2275,10 +2277,14 @@ int be_process_init(TALLOC_CTX *mem_ctx,
DEBUG(SSSDBG_FATAL_FAILURE,
("Could not initialize sigchld context: [%s]\n",
strerror(ret)));
- return ret;
+ goto fail;
}
return EOK;
+
+fail:
+ talloc_free(ctx);
+ return ret;
}
int main(int argc, const char *argv[])
--
1.7.6.5
From 8abafe202c458617ce99e969a0fbdd6b43d23a17 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
Date: Mon, 10 Sep 2012 13:41:08 +0200
Subject: [PATCH 2/2] backend: initialize sudo only when it is enabled in
services
https://fedorahosted.org/sssd/ticket/1458
When the responder is disabled and sudo_provider is set explicitly,
a warning is print and the module will be initialized.
---
src/providers/data_provider_be.c | 69 ++++++++++++++++++++++++++++++++++++--
1 files changed, 66 insertions(+), 3 deletions(-)
diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index b979ccc640c7cedb68e8d19493d04a07d68032af..6aa7b0b1ae0b562e4eaa56ab4030edd21cacd350 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -2076,6 +2076,71 @@ static void signal_be_offline(struct tevent_context *ev,
be_mark_offline(ctx);
}
+int be_process_init_sudo(struct be_ctx *be_ctx)
+{
+ TALLOC_CTX *tmp_ctx = NULL;
+ char **services = NULL;
+ char *provider = NULL;
+ bool responder_enabled = false;
+ bool provider_enabled = false;
+ int i;
+ int ret;
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_new() failed\n"));
+ return ENOMEM;
+ }
+
+ ret = confdb_get_string_as_list(be_ctx->cdb, tmp_ctx,
+ CONFDB_MONITOR_CONF_ENTRY,
+ CONFDB_MONITOR_ACTIVE_SERVICES, &services);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_FATAL_FAILURE, ("Unable to read from confdb [%d]: %s\n",
+ ret, strerror(ret)));
+ goto done;
+ }
+
+ for (i = 0; services[i] != NULL; i++) {
+ if (strcmp(services[i], "sudo") == 0) {
+ responder_enabled = true;
+ break;
+ }
+ }
+
+ ret = confdb_get_string(be_ctx->cdb, tmp_ctx, be_ctx->conf_path,
+ CONFDB_DOMAIN_SUDO_PROVIDER, NULL, &provider);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_FATAL_FAILURE, ("Unable to read from confdb [%d]: %s\n",
+ ret, strerror(ret)));
+ goto done;
+ }
+
+ if (!responder_enabled && provider == NULL) {
+ /* provider is not set explicitly */
+ DEBUG(SSSDBG_TRACE_FUNC,
+ ("SUDO is not listed in services, disabling SUDO module.\n"));
+ ret = ENOENT;
+ goto done;
+ }
+
+ if (!responder_enabled && provider != NULL
+ && strcmp(provider, NO_PROVIDER) != 0) {
+ /* provider is set but responder is disabled */
+ DEBUG(SSSDBG_MINOR_FAILURE, ("SUDO provider is set, but it is not "
+ "listed in active services. SUDO support will not work!\n"));
+ ret = EOK;
+ goto done;
+ }
+
+ ret = load_backend_module(be_ctx, BET_SUDO, &be_ctx->bet_info[BET_SUDO],
+ be_ctx->bet_info[BET_ID].mod_name);
+
+done:
+ talloc_free(tmp_ctx);
+ return ret;
+}
+
int be_process_init(TALLOC_CTX *mem_ctx,
const char *be_domain,
struct tevent_context *ev,
@@ -2187,9 +2252,7 @@ int be_process_init(TALLOC_CTX *mem_ctx,
"from provider [%s].\n", ctx->bet_info[BET_CHPASS].mod_name));
}
- ret = load_backend_module(ctx, BET_SUDO,
- &ctx->bet_info[BET_SUDO],
- ctx->bet_info[BET_ID].mod_name);
+ ret = be_process_init_sudo(ctx);
if (ret != EOK) {
if (ret != ENOENT) {
DEBUG(SSSDBG_FATAL_FAILURE,
--
1.7.6.5
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
