[LOCAL-CI v2 3/3] scripts: Add local-tester.sh script to run local CI tests
Signed-off-by: Glenn Washburn
---
scripts/ci/functions.local.sh | 37 +
scripts/local-tester.sh | 39 +++
2 files changed, 76 insertions(+)
create mode 100644 scripts/ci/functions.local.sh
create mode 100755 scripts/local-tester.sh
diff --git a/scripts/ci/functions.local.sh b/scripts/ci/functions.local.sh
new file mode 100644
index 0..7a6766c07
--- /dev/null
+++ b/scripts/ci/functions.local.sh
@@ -0,0 +1,37 @@
+#!/bin/bash
+
+TXT_RED="\e[31m"
+TXT_YELLOW="\e[33m"
+TXT_CLEAR="\e[0m"
+TXT_LOG_COLOR="\e[97;100m"
+
+function start_log() {
+ local LOG_COLLAPSE LOG_NAME
+ while [ "$#" -gt 0 ]; do
+case "$1" in
+ -c) LOG_COLLAPSE=1; shift;;
+ -n) LOG_NAME="$2"; shift;;
+ *) [ "$#" -eq 1 ] && LOG_MSG="$1"; shift;;
+esac
+ done
+
+ echo -e "=="
+ echo -e "== ${TXT_YELLOW}Start Section ${LOG_NAME}:
${LOG_MSG}${TXT_CLEAR}"
+ echo -e "=="
+ echo -e "${LOG_NAME} STARTTIME=`date +%s`" >&2
+}
+
+function end_log() {
+ local LOG_NAME
+ while [ "$#" -gt 0 ]; do
+case "$1" in
+ -n) LOG_NAME=$2; shift;;
+ *) shift;;
+esac
+ done
+
+ echo -e "${LOG_NAME} ENDTIME=`date +%s`" >&2
+ echo -e "== End Section ${LOG_NAME} =="
+}
+
+:;
diff --git a/scripts/local-tester.sh b/scripts/local-tester.sh
new file mode 100755
index 0..9cc7b8f43
--- /dev/null
+++ b/scripts/local-tester.sh
@@ -0,0 +1,39 @@
+#!/bin/bash
+set -e
+
+SDIR=$(realpath -e "${SDIR:-"${0%/*}"}")
+ROOT=$(realpath -m "${ROOT:-"./grub-tests"}")
+
+mkdir -p "$ROOT"
+cd "$ROOT"
+
+# Log stdout+stderr to file while keeping stdout going to original stdout
+exec 2>"$ROOT/$(basename "$0" .sh).$(date '+%Y%m%d%H%M%S').log" > >(exec 3>&1;
exec tee >(exec cat >&3) >&2)
+PTTY=$!
+
+if [ "x$SHELL_TRACE" = "xy" ]; then
+ set -x
+ export >&2
+fi
+
+export CI_TYPE=local
+if [ -d "${SDIR}/ci" ]; then
+ export SCRIPTS_DIR=${SCRIPTS_DIR:-"${SDIR}"}
+ export CI_SCRIPTS_DIR=${CI_SCRIPTS_DIR:-"${SCRIPTS_DIR}/ci"}
+fi
+
+# If found, source helper functions
+[ -f "${CI_SCRIPTS_DIR}/functions.sh" ] &&
+. "${CI_SCRIPTS_DIR}/functions.sh"
+
+
+onexit() {
+ set +x
+ # Must close our end of the pipe to tee, else it will not exit and
+ # we'll wait forever on it to exit.
+ exec >&-
+ wait_anypid $PTTY
+}
+trap onexit exit
+
+main "$@"
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[LOCAL-CI v2 0/3] Add support for local automated testing
Updates since v1:
* update multipipe to return error code from children
* fix bootstrap run cache file not written to correct location
* fix run_test and run_build to properly return test and build error codes
* fix functional test post-processing to recognize failures in any functional
test, not just ones whose name ends in "test"
* fix issue where tests which were scripts that didn't use @BUILD_SHEBANG@ as
shell were being run with @BUILD_SHEBANG@ shell, and thus weren't running
those tests properly (or at all)
* other minor improvements
This patch series aims to make automated testing of all supported targets easy.
Not all targets are actually tested for a variety of reasons, but this series
covers a significant portion of the target space and should make it easy to
add more targets as we figure out how do run their tests. All supported targets
(that I know of) are build tested.
The main work horse is the scripts/local-tester.sh script which ties everything
together. It is very configurable, but defaults to building and testing as much
as it can. To start it just run "./scripts/local-tester.sh" from the root of the
repository. By default a directory named 'grub-tests' in the current working
directory is created and everything is put in there. See the beginning of
./scripts/ci/function.sh for available environment variables that can be used
to configure how you want it to run.
The file ./scripts/ci/function.sh has a bunch of functions most of which are
for stages in the automated testing. It is intended to be sourced by config
files for other CI systems so that they can reuse this code.
One might ask, why not just have all CI systems use local-tester.sh and put
everything in there? The issue is that currently local-tester.sh does not do
things in parallel (the make subprocess can be made more parallel with the
JOBS environment variable). So in many CI systems, one could have all targets
building and testing at the same time, which local-tester.sh only does this
serially. This makes local-tester.sh a lot slower than it needs to be. Also
other CI systems allow the CI pipeline to be broken into many stages, each of
which could be run on a different machine, with the ability to cache the
output of certain stages.
These scripts have been written and tested on debian based systems, specifically
the reference system, Debian 11. Some of the stages are debian or debian-
derivative specific, such as the package install stage which uses apt and dpkg.
Most of the stages, I believe, are fairly distro agnostic and the ones that
aren't should be able to be adapted for a specific distro fairly easily.
Also, this patch series is meant to be used on top of the grub-shell patch
series already submitted to the list. It will run without that series, but
some of the features may not work or work as well. Noteably, the QEMU tests
for targets i386-efi, arm-efi and arm64-efi will fail.
Of particular note, there are some knobs that can provide a lot debugging
output and save the intermediate files of failed tests for later analysis.
local-tester.sh will try to download and install all packages it needs to
function. Obviously, this will not work when not running as a privileged user.
A further patch series is intended, which will add support for running the
system successfully completely as an unprivileged user. If local-tester.sh is
run as an unprivileged user, it will skip running of privileged commands, like
the package installer. This means it can continue past the package install
phase, but it assumes that the needed packages are already installed.
Glenn
Glenn Washburn (3):
scripts: Add general scripts to aid automated testing
scripts: Add functions for CI stages and default variables to
functions.sh
scripts: Add local-tester.sh script to run local CI tests
scripts/ci/build.sh | 67 +++
scripts/ci/functions.local.sh | 37 ++
scripts/ci/functions.sh | 954 ++
scripts/ci/make-images.sh | 86 +++
scripts/ci/process-tests.sh | 111
scripts/ci/test.sh| 121 +
scripts/local-tester.sh | 39 ++
7 files changed, 1415 insertions(+)
create mode 100755 scripts/ci/build.sh
create mode 100644 scripts/ci/functions.local.sh
create mode 100644 scripts/ci/functions.sh
create mode 100755 scripts/ci/make-images.sh
create mode 100755 scripts/ci/process-tests.sh
create mode 100755 scripts/ci/test.sh
create mode 100755 scripts/local-tester.sh
Interdiff against v1:
diff --git a/scripts/ci/functions.sh b/scripts/ci/functions.sh
index f94f90dc1..734b36d3f 100644
--- a/scripts/ci/functions.sh
+++ b/scripts/ci/functions.sh
@@ -350,7 +350,8 @@ EXPECTED_FAILURES=${EXPECTED_FAILURES:-"
### Usually this test passes, but occasionally the virtual machine is
### too slow and the time delta threshold is exceeded. So allow failures.
#grub_cmd_sleep
- ### This fails on the volume label check
+ ### This can fail on
[LOCAL-CI v2 2/3] scripts: Add functions for CI stages and default variables to functions.sh
Signed-off-by: Glenn Washburn
---
scripts/ci/functions.sh | 923 +++-
1 file changed, 922 insertions(+), 1 deletion(-)
diff --git a/scripts/ci/functions.sh b/scripts/ci/functions.sh
index 2f4cecaa1..734b36d3f 100644
--- a/scripts/ci/functions.sh
+++ b/scripts/ci/functions.sh
@@ -1,5 +1,412 @@
#!/bin/bash
+### Documentation on configuration environment variables
+
+# SDIR [dirname of $0]:
+# Path to directory the caller of this script resides in.
+# ROOT [./grub-tests]:
+# Path to directory where all file operations take place, unless
overridden
+# by another environment variable.
+# CI_PROJECT_DIR [$ROOT]:
+# Base directory to use in commands.
+# CI_TYPE [local]:
+# Value used to control sourcing of ${CI_SCRIPT_DIR}/functions.$CI_TYPE.sh
+# which overrides the variables/functions in this file.
+# SCRIPTS_DIR [${SDIR} if ${SDIR}/ci else ${SRCDIR}/scripts]:
+# Path to scripts directory from source.
+# CI_SCRIPTS_DIR [${SCRIPTS_DIR}/ci]:
+# Path to CI scripts directory, normally the ci subdirectory of SCRITS_DIR
+# and where this file resides.
+# SHELL_TRACE [n]:
+# If set, turn on shell tracing of everything. NOTE: TEST_VERBOSITY=3
turns
+# on more targeted shell tracing
+# TMPDIR [$ROOT/testtmp]:
+# Path to directory to use for temporary files.
+# MATRIX [see definition below]:
+# A list of $ARCH:$TARGET[,...]:$CROSS_ARCH, one per line, with lines
+# starting with # being ignored.
+# JOBS:
+# Number of make jobs to run in parallel. Defaulted in build.sh and
test.sh
+# to the number of available processors.
+# NUM_FAILED_LOG_LINES [100]:
+# Set to integer number of log lines to display from the end of a failed
+# log file.
+
+### Source checkout variables
+# GIT_REPO:
+# URL to git repo
+# GIT_BRANCH:
+# Branch name in git repo
+# GIT_CLONE_ARGS [--depth=1 --no-single-branch]:
+# Extra args to use when cloning repository
+# SRCDIR [${CI_PROJECT_DIR}/grub]:
+# Path to source checkout
+
+### Distro package variables
+# APT_OPTS [--no-upgrade]:
+# Extra options when running apt.
+# PACKAGES [see definition below]:
+# A list of packages to add to a default list of packages needing to be
+# installed. The default list can only be appended to.
+# PACKAGE_CACHE [${CI_PROJECT_DIR}/packages]:
+# Path to directory where downloaded packages will be cached.
+
+### Ccache variables
+# CCACHE_DIR [${CI_PROJECT_DIR}/ccache]:
+# Directory to use for ccache
+
+### Cross compiler tool chain variables
+# CROSS_DIR [$CI_PROJECT_DIR/cross]:
+# Path to directory containing cross compilers tool chains for each
+# architecture. If it doesn't exist, it will be created and required tool
+# chains will be downloaded/installed there.
+# CROSS_VERSION [10.1.0]:
+# Version of cross-compiler to use (suggested: 10.1.0, 9.3.0, 8.1.0,
+# 7.5.0 [riscv32/64 builds fail before 7.3.0]).
+
+### Configure/Build variables
+# CONFIGURE_OPTS [--enable-boot-time]:
+# Extra options to pass to configure.
+# FONT_SOURCE:
+# Path to unicode font named "unifont." where ext is one of:
+#pcf pcf.gz bdf bdf.gz ttf ttf.gz
+# DJVU_FONT_SOURCE:
+# Path to DejaVu font named "DejaVuSans." where ext is one of:
+#pcf pcf.gz bdf bdf.gz ttf ttf.gz
+# BUILD_ALL_TARGETS:
+# If set 'y', all targets defined in the build matrix will be
+# built ignoring DISABLED_BUILDS, but can still be disabled by
+# DISABLE_ALL_BUILDS.
+# DISABLED_BUILDS [see definition below]:
+# A list of targets for which builds are disabled, one per line. These
+# default targets are disabled generally because they do not work yet.
+# Lines beginning with '#' may be used as comments and are ignored. Some
+# hints as to why the tests fail are included in comment lines. Patches
+# which get these tests working are very welcome.
+# DISABLE_ALL_BUILDS [n]:
+# If set to 'y', disable completely building (eg. the run_build command).
+
+### Tests variables
+# TESTS_TIMEOUT:
+# Set timeout for completion of all make check tests (see "man timeout"
+# duration argument). This is most useful when the make check job is
+# taking longer than the job timeout configured by the runner. In this
+# case, set this to 5-10 minutes less than runner timeout so that there
+# is time to package up the logs for debugging.
+# IGNORE_TIMEDOUT_TEST_FAILURE [y]:
+# If set to 'y', tests which fail due to a timeout in grub-shell of qemu
+# will not be counted as a failure. These failures are almost always the
+# result of insufficient runner resources to complete the execution of
qemu
+# within the timeout period.
+# FAIL_ON_HARD_ERRORS [n]:
+# If set to 'y', hard errors will cause a failure. A hard error indicates
+# that the test was not able to be run (eg. dependencies not found), and
+# as su
[LOCAL-CI v2 1/3] scripts: Add general scripts to aid automated testing
Signed-off-by: Glenn Washburn
---
scripts/ci/build.sh | 67
scripts/ci/functions.sh | 33 ++
scripts/ci/make-images.sh | 86 +
scripts/ci/process-tests.sh | 111 +
scripts/ci/test.sh | 121
5 files changed, 418 insertions(+)
create mode 100755 scripts/ci/build.sh
create mode 100644 scripts/ci/functions.sh
create mode 100755 scripts/ci/make-images.sh
create mode 100755 scripts/ci/process-tests.sh
create mode 100755 scripts/ci/test.sh
diff --git a/scripts/ci/build.sh b/scripts/ci/build.sh
new file mode 100755
index 0..723dc292f
--- /dev/null
+++ b/scripts/ci/build.sh
@@ -0,0 +1,67 @@
+#!/bin/bash
+
+set -eo pipefail
+
+# Environment variables
+# JOBS: Number of concurrent jobs while building, defaults to number of
+# processors plus 1, unless number of processors is 1 in which case its 2.
+# SRCDIR: Path to source files
+# BUILDDIR: Directory in which to place the build
+# INSTALLDIR: Directory to install binaries
+# ARCH: Architecture to build for
+# PLATFORM: Platform to build for
+# CONFIGURE_OPTS: Extra configure options
+# FONT_SOURCE: Path to unicode font named "unifont." where ext is one of:
+# pcf pcf.gz bdf bdf.gz ttf ttf.gz
+# DJVU_FONT_SOURCE: Path to DejaVu font named "DejaVuSans." where ext is
+# one of: pcf pcf.gz bdf bdf.gz ttf ttf.gz
+# SHELL_TRACE: Set to 'y' to enable shell tracing
+
+[ "x${SHELL_TRACE}" = "xy" ] && set -x
+
+[ -f "$(dirname "$0")/functions.sh" ] &&
+. "$(dirname "$0")/functions.sh"
+
+[ -f "$(dirname "$0")/functions.$CI_TYPE.sh" ] &&
+. "$(dirname "$0")/functions.$CI_TYPE.sh"
+
+JOBS=${JOBS:-`getconf _NPROCESSORS_ONLN 2> /dev/null || echo 2`}
+[ "$JOBS" = 1 ] || JOBS=$(($JOBS + 1))
+
+TARGET="${ARCH}-${PLATFORM}"
+
+mkdir -pv ${BUILDDIR}
+
+RET=0
+cd ${BUILDDIR}
+
+if test -f "$FONT_SOURCE"; then
+ ln -svf "$(realpath -e "$FONT_SOURCE")" ./
+fi
+
+if test -f "$DJVU_FONT_SOURCE"; then
+ ln -svf "$(realpath -e "$DJVU_FONT_SOURCE")" ./
+fi
+
+start_log -c -n "configure-$TARGET" "Configuring $TARGET"
+[ -x "$SRCDIR/configure" ] && $SRCDIR/configure --help
+$SRCDIR/configure --target=$ARCH --with-platform=$PLATFORM \
+ --prefix=${INSTALLDIR} $CONFIGURE_OPTS || RET=$?
+end_log -n "configure-$TARGET"
+
+if [ "$RET" -ne 0 ]; then
+ start_log -c -n "showlogs-$TARGET" "Configuring $TARGET failed, showing logs"
+ cat ${BUILDDIR}/config.log
+ end_log -n "showlogs-$TARGET"
+ exit $RET
+fi
+
+start_log -c -n "build-$TARGET" "Building $TARGET"
+make ${JOBS:+-j$JOBS} || RET=$?
+end_log -n "build-$TARGET"
+[ "$RET" -ne 0 ] && exit $RET
+
+start_log -c -n "install-$TARGET" "Installing $TARGET"
+make ${JOBS:+-j$JOBS} install || RET=$?
+end_log -n "install-$TARGET"
+exit $RET
diff --git a/scripts/ci/functions.sh b/scripts/ci/functions.sh
new file mode 100644
index 0..2f4cecaa1
--- /dev/null
+++ b/scripts/ci/functions.sh
@@ -0,0 +1,33 @@
+#!/bin/bash
+
+TXT_RED="\e[31m"
+TXT_YELLOW="\e[33m"
+TXT_CLEAR="\e[0m"
+TXT_LOG_COLOR="\e[97;100m"
+
+function start_log() {
+ local LOG_COLLAPSE LOG_NAME
+ while [ "$#" -gt 0 ]; do
+case "$1" in
+ -c) LOG_COLLAPSE=1; shift;;
+ -n) LOG_NAME="$2"; shift;;
+ *) [ "$#" -eq 1 ] && LOG_MSG="$1"; shift;;
+esac
+ done
+
+ echo -e "Start:${LOG_NAME} ${LOG_MSG}"
+}
+
+function end_log() {
+ local LOG_NAME
+ while [ "$#" -gt 0 ]; do
+case "$1" in
+ -n) LOG_NAME=$2; shift;;
+ *) shift;;
+esac
+ done
+
+ echo -e "End:${LOG_NAME}"
+}
+
+:;
diff --git a/scripts/ci/make-images.sh b/scripts/ci/make-images.sh
new file mode 100755
index 0..d156af46f
--- /dev/null
+++ b/scripts/ci/make-images.sh
@@ -0,0 +1,86 @@
+#!/bin/bash
+
+set -eo pipefail
+
+# Environment variables
+# SRCDIR: Path to source files
+# BUILDDIR: Directory in which to place the build
+# TARGET: Target to test
+# MAKE_ALL_IMAGE_TARGETS: If set to 'y', then all image targets, even disabled
+# ones, will be run.
+# DISABLED_IMAGES: String of target formats to disable when building grub
+# images delimited by new lines.
+# SHELL_TRACE: Set to 'y' to enable shell tracing
+# NUM_FAILED_LOG_LINES: Set to integer number of log lines to display from the
+# end of a failed log file
+
+if [ "x${SHELL_TRACE}" = "xy" ]; then
+ # If we export SHELL_OPTS, then all shells will be traced, most of which we
do not want
+ SHELL_OPTS="-x"
+ set -x
+fi
+
+[ -f "$(dirname "$0")/functions.sh" ] &&
+. "$(dirname "$0")/functions.sh"
+
+[ -f "$(dirname "$0")/functions.$CI_TYPE.sh" ] &&
+. "$(dirname "$0")/functions.$CI_TYPE.sh"
+
+start_log -c -n "images-$TARGET" "Making images for $TARGET"
+
+if [ ! -x "${BUILDDIR}/grub-mkimage" ]; then
+ echo "Aborting, no grub-mkimage found in builddir"
+ exit 1
+fi
+
+function build_tformat() {
+ TARGET=$1
+ FORMATS=$2
+ for fmt in $FORMATS; do
+if [ -z "${fmt%~*}" ]; then
+ echo "${TARGET}"
+else
+ echo "${TARGET}-${fmt}"
+
Re: [PATCH v8 3/7] cryptodisk: enable the backends to implement detached headers
This comes from Dmitry author of the previously submitted LUKS2 master
key support. Since he's not on the list, I'm taking some of his
response and re-posting it here (modified to be faithful to his
original message) and will add him to future discussions on this.
Glenn
On Tue, 4 Jan 2022 21:25:14 +0300
Dmitry wrote:
> Signed-off-by: John Lane
> gnu...@cyberdimension.org: rebase, patch split, small fixes, commit message
> Signed-off-by: Denis 'GNUtoo' Carikli
> developm...@efficientek.com: rebase, rework for cryptomount parameter passing
> Signed-off-by: Glenn Washburn
> ---
> grub-core/disk/cryptodisk.c | 15 ++-
> grub-core/disk/geli.c | 10 ++
> grub-core/disk/luks.c | 8
> grub-core/disk/luks2.c | 8
> include/grub/cryptodisk.h | 2 ++
> include/grub/file.h | 2 ++
> 6 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 497097394..e90f680f0 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -42,6 +42,7 @@ static const struct grub_arg_option options[] =
> {"all", 'a', 0, N_("Mount all."), 0, 0},
> {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> {"password", 'p', 0, N_("Password to open volumes."), 0,
> ARG_TYPE_STRING},
> +{"header", 'H', 0, N_("Read header from file"), 0, ARG_TYPE_STRING},
> {0, 0, 0, 0, 0, 0}
>};
>
> @@ -1173,6 +1174,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
>cargs.key_len = grub_strlen (state[3].arg);
> }
>
> + if (state[4].set) /* Detached header */
> +{
> + if (state[0].set)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +N_("Cannot use UUID lookup with detached header"));
> +
> + cargs.hdr_file = grub_file_open (state[4].arg,
> + GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER);
> + if (!cargs.hdr_file)
> + return grub_errno;
> +}
> +
>if (state[0].set) /* uuid */
> {
>int found_uuid;
> @@ -1385,7 +1398,7 @@ GRUB_MOD_INIT (cryptodisk)
> {
>grub_disk_dev_register (&grub_cryptodisk_dev);
>cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
> - N_("[-p password] "),
> + N_("[-p password] [-H file] UUID|-a|-b>"),
> N_("Mount a crypto device."), options);
>grub_procfs_register ("luks_script", &luks_script);
> }
> diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> index 5b3a11881..0b8046746 100644
> --- a/grub-core/disk/geli.c
> +++ b/grub-core/disk/geli.c
> @@ -52,6 +52,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -121,6 +122,7 @@ enum
>
> /* FIXME: support version 0. */
> /* FIXME: support big-endian pre-version-4 volumes. */
> +/* FIXME: support for detached headers. */
> /* FIXME: support for keyfiles. */
> /* FIXME: support for HMAC. */
> const char *algorithms[] = {
> @@ -252,6 +254,10 @@ geli_scan (grub_disk_t disk, grub_cryptomount_args_t
> cargs)
>grub_disk_addr_t sector;
>grub_err_t err;
>
> + /* Detached headers are not implemented yet */
> + if (cargs->hdr_file)
> +return NULL;
> +
>if (2 * GRUB_MD_SHA256->mdlen + 1 > GRUB_CRYPTODISK_MAX_UUID_LENGTH)
> return NULL;
>
> @@ -412,6 +418,10 @@ geli_recover_key (grub_disk_t source, grub_cryptodisk_t
> dev, grub_cryptomount_ar
>if (cargs->key_data == NULL || cargs->key_len == 0)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
>
> + /* Detached headers are not implemented yet */
> + if (cargs->hdr_file)
> + return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +
>if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
> return grub_error (GRUB_ERR_BUG, "cipher block is too long");
>
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index d57257b3e..032a9db3c 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -75,6 +75,10 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
>char hashspec[sizeof (header.hashSpec) + 1];
>grub_err_t err;
>
> + /* Detached headers are not implemented yet */
> + if (cargs->hdr_file)
> +return NULL;
> +
>if (cargs->check_boot)
> return NULL;
>
> @@ -164,6 +168,10 @@ luks_recover_key (grub_disk_t source,
>if (cargs->key_data == NULL || cargs->key_len == 0)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
>
> + /* Detached headers are not implemented yet */
> + if (cargs->hdr_file)
> + return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +
>err = grub_disk_read (source, 0, 0, sizeof (header), &header);
>if (err)
> return err;
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index ccfacb63a..567368f11 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/d
Re: [PATCH v8 5/7] cryptodisk: enable the backends to implement key files
Also from Dmitry.
On Tue, 4 Jan 2022 21:25:14 +0300
Dmitry wrote:
> From: John Lane
>
> Signed-off-by: John Lane
> gnu...@cyberdimension.org: rebase, patch split, small fixes, commit message
> Signed-off-by: Denis 'GNUtoo' Carikli
> developm...@efficientek.com: rebase and rework to use cryptomount arg passing
> Signed-off-by: Glenn Washburn
> ---
> grub-core/disk/cryptodisk.c | 83 +
> include/grub/cryptodisk.h | 2 +
> include/grub/file.h | 2 +
> 3 files changed, 87 insertions(+)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index e90f680f0..ea8ed20e2 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -43,6 +43,9 @@ static const struct grub_arg_option options[] =
> {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> {"password", 'p', 0, N_("Password to open volumes."), 0,
> ARG_TYPE_STRING},
> {"header", 'H', 0, N_("Read header from file"), 0, ARG_TYPE_STRING},
> +{"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
You have custom options --header, --keyfile. I suggest renaming in a
similar
way as in cryptsetup(8) - --header, --key-file, (--master-key-file)
> +{"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0,
> ARG_TYPE_INT},
> +{"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0,
> ARG_TYPE_INT},
> {0, 0, 0, 0, 0, 0}
>};
>
> @@ -1186,6 +1189,86 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
> return grub_errno;
> }
>
> + if (state[5].set) /* keyfile */
> +{
> + const char *p = NULL;
> + grub_file_t keyfile;
> + int keyfile_offset;
> + grub_size_t requested_keyfile_size = 0;
> +
> +
> + if (state[6].set) /* keyfile-offset */
> + {
> + keyfile_offset = grub_strtoul (state[6].arg, &p, 0);
> +
> + if (grub_errno != GRUB_ERR_NONE)
> + return grub_errno;
> +
> + if (*p != '\0')
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +N_("unrecognized number"));
> + }
> + else
> + {
> + keyfile_offset = 0;
> + }
> +
> + if (state[7].set) /* keyfile-size */
> + {
> + requested_keyfile_size = grub_strtoul (state[7].arg, &p, 0);
> +
> + if (*p != '\0')
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +N_("unrecognized number"));
> +
> + if (grub_errno != GRUB_ERR_NONE)
> + return grub_errno;
> +
> + if (requested_keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
> + return grub_error (GRUB_ERR_OUT_OF_RANGE,
> + N_("Key file size exceeds maximum (%d)\n"),
> + GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
> +
> + if (requested_keyfile_size == 0)
> + return grub_error (GRUB_ERR_OUT_OF_RANGE,
> + N_("Key file size is 0\n"));
> + }
> +
> + keyfile = grub_file_open (state[5].arg,
> + GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
> + if (!keyfile)
> + return grub_errno;
> +
> + if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
> + return grub_errno;
> +
> + if (requested_keyfile_size)
> + {
> + if (requested_keyfile_size > (keyfile->size - keyfile_offset))
> + return grub_error (GRUB_ERR_FILE_READ_ERROR,
> +N_("Keyfile is too small: "
> + "requested %" PRIuGRUB_SIZE " bytes, "
> + "but the file only has %" PRIuGRUB_UINT64_T
> + " bytes.\n"),
> +requested_keyfile_size,
> +keyfile->size);
> +
> + cargs.key_len = requested_keyfile_size;
> + }
> + else
> + {
> + cargs.key_len = keyfile->size - keyfile_offset;
> + }
> +
> + cargs.key_data = grub_malloc (cargs.key_len);
> + if (!cargs.key_data)
> + return GRUB_ERR_OUT_OF_MEMORY;
> +
> + if (grub_file_read (keyfile, cargs.key_data, cargs.key_len) !=
> (grub_ssize_t) cargs.key_len)
> + return grub_error (GRUB_ERR_FILE_READ_ERROR,
> +(N_("Error reading key file\n")));
> +}
> +
>if (state[0].set) /* uuid */
> {
>int found_uuid;
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 9fe451de9..d94df68b6 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -62,6 +62,8 @@ typedef enum
> #define GRUB_CRYPTODISK_MAX_KEYLEN 128
> #define GRUB_CRYPTODISK_MAX_PASSPHRASE 256
>
> +#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192
> +
> struct grub_cryptodisk;
>
> typedef gcry_err_code_t
> diff --git a/include/grub/file.h b/include/grub/file.h
> index 3a3c49a04..2d5d16cd2 100644
> --- a/include/grub/file.h
> +++ b/include/grub/file.h
> @@ -92,6 +92,8 @@ enum grub
Re: [PATCH v8 5/7] cryptodisk: enable the backends to implement key files
On Tue, 4 Jan 2022 15:46:19 -0600
Glenn Washburn wrote:
> Also from Dmitry.
>
> On Tue, 4 Jan 2022 21:25:14 +0300
> Dmitry wrote:
>
> > From: John Lane
> >
> > Signed-off-by: John Lane
> > gnu...@cyberdimension.org: rebase, patch split, small fixes, commit message
> > Signed-off-by: Denis 'GNUtoo' Carikli
> > developm...@efficientek.com: rebase and rework to use cryptomount arg
> > passing
> > Signed-off-by: Glenn Washburn
> > ---
> > grub-core/disk/cryptodisk.c | 83 +
> > include/grub/cryptodisk.h | 2 +
> > include/grub/file.h | 2 +
> > 3 files changed, 87 insertions(+)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index e90f680f0..ea8ed20e2 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -43,6 +43,9 @@ static const struct grub_arg_option options[] =
> > {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> > {"password", 'p', 0, N_("Password to open volumes."), 0,
> > ARG_TYPE_STRING},
> > {"header", 'H', 0, N_("Read header from file"), 0, ARG_TYPE_STRING},
> > +{"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
>
> You have custom options --header, --keyfile. I suggest renaming in a
> similar
> way as in cryptsetup(8) - --header, --key-file, (--master-key-file)
I think sounds reasonable. I'll change this in the next version unless
I hear a good reason not too.
Glenn
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v8 3/7] cryptodisk: enable the backends to implement detached headers
On Tue, 4 Jan 2022 15:42:22 -0600
Glenn Washburn wrote:
> This comes from Dmitry author of the previously submitted LUKS2 master
> key support. Since he's not on the list, I'm taking some of his
> response and re-posting it here (modified to be faithful to his
> original message) and will add him to future discussions on this.
>
> Glenn
>
> On Tue, 4 Jan 2022 21:25:14 +0300
> Dmitry wrote:
>
> > Signed-off-by: John Lane
> > gnu...@cyberdimension.org: rebase, patch split, small fixes, commit message
> > Signed-off-by: Denis 'GNUtoo' Carikli
> > developm...@efficientek.com: rebase, rework for cryptomount parameter
> > passing
> > Signed-off-by: Glenn Washburn
> > ---
> > grub-core/disk/cryptodisk.c | 15 ++-
> > grub-core/disk/geli.c | 10 ++
> > grub-core/disk/luks.c | 8
> > grub-core/disk/luks2.c | 8
> > include/grub/cryptodisk.h | 2 ++
> > include/grub/file.h | 2 ++
> > 6 files changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 497097394..e90f680f0 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -42,6 +42,7 @@ static const struct grub_arg_option options[] =
> > {"all", 'a', 0, N_("Mount all."), 0, 0},
> > {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> > {"password", 'p', 0, N_("Password to open volumes."), 0,
> > ARG_TYPE_STRING},
> > +{"header", 'H', 0, N_("Read header from file"), 0, ARG_TYPE_STRING},
> > {0, 0, 0, 0, 0, 0}
> >};
> >
> > @@ -1173,6 +1174,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt,
> > int argc, char **args)
> >cargs.key_len = grub_strlen (state[3].arg);
> > }
> >
> > + if (state[4].set) /* Detached header */
> > +{
> > + if (state[0].set)
> > + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > + N_("Cannot use UUID lookup with detached header"));
> > +
> > + cargs.hdr_file = grub_file_open (state[4].arg,
> > + GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER);
> > + if (!cargs.hdr_file)
> > + return grub_errno;
> > +}
> > +
> >if (state[0].set) /* uuid */
> > {
> >int found_uuid;
> > @@ -1385,7 +1398,7 @@ GRUB_MOD_INIT (cryptodisk)
> > {
> >grub_disk_dev_register (&grub_cryptodisk_dev);
> >cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
> > - N_("[-p password] "),
> > + N_("[-p password] [-H file] > UUID|-a|-b>"),
> > N_("Mount a crypto device."), options);
> >grub_procfs_register ("luks_script", &luks_script);
> > }
> > diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> > index 5b3a11881..0b8046746 100644
> > --- a/grub-core/disk/geli.c
> > +++ b/grub-core/disk/geli.c
> > @@ -52,6 +52,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -121,6 +122,7 @@ enum
> >
> > /* FIXME: support version 0. */
> > /* FIXME: support big-endian pre-version-4 volumes. */
> > +/* FIXME: support for detached headers. */
> > /* FIXME: support for keyfiles. */
> > /* FIXME: support for HMAC. */
> > const char *algorithms[] = {
> > @@ -252,6 +254,10 @@ geli_scan (grub_disk_t disk, grub_cryptomount_args_t
> > cargs)
> >grub_disk_addr_t sector;
> >grub_err_t err;
> >
> > + /* Detached headers are not implemented yet */
> > + if (cargs->hdr_file)
> > +return NULL;
> > +
> >if (2 * GRUB_MD_SHA256->mdlen + 1 > GRUB_CRYPTODISK_MAX_UUID_LENGTH)
> > return NULL;
> >
> > @@ -412,6 +418,10 @@ geli_recover_key (grub_disk_t source,
> > grub_cryptodisk_t dev, grub_cryptomount_ar
> >if (cargs->key_data == NULL || cargs->key_len == 0)
> > return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
> >
> > + /* Detached headers are not implemented yet */
> > + if (cargs->hdr_file)
> > + return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > +
> >if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
> > return grub_error (GRUB_ERR_BUG, "cipher block is too long");
> >
> > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> > index d57257b3e..032a9db3c 100644
> > --- a/grub-core/disk/luks.c
> > +++ b/grub-core/disk/luks.c
> > @@ -75,6 +75,10 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t
> > cargs)
> >char hashspec[sizeof (header.hashSpec) + 1];
> >grub_err_t err;
> >
> > + /* Detached headers are not implemented yet */
> > + if (cargs->hdr_file)
> > +return NULL;
> > +
> >if (cargs->check_boot)
> > return NULL;
> >
> > @@ -164,6 +168,10 @@ luks_recover_key (grub_disk_t source,
> >if (cargs->key_data == NULL || cargs->key_len == 0)
> > return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
> >
> > + /* Detached headers are not implemented yet */
> > + if
Re: [PATCH v8 3/7] cryptodisk: enable the backends to implement detached headers
ср, 5 янв. 2022 г. в 01:07, Glenn Washburn : > On Tue, 4 Jan 2022 15:42:22 -0600 > Glenn Washburn wrote: > > I'm generally very pro-flexibility, but I'm not sure I like this from a > user perspective. For the common case, which is detached headers in a > file, this will cause the user to do more work (create a loopback > device of the file). What's a reasonable scenario where a user would > want the detached header on a device as opposed to a file system? Am I > correct in thinking that you use such functionality? > Actually no, I only use a file for the external header, not a disk. I have now looked at the patches again and will try to state my point of view in more detail: I don't think the hdr_file field as it stands in the patch set is relevant. I mean the hdr_file field of type grub_file_t in the grub_cryptomount_args structure. Even the grub_disk_t type may not be relevant here. You could only pass a header file name or a disk name (as char*) through this structure. This would reflect the essence of this structure, but further implementation the code will not be pretty in this case. I still suggest expanding the number of parameters for the recover_key function and use grub_disk_t to pass the header from the user directly. I'll leave it here for your reference: https://gitlab.com/reagentoo/grub/-/blob/0921badcf817071639058bc4a534c8e6bd05f212/grub-core/disk/luks2.c#L545 header_source is equal source - if we are not using external header > > I do like that it simplifies the code. An alternative route to get the > same simplification while still being given a file path as an argument > to cryptomount would be to internally create a loopback device and use > that instead of passing a file around. A possible undesirabe > side-effect, this might require being dependent on the loopback module. > > Glenn > > > > > > }; > > > typedef struct grub_cryptomount_args *grub_cryptomount_args_t; > > > > > > diff --git a/include/grub/file.h b/include/grub/file.h > > > index 31567483c..3a3c49a04 100644 > > > --- a/include/grub/file.h > > > +++ b/include/grub/file.h > > > @@ -90,6 +90,8 @@ enum grub_file_type > > > GRUB_FILE_TYPE_FONT, > > > /* File holding encryption key for encrypted ZFS. */ > > > GRUB_FILE_TYPE_ZFS_ENCRYPTION_KEY, > > > +/* File holding the encryption metadata header */ > > > +GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER, > > > /* File we open n grub-fstest. */ > > > GRUB_FILE_TYPE_FSTEST, > > > /* File we open n grub-mount. */ > > > > Dmitry > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v8 3/7] cryptodisk: enable the backends to implement detached headers
ср, 5 янв. 2022 г. в 01:57, Dmitry : > > > ср, 5 янв. 2022 г. в 01:07, Glenn Washburn : > >> On Tue, 4 Jan 2022 15:42:22 -0600 >> Glenn Washburn wrote: >> >> I'm generally very pro-flexibility, but I'm not sure I like this from a >> user perspective. For the common case, which is detached headers in a >> file, this will cause the user to do more work (create a loopback >> device of the file). What's a reasonable scenario where a user would >> want the detached header on a device as opposed to a file system? Am I >> correct in thinking that you use such functionality? >> > > Actually no, I only use a file for the external header, not a disk. > I have now looked at the patches again and will try to state my point of > view in > more detail: > > I don't think the hdr_file field as it stands in the patch set is > relevant. I mean > the hdr_file field of type grub_file_t in the grub_cryptomount_args > structure. > Even the grub_disk_t type may not be relevant here. You could only pass > a header file name or a disk name (as char*) through this structure. This > would > reflect the essence of this structure, but further implementation the code > will > not be pretty in this case. > > I still suggest expanding the number of parameters for the recover_key > function > and use grub_disk_t to pass the header from the user directly. > Although in general I'm quite satisfied with the current patch set. It suits my requirements. Maybe disk may be really useless and I overdid it.. It will only remain to add the master key parameter in the future. ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v8 3/7] cryptodisk: enable the backends to implement detached headers
ср, 5 янв. 2022 г. в 02:30, Dmitry : > > > ср, 5 янв. 2022 г. в 01:57, Dmitry : > >> >> >> ср, 5 янв. 2022 г. в 01:07, Glenn Washburn : >> >>> On Tue, 4 Jan 2022 15:42:22 -0600 >>> Glenn Washburn wrote: >>> >>> I'm generally very pro-flexibility, but I'm not sure I like this from a >>> user perspective. For the common case, which is detached headers in a >>> file, this will cause the user to do more work (create a loopback >>> device of the file). What's a reasonable scenario where a user would >>> want the detached header on a device as opposed to a file system? Am I >>> correct in thinking that you use such functionality? >>> >> >> Actually no, I only use a file for the external header, not a disk. >> I have now looked at the patches again and will try to state my point of >> view in >> more detail: >> >> I don't think the hdr_file field as it stands in the patch set is >> relevant. I mean >> the hdr_file field of type grub_file_t in the grub_cryptomount_args >> structure. >> Even the grub_disk_t type may not be relevant here. You could only pass >> a header file name or a disk name (as char*) through this structure. This >> would >> > So, please ignore these statements. Looks like it's not valid. > reflect the essence of this structure, but further implementation the code >> will >> not be pretty in this case. >> >> I still suggest expanding the number of parameters for the recover_key >> function >> and use grub_disk_t to pass the header from the user directly. >> > > Although in general I'm quite satisfied with the current patch set. It > suits my > requirements. Maybe disk may be really useless and I overdid it.. It will > only > remain to add the master key parameter in the future. > > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v8 3/7] cryptodisk: enable the backends to implement detached headers
On Wed, 5 Jan 2022 02:50:36 +0300 Dmitry wrote: > ср, 5 янв. 2022 г. в 02:30, Dmitry : > > > > > > > ср, 5 янв. 2022 г. в 01:57, Dmitry : > > > >> > >> > >> ср, 5 янв. 2022 г. в 01:07, Glenn Washburn : > >> > >>> On Tue, 4 Jan 2022 15:42:22 -0600 > >>> Glenn Washburn wrote: > >>> > >>> I'm generally very pro-flexibility, but I'm not sure I like this from a > >>> user perspective. For the common case, which is detached headers in a > >>> file, this will cause the user to do more work (create a loopback > >>> device of the file). What's a reasonable scenario where a user would > >>> want the detached header on a device as opposed to a file system? Am I > >>> correct in thinking that you use such functionality? > >>> > >> > >> Actually no, I only use a file for the external header, not a disk. > >> I have now looked at the patches again and will try to state my point of > >> view in > >> more detail: > >> > >> I don't think the hdr_file field as it stands in the patch set is > >> relevant. I mean > >> the hdr_file field of type grub_file_t in the grub_cryptomount_args > >> structure. > >> Even the grub_disk_t type may not be relevant here. You could only pass > >> a header file name or a disk name (as char*) through this structure. This > >> would > >> > > > So, please ignore these statements. Looks like it's not valid. I still like the idea of not having to conditionally choose to use the disk vs. file api for reading the header. I think it would be nice for the -H argument to be either a file or a device. It seems to me the most logical place for this to be handled is in the cryptomount arg handling. If a file is passed, setup a loopback device and pass that as the header, otherwise if its a device, just open it and pass it along. This would make cryptodisk module dependent on the loopback module, which I don't particularly like and may not be acceptable to others. Dnaiel do you have an opinion about this? Also, I've looked over the code again and I don't think the benefit of having detached headers be disk internally is that great at this point either. If more cryptodisk backends get added that support detached headers, then the benefit will increase. I definitely don't want to require that a disk device be passed in as the detached header argument to cryptomount. So I think the current approach is acceptable and further down the road someone can propose a patch to go in the direction you suggest. If you have an alternative proposal that I'm not thinking of, I'm more than willing to hear it out and modify these patches if it sounds good. Glenn > > reflect the essence of this structure, but further implementation the code > >> will > >> not be pretty in this case. > >> > >> I still suggest expanding the number of parameters for the recover_key > >> function > >> and use grub_disk_t to pass the header from the user directly. > >> > > > > Although in general I'm quite satisfied with the current patch set. It > > suits my > > requirements. Maybe disk may be really useless and I overdid it.. It will > > only > > remain to add the master key parameter in the future. > > > > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
