Mimi,
On Sun, Jul 28, 2019 at 01:17:47PM -0400, Mimi Zohar wrote:
> On Sat, 2019-07-27 at 07:41 +0300, Vitaly Chikunov wrote:
> > - Since I still edit all 5 files I did not split the patch into multiple
> > commits to separate the files, otherwise editing will become too
> > complicated, as I ought to continuously rebase and edit different
> > commits. This was really non-productive suggestion
>
> Ok, but the review will be broken up. For now, the comments below are
> limited to tests/Makefile.am, tests/functions.sh and
> tests/ima_hash.test. Some of the comments are intrusive, so I'm going
> to hold off on reviewing the other tests.
This is good, since I am reworking ima_sign/ima_verify tests into a single
test that will also cover EVM sign/verify.
> Autotools generates "test-driver". Should it be added to git-ignore?
Didn't notice this.
> Should we be using SPDX, at least for new files?
OK.
> > + if ! type $i; then
>
> "type" is a bashism.
Tests are on bash.
> > +# Define FAILEARLY to exit testing on the first error.
> > +exit_early() {
> > + if [ $FAILEARLY ]; then
> > + exit $1
> > + fi
> > +}
>
> I would group all of the environment variable function checking
> together at the top of functions.sh.
Some functions check VERBOSE should they be on top too?
Or you meant this is just variable checking function? It isn't.
> The functions "pos" and "neg" are written very concisely, but they are
> part of a common set of functions, which are the crux of the tests
> scripts. I'm really hesitant about having common functions that
> execute any command passed to it, without any form of verification.
What verification and why?
> > + set -- evmctl $V ${ENGINE:+--engine $ENGINE} "$@"
> > + echo $YELLOW$TMODE $*$NORM
> > + eval "$@" >$out 2>&1
>
> Here at least the command is limited to "evmctl".
This is emvctl runner. pos/neg can and should run anything that needs
their exit code be checked and accounted as test result.
> Is there any benefit to using "set --", as opposed to defining a local
> variable and executing it? Is this simply a question of style?
I will make it using variable.
> > +_enable_gost_engine() {
> > + # Do not enable if it's already working (enabled by user)
> > + if ! openssl md_gost12_256 /dev/null >/dev/null 2>&1 \
> > + && openssl engine gost >/dev/null 2>&1; then
> > + ENGINE=gost
> > + fi
> > +}
>
> With gost provided as an Openssl engine, is it possible to be able to
> execute the first command without the gost engine enabled? With
> commit 782224f33cd7 ("ima-evm-utils: Rework openssl init"),
I don't understand question. What is 'first command'? `openssl
md_gost12_256` will not work if gost-engine is not configured somehow.
> I'm now wondering if the "--engine e' option is still needed?
It's needed. Why you thinking it doesn't? Commit 782224f33cd7 will not
load gost (or any other) engine on its own.
> > +# Check with constant
> > +check_const() {
>
> This function comment doesn't provide any more details than the
> function name. Please either rename this function (eg. check_xattr)
> or expand the function comment.
OK.
(check* was supposed to be top-level tests. I will change this in v3.)
> > + local alg=$1 pref=$2 hash=$3 file=$4
> > +
> > + FOR=$alg DEL=$file
>
> Why not use ALG=$alg and FILE=$file as the global variable names?
check was called once for every algo. Are you proposing to change
call like
check_const sha1 0x01 sha1-hash.txt
to
ALG=sha1 FILE=sha1-hash.txt
check_const 0x01
?
(I tried to put every mandatory argument into a argument list.)
> > + cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$alg $file"
> > + echo - $cmd
> > + hash=$(set -o pipefail; eval "$cmd" 2>/dev/null | cut -d' ' -f2)
>
> Is there a reason for not executing $cmd directly? Is it safer
> calling "pipefail" and "eval"? Is this a question of style?
I will remove eval (it also don't let me pass empty arguments into
called functions). `pipefail' is needed, so I can see exit code of
$cmd and not of `cut' in $?.
> > + if [ $? -ne 0 ] && _is_positive_test; then
> > + echo $CYAN"$alg test is skipped"$NORM
> > + rm $file
> > + return $SKIP
> > + fi
> > + check_const $alg $pref "$hash" $file
> > +}
> > +
> > +# check args: algo prefix hex-hash
>
> The first keyword - test type - is missing in the comment above. It
> would be clearer if instead of "pos" or "neg", the key words included
> the words "pass" and "fail", to indicate that the test is expected to
> pass or fail.
pass and fail looks like imperative statements, and not like something that
will check other thing to pass or fail. I will rename them to something
else.
Thanks for the review!
