On 9/3/21 3:11 PM, Mimi Zohar wrote:

Hi Stefan,

On Tue, 2021-08-10 at 09:45 -0400, Stefan Berger wrote:

quoted

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.

Ok, softhsm_setup will be a separate patch then before function.sh and 
sign_verify.test are being patched.

quoted

---
  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.

I can add this.

quoted

+_softhsm_setup() {
+  local workdir="$1"
+

${WORKDIR} is being passed as a parameter.  Why is a local environment
variable needed?

I prefer to avoid accessing them when they can be passed to functions as 
parameters. I rather only use global variables at the 'top level' and 
then pass them down as parameters to all the lower level functions.

quoted

+  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".

softhsm_setup is being contributed to this project via the code above, 
so it should be available.

quoted

+}

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