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
