Merge ~sergiodj/ubuntu/+source/sssd:bug1900642-condpathexists-hirsute into ubuntu/+source/sssd:ubuntu/devel

Proposed by Sergio Durigan Junior
Status: Merged
Approved by: Sergio Durigan Junior
Approved revision: 75fe00ec14da927c4928226d843f9df1a5e1729a
Merged at revision: 75fe00ec14da927c4928226d843f9df1a5e1729a
Proposed branch: ~sergiodj/ubuntu/+source/sssd:bug1900642-condpathexists-hirsute
Merge into: ubuntu/+source/sssd:ubuntu/devel
Diff against target: 160 lines (+96/-37)
4 files modified
debian/changelog (+9/-0)
debian/patches/0003-Only-start-sssd.service-if-there-s-a-configuration-f.patch (+86/-0)
debian/patches/series (+1/-1)
dev/null (+0/-36)
Reviewer Review Type Date Requested Status
Bryce Harrington (community) Approve
Canonical Server Pending
Review via email: mp+396206@code.launchpad.net

Description of the change

This is a MP to address bug 1900642 on hirsute. It includes a patch that comes directly from upstream, which accepted my solution (after some tweaking). This MP basically replaces an old approach to solve the problem by a new one, which is more complete.

In a nutshell, what the new patch does is to include some conditions in the sssd.service file that are responsible for checking whether the user as (a) a file named /etc/sssd/sssd.conf, or (b) some configuration snippet under /etc/sssd/conf.d/. If either is true, then the service can be started.

It's important to mention that these conditions will only be added to the service file if sssd has been compiled without --enable-files-domain support. This is true for the Debian/Ubuntu sssd packages.

To post a comment you must log in.
Revision history for this message
Bryce Harrington (bryce) wrote :

Looks good. I reviewed bug report (and SRU template), and past work on this bug. I did not test out the behavior but assume you've tested it thoroughly already. The approach looks correct and I verified the systemd logic against documentation.

review: Approve
Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

Thanks for the review.

$ git push pkg upload/2.4.0-1ubuntu2
Enumerating objects: 16, done.
Counting objects: 100% (16/16), done.
Delta compression using up to 8 threads
Compressing objects: 100% (11/11), done.
Writing objects: 100% (11/11), 2.96 KiB | 378.00 KiB/s, done.
Total 11 (delta 5), reused 0 (delta 0)
To ssh://git.launchpad.net/ubuntu/+source/sssd
 * [new tag] upload/2.4.0-1ubuntu2 -> upload/2.4.0-1ubuntu2

$ dput sssd_2.4.0-1ubuntu2_source.changes
Trying to upload package to ubuntu
Checking signature on .changes
gpg: /home/sergio/work/sssd/sssd_2.4.0-1ubuntu2_source.changes: Valid signature from 106DA1C8C3CBBF14
Checking signature on .dsc
gpg: /home/sergio/work/sssd/sssd_2.4.0-1ubuntu2.dsc: Valid signature from 106DA1C8C3CBBF14
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading sssd_2.4.0-1ubuntu2.dsc: done.
  Uploading sssd_2.4.0-1ubuntu2.debian.tar.xz: done.
  Uploading sssd_2.4.0-1ubuntu2_source.buildinfo: done.
  Uploading sssd_2.4.0-1ubuntu2_source.changes: done.
