Merge ~bryce/ubuntu/+source/docker.io:sru-lp1870514-docker-dh-hirsute into ubuntu/+source/docker.io:ubuntu/devel

[Bryce Harrington]

This addresses two separate but related issues. First that docker.io and containerd are too tightly coupled, such that reinstalling containerd can trigger docker.io's prerm to stop the service without restarting it, leading to an outage. Second, it disables docker.io's policy to automatically restart on package upgrade. We found both fixes are required for a full solution to the problem.

Correcting this issue will help facilitate release of CVE-2020-15257 to containerd. This docker.io update will be coordinated with that security update.

While this MP targets hirsute, our intent is to roll the fix out to all supported versions of Ubuntu via the security pocket. Thus, please review with SRU-style requirements in mind.

Bugs: LP: #1906364, LP: #1870514
PPA: https://launchpad.net/~bryce/+archive/ubuntu/containerd-sru-lp1870514-docker-dh

[Sergio Durigan Junior (sergiodj)]

Thanks for the MP, Bryce. I've reviewed it and it looks good (modulo a nit about "--no-restart-on-upgrade").

I'm not marking it as Approved yet because I will take a closer look later.

[Bryce Harrington (bryce)]

mwhudson, adding you as reviewer given your history with LP: #1658691. I want to make sure by implementing this change that it doesn't resurrect an issue fixed previously.

[Christian Ehrhardt  (paelzer)]

@Bryce - along the changes I've seen for containerd this looks rather good to me. The one related thing I wondered about is: Due to eventually no more restarting docker on upgrade would that imply that we'd now want to mark "needs reboot" (ok a needs a service restart, but until we have that granularity).

I say that because I can see a world where we correctly fixed the bug of restarting.
But then people might complain that upgrading to e.g. a new CVE fix version will keep them stay affected until a restart.
Flagging "need reboot" or such would at least make them aware.

[Steve Langasek (vorlon)]

