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 Approve
Canonical Server Team 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
1diff --git a/debian/changelog b/debian/changelog
2index b718179..568e3cc 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,12 @@
6+sssd (2.4.0-1ubuntu2) hirsute; urgency=medium
7+
8+ * d/p/0003-Only-start-sssd.service-if-there-s-a-configuration-f.patch:
9+ Upstream patch to make sssd.service only able to start when there
10+ is a configuration file present. (LP: #1900642)
11+ * d/p/condition-path-exists-sssd-conf.patch: Remove.
12+
13+ -- Sergio Durigan Junior <sergio.durigan@canonical.com> Tue, 12 Jan 2021 16:17:38 -0500
14+
15 sssd (2.4.0-1ubuntu1) hirsute; urgency=medium
16
17 * d/p/condition-path-exists-sssd-conf.patch: Only start
18diff --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
19new file mode 100644
20index 0000000..622ddc3
21--- /dev/null
22+++ b/debian/patches/0003-Only-start-sssd.service-if-there-s-a-configuration-f.patch
23@@ -0,0 +1,86 @@
24+From: Sergio Durigan Junior <sergio.durigan@canonical.com>
25+Date: Wed, 9 Dec 2020 22:54:21 -0500
26+Subject: Only start sssd.service if there's a configuration file present
27+
28+This commit is the follow-up of the discussion that is happening here:
29+
30+https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1900642
31+
32+In a nutshell, SSSD is compile with --disable-files-domain and
33+installed without a configuration file by default, which means that
34+it's impossible to start it successfully unless the user has actively
35+created/copied a sssd.conf inside /etc/sssd.
36+
37+There are two possible ways to have sssd.service successfully start:
38+
39+1) If SSSD is configured with --enable-files-domain, then no
40+ configuration file is required, and the service can start normally.
41+
42+2) If SSSD is configured with --disable-files-domain, then a
43+ configuration file is required. This can be either
44+ /etc/sssd/sssd.conf, or a snippet under /etc/sssd/conf.d/.
45+
46+For this reason, I'd like to suggest that we conditionally add the
47+following lines to sssd.service:
48+
49+ ConditionPathExists=|/etc/sssd/sssd.conf
50+ ConditionDirectoryNotEmpty=|/etc/sssd/conf.d/
51+
52+These lines will be added only if SSSD is not configured with
53+--enable-files-domain.
54+
55+Signed-off-by: Sergio Durigan Junior <sergio.durigan@canonical.com>
56+
57+Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
58+
59+Author: Sergio Durigan Junior <sergio.durigan@canonical.com>
60+Origin: upstream, https://github.com/SSSD/sssd/commit/a25256fe22dd0b976093d15a5c7c73e1dc64bbcc
61+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1900642
62+Last-Updated: 2021-01-11
63+---
64+ Makefile.am | 12 +++++++++++-
65+ src/sysv/systemd/sssd.service.in | 1 +
66+ 2 files changed, 12 insertions(+), 1 deletion(-)
67+
68+diff --git a/Makefile.am b/Makefile.am
69+index 4bacabd..1e5c0e8 100644
70+--- a/Makefile.am
71++++ b/Makefile.am
72+@@ -95,6 +95,15 @@ if HAVE_SYSTEMD_UNIT
73+ ifp_exec_cmd = $(sssdlibexecdir)/sssd_ifp --uid 0 --gid 0 --dbus-activated
74+ ifp_systemdservice = SystemdService=sssd-ifp.service
75+ ifp_restart = Restart=on-failure
76++# If sssd is configured with --enable-files-domain, the service is
77++# able to start even without a configuration file. Otherwise, sssd
78++# requires a configuration file (either /etc/sssd/sssd.conf, or some
79++# snippet under /etc/sssd/sssd.conf.d/) to be present.
80++if ADD_FILES_DOMAIN
81++condconfigexists =
82++else
83++condconfigexists = ConditionPathExists=\|/etc/sssd/sssd.conf\nConditionDirectoryNotEmpty=\|/etc/sssd/conf.d/
84++endif
85+ else
86+ ifp_exec_cmd = $(sssdlibexecdir)/sss_signal
87+ ifp_systemdservice =
88+@@ -5128,7 +5137,8 @@ edit_cmd = $(SED) \
89+ -e 's|@libexecdir[@]|$(libexecdir)|g' \
90+ -e 's|@pipepath[@]|$(pipepath)|g' \
91+ -e 's|@prefix[@]|$(prefix)|g' \
92+- -e 's|@SSSD_USER[@]|$(SSSD_USER)|g'
93++ -e 's|@SSSD_USER[@]|$(SSSD_USER)|g' \
94++ -e 's|@condconfigexists[@]|$(condconfigexists)|g'
95+
96+ replace_script = \
97+ @rm -f $@ $@.tmp; \
98+diff --git a/src/sysv/systemd/sssd.service.in b/src/sysv/systemd/sssd.service.in
99+index 7a4b7c7..aae36f7 100644
100+--- a/src/sysv/systemd/sssd.service.in
101++++ b/src/sysv/systemd/sssd.service.in
102+@@ -3,6 +3,7 @@ Description=System Security Services Daemon
103+ # SSSD must be running before we permit user sessions
104+ Before=systemd-user-sessions.service nss-user-lookup.target
105+ Wants=nss-user-lookup.target
106++@condconfigexists@
107+
108+ [Service]
109+ Environment=DEBUG_LOGGER=--logger=files
110diff --git a/debian/patches/condition-path-exists-sssd-conf.patch b/debian/patches/condition-path-exists-sssd-conf.patch
111deleted file mode 100644
112index 7e297c6..0000000
113--- a/debian/patches/condition-path-exists-sssd-conf.patch
114+++ /dev/null
115@@ -1,36 +0,0 @@
116-From: Sergio Durigan Junior <sergio.durigan@canonical.com>
117-Date: Thu, 10 Dec 2020 14:17:09 -0500
118-Subject: Only start sssd.service if there's a configuration file present
119-
120-This commit is the follow-up of the discussion that is happening here:
121-
122-https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1900642
123-
124-In a nutshell, SSSD is installed without a configuration file by
125-default, which means that it's impossible to start it successfully
126-unless the user has actively created/copied a sssd.conf inside
127-/etc/sssd. For this reason, I'd like to suggest that we add
128-"ConditionPathExists=/etc/sssd/sssd.conf" to sssd.service, which
129-mitigates the problem of SSSD not properly starting and generating
130-error messages in the system log.
131-
132-Author: Sergio Durigan Junior <sergio.durigan@canonical.com>
133-Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1900642
134-Forwarded: yes, https://github.com/SSSD/sssd/pull/5433
135-Last-Updated: 2020-12-10
136----
137- src/sysv/systemd/sssd.service.in | 1 +
138- 1 file changed, 1 insertion(+)
139-
140-diff --git a/src/sysv/systemd/sssd.service.in b/src/sysv/systemd/sssd.service.in
141-index 7a4b7c7..4b0fe98 100644
142---- a/src/sysv/systemd/sssd.service.in
143-+++ b/src/sysv/systemd/sssd.service.in
144-@@ -3,6 +3,7 @@ Description=System Security Services Daemon
145- # SSSD must be running before we permit user sessions
146- Before=systemd-user-sessions.service nss-user-lookup.target
147- Wants=nss-user-lookup.target
148-+ConditionPathExists=/etc/sssd/sssd.conf
149-
150- [Service]
151- Environment=DEBUG_LOGGER=--logger=files
152diff --git a/debian/patches/series b/debian/patches/series
153index 18be75c..b8ec7a5 100644
154--- a/debian/patches/series
155+++ b/debian/patches/series
156@@ -1,3 +1,3 @@
157 fix-whitespace-test.diff
158 default-to-socket-activated-services.diff
159-condition-path-exists-sssd-conf.patch
160+0003-Only-start-sssd.service-if-there-s-a-configuration-f.patch

Subscribers

People subscribed via source and target branches