Re: [PATCH v2 1/3] grub-fs-tester: add luks1 and luks2 support

[Glenn Washburn]

Thanks for the update, I'd like to see this get into master. However,
while this was waiting for review some changes were made to master that
make this need some extra work. I've commented below on that and other
issues.

On Tue, 29 Mar 2022 12:31:56 +0200
Pierre-Louis Bonicoli <pierre-louis.bonicoli@libregerbil.fr> wrote:

> The logical sector size used by LUKS1 is 512 bytes. LUKS2 uses 512 to
> 4069 bytes.
> ---
>  .gitignore                   |  2 ++
>  Makefile.util.def            | 12 ++++++++++
>  tests/luks2_test.in          | 23 ++++++++++++++++++
>  tests/luks_test.in           | 23 ++++++++++++++++++
>  tests/util/grub-fs-tester.in | 46 ++++++++++++++++++++++++++++++++++--
>  5 files changed, 104 insertions(+), 2 deletions(-)
>  create mode 100644 tests/luks2_test.in
>  create mode 100644 tests/luks_test.in
> 
> diff --git a/.gitignore b/.gitignore
> index f6a1bd051..9f91f4cf4 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -230,6 +230,8 @@ widthspec.bin
>  /lib/libgcrypt-grub
>  /libgrub_a_init.c
>  /lzocompress_test
> +/luks_test
> +/luks2_test
>  /m4/
>  /minixfs_test
>  /missing
> diff --git a/Makefile.util.def b/Makefile.util.def
> index d919c562c..a910faa4c 100644
> --- a/Makefile.util.def
> +++ b/Makefile.util.def
> @@ -1213,6 +1213,18 @@ script = {
>    common = tests/syslinux_test.in;
>  };
>  
> +script = {
> +  testcase = native;
> +  name = luks_test;
> +  common = tests/luks_test.in;
> +};
> +
> +script = {
> +  testcase = native;
> +  name = luks2_test;
> +  common = tests/luks2_test.in;
> +};
> +
>  program = {
>    testcase = native;
>    name = example_unit_test;
> diff --git a/tests/luks2_test.in b/tests/luks2_test.in
> new file mode 100644
> index 000000000..6a26ba626
> --- /dev/null
> +++ b/tests/luks2_test.in
> @@ -0,0 +1,23 @@
> +#!@BUILD_SHEBANG@
> +
> +set -e
> +
> +if [ "x$EUID" = "x" ] ; then
> +  EUID=`id -u`
> +fi
> +
> +if [ "$EUID" != 0 ] ; then
> +   exit 99
> +fi
> +
> +if ! which mkfs.ext2 >/dev/null 2>&1; then
> +   echo "mkfs.ext2 not installed; cannot test luks2."
> +   exit 99
> +fi
> +
> +if ! which cryptsetup >/dev/null 2>&1; then
> +   echo "cryptsetup not installed; cannot test luks2."
> +   exit 99
> +fi
> +
> +"@builddir@/grub-fs-tester" luks2
> diff --git a/tests/luks_test.in b/tests/luks_test.in
> new file mode 100644
> index 000000000..1b155e29f
> --- /dev/null
> +++ b/tests/luks_test.in
> @@ -0,0 +1,23 @@
> +#!@BUILD_SHEBANG@
> +
> +set -e
> +
> +if [ "x$EUID" = "x" ] ; then
> +  EUID=`id -u`
> +fi
> +
> +if [ "$EUID" != 0 ] ; then
> +   exit 99
> +fi
> +
> +if ! which mkfs.ext2 >/dev/null 2>&1; then
> +   echo "mkfs.ext2 not installed; cannot test luks."
> +   exit 99
> +fi
> +
> +if ! which cryptsetup >/dev/null 2>&1; then
> +   echo "cryptsetup not installed; cannot test luks."
> +   exit 99
> +fi
> +
> +"@builddir@/grub-fs-tester" luks
> diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
> index 65633c7f8..709927f5c 100644
> --- a/tests/util/grub-fs-tester.in
> +++ b/tests/util/grub-fs-tester.in
> @@ -15,7 +15,12 @@ XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset 
> UTF-8"
>  
>  # This wrapper is to ease insertion of valgrind or time statistics
>  run_it () {
> -    LC_ALL=C "$GRUBFSTEST" "$@"
> +    case x"$fs" in
> +    xluks*)
> +     LC_ALL=C echo -n pass | "$GRUBFSTEST" -C "$@";;

I wasn't looking at the totality of the changes and code when I made m
previous comment about doing this where run_grubfstest was called. So
I'll revoke the previous comment. However, I think this should be done
inside run_grubfstest() instead of here.

> +    *)
> +     LC_ALL=C "$GRUBFSTEST" "$@";;
> +    esac
>  }
>  
>  range() {
> @@ -48,6 +53,8 @@ run_grubfstest () {
>  MINLOGSECSIZE=9
>  MAXLOGSECSIZE=9
>  case x"$fs" in
> +    xluks2*)

I don't think we need the '*' here.

> +     MAXLOGSECSIZE=12;;
>      xntfs*)
>       MINLOGSECSIZE=8
>       MAXLOGSECSIZE=12;;
> @@ -335,7 +342,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" 
> "$MAXLOGSECSIZE" 1); do
>                   #FSLABEL="g;/_é莭莽😁кит u"
>                   ;;
>               # FS LIMITATION: reiserfs, extN and jfs label is at most 16 
> UTF-8 characters
> -             x"reiserfs_old" | x"reiserfs" | x"ext"* | x"lvm"* | x"mdraid"* 
> | x"jfs" | x"jfs_caseins")
> +             x"reiserfs_old" | x"reiserfs" | x"ext"* | x"lvm"* | x"luks"* | 
> x"mdraid"* | x"jfs" | x"jfs_caseins")
>                   FSLABEL="g;/éт 莭😁";;
>               # FS LIMITATION: No underscore, space, semicolon, slash or 
> international characters in UFS* in label. Limited to 32 UTF-8 characters
>               x"ufs1" | x"ufs1_sun" | x"ufs2")
> @@ -804,6 +811,12 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" 
> "$MAXLOGSECSIZE" 1); do
>                   MOUNTDEVICE="/dev/mapper/grub_test-testvol"
>                   MOUNTFS=ext2
>                   "mkfs.ext2" -L "$FSLABEL" -q "${MOUNTDEVICE}"  ;;
> +             x"luks"*)
> +                 echo -n pass | cryptsetup luksFormat --type $fs 
> --sector-size $SECSIZE --pbkdf pbkdf2 $LODEVICE
> +                 echo -n pass | cryptsetup open $LODEVICE grub_luks_test

