Mimi,
On Wed, Apr 01, 2020 at 02:00:55PM -0400, Mimi Zohar wrote:
> On Fri, 2020-03-27 at 07:25 +0300, Vitaly Chikunov wrote:
>
> <snip>
> > + # Multiple files and some don't verify
> > + expect_fail check_verify FILE=/dev/null,$file
>
> The comment and the code don't seem to be in sync. This seems to be a
> single file, for example, named "/dev/null,sha1.txt", which properly
> fails.
Looks like my mistake. There was code that parse multiple files
separated by comma. And it seems that there I thought this logic should
be applicable here. Of course this should be space separated file list.
> > +# Test --keys
> > +try_different_keys() {
> > + # This run after sign_verify which leaves
> > + # TYPE=evm and file is evm signed
> > +
> > + # v2 signing can work with multiple keys in --key option
> > + if [[ ! $OPTS =~ --rsa ]]; then
> > +
> > + # Have correct key in the key list
> > + expect_pass check_verify KEY=test-rsa2048.cer,$KEY
> > + expect_pass check_verify KEY=/dev/null,$KEY,
>
> First test has multiple keys in the key list. The key list with
> "/dev/null" obviously fails to add the first key, so it lands up being
> a single key on the list.
All tests do obvious things. So I don't see a problem in this test. (There
comma separated list is correct.)
> > +
> > + # Test --portable
> > + expect_pass check_sign OPTS="$OPTS --portable" PREF=0x05
> > + # Cannot be verified
>
> True, evmctl does not support verifying portable signatures, but it
> should be possible not only locally, but remotely to verify a portable
> signature. That's the whole point of having portable EVM signatures.
> The comment is a bit misleading and could say something to that
> effect - "todo: add support for evmctl portable signature
> verification".
Well, tests are not right place to note todos for other code.
This todo would look like we need to add test case to the test, like
test is missing something. While now it says that it impossible to test.
I will change text to something like "Cannot be verified for now, until
that support is added to evmctl".
Thanks,
>
> Mimi
