Date: Mon, 09 Aug 2021 11:07:20 +0000
To: Krystian =?UTF-8?Q?Chachu=C5=82a?= <krystian@krystianch.com>
Cc: ~alpine/devel@lists.alpinelinux.org, Drew DeVault <sir@cmpwn.com>
Subject: Re: [PATCH v7] Support encrypted root in setup-disk
From: Alex Denes <caskd@redxen.eu>
References: <20210809054945.31750-1-krystian@krystianch.com>
In-Reply-To: <20210809054945.31750-1-krystian@krystianch.com>
Message-Id: <23ERVP4T61ES0.38UZHUML8X76T@unix.is.love.unix.is.life>
User-Agent: mblaze/20210212-175-g8dd82197f3
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="----_=_52d76788307470761ac3f93c_=_"

This is a multipart message in MIME format.

------_=_52d76788307470761ac3f93c_=_
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----_=_7a5e82c90753041b20607855_=_"

This is a multipart message in MIME format.

------_=_7a5e82c90753041b20607855_=_
Content-Type: text/plain; charset=UTF-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hello,

Thanks for the patch and contribution. I've added my feedback to everything=
 below.

> I have tested the script with syslinux (no EFI). Now there is a question
> about making it work with GRUB (EFI). This would require:
>=20
> 1. changing luksFormat type to luks1 as GRUB2 does not support luks2;
GRUB has recently added support for luks2 but the mkimage wrapper doesn't i=
nclude the required modules so the support is not as automated as it should=
 be.
I would instead go for a encrypted root with a unencrypted /boot to avoid u=
sing luks1 for everything and it would simplify the setup. This is not "ful=
l-disk" encryption as one would want but it doesn't have the limitation tha=
t luks1 has.

"LUKS provides a generic key store on the dedicated area on a disk, with th=
e ability to use multiple passphrases to unlock a stored key. LUKS2 extends=
 this concept for more flexible ways of storing metadata, redundant informa=
tion to provide recovery in the case of corruption in a metadata area, and =
an interface to store externally managed metadata for integration with othe=
r tools."

https://gitlab.com/cryptsetup/cryptsetup/blob/master/docs/on-disk-format-lu=
ks2.pdf
=20
> 2. adding a workaround in order for grub-install to succeed (it is not
> called from a chroot so it complains about GRUB_ENABLE_CRYPTODISK=3Dy
> not being present in /etc/default/grub, we can temporarily replace
> /etc/default/grub with "$mnt"/etc/default/grub);
A temporary symlink could work for this instead of making a full copy. Alte=
rnatively, a change of root would work as well but that requires setting up=
 the chroot so that grub-install has the required paths.