Small nit, I'd like to see grub_luks_test be a shell variable defined
at the top and made reasonably unique per test run. As it is now, two
simultaneous test runs of LUKS* will use the same device, not what we
want. I'm thinking this is fine as is for now and I'll create a patch
after this is merged to do that.

> +                 MOUNTDEVICE="/dev/mapper/grub_luks_test"
> +                 MOUNTFS=ext2
> +                 "mkfs.ext2" -L "$FSLABEL" -q "${MOUNTDEVICE}"  ;;
>               xf2fs)
>                   "mkfs.f2fs" -l "$FSLABEL" -q "${MOUNTDEVICE}" ;;
>               xnilfs2)
> @@ -916,6 +929,20 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" 
> "$MAXLOGSECSIZE" 1); do
>                   GRUBDEVICE="mduuid/`mdadm --detail --export $MOUNTDEVICE | 
> grep MD_UUID=|sed 's,MD_UUID=,,g;s,:,,g'`";;
>               xlvm*)
>                   GRUBDEVICE="lvm/grub_test-testvol";;
> +             xluks*)
> +                 if test x"$fs" = xluks2 && ! (cryptsetup luksDump 
> --dump-json-metadata $LODEVICE | grep -q "\"sector_size\":$SECSIZE"); then

Use --debug-json instead of --dump-json-metadata because the latter is
not available for cryptsetup version 2.3.7 which is what is available on
Debian 11, the test system target machine.

> +                         echo "Unexpected sector size for $LODEVICE 
> (expected: $SECSIZE)"
> +                         exit 1

When this fails, there is no code to cleanup /dev/mapper/grub_luks_test,
the LUKS mapping. In master there is now a way to do this in the
cleanup() function near the top. Code should be added similar to ZFS
which tries to remove/close the LUKS device if it exists.

> +                 fi
> +
> +                 uuid=$(cryptsetup luksUUID $LODEVICE | tr -d '-')
> +                 probe_uuid=$(@builddir@/grub-probe --device $MOUNTDEVICE 
> --target=cryptodisk_uuid)

These variable should be upper case to follow the (mostly) convention.
Also I would rather have @builddir@/grub-probe be $GRUBPROBE and set at
the top where GRUBFSTEST is set.

> +                 if [ x"$uuid" != x"$probe_uuid" ]; then
> +                     echo "$fs probe command FAIL"

This instead should be:
  echo "UUID FAIL"
  echo "$UUID"
  echo "$PROBE_UUID"

> +                     exit 1
> +                 fi
> +                 GRUBDEVICE="cryptouuid/${uuid}"

s/uuid/UUID/

> +                 ;;
>           esac
>           GRUBDIR="($GRUBDEVICE)"
>           case x"$fs" in
> @@ -1071,6 +1098,15 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" 
> "$MAXLOGSECSIZE" 1); do
>                   sleep 1
>                   vgchange -a n grub_test
>                   ;;
> +             xluks*)
> +                 for try in $(range 0 20 1); do
> +                     if umount "$MNTPOINTRW" ; then
> +                         break;
> +                     fi
> +                 done
> +                 UMOUNT_TIME=$(date -u "+%Y-%m-%d %H:%M:%S")
> +                 cryptsetup close grub_luks_test
> +                 ;;
>               xmdraid*)
>                   sleep 1
>                   for try in $(range 0 20 1); do
> @@ -1117,6 +1153,9 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" 
> "$MAXLOGSECSIZE" 1); do
>                   vgchange -a y grub_test
>                   sleep 1
>                   mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o 
> ${MOUNTOPTS}${SELINUXOPTS}ro ;;
> +             xluks*)
> +                 echo -n pass | cryptsetup open $LODEVICE grub_luks_test
> +                 mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o 
> ${MOUNTOPTS}${SELINUXOPTS}ro ;;

After rebasing to master, there should be a line 'MOUNTS="$MOUNTS
$MNTPOINTRO"', like the cases around it.

>               xmdraid*)
>                   mdadm --assemble /dev/md/"${fs}_$NDEVICES" $LODEVICES
>                   sleep 1
> @@ -1558,6 +1597,9 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" 
> "$MAXLOGSECSIZE" 1); do
>                   vgchange -a n grub_test
>                   sleep 1
>                   ;;
> +             xluks*)
> +                 cryptsetup close grub_luks_test
> +                 ;;
>           esac
>           case x"$fs" in
>               x"tarfs" | x"cpio_"* | x"iso9660" | xrockridge | xjoliet | 
> xrockridge_joliet | x"ziso9660" | x"romfs" | x"squash4_"* | x"iso9660_1999" | 
> xrockridge_1999 | xjoliet_1999 | xrockridge_joliet_1999) ;;

Glenn