Re: [PATCH v2 7/8] tests: Extend sign_verify test with pkcs11-specific test
From: Mimi Zohar <zohar@linux.ibm.com>
Date: 2021-09-03 19:11:41
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.
quoted hunk ↗ jump to hunk
--- 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_setupdiff --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}"
+ fiShould there be a test checking that softhsm_setup is installed before using it? If it's not installed, then the test is "skipped".
quoted hunk ↗ jump to hunk
+}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