Received: from mail.cmpwn.com (mail.cmpwn.com [45.56.77.53])
	by nld3-dev1.alpinelinux.org (Postfix) with ESMTPS id CD0AE781A0C
	for <~alpine/devel@lists.alpinelinux.org>; Mon,  4 May 2020 13:43:14 +0000 (UTC)
DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=cmpwn.com; s=cmpwn;
	t=1588599792; bh=YMs7b1tk4BuxfOwQmRZPSVwUXGCUwabfyfO/8VzDQdc=;
	h=Cc:Subject:From:To:Date:In-Reply-To;
	b=V9i/YnewqgTEir9wgk6lAfVVwaAcfVP0U/AItuIw8Kf4sGoHZJbjFHtdAmh+l/E0j
	 WTUksewM2+57TAEYvGSpPNoEGaE9MmlUU21CrbxG8Bxv7dHLxfKYi7V/IuWCB9/miO
	 fKeJ66v7hH8K+Yu+fT0pweBaBYkCQjXF3FWPzUMQ=
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Cc: <~alpine/devel@lists.alpinelinux.org>
Subject: Re: [PATCH v5] Support encrypted root in setup-disk
From: "Drew DeVault" <sir@cmpwn.com>
To: "Natanael Copa" <ncopa@alpinelinux.org>
Date: Mon, 04 May 2020 09:35:15 -0400
Message-Id: <C2HZGIQXJBH5.5GKVB3WJ6CZE@homura>
In-Reply-To: <20200416133308.0f6b27b7@ncopa-desktop.copa.dup.pw>

Thanks for the review. Are you okay with receiving v6 here on the
mailing list or has alpine-conf been moved to GitLab yet?

On Thu Apr 16, 2020 at 9:33 AM PST, Natanael Copa wrote:
> > +	if crypt_required <"$mnt"/etc/fstab; then
>
> Maybe use sed to remove comments here?
>
> sed 's/#.*//' "$mnt"/etc/fstab | crypt_required

crypt_required strips out comments itself, which seems more robust imo.

> > +setup_crypt() {
> > +	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 "$1" >&2
> > +	echo "Enter password again to unlock disk for installation." >&2
> > +	cryptsetup open "$1" root >&2
> > +	cryptroot=3D"$1"
> > +	echo "/dev/mapper/root"
> > +}
>
> I think we should pass "root" as an argument so it can be reused for a
> encrypted data partition in the future.
>
> setup_crypt() {
> local dev=3D"$1" local dmname=3D"$2"
> ...
> echo "Preparing your $dmname partition for encryption."
> ...
> cryptsetup open "$dev" "$dmname"
> }
>
> That way we also don't need send the prompt to stderr.

This is a good idea, but I'm not sure how it saves us from sending the
prompt to stderr.

> Maybe something like if we do the above setup_crypt adjustment:
>
> if [ "$USE_CRYPT" ]; then
> setup_crypt $root_dev root
> root_dev=3D/dev/mapper/root
> fi

I just adjusted setup_crypt to echo the final dev name instead, so e.g.

setup_crypt /dev/sda1 data

Now echos "/dev/mapper/data". Similarly to crypt_required, it seems more
robust to give the onus of correctness over here.

> What happens if you enable bot encryption and lvm (setup-disk -e -L)?
> should we error out and say its not yet supported?

I can make it do so, for sure.