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

[Petr Vorel <pvorel@suse.cz>]

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

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

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


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