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

[Stefan Berger <stefanb@linux.ibm.com>]

On 9/3/21 3:11 PM, Mimi Zohar wrote:
> 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.

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


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

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


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


>
>> +}
>> 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
>