[PATCH v8 0/2] ima-evm-utils: Add some tests for evmctl

[PATCH v8 0/2] ima-evm-utils: Add some tests for evmctl

[Vitaly Chikunov]

This series adds simple evmctl tests for hash, sign, and verify operations.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
Changelog since v7:
- Absence of key file cause test skip instead of fail.
- Mono patch is split into two by Mimi Zohar request.

Changelog since v6:
- ima_hash.test: Compare generated hashes with known values.
  I found that on power8 qemu with default -cpu calculates hashes wrongly.
  For exmaple: `openssl dgst -sha224 -hex /dev/null' results in
  83b2514951547ee00c7062fa3eb42079ff19f280ec81eedbdb0e3997 instead of
  d14a028c2a3a2bc9476102bb288234c415a2b01f828ea62ac5b3e42f.
- Make `_keyid' use temporary files to not rely on presence of `/des/stdout'.
- evm sign: add `--generation 0' option.
  I found that on overlay fs (with ext4 over 9p) FS_IOC_GETVERSION does
  not work.
- gen-keys.sh: allows `force' argument to regenerate all test keys.

Changelog since v5:
- Fix tests if gost-engine is not present. Thx to Mimi Zohar for testing on
  Xenial.

Changelog since v4:
- Fix bugs found by Mimi Zohar:
 - Fix typos in variable names
 - Fix `-out -' in asn1parse for openssl 1.0.x

Changelog since v3:
- Apply changes based on some suggestions from Petr Vorel, such as
- Add .gitignore
- Do not color output if stdout is not tty.
- Fix blkid error with --uuid option.
- Remove or change some comments
- Replace ENGINE with EVMCTL_ENGINE and OPENSSL_ENGINE.
- All tests pass over next branch.

Changelog since v2:
- ima_sign.test and ima_verify.test merged into sign_verify.test
  which is also able to test evm signatures.
- Apply Mimi Zohar suggestions regarding commenting, variable renaming,
  code rearranging, etc. with intent to simplify the code and review.

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.

Vitaly Chikunov (2):
  ima-evm-utils: Add some tests for evmctl
  ima-evm-utils: Add sign/verify tests for evmctl

 .gitignore             |   2 +-
 Makefile.am            |   2 +-
 configure.ac           |   1 +
 tests/.gitignore       |  16 +++
 tests/Makefile.am      |  12 ++
 tests/functions.sh     | 272 ++++++++++++++++++++++++++++++++++++
 tests/gen-keys.sh      |  97 +++++++++++++
 tests/ima_hash.test    |  80 +++++++++++
 tests/sign_verify.test | 364 +++++++++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 844 insertions(+), 2 deletions(-)
 create mode 100644 tests/.gitignore
 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/sign_verify.test

-- 
2.11.0

[PATCH v8 1/2] ima-evm-utils: Add some tests for evmctl

[Vitaly Chikunov]

Run `make check' to execute the tests.
This commit only adds ima_hash test.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 .gitignore          |   2 +-
 Makefile.am         |   2 +-
 configure.ac        |   1 +
 tests/.gitignore    |  16 ++++
 tests/Makefile.am   |   7 ++
 tests/functions.sh  | 272 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/ima_hash.test |  80 ++++++++++++++++
 7 files changed, 378 insertions(+), 2 deletions(-)
 create mode 100644 tests/.gitignore
 create mode 100644 tests/Makefile.am
 create mode 100755 tests/functions.sh
 create mode 100755 tests/ima_hash.test

diff --git a/.gitignore b/.gitignore
index cb82166..c579199 100644
--- a/.gitignore
+++ b/.gitignore
@@ -21,7 +21,7 @@ missing
 compile
 libtool
 ltmain.sh
-
+test-driver
 
 # Compiled executables
 *.o
diff --git a/Makefile.am b/Makefile.am
index dba408d..45c6f82 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,4 +1,4 @@
-SUBDIRS = src
+SUBDIRS = src tests
 dist_man_MANS = evmctl.1
 
 doc_DATA =  examples/ima-genkey-self.sh examples/ima-genkey.sh examples/ima-gen-local-ca.sh
diff --git a/configure.ac b/configure.ac
index 099c3b1..f246182 100644
--- a/configure.ac
+++ b/configure.ac
@@ -72,6 +72,7 @@ EVMCTL_MANPAGE_DOCBOOK_XSL
 
 AC_CONFIG_FILES([Makefile
 		src/Makefile
+		tests/Makefile
 		packaging/ima-evm-utils.spec
 		])
 AC_OUTPUT
