[SSSD] Append PID to sbus server socket name, let clients use a symlink

Jakub Hrozek jhrozek at redhat.com
Sun Oct 9 10:32:28 UTC 2011


[PATCH 1/2] Add option to follow symlinks to check_file()
This functionality will be used later on

[PATCH 2/2] Append PID to sbus server socket name, let clients use a symlink
Only the Data Provider uses a symlink now -- I don't think it makes
sense for monitor, it cannot be restarted anyway. The proxy provider
spawns the proxy_child processes, so if it goes away, so do the
children.

Related question -- the clients now only reconnect when the dbus connection
goes away. Would it make sense to explicitly instruct them to reconnect when
monitor kills the server or perhaps when a new server is registered? This
might help the clients reconnect more seamlessly when the server does not
go away after receiving the shutdown signal.
-------------- next part --------------
From ac8a65a421e1840b2f0a3ccc72564655d0c302ec Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Wed, 5 Oct 2011 17:52:06 +0200
Subject: [PATCH 1/2] Add option to follow symlinks to check_file()

---
 src/monitor/monitor.c            |    2 +-
 src/sbus/sbus_client.c           |    2 +-
 src/sbus/sssd_dbus_server.c      |    2 +-
 src/tests/check_and_open-tests.c |   29 ++++++++++++++++++++++++++++-
 src/util/check_and_open.c        |    5 +++--
 src/util/util.h                  |    2 +-
 6 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index 2d54748..18b5b34 100644
--- a/src/monitor/monitor.c
+++ b/src/monitor/monitor.c
@@ -2473,7 +2473,7 @@ int main(int argc, const char *argv[])
     }
 
     /* Warn if nscd seems to be running */
