ima-evm-utils: Add some tests for evmctl

Run `make check' to execute the tests.
Currently only ima_hash, ima_sign, and ima_verify are tested.

Signed-off-by Vitaly Chikunov <[email protected]>

....

tests/functions.sh new file +9 -0

+_evmctl_run() {
+  local cmd=$1 out=$1-$$.out
+  # Additional parameters:
+  # FOR: append to text as 'for $FOR'
+  # DEL: additional files to rm if test failed
+
+  set -- evmctl $V ${ENGINE:+--engine $ENGINE} "$@"
+  echo $YELLOW$TMODE $*$NORM
+  eval "$@" >$out 2>&1

Run `make check' to execute the tests.
Currently only ima_hash, ima_sign, and ima_verify are tested.

Signed-off-by Vitaly Chikunov <[email protected]>

---
Changelog since v1:
- Apply suggestions by Petr Vorel:
- Rename function names and variables to be more understandable.
- Rename tests/functions to tests/functions.sh.
- Define exit codes (77, 99, ...) as variables.
- Added more comments and remove single letter variables (for Mimi Zohar).

- Move getfattr check into function.
- Move evmctl run and check into single function.
- Add sign/verify tests for v1 signatures.
- Minor improvements.
- Since I still edit all 5 files I did not split the patch into multiple commits to separate the files, otherwise editing will become too complicated, as I ought to continuously rebase and edit different commits. This was really non-productive suggestion

Please test the tests.

 Makefile.am           |   2 +-
 configure.ac          |   1 +
 tests/Makefile.am     |  15 ++++
 tests/functions.sh    | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/gen-keys.sh     |  90 +++++++++++++++++++++
 tests/ima_hash.test   |  72 +++++++++++++++++
 tests/ima_sign.test   | 211 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/ima_verify.test |  93 +++++++++++++++++++++
 8 files changed, 701 insertions(+), 1 deletion(-)
 create mode 100644 tests/Makefile.am
 create mode 100755 tests/functions.sh
 create mode 100755 tests/gen-keys.sh
 create mode 100755 tests/ima_hash.test
 create mode 100755 tests/ima_sign.test
 create mode 100755 tests/ima_verify.test

tests/Makefile.am new file +5 -0

new file mode 100644
index 0000000..145ea30
@@ -0,0 +1,15 @@
+check_SCRIPTS =
+TESTS = $(check_SCRIPTS)
+
+check_SCRIPTS += ima_hash.test ima_verify.test ima_sign.test
+

tests/functions.sh new file +6 -0

new file mode 100755
index 0000000..8794120
@@ -0,0 +1,218 @@
+#!/bin/bash
+#
+# ima-evm-utils tests bash functions
+#
+# Copyright (C) 2019 Vitaly Chikunov <[email protected]>
+#

tests/functions.sh new file +9 -0

+# tests accounting
+declare -i testspass=0 testsfail=0 testsskip=0/
+
+# exit codes (compatible with automake)
+declare -r OK=0
+declare -r FAIL=1
+declare -r HARDFAIL=99 # hard failure no matter testing mode
+declare -r SKIP=77     # skip test
+

tests/functions.sh new file +13 -0

+      echo "$i is required for test"
+      ret=1
+    fi
+  done
+  [ $ret ] && exit $HARDFAIL
+}
+
+# only allow color output on tty
+if tty -s; then
+     RED=$'\e[1;31m'
+   GREEN=$'\e[1;32m'
+  YELLOW=$'\e[1;33m'
+    BLUE=$'\e[1;34m'

tests/functions.sh new file +27 -0