Successfully uploaded packages.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/changelog b/debian/changelog
index b718179..568e3cc 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,12 @@
1sssd (2.4.0-1ubuntu2) hirsute; urgency=medium
2
3 * d/p/0003-Only-start-sssd.service-if-there-s-a-configuration-f.patch:
4 Upstream patch to make sssd.service only able to start when there
5 is a configuration file present. (LP: #1900642)
6 * d/p/condition-path-exists-sssd-conf.patch: Remove.
7
8 -- Sergio Durigan Junior <sergio.durigan@canonical.com> Tue, 12 Jan 2021 16:17:38 -0500
9
1sssd (2.4.0-1ubuntu1) hirsute; urgency=medium10sssd (2.4.0-1ubuntu1) hirsute; urgency=medium
211
3 * d/p/condition-path-exists-sssd-conf.patch: Only start12 * d/p/condition-path-exists-sssd-conf.patch: Only start
diff --git a/debian/patches/0003-Only-start-sssd.service-if-there-s-a-configuration-f.patch b/debian/patches/0003-Only-start-sssd.service-if-there-s-a-configuration-f.patch
4new file mode 10064413new file mode 100644
index 0000000..622ddc3
--- /dev/null
+++ b/debian/patches/0003-Only-start-sssd.service-if-there-s-a-configuration-f.patch
@@ -0,0 +1,86 @@
1From: Sergio Durigan Junior <sergio.durigan@canonical.com>
2Date: Wed, 9 Dec 2020 22:54:21 -0500
3Subject: Only start sssd.service if there's a configuration file present
4
5This commit is the follow-up of the discussion that is happening here:
6
7https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1900642
8
9In a nutshell, SSSD is compile with --disable-files-domain and
10installed without a configuration file by default, which means that
11it's impossible to start it successfully unless the user has actively
12created/copied a sssd.conf inside /etc/sssd.
13
14There are two possible ways to have sssd.service successfully start:
15
161) If SSSD is configured with --enable-files-domain, then no
17 configuration file is required, and the service can start normally.
18
192) If SSSD is configured with --disable-files-domain, then a
20 configuration file is required. This can be either
21 /etc/sssd/sssd.conf, or a snippet under /etc/sssd/conf.d/.
22
23For this reason, I'd like to suggest that we conditionally add the
24following lines to sssd.service:
25
26 ConditionPathExists=|/etc/sssd/sssd.conf
27 ConditionDirectoryNotEmpty=|/etc/sssd/conf.d/
28
29These lines will be added only if SSSD is not configured with
30--enable-files-domain.
31
32Signed-off-by: Sergio Durigan Junior <sergio.durigan@canonical.com>
33
34Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
35
36Author: Sergio Durigan Junior <sergio.durigan@canonical.com>
37Origin: upstream, https://github.com/SSSD/sssd/commit/a25256fe22dd0b976093d15a5c7c73e1dc64bbcc
38Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1900642
39Last-Updated: 2021-01-11
40---
41 Makefile.am | 12 +++++++++++-
42 src/sysv/systemd/sssd.service.in | 1 +
43 2 files changed, 12 insertions(+), 1 deletion(-)
44
45diff --git a/Makefile.am b/Makefile.am
46index 4bacabd..1e5c0e8 100644
47--- a/Makefile.am
48+++ b/Makefile.am
49@@ -95,6 +95,15 @@ if HAVE_SYSTEMD_UNIT
50 ifp_exec_cmd = $(sssdlibexecdir)/sssd_ifp --uid 0 --gid 0 --dbus-activated
51 ifp_systemdservice = SystemdService=sssd-ifp.service
52 ifp_restart = Restart=on-failure
53+# If sssd is configured with --enable-files-domain, the service is
54+# able to start even without a configuration file. Otherwise, sssd
55+# requires a configuration file (either /etc/sssd/sssd.conf, or some
56+# snippet under /etc/sssd/sssd.conf.d/) to be present.
57+if ADD_FILES_DOMAIN
58+condconfigexists =
59+else
60+condconfigexists = ConditionPathExists=\|/etc/sssd/sssd.conf\nConditionDirectoryNotEmpty=\|/etc/sssd/conf.d/
61+endif
62 else
63 ifp_exec_cmd = $(sssdlibexecdir)/sss_signal
64 ifp_systemdservice =
65@@ -5128,7 +5137,8 @@ edit_cmd = $(SED) \
66 -e 's|@libexecdir[@]|$(libexecdir)|g' \
67 -e 's|@pipepath[@]|$(pipepath)|g' \
68 -e 's|@prefix[@]|$(prefix)|g' \
69- -e 's|@SSSD_USER[@]|$(SSSD_USER)|g'
70+ -e 's|@SSSD_USER[@]|$(SSSD_USER)|g' \
71+ -e 's|@condconfigexists[@]|$(condconfigexists)|g'
72
73 replace_script = \
74 @rm -f $@ $@.tmp; \
75diff --git a/src/sysv/systemd/sssd.service.in b/src/sysv/systemd/sssd.service.in
76index 7a4b7c7..aae36f7 100644
77--- a/src/sysv/systemd/sssd.service.in
78+++ b/src/sysv/systemd/sssd.service.in
79@@ -3,6 +3,7 @@ Description=System Security Services Daemon
80 # SSSD must be running before we permit user sessions
81 Before=systemd-user-sessions.service nss-user-lookup.target
82 Wants=nss-user-lookup.target
83+@condconfigexists@
84
85 [Service]
86 Environment=DEBUG_LOGGER=--logger=files
diff --git a/debian/patches/condition-path-exists-sssd-conf.patch b/debian/patches/condition-path-exists-sssd-conf.patch
0deleted file mode 10064487deleted file mode 100644
index 7e297c6..0000000
--- a/debian/patches/condition-path-exists-sssd-conf.patch
+++ /dev/null
@@ -1,36 +0,0 @@
1From: Sergio Durigan Junior <sergio.durigan@canonical.com>
2Date: Thu, 10 Dec 2020 14:17:09 -0500
3Subject: Only start sssd.service if there's a configuration file present
4
5This commit is the follow-up of the discussion that is happening here:
6
7https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1900642
8
9In a nutshell, SSSD is installed without a configuration file by
10default, which means that it's impossible to start it successfully
11unless the user has actively created/copied a sssd.conf inside
12/etc/sssd. For this reason, I'd like to suggest that we add
13"ConditionPathExists=/etc/sssd/sssd.conf" to sssd.service, which
14mitigates the problem of SSSD not properly starting and generating
15error messages in the system log.
16
17Author: Sergio Durigan Junior <sergio.durigan@canonical.com>
18Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1900642
19Forwarded: yes, https://github.com/SSSD/sssd/pull/5433
20Last-Updated: 2020-12-10
21---
22 src/sysv/systemd/sssd.service.in | 1 +
23 1 file changed, 1 insertion(+)
24
25diff --git a/src/sysv/systemd/sssd.service.in b/src/sysv/systemd/sssd.service.in
26index 7a4b7c7..4b0fe98 100644
27--- a/src/sysv/systemd/sssd.service.in
28+++ b/src/sysv/systemd/sssd.service.in
29@@ -3,6 +3,7 @@ Description=System Security Services Daemon
30 # SSSD must be running before we permit user sessions
31 Before=systemd-user-sessions.service nss-user-lookup.target
32 Wants=nss-user-lookup.target
33+ConditionPathExists=/etc/sssd/sssd.conf
34
35 [Service]
36 Environment=DEBUG_LOGGER=--logger=files
diff --git a/debian/patches/series b/debian/patches/series
index 18be75c..b8ec7a5 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1,3 +1,3 @@
1fix-whitespace-test.diff1fix-whitespace-test.diff
2default-to-socket-activated-services.diff2default-to-socket-activated-services.diff
3condition-path-exists-sssd-conf.patch30003-Only-start-sssd.service-if-there-s-a-configuration-f.patch

Subscribers

People subscribed via source and target branches