URL: https://github.com/SSSD/sssd/pull/142 Author: fidencio Title: #142: Fix breakage caused by having the NSS responder socket-activated Action: opened
PR body: """ **MONITOR: Don't return an error in case we fail to register a service** This patch is a pre-requisite for the next one and both patches could be squashed. Although, I've decided to keep this patch separated as it brings back a behaviour that has been used before the socket-activation series. **MONITOR: Avoid starting an already started service** This patch basically prevents that we end up socket-activating (or systemd enabling) a service that has been explicitly enabled in the services' line. **NSS: Make sssd-nss.service a hard requirement** This patch takes a step back and do not allow the NSS responder to be socket-activated. Instead, it's added as a hard requirement for SSSD (in sssd.service) and will be running whenever SSSD is running. Thankfully to our previous patch we ensure that the only one instance of the responder will be running. So, if nss is not present in the services' line, it will be started as a dep. In case it's part of the services' line, it will be started by the monitor and later on started by systemd. The second instance will be killed during the services' registration part. """ To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/142/head:pr142 git checkout pr142
From fe4543ac1faccc113b8ceb28de6d376113d7c010 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Fri, 3 Feb 2017 18:31:51 +0100 Subject: [PATCH 1/3] MONITOR: Don't return an error in case we fail to register a service MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This behaviour was mistakenly changed by the {dbus,socket}-activation series and, as it's now, I've noticed the monitor may end up in some weird state due to this change, where it doesn't stop properly and leave some defuncts children processes. This patch is needed for the coming one (where I've noticed the weird behaviour) but I've decided to split in a different patch for the clarity's sake. Related: https://fedorahosted.org/sssd/ticket/3298 Signed-off-by: Fabiano FidĂȘncio <fiden...@redhat.com> --- src/monitor/monitor.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c index b82c6e5..d4c8615 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c @@ -363,11 +363,7 @@ static int client_registration(struct sbus_request *dbus_req, void *data) sbus_request_finish(dbus_req, NULL); /* FIXME: should we just talloc_zfree(conn) ? */ - if (ret == ENOENT) { - goto done; - } - - return ret; + goto done; } /* Fill in svc structure with connection data */ From cb9a5f8a666aa189195da0f50dc6530f60269888 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Fri, 3 Feb 2017 16:29:04 +0100 Subject: [PATCH 2/3] MONITOR: Avoid starting an already started service MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This situation can happen by socket-activating a service that has been configured as explicitly-activated. As there's no way, from the service side, to know whether the same service is running, it'll always contact the monitor which will be responsible for not having both services running at the same time. In order to be able to kill the newly started process from the monitor, now the processes send also their pid when registering and in case the service's been already started the monitor sends a SIGTERM to the processes. Related: https://fedorahosted.org/sssd/ticket/3298 Signed-off-by: Fabiano FidĂȘncio <fiden...@redhat.com> --- src/monitor/monitor.c | 15 +++++++++++++++ src/monitor/monitor_interfaces.h | 1 + src/monitor/monitor_sbus.c | 7 +++++-- src/providers/data_provider_be.c | 2 +- src/responder/common/responder_common.c | 2 +- 5 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c index d4c8615..b768e44 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c @@ -292,6 +292,13 @@ static int get_service_in_the_list(struct mon_init_conn *mini, for (svc = mini->ctx->svc_list; svc != NULL; svc = svc->next) { if (strcasecmp(svc->identity, svc_name) == 0) { + if (svc->svc_started) { + DEBUG(SSSDBG_MINOR_FAILURE, + "Service \"%s\" is already running but only " + "one instance of the service is allowed!\n", + svc_name); + return EINVAL; + } svc->socket_activated = false; *_svc = svc; return EOK; @@ -322,6 +329,7 @@ static int client_registration(struct sbus_request *dbus_req, void *data) DBusError dbus_error; dbus_uint16_t svc_ver; dbus_uint16_t svc_type; + dbus_uint32_t svc_pid; char *svc_name; dbus_bool_t dbret; int ret; @@ -341,6 +349,7 @@ static int client_registration(struct sbus_request *dbus_req, void *data) DBUS_TYPE_STRING, &svc_name, DBUS_TYPE_UINT16, &svc_ver, DBUS_TYPE_UINT16, &svc_type, + DBUS_TYPE_UINT32, &svc_pid, DBUS_TYPE_INVALID); if (!dbret) { DEBUG(SSSDBG_CRIT_FAILURE, @@ -363,6 +372,12 @@ static int client_registration(struct sbus_request *dbus_req, void *data) sbus_request_finish(dbus_req, NULL); /* FIXME: should we just talloc_zfree(conn) ? */ + if (ret == EINVAL) { + DEBUG(SSSDBG_MINOR_FAILURE, + "Sending a SIGTERM to the process %d\n", svc_pid); + kill(svc_pid, SIGTERM); + } + goto done; } diff --git a/src/monitor/monitor_interfaces.h b/src/monitor/monitor_interfaces.h index 136beb4..ed0307a 100644 --- a/src/monitor/monitor_interfaces.h +++ b/src/monitor/monitor_interfaces.h @@ -49,6 +49,7 @@ errno_t sss_monitor_init(TALLOC_CTX *mem_ctx, const char *svc_name, uint16_t svc_version, uint16_t svc_type, + uint32_t svc_pid, void *pvt, time_t *last_request_time, struct sbus_connection **mon_conn); diff --git a/src/monitor/monitor_sbus.c b/src/monitor/monitor_sbus.c index faf28f7..702f4dd 100644 --- a/src/monitor/monitor_sbus.c +++ b/src/monitor/monitor_sbus.c @@ -112,7 +112,8 @@ static void id_callback(DBusPendingCall *pending, void *ptr) static int monitor_common_send_id(struct sbus_connection *conn, const char *name, uint16_t version, - uint16_t type) + uint16_t type, + uint32_t pid) { DBusMessage *msg; dbus_bool_t ret; @@ -134,6 +135,7 @@ static int monitor_common_send_id(struct sbus_connection *conn, DBUS_TYPE_STRING, &name, DBUS_TYPE_UINT16, &version, DBUS_TYPE_UINT16, &type, + DBUS_TYPE_UINT32, &pid, DBUS_TYPE_INVALID); if (!ret) { DEBUG(SSSDBG_CRIT_FAILURE, "Failed to build message\n"); @@ -166,6 +168,7 @@ errno_t sss_monitor_init(TALLOC_CTX *mem_ctx, const char *svc_name, uint16_t svc_version, uint16_t svc_type, + uint32_t svc_pid, void *pvt, time_t *last_request_time, struct sbus_connection **mon_conn) @@ -196,7 +199,7 @@ errno_t sss_monitor_init(TALLOC_CTX *mem_ctx, } /* Identify ourselves to the monitor */ - ret = monitor_common_send_id(conn, svc_name, svc_version, svc_type); + ret = monitor_common_send_id(conn, svc_name, svc_version, svc_type, svc_pid); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, "Failed to identify to the monitor!\n"); return ret; diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c index 12b5f43..4c85c54 100644 --- a/src/providers/data_provider_be.c +++ b/src/providers/data_provider_be.c @@ -408,7 +408,7 @@ errno_t be_process_init(TALLOC_CTX *mem_ctx, ret = sss_monitor_init(be_ctx, be_ctx->ev, &monitor_be_methods, be_ctx->identity, DATA_PROVIDER_VERSION, - MT_SVC_PROVIDER, be_ctx, NULL, + MT_SVC_PROVIDER, getpid(), be_ctx, NULL, &be_ctx->mon_conn); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, "Unable to initialize monitor connection\n"); diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c index 67922bf..664994a 100644 --- a/src/responder/common/responder_common.c +++ b/src/responder/common/responder_common.c @@ -1173,7 +1173,7 @@ int sss_process_init(TALLOC_CTX *mem_ctx, ret = sss_monitor_init(rctx, rctx->ev, monitor_intf, svc_name, svc_version, MT_SVC_SERVICE, - rctx, &rctx->last_request_time, + getpid(), rctx, &rctx->last_request_time, &rctx->mon_conn); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, "fatal error setting up message bus\n"); From 862a1326e9940f15a4d082f3233af86629705fe9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Fri, 3 Feb 2017 23:02:43 +0100 Subject: [PATCH 3/3] NSS: Make sssd-nss.service a hard requirement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NSS responder seems to be a little bit problematic for having it socket-activated. Some headaches already happened during the development of the socket-activation series and now seems that having sssd-nss,socket enabled makes some services (like systemd-logind) do not start up, making the boot process to take forecever and making the user end up with a broken system, In order to avoid those issues, let's take a step back and make the sssd-nss.service a hard requirement for sssd.service. Although it won't be socket-activated, it still may be supressed from the services' line. Resolves: https://fedorahosted.org/sssd/ticket/3298 Signed-off-by: Fabiano FidĂȘncio <fiden...@redhat.com> --- Makefile.am | 7 ------- contrib/sssd.spec.in | 4 ---- src/sysv/systemd/sssd-nss.service.in | 6 +----- src/sysv/systemd/sssd-nss.socket.in | 12 ------------ src/sysv/systemd/sssd.service.in | 1 + 5 files changed, 2 insertions(+), 28 deletions(-) delete mode 100644 src/sysv/systemd/sssd-nss.socket.in diff --git a/Makefile.am b/Makefile.am index 674d328..62e9471 100644 --- a/Makefile.am +++ b/Makefile.am @@ -3967,7 +3967,6 @@ systemdconf_DATA = if HAVE_SYSTEMD_UNIT systemdunit_DATA += \ src/sysv/systemd/sssd.service \ - src/sysv/systemd/sssd-nss.socket \ src/sysv/systemd/sssd-nss.service \ src/sysv/systemd/sssd-pam.socket \ src/sysv/systemd/sssd-pam-priv.socket \ @@ -4057,7 +4056,6 @@ replace_script = \ EXTRA_DIST += \ src/sysv/systemd/sssd.service.in \ src/sysv/systemd/journal.conf.in \ - src/sysv/systemd/sssd-nss.socket.in \ src/sysv/systemd/sssd-nss.service.in \ src/sysv/systemd/sssd-pam.socket.in \ src/sysv/systemd/sssd-pam-priv.socket.in \ @@ -4104,10 +4102,6 @@ src/sysv/systemd/journal.conf: src/sysv/systemd/journal.conf.in Makefile @$(MKDIR_P) src/sysv/systemd/ $(replace_script) -src/sysv/systemd/sssd-nss.socket: src/sysv/systemd/sssd-nss.socket.in Makefile - @$(MKDIR_P) src/sysv/systemd/ - $(replace_script) - src/sysv/systemd/sssd-nss.service: src/sysv/systemd/sssd-nss.service.in Makefile @$(MKDIR_P) src/sysv/systemd/ $(replace_script) @@ -4401,7 +4395,6 @@ endif rm -f $(builddir)/src/sysv/systemd/sssd-autofs.socket rm -f $(builddir)/src/sysv/systemd/sssd-autofs.service rm -f $(builddir)/src/sysv/systemd/sssd-ifp.service - rm -f $(builddir)/src/sysv/systemd/sssd-nss.socket rm -f $(builddir)/src/sysv/systemd/sssd-nss.service rm -f $(builddir)/src/sysv/systemd/sssd-pac.socket rm -f $(builddir)/src/sysv/systemd/sssd-pac.service diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index c0c9e36..e690fe5 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -802,7 +802,6 @@ done %{_unitdir}/sssd-autofs.socket %{_unitdir}/sssd-autofs.service %{_unitdir}/sssd-ifp.service -%{_unitdir}/sssd-nss.socket %{_unitdir}/sssd-nss.service %{_unitdir}/sssd-pac.socket %{_unitdir}/sssd-pac.service @@ -1148,7 +1147,6 @@ getent passwd sssd >/dev/null || useradd -r -g sssd -d / -s /sbin/nologin -c "Us %post common %systemd_post sssd.service %systemd_post sssd-autofs.socket -%systemd_post sssd-nss.socket %systemd_post sssd-pac.socket %systemd_post sssd-pam.socket %systemd_post sssd-pam-priv.socket @@ -1159,7 +1157,6 @@ getent passwd sssd >/dev/null || useradd -r -g sssd -d / -s /sbin/nologin -c "Us %preun common %systemd_preun sssd.service %systemd_preun sssd-autofs.socket -%systemd_preun sssd-nss.socket %systemd_preun sssd-pac.socket %systemd_preun sssd-pam.socket %systemd_preun sssd-pam-priv.socket @@ -1171,7 +1168,6 @@ getent passwd sssd >/dev/null || useradd -r -g sssd -d / -s /sbin/nologin -c "Us %systemd_postun_with_restart sssd.service %systemd_postun_with_restart sssd-autofs.socket %systemd_postun_with_restart sssd-autofs.service -%systemd_postun_with_restart sssd-nss.socket %systemd_postun_with_restart sssd-nss.service %systemd_postun_with_restart sssd-pac.socket %systemd_postun_with_restart sssd-pac.service diff --git a/src/sysv/systemd/sssd-nss.service.in b/src/sysv/systemd/sssd-nss.service.in index 2e6fc79..3596b7b 100644 --- a/src/sysv/systemd/sssd-nss.service.in +++ b/src/sysv/systemd/sssd-nss.service.in @@ -4,10 +4,6 @@ Documentation=man:sssd.conf(5) After=sssd.service BindsTo=sssd.service -[Install] -Also=sssd-nss.socket - [Service] -ExecStartPre=-/bin/chown root:root @logpath@/sssd_nss.log -ExecStart=@libexecdir@/sssd/sssd_nss --debug-to-files --socket-activated +ExecStart=@libexecdir@/sssd/sssd_nss --uid 0 --gid 0 --debug-to-files Restart=on-failure diff --git a/src/sysv/systemd/sssd-nss.socket.in b/src/sysv/systemd/sssd-nss.socket.in deleted file mode 100644 index 530fa0c..0000000 --- a/src/sysv/systemd/sssd-nss.socket.in +++ /dev/null @@ -1,12 +0,0 @@ -[Unit] -Description=SSSD NSS Service responder socket -Documentation=man:sssd.conf(5) -BindsTo=sssd.service - -[Socket] -ListenStream=@pipepath@/nss -SocketUser=@SSSD_USER@ -SocketGroup=@SSSD_USER@ - -[Install] -WantedBy=sssd.service diff --git a/src/sysv/systemd/sssd.service.in b/src/sysv/systemd/sssd.service.in index 05cfd37..5632019 100644 --- a/src/sysv/systemd/sssd.service.in +++ b/src/sysv/systemd/sssd.service.in @@ -3,6 +3,7 @@ Description=System Security Services Daemon # SSSD must be running before we permit user sessions Before=systemd-user-sessions.service nss-user-lookup.target Wants=nss-user-lookup.target +Requires=sssd-nss.service [Service] EnvironmentFile=-@environment_file@
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org