diff --git a/tests/.gitignore b/tests/.gitignore
new file mode 100644
index 0000000..9ecc984
--- /dev/null
+++ b/tests/.gitignore
@@ -0,0 +1,16 @@
+# Generated by test driver
+*.log
+*.trs
+
+# Generated by tests
+*.txt
+*.out
+*.sig
+*.sig2
+
+# Generated certs and keys (by gen-keys.sh)
+*.cer
+*.pub
+*.key
+*.conf
+
diff --git a/tests/Makefile.am b/tests/Makefile.am
new file mode 100644
index 0000000..e37b958
--- /dev/null
+++ b/tests/Makefile.am
@@ -0,0 +1,7 @@
+check_SCRIPTS =
+TESTS = $(check_SCRIPTS)
+
+check_SCRIPTS += ima_hash.test
+
+clean-local:
+	-rm -f *.txt *.out *.sig *.sig2
diff --git a/tests/functions.sh b/tests/functions.sh
new file mode 100755
index 0000000..16c8d89
--- /dev/null
+++ b/tests/functions.sh
@@ -0,0 +1,272 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# ima-evm-utils tests bash functions
+#
+# Copyright (C) 2019 Vitaly Chikunov <vt@altlinux.org>
+#
+# 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
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# 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
+
+# You can set env VERBOSE=1 to see more output from evmctl
+V=vvvv
+V=${V:0:$VERBOSE}
+V=${V:+-$V}
+
+# Exit if env FAILEARLY is defined.
+# Used in expect_{pass,fail}.
+exit_early() {
+  if [ $FAILEARLY ]; then
+    exit $1
+  fi
+}
+
+# Require particular executables to be present
+_require() {
+  ret=
+  for i; do
+    if ! type $i; then
+      echo "$i is required for test"
+      ret=1
+    fi
+  done
+  [ $ret ] && exit $HARDFAIL
+}
+
+# Only allow color output on tty
+if [ -t 1 ]; then
+     RED=$'\e[1;31m'
+   GREEN=$'\e[1;32m'
+  YELLOW=$'\e[1;33m'
+    BLUE=$'\e[1;34m'
+    CYAN=$'\e[1;36m'
+    NORM=$'\e[m'
+fi
+
+# Test mode determined by TFAIL variable:
+#   undefined: to success testing
+#   defined: failure testing
+TFAIL=
+TMODE=+ # mode character to prepend running command in log
+declare -i TNESTED=0 # just for sanity checking
+
+# Run positive test (one that should pass) and account its result
+expect_pass() {
+  local ret
+
+  if [ $TNESTED -gt 0 ]; then
+    echo $RED"expect_pass should not be run nested"$NORM
+    testsfail+=1
+    exit $HARDFAIL
+  fi
+  TFAIL=
+  TMODE=+
+  TNESTED+=1
+  [[ "$VERBOSE" -gt 1 ]] && echo "____ START positive test: $@"
+  "$@"
+  ret=$?
+  [[ "$VERBOSE" -gt 1 ]] && echo "^^^^ STOP ($ret) positive test: $@"
+  TNESTED+=-1
+  case $ret in
+    0)  testspass+=1 ;;
+    77) testsskip+=1 ;;
+    99) testsfail+=1; exit_early 1 ;;
+    *)  testsfail+=1; exit_early 2 ;;
+  esac
+  return $ret
+}
+
+# Eval negative test (one that should fail) and account its result
+expect_fail() {
+  local ret
+
+  if [ $TNESTED -gt 0 ]; then
+    echo $RED"expect_fail should not be run nested"$NORM
+    testsfail+=1
+    exit $HARDFAIL
+  fi
+
+  TFAIL=yes
+  TMODE=-
+  TNESTED+=1
+  [[ "$VERBOSE" -gt 1 ]] && echo "____ START negative test: $@"
+  "$@"
+  ret=$?
+  [[ "$VERBOSE" -gt 1 ]] && echo "^^^^ STOP ($ret) negative test: $@"
+  TNESTED+=-1
+  case $ret in
+    0)  testsfail+=1; exit_early 3 ;;
+    77) testsskip+=1 ;;
+    99) testsfail+=1; exit_early 4 ;;
+    *)  testspass+=1 ;;
+  esac
+  # Restore defaults (as in positive tests)
+  # for tests to run without wrappers
+  TFAIL=
+  TMODE=+
+  return $ret
+}
+
+# return true if current test is positive
+_is_expect_pass() {
+  [ ! $TFAIL ]
+}
+
+# return true if current test is negative
+_is_expect_fail() {
+  [ $TFAIL ]
+}
+
+# Show blank line and color following text to red
+# if it's real error (ie we are in expect_pass mode).
+red_if_failure() {
+  if _is_expect_pass; then
+    echo $@ $RED
+    COLOR_RESTORE=1
+  fi
+}
+
+# For hard errors
+red_always() {
+  echo $@ $RED
+  COLOR_RESTORE=1
+}
+
+color_restore() {
+  [ $COLOR_RESTORE ] && echo $@ $NORM
+  COLOR_RESTORE=
+}
+
+ADD_DEL=
+ADD_TEXT_FOR=
+# _evmctl_run should be run as `_evmctl_run ... || return'
+_evmctl_run() {
+  local op=$1 out=$1-$$.out
+  local text_for=${FOR:+for $ADD_TEXT_FOR}
+  # Additional parameters:
+  # ADD_DEL: additional files to rm on failure
+  # ADD_TEXT_FOR: append to text as 'for $ADD_TEXT_FOR'
+
+  cmd="evmctl $V $EVMCTL_ENGINE $@"
+  echo $YELLOW$TMODE $cmd$NORM
+  $cmd >$out 2>&1
+  ret=$?
+
+  # Shell special and signal exit codes (except 255)
+  if [ $ret -ge 126 -a $ret -lt 255 ]; then
+    red_always
+    echo "evmctl $op failed hard with ($ret) $text_for"
+    sed 's/^/  /' $out
+    color_restore
+    rm $out $ADD_DEL
+    ADD_DEL=
+    ADD_TEXT_FOR=
+    return $HARDFAIL
+  elif [ $ret -gt 0 ]; then
+    red_if_failure
+    echo "evmctl $op failed" ${TFAIL:+properly} "with ($ret) $text_for"
+    # Show evmctl output only in verbose mode or if real failure.
+    if _is_expect_pass || [ "$VERBOSE" ]; then
+      sed 's/^/  /' $out
+    fi
+    color_restore
+    rm $out $ADD_DEL
+    ADD_DEL=
+    ADD_TEXT_FOR=
+    return $FAIL
+  elif _is_expect_fail; then
+    red_always
+    echo "evmctl $op wrongly succeeded $text_for"
+    sed 's/^/  /' $out
+    color_restore
+  else
+    [ "$VERBOSE" ] && sed 's/^/  /' $out
+  fi
+  rm $out
+  ADD_DEL=
+  ADD_TEXT_FOR=
+  return $OK
+}
+
+# Extract xattr $attr from $file into $out file skipping $pref'ix
+_extract_xattr() {
+  local file=$1 attr=$2 out=$3 pref=$4
+
+  getfattr -n $attr -e hex $file \
+    | grep "^$attr=" \
+    | sed "s/^$attr=$pref//" \
+    | xxd -r -p > $out
+}
+
+# Test if xattr $attr in $file matches $pref'ix
+# Show error and fail otherwise.
+_test_xattr() {
+  local file=$1 attr=$2 pref=$3
+  local text_for=${ADD_TEXT_FOR:+ for $ADD_TEXT_FOR}
+
+  if ! getfattr -n $attr -e hex $file | egrep -qx "$attr=$pref"; then
+    red_if_failure
+    echo "Did not find expected hash$text_for:"
+    echo "    $attr=$pref"
+    echo ""
+    echo "Actual output below:"
+    getfattr -n $attr -e hex $file | sed 's/^/    /'
+    color_restore
+    rm $file
+    ADD_TEXT_FOR=
+    return $FAIL
+  fi
+  ADD_TEXT_FOR=
+}
+
+# Try to enable gost-engine if needed.
+_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
+    EVMCTL_ENGINE="--engine gost"
+    OPENSSL_ENGINE="-engine gost"
+  fi
+}
+
+# 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
+}
+
diff --git a/tests/ima_hash.test b/tests/ima_hash.test
new file mode 100755
index 0000000..30aec19
--- /dev/null
+++ b/tests/ima_hash.test
@@ -0,0 +1,80 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# evmctl ima_hash tests
+#
+# Copyright (C) 2019 Vitaly Chikunov <vt@altlinux.org>
+#
+# 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
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+cd $(dirname $0)
+PATH=../src:$PATH
+source ./functions.sh
+_require evmctl openssl getfattr
+
+trap _report_exit EXIT
+set -f # disable globbing
+
+check() {
+  local alg=$1 pref=$2 chash=$3 hash
+  local file=$alg-hash.txt
+
+  rm -f $file
+  touch $file
+  # Generate hash with openssl, if it's failed skip test,
+  # unless it's negative test, then pass to evmctl
+  cmd="openssl dgst $OPENSSL_ENGINE -$alg $file"
+  echo - $cmd
+  hash=$(set -o pipefail; $cmd 2>/dev/null | cut -d' ' -f2)
+  if [ $? -ne 0 ] && _is_expect_pass; then
+    echo $CYAN"$alg test is skipped"$NORM
+    rm $file
+    return $SKIP
+  fi
+  if [ "$chash" ] && [ "$chash" != "$hash" ]; then
+    red_always
+    echo "Invalid hash for $alg from openssl"
+    echo "Expected: $chash"
+    echo "Returned: $hash"
+    color_restore
+    rm $file
+    return $HARDFAIL
+  fi
+
+  ADD_TEXT_FOR=$alg ADD_DEL=$file \
+    _evmctl_run ima_hash --hashalgo $alg --xattr-user $file || return
+  ADD_TEXT_FOR=$alg \
+    _test_xattr $file user.ima $pref$hash || return
+  rm $file
+  return $OK
+}
+
+# check args: algo hdr-prefix canonic-hash
+expect_pass check  md4        0x01 31d6cfe0d16ae931b73c59d7e0c089c0
+expect_pass check  md5        0x01 d41d8cd98f00b204e9800998ecf8427e
+expect_pass check  sha1       0x01 da39a3ee5e6b4b0d3255bfef95601890afd80709
+expect_fail check  SHA1       0x01 # uppercase
+expect_fail check  sha512-224 0x01 # valid for pkcs1
+expect_fail check  sha512-256 0x01 # valid for pkcs1
+expect_fail check  unknown    0x01 # nonexistent
+expect_pass check  sha224     0x0407 d14a028c2a3a2bc9476102bb288234c415a2b01f828ea62ac5b3e42f
+expect_pass check  sha256     0x0404 e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
+expect_pass check  sha384     0x0405 38b060a751ac96384cd9327eb1b1e36a21fdb71114be07434c0cc7bf63f6e1da274edebfe76f65fbd51ad2f14898b95b
+expect_pass check  sha512     0x0406 cf83e1357eefb8bdf1542850d66d8007d620e4050b5715dc83f4a921d36ce9ce47d0d13c5d85f2b0ff8318d2877eec2f63b931bd47417a81a538327af927da3e
+expect_pass check  rmd160     0x0403 9c1185a5c5e9fc54612808977ee8f548b2258d31
+expect_fail check  sm3        0x01
+expect_fail check  sm3-256    0x01
+_enable_gost_engine
+expect_pass check  md_gost12_256 0x0412 3f539a213e97c802cc229d474c6aa32a825a360b2a933a949fd925208d9ce1bb
+expect_pass check  streebog256   0x0412 3f539a213e97c802cc229d474c6aa32a825a360b2a933a949fd925208d9ce1bb
+expect_pass check  md_gost12_512 0x0413 8e945da209aa869f0455928529bcae4679e9873ab707b55315f56ceb98bef0a7362f715528356ee83cda5f2aac4c6ad2ba3a715c1bcd81cb8e9f90bf4c1c1a8a
+expect_pass check  streebog512   0x0413 8e945da209aa869f0455928529bcae4679e9873ab707b55315f56ceb98bef0a7362f715528356ee83cda5f2aac4c6ad2ba3a715c1bcd81cb8e9f90bf4c1c1a8a
+
-- 
2.11.0

[PATCH v8 2/2] ima-evm-utils: Add sign/verify tests for evmctl

[Vitaly Chikunov]

This commit adds (evm) sign, (evm) verify, ima_sign, and
ima_verify tests for different algos.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 tests/Makefile.am      |   7 +-
 tests/gen-keys.sh      |  97 +++++++++++++
 tests/sign_verify.test | 364 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 467 insertions(+), 1 deletion(-)
 create mode 100755 tests/gen-keys.sh
 create mode 100755 tests/sign_verify.test

