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

Proposed by Sergio Durigan Junior
Status: Merged
Approved by: Sergio Durigan Junior
Approved revision: 948bc31f6574058cd94234d80b561213c03ba654
Merged at revision: 948bc31f6574058cd94234d80b561213c03ba654
Proposed branch: ~sergiodj/ubuntu/+source/sssd:bug1900642-condpathexists-groovy
Merge into: ubuntu/+source/sssd:ubuntu/groovy-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+396186@code.launchpad.net

Description of the change

This is a new MP to address bug 1900642. The difference between it and the previous one (here: https://code.launchpad.net/~sergiodj/ubuntu/+source/sssd/+git/sssd/+merge/395175) is that it incorporates a better, more complete version of the patch I used to fix the issue. In fact, this version of the patch comes directly from upstream, which accepted my solution (after some tweaking).

The previous MP had been approved, and the SRU team had even accepted the package, but I decided to not proceed with the verification process and re-do everything. This is the result.

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.

The good thing is that the SRU template can be left unmodified. I asked Robie how I should proceed in the specific case that a package has already been accepted (but not verified) by the SRU team, and he told me I should bump the version number, so that's what I did. Hopefully everything is correct.

To post a comment you must log in.
Bryce Harrington (bryce) wrote :

Looks like this needs to be addressed in hirsute, prior to the SRUs? That will need-fixing, since the SRU won't be accepted without it landing in hirsute.

Otherwise, the merge proposal looks good, and is a logical refinement over the previously reviewed & approved MP, https://code.launchpad.net/~sergiodj/ubuntu/+source/sssd/+git/sssd/+merge/395175

Approving, contingent on the fix also going into hirsute.

review: Approve
Sergio Durigan Junior (sergiodj) wrote :

Thanks for the review. The hirsute MP has been approved, and the package pushed.

$ git push pkg upload/2.3.1-3ubuntu2
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.95 KiB | 377.00 KiB/s, done.
Total 11 (delta 5), reused 0 (delta 0)
To ssh://git.launchpad.net/ubuntu/+source/sssd
 * [new tag] upload/2.3.1-3ubuntu2 -> upload/2.3.1-3ubuntu2

$ dput sssd_2.3.1-3ubuntu2_source.changes
Trying to upload package to ubuntu
Checking signature on .changes
gpg: /home/sergio/work/sssd/sssd_2.3.1-3ubuntu2_source.changes: Valid signature from 106DA1C8C3CBBF14
Checking signature on .dsc
gpg: /home/sergio/work/sssd/sssd_2.3.1-3ubuntu2.dsc: Valid signature from 106DA1C8C3CBBF14
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading sssd_2.3.1-3ubuntu2.dsc: done.
  Uploading sssd_2.3.1-3ubuntu2.debian.tar.xz: done.
  Uploading sssd_2.3.1-3ubuntu2_source.buildinfo: done.
  Uploading sssd_2.3.1-3ubuntu2_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 b25292d..4402380 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,12 @@
6+sssd (2.3.1-3ubuntu2) groovy; 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> Mon, 11 Jan 2021 14:30:55 -0500
14+
15 sssd (2.3.1-3ubuntu1) groovy; 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