Re: [PATCH v2 7/8] tests: Extend sign_verify test with pkcs11-specific test

[Mimi Zohar <zohar@linux.ibm.com>]

Hi Stefan,

On Tue, 2021-08-10 at 09:45 -0400, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
> 
> 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@linux.ibm.com>

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