diff --git a/tests/Makefile.am b/tests/Makefile.am
index e37b958..029f2ff 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1,7 +1,12 @@
 check_SCRIPTS =
 TESTS = $(check_SCRIPTS)
 
-check_SCRIPTS += ima_hash.test
+check_SCRIPTS += ima_hash.test sign_verify.test
 
 clean-local:
 	-rm -f *.txt *.out *.sig *.sig2
+
+distclean: distclean-keys
+.PHONY: distclean-keys
+distclean-keys:
+	./gen-keys.sh clean
diff --git a/tests/gen-keys.sh b/tests/gen-keys.sh
new file mode 100755
index 0000000..53eb32f
--- /dev/null
+++ b/tests/gen-keys.sh
@@ -0,0 +1,97 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Generate keys for the tests
+#
+# Copyright (C) 2019 Vitaly Chikunov <vt@altlinux.org>
+#
+# 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
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+cd $(dirname $0)
+PATH=../src:$PATH
+type openssl
+
+log() {
+  echo - "$*"
+  eval "$@"
+}
+
+if [ "$1" = clean ]; then
+  rm -f test-ca.conf
+elif [ "$1" = force ] || [ ! -e test-ca.conf ]; then
+cat > test-ca.conf <<- EOF
+	[ req ]
+	distinguished_name = req_distinguished_name
+	prompt = no
+	string_mask = utf8only
+	x509_extensions = v3_ca
+
+	[ req_distinguished_name ]
+	O = IMA-CA
+	CN = IMA/EVM certificate signing key
+	emailAddress = ca@ima-ca
+
+	[ v3_ca ]
+	basicConstraints=CA:TRUE
+	subjectKeyIdentifier=hash
+	authorityKeyIdentifier=keyid:always,issuer
+EOF
+fi
+
+# RSA
+# Second key will be used for wrong key tests.
+for m in 1024 2048; do
+  if [ "$1" = clean ] || [ "$1" = force ]; then
+    rm -f test-rsa$m.cer test-rsa$m.key test-rsa$m.pub
+  fi
+  if [ "$1" = clean ]; then
+    continue
+  fi
+  if [ ! -e test-rsa$m.key ]; then
+    log openssl req -verbose -new -nodes -utf8 -sha1 -days 10000 -batch -x509 \
+      -config test-ca.conf \
+      -newkey rsa:$m \
+      -out test-rsa$m.cer -outform DER \
+      -keyout test-rsa$m.key
+    # for v1 signatures
+    log openssl pkey -in test-rsa$m.key -out test-rsa$m.pub -pubout
+  fi
+done
+
+# EC-RDSA
+for m in \
+  gost2012_256:A \
+  gost2012_256:B \
+  gost2012_256:C \
+  gost2012_512:A \
+  gost2012_512:B; do
+    IFS=':' read -r algo param <<< "$m"
+    if [ "$1" = clean ] || [ "$1" = force ]; then
+      rm -f test-$algo-$param.key test-$algo-$param.cer test-$algo-$param.pub
+    fi
+    if [ "$1" = clean ]; then
+      continue
+    fi
+    [ -e test-$algo-$param.key ] && continue
+    log openssl req -nodes -x509 -utf8 -days 10000 -batch \
+      -config test-ca.conf \
+      -newkey $algo \
+      -pkeyopt paramset:$param \
+      -out    test-$algo-$param.cer -outform DER \
+      -keyout test-$algo-$param.key
+    if [ -s test-$algo-$param.key ]; then
+      log openssl pkey -in test-$algo-$param.key -out test-$algo-$param.pub -pubout
+    fi
+done
+
+# This script leaves test-ca.conf, *.cer, *.pub, *.key files for sing/verify tests.
+# They are never deleted except by `make distclean'.
+
diff --git a/tests/sign_verify.test b/tests/sign_verify.test
new file mode 100755
index 0000000..190b23a
--- /dev/null
+++ b/tests/sign_verify.test
@@ -0,0 +1,364 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# evmctl {,ima_}{sign,verify} tests
+#
+# Copyright (C) 2019 Vitaly Chikunov <vt@altlinux.org>
+#
+# 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
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+cd $(dirname $0)
+PATH=../src:$PATH
+source ./functions.sh
+_require evmctl openssl xxd getfattr
+./gen-keys.sh >/dev/null 2>&1
+
+trap _report_exit EXIT
+set -f # disable globbing
+
+# Determine keyid from a cert
+_keyid() {
+  local cer=${1%.*}.cer cmd
+  local tmp
+
+  cer=test-${cer#test-}
+  cmd="openssl x509 $OPENSSL_ENGINE \
+          -in $cer -inform DER -pubkey -noout"
+  id=$($cmd 2>/dev/null \
+    | openssl asn1parse \
+    | grep BIT.STRING \
+    | cut -d: -f1)
+  if [ -z "$id" ]; then
+    echo - $cmd >&2
+    echo "Cannot asn1parse $cer to determine keyid" >&2
+    exit 1
+  fi
+  tmp=$(mktemp)
+  openssl x509 $OPENSSL_ENGINE \
+      -in $cer -inform DER -pubkey -noout 2>/dev/null \
+    | openssl asn1parse -strparse $id -out $tmp -noout
+  cat $tmp \
+    | openssl dgst -c -sha1 \
+    | cut -d' ' -f2 \
+    | grep -o ":..:..:..:..$" \
+    | tr -d :
+  rm -f $tmp
+}
+
+# Convert test $type into evmctl op prefix
+_op() {
+  if [ $1 = ima ]; then
+    echo ima_
+  fi
+}
+
+# Convert test $type into xattr name
+_xattr() {
+  if [ $1 = ima ]; then
+    echo user.ima
+  else
+    echo user.evm
+  fi
+}
+
+# Check that detached signature matches xattr signature
+_test_sigfile() {
+  local file=$1 attr=$2 file_sig=$3 file_sig2=$4
+
+  if [ ! -e $file_sig ]; then
+    red_always
+    echo "evmctl ima_sign: no detached signature $file_sig"
+    color_restore
+    rm $file
+    return $FAIL
+  fi
+
+  _extract_xattr $file $attr $file_sig2
+  if ! cmp -bl $file_sig $file_sig2; then
+    red_always
+    echo "evmctl ima_sign: xattr signature on $file differ from detached $file_sig"
+    color_restore
+    rm $file $file_sig $file_sig2
+    return $FAIL
+  fi
+
+  rm $file_sig $file_sig2
+}
+
+# Run single sign command
+_evmctl_sign() {
+  local type=$1 key=$2 alg=$3 file=$4 opts=$5
+
+  # Can check --sigfile for ima_sign
+  [ $type = ima ] && opts+=" --sigfile"
+
+  ADD_TEXT_FOR="$alg ($key)" ADD_DEL=$file \
+    _evmctl_run $(_op $type)sign $opts \
+    --hashalgo $alg --key $key --xattr-user $file || return
+
+  if [ $type = ima ]; then
+    _test_sigfile $file $(_xattr $type) $file.sig $file.sig2
+  fi
+}
+
+# Run and test {ima_,}sign operation
+check_sign() {
+  # Arguments are passed via global vars:
+  # TYPE (ima or evm),
+  # KEY,
+  # ALG (hash algo),
+  # PREF (signature header prefix),
+  # OPTS (additional options for evmctl),
+  # FILE (working file to sign).
+  local "$@"
+  local KEY=${KEY%.*}.key
+  local FILE=${FILE:-$ALG.txt}
+
+  # Normalize key filename
+  KEY=test-${KEY#test-}
+
+  # Append suffix to files for negative tests, because we may
+  # leave only good files for verify tests.
+  _is_expect_fail && FILE+='~'
+
+  rm -f $FILE
+  if ! touch $FILE; then
+    red_always
+    echo "Can't create test file: $FILE"
+    color_restore
+    return $HARDFAIL
+  fi
+
+  if _is_expect_pass; then
+    # Can openssl work with this digest?
+    cmd="openssl dgst $OPENSSL_ENGINE -$ALG $FILE"
+    echo - $cmd
+    if ! $cmd >/dev/null; then
+      echo $CYAN"$ALG ($KEY) test is skipped (openssl is unable to digest)"$NORM
+      return $SKIP
+    fi
+
+    if [ ! -e $KEY ]; then
+      echo $CYAN"$ALG ($KEY) test is skipped (key file not found)"$NORM
+      return $SKIP
+    fi
+
+    # Can openssl sign with this digest and key?
+    cmd="openssl dgst $OPENSSL_ENGINE -$ALG -sign $KEY -hex $FILE"
+    echo - $cmd
+    if ! $cmd >/dev/null; then
+      echo $CYAN"$ALG ($KEY) test is skipped (openssl is unable to sign)"$NORM
+      return $SKIP
+    fi
+  fi
+
+  # Fix keyid in the prefix.
+  if [[ $PREF =~ K ]]; then
+    keyid=$(_keyid $KEY)
+    if [ $? -ne 0 ]; then
+      red_always
+      echo "Unable to determine keyid for $KEY"
+      color_restore
+      return $HARDFAIL
+    fi
+    [[ "$VERBOSE" -gt 2 ]] && echo "  Expected keyid: $keyid"
+    PREF=$(echo $PREF | sed "s/K/$keyid/")
+  fi
+
+  # Perform signing by evmctl
+  _evmctl_sign $TYPE $KEY $ALG $FILE "$OPTS" || return
+
+  # First simple pattern match the signature.
+  ADD_TEXT_FOR=$ALG \
+    _test_xattr $FILE $(_xattr $TYPE) $PREF.* || return
+
+  # This is all we can do for v1 signatures.
+  [[ "$OPTS" =~ --rsa ]] && return $OK
+
+  # This is all we can do for evm.
+  [[ $TYPE =~ evm ]] && return $OK
+
+  # Extract signature to a file
+  _extract_xattr $FILE $(_xattr $TYPE) $FILE.sig2 $PREF
+
+  # Verify extracted signature with openssl
+  cmd="openssl dgst $OPENSSL_ENGINE -$ALG -verify ${KEY%.*}.pub \
+	-signature $FILE.sig2 $FILE"
+  echo - $cmd
+  if ! $cmd; then
+    red_if_failure
+    echo "Signature v2 verification with openssl is failed."
+    color_restore
+    rm $FILE.sig2
+    return $FAIL
+  fi
+
+  rm $FILE.sig2
+  return $OK
+}
+
+# Test verify operation
+check_verify() {
+  # Arguments are passed via global vars:
+  # TYPE (ima or evm),
+  # KEY,
+  # ALG (hash algo),
+  # OPTS (additional options for evmctl),
+  # FILE (filename to verify).
+  local "$@"
+
+  if ! openssl dgst $OPENSSL_ENGINE -$ALG /dev/null >/dev/null 2>&1; then
+    echo $CYAN"$ALG ($KEY) test is skipped (openssl does not support $ALG)"$NORM
+    return $SKIP
+  fi
+
+  ADD_TEXT_FOR="$FILE ($KEY)" \
+    _evmctl_run $(_op $TYPE)verify --key $KEY --xattr-user $OPTS $FILE
+}
+
+# Test runners
+
+# Perform sign and verify ima and evm testing
+sign_verify() {
+  local key=$1 alg=$2 pref="$3" opts="$4"
+  local file=$alg.txt
+
+  # Set defaults:
+  # Public key is different for v1 and v2 (where x509 cert is used).
+  if [[ $opts =~ --rsa ]]; then
+    KEY=test-$key.pub
+  else
+    KEY=test-$key.cer
+  fi
+  ALG=$alg
+  PREF=$pref
+  OPTS=$opts
+  FILE=$file
+
+  TYPE=ima
+  if expect_pass check_sign; then
+
+    # Normal verify with proper key should pass
+    expect_pass check_verify
+
+    # Multiple files and some don't verify
+    expect_fail check_verify FILE=/dev/null,$file
+  fi
+
+  TYPE=evm
+  # Avoid running blkid for evm tests which may require root
+  # No generation on overlayfs:
+  # ioctl(3, FS_IOC_GETVERSION, 0x7ffd8e0bd628) = -1 ENOTTY (Inappropriate ioctl for device)
+  OPTS="$opts --uuid --generation 0"
+  if expect_pass check_sign; then
+
+    # Normal verify with proper key
+    expect_pass check_verify
+
+    # Verify with wrong key
+    expect_fail check_verify KEY=rsa2048
+  fi
+
+  # Note: Leaving TYPE=evm and file is evm signed
+}
+
+# Test --keys
+try_different_keys() {
+  # This run after sign_verify which leaves
+  # TYPE=evm and file is evm signed
+
+  # v2 signing can work with multiple keys in --key option
+  if [[ ! $OPTS =~ --rsa ]]; then
+
+    # Have correct key in the key list
+    expect_pass check_verify KEY=test-rsa2048.cer,$KEY
+    expect_pass check_verify KEY=/dev/null,$KEY,
+  fi
+
+  # Try key that is not used for signing
+  expect_fail check_verify KEY=rsa2048
+
+  # Try completely wrong key files
+  expect_fail check_verify KEY=/dev/null
+  expect_fail check_verify KEY=/dev/zero
+}
+
+try_different_sigs() {
+  # TYPE=evm and file is evm signed
+
+  # Test --imasig
+  if expect_pass check_sign OPTS="$OPTS --imasig"; then
+
+    # Verify both evm and ima sigs
+    expect_pass check_verify
+    expect_pass check_verify TYPE=ima
+  fi
+
+  # Test --imahash
+  if expect_pass check_sign OPTS="$OPTS --imahash"; then
+
+    expect_pass check_verify
+
+    # IMA hash is not verifiable by ima_verify
+    expect_fail check_verify TYPE=ima
+  fi
+
+  # Test --portable
+  expect_pass check_sign OPTS="$OPTS --portable" PREF=0x05
+  # Cannot be verified
+
+  # Test -i (immutable)
+  expect_pass check_sign OPTS="$OPTS -i" PREF=0x0303
+  # Cannot be verified
+}
+
+# Single test args: type key hash signature-prefix "evmctl-options"
+# sign_verify args:      key hash signature-prefix "evmctl-options"
+# Only single test can be prefixed with expect_{fail,pass}
+# `sign_verify' can not be prefixed with expect_{fail,pass} because
+# it runs multiple tests inside. See more tests there.
+# signature-prefix can contain `K' which will be resolved to keyid (v2 only)
+
+## Test v1 signatures
+# Signature v1 only supports sha1 and sha256 so any other should fail
+expect_fail \
+  check_sign TYPE=ima KEY=rsa1024 ALG=md5 PREF=0x0301 OPTS=--rsa
+
+sign_verify  rsa1024  sha1    0x0301 --rsa
+sign_verify  rsa1024  sha256  0x0301 --rsa
+  try_different_keys
+  try_different_sigs
+
+## Test v2 signatures with RSA PKCS#1
+# List of allowed hashes much greater but not all are supported.
+sign_verify  rsa1024  md5     0x030201K0080
+sign_verify  rsa1024  sha1    0x030202K0080
+sign_verify  rsa1024  sha224  0x030207K0080
+sign_verify  rsa1024  sha256  0x030204K0080
+  try_different_keys
+  try_different_sigs
+sign_verify  rsa1024  sha384  0x030205K0080
+sign_verify  rsa1024  sha512  0x030206K0080
+sign_verify  rsa1024  rmd160  0x030203K0080
+
+# Test v2 signatures with EC-RDSA
+_enable_gost_engine
+sign_verify  gost2012_256-A md_gost12_256 0x030212K0040
+sign_verify  gost2012_256-B md_gost12_256 0x030212K0040
+sign_verify  gost2012_256-C md_gost12_256 0x030212K0040
+sign_verify  gost2012_512-A md_gost12_512 0x030213K0080
+sign_verify  gost2012_512-B md_gost12_512 0x030213K0080
+# Test if signing with wrong key length does not work.
+expect_fail \
+  check_sign TYPE=ima KEY=gost2012_512-B ALG=md_gost12_256 PREF=0x0302 OPTS=
+expect_fail \
+  check_sign TYPE=ima KEY=gost2012_256-B ALG=md_gost12_512 PREF=0x0302 OPTS=
+
-- 
2.11.0

Re: [PATCH v8 0/2] ima-evm-utils: Add some tests for evmctl

[Lakshmi Ramasubramanian]

On 3/30/20 9:16 AM, Lakshmi Ramasubramanian wrote:

> On 3/26/20 9:25 PM, Vitaly Chikunov wrote:
>> This series adds simple evmctl tests for hash, sign, and verify 
>> operations.
> 
> Could you please add the steps to initialize, configure, and run the 
> tests in cover letter?
> 
> thanks,
>   -lakshmi

Sorry - forgot to reply-all.

Re: [PATCH v8 1/2] ima-evm-utils: Add some tests for evmctl

[Lakshmi Ramasubramanian]

On 3/30/20 9:16 AM, Lakshmi Ramasubramanian wrote:

> On 3/26/20 9:25 PM, Vitaly Chikunov wrote:
> 
>> Run `make check' to execute the tests.
> 
> autogen.sh followed by configure need to be run. Could you please add 
> that in the patch description?
> 
>> diff --git a/tests/functions.sh b/tests/functions.sh
>> new file mode 100755
>> index 0000000..16c8d89
>> --- /dev/null
>> +++ b/tests/functions.sh
>> @@ -0,0 +1,272 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
> 
> In some source files in the kernel repo I have seen the following:
> 
>      // SPDX-License-Identifier: GPL-2.0+
> 
> Not sure if it is applicable here.
> 
>> +check() {
>> +  local alg=$1 pref=$2 chash=$3 hash
>> +  local file=$alg-hash.txt
>> +
>> +  rm -f $file
>> +  touch $file
>> +  # Generate hash with openssl, if it's failed skip test,
> 
> "if it failed skip test"
> 
>> +  # unless it's negative test, then pass to evmctl
>> +  cmd="openssl dgst $OPENSSL_ENGINE -$alg $file"
>> +}
>> +
> 
>> +# check args: algo hdr-prefix canonic-hash
>> +expect_pass check  md4        0x01 31d6cfe0d16ae931b73c59d7e0c089c0
>> +expect_pass check  md5        0x01 d41d8cd98f00b204e9800998ecf8427e
> Is MD4 and MD5 tests still needed?
> 
> 
>   -lakshmi

+ Mimi and linux-integrity

Re: [PATCH v8 2/2] ima-evm-utils: Add sign/verify tests for evmctl

[Lakshmi Ramasubramanian]

On 3/30/20 9:26 AM, Lakshmi Ramasubramanian wrote:
> On 3/26/20 9:25 PM, Vitaly Chikunov wrote:
> 
>> --- /dev/null
>> +++ b/tests/gen-keys.sh
>> @@ -0,0 +1,97 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
> Should this be "# SPDX-License-Identifier: GPL-2.0+"
> 
>> +# This script leaves test-ca.conf, *.cer, *.pub, *.key files for 
>> sing/verify tests.
>> +# They are never deleted except by `make distclean'.
>> +
>> diff --git a/tests/sign_verify.test b/tests/sign_verify.test
>> new file mode 100755
>> index 0000000..190b23a
>> --- /dev/null
>> +++ b/tests/sign_verify.test
>> @@ -0,0 +1,364 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
> Same comment as above.
> 
>> +# Check that detached signature matches xattr signature
>> +_test_sigfile() {
>> +  local file=$1 attr=$2 file_sig=$3 file_sig2=$4
>> +
>> +  if [ ! -e $file_sig ]; then
>> +    red_always
>> +    echo "evmctl ima_sign: no detached signature $file_sig"
>> +    color_restore
>> +    rm $file
> 
> Should the file be not deleted in case of failure, so that one can check 
> the file manually?
> 
>> +    return $FAIL
>> +  fi
>> +
>> +  _extract_xattr $file $attr $file_sig2
>> +  if ! cmp -bl $file_sig $file_sig2; then
>> +    red_always
>> +    echo "evmctl ima_sign: xattr signature on $file differ from 
>> detached $file_sig"
>> +    color_restore
>> +    rm $file $file_sig $file_sig2
> Same as above - should the files be not deleted in case of failure?
> 
> thanks,
>   -lakshmi

+ Mimi and linux-integrity

Re: [PATCH v8 1/2] ima-evm-utils: Add some tests for evmctl

[Vitaly Chikunov]

Lakshmi,

On Mon, Mar 30, 2020 at 09:29:29AM -0700, Lakshmi Ramasubramanian wrote:
> On 3/30/20 9:16 AM, Lakshmi Ramasubramanian wrote:
> 
> > On 3/26/20 9:25 PM, Vitaly Chikunov wrote:
> > 
> > > Run `make check' to execute the tests.
> > 
> > autogen.sh followed by configure need to be run. Could you please add
> > that in the patch description?

Why this patch should have this and not any other patch/commit?

> > > diff --git a/tests/functions.sh b/tests/functions.sh
> > > new file mode 100755
> > > index 0000000..16c8d89
> > > --- /dev/null
> > > +++ b/tests/functions.sh
> > > @@ -0,0 +1,272 @@
> > > +#!/bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > 
> > In some source files in the kernel repo I have seen the following:
> > 
> >      // SPDX-License-Identifier: GPL-2.0+
> > 
> > Not sure if it is applicable here.

I'm sure it's applicable.

> > 
> > > +check() {
> > > +  local alg=$1 pref=$2 chash=$3 hash
> > > +  local file=$alg-hash.txt
> > > +
> > > +  rm -f $file
> > > +  touch $file
> > > +  # Generate hash with openssl, if it's failed skip test,
> > 
> > "if it failed skip test"
> > 
> > > +  # unless it's negative test, then pass to evmctl
> > > +  cmd="openssl dgst $OPENSSL_ENGINE -$alg $file"
> > > +}
> > > +
> > 
> > > +# check args: algo hdr-prefix canonic-hash
> > > +expect_pass check  md4        0x01 31d6cfe0d16ae931b73c59d7e0c089c0
> > > +expect_pass check  md5        0x01 d41d8cd98f00b204e9800998ecf8427e
> > Is MD4 and MD5 tests still needed?

If it's supported why it shouldn't be tested.

> > 
> > 
> >   -lakshmi
> 
> + Mimi and linux-integrity

Re: [PATCH v8 2/2] ima-evm-utils: Add sign/verify tests for evmctl

[Vitaly Chikunov]

Lakshmi,

On Mon, Mar 30, 2020 at 09:29:54AM -0700, Lakshmi Ramasubramanian wrote:
> On 3/30/20 9:26 AM, Lakshmi Ramasubramanian wrote:
> > On 3/26/20 9:25 PM, Vitaly Chikunov wrote:
> > 
> > > --- /dev/null
> > > +++ b/tests/gen-keys.sh
> > > @@ -0,0 +1,97 @@
> > > +#!/bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > Should this be "# SPDX-License-Identifier: GPL-2.0+"
> > 
> > > +# This script leaves test-ca.conf, *.cer, *.pub, *.key files for
> > > sing/verify tests.
> > > +# They are never deleted except by `make distclean'.
> > > +
> > > diff --git a/tests/sign_verify.test b/tests/sign_verify.test
> > > new file mode 100755
> > > index 0000000..190b23a
> > > --- /dev/null
> > > +++ b/tests/sign_verify.test
> > > @@ -0,0 +1,364 @@
> > > +#!/bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > Same comment as above.
> > 
> > > +# Check that detached signature matches xattr signature
> > > +_test_sigfile() {
> > > +  local file=$1 attr=$2 file_sig=$3 file_sig2=$4
> > > +
> > > +  if [ ! -e $file_sig ]; then
> > > +    red_always
> > > +    echo "evmctl ima_sign: no detached signature $file_sig"
> > > +    color_restore
> > > +    rm $file
> > 
> > Should the file be not deleted in case of failure, so that one can check
> > the file manually?
> > 
> > > +    return $FAIL
> > > +  fi
> > > +
> > > +  _extract_xattr $file $attr $file_sig2
> > > +  if ! cmp -bl $file_sig $file_sig2; then
> > > +    red_always
> > > +    echo "evmctl ima_sign: xattr signature on $file differ from
> > > detached $file_sig"
> > > +    color_restore
> > > +    rm $file $file_sig $file_sig2
> > Same as above - should the files be not deleted in case of failure?

These files will be overwritten by subsequent tests anyway, so there is
not point to keep them.

Thanks,

> > 
> > thanks,
> >   -lakshmi
> 
> + Mimi and linux-integrity

Re: [PATCH v8 0/2] ima-evm-utils: Add some tests for evmctl

[James Bottomley]

On Mon, 2020-03-30 at 09:28 -0700, Lakshmi Ramasubramanian wrote:
> On 3/30/20 9:16 AM, Lakshmi Ramasubramanian wrote:
> 
> > On 3/26/20 9:25 PM, Vitaly Chikunov wrote:
> > > This series adds simple evmctl tests for hash, sign, and verify 
> > > operations.
> > 
> > Could you please add the steps to initialize, configure, and run
> > the tests in cover letter?
> > 
> > thanks,
> >   -lakshmi
> 
> Sorry - forgot to reply-all.

It looks like it's the standard autoreconf -fiv; ./configure; make;
make check?

That's basically the universal autoconf way of running tests ... I'm
not really sure documenting how to do this is a necessary requirement
for the series.

James

Re: [PATCH v8 1/2] ima-evm-utils: Add some tests for evmctl

[Mimi Zohar]

Hi Vitaly,

On Fri, 2020-03-27 at 07:25 +0300, Vitaly Chikunov wrote:
> Run `make check' to execute the tests.
> This commit only adds ima_hash test.

> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>

Thank you for breaking up the patch based on tests. "ima_hash.test"
may be executed by running "make check", but obviously also by
invoking it directly.

A couple of comments/questions inline below.

> diff --git a/tests/functions.sh b/tests/functions.sh
> new file mode 100755
> index 0000000..16c8d89
> --- /dev/null
> +++ b/tests/functions.sh
> @@ -0,0 +1,272 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# ima-evm-utils tests bash functions
> +#
> +# Copyright (C) 2019 Vitaly Chikunov <vt@altlinux.org>
> +#
> +# 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
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +# 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
> +
> +# You can set env VERBOSE=1 to see more output from evmctl
> +V=vvvv
> +V=${V:0:$VERBOSE}
> +V=${V:+-$V}
> +
> +# Exit if env FAILEARLY is defined.
> +# Used in expect_{pass,fail}.
> +exit_early() {
> +  if [ $FAILEARLY ]; then

Throughout variables are not double quoted, though the tests
themselves turn off globbing (set -f).  shellcheck doesn't detect that
globbing has been turned off.  It complains about each variable usage
that is not double quoted.

> +    exit $1
> +  fi
> +}
> +
> +# Require particular executables to be present
> +_require() {
> +  ret=
> +  for i; do
> +    if ! type $i; then
> +      echo "$i is required for test"
> +      ret=1
> +    fi
> +  done
> +  [ $ret ] && exit $HARDFAIL
> +}
> +
> +# Only allow color output on tty
> +if [ -t 1 ]; then
> +     RED=$'\e[1;31m'
> +   GREEN=$'\e[1;32m'
> +  YELLOW=$'\e[1;33m'
> +    BLUE=$'\e[1;34m'
> +    CYAN=$'\e[1;36m'
> +    NORM=$'\e[m'
> +fi
> +
> +# Test mode determined by TFAIL variable:
> +#   undefined: to success testing
> +#   defined: failure testing
> +TFAIL=
> +TMODE=+ # mode character to prepend running command in log
> +declare -i TNESTED=0 # just for sanity checking
> +
> +# Run positive test (one that should pass) and account its result
> +expect_pass() {
> +  local ret
> +
> +  if [ $TNESTED -gt 0 ]; then
> +    echo $RED"expect_pass should not be run nested"$NORM
> +    testsfail+=1
> +    exit $HARDFAIL
> +  fi
> +  TFAIL=
> +  TMODE=+
> +  TNESTED+=1
> +  [[ "$VERBOSE" -gt 1 ]] && echo "____ START positive test: $@"

Unless VERBOSE is set as an environment variable, it isn't defined.
 This isn't an issue when using [[ ... ]], but should it be
initialized: VERBOSE="${VERBOSE:-0}"?

> +  "$@"
> +  ret=$?
> +  [[ "$VERBOSE" -gt 1 ]] && echo "^^^^ STOP ($ret) positive test: $@"
> +  TNESTED+=-1
> +  case $ret in
> +    0)  testspass+=1 ;;
> +    77) testsskip+=1 ;;
> +    99) testsfail+=1; exit_early 1 ;;
> +    *)  testsfail+=1; exit_early 2 ;;
> +  esac
> +  return $ret
> +}
> +
> +# Eval negative test (one that should fail) and account its result
> +expect_fail() {
> +  local ret
> +
> +  if [ $TNESTED -gt 0 ]; then
> +    echo $RED"expect_fail should not be run nested"$NORM
> +    testsfail+=1
> +    exit $HARDFAIL
> +  fi
> +
> +  TFAIL=yes
> +  TMODE=-
> +  TNESTED+=1
> +  [[ "$VERBOSE" -gt 1 ]] && echo "____ START negative test: $@"
> +  "$@"
> +  ret=$?
> +  [[ "$VERBOSE" -gt 1 ]] && echo "^^^^ STOP ($ret) negative test: $@"
> +  TNESTED+=-1
> +  case $ret in
> +    0)  testsfail+=1; exit_early 3 ;;
> +    77) testsskip+=1 ;;
> +    99) testsfail+=1; exit_early 4 ;;
> +    *)  testspass+=1 ;;
> +  esac
> +  # Restore defaults (as in positive tests)
> +  # for tests to run without wrappers
> +  TFAIL=
> +  TMODE=+
> +  return $ret
> +}
> +
> +# return true if current test is positive
> +_is_expect_pass() {
> +  [ ! $TFAIL ]
> +}
> +
> +# return true if current test is negative
> +_is_expect_fail() {
> +  [ $TFAIL ]
> +}
> +
> +# Show blank line and color following text to red
> +# if it's real error (ie we are in expect_pass mode).
> +red_if_failure() {
> +  if _is_expect_pass; then
> +    echo $@ $RED
> +    COLOR_RESTORE=1
> +  fi
> +}
> +
> +# For hard errors
> +red_always() {
> +  echo $@ $RED

A few functions - "red_always", "red_if_failure", "color_restore" -
 use "$@", but none of the function callers pass any parameters.  Is
there a reason for the "$@" or just something left over from
debugging?

> +  COLOR_RESTORE=1
> +}
> +
> +color_restore() {
> +  [ $COLOR_RESTORE ] && echo $@ $NORM
> +  COLOR_RESTORE=
> +}
> +
> +ADD_DEL=
> +ADD_TEXT_FOR=
> +# _evmctl_run should be run as `_evmctl_run ... || return'
> +_evmctl_run() {
> +  local op=$1 out=$1-$$.out
> +  local text_for=${FOR:+for $ADD_TEXT_FOR}
> +  # Additional parameters:
> +  # ADD_DEL: additional files to rm on failure
> +  # ADD_TEXT_FOR: append to text as 'for $ADD_TEXT_FOR'
> +
> +  cmd="evmctl $V $EVMCTL_ENGINE $@"
> +  echo $YELLOW$TMODE $cmd$NORM
> +  $cmd >$out 2>&1
> +  ret=$?
> +
> +  # Shell special and signal exit codes (except 255)
> +  if [ $ret -ge 126 -a $ret -lt 255 ]; then
> +    red_always
> +    echo "evmctl $op failed hard with ($ret) $text_for"
> +    sed 's/^/  /' $out
> +    color_restore
> +    rm $out $ADD_DEL
> +    ADD_DEL=
> +    ADD_TEXT_FOR=
> +    return $HARDFAIL
> +  elif [ $ret -gt 0 ]; then
> +    red_if_failure
> +    echo "evmctl $op failed" ${TFAIL:+properly} "with ($ret) $text_for"
> +    # Show evmctl output only in verbose mode or if real failure.
> +    if _is_expect_pass || [ "$VERBOSE" ]; then
> +      sed 's/^/  /' $out
> +    fi
> +    color_restore
> +    rm $out $ADD_DEL
> +    ADD_DEL=
> +    ADD_TEXT_FOR=
> +    return $FAIL
> +  elif _is_expect_fail; then
> +    red_always
> +    echo "evmctl $op wrongly succeeded $text_for"
> +    sed 's/^/  /' $out
> +    color_restore
> +  else
> +    [ "$VERBOSE" ] && sed 's/^/  /' $out
> +  fi
> +  rm $out
> +  ADD_DEL=
> +  ADD_TEXT_FOR=
> +  return $OK
> +}
> +
> +# Extract xattr $attr from $file into $out file skipping $pref'ix
> +_extract_xattr() {
> +  local file=$1 attr=$2 out=$3 pref=$4
> +
> +  getfattr -n $attr -e hex $file \
> +    | grep "^$attr=" \
> +    | sed "s/^$attr=$pref//" \
> +    | xxd -r -p > $out
> +}
> +
> +# Test if xattr $attr in $file matches $pref'ix
> +# Show error and fail otherwise.
> +_test_xattr() {
> +  local file=$1 attr=$2 pref=$3
> +  local text_for=${ADD_TEXT_FOR:+ for $ADD_TEXT_FOR}
> +
> +  if ! getfattr -n $attr -e hex $file | egrep -qx "$attr=$pref"; then
> +    red_if_failure
> +    echo "Did not find expected hash$text_for:"
> +    echo "    $attr=$pref"
> +    echo ""
> +    echo "Actual output below:"
> +    getfattr -n $attr -e hex $file | sed 's/^/    /'
> +    color_restore
> +    rm $file
> +    ADD_TEXT_FOR=
> +    return $FAIL
> +  fi
> +  ADD_TEXT_FOR=
> +}
> +
> +# Try to enable gost-engine if needed.
> +_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 \

Interesting /dev/null usage here as a filename.

> +    && openssl engine gost >/dev/null 2>&1; then
> +    EVMCTL_ENGINE="--engine gost"
> +    OPENSSL_ENGINE="-engine gost"
> +  fi
> +}
> +
> +# 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
> +}
> +
> diff --git a/tests/ima_hash.test b/tests/ima_hash.test
> new file mode 100755
> index 0000000..30aec19
> --- /dev/null
> +++ b/tests/ima_hash.test
> @@ -0,0 +1,80 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# evmctl ima_hash tests
> +#
> +# Copyright (C) 2019 Vitaly Chikunov <vt@altlinux.org>
> +#
> +# 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
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +cd $(dirname $0)
> +PATH=../src:$PATH
> +source ./functions.sh
> +_require evmctl openssl getfattr
> +
> +trap _report_exit EXIT
> +set -f # disable globbing
> +
> +check() {
> +  local alg=$1 pref=$2 chash=$3 hash
> +  local file=$alg-hash.txt
> +
> +  rm -f $file
> +  touch $file
> +  # Generate hash with openssl, if it's failed skip test,
> +  # unless it's negative test, then pass to evmctl
> +  cmd="openssl dgst $OPENSSL_ENGINE -$alg $file"
> +  echo - $cmd
> +  hash=$(set -o pipefail; $cmd 2>/dev/null | cut -d' ' -f2)
> +  if [ $? -ne 0 ] && _is_expect_pass; then
> +    echo $CYAN"$alg test is skipped"$NORM
> +    rm $file
> +    return $SKIP
> +  fi
> +  if [ "$chash" ] && [ "$chash" != "$hash" ]; then
> +    red_always

Only when "ima_hash.test" is invoked directly, the output is colored
red.  Really confusing.

> +    echo "Invalid hash for $alg from openssl"
> +    echo "Expected: $chash"
> +    echo "Returned: $hash"
> +    color_restore
> +    rm $file
> +    return $HARDFAIL
> +  fi
> +
> +  ADD_TEXT_FOR=$alg ADD_DEL=$file \
> +    _evmctl_run ima_hash --hashalgo $alg --xattr-user $file || return
> +  ADD_TEXT_FOR=$alg \
> +    _test_xattr $file user.ima $pref$hash || return
> +  rm $file
> +  return $OK
> +}
> +
> +# check args: algo hdr-prefix canonic-hash
> +expect_pass check  md4        0x01 31d6cfe0d16ae931b73c59d7e0c089c0
> +expect_pass check  md5        0x01 d41d8cd98f00b204e9800998ecf8427e
> +expect_pass check  sha1       0x01 da39a3ee5e6b4b0d3255bfef95601890afd80709
> +expect_fail check  SHA1       0x01 # uppercase
> +expect_fail check  sha512-224 0x01 # valid for pkcs1
> +expect_fail check  sha512-256 0x01 # valid for pkcs1
> +expect_fail check  unknown    0x01 # nonexistent
> +expect_pass check  sha224     0x0407 d14a028c2a3a2bc9476102bb288234c415a2b01f828ea62ac5b3e42f
> +expect_pass check  sha256     0x0404 e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
> +expect_pass check  sha384     0x0405 38b060a751ac96384cd9327eb1b1e36a21fdb71114be07434c0cc7bf63f6e1da274edebfe76f65fbd51ad2f14898b95b
> +expect_pass check  sha512     0x0406 cf83e1357eefb8bdf1542850d66d8007d620e4050b5715dc83f4a921d36ce9ce47d0d13c5d85f2b0ff8318d2877eec2f63b931bd47417a81a538327af927da3e
> +expect_pass check  rmd160     0x0403 9c1185a5c5e9fc54612808977ee8f548b2258d31
> +expect_fail check  sm3        0x01
> +expect_fail check  sm3-256    0x01
> +_enable_gost_engine
> +expect_pass check  md_gost12_256 0x0412 3f539a213e97c802cc229d474c6aa32a825a360b2a933a949fd925208d9ce1bb
> +expect_pass check  streebog256   0x0412 3f539a213e97c802cc229d474c6aa32a825a360b2a933a949fd925208d9ce1bb
> +expect_pass check  md_gost12_512 0x0413 8e945da209aa869f0455928529bcae4679e9873ab707b55315f56ceb98bef0a7362f715528356ee83cda5f2aac4c6ad2ba3a715c1bcd81cb8e9f90bf4c1c1a8a
> +expect_pass check  streebog512   0x0413 8e945da209aa869f0455928529bcae4679e9873ab707b55315f56ceb98bef0a7362f715528356ee83cda5f2aac4c6ad2ba3a715c1bcd81cb8e9f90bf4c1c1a8a
> +

Nice!  The code is very concisely written.

Reviewing this patch would be a lot easier, if it was broken up into
smaller pieces.  For example, and this is only an example, the initial
patch could define the base ima_hash.test, a subsequent patch could
add coloring for the base ima_hash.test, another patch could introduce
"make check" and add its coloring.  There's all sorts of ways to break
up this patch to simplify review.

Mimi

Re: [PATCH v8 1/2] ima-evm-utils: Add some tests for evmctl

[Vitaly Chikunov]

Mimi,

On Tue, Mar 31, 2020 at 10:25:24AM -0400, Mimi Zohar wrote:
> > +# For hard errors
> > +red_always() {
> > +  echo $@ $RED
> 
> A few functions - "red_always", "red_if_failure", "color_restore" -
>  use "$@", but none of the function callers pass any parameters.  Is
> there a reason for the "$@" or just something left over from
> debugging?

It was to pass `-n` I think, but it is never needed in the end.

> > +  if [ "$chash" ] && [ "$chash" != "$hash" ]; then
> > +    red_always
> 
> Only when "ima_hash.test" is invoked directly, the output is colored
> red.  Really confusing.

Non-TTY output is never colored to not clutter log files.
But logic is to color the errors in red.

So it thought as 'always red', _when_ there is colored output (TTY).

And it's "always" in contract to "red if failure" - which make text
red only when the test is expected to pass (thus, this is real error
condition), when the test is expected to fail there is no point to
color it red, because it's not real error (to not confuse user).

Because it is unconditional (in that sense) is it named "red always".

I can rename it to something like `color_red'. And rename
`red_if_failure' to `color_red_on_failure'.

> Nice!  The code is very concisely written.
> 
> Reviewing this patch would be a lot easier, if it was broken up into
> smaller pieces.  For example, and this is only an example, the initial
> patch could define the base ima_hash.test, a subsequent patch could
> add coloring for the base ima_hash.test, another patch could introduce
> "make check" and add its coloring.  There's all sorts of ways to break
> up this patch to simplify review.

This would make following commits to change code which is already
committed in previous commits. This would make editing code extra hard.
Especially, when tests was reworked a lot.

Also, I don't think splitting coloring into separate patch improves
review. Instead, we can just remember the rule that (real) errors are 
going to be printed in red.

For example, if we prefix every error message with word "ERROR:" - why
it would be easier to review if we split adding this prefix to every
message in a separate commit?

Red color, similarly to uppercase "ERROR", just improves visibility of
errors. (Which is useful, because there is really a lot of tests).

Thanks,

> 
> Mimi

Re: [PATCH v8 1/2] ima-evm-utils: Add some tests for evmctl

[Mimi Zohar]

On Tue, 2020-03-31 at 18:14 +0300, Vitaly Chikunov wrote:
> Mimi,
> 
> On Tue, Mar 31, 2020 at 10:25:24AM -0400, Mimi Zohar wrote:
> > > +# For hard errors
> > > +red_always() {
> > > +  echo $@ $RED
> > 
> > A few functions - "red_always", "red_if_failure", "color_restore" -
> >  use "$@", but none of the function callers pass any parameters.  Is
> > there a reason for the "$@" or just something left over from
> > debugging?
> 
> It was to pass `-n` I think, but it is never needed in the end.
> 
> > > +  if [ "$chash" ] && [ "$chash" != "$hash" ]; then
> > > +    red_always
> > 
> > Only when "ima_hash.test" is invoked directly, the output is colored
> > red.  Really confusing.
> 
> Non-TTY output is never colored to not clutter log files.
> But logic is to color the errors in red.
> 
> So it thought as 'always red', _when_ there is colored output (TTY).
> 
> And it's "always" in contract to "red if failure" - which make text
> red only when the test is expected to pass (thus, this is real error
> condition), when the test is expected to fail there is no point to
> color it red, because it's not real error (to not confuse user).
> 
> Because it is unconditional (in that sense) is it named "red always".
> 
> I can rename it to something like `color_red'. And rename
> `red_if_failure' to `color_red_on_failure'.

Sure, and add a short comment - "Non-TTY output is never colored".

> 
> > Nice!  The code is very concisely written.
> > 
> > Reviewing this patch would be a lot easier, if it was broken up into
> > smaller pieces.  For example, and this is only an example, the initial
> > patch could define the base ima_hash.test, a subsequent patch could
> > add coloring for the base ima_hash.test, another patch could introduce
> > "make check" and add its coloring.  There's all sorts of ways to break
> > up this patch to simplify review.
> 
> This would make following commits to change code which is already
> committed in previous commits. 

Nothing is committed yet.  I pushed it out in order for others to
review and comment, not only your patches, but mine as well.

> This would make editing code extra hard.
> Especially, when tests was reworked a lot.
> 
> Also, I don't think splitting coloring into separate patch improves
> review. Instead, we can just remember the rule that (real) errors are 
> going to be printed in red.
> 
> For example, if we prefix every error message with word "ERROR:" - why
> it would be easier to review if we split adding this prefix to every
> message in a separate commit?
> 
> Red color, similarly to uppercase "ERROR", just improves visibility of
> errors. (Which is useful, because there is really a lot of tests).

Trust me, I understand breaking up patches to simplify review is a lot
of work.  It's one of the hardest things to teach newbies and explain
to them why it is necessary and why a clean history is worthwhile, but
you know how to do it.  These were just suggestions, just as
separating the two tests was just a suggestion.

BTW, in case it got lost along the way, I really do appreciate your
help.

Thanks!

Mimi

Re: [PATCH v8 2/2] ima-evm-utils: Add sign/verify tests for evmctl

[Mimi Zohar]

On Fri, 2020-03-27 at 07:25 +0300, Vitaly Chikunov wrote:

<snip>

> +# Perform sign and verify ima and evm testing
> +sign_verify() {
> +  local key=$1 alg=$2 pref="$3" opts="$4"
> +  local file=$alg.txt
> +
> +  # Set defaults:
> +  # Public key is different for v1 and v2 (where x509 cert is used).
> +  if [[ $opts =~ --rsa ]]; then
> +    KEY=test-$key.pub
> +  else
> +    KEY=test-$key.cer
> +  fi
> +  ALG=$alg
> +  PREF=$pref
> +  OPTS=$opts
> +  FILE=$file
> +
> +  TYPE=ima
> +  if expect_pass check_sign; then
> +
> +    # Normal verify with proper key should pass
> +    expect_pass check_verify
> +
> +    # Multiple files and some don't verify
> +    expect_fail check_verify FILE=/dev/null,$file

The comment and the code don't seem to be in sync.  This seems to be a
single file, for example, named "/dev/null,sha1.txt", which properly
fails.

____ START positive test: check_verify
+ evmctl -vvv ima_verify --key test-rsa1024.pub --xattr-user --rsa
sha1.txt
  hash-v1: da39a3ee5e6b4b0d3255bfef95601890afd80709
  sighash: e34807780b93cc17bdac89573df40ee4a3e984f1
  sha1.txt: verification is OK
^^^^ STOP (0) positive test: check_verify
____ START negative test: check_verify FILE=/dev/null,sha1.txt
- evmctl -vvv ima_verify --key test-rsa1024.pub --xattr-user --rsa
/dev/null,sha1.txt
evmctl ima_verify failed properly with (1)
  getxattr failed: /dev/null,sha1.txt
  errno: No such file or directory (2)
^^^^ STOP (1) negative test: check_verify FILE=/dev/null,sha1.txt

> +  fi
> +
> +  TYPE=evm
> +  # Avoid running blkid for evm tests which may require root
> +  # No generation on overlayfs:
> +  # ioctl(3, FS_IOC_GETVERSION, 0x7ffd8e0bd628) = -1 ENOTTY (Inappropriate ioctl for device)
> +  OPTS="$opts --uuid --generation 0"
> +  if expect_pass check_sign; then
> +
> +    # Normal verify with proper key
> +    expect_pass check_verify
> +
> +    # Verify with wrong key
> +    expect_fail check_verify KEY=rsa2048
> +  fi
> +
> +  # Note: Leaving TYPE=evm and file is evm signed
> +}
> +
> +# Test --keys
> +try_different_keys() {
> +  # This run after sign_verify which leaves
> +  # TYPE=evm and file is evm signed
> +
> +  # v2 signing can work with multiple keys in --key option
> +  if [[ ! $OPTS =~ --rsa ]]; then
> +
> +    # Have correct key in the key list
> +    expect_pass check_verify KEY=test-rsa2048.cer,$KEY
> +    expect_pass check_verify KEY=/dev/null,$KEY,

First test has multiple keys in the key list.  The key list with
"/dev/null" obviously fails to add the first key, so it lands up being
a single key on the list.

> +  fi
> +
> +  # Try key that is not used for signing
> +  expect_fail check_verify KEY=rsa2048
> +
> +  # Try completely wrong key files
> +  expect_fail check_verify KEY=/dev/null
> +  expect_fail check_verify KEY=/dev/zero
> +}
> +
> +try_different_sigs() {
> +  # TYPE=evm and file is evm signed
> +
> +  # Test --imasig
> +  if expect_pass check_sign OPTS="$OPTS --imasig"; then
> +
> +    # Verify both evm and ima sigs
> +    expect_pass check_verify
> +    expect_pass check_verify TYPE=ima
> +  fi
> +
> +  # Test --imahash
> +  if expect_pass check_sign OPTS="$OPTS --imahash"; then
> +
> +    expect_pass check_verify
> +
> +    # IMA hash is not verifiable by ima_verify
> +    expect_fail check_verify TYPE=ima
> +  fi
> +
> +  # Test --portable
> +  expect_pass check_sign OPTS="$OPTS --portable" PREF=0x05
> +  # Cannot be verified

True, evmctl does not support verifying portable signatures, but it
should be possible not only locally, but remotely to verify a portable
signature.  That's the whole point of having portable EVM signatures.
 The comment is a bit misleading and could say something to that
effect - "todo: add support for evmctl portable signature
verification".

Mimi

Re: [PATCH v8 2/2] ima-evm-utils: Add sign/verify tests for evmctl

[Vitaly Chikunov]

Mimi,

On Wed, Apr 01, 2020 at 02:00:55PM -0400, Mimi Zohar wrote:
> On Fri, 2020-03-27 at 07:25 +0300, Vitaly Chikunov wrote:
> 
> <snip>
> > +    # Multiple files and some don't verify
> > +    expect_fail check_verify FILE=/dev/null,$file
> 
> The comment and the code don't seem to be in sync.  This seems to be a
> single file, for example, named "/dev/null,sha1.txt", which properly
> fails.

Looks like my mistake. There was code that parse multiple files
separated by comma. And it seems that there I thought this logic should
be applicable here. Of course this should be space separated file list.


> > +# Test --keys
> > +try_different_keys() {
> > +  # This run after sign_verify which leaves
> > +  # TYPE=evm and file is evm signed
> > +
> > +  # v2 signing can work with multiple keys in --key option
> > +  if [[ ! $OPTS =~ --rsa ]]; then
> > +
> > +    # Have correct key in the key list
> > +    expect_pass check_verify KEY=test-rsa2048.cer,$KEY
> > +    expect_pass check_verify KEY=/dev/null,$KEY,
> 
> First test has multiple keys in the key list.  The key list with
> "/dev/null" obviously fails to add the first key, so it lands up being
> a single key on the list.

All tests do obvious things. So I don't see a problem in this test. (There
comma separated list is correct.)

> > +
> > +  # Test --portable
> > +  expect_pass check_sign OPTS="$OPTS --portable" PREF=0x05
> > +  # Cannot be verified
> 
> True, evmctl does not support verifying portable signatures, but it
> should be possible not only locally, but remotely to verify a portable
> signature.  That's the whole point of having portable EVM signatures.
>  The comment is a bit misleading and could say something to that
> effect - "todo: add support for evmctl portable signature
> verification".

Well, tests are not right place to note todos for other code.
This todo would look like we need to add test case to the test, like
test is missing something. While now it says that it impossible to test.

I will change text to something like "Cannot be verified for now, until
that support is added to evmctl".

Thanks,


> 
> Mimi

Re: [PATCH v8 2/2] ima-evm-utils: Add sign/verify tests for evmctl

[Mimi Zohar]

On Thu, 2020-04-02 at 10:19 +0300, Vitaly Chikunov wrote:
> > > +
> > > +  # Test --portable
> > > +  expect_pass check_sign OPTS="$OPTS --portable" PREF=0x05
> > > +  # Cannot be verified
> > 
> > True, evmctl does not support verifying portable signatures, but it
> > should be possible not only locally, but remotely to verify a portable
> > signature.  That's the whole point of having portable EVM signatures.
> >  The comment is a bit misleading and could say something to that
> > effect - "todo: add support for evmctl portable signature
> > verification".
> 
> Well, tests are not right place to note todos for other code.
> This todo would look like we need to add test case to the test, like
> test is missing something. While now it says that it impossible to test.
> 
> I will change text to something like "Cannot be verified for now, until
> that support is added to evmctl".

Thanks.

While you're updating the "sign_verify.test", could you expand this
comment a bit?  Referencing an example would help.

# Fix keyid in the prefix.
  if [[ $PREF =~ K ]]; then
    keyid=$(_keyid $KEY)

thanks,

Mimi