-    ret = check_file(NSCD_SOCKET_PATH, -1, -1, -1, CHECK_SOCK, NULL);
+    ret = check_file(NSCD_SOCKET_PATH, -1, -1, -1, CHECK_SOCK, NULL, false);
     if (ret == EOK) {
         sss_log(SSS_LOG_NOTICE,
                 "nscd socket was detected.  Nscd caching capabilities "
diff --git a/src/sbus/sbus_client.c b/src/sbus/sbus_client.c
index 1c5c1b2..5b15ef3 100644
--- a/src/sbus/sbus_client.c
+++ b/src/sbus/sbus_client.c
@@ -46,7 +46,7 @@ int sbus_client_init(TALLOC_CTX *mem_ctx,
         return EIO;
     }
 
-    ret = check_file(filename, 0, 0, 0600, CHECK_SOCK, NULL);
+    ret = check_file(filename, 0, 0, 0600, CHECK_SOCK, NULL, false);
     if (ret != EOK) {
         DEBUG(1, ("check_file failed for [%s].\n", filename));
         return EIO;
diff --git a/src/sbus/sssd_dbus_server.c b/src/sbus/sssd_dbus_server.c
index 1bb58d4..fdcbc4f 100644
--- a/src/sbus/sssd_dbus_server.c
+++ b/src/sbus/sssd_dbus_server.c
@@ -120,7 +120,7 @@ int sbus_new_server(TALLOC_CTX *mem_ctx,
         return EIO;
     }
 
-    ret = check_file(filename, 0, 0, -1, CHECK_SOCK, &stat_buf);
+    ret = check_file(filename, 0, 0, -1, CHECK_SOCK, &stat_buf, false);
     if (ret != EOK) {
         DEBUG(1, ("check_file failed for [%s].\n", filename));
         return EIO;
diff --git a/src/tests/check_and_open-tests.c b/src/tests/check_and_open-tests.c
index 59879cc..8e1bc98 100644
--- a/src/tests/check_and_open-tests.c
+++ b/src/tests/check_and_open-tests.c
@@ -100,7 +100,7 @@ START_TEST(test_symlink)
     ret = symlink(filename, newpath);
     fail_unless(ret == 0, "symlink failed [%d][%s]", ret, strerror(errno));
 
-    ret = check_file(newpath, uid, gid, mode, CHECK_REG, NULL);
+    ret = check_file(newpath, uid, gid, mode, CHECK_REG, NULL, false);
     unlink(newpath);
 
     fail_unless(ret == EINVAL,
@@ -108,6 +108,32 @@ START_TEST(test_symlink)
 }
 END_TEST
 
+START_TEST(test_follow_symlink)
+{
+    int ret;
+    char *newpath;
+    size_t newpath_length;
+
+    newpath_length = strlen(filename) + strlen(SUFFIX) + 1;
+    newpath = malloc((newpath_length) * sizeof(char));
+    fail_unless(newpath != NULL, "malloc failed");
+
+    ret = snprintf(newpath, newpath_length, "%s%s", filename, SUFFIX);
+    fail_unless(ret == newpath_length - 1,
+                "snprintf failed: expected [%d] got [%d]", newpath_length -1,
+                                                           ret);
+
+    ret = symlink(filename, newpath);
+    fail_unless(ret == 0, "symlink failed [%d][%s]", ret, strerror(errno));
+
+    ret = check_file(newpath, uid, gid, mode, CHECK_REG, NULL, true);
+    unlink(newpath);
+
+    fail_unless(ret == EOK,
+                "check_and_open_readonly failed on symlink with follow=true");
+}
+END_TEST
+
 START_TEST(test_not_regular_file)
 {
     int ret;
@@ -196,6 +222,7 @@ Suite *check_and_open_suite (void)
     tcase_add_test (tc_check_and_open_readonly, test_wrong_filename);
     tcase_add_test (tc_check_and_open_readonly, test_not_regular_file);
     tcase_add_test (tc_check_and_open_readonly, test_symlink);
+    tcase_add_test (tc_check_and_open_readonly, test_follow_symlink);
     tcase_add_test (tc_check_and_open_readonly, test_wrong_uid);
     tcase_add_test (tc_check_and_open_readonly, test_wrong_gid);
     tcase_add_test (tc_check_and_open_readonly, test_wrong_permission);
diff --git a/src/util/check_and_open.c b/src/util/check_and_open.c
index db926f1..27ef1c8 100644
--- a/src/util/check_and_open.c
+++ b/src/util/check_and_open.c
@@ -35,7 +35,7 @@ static errno_t perform_checks(struct stat *stat_buf,
 
 errno_t check_file(const char *filename, const int uid, const int gid,
                    const int mode, enum check_file_type type,
-                   struct stat *caller_stat_buf)
+                   struct stat *caller_stat_buf, bool follow_symlink)
 {
     int ret;
     struct stat local_stat_buf;
@@ -47,7 +47,8 @@ errno_t check_file(const char *filename, const int uid, const int gid,
         stat_buf = caller_stat_buf;
     }
 
-    ret = lstat(filename, stat_buf);
+    ret = follow_symlink ? stat(filename, stat_buf) : \
+                           lstat(filename, stat_buf);
     if (ret == -1) {
         DEBUG(1, ("lstat for [%s] failed: [%d][%s].\n", filename, errno,
                                                         strerror(errno)));
diff --git a/src/util/util.h b/src/util/util.h
index 6cc8a54..9a00647 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -429,7 +429,7 @@ enum check_file_type {
  */
 errno_t check_file(const char *filename, const int uid, const int gid,
                    const int mode, enum check_file_type type,
-                   struct stat *caller_stat_buf);
+                   struct stat *caller_stat_buf, bool follow_symlink);
 
 /* check_fd()
  * Verify that an open file descriptor has certain permissions and/or
-- 
1.7.6.4

-------------- next part --------------
From 9213c24fc3069895f5f792059da0f471a3f3b799 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Thu, 6 Oct 2011 18:53:51 +0200
Subject: [PATCH 2/2] Append PID to sbus server socket name, let clients use a
 symlink

https://fedorahosted.org/sssd/ticket/1034
---
 src/monitor/monitor.c            |    2 +-
 src/providers/data_provider_be.c |    2 +-
 src/providers/proxy/proxy_init.c |    2 +-
 src/sbus/sbus_client.c           |    2 +-
 src/sbus/sssd_dbus.h             |    1 +
 src/sbus/sssd_dbus_private.h     |    1 +
 src/sbus/sssd_dbus_server.c      |  186 ++++++++++++++++++++++++++++++++++----
 7 files changed, 175 insertions(+), 21 deletions(-)

diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index 18b5b34..9fb277e 100644
--- a/src/monitor/monitor.c
+++ b/src/monitor/monitor.c
@@ -462,7 +462,7 @@ static int monitor_dbus_init(struct mt_ctx *ctx)
 
     ret = sbus_new_server(ctx, ctx->ev,
                           monitor_address, &monitor_server_interface,
-                          &ctx->sbus_srv, monitor_service_init, ctx);
+                          false, &ctx->sbus_srv, monitor_service_init, ctx);
 
     talloc_free(monitor_address);
 
diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index da80f13..6557f9b 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -860,7 +860,7 @@ static int be_srv_init(struct be_ctx *ctx)
     }
 
     ret = sbus_new_server(ctx, ctx->ev, sbus_address,
-                          &be_interface, &ctx->sbus_srv,
+                          &be_interface, true, &ctx->sbus_srv,
                           be_client_init, ctx);
     if (ret != EOK) {
         DEBUG(0, ("Could not set up sbus server.\n"));
diff --git a/src/providers/proxy/proxy_init.c b/src/providers/proxy/proxy_init.c
index d281dea..f53d6de 100644
--- a/src/providers/proxy/proxy_init.c
+++ b/src/providers/proxy/proxy_init.c
@@ -465,7 +465,7 @@ int sssm_proxy_auth_init(struct be_ctx *bectx,
     }
 
     ret = sbus_new_server(ctx, bectx->ev, sbus_address, &proxy_interface,
-                          &ctx->sbus_srv, proxy_client_init, ctx);
+                          false, &ctx->sbus_srv, proxy_client_init, ctx);
     if (ret != EOK) {
         DEBUG(0, ("Could not set up sbus server.\n"));
         goto done;
diff --git a/src/sbus/sbus_client.c b/src/sbus/sbus_client.c
index 5b15ef3..a06b28d 100644
--- a/src/sbus/sbus_client.c
+++ b/src/sbus/sbus_client.c
@@ -46,7 +46,7 @@ int sbus_client_init(TALLOC_CTX *mem_ctx,
         return EIO;
     }
 
-    ret = check_file(filename, 0, 0, 0600, CHECK_SOCK, NULL, false);
+    ret = check_file(filename, 0, 0, 0600, CHECK_SOCK, NULL, true);
     if (ret != EOK) {
         DEBUG(1, ("check_file failed for [%s].\n", filename));
         return EIO;
diff --git a/src/sbus/sssd_dbus.h b/src/sbus/sssd_dbus.h
index 2dbf4ab..98159a7 100644
--- a/src/sbus/sssd_dbus.h
+++ b/src/sbus/sssd_dbus.h
@@ -79,6 +79,7 @@ int sbus_new_server(TALLOC_CTX *mem_ctx,
                     struct tevent_context *ev,
                     const char *address,
                     struct sbus_interface *intf,
+                    bool use_symlink,
                     struct sbus_connection **server,
                     sbus_server_conn_init_fn init_fn, void *init_pvt_data);
 
diff --git a/src/sbus/sssd_dbus_private.h b/src/sbus/sssd_dbus_private.h
index 78a2fe5..c62e0e3 100644
--- a/src/sbus/sssd_dbus_private.h
+++ b/src/sbus/sssd_dbus_private.h
@@ -58,6 +58,7 @@ struct sbus_connection {
     void *reconnect_pvt;
 
     /* server related stuff */
+    char *symlink;
     struct sbus_interface *server_intf;
     sbus_server_conn_init_fn srv_init_fn;
     void *srv_init_data;
diff --git a/src/sbus/sssd_dbus_server.c b/src/sbus/sssd_dbus_server.c
index fdcbc4f..759eb69 100644
--- a/src/sbus/sssd_dbus_server.c
+++ b/src/sbus/sssd_dbus_server.c
@@ -81,6 +81,97 @@ static void sbus_server_init_new_connection(DBusServer *dbus_server,
     }
 }
 
+const char *
+get_socket_address(TALLOC_CTX *mem_ctx, const char *address, bool use_symlink)
+{
+    if (!use_symlink) {
+        return talloc_strdup(mem_ctx, address);
+    }
+
+    return talloc_asprintf(mem_ctx,
+                           "%s.%lu", address, (unsigned long) getpid());
+}
+
+static errno_t
+create_socket_symlink(const char *filename, const char *symlink_filename)
+{
+    errno_t ret;
+
+    DEBUG(7, ("Symlinking the dbus path %s to a link %s\n",
+              filename, symlink_filename));
+    errno = 0;
+    ret = symlink(filename, symlink_filename);
+    if (ret != 0 && errno == EEXIST) {
+        /* Perhaps cruft after a previous server? */
+        ret = unlink(symlink_filename);
+        if (ret != 0) {
+            DEBUG(1, ("Cannot remove old symlink: [%d][%s].\n",
+                      ret, strerror(ret)));
+            return EIO;
+        }
+        errno = 0;
+        ret = symlink(filename, symlink_filename);
+    }
+
+    if (ret != 0) {
+        ret = errno;
+        DEBUG(1, ("symlink() failed on file '%s': [%d][%s].\n",
+                  filename, ret, strerror(ret)));
+        return EIO;
+    }
+
+    return EOK;
+}
+
+static errno_t
+remove_socket_symlink(const char *symlink_name)
+{
+    errno_t ret;
+    char target[PATH_MAX];
+    char pidpath[PATH_MAX];
+    ssize_t numread = 0;
+
+    errno = 0;
+    numread = readlink(symlink_name, target, PATH_MAX);
+    if (numread < 0) {
+        ret = errno;
+        DEBUG(2, ("readlink failed [%d]: %s\n", ret, strerror(ret)));
+        return ret;
+    }
+    target[numread] = '\0';
+    DEBUG(9, ("The symlink points to [%s]\n", target));
+
+    /* We can only remove the symlink if it points to a socket with
+     * the same PID */
+    ret = snprintf(pidpath, PATH_MAX, "%s.%lu",
+                   symlink_name, (unsigned long) getpid());
+    if (ret < 0) {
+        DEBUG(2, ("snprintf failed"));
+        return EIO;
+    } else if (ret >= PATH_MAX) {
+        DEBUG(2, ("path too long?!?!\n"));
+        return EIO;
+    }
+    DEBUG(9, ("The path including our pid is [%s]\n", pidpath));
+
+    if (strcmp(pidpath, target) != 0) {
+        DEBUG(4, ("Will not remove symlink, seems to be owned by "
+                  "another process\n"));
+        return EOK;
+    }
+
+    ret = unlink(symlink_name);
+    if (ret != 0) {
+        ret = errno;
+        DEBUG(2, ("unlink failed to remove [%s] [%d]: %s\n",
+                   symlink, ret, strerror(ret)));
+        return ret;
+    }
+
+    DEBUG(9, ("Removed the symlink\n"));
+    return EOK;
+}
+
 /*
  * dbus_new_server
  * Set up a D-BUS server, integrate with the event loop
@@ -90,8 +181,10 @@ int sbus_new_server(TALLOC_CTX *mem_ctx,
                     struct tevent_context *ev,
                     const char *address,
                     struct sbus_interface *intf,
+                    bool use_symlink,
                     struct sbus_connection **_server,
-                    sbus_server_conn_init_fn init_fn, void *init_pvt_data)
+                    sbus_server_conn_init_fn init_fn,
+                    void *init_pvt_data)
 {
     struct sbus_connection *server;
     DBusServer *dbus_server;
@@ -100,30 +193,64 @@ int sbus_new_server(TALLOC_CTX *mem_ctx,
     char *tmp;
     int ret;
     char *filename;
+    char *symlink_filename = NULL;
+    const char *socket_address;
     struct stat stat_buf;
+    TALLOC_CTX *tmp_ctx;
 
     *_server = NULL;
 
+    tmp_ctx = talloc_new(NULL);
+    if (!tmp_ctx) return ENOMEM;
+
+    socket_address = get_socket_address(tmp_ctx, address, use_symlink);
+    if (!socket_address) {
+        ret = ENOMEM;
+        goto done;
+    }
+
     /* Set up D-BUS server */
     dbus_error_init(&dbus_error);
-    dbus_server = dbus_server_listen(address, &dbus_error);
+    dbus_server = dbus_server_listen(socket_address, &dbus_error);
     if (!dbus_server) {
         DEBUG(1,("dbus_server_listen failed! (name=%s, message=%s)\n",
                  dbus_error.name, dbus_error.message));
         if (dbus_error_is_set(&dbus_error)) dbus_error_free(&dbus_error);
-        return EIO;
+        ret = EIO;
+        goto done;
     }
 
-    filename = strchr(address, '/');
+    filename = strchr(socket_address, '/');
     if (filename == NULL) {
-        DEBUG(1, ("Unexpected dbus address [%s].\n", address));
-        return EIO;
+        DEBUG(1, ("Unexpected dbus address [%s].\n", socket_address));
+        ret = EIO;
+        goto done;
+    }
+
+    if (use_symlink) {
+        symlink_filename = strchr(address, '/');
+        if (symlink_filename == NULL) {
+            DEBUG(1, ("Unexpected dbus address [%s].\n", address));
+            ret = EIO;
+            goto done;
+        }
+
+        ret = create_socket_symlink(filename, symlink_filename);
+        if (ret != EOK) {
+            DEBUG(1, ("Could not create symlink [%d]: %s\n",
+                      ret, strerror(ret)));
+            ret = EIO;
+            goto done;
+        }
     }
 
-    ret = check_file(filename, 0, 0, -1, CHECK_SOCK, &stat_buf, false);
+    /* Both check_file and chmod can handle both the symlink and
+     * the socket */
+    ret = check_file(filename, 0, 0, -1, CHECK_SOCK, &stat_buf, true);
     if (ret != EOK) {
         DEBUG(1, ("check_file failed for [%s].\n", filename));
-        return EIO;
+        ret = EIO;
+        goto done;
     }
 
     if ((stat_buf.st_mode & ~S_IFMT) != 0600) {
@@ -131,7 +258,8 @@ int sbus_new_server(TALLOC_CTX *mem_ctx,
         if (ret != EOK) {
             DEBUG(1, ("chmod failed for [%s]: [%d][%s].\n", filename, errno,
                                                          strerror(errno)));
-            return EIO;
+            ret = EIO;
+            goto done;
         }
     }
 
@@ -139,9 +267,10 @@ int sbus_new_server(TALLOC_CTX *mem_ctx,
     DEBUG(3, ("D-BUS Server listening on %s\n", tmp));
     free(tmp);
 
-    server = talloc_zero(mem_ctx, struct sbus_connection);
+    server = talloc_zero(tmp_ctx, struct sbus_connection);
     if (!server) {
-        return ENOMEM;
+        ret = ENOMEM;
+        goto done;
     }
 
     server->ev = ev;
@@ -153,6 +282,14 @@ int sbus_new_server(TALLOC_CTX *mem_ctx,
 
     talloc_set_destructor((TALLOC_CTX *)server, sbus_server_destructor);
 
+    if (use_symlink) {
+        server->symlink = talloc_strdup(server, symlink_filename);
+        if (!server->symlink) {
+            ret = ENOMEM;
+            goto done;
+        }
+    }
+
     /* Set up D-BUS new connection handler */
     dbus_server_set_new_connection_function(server->dbus.server,
                                             sbus_server_init_new_connection,
@@ -166,8 +303,8 @@ int sbus_new_server(TALLOC_CTX *mem_ctx,
                                             server, NULL);
     if (!dbret) {
         DEBUG(4, ("Error setting up D-BUS server watch functions\n"));
-        talloc_free(server);
-        return EIO;
+        ret = EIO;
+        goto done;
     }
 
     /* Set up DBusTimeout functions */
@@ -180,19 +317,34 @@ int sbus_new_server(TALLOC_CTX *mem_ctx,
         DEBUG(4,("Error setting up D-BUS server timeout functions\n"));
         dbus_server_set_watch_functions(server->dbus.server,
                                         NULL, NULL, NULL, NULL, NULL);
-        talloc_free(server);
-        return EIO;
+        ret = EIO;
+        goto done;
     }
 
-    *_server = server;
-    return EOK;
+    *_server = talloc_steal(mem_ctx, server);
+    ret = EOK;
+done:
+    if (ret != EOK && symlink_filename) {
+        unlink(symlink_filename);
+    }
+    talloc_free(tmp_ctx);
+    return ret;
 }
 
 static int sbus_server_destructor(void *ctx)
 {
     struct sbus_connection *server;
+    errno_t ret;
 
     server = talloc_get_type(ctx, struct sbus_connection);
     dbus_server_disconnect(server->dbus.server);
+
+    if (server->symlink) {
+        ret = remove_socket_symlink(server->symlink);
+        if (ret != EOK) {
+            DEBUG(3, ("Could not remove the server symlink\n"));
+        }
+    }
+
     return 0;
 }
-- 
1.7.6.4



More information about the sssd-devel mailing list