Merge lp:~tyhicks/ecryptfs/lp1597154-packaging into lp:ecryptfs

Proposed by Tyler Hicks on 2016-07-08
Status: Merged
Merge reported by: Tyler Hicks
Merged at revision: not available
Proposed branch: lp:~tyhicks/ecryptfs/lp1597154-packaging
Merge into: lp:ecryptfs
Diff against target: 84 lines (+52/-0)
3 files modified
debian/changelog (+6/-0)
debian/ecryptfs-utils.postinst (+42/-0)
src/utils/ecryptfs-setup-swap (+4/-0)
To merge this branch: bzr merge lp:~tyhicks/ecryptfs/lp1597154-packaging
Reviewer Review Type Date Requested Status
Martin Pitt (community) 2016-07-08 Approve on 2016-07-12
Jason Gerard DeRose (community) 2016-07-08 Approve on 2016-07-12
eCryptfs 2016-07-08 Pending
Review via email: mp+299605@code.launchpad.net

Description of the change

Changes to the Debian packaging to automatically fixup existing installations which are affected by the GPT unencrypted swap partitions that are marked as auto mount.

To post a comment you must log in.
Jason Gerard DeRose (jderose) wrote :

Tyler,

Reading through the code, everything looks solid.

I also (synthetically) tested your proposed postinst script using `sudo dash ecryptfs-utils.postinst configure` on an appropriately configured system with an NVMe drive on which I first deliberately unset bit 63, and it worked fine. Your postinst script likewise worked fine when running it again after bit 63 was already set. (Although note that I didn't yet test using a built package with this change, with all the real-word mechanics of the postinst script being called in exact the circumstance that it would be for normal users.)

There are a few small issue I found with https://www.shellcheck.net/ (within the code added by this merge) that should probably be addressed eventually, but nothing that I consider deal breaking.

Finally, thanks for your comment about keeping ecryptfs-utils.postinst and ecryptfs-setup-swap in-sync. A very smart call there, in my opinion.

As soon as there's a package in xenial-proposed, I'll put this change through its paces in a 100% real-world scenario.

-Jason

review: Approve
Martin Pitt (pitti) wrote :

I didn't realize that we didn't have a corresponding postinst snippet for un-marking those as swap partition; I did that in the vivid SRU (http://launchpadlibrarian.net/205003673/ecryptfs-utils_107-0ubuntu1_107-0ubuntu1.1.diff.gz), was that lost during merge?

Either way, thanks for bringing it back, and adjusting it for nvme!

review: Approve
Tyler Hicks (tyhicks) wrote :

Thanks for the reviews.

@Jason I fixed up a couple of the shellcheck warnings before merging.

@Martin Yes, it looks like your postinst snippet for vivid was lost. The way that new upstream eCryptfs releases are cut and uploaded to Ubuntu makes this a possibility for any packaging changes that are not upstreamed to lp:ecryptfs. :/

Dustin Kirkland  (kirkland) wrote :

@pitti, it would be great if, when you upload to Ubuntu, you also
proposal a merge against lp:ecryptfs, so that we don't lose track of
it. I try to stay on top of non-maintainer uploads, and pull those
back upstream, but occasionally these get missed. Thanks!

Dustin Kirkland
Ubuntu Product & Strategy
Canonical, Ltd.

On Tue, Jul 12, 2016 at 11:03 PM, Tyler Hicks <email address hidden> wrote:
> Thanks for the reviews.
>
> @Jason I fixed up a couple of the shellcheck warnings before merging.
>
> @Martin Yes, it looks like your postinst snippet for vivid was lost. The way that new upstream eCryptfs releases are cut and uploaded to Ubuntu makes this a possibility for any packaging changes that are not upstreamed to lp:ecryptfs. :/
> --
> https://code.launchpad.net/~tyhicks/ecryptfs/lp1597154-packaging/+merge/299605
> Your team eCryptfs is requested to review the proposed merge of lp:~tyhicks/ecryptfs/lp1597154-packaging into lp:ecryptfs.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2016-07-06 22:31:21 +0000
3+++ debian/changelog 2016-07-08 21:35:34 +0000
4@@ -18,6 +18,12 @@
5 erroneously prompted to enter a pass-phrase to unlock their swap partition
6 at boot. (LP: #1597154)
7
8+ [ Tyler Hicks ]
9+ * debian/ecryptfs-utils.postinst: Fix any unencrypted GPT swap partitions
10+ that have mistakenly remained marked as auto mount. This should only
11+ modify the swap partitions on systems that ecryptfs-setup-swap has been
12+ used on. (LP: #1447282, LP: #1597154)
13+
14 -- Dustin Kirkland <kirkland@ubuntu.com> Fri, 26 Feb 2016 18:00:18 -0600
15
16 ecryptfs-utils (111) xenial; urgency=medium
17
18=== modified file 'debian/ecryptfs-utils.postinst'
19--- debian/ecryptfs-utils.postinst 2015-08-04 16:29:37 +0000
20+++ debian/ecryptfs-utils.postinst 2016-07-08 21:35:34 +0000
21@@ -66,6 +66,48 @@
22 done
23 done < /etc/crypttab
24 fi
25+
26+ # Fix up GPT unencrypted swap partitions that have not been
27+ # marked as no-auto mounting in order to prevent systemd from
28+ # using them before the encrypted swap can be initialized
29+ # (LP: #1447282, LP: #1597154).
30+ #
31+ # IMPORTANT: Much of this code is duplicated from
32+ # ecryptfs-setup-swap. Please keep the two in sync when making
33+ # any changes.
34+ if [ -e /etc/crypttab ] && [ -e /etc/fstab ]; then
35+ while read mapper_dev phys_dev keyfile options; do
36+ # only consider cryptswapN devices from ecryptfs-setup-swap
37+ [ "$mapper_dev" != "${mapper_dev#cryptswap}" ] || continue
38+ [ "${options#*swap,}" != "$options" ] || continue
39+ # ignore devices without offset=, they would cause #953875 again
40+ [ "${options%offset=*}" != "$options" ] || continue
41+ # get/verify UUID
42+ uuid="${phys_dev#UUID=}"
43+ [ -e /dev/disk/by-uuid/$uuid ] || continue
44+
45+ # If this is a GPT partition, mark it as no-auto mounting, to avoid
46+ # auto-activating it on boot
47+ swap_dev=$(blkid -U $uuid)
48+ if [ "$(blkid -p -s PART_ENTRY_SCHEME -o value "$swap_dev")" = "gpt" ]; then
49+ # Correctly handle NVMe/MMC drives, as well as any similar physical
50+ # block device that follow the "/dev/foo0p1" pattern (LP: #1597154)
51+ if echo "$swap_dev" | grep -qE "^/dev/.+[0-9]+p[0-9]+$"; then
52+ drive=$(echo "$swap_dev" | sed "s:\(.\+[0-9]\)p[0-9]\+:\1:")
53+ else
54+ drive=$(echo "$swap_dev" | sed "s:\(.\+[^0-9]\)[0-9]\+:\1:")
55+ fi
56+ partno=$(echo "$swap_dev" | sed "s:.\+[^0-9]\([0-9]\+\):\1:")
57+ if [ -b "$drive" ] && \
58+ ! printf "x\np\n" | fdisk "$drive" | grep -q "^$swap_dev .* GUID:.*\b63\b"; then
59+ # toggle flag 63 ("no auto")
60+ echo "Marking GPT swap partition $swap_dev as no-auto ..."
61+ # unfortunately fdisk fails on "cannot re-read part table" and is very verbose
62+ printf "x\nS\n$partno\n63\nr\nw\n" | fdisk "$drive" >/dev/null 2>&1 || true
63+ fi
64+ fi
65+ done < /etc/crypttab
66+ fi
67 ;;
68 abort-upgrade|abort-remove|abort-deconfigure)
69
70
71=== modified file 'src/utils/ecryptfs-setup-swap'
72--- src/utils/ecryptfs-setup-swap 2016-07-06 22:31:21 +0000
73+++ src/utils/ecryptfs-setup-swap 2016-07-08 21:35:34 +0000
74@@ -165,6 +165,10 @@
75
76 # If this is a GPT partition, mark it as no-auto mounting, to avoid
77 # auto-activating it on boot
78+ #
79+ # IMPORTANT: Much of this code is duplicated in
80+ # debian/ecryptfs-utils.postinst. Please keep the two in sync when
81+ # making any changes.
82 if [ "$(blkid -p -s PART_ENTRY_SCHEME -o value "$swap")" = "gpt" ]; then
83 # Correctly handle NVMe/MMC drives, as well as any similar physical
84 # block device that follow the "/dev/foo0p1" pattern (LP: #1597154)

Subscribers

People subscribed via source and target branches