Hi Stefan,
On Tue, 2021-08-10 at 09:45 -0400, Stefan Berger wrote:
> From: Stefan Berger <stefanb@xxxxxxxxxxxxx>
>
> Extend the sign_verify test with a pkcs11-specific test.
> Import softhsm_setup script from my swtpm project and contribute
> it to this porject under dual license BSD 3-clause and GLP 2.0.
>
> Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
Up to here, the patches were nicely split up. Just from reading the
patch description, this patch needs to be split up.
> ---
> tests/functions.sh | 26 ++++
> tests/sign_verify.test | 50 +++++--
> tests/softhsm_setup | 290 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 352 insertions(+), 14 deletions(-)
> create mode 100755 tests/softhsm_setup
>
> diff --git a/tests/functions.sh b/tests/functions.sh
> index 91cd5d9..cbb7ea4 100755
> --- a/tests/functions.sh
> +++ b/tests/functions.sh
> @@ -272,3 +272,29 @@ _report_exit() {
> fi
> }
>
> +_at_exit() {
> + _report_exit
> + if [ -n "${WORKDIR}" ]; then
> + rm -f "${WORKDIR}"
> + fi
> +}
> +
It would be nice to have a function comment here.
> +_softhsm_setup() {
> + local workdir="$1"
> +
${WORKDIR} is being passed as a parameter. Why is a local environment
variable needed?
> + local msg
> +
> + export SOFTHSM_SETUP_CONFIGDIR="${workdir}"
> + export SOFTHSM2_CONF="${workdir}/softhsm2.conf"
> +
> + msg=$(./softhsm_setup setup 2>&1)
> + if [ $? -eq 0 ]; then
> + echo "softhsm_setup setup succeeded: $msg"
> + PKCS11_KEYURI=$(echo $msg | sed -n 's|^keyuri: \(.*\)|\1|p')
> +
> + export OPENSSL_ENGINE="-engine pkcs11"
> + export OPENSSL_KEYFORM="-keyform engine"
> + else
> + echo "softhsm_setup setup failed: ${msg}"
> + fi
Should there be a test checking that softhsm_setup is installed before
using it? If it's not installed, then the test is "skipped".
> +}
> diff --git a/tests/sign_verify.test b/tests/sign_verify.test
> index 3b42eec..369765e 100755
> --- a/tests/sign_verify.test
> +++ b/tests/sign_verify.test
> @@ -28,7 +28,8 @@ fi
>
> ./gen-keys.sh >/dev/null 2>&1
>
> -trap _report_exit EXIT
> +trap _at_exit EXIT
> +WORKDIR=$(mktemp -d)
> set -f # disable globbing
>
> # Determine keyid from a cert
> @@ -132,11 +133,16 @@ check_sign() {
> # OPTS (additional options for evmctl),
> # FILE (working file to sign).
> local "$@"
> - local KEY=${KEY%.*}.key
> + local key verifykey
Agreed, don't modify the global variable, use a local one. Making this
a separate patch, would simplify review.
thanks,
Mimi