+
+# Test mode determined by TNEG variable:
+#   undefined: to positive testing
+#   defined: negative testing
+TNEG=
+TMODE=+
+
+# Eval positive test and account its result
+pos() {
+  TNEG= TMODE=+
+  [ "$VERBOSE" ] && echo "Start positive test $*"
+  eval "$@"
+  E=$?
+  [ "$VERBOSE" ] && echo "Stop ($E) positive test $*"
+  case $E in
+    0)  testspass+=1 ;;
+    77) testsskip+=1 ;;
+    99) testsfail+=1; exit_early 1 ;;
+    *)  testsfail+=1; exit_early 2 ;;
+  esac
+}
+
+# Eval negative test and accoutn its result
+neg() {
+  TNEG=1 TMODE=-
+  [ "$VERBOSE" ] && echo "Start negative test $*"
+  eval "$@"

tests/functions.sh new file +21 -0

+# return true if current test is positive
+_is_positive_test() {
+  [ -z "$TNEG" ]
+}
+
+# return true if current test is negative
+_is_negative_test() {
+  [ "$TNEG" ]
+}
+
+# Color following text to red if it's real error
+red_if_pos() {
+  _is_positive_test && echo $@ $RED
+}
+
+norm_if_pos() {
+  _is_positive_test && echo $@ $NORM
+}
+
+DEL=
+FOR=

tests/functions.sh new file +55 -0

+  ret=$?
+
+  if [ $ret -ge 126 -a $ret -lt 255 ]; then
+    echo $RED
+    echo "evmctl $cmd failed hard with ($ret) ${FOR:+for $FOR}"
+    sed 's/^/  /' $out
+    echo $NORM
+    rm $out $DEL
+    FOR= DEL=
+    return $SKIP
+  elif [ $ret -gt 0 ]; then
+    red_if_pos
+    echo "evmctl $cmd failed" ${TNEG:+properly} "with ($ret) ${FOR:+for $FOR}"
+    sed 's/^/  /' $out
+    norm_if_pos
+    rm $out $DEL
+    FOR= DEL=
+    return $FAIL
+  elif _is_negative_test; then
+    echo $RED
+    echo "evmctl $cmd wrongly succeeded ${FOR:+for $FOR}"
+    sed 's/^/  /' $out
+    echo $NORM
+  else
+    [ "$VERBOSE" ] && sed 's/^/  /' $out
+  fi
+  rm $out
+  FOR= DEL=
+  return $OK
+}
+
+_extract_ima_xattr() {
+  local file=$1 out=$2 pref=$3
+
+  getfattr -n user.ima -e hex $file \
+    | grep ^user.ima= \
+    | sed s/^user.ima=$pref// \
+    | xxd -r -p > $out
+}
+
+_test_ima_xattr() {
+  local file=$1 pref=$2
+
+  if ! getfattr -n user.ima -e hex $file | egrep -qx "user.ima=$pref"; then
+    red_if_pos
+    echo "Did not find expected hash${FOR:+ for $FOR}:"
+    echo "    user.ima=$pref"
+    echo ""
+    echo "Actual output below:"
+    getfattr -n user.ima -e hex $file | sed 's/^/    /'
+    norm_if_pos
+    rm $file
+    FOR=
+    return $FAIL
+  fi

tests/functions.sh new file +25 -0

+
+# Show test stats and exit into automake test system
+# with proper exit code (same as ours).
+_report_exit() {
+  if [ $testsfail -gt 0 ]; then
+    echo "=============================="
+    echo "Run with FAILEARLY=1 $0 $@"
+    echo "To stop after first failure"
+    echo "=============================="
+  fi
+  [ $testspass -gt 0 ] && echo -n $GREEN || echo -n $NORM
+  echo -n "PASS: $testspass"
+  [ $testsskip -gt 0 ] && echo -n $YELLOW || echo -n $NORM
+  echo -n " SKIP: $testsskip"
+  [ $testsfail -gt 0 ] && echo -n $RED || echo -n $NORM
+  echo " FAIL: $testsfail"
+  echo $NORM
+  if [ $testsfail -gt 0 ]; then
+    exit $FAIL
+  elif [ $testspass -gt 0 ]; then
+    exit $OK
+  else
+    exit $SKIP
+  fi
+}

tests/ima_hash.test new file +13 -0

new file mode 100755
index 0000000..d4bfe19
@@ -0,0 +1,72 @@
+#!/bin/bash
+#
+# evmctl ima_hash tests
+#
+# Copyright (C) 2019 Vitaly Chikunov <[email protected]>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2, or (at your option)
+# any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of

tests/ima_hash.test new file +6 -0

+    _evmctl_run ima_hash --hashalgo $alg --xattr-user $file || return
+  FOR=$alg \
+    _test_ima_xattr $file $pref$hash || return
+  rm $file
+  return $OK
+}

