[CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for cryptomount

[CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for cryptomount

[Glenn Washburn]

Grub maintainers,

Here is a patch set that adds functional testing for cryptomount with
LUKS1/2 volumes.  Also added is a required patch to add a new -p option
to cryptomount for specifying passwords on the command line and
importantly by-passing user input from the terminal.  Currently, I rely
heavily on the keyfile code added in the v5 patchset by Denis Carikli.
It appears as though this code has been accepted for inclusion into
master, but has not yet made it there.

As I see it, it makes some sense to have testing code go in before code
that it would be testing so that it might catch any issues with that
code.  However, because it leverages the keyfile code, it wouldn't be
testing that much anyway, though it would for the detached header code.
And from a git history perspective, it seems best to have testing come
first.

However, my concern is that that is not an option due to the timing of
things.  Also, its easier for me to not have to rework history to
include my cryptodisk testing patchset and then update the v5 patchset.
So my question, mainly for those who would ultimately decide on the
inclusion of my patchset, is what is the route I should take to most
easily get my testing patchset accepted?  Should I base my patchset on
current master (e7b8856f8be) (which would entail updating the Carikli's
patchset) or base it on Carikli's patchset?

As can be see from the current tests, LUKS1 detached header support from
the v5 patchset is expected to fail.  I have a patch for that, which I
mentioned in a reply to the offending patch of the v5 patchset.  Also,
I plan on breaking up my CRYPTO-LUKS patchset into several smaller
patchsets and change the current expected failure cases to expecting
success as necessary, and basing those future patchsets on this one.

Guidance would be much appreciated,
Glenn

Glenn Washburn (7):
  cryptodisk: Improve cryptomount short help string.
  cryptodisk: Allow cryptomount password to be specified as argument.
  grub-shell: Allow specifying non-default trim line contents.
  grub-shell: Trim line should always be matched from the beginning of
    the line.
  grub-shell: Only show grub-mkrescue output if it returns an error.
  tests: Add grub-shell-luks-tester to facilitate functional LUKS1/2
    testing.
  test: Add cryptomount test.

 Makefile.util.def                    |  12 +
 grub-core/disk/cryptodisk.c          |  29 ++-
 tests/grub_cmd_cryptomount.in        | 156 +++++++++++++
 tests/util/grub-shell-luks-tester.in | 319 +++++++++++++++++++++++++++
 tests/util/grub-shell.in             |  33 ++-
 5 files changed, 530 insertions(+), 19 deletions(-)
 create mode 100644 tests/grub_cmd_cryptomount.in
 create mode 100644 tests/util/grub-shell-luks-tester.in

-- 
2.25.1

Re: [CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for cryptomount

[Glenn Washburn]

Ugh, I accidentally send this patch series unthreaded.  Also, the v2
patches for patches 6 and 7 supersede the original ones. There is a bug
fix for grub-shell-luks-tester when using --debug and a the changes in
grub_cmd_cryptomount are essentially functionally equivalent to the
original (mostly to mirror the LUKS1 tests).  In the future, is it
better to send out the full patchset again, even if most of the patches
are unchanged?  What's the best practice here?

On Sun, 16 Aug 2020 19:05:11 -0500
Glenn Washburn <development@efficientek.com> wrote:

> Grub maintainers,
> 
> Here is a patch set that adds functional testing for cryptomount with
> LUKS1/2 volumes.  Also added is a required patch to add a new -p
> option to cryptomount for specifying passwords on the command line and
> importantly by-passing user input from the terminal.  Currently, I
> rely heavily on the keyfile code added in the v5 patchset by Denis
> Carikli. It appears as though this code has been accepted for
> inclusion into master, but has not yet made it there.
> 
> As I see it, it makes some sense to have testing code go in before
> code that it would be testing so that it might catch any issues with
> that code.  However, because it leverages the keyfile code, it
> wouldn't be testing that much anyway, though it would for the
> detached header code. And from a git history perspective, it seems
> best to have testing come first.
> 
> However, my concern is that that is not an option due to the timing of
> things.  Also, its easier for me to not have to rework history to
> include my cryptodisk testing patchset and then update the v5
> patchset. So my question, mainly for those who would ultimately
> decide on the inclusion of my patchset, is what is the route I should
> take to most easily get my testing patchset accepted?  Should I base
> my patchset on current master (e7b8856f8be) (which would entail
> updating the Carikli's patchset) or base it on Carikli's patchset?
> 
> As can be see from the current tests, LUKS1 detached header support
> from the v5 patchset is expected to fail.  I have a patch for that,
> which I mentioned in a reply to the offending patch of the v5
> patchset.  Also, I plan on breaking up my CRYPTO-LUKS patchset into
> several smaller patchsets and change the current expected failure
> cases to expecting success as necessary, and basing those future
> patchsets on this one.
> 
> Guidance would be much appreciated,
> Glenn
> 
> Glenn Washburn (7):
>   cryptodisk: Improve cryptomount short help string.
>   cryptodisk: Allow cryptomount password to be specified as argument.
>   grub-shell: Allow specifying non-default trim line contents.
>   grub-shell: Trim line should always be matched from the beginning of
>     the line.
>   grub-shell: Only show grub-mkrescue output if it returns an error.
>   tests: Add grub-shell-luks-tester to facilitate functional LUKS1/2
>     testing.
>   test: Add cryptomount test.
> 
>  Makefile.util.def                    |  12 +
>  grub-core/disk/cryptodisk.c          |  29 ++-
>  tests/grub_cmd_cryptomount.in        | 156 +++++++++++++
>  tests/util/grub-shell-luks-tester.in | 319
> +++++++++++++++++++++++++++ tests/util/grub-shell.in             |
> 33 ++- 5 files changed, 530 insertions(+), 19 deletions(-)
>  create mode 100644 tests/grub_cmd_cryptomount.in
>  create mode 100644 tests/util/grub-shell-luks-tester.in
>