The implementation here looks ok to me (mod Sergio's feedback).

Will there be a corresponding change to the containerd package declaring a Breaks: on versions of docker.io that don't implement this, to ensure docker.io is always upgraded to a fixed version first before containerd restarts?

review: Needs Information

[Bryce Harrington (bryce)]

I've added Sergio's feedback and repushed to this MP.

@Steve, yes, a breaks for containerd is a good idea.

@Christian, the approach we're taking seems to tie in with some underlying "service needs restarted" handling code (see https://i.imgur.com/2Za5dbQ.png). It would be worth doublechecking what happens if the user answers 'No', but perhaps it is already marking it as needs rebooting? If not, guess we'd just need to add to the prerm a reboot-needed touch like done in proposal X.

[Lucas Kanashiro (lucaskanashiro)]

I re-ran the test scenarios Sergio described in his email and all good. Thanks for all the work folks.

As Steve requested I submitted this PR adding a Breaks to containerd:

https://github.com/tianon/debian-containerd/pull/14

If everyone is happy we can upload both containerd and docker.io to Hirsute (as soon as Tianon approves my PR).

In our meeting earlier today Bryce mentioned that even with the fix the docker daemon will be restarted one more time, and a possible solution to avoid that would be to create another package which would try to change the prerm maintainer script before the update is performed. @Steve, do you think we need to be concerned about that? Or is the current solution good enough in your opinion?

[Steve Langasek (vorlon)]

review: Approve

[Lucas Kanashiro (lucaskanashiro)]

Uploaded:

$ git push pkg upload/19.03.13-0ubuntu4
Enumerating objects: 21, done.
Counting objects: 100% (21/21), done.
Delta compression using up to 32 threads
Compressing objects: 100% (15/15), done.
Writing objects: 100% (15/15), 3.36 KiB | 1.68 MiB/s, done.
Total 15 (delta 8), reused 0 (delta 0)
To ssh://git.launchpad.net/ubuntu/+source/docker.io
 * [new tag] upload/19.03.13-0ubuntu4 -> upload/19.03.13-0ubuntu4
$ dput ubuntu ../docker.io_19.03.13-0ubuntu4_source.changes
Checking signature on .changes
gpg: ../docker.io_19.03.13-0ubuntu4_source.changes: Valid signature from F823A2729883C97C
Checking signature on .dsc
gpg: ../docker.io_19.03.13-0ubuntu4.dsc: Valid signature from F823A2729883C97C
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading docker.io_19.03.13-0ubuntu4.dsc: done.
  Uploading docker.io_19.03.13-0ubuntu4.debian.tar.xz: done.
  Uploading docker.io_19.03.13-0ubuntu4_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/changelog b/debian/changelog
index 7894341..2d88f98 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,15 @@
+docker.io (19.03.13-0ubuntu4) hirsute; urgency=medium
+
+  * d/p/do_not_bind_docker_to_containerd.patch: Update docker.io to not
+    stop when containerd is upgraded, by using Wants= rather than BindTo=.
+    (LP: #1870514)
+  * d/rules: Fix docker.io to not restart its service during package
+    upgrades, to prevent service downtime from automatic updates via
+    unattended-upgrade.
+    (LP: #1906364)
+
+ -- Bryce Harrington <[e-mail]>  Fri, 04 Dec 2020 23:02:49 +0000
+
 docker.io (19.03.13-0ubuntu3) groovy; urgency=medium
 
   * SECURITY UPDATE: Sensitive information disclosure
diff --git a/debian/patches/do-not-bind-docker-to-containerd.patch b/debian/patches/do-not-bind-docker-to-containerd.patch
new file mode 100644
index 0000000..d2202cc
--- /dev/null
+++ b/debian/patches/do-not-bind-docker-to-containerd.patch
@@ -0,0 +1,64 @@
+From 22f15d4137cb5d090f13fc5d9093dc3085dce67b Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Micha=C5=82=20Kosek?= <[e-mail]>
+Date: Tue, 9 Jul 2019 15:34:13 +0200
+Subject: [PATCH] Do not "Bind" docker "To" containerd.
+
+relates to https://github.com/docker/for-linux/issues/678
+
+When using the BindTo directive, Docker is permanently stopped by systemd
+when containerd is temporarily killed and restarted;
+
+Using `Requires` achieves mostly the same, but defines a weaker dependency;
+
+https://www.freedesktop.org/software/systemd/man/systemd.unit.html#Requires=
+
+> Requires=
+>
+> .. If this unit gets activated, the units listed will be activated as well.
+> If one of the other units fails to activate, and an ordering dependency
+> After= on the failing unit is set, this unit will not be started. Besides,
+> with or without specifying After=, this unit will be stopped if one of the
+> other units is explicitly stopped.
+
+We may want to look into using `Wants=` instead of `Requires=`, because
+that allows docker to continue running if containerd is restarted, quoting
+the systemd documentation:
+
+> Often, it is a better choice to use Wants= instead of Requires= in order
+> to achieve a system that is more robust when dealing with failing services.
+
+Given that docker will likely still fail if the containerd socket is not
+present, startup will fail if containerd is not running, but if containerd
+is restarted, the docker daemon may be able to try reconnecting.
+
+Signed-off-by: Sebastiaan van Stijn <[e-mail]>
+---
+ systemd/docker.service | 3 +--
+ 1 file changed, 1 insertion(+), 2 deletions(-)
+
+Origin: upstream, https://github.com/docker/docker-ce-packaging/pull/508
+Bug: https://github.com/docker/for-linux/issues/678
+Ubuntu-Bug: https://bugs.launchpad.net/ubuntu/+source/containerd/+bug/1870514
+Reviewed-By: Bryce Harrington <[e-mail]>
+Description: This is a backport of upstream's patch, with the path to
+ the docker.service modified and using Wants= rather than Requires=, for
+ reasons outlined above (Requires= did not work).  The After= target
+ also differs from upstream (we don't carry commit 36bb01538.)
+Last-Updated: 2020-12-02
+
+diff --git a/systemd/docker.service b/systemd/docker.service
+index 9c1d9e6d37..0a6a3064a4 100644
+--- a/components/packaging/systemd/docker.service
++++ b/components/packaging/systemd/docker.service
+@@ -1,10 +1,10 @@
+ [Unit]
+ Description=Docker Application Container Engine
+ Documentation=https://docs.docker.com
+-BindsTo=containerd.service
+ After=network-online.target firewalld.service containerd.service
+ Wants=network-online.target
+ Requires=docker.socket
++Wants=containerd.service
+ 
+ [Service]
+ Type=notify
diff --git a/debian/patches/series b/debian/patches/series
index 9dddd87..f9393b5 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1,2 +1,3 @@
 41518--apparmor-parser-beta.patch
 CVE-2020-15157.patch
+do-not-bind-docker-to-containerd.patch
diff --git a/debian/rules b/debian/rules
index 0b10b04..6eab5be 100755
--- a/debian/rules
+++ b/debian/rules
@@ -124,5 +124,8 @@ override_dh_shlibdeps:
 override_dh_auto_clean:
 	@# stop debhelper from doing "make clean"
 
+override_dh_systemd_start:
+	dh_systemd_start --package=docker.io --no-stop-on-upgrade
+
 %:
 	dh $@ --with=bash-completion,systemd