tests/ima_hash.test new file +21 -0

+pos check md4    0x01
+pos check md5    0x01
+pos check sha1   0x01
+neg check SHA1   0x01 # uppercase
+neg check sha512-224 0x01 # valid for pkcs1
+neg check sha512-256 0x01 # valid for pkcs1
+neg check unknown 0x01 # nonexistent
+pos check sha224 0x0407
+pos check sha256 0x0404
+pos check sha384 0x0405
+pos check sha512 0x0406
+pos check rmd160 0x0403
+neg check sm3     0x01
+neg check sm3-256 0x01
+_enable_gost_engine
+pos check md_gost12_256 0x0412
+pos check streebog256   0x0412
+pos check md_gost12_512 0x0413
+pos check streebog512   0x0413
+
+_report_exit

Quoted 4 lines · Vitaly Chikunov

- Since I still edit all 5 files I did not split the patch into multiple commits to separate the files, otherwise editing will become too complicated, as I ought to continuously rebase and edit different commits. This was really non-productive suggestion

Ok, but the review will be broken up.  For now, the comments below are limited to tests/Makefile.am, tests/functions.sh and tests/ima_hash.test.  Some of the comments are intrusive, so I'm going to hold off on reviewing the other tests.

Autotools generates "test-driver".  Should it be added to git-ignore?

Should we be using SPDX, at least for new files?

Quoted 1 line

+ if ! type $i; then

"type" is a bashism.

tests/functions.sh line 58

Quoted 6 lines

tests/functions.sh new file +6 -0

+# Define FAILEARLY to exit testing on the first error.
+exit_early() {
+  if [ $FAILEARLY ]; then
+    exit $1
+  fi
+}

I would group all of the environment variable function checking together at the top of functions.sh.

The functions "pos" and "neg" are written very concisely, but they are part of a common set of functions, which are the crux of the tests scripts.  I'm really hesitant about having common functions that execute any command passed to it, without any form of verification.

tests/functions.sh line 127

Quoted 3 lines

tests/functions.sh new file +3 -0

+  set -- evmctl $V ${ENGINE:+--engine $ENGINE} "$@"
+  echo $YELLOW$TMODE $*$NORM
+  eval "$@" >$out 2>&1

Here at least the command is limited to "evmctl".

Is there any benefit to using "set --", as opposed to defining a local variable and executing it?  Is this simply a question of style?

tests/functions.sh line 192

Quoted 7 lines

tests/functions.sh new file +7 -0

+_enable_gost_engine() {
+  # Do not enable if it's already working (enabled by user)
+  if ! openssl md_gost12_256 /dev/null >/dev/null 2>&1 \
+    && openssl engine gost >/dev/null 2>&1; then
+    ENGINE=gost
+  fi
+}

With gost provided as an Openssl engine, is it possible to be able to execute the first command without the gost engine enabled?  With commit 782224f33cd7 ("ima-evm-utils: Rework openssl init"),

I'm now wondering if the "--engine e' option is still needed?

tests/ima_hash.test line 23

Quoted 2 lines

tests/ima_hash.test new file +2 -0

+# Check with constant
+check_const() {

This function comment doesn't provide any more details than the function name.  Please either rename this function (eg. check_xattr) or expand the function comment.

tests/ima_hash.test

Quoted 3 lines

tests/ima_hash.test new file +3 -0

+  local alg=$1 pref=$2 hash=$3 file=$4
+
+  FOR=$alg DEL=$file 

Why not use ALG=$alg and FILE=$file as the global variable names?

tests/ima_hash.test line 42

Quoted 3 lines

tests/ima_hash.test new file +3 -0

+  cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$alg $file"
+  echo - $cmd
+  hash=$(set -o pipefail; eval "$cmd" 2>/dev/null | cut -d' ' -f2)

Is there a reason for not executing $cmd directly?  Is it safer calling "pipefail" and "eval"?  Is this a question of style?

Quoted 6 lines

tests/ima_hash.test new file +6 -0

+  if [ $? -ne 0 ] && _is_positive_test; then
+    echo $CYAN"$alg test is skipped"$NORM
+    rm $file
+    return $SKIP
+  fi
+  check_const $alg $pref "$hash" $file

tests/ima_hash.test line 51

Quoted 3 lines

tests/ima_hash.test new file +3 -0

+}
+
+# check args: algo prefix hex-hash

