ima-evm-utils: Add some tests for evmctl
Subsystems (2)
PATCH v3 1/1 ima-evm-utils: Add some tests for evmctl
Run `make check' to execute the tests.
Currently only ima_hash, sign, verify, ima_sign, and ima_verify are tested.
---
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.
Please test the tests.
.gitignore | 2 +- Makefile.am | 2 +- configure.ac | 1 + tests/Makefile.am | 12 ++ tests/functions.sh | 269 ++++++++++++++++++++++++++++++++++++++ tests/gen-keys.sh | 91 +++++++++++++ tests/ima_hash.test | 71 ++++++++++ tests/sign_verify.test | 347 +++++++++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 793 insertions(+), 2 deletions(-) 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
.gitignore +1 -1
@@ -21,7 +21,7 @@ missing compile libtool ltmain.sh - +test-driver # Compiled executables *.o
Makefile.am +1 -1
@@ -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
configure.ac +1 -0
@@ -67,6 +67,7 @@ EVMCTL_MANPAGE_DOCBOOK_XSL AC_CONFIG_FILES([Makefile src/Makefile + tests/Makefile packaging/ima-evm-utils.spec ]) AC_OUTPUT
tests/Makefile.am new file +12 -0
@@ -0,0 +1,12 @@ +check_SCRIPTS = +TESTS = $(check_SCRIPTS) + +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
tests/functions.sh new file +269 -0
269 lines not on this page · raw message
tests/gen-keys.sh new file +91 -0
91 lines not on this page · raw message
tests/ima_hash.test new file +71 -0
71 lines not on this page · raw message
tests/sign_verify.test new file +347 -0
347 lines not on this page · raw message
Signature · 1 line
2.11.0
Hi Vitaly,
only v1 runs without failure, when running v2 and v3 sign_verify fails:
(testing on on bash 5.0.7 on openSUSE and 5.0.3 on Debian)
evmctl is ../src/evmctl
openssl is /usr/bin/openssl
xxd is /usr/bin/xxd
getfattr is /usr/bin/getfattr
[1;33m- evmctl ima_sign --rsa --sigfile --hashalgo md5 --key test-rsa1024.key --xattr-user md5.txt~[m evmctl ima_sign failed properly with (255)
- openssl dgst -sha1 sha1.txt
- openssl dgst -sha1 -sign test-rsa1024.key -hex sha1.txt
[1;33m+ evmctl ima_sign --rsa --sigfile --hashalgo sha1 --key test-rsa1024.key --xattr-user sha1.txt[m [1;33m+ evmctl ima_verify --key test-rsa1024.pub --xattr-user --rsa sha1.txt[m
[1;31m
evmctl ima_verify failed with (1)
Failed to d2i_X509_fp key file: test-rsa1024.pub
openssl: error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag
openssl: error:0D07803A:asn1 encoding routines:asn1_item_embed_d2i:nested asn1 error
[m
[1;33m- evmctl ima_verify --key test-rsa1024.pub --xattr-user --rsa /dev/null,sha1.txt[m evmctl ima_verify failed properly with (255)
- openssl dgst -sha1 sha1.txt
- openssl dgst -sha1 -sign test-rsa1024.key -hex sha1.txt
[1;33m+ evmctl sign --rsa --hashalgo sha1 --key test-rsa1024.key --xattr-user sha1.txt[m [1;31m
evmctl sign failed with (255)
Failed to read UUID. Root access might require.
errno: No data available (61)
[m
- openssl dgst -sha256 sha256.txt
- openssl dgst -sha256 -sign test-rsa1024.key -hex sha256.txt
[1;33m+ evmctl ima_sign --rsa --sigfile --hashalgo sha256 --key test-rsa1024.key --xattr-user sha256.txt[m [1;33m+ evmctl ima_verify --key test-rsa1024.pub --xattr-user --rsa sha256.txt[m
[1;31m
evmctl ima_verify failed with (1)
Failed to d2i_X509_fp key file: test-rsa1024.pub
openssl: error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag
openssl: error:0D07803A:asn1 encoding routines:asn1_item_embed_d2i:nested asn1 error
[m
[1;33m- evmctl ima_verify --key test-rsa1024.pub --xattr-user --rsa /dev/null,sha256.txt[m evmctl ima_verify failed properly with (255)
- openssl dgst -sha256 sha256.txt
- openssl dgst -sha256 -sign test-rsa1024.key -hex sha256.txt
[1;33m+ evmctl sign --rsa --hashalgo sha256 --key test-rsa1024.key --xattr-user sha256.txt[m [1;31m
evmctl sign failed with (255)
Failed to read UUID. Root access might require.
errno: No data available (61)
...
tests/functions.sh line 1
Quoted 4 lines
tests/functions.sh new file +1
-0@@ -0,0 +1,269 @@ +#!/bin/bash
I'd still vote for /bin/sh and posix syntax. evmctl can be cross compiled for embedded device. Than, it'd have to be run on the target, which can be tiny linux customised distro with busybox shell. That's why POSIX shell syntax instead of relying on bash.
Debian also has nice script that can be used to check for non-portable shell code:
https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkbashisms.pl
tests/functions.sh line 2
Quoted 1 line
tests/functions.sh new file +1
-0+# SPDX-License-Identifier: GPL-2.0
This is the correct SPDX for GPL v2+:
# SPDX-License-Identifier: GPL-2.0-or-later
Quoted 4 lines
tests/functions.sh new file +4
-0+# +# ima-evm-utils tests bash functions +# +# Copyright (C) 2019 Vitaly Chikunov <[email protected]>
tests/functions.sh line 16
Quoted 10 lines
tests/functions.sh new file +10
-0+# +# 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.
SPDX license identifier replaces GPL verbose text. These 2 paragraphs can be deleted.
tests/functions.sh line 22
Quoted 6 lines
tests/functions.sh new file +6
-0+ +# Tests accounting +declare -i testspass=0 testsfail=0 testsskip=0 + +# Exit codes (compatible with automake) +declare -r OK=0
Can you please omit 'declare -r'? It's nice to have read only variables, but it's a bashism (not working in busybox nor in dash).
tests/functions.sh line 25
Quoted 3 lines
tests/functions.sh new file +3
-0+declare -r FAIL=1 +declare -r HARDFAIL=99 # hard failure no matter testing mode +declare -r SKIP=77 # skip test
Also comment "skip test" might be omitted (it's obvious).
tests/functions.sh line 30
Quoted 5 lines
tests/functions.sh new file +5
-0+ +# You can set env VERBOSE=1 to see more output from evmctl +V=vvvv +V=${V:0:$VERBOSE} +V=${V:+-$V}
Maybe it's just me but it's not really readable.
Quoted 13 lines
tests/functions.sh new file +13
-0+ +# 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
tests/functions.sh line 53
Quoted 10 lines
tests/functions.sh new file +10
-0+ if ! type $i; then + echo "$i is required for test" + ret=1 + fi + done + [ $ret ] && exit $HARDFAIL +} + +# Only allow color output on tty +if tty -s; then
BTW I'd prefer the output to file to be plain text (I have vim plugin to show colors, but not everybody has). Now it does not (forcing by red_always ?). [ -t 1 ] will do the check.
Or, you could even have variable to allow user to chose, example from LTP [1]
tst_color_enabled()
{
[ "$LTP_COLORIZE_OUTPUT" = "n" ] || [ "$LTP_COLORIZE_OUTPUT" = "0" ] && return 0
[ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" = "1" ] && return 1
[ -t 1 ] || return 0
return 1
}Quoted 18 lines
tests/functions.sh new file +18
-0+ 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
tests/functions.sh line 81
Quoted 10 lines
tests/functions.sh new file +10
-0+ + 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: $@"
Bashism, use [ ]
tests/functions.sh line 87
Quoted 6 lines
tests/functions.sh new file +6
-0+ "$@" + ret=$? + [[ "$VERBOSE" -gt 1 ]] && echo "^^^^ STOP ($ret) positive test: $@" + TNESTED+=-1 + case $ret in + 0) testspass+=1 ;;
Bashism, use testspass=$((testspass+1))
tests/functions.sh line 100
Quoted 13 lines
tests/functions.sh new file +13
-0+ 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
IMHO it's safer to quote whole string (here ok, but on some circumstances shell handles quoted and unquoted string differently):
echo "$REDexpect_fail should not be run nested $NORM"
Quoted 10 lines
tests/functions.sh new file +10
-0+ testsfail+=1 + exit $HARDFAIL + fi + + TFAIL=yes + TMODE=- + TNESTED+=1 + [[ "$VERBOSE" -gt 1 ]] && echo "____ START negative test: $@" + "$@" + ret=$?
tests/functions.sh line 120
Quoted 10 lines
tests/functions.sh new file +10
-0+ [[ "$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 + TFAIL= # Restore defaults for tests run without wrappers + TMODE=+
I guess you wanted TMODE=
Quoted 2 lines
tests/functions.sh new file +2
-0+ return $ret +}
...
Quoted 11 lines
tests/functions.sh new file +11
-0+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 ${ENGINE:+--engine $ENGINE} $@"
tests/functions.sh line 174
Quoted 10 lines
tests/functions.sh new file +10
-0+ 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
I'd reset terminal inside coloring functions (color_restore() and $NORM). red()
{
printf '\033[1;31m'
echo "$@"
printf '\033[0m'
}red "evmctl $op failed hard with ($ret) $text_for"
Quoted 29 lines
tests/functions.sh new file +29
-0+ 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 +}
...
Quoted 17 lines
tests/ima_hash.test new file +11
-0@@ -0,0 +1,71 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# 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.
tests/ima_hash.test line 20
Quoted 10 lines
tests/ima_hash.test new file +9
-0+# +# 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
Bashism, use .
Maybe
. $(dirname $0)/functions.sh
and in functions.sh
export PATH="../src:$PATH"
(that reduces duplicity, but then you'd need to source it also in tests/gen-keys.sh)
Quoted 4 lines
tests/ima_hash.test new file +4
-0+_require evmctl openssl getfattr + +trap _report_exit EXIT +set -f # disable globbing
tests/ima_hash.test line 34
Quoted 10 lines
tests/ima_hash.test new file +10
-0+ +check() { + local alg=$1 pref=$2 hash=$3 + 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 ${ENGINE:+-engine $ENGINE} -$alg $file"
Why not set ENGINE once at the beginning of the script (or in functions.sh if needed on more places)?
...
Quoted 1 line
tests/sign_verify.test +0
-0
...
tests/sign_verify.test line 56
Quoted 5 lines
tests/sign_verify.test +5
-0+# Convert test $type into evmctl op prefix +op() { + if [ $1 = ima ]; then + echo ima_ + fi
Maybe just
[ "$1" = "ima" ] && echo "ima_"
tests/sign_verify.test line 65
Quoted 9 lines
tests/sign_verify.test +9
-0+} + +# Convert test $type into xattr name +xattr() { + if [ $1 = ima ]; then + echo user.ima + else + echo user.evm + fi
Maybe just:
[ "$1" = "ima" ] && echo "user.ima" || echo "user.evm"
...
tests/sign_verify.test line 117
Quoted 10 lines
tests/sign_verify.test +10
-0+# 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 "$@"
Not sure if this works everywhere as you expect.
In most of the cases you set these global variables anyway, so cleaner would be to use getopts (instead overwriting global variables).
tests/sign_verify.test line 119
Quoted 2 lines
tests/sign_verify.test +2
-0+ local KEY=${KEY%.*}.key + local FILE=${FILE:-$ALG.txt}
I'd use lower case for local variables.
Quoted 33 lines
tests/sign_verify.test +33
-0+ + # 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 + if [ ! -e $KEY ]; then + red_always + echo "Key $KEY does not exist." + color_restore + return $HARDFAIL + fi + + # Can openssl work with this digest? + cmd="openssl dgst ${ENGINE:+-engine $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 + + # Can openssl sign with this digest and key?
tests/sign_verify.test line 162
Quoted 10 lines
tests/sign_verify.test +10
-0+ cmd="openssl dgst ${ENGINE:+-engine $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
Bashism, could you please use grep instead of [[ =~ ]]?
...
Kind regards,
Petr
[1] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_ansi_color.sh
Petr,
Quoted 2 lines · Petr Vorel
only v1 runs without failure, when running v2 and v3
sign_verifyfails:
(testing on on bash 5.0.7 on openSUSE and 5.0.3 on Debian)
v3 should be run over all my other fixes I sent to pass.
Quoted 7 lines
evmctl is ../src/evmctl
openssl is/usr/bin/openssl
xxd is/usr/bin/xxd
getfattr is/usr/bin/getfattr
[1;33m- evmctlima_sign--rsa --sigfile --hashalgo md5 --key test-rsa1024.key --xattr-user md5.txt~[m evmctlima_signfailed properly with (255)
- openssl dgst -sha1 sha1.txt
Quoted 8 lines
- openssl dgst -sha1 -sign test-rsa1024.key -hex sha1.txt
[1;33m+ evmctlima_sign--rsa --sigfile --hashalgo sha1 --key test-rsa1024.key --xattr-user sha1.txt[m [1;33m+ evmctlima_verify--key test-rsa1024.pub --xattr-user --rsa sha1.txt[m [1;31m
evmctlima_verifyfailed with (1)
Failed tod2i_X509_fpkey file: test-rsa1024.pub
openssl: error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag
openssl: error:0D07803A:asn1 encoding routines:asn1_item_embed_d2i:nested asn1 error
evmctl w/o -v does not show why verify is failed.
This "Failed to d2i_X509_fp key file" is failure to read multiple keys which is happen _before_ verify operation. (This is fixed in "ima-evm-utils: Do not load keys from x509 certs if user pass --rsa").
You may re-run test with VERBOSE=1 ./sign_verify.test for more output to see why it's actually failed to verify.
But, if you didn't apply my other fixes it's probably that public key is not loaded.
So, this was valid bug in evmctl found by the tests.
Quoted 8 lines
[1;33m- evmctl
ima_verify--key test-rsa1024.pub --xattr-user --rsa /dev/null,sha1.txt[m evmctlima_verifyfailed properly with (255)
- openssl dgst -sha1 sha1.txt
- openssl dgst -sha1 -sign test-rsa1024.key -hex sha1.txt
[1;33m+ evmctl sign --rsa --hashalgo sha1 --key test-rsa1024.key --xattr-user sha1.txt[m [1;31m
evmctl sign failed with (255)
Failed to read UUID. Root access might require.
I never got this error myself (always running the tests under user). This is error from `blkid'.
I should pass `--uuid ...' for this test, thanks.
Quoted 8 lines
tests/functions.sh line 1
Quoted 4 lines
tests/functions.sh new file +1
-0@@ -0,0 +1,269 @@ +#!/bin/bashI'd still vote for
/bin/shand posix syntax. evmctl can be cross compiled for embedded device. Than, it'd have to be run on the target, which can be tiny linux customised distro with busybox shell. That's why POSIX shell syntax instead of relying on bash.
Tests are not required to compile evmctl.
On tiny embedded box you may not have gcc so lets code on perl instead. You may not have also asciidoc but it is used in ima-evm-utils build.
Quoted 2 lines
Debian also has nice script that can be used to check for non-portable shell code:
https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkbashisms.pl
I know, but I disagree to port tests from bash to posix shell.
Quoted 3 lines
tests/functions.sh line 2
Quoted 1 line
tests/functions.sh new file +1
-0+# SPDX-License-Identifier: GPL-2.0This is the correct SPDX for GPL v2+:
# SPDX-License-Identifier: GPL-2.0-or-later
I am not expert is this, but, as I understand, text in license "or later" doesn't mean license is GPL-2.0-or-later, because both texts contain "or later" statement.
https://spdx.org/licenses/GPL-2.0-only.html
https://spdx.org/licenses/GPL-2.0-or-later.html
GPL-2.0 is ambiguous enough like real license is.
I agree to change to license which is stated in COPYING of ima-evm-utils, but what it's correct tag?
Quoted 11 lines
tests/functions.sh line 16
Quoted 9 lines
tests/functions.sh new file +9
-0+# 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.SPDX license identifier replaces GPL verbose text. These 2 paragraphs can be deleted.
All countries agree to that?
Quoted 4 lines
tests/functions.sh line 22
Quoted 2 lines
tests/functions.sh new file +2
-0+# Exit codes (compatible with automake) +declare -r OK=0Can you please omit 'declare -r'? It's nice to have read only variables, but it's a bashism (not working in busybox nor in dash).
Please no dash propaganda.
Quoted 4 lines
tests/functions.sh line 25
Quoted 3 lines
tests/functions.sh new file +3
-0+declare -r FAIL=1 +declare -r HARDFAIL=99 # hard failure no matter testing mode +declare -r SKIP=77 # skip testAlso comment "skip test" might be omitted (it's obvious).
Hm.
Quoted 5 lines
tests/functions.sh line 30
Quoted 4 lines
tests/functions.sh new file +4
-0+# You can set env VERBOSE=1 to see more output from evmctl +V=vvvv +V=${V:0:$VERBOSE} +V=${V:+-$V}Maybe it's just me but it's not really readable.
Yeah. But that would you suggest to make number of -v in V the same as number in $VERBOSE?
Quoted 6 lines
tests/functions.sh line 53
Quoted 2 lines
tests/functions.sh new file +2
-0+# Only allow color output on tty +if tty -s; thenBTW I'd prefer the output to file to be plain text (I have vim plugin to show colors, but not everybody has). Now it does not (forcing by
red_always?). [ -t 1 ] will do the check.
Or, you could even have variable to allow user to chose, example from LTP [1]
Yes I will disable coloring if tests are run inside of make. I neglected to fix that, because I found colored test-suite.log is still useful. (Even though less -R does not show red colored parts.)
Quoted 2 lines
tests/functions.sh line 81
Quoted 1 line
tests/functions.sh new file +1
-0+ [[ "$VERBOSE" -gt 1 ]] && echo "____ START positive test: $@"Bashism, use [ ]
This is intentional since ['s -gt will output error if $VERBOSE is not a number.
Quoted 19 lines
tests/functions.sh line 87
Quoted 2 lines
tests/functions.sh new file +2
-0+ case $ret in + 0) testspass+=1 ;;Bashism, use testspass=$((testspass+1))
tests/functions.sh line 100
Quoted 13 lines
tests/functions.sh new file +13
-0+ 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"$NORMIMHO it's safer to quote whole string (here ok, but on some circumstances shell handles quoted and unquoted string differently):
echo "$REDexpect_fail should not be run nested $NORM"
I think quoting there is just a matter of style, not of safety.
Quoted 16 lines
Quoted 6 lines
tests/functions.sh new file +6
-0+ TFAIL=yes + TMODE=- + TNESTED+=1 + [[ "$VERBOSE" -gt 1 ]] && echo "____ START negative test: $@" + "$@" + ret=$?tests/functions.sh line 120
Quoted 10 lines
tests/functions.sh new file +10
-0+ [[ "$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 + TFAIL= # Restore defaults for tests run without wrappers + TMODE=+
Quoted 1 line
I guess you wanted TMODE=
No. Comment says I want to restore defaults, and default values for these are same as for positive testing. I may expend comment.
Quoted 5 lines
tests/functions.sh line 174
Quoted 5 lines
tests/functions.sh new file +5
-0+ 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
Quoted 7 lines
I'd reset terminal inside coloring functions (
color_restore()and $NORM).red(){ printf '\033[1;31m' echo "$@" printf '\033[0m' }
What do you mean? Hm...
Argument in red_always' is just to pass -n if needed. But it was never needed, so I can delete it. Blank line was intentional. And it enables red for all next lines until color_restore' sends color restore.
Quoted 1 line
red "evmctl $op failed hard with ($ret) $text_for"
This may be useful for red oneliners, I thought about them, but not added. I thought it's beneficent that I output only with `echo' and not with some unusual custom printing function.
Quoted 5 lines
tests/functions.sh
Quoted 4 lines
tests/functions.sh new file +3
-0+cd $(dirname $0) +PATH=../src:$PATH +source ./functions.shBashism, use .
Since I don't plan to remove bashisms...
Quoted 8 lines
Maybe
. $(dirname $0)/functions.sh
and in
functions.sh
export PATH="../src:$PATH"(that reduces duplicity, but then you'd need to source it also in
tests/gen-keys.sh)
You forgot to `cd'. To put it in functions.sh would be too strong side effect.
Quoted 3 lines
Quoted 1 line
+ cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$alg $file"
Why not set ENGINE once at the beginning of the script (or in
functions.shif needed on more places)?
It could be replaced with two env variables.
One for openssl with `-engine x'
and another for evmctl with `--engine x'.
It's possible but this is purely optimisation. Maybe OK.
Quoted 7 lines
tests/functions.sh
Quoted 5 lines
tests/functions.sh new file +5
-0+# Convert test $type into evmctl op prefix +op() { + if [ $1 = ima ]; then + echo ima_ + fiMaybe just
[ "$1" = "ima" ] && echo "ima_"
Maybe.
Quoted 9 lines
tests/functions.sh
Quoted 7 lines
tests/functions.sh new file +7
-0+# Convert test $type into xattr name +xattr() { + if [ $1 = ima ]; then + echo user.ima + else + echo user.evm + fiMaybe just:
[ "$1" = "ima" ] && echo "user.ima" || echo "user.evm"
Matter of style. Besides x && y || z is not too understandable in shell context for general person. (Yes I use it too in non critical part to output colors). But, what if second || affects result of echo and not of [? (Question about ease of understandability).
Quoted 2 lines
Quoted 1 line
+ local "$@"
Not sure if this works everywhere as you expect.
It works in bash.
Quoted 2 lines
In most of the cases you set these global variables anyway, so cleaner would be to use getopts (instead overwriting global variables).
This would require a lot of rework and make test more complicated?
Quoted 3 lines
tests/functions.sh
Quoted 2 lines
tests/functions.sh new file +2
-0+ local KEY=${KEY%.*}.key + local FILE=${FILE:-$ALG.txt}I'd use lower case for local variables.
But they are globals (becoming locals).
Quoted 3 lines
tests/functions.sh
Quoted 2 lines
tests/functions.sh new file +2
-0+ # Fix keyid in the prefix. + if [[ $PREF =~ K ]]; thenBashism, could you please use grep instead of [[ =~ ]]?
If I were against bashisms I'd suggest using expr.
Quoted 1 line
Kind regards,
Thanks for the review!
Thanks,
Hi Vitaly,
Quoted 11 lines
evmctl w/o
-vdoes not show why verify is failed.This "Failed to
d2i_X509_fpkey file" is failure to read multiple keys which is happen _before_ verify operation. (This is fixed in "ima-evm-utils: Do not load keys from x509 certs if user pass --rsa").You may re-run test with VERBOSE=1 ./sign_verify.test for more output to see why it's actually failed to verify.
But, if you didn't apply my other fixes it's probably that public key is not loaded.
I tested v3 on the top of current next, i.e. on the top of commit cf1b8fd ("ima-evm-utils: Allow EVM verify to determine hash algo").
Quoted 11 lines
So, this was valid bug in evmctl found by the tests.
Quoted 8 lines
[1;33m- evmctl
ima_verify--key test-rsa1024.pub --xattr-user --rsa /dev/null,sha1.txt[m evmctlima_verifyfailed properly with (255)
- openssl dgst -sha1 sha1.txt
- openssl dgst -sha1 -sign test-rsa1024.key -hex sha1.txt
[1;33m+ evmctl sign --rsa --hashalgo sha1 --key test-rsa1024.key --xattr-user sha1.txt[m [1;31m
evmctl sign failed with (255)
Failed to read UUID. Root access might require.
Quoted 4 lines
I never got this error myself (always running the tests under user). This is error from `blkid'.
I should pass `--uuid ...' for this test, thanks.
With proper cleanup VERBOSE=1 ./sign_verify.test has now only 10 failures, all about missing key test-gost2012_256-*.key.
Now command blkid -s UUID -o value /dev/block/254:4 works even on non-root. Not sure what happen, I suppose the problem is somewhere on my side.
Quoted 9 lines
Quoted 8 lines
tests/functions.sh line 1
Quoted 4 lines
tests/functions.sh new file +1
-0@@ -0,0 +1,269 @@ +#!/bin/bashI'd still vote for
/bin/shand posix syntax. evmctl can be cross compiled for embedded device. Than, it'd have to be run on the target, which can be tiny linux customised distro with busybox shell. That's why POSIX shell syntax instead of relying on bash.
Quoted 3 lines
Tests are not required to compile evmctl.
On tiny embedded box you may not have gcc so lets code on perl instead. You may not have also asciidoc but it is used in ima-evm-utils build.
My point was cross compile on host, but run the test on target. My assumption was that it makes sense to run tests on target.
I mean if you get a bug report on arm or unusual architecture (I got the impression that IMA+EVM is used on arm a lot), you ask reporter to copy tests directory and run tests. Now you have to ask for bash as well (but better than perl/python :)).
Although, as there is AFAIK no arch dependent code, it might not make sense to test it on target (enough to run the code compiled natively).
The changes wouldn't be that hard, just using a different style and more external commands instead of bash specific builtins.
But whatever, I got the message that bash dependency stay, sorry for bothering you with it :). The main goal is to cover evmctl with tests, which you did very well.
...
Quoted 8 lines
Quoted 5 lines
tests/functions.sh line 30
Quoted 4 lines
tests/functions.sh new file +4
-0+# You can set env VERBOSE=1 to see more output from evmctl +V=vvvv +V=${V:0:$VERBOSE} +V=${V:+-$V}Maybe it's just me but it's not really readable.
Yeah. But that would you suggest to make number of -v in V the same as number in $VERBOSE?
Well, my style would be to use awk (awk is very common and portable), but many people (including me) consider awk syntax obscure and nonintuitive, so understand it's not an option for you.
Maybe more descriptive variable would make it clearer. And mention in docs that VERBOSE can be <1-4>. But, again, a detail.
Quoted 10 lines
Quoted 6 lines
tests/functions.sh line 53
Quoted 2 lines
tests/functions.sh new file +2
-0+# Only allow color output on tty +if tty -s; thenBTW I'd prefer the output to file to be plain text (I have vim plugin to show colors, but not everybody has). Now it does not (forcing by
red_always?). [ -t 1 ] will do the check.
Or, you could even have variable to allow user to chose, example from LTP [1]Yes I will disable coloring if tests are run inside of make. I neglected to fix that, because I found colored test-suite.log is still useful. (Even though less -R does not show red colored parts.)
ok, thanks.
Quoted 87 lines
Quoted 2 lines
tests/functions.sh line 81
Quoted 1 line
tests/functions.sh new file +1
-0+ [[ "$VERBOSE" -gt 1 ]] && echo "____ START positive test: $@"Bashism, use [ ]
This is intentional since ['s -gt will output error if $VERBOSE is not a number.
Quoted 19 lines
tests/functions.sh line 87
Quoted 2 lines
tests/functions.sh new file +2
-0+ case $ret in + 0) testspass+=1 ;;Bashism, use testspass=$((testspass+1))
tests/functions.sh line 100
Quoted 13 lines
tests/functions.sh new file +13
-0+ 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"$NORMIMHO it's safer to quote whole string (here ok, but on some circumstances shell handles quoted and unquoted string differently):
echo "$REDexpect_fail should not be run nested $NORM"I think quoting there is just a matter of style, not of safety.
Quoted 16 lines
Quoted 6 lines
tests/functions.sh new file +6
-0+ TFAIL=yes + TMODE=- + TNESTED+=1 + [[ "$VERBOSE" -gt 1 ]] && echo "____ START negative test: $@" + "$@" + ret=$?tests/functions.sh line 120
Quoted 10 lines
tests/functions.sh new file +10
-0+ [[ "$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 + TFAIL= # Restore defaults for tests run without wrappers + TMODE=+Quoted 1 line
I guess you wanted TMODE=
No. Comment says I want to restore defaults, and default values for these are same as for positive testing. I may expend comment.
Quoted 5 lines
tests/functions.sh line 174
Quoted 5 lines
tests/functions.sh new file +5
-0+ if [ $ret -ge 126 -a $ret -lt 255 ]; then + red_always + echo "evmctl $op failed hard with ($ret) $text_for" + sed 's/^/ /' $out + color_restoreQuoted 7 lines
I'd reset terminal inside coloring functions (
color_restore()and $NORM).red(){ printf '\033[1;31m' echo "$@" printf '\033[0m' }What do you mean? Hm...
Argument in
red_always' is just to pass-nif needed. But it was never needed, so I can delete it. Blank line was intentional. And it enables red for all next lines untilcolor_restore' sends color restore.Quoted 1 line
red "evmctl $op failed hard with ($ret) $text_for"
This may be useful for red oneliners, I thought about them, but not added. I thought it's beneficent that I output only with `echo' and not with some unusual custom printing function.
Quoted 5 lines
tests/functions.sh
Quoted 4 lines
tests/functions.sh new file +3
-0+cd $(dirname $0) +PATH=../src:$PATH +source ./functions.shBashism, use .
Since I don't plan to remove bashisms...
Quoted 3 lines
Maybe
. $(dirname $0)/functions.sh
Quoted 7 lines
Quoted 4 lines
and in
functions.sh
export PATH="../src:$PATH"(that reduces duplicity, but then you'd need to source it also in
tests/gen-keys.sh)You forgot to `cd'. To put it in
functions.shwould be too strong side effect.
Agree on both. I'd still prefer put PATH to functions.sh, but that's a detail.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
+ cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$alg $file"
Why not set ENGINE once at the beginning of the script (or in
functions.shif needed on more places)?It could be replaced with two env variables.
One for openssl with `-engine x'
and another for evmctl with `--engine x'.
It's possible but this is purely optimisation. Maybe OK.
I noted it, as simple variable increases readability a bit.
Quoted 20 lines
Quoted 7 lines
tests/functions.sh
Quoted 5 lines
tests/functions.sh new file +5
-0+# Convert test $type into evmctl op prefix +op() { + if [ $1 = ima ]; then + echo ima_ + fiMaybe just
[ "$1" = "ima" ] && echo "ima_"Maybe.
Quoted 9 lines
tests/functions.sh
Quoted 7 lines
tests/functions.sh new file +7
-0+# Convert test $type into xattr name +xattr() { + if [ $1 = ima ]; then + echo user.ima + else + echo user.evm + fiMaybe just:
[ "$1" = "ima" ] && echo "user.ima" || echo "user.evm"
Quoted 4 lines
Matter of style. Besides
x && y || zis not too understandable in shell context for general person. (Yes I use it too in non critical part to output colors). But, what if second || affects result of echo and not of [? (Question about ease of understandability).
OK, sorry for bothering with details and pushing my style.
...
Kind regards,
Petr