Re: [PATCH ima-evm-utils v6] ima-evm-utils: Support SM2/3 algorithm for sign and verify

[Tianjia Zhang <tianjia.zhang@linux.alibaba.com>]

Hi Petr,

On 7/20/21 9:41 PM, Petr Vorel wrote:
> Hi Tianjia,
> 
> few notes below, feel free to completely ignore it.
> 
> ...
>> diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
>> index 088c041..b890481 100644
>> --- a/.github/workflows/ci.yml
>> +++ b/.github/workflows/ci.yml
>> @@ -17,6 +17,7 @@ jobs:
>>                 ARCH: i386
>>                 TSS: tpm2-tss
>>                 VARIANT: i386
>> +              COMPILE_SSL: openssl-3
> I'd either put here value openssl-3.0.0-beta1 and pass it to
> ./tests/install-openssl3.sh or put value as true. Because why define version in
> yaml and also in the script? (sooner or later these two will not match).
> 

This is a good way, I will follow you.

>>             # cross compilation builds
>>             - container: "debian:stable"
>> @@ -51,6 +52,7 @@ jobs:
>>               env:
>>                 CC: clang
>>                 TSS: ibmtss
>> +              COMPILE_SSL: openssl-3
> 
>>             - container: "opensuse/leap"
>>               env:
>> @@ -61,6 +63,7 @@ jobs:
>>               env:
>>                 CC: gcc
>>                 TSS: ibmtss
>> +              COMPILE_SSL: openssl-3
> 
>>             - container: "ubuntu:xenial"
>>               env:
>> @@ -115,6 +118,7 @@ jobs:
>>           INSTALL="${INSTALL%%/*}"
>>           if [ "$VARIANT" ]; then ARCH="$ARCH" ./ci/$INSTALL.$VARIANT.sh; fi
>>           ARCH="$ARCH" CC="$CC" TSS="$TSS" ./ci/$INSTALL.sh
>> +        if [ "$COMPILE_SSL" ]; then ./tests/install-openssl3.sh; fi
> 
>>       - name: Build swtpm
>>         run: |
>> @@ -128,5 +132,8 @@ jobs:
>>       - name: Compiler version
>>         run: $CC --version
> 
>> +    - name: Default OpenSSL version
>> +      run: openssl version
> you should run this only on native build:
> 
>           run: [ "$VARIANT" != "cross-compile" ] && openssl version
> 
> Also aren't ve interested at the version which is actually being used for
> compilation?
> 
> Also we don't print this info for Travis CI.
> 

It's just to facilitate troubleshooting when CI fails. This is not 
necessary, and it seems useless now, I will delete.

