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@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
> +#
> +# 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@xxxxxxxxxxxx>
> +#
> +# 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
