Re: [PATCH v2] ima-evm-utils: Add some tests for evmctl

[Mimi Zohar <zohar@linux.ibm.com>]

On Mon, 2019-07-29 at 02:40 +0300, Vitaly Chikunov wrote:
> 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.

There isn't a "Usage" or any documentation listing the environment
variables.  I'm suggesting to at least group them together.

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

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

> 
> > > +  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.

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

> 
> > I'm now wondering if the "--engine e' option is still needed?
> 
> It's needed. Why you thinking it doesn't? Commit 782224f33cd7 will not
> load gost (or any other) engine on its own.

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

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

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

> 
>   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!

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

Mimi