>> +
>>       - name: Compile
>>         run: CC="$CC" VARIANT="$VARIANT" ./build.sh
>> diff --git a/.travis.yml b/.travis.yml
>> index 7a76273..a73f20e 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -9,7 +9,7 @@ matrix:
>>       include:
>>           # 32 bit build
>>           - os: linux
>> -          env: DISTRO=debian:stable VARIANT=i386 ARCH=i386 TSS=tpm2-tss
>> +          env: DISTRO=debian:stable VARIANT=i386 ARCH=i386 TSS=tpm2-tss COMPILE_SSL: openssl-3
>>             compiler: gcc
> 
>>           # cross compilation builds
>> @@ -32,7 +32,7 @@ matrix:
> 
>>           # glibc (gcc/clang)
>>           - os: linux
>> -          env: DISTRO=opensuse/tumbleweed TSS=ibmtss CONTAINER=podman CONTAINER_ARGS="--runtime=/usr/bin/runc --network=host"
>> +          env: DISTRO=opensuse/tumbleweed TSS=ibmtss CONTAINER=podman CONTAINER_ARGS="--runtime=/usr/bin/runc --network=host" COMPILE_SSL: openssl-3
>>             compiler: clang
> 
>>           - os: linux
>> @@ -40,7 +40,7 @@ matrix:
>>             compiler: gcc
> 
>>           - os: linux
>> -          env: DISTRO=ubuntu:groovy TSS=ibmtss
>> +          env: DISTRO=ubuntu:groovy TSS=ibmtss COMPILE_SSL: openssl-3
>>             compiler: gcc
> 
>>           - os: linux
>> @@ -97,4 +97,4 @@ before_install:
>>   script:
>>       - INSTALL="${DISTRO%%:*}"
>>       - INSTALL="${INSTALL%%/*}"
>> -    - $CONTAINER run $CONTAINER_ARGS -t ima-evm-utils /bin/sh -c "if [ \"$VARIANT\" ]; then ARCH=\"$ARCH\" ./ci/$INSTALL.$VARIANT.sh; fi && ARCH=\"$ARCH\" CC=\"$CC\" TSS=\"$TSS\" ./ci/$INSTALL.sh && if [ ! \"$VARIANT\" ]; then which tpm_server || which swtpm || if which tssstartup; then ./tests/install-swtpm.sh; fi; fi && CC=\"$CC\" VARIANT=\"$VARIANT\" ./build.sh"
>> +    - $CONTAINER run $CONTAINER_ARGS -t ima-evm-utils /bin/sh -c "if [ \"$VARIANT\" ]; then ARCH=\"$ARCH\" ./ci/$INSTALL.$VARIANT.sh; fi && ARCH=\"$ARCH\" CC=\"$CC\" TSS=\"$TSS\" ./ci/$INSTALL.sh && if [ "$COMPILE_SSL" ]; then ./tests/install-openssl3.sh; fi && if [ ! \"$VARIANT\" ]; then which tpm_server || which swtpm || if which tssstartup; then ./tests/install-swtpm.sh; fi; fi && CC=\"$CC\" VARIANT=\"$VARIANT\" ./build.sh"
>> diff --git a/src/libimaevm.c b/src/libimaevm.c
>> index 19f1041..8e96157 100644
> ...
>> --- a/tests/gen-keys.sh
>> +++ b/tests/gen-keys.sh
>> @@ -131,6 +131,31 @@ for m in \
>>       fi
>>   done
> 
>> +# SM2, If openssl 3.0 is installed, gen SM2 keys using
>> +if [ -x /opt/openssl3/bin/openssl ]; then
>> +  (PATH=/opt/openssl3/bin:$PATH LD_LIBRARY_PATH=/opt/openssl3/lib
>> +  for curve in sm2; do
> I'd just export PATH and LD_LIBRARY_PATH than wrap them in ().

Direct export PATH and LD_LIBRARY_PATH will affect the following 
commands. If add other commands after this code block in the future, it 
will cause potential problems. Of course, this file will not affect it 
now. If you export PATH and LD_LIBRARY_PATH directly in sign_verify.test 
script, there will cause error.

>> +    if [ "$1" = clean ] || [ "$1" = force ]; then
>> +      rm -f test-$curve.cer test-$curve.key test-$curve.pub
>> +    fi
>> +    if [ "$1" = clean ]; then
>> +      continue
>> +    fi
>> +    if [ ! -e test-$curve.key ]; then
>> +      log openssl req -verbose -new -nodes -utf8 -days 10000 -batch -x509 \
>> +        -sm3 -sigopt "distid:1234567812345678" \
>> +        -config test-ca.conf \
>> +        -copy_extensions copyall \
>> +        -newkey $curve \
>> +        -out test-$curve.cer -outform DER \
>> +        -keyout test-$curve.key
>> +      if [ -s test-$curve.key ]; then
>> +        log openssl pkey -in test-$curve.key -out test-$curve.pub -pubout
>> +      fi
>> +    fi
>> +  done)
>> +fi
> ...
> 
>> --- /dev/null
>> +++ b/tests/install-openssl3.sh
>> @@ -0,0 +1,17 @@
>> +#!/bin/sh

will change to #!/bin/bash

>> +
>> +set -ex
>> +
>> +# 3.0.0-beta1 is the latest version in July 2021
> I'd define a variable and use it.
> version="openssl-3.0.0-beta1"
> 

Good way.

> 
> Kind regards,
> Petr
> 
>> +wget --no-check-certificate https://github.com/openssl/openssl/archive/refs/tags/openssl-3.0.0-beta1.tar.gz
>> +tar --no-same-owner -xzf openssl-3.0.0-beta1.tar.gz
>> +cd openssl-openssl-3.0.0-beta1
>> +
>> +./Configure --prefix=/opt/openssl3 --openssldir=/opt/openssl3/ssl
>> +make -j$(nproc)
>> +# only install apps and library
>> +sudo make install_sw
>> +
>> +cd ..
>> +rm -rf openssl-3.0.0-beta1.tar.gz
>> +rm -rf openssl-openssl-3.0.0-beta1
> ...
> 

Best regards,
Tianjia