I have also added some feedback to the patch:
>  setup-disk.in | 120 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 96 insertions(+), 24 deletions(-)
>=20
> diff --git a/setup-disk.in b/setup-disk.in
> index 656b5bc..e617101 100644
> --- a/setup-disk.in
> +++ b/setup-disk.in
> @@ -82,6 +82,26 @@ enumerate_fstab() {
>  		done
>  }
> =20
> +# given an fstab on stdin, determine if any of the mountpoints are encry=
pted
> +crypt_required() {
> +	while read devname mountpoint fstype mntops freq passno; do
I would use the '-r' flag for read as fstab has no escapes.

> +		if [ -z "$devname" ] || [ "${devname###}" !=3D "$devname" ]; then
> +			continue
> +		fi
> +		uuid=3D"${devname##UUID=3D}"
> +		if [ "$uuid" !=3D "$devname" ]; then
> +			devname=3D"$(blkid --uuid "$uuid")"
> +		fi
> +		mapname=3D"${devname##/dev/mapper/}"
> +		if [ "$mapname" !=3D "$devname" ]; then
> +			if cryptsetup status "$mapname" >&1 >/dev/null; then
> +				return 0
> +			fi
> +		fi
> +	done
> +	return 1
> +}
> +
>  is_vmware() {
>  	grep -q VMware /proc/scsi/scsi 2>/dev/null \
>  		|| grep -q VMware /proc/ide/hd*/model 2>/dev/null
> @@ -427,7 +447,7 @@ setup_raspberrypi_bootloader() {
>  install_mounted_root() {
>  	local mnt=3D"$1"
>  	shift 1
> -	local disks=3D"${@}" mnt_boot=3D boot_fs=3D root_fs=3D
> +	local disks=3D"${@}" mnt_boot=3D boot_fs=3D root_fs=3D use_crypt=3D
>  	local initfs_features=3D"ata base ide scsi usb virtio"
>  	local pvs=3D dev=3D rootdev=3D bootdev=3D extlinux_raidopt=3D root=3D m=
odules=3D
>  	local kernel_opts=3D"$KERNELOPTS"
> @@ -490,7 +510,6 @@ install_mounted_root() {
>  		esac
>  	done
> =20
> -
>  	if [ -n "$VERBOSE" ]; then
>  		echo "Root device:     $rootdev"
>  		echo "Root filesystem: $root_fs"
> @@ -513,6 +532,28 @@ install_mounted_root() {
>  	# we should not try start modloop on sys install
>  	rm -f "$mnt"/etc/runlevels/*/modloop
> =20
> +	# generate the fstab
> +	if [ -f "$mnt"/etc/fstab ]; then
> +		mv "$mnt"/etc/fstab "$mnt"/etc/fstab.old
> +	fi
> +	enumerate_fstab "$mnt" >> "$mnt"/etc/fstab
> +	if [ -n "$SWAP_DEVICES" ]; then
> +		local swap_dev
> +		for swap_dev in $SWAP_DEVICES; do
> +			echo -e "$(uuid_or_device ${swap_dev})\tswap\tswap\tdefaults\t0 0" \
> +				>> "$mnt"/etc/fstab
> +		done
> +	fi
> +	cat >>"$mnt"/etc/fstab <<-__EOF__
> +		/dev/cdrom	/media/cdrom	iso9660	noauto,ro 0 0
> +		/dev/usbdisk	/media/usb	vfat	noauto	0 0
> +	__EOF__
> +
> +	if crypt_required <"$mnt"/etc/fstab; then
> +		use_crypt=3D1
> +		initfs_features=3D"${initfs_features% cryptsetup} cryptsetup"
Isn't this the only place where cryptsetup can be added to the initfs_featu=
res? Wouldn't this duplicate the feature if it wasn't the last appended?

> +	fi
> +
>  	# generate mkinitfs.conf
>  	mkdir -p "$mnt"/etc/mkinitfs/features.d
>  	echo "features=3D\"$initfs_features\"" > "$mnt"/etc/mkinitfs/mkinitfs.c=
onf
> @@ -530,24 +571,14 @@ install_mounted_root() {
>  	if [ -n "$(get_bootopt nomodeset)" ]; then
>  		kernel_opts=3D"nomodeset $kernel_opts"
>  	fi
> +	if [ "$use_crypt" ] && cryptsetup status "$rootdev" 2>&1 >/dev/null; th=
en
> +		# Boot to encrypted root
> +		root=3D$(cryptsetup status "$rootdev" | awk '/device:/ { print $2 }')
> +		kernel_opts=3D"cryptroot=3D$root cryptdm=3Droot"
cryptsetup returns devices without unique identifiers which means this migh=
t fail when the order of devices found by the kernel changes.

> +		root=3D/dev/mapper/root
> +	fi
>  	modules=3D"sd-mod,usb-storage,${root_fs}${raidmod}"
> =20
> -	# generate the fstab
> -	if [ -f "$mnt"/etc/fstab ]; then
> -		mv "$mnt"/etc/fstab "$mnt"/etc/fstab.old
> -	fi
> -	enumerate_fstab "$mnt" >> "$mnt"/etc/fstab
> -	if [ -n "$SWAP_DEVICES" ]; then
> -		local swap_dev
> -		for swap_dev in $SWAP_DEVICES; do
> -			echo -e "$(uuid_or_device ${swap_dev})\tswap\tswap\tdefaults\t0 0" \
> -				>> "$mnt"/etc/fstab
> -		done
> -	fi
> -	cat >>"$mnt"/etc/fstab <<-__EOF__
> -		/dev/cdrom	/media/cdrom	iso9660	noauto,ro 0 0
> -		/dev/usbdisk	/media/usb	vfat	noauto	0 0
> -	__EOF__
>  	# remove the installed db in case its there so we force re-install
>  	rm -f "$mnt"/var/lib/apk/installed "$mnt"/lib/apk/db/installed
>  	echo "Installing system on $rootdev:"
> @@ -595,6 +626,10 @@ unmount_partitions() {
> =20
>  	# unmount the partitions
>  	umount $(awk '{print $2}' /proc/mounts | egrep "^$mnt(/|\$)" | sort -r)=

> +
> +	if [ "$USE_CRYPT" ]; then
> +		cryptsetup close /dev/mapper/root
> +	fi
>  }
> =20
>  # figure out decent default swap size in mega bytes
> @@ -697,9 +732,10 @@ select_bootloader_pkg() {
> =20
>  # install needed programs
>  init_progs() {
> -	local raidpkg=3D lvmpkg=3D fs=3D fstools=3D grub=3D
> +	local raidpkg=3D lvmpkg=3D cryptpkg=3D fs=3D fstools=3D grub=3D
>  	[ -n "$USE_RAID" ] && raidpkg=3D"mdadm"
>  	[ -n "$USE_LVM" ] && lvmpkg=3D"lvm2"
> +	[ -n "$USE_CRYPT" ] && cryptpkg=3D"cryptsetup"
>  	for fs in $BOOTFS $ROOTFS $VARFS; do
>  		# we need load btrfs module early to avoid the error message:
>  		# 'failed to open /dev/btrfs-control'
> @@ -714,7 +750,7 @@ init_progs() {
>  		vfat) fstools=3D"$fstools dosfstools";;
>  		esac
>  	done
> -	apk add --quiet sfdisk $lvmpkg $raidpkg $fstools $@
> +	apk add --quiet sfdisk $cryptpkg $lvmpkg $raidpkg $fstools $@
>  }
> =20
>  show_disk_info() {
> @@ -1082,6 +1118,18 @@ native_disk_install_lvm() {
>  	setup_root $root_dev $BOOT_DEV
>  }
> =20
> +setup_crypt() {
> +	local dev=3D"$1" local dmname=3D"$2"
> +	mkdir -p /run/cryptsetup
> +	echo "Preparing root partition for encryption." >&2
> +	echo "You will be prompted for your password at boot." >&2
> +	echo "If you forget your password, your data will be lost." >&2
> +	cryptsetup luksFormat --type luks2 "$dev" >&2
> +	echo "Enter password again to unlock disk for installation." >&2
> +	cryptsetup open "$dev" "$dmname" >&2
> +	echo "/dev/mapper/$dmname"
I would check that the device is unlocked properly here and if not, prompt =
for a retry.

> +}
> +
>  native_disk_install() {
>  	local prep_part_type=3D$(partition_id prep)
>  	local root_part_type=3D$(partition_id linux)
> @@ -1156,6 +1204,10 @@ native_disk_install() {
>  		root_dev=3D$(find_nth_non_boot_parts $index "$root_part_type" $@)
>  	fi
> =20
> +	if [ "$USE_CRYPT" ]; then
> +		root_dev=3D$(setup_crypt $root_dev root)
> +	fi
> +
>  	[ $SWAP_SIZE -gt 0 ] && setup_swap_dev $swap_dev
>  	setup_root $root_dev $BOOT_DEV $@
>  }
> @@ -1176,7 +1228,7 @@ diskselect_help() {
>  diskmode_help() {
>  	cat <<-__EOF__
> =20
> -		You can select between 'sys', 'data', 'lvm', 'lvmsys' or 'lvmdata'.
> +		You can select between 'sys', 'cryptsys', 'data', 'lvm', 'lvmsys' or '=
lvmdata'.
> =20
>  		sys:
>  		  This mode is a traditional disk install. The following partitions wi=
ll be
> @@ -1184,6 +1236,12 @@ diskmode_help() {
> =20
>  		  This mode may be used for development boxes, desktops, virtual serve=
rs, etc.
> =20
> +		cryptsys:
> +		  This mode is equivalent to sys, except that the root filesystem will=
 be
> +		  encrypted with cryptsetup. You will be prompted to enter a decryptio=
n
> +		  password, and will need to use this password to boot up the operatin=
g
> +		  system after installation.
> +
>  		data:
>  		  This mode uses your disk(s) for data storage, not for the operating =
system.
>  		  The system itself will run from tmpfs (RAM).
> @@ -1233,7 +1291,7 @@ ask_disk() {
> =20
>  usage() {
>  	cat <<-__EOF__
> -		usage: setup-disk [-hLqrv] [-k kernelflavor] [-m MODE] [-o apkovl] [-s=
 SWAPSIZE]
> +		usage: setup-disk [-hLqrve] [-k kernelflavor] [-m MODE] [-o apkovl] [-=
s SWAPSIZE]
>  		                  [MOUNTPOINT | DISKDEV...]
> =20
>  		Install alpine on harddisk.
> @@ -1247,6 +1305,7 @@ usage() {
> =20
>  		options:
>  		 -h  Show this help
> +		 -e  Encrypt disk
>  		 -m  Use disk for MODE without asking, where MODE is either 'data' or =
'sys'
>  		 -o  Restore system from given apkovl file
>  		 -k  Use kernelflavor instead of $KERNEL_FLAVOR
> @@ -1286,11 +1345,13 @@ case $kver in
>  	*) KERNEL_FLAVOR=3Dlts;;
>  esac
> =20
> +USE_CRYPT=3D
>  DISK_MODE=3D
>  USE_LVM=3D
>  # Parse args
> -while getopts "hk:Lm:o:qrs:v" opt; do
> +while getopts "hek:Lm:o:qrs:v" opt; do
>  	case $opt in
> +		e) USE_CRYPT=3D1;;
>  		m) DISK_MODE=3D"$OPTARG";;
>  		k) KERNEL_FLAVOR=3D"$OPTARG";;
>  		L) USE_LVM=3D"_lvm";;
> @@ -1319,6 +1380,12 @@ fi
>  reset_var
>  swapoff -a
> =20
> +if [ $USE_CRYPT -eq 1 ] && [ -n "$USE_LVM" ]; then
> +	echo "cryptsys and lvm are currently mutually incompatible."
> +	echo "Please run $0 again with only one of these options selected."
> +	exit 1
> +fi
> +
>  # stop all volume groups in use
>  vgchange --ignorelockingfailure -a n >/dev/null 2>&1
> =20
> @@ -1381,9 +1448,14 @@ if [ -n "$diskdevs" ] && [ -z "$DISK_MODE" ]; then=

>  		echo "The following $disk_is_or_disks_are selected${USE_LVM:+ (with LV=
M)}:"
>  		show_disk_info $diskdevs
>  		_lvm=3D${USE_LVM:-", 'lvm'"}
> -		ask "How would you like to use $it_them? ('sys', 'data'${_lvm#_lvm} or=
 '?' for help)" "?"
> +		ask "How would you like to use $it_them? ('sys', 'cryptsys', 'data'${_=
lvm#_lvm} or '?' for help)" "?"
>  		case "$resp" in
>  		'?') diskmode_help;;
> +		cryptsys)
> +			resp=3Dsys
> +			USE_CRYPT=3D1
> +			break
> +			;;
>  		sys|data) break;;
>  		lvm) USE_LVM=3D"_lvm" ;;
>  		nolvm) USE_LVM=3D"";;


--=20
Alex D.
RedXen System & Infrastructure Administration
https://redxen.eu/

------_=_7a5e82c90753041b20607855_=_--

------_=_52d76788307470761ac3f93c_=_
Content-Disposition: attachment; filename=signature.asc
Content-Type: application/pgp-signature
Content-Transfer-Encoding: 7bit

-----BEGIN PGP SIGNATURE-----

iQJEBAABCAAuFiEEnybAVSdgNd2sqr055a6KR7jvx+0FAmERDBUQHGNhc2tkQHJl
ZHhlbi5ldQAKCRDlropHuO/H7ZubD/45lfmhvLQGnP18rSD9wSJYx7xRYx1KKUNL
5MymHD2Cwl9u7B2Sczj/3fnHV40T8lvoGoDwdEiAWSneROXjlHslZ/lbTla8st1T
JyageL9Rk5kh9YbOvU2VwdyEV4LaDxmcMm+kdmL8RGP5V52cnV2NG5n2l7YUUNZq
5BSPO/FiBrMxC4kr9YQVCOlPe2XaL2HimIgCwpvcGJrNixifYNzqwKxvGpuf36XQ
WT6t7kpGkT57joqdD+jzvsIEogKHCC+63IawndLhJfhF+yOP3z0WXMLYsBzYmPv3
/Y2a8fhJOxWogIZ+999oIsi3bZJFjR23p8SullBkH98lDEDZu4HPtsoYJa6RHqJN
6t1M87gtxVxSJ/31hGcIZhNDoshzzYnEq8vBnv07Td8AGUOBp9cKVycRtXjbH6KO
HIScRqx47EEWGI/RW06ToicEvYXGNF8rpnAot+8yZfQn/sqeJ6/sgV7tt6mkfzKm
p5c2XAkW0fEfcCvrzc5PzVPt9oom6UXuq4mQ1aTiN+LUKi7a3MmOk4vm0k90zNON
LjYE5zRcxvGTeK4uU0wdr0TKn4NIvJodM0fe/2eBhUsfRyORMcYjcEZXwVTzRMem
CPeJgk+gbRnbh1nZekGqBcb5nLnVKIBNG+kt1z/LbXNOM2PeLi9GAA4zyJ2Lg6Zz
BxwIVexUnQ==
=hTHX
-----END PGP SIGNATURE-----

------_=_52d76788307470761ac3f93c_=_--