The first keyword - test type - is missing in the comment above.  It would be clearer if instead of "pos" or "neg", the key words included the words "pass" and "fail", to indicate that the test is expected to pass or fail.

Mimi,

Quoted 10 lines · Mimi Zohar

Quoted 4 lines · Vitaly Chikunov

- Since I still edit all 5 files I did not split the patch into multiple commits to separate the files, otherwise editing will become too complicated, as I ought to continuously rebase and edit different commits. This was really non-productive suggestion

Ok, but the review will be broken up.  For now, the comments below are limited to tests/Makefile.am, tests/functions.sh and tests/ima_hash.test.  Some of the comments are intrusive, so I'm going to hold off on reviewing the other tests.

This is good, since I am reworking ima_sign/ima_verify tests into a single test that will also cover EVM sign/verify.

Quoted 1 line

Autotools generates "test-driver".  Should it be added to git-ignore?

Didn't notice this.

Quoted 1 line

Should we be using SPDX, at least for new files?

OK.

Quoted 3 lines

Quoted 1 line

+ if ! type $i; then

"type" is a bashism.

Tests are on bash.

Quoted 9 lines

tests/functions.sh line 58

Quoted 6 lines

tests/functions.sh new file +6 -0

+# Define FAILEARLY to exit testing on the first error.
+exit_early() {
+  if [ $FAILEARLY ]; then
+    exit $1
+  fi
+}

I would group all of the environment variable function checking together at the top of functions.sh.

Some functions check VERBOSE should they be on top too?

Or you meant this is just variable checking function? It isn't.

Quoted 5 lines

The functions "pos" and "neg" are written very concisely, but they are part of a common set of functions, which are the crux of the tests scripts.  I'm really hesitant about having common functions that execute any command passed to it, without any form of verification.

What verification and why?

Quoted 6 lines

tests/functions.sh line 127

Quoted 4 lines

tests/functions.sh new file +3 -0

+  set -- evmctl $V ${ENGINE:+--engine $ENGINE} "$@"
+  echo $YELLOW$TMODE $*$NORM
+  eval "$@" >$out 2>&1

Here at least the command is limited to "evmctl".

This is emvctl runner. pos/neg can and should run anything that needs their exit code be checked and accounted as test result.

Quoted 2 lines

Is there any benefit to using "set --", as opposed to defining a local variable and executing it?  Is this simply a question of style?

I will make it using variable.

Quoted 11 lines

tests/functions.sh line 192

Quoted 7 lines

tests/functions.sh new file +7 -0

+_enable_gost_engine() {
+  # Do not enable if it's already working (enabled by user)
+  if ! openssl md_gost12_256 /dev/null >/dev/null 2>&1 \
+    && openssl engine gost >/dev/null 2>&1; then
+    ENGINE=gost
+  fi
+}

With gost provided as an Openssl engine, is it possible to be able to execute the first command without the gost engine enabled?  With commit 782224f33cd7 ("ima-evm-utils: Rework openssl init"),

I don't understand question. What is 'first command'? openssl md_gost12_256 will not work if gost-engine is not configured somehow.

Quoted 2 lines

I'm now wondering if the "--engine e' option is still needed?

It's needed. Why you thinking it doesn't? Commit 782224f33cd7 will not load gost (or any other) engine on its own.

Quoted 7 lines

tests/ima_hash.test line 23

Quoted 3 lines

tests/ima_hash.test new file +2 -0

+# Check with constant
+check_const() {

This function comment doesn't provide any more details than the function name.  Please either rename this function (eg. check_xattr) or expand the function comment.

OK.

(check* was supposed to be top-level tests. I will change this in v3.)

Quoted 5 lines

tests/ima_hash.test

Quoted 3 lines

tests/ima_hash.test new file +3 -0

+  local alg=$1 pref=$2 hash=$3 file=$4
+
+  FOR=$alg DEL=$file 

Why not use ALG=$alg and FILE=$file as the global variable names?

check was called once for every algo. Are you proposing to change call like

check_const sha1 0x01 sha1-hash.txt
to
ALG=sha1 FILE=sha1-hash.txt
check_const 0x01
?

(I tried to put every mandatory argument into a argument list.)

Quoted 6 lines

tests/ima_hash.test line 42

Quoted 3 lines

tests/ima_hash.test new file +3 -0

+  cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$alg $file"
+  echo - $cmd
+  hash=$(set -o pipefail; eval "$cmd" 2>/dev/null | cut -d' ' -f2)

Is there a reason for not executing $cmd directly?  Is it safer calling "pipefail" and "eval"?  Is this a question of style?

I will remove eval (it also don't let me pass empty arguments into called functions). pipefail' is needed, so I can see exit code of $cmd and not of cut' in $?.

Quoted 6 lines

Quoted 6 lines

tests/ima_hash.test new file +6 -0

+  if [ $? -ne 0 ] && _is_positive_test; then
+    echo $CYAN"$alg test is skipped"$NORM
+    rm $file
+    return $SKIP
+  fi
+  check_const $alg $pref "$hash" $file

Quoted 8 lines

tests/ima_hash.test line 51

Quoted 3 lines

tests/ima_hash.test new file +3 -0

+}
+
+# check args: algo prefix hex-hash

The first keyword - test type - is missing in the comment above.  It would be clearer if instead of "pos" or "neg", the key words included the words "pass" and "fail", to indicate that the test is expected to pass or fail.

pass and fail looks like imperative statements, and not like something that will check other thing to pass or fail. I will rename them to something else.

Thanks for the review!

Quoted 6 lines · Vitaly Chikunov

Quoted 4 lines

The functions "pos" and "neg" are written very concisely, but they are part of a common set of functions, which are the crux of the tests scripts.  I'm really hesitant about having common functions that execute any command passed to it, without any form of verification.

What verification and why?

Even though the tests are not running as root, it's still executing "$@", whatever that might be.  For ima_hash.test, the first argument is "check".

Quoted 12 lines

Quoted 11 lines

tests/functions.sh line 192

Quoted 7 lines

tests/functions.sh new file +7 -0

+_enable_gost_engine() {
+  # Do not enable if it's already working (enabled by user)
+  if ! openssl md_gost12_256 /dev/null >/dev/null 2>&1 \
+    && openssl engine gost >/dev/null 2>&1; then
+    ENGINE=gost
+  fi
+}

With gost provided as an Openssl engine, is it possible to be able to execute the first command without the gost engine enabled?  With commit 782224f33cd7 ("ima-evm-utils: Rework openssl init"),

Quoted 2 lines

I don't understand question. What is 'first command'? openssl md_gost12_256 will not work if gost-engine is not configured somehow.

Exactly.  "openssl md_gost12_256 /dev/null" (returns 0, but is negated) will succeed only if the engine is enabled.  The "openssl engine gost" test will never fail.

Quoted 4 lines

Quoted 1 line

I'm now wondering if the "--engine e' option is still needed?

It's needed. Why you thinking it doesn't? Commit 782224f33cd7 will not load gost (or any other) engine on its own.

If the gost engine is enabled in openssl.cnf then we don't need to set "ENGINE=gost".  I'm obviously missing something here.

Quoted 8 lines

(check* was supposed to be top-level tests. I will change this in v3.)

Quoted 5 lines

tests/ima_hash.test

Quoted 3 lines

tests/ima_hash.test new file +3 -0

+  local alg=$1 pref=$2 hash=$3 file=$4
+
+  FOR=$alg DEL=$file 

Why not use ALG=$alg and FILE=$file as the global variable names?

Quoted 2 lines

check was called once for every algo. Are you proposing to change call like

Although "FOR" is capitalized, I was reading it as "for".  It took me a while to realize that "FOR" and "DEL" are global variables being used in "_evmctl_run".  Anything you can do to make it easier to read would be appreciated.  Just adding comments would help.

While further testing, "_require evmctl openssl" should also make sure that getfattr is installed.