[U-Boot,14/14] test: Convert the vboot test to test/py

Now that we have a suitable test framework we should move all tests into it.
The vboot test is a suitable candidate. Rewrite it in Python and move the
data files into an appropriate directory.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/README                                       |   1 -
 test/py/tests/test_vboot.py                       | 185 ++++++++++++++++++++++
 test/{ => py/tests}/vboot/sandbox-kernel.dts      |   0
 test/{ => py/tests}/vboot/sandbox-u-boot.dts      |   0
 test/{ => py/tests}/vboot/sign-configs-sha1.its   |   0
 test/{ => py/tests}/vboot/sign-configs-sha256.its |   0
 test/{ => py/tests}/vboot/sign-images-sha1.its    |   0
 test/{ => py/tests}/vboot/sign-images-sha256.its  |   0
 test/vboot/.gitignore                             |   3 -
 test/vboot/vboot_test.sh                          | 151 ------------------
 10 files changed, 185 insertions(+), 155 deletions(-)
 create mode 100644 test/py/tests/test_vboot.py
 rename test/{ => py/tests}/vboot/sandbox-kernel.dts (100%)
 rename test/{ => py/tests}/vboot/sandbox-u-boot.dts (100%)
 rename test/{ => py/tests}/vboot/sign-configs-sha1.its (100%)
 rename test/{ => py/tests}/vboot/sign-configs-sha256.its (100%)
 rename test/{ => py/tests}/vboot/sign-images-sha1.its (100%)
 rename test/{ => py/tests}/vboot/sign-images-sha256.its (100%)
 delete mode 100644 test/vboot/.gitignore
 delete mode 100755 test/vboot/vboot_test.sh

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> Now that we have a suitable test framework we should move all tests into it.
> The vboot test is a suitable candidate. Rewrite it in Python and move the
> data files into an appropriate directory.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  test/README                                       |   1 -
>  test/py/tests/test_vboot.py                       | 185 ++++++++++++++++++++++
>  test/{ => py/tests}/vboot/sandbox-kernel.dts      |   0
>  test/{ => py/tests}/vboot/sandbox-u-boot.dts      |   0
>  test/{ => py/tests}/vboot/sign-configs-sha1.its   |   0
>  test/{ => py/tests}/vboot/sign-configs-sha256.its |   0
>  test/{ => py/tests}/vboot/sign-images-sha1.its    |   0
>  test/{ => py/tests}/vboot/sign-images-sha256.its  |   0
>  test/vboot/.gitignore                             |   3 -
>  test/vboot/vboot_test.sh                          | 151 ------------------
>  10 files changed, 185 insertions(+), 155 deletions(-)
>  create mode 100644 test/py/tests/test_vboot.py
>  rename test/{ => py/tests}/vboot/sandbox-kernel.dts (100%)
>  rename test/{ => py/tests}/vboot/sandbox-u-boot.dts (100%)
>  rename test/{ => py/tests}/vboot/sign-configs-sha1.its (100%)
>  rename test/{ => py/tests}/vboot/sign-configs-sha256.its (100%)
>  rename test/{ => py/tests}/vboot/sign-images-sha1.its (100%)
>  rename test/{ => py/tests}/vboot/sign-images-sha256.its (100%)
>  delete mode 100644 test/vboot/.gitignore
>  delete mode 100755 test/vboot/vboot_test.sh
>
> diff --git a/test/README b/test/README
> index 6cfee05..ee55972 100644
> --- a/test/README
> +++ b/test/README
> @@ -58,7 +58,6 @@ There are several ad-hoc tests which run outside the pytest environment:
>     test/image  - FIT and lagacy image tests (shell script and Python)
>     test/stdint - A test that stdint.h can be used in U-Boot (shell script)
>     trace       - Test for the tracing feature (shell script)
> -   vboot       - Test for verified boot (shell script)
>
>  The above should be converted to run as part of the pytest suite.
>
> diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
> new file mode 100644
> index 0000000..c779895
> --- /dev/null
> +++ b/test/py/tests/test_vboot.py
> @@ -0,0 +1,185 @@
> +# Copyright (c) 2013, Google Inc.
> +#
> +# SPDX-License-Identifier:     GPL-2.0+
> +#
> +# U-Boot Verified Boot Test
> +
> +"""
> +This tests verified boot in the following ways:
> +
> +For image verification:
> +- Create FIT (unsigned) with mkimage
> +- Check that verification shows that no keys are verified
> +- Sign image
> +- Check that verification shows that a key is now verified
> +
> +For configuration verification:
> +- Corrupt signature and check for failure
> +- Create FIT (with unsigned configuration) with mkimage
> +- Check that image veriication works

verification

> +- Sign the FIT and mark the key as 'required' for verification
> +- Check that image verification works
> +- Corrupt the signature
> +- Check that image verification no-longer works
> +
> +Tests run with both SHA1 and SHA256 hashing.
> +"""
> +
> +import pytest
> +import sys
> +import u_boot_utils as util
> +
> +@pytest.mark.buildconfigspec('fit_signature')
> +def test_vboot(u_boot_console):
> +    """Test verified boot signing with mkimage and verification with 'bootm'.
> +
> +    This works using sandbox only as it needs to update the device tree used
> +    by U-Boot to hold public keys from the signing process.
> +
> +    The SHA1 and SHA256 tests are combined into a single test since the
> +    key-generation process is quite slow and we want to avoid doing it twice.
> +    """
> +    def dtc(dts):
> +        """Run the device-tree compiler to compile a .dts file

In a few other places it reads: "device tree" without a dash. ;)

> +
> +        The output file will be the same as the input file but with a .dtb
> +        extension.
> +
> +        Args:
> +            dts: Device tree file to compile.
> +        """
> +        dtb = dts.replace('.dts', '.dtb')
> +        util.cmd(cons, 'dtc %s %s%s -O dtb '
> +                       '-o %s%s' % (dtc_args, datadir, dts, tmpdir, dtb))
> +
> +    def run_bootm(test_type, expect_string):
> +        """Run a 'bootm' command U-Boot.
> +
> +        This always starts a fresh U-Boot instance since the device tree may
> +        contain a new public key.
> +
> +        Args:
> +            test_type: A string identifying the test type
> +            expect_string: A string which is expected in the output

nit, punctuation.

> +        """
> +        cons.cleanup_spawn()
> +        cons.ensure_spawned()
> +        cons.log.action('%s: Test Verified Boot Run: %s' % (algo, test_type))
> +        output = cons.run_command_list(
> +            ['sb load hostfs - 100 %stest.fit' % tmpdir,
> +             'fdt addr 100',
> +             'bootm 100'])
> +        assert(expect_string in output)
> +
> +    def make_fit(its):
> +        """Make a new FIT from the .its source file
> +
> +        This runs 'mkimage -f' to create a new FIT.
> +
> +        Args:
> +            its: Filename containing .its source

nit, same

> +        """
> +        util.run_and_log(cons, [mkimage, '-D', dtc_args, '-f',
> +                                '%s%s' % (datadir, its), fit])
> +
> +    def sign_fit():
> +        """Sign the FIT
> +
> +        Signs the FIT and writes the signature into it. It also writes the
> +        public key into the dtb.
> +        """
> +        cons.log.action('%s: Sign images' % algo)
> +        util.run_and_log(cons, [mkimage, '-F', '-k', tmpdir, '-K', dtb,
> +                                '-r', fit])
> +
> +    def test_with_algo(sha):

nit, maybe sha_algo?

> +        """Test verified boot with the given hash algorithm

nit, same

> +
> +        This is the main part of the test code. The same procedure is followed
> +        for both hashing algorithms.
> +
> +        Args:
> +            sha: Either 'sha1' or 'sha256', to select the algorithm to use
> +        """
> +        global algo
> +
> +        algo = sha
> +
> +        # Compile our device tree files for kernel and U-Boot
> +        dtc('sandbox-kernel.dts')
> +        dtc('sandbox-u-boot.dts')
> +
> +        # Build the FIT, but don't sign anything yet
> +        cons.log.action('%s: Test FIT with signed images' % algo)
> +        make_fit('sign-images-%s.its' % algo)
> +        run_bootm('unsigned images', 'dev-')
> +
> +        # Sign images with our dev keys
> +        sign_fit()
> +        run_bootm('signed images', 'dev+')
> +
> +        # Create a fresh .dtb without the public keys
> +        dtc('sandbox-u-boot.dts')
> +
> +        cons.log.action('%s: Test FIT with signed configuration' % algo)
> +        make_fit('sign-configs-%s.its' % algo)
> +        run_bootm('unsigned config', '%s+ OK' % algo)
> +
> +        # Sign images with our dev keys
> +        sign_fit()
> +        run_bootm('signed config', 'dev+')
> +
> +        cons.log.action('%s: Check signed config on the host' % algo)
> +
> +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', tmpdir,
> +                                '-k', dtb])
> +
> +        # Increment the first byte of the signature, which should cause failure
> +        sig = util.cmd(cons, 'fdtget -t bx %s %s value' % (fit, sig_node))
> +        byte_list = sig.split()
> +        byte = int(byte_list[0], 16)
> +        byte_list = ['%x' % (byte + 1)] + byte_list[1:]
> +        sig = ' '.join(byte_list)
> +        util.cmd(cons, 'fdtput -t bx %s %s value %s' % (fit, sig_node, sig))
> +
> +        run_bootm('Signed config with bad hash', 'Bad Data Hash')
> +
> +        cons.log.action('%s: Check bad config on the host' % algo)
> +        util.run_and_log_expect_exception(cons, [fit_check_sign, '-f', fit,
> +                '-k', dtb], 1, 'Failed to verify required signature')
> +
> +    cons = u_boot_console
> +    tmpdir = cons.config.result_dir + '/'
> +    tmp = tmpdir + 'vboot.tmp'

Is there a need to clean these up afterward?

Python's tempfile might be helpful, you can supply the result_dir as
the directory prefix.

> +    datadir = 'test/py/tests/vboot/'
> +    fit = '%stest.fit' % tmpdir
> +    mkimage = cons.config.build_dir + '/tools/mkimage'
> +    fit_check_sign = cons.config.build_dir + '/tools/fit_check_sign'
> +    dtc_args = '-I dts -O dtb -i %s' % tmpdir
> +    dtb = '%ssandbox-u-boot.dtb' % tmpdir
> +    sig_node = '/configurations/conf@1/signature@1'

If these variables are used throughout the tests like globals, should
they be DATADIR, MKIMAGE, etc?

> +
> +    # Create an RSA key pair
> +    public_exponent = 65537
> +    util.cmd(cons, 'openssl genpkey -algorithm RSA -out %sdev.key '
> +                   '-pkeyopt rsa_keygen_bits:2048 '
> +                   '-pkeyopt rsa_keygen_pubexp:%d '
> +                   '2>/dev/null'  % (tmpdir, public_exponent))
> +
> +    # Create a certificate containing the public key
> +    util.cmd(cons, 'openssl req -batch -new -x509 -key %sdev.key -out '
> +                   '%sdev.crt' % (tmpdir, tmpdir))
> +
> +    # Create a number kernel image with zeroes
> +    with open('%stest-kernel.bin' % tmpdir, 'w') as fd:
> +        fd.write(5000 * chr(0))
> +

Any need to clean these up afterward or place them into
test-run-unique directories? Is the expectation that subsequent tests
will overwrite existing test data (also that vboot tests cannot run
concurrently)?

> +    try:
> +        # We need to use our own device tree file. Remember to restore it
> +        # afterwards.
> +        old_dtb = cons.config.dtb
> +        cons.config.dtb = dtb
> +        test_with_algo('sha1')
> +        test_with_algo('sha256')
> +    finally:
> +        cons.config.dtb = old_dtb
> diff --git a/test/vboot/sandbox-kernel.dts b/test/py/tests/vboot/sandbox-kernel.dts
> similarity index 100%
> rename from test/vboot/sandbox-kernel.dts
> rename to test/py/tests/vboot/sandbox-kernel.dts
> diff --git a/test/vboot/sandbox-u-boot.dts b/test/py/tests/vboot/sandbox-u-boot.dts
> similarity index 100%
> rename from test/vboot/sandbox-u-boot.dts
> rename to test/py/tests/vboot/sandbox-u-boot.dts
> diff --git a/test/vboot/sign-configs-sha1.its b/test/py/tests/vboot/sign-configs-sha1.its
> similarity index 100%
> rename from test/vboot/sign-configs-sha1.its
> rename to test/py/tests/vboot/sign-configs-sha1.its
> diff --git a/test/vboot/sign-configs-sha256.its b/test/py/tests/vboot/sign-configs-sha256.its
> similarity index 100%
> rename from test/vboot/sign-configs-sha256.its
> rename to test/py/tests/vboot/sign-configs-sha256.its
> diff --git a/test/vboot/sign-images-sha1.its b/test/py/tests/vboot/sign-images-sha1.its
> similarity index 100%
> rename from test/vboot/sign-images-sha1.its
> rename to test/py/tests/vboot/sign-images-sha1.its
> diff --git a/test/vboot/sign-images-sha256.its b/test/py/tests/vboot/sign-images-sha256.its
> similarity index 100%
> rename from test/vboot/sign-images-sha256.its
> rename to test/py/tests/vboot/sign-images-sha256.its
> diff --git a/test/vboot/.gitignore b/test/vboot/.gitignore
> deleted file mode 100644
> index 4631242..0000000
> --- a/test/vboot/.gitignore
> +++ /dev/null
> @@ -1,3 +0,0 @@
> -/*.dtb
> -/test.fit
> -/dev-keys
> diff --git a/test/vboot/vboot_test.sh b/test/vboot/vboot_test.sh
> deleted file mode 100755
> index 6d7abb8..0000000
> --- a/test/vboot/vboot_test.sh
> +++ /dev/null
> @@ -1,151 +0,0 @@
> -#!/bin/bash
> -#
> -# Copyright (c) 2013, Google Inc.
> -#
> -# Simple Verified Boot Test Script
> -#
> -# SPDX-License-Identifier:     GPL-2.0+
> -
> -set -e
> -
> -# Run U-Boot and report the result
> -# Args:
> -#      $1:     Test message
> -run_uboot() {
> -       echo -n "Test Verified Boot Run: $1: "
> -       ${uboot} -d sandbox-u-boot.dtb >${tmp} -c '
> -sb load hostfs - 100 test.fit;
> -fdt addr 100;
> -bootm 100;
> -reset'
> -       if ! grep -q "$2" ${tmp}; then
> -               echo
> -               echo "Verified boot key check failed, output follows:"
> -               cat ${tmp}
> -               false
> -       else
> -               echo "OK"
> -       fi
> -}
> -
> -echo "Simple Verified Boot Test"
> -echo "========================="
> -echo
> -echo "Please see doc/uImage.FIT/verified-boot.txt for more information"
> -echo
> -
> -err=0
> -tmp=/tmp/vboot_test.$$
> -
> -dir=$(dirname $0)
> -
> -if [ -z ${O} ]; then
> -       O=.
> -fi
> -O=$(readlink -f ${O})
> -
> -dtc="-I dts -O dtb -p 2000"
> -uboot="${O}/u-boot"
> -mkimage="${O}/tools/mkimage"
> -fit_check_sign="${O}/tools/fit_check_sign"
> -keys="${dir}/dev-keys"
> -echo ${mkimage} -D "${dtc}"
> -
> -echo "Build keys"
> -mkdir -p ${keys}
> -
> -PUBLIC_EXPONENT=${1}
> -
> -if [ -z "${PUBLIC_EXPONENT}" ]; then
> -       PUBLIC_EXPONENT=65537
> -fi
> -
> -# Create an RSA key pair
> -openssl genpkey -algorithm RSA -out ${keys}/dev.key \
> -    -pkeyopt rsa_keygen_bits:2048 \
> -    -pkeyopt rsa_keygen_pubexp:${PUBLIC_EXPONENT} 2>/dev/null
> -
> -# Create a certificate containing the public key
> -openssl req -batch -new -x509 -key ${keys}/dev.key -out ${keys}/dev.crt
> -
> -pushd ${dir} >/dev/null
> -
> -function do_test {
> -       echo do $sha test
> -       # Compile our device tree files for kernel and U-Boot
> -       dtc -p 0x1000 sandbox-kernel.dts -O dtb -o sandbox-kernel.dtb
> -       dtc -p 0x1000 sandbox-u-boot.dts -O dtb -o sandbox-u-boot.dtb
> -
> -       # Create a number kernel image with zeroes
> -       head -c 5000 /dev/zero >test-kernel.bin
> -
> -       # Build the FIT, but don't sign anything yet
> -       echo Build FIT with signed images
> -       ${mkimage} -D "${dtc}" -f sign-images-$sha.its test.fit >${tmp}
> -
> -       run_uboot "unsigned signatures:" "dev-"
> -
> -       # Sign images with our dev keys
> -       echo Sign images
> -       ${mkimage} -D "${dtc}" -F -k dev-keys -K sandbox-u-boot.dtb \
> -               -r test.fit >${tmp}
> -
> -       run_uboot "signed images" "dev+"
> -
> -
> -       # Create a fresh .dtb without the public keys
> -       dtc -p 0x1000 sandbox-u-boot.dts -O dtb -o sandbox-u-boot.dtb
> -
> -       echo Build FIT with signed configuration
> -       ${mkimage} -D "${dtc}" -f sign-configs-$sha.its test.fit >${tmp}
> -
> -       run_uboot "unsigned config" $sha"+ OK"
> -
> -       # Sign images with our dev keys
> -       echo Sign images
> -       ${mkimage} -D "${dtc}" -F -k dev-keys -K sandbox-u-boot.dtb \
> -               -r test.fit >${tmp}
> -
> -       run_uboot "signed config" "dev+"
> -
> -       echo check signed config on the host
> -       if ! ${fit_check_sign} -f test.fit -k sandbox-u-boot.dtb >${tmp}; then
> -               echo
> -               echo "Verified boot key check on host failed, output follows:"
> -               cat ${tmp}
> -               false
> -       else
> -               if ! grep -q "dev+" ${tmp}; then
> -                       echo
> -                       echo "Verified boot key check failed, output follows:"
> -                       cat ${tmp}
> -                       false
> -               else
> -                       echo "OK"
> -               fi
> -       fi
> -
> -       run_uboot "signed config" "dev+"
> -
> -       # Increment the first byte of the signature, which should cause failure
> -       sig=$(fdtget -t bx test.fit /configurations/conf@1/signature@1 value)
> -       newbyte=$(printf %x $((0x${sig:0:2} + 1)))
> -       sig="${newbyte} ${sig:2}"
> -       fdtput -t bx test.fit /configurations/conf@1/signature@1 value ${sig}
> -
> -       run_uboot "signed config with bad hash" "Bad Data Hash"
> -}
> -
> -sha=sha1
> -do_test
> -sha=sha256
> -do_test
> -
> -popd >/dev/null
> -
> -echo
> -if ${ok}; then
> -       echo "Test passed"
> -else
> -       echo "Test failed"
> -fi
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Thanks for this refactor! If the comments related to the sh to Python
are too nit-picky we can certainly change and expand the test harness
within additional patches later.

Hi Teddy,

On 3 July 2016 at 15:38, Teddy Reed <teddy.reed@gmail.com> wrote:
> Hi Simon,
>
> On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
>> Now that we have a suitable test framework we should move all tests into it.
>> The vboot test is a suitable candidate. Rewrite it in Python and move the
>> data files into an appropriate directory.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  test/README                                       |   1 -
>>  test/py/tests/test_vboot.py                       | 185 ++++++++++++++++++++++
>>  test/{ => py/tests}/vboot/sandbox-kernel.dts      |   0
>>  test/{ => py/tests}/vboot/sandbox-u-boot.dts      |   0
>>  test/{ => py/tests}/vboot/sign-configs-sha1.its   |   0
>>  test/{ => py/tests}/vboot/sign-configs-sha256.its |   0
>>  test/{ => py/tests}/vboot/sign-images-sha1.its    |   0
>>  test/{ => py/tests}/vboot/sign-images-sha256.its  |   0
>>  test/vboot/.gitignore                             |   3 -
>>  test/vboot/vboot_test.sh                          | 151 ------------------
>>  10 files changed, 185 insertions(+), 155 deletions(-)
>>  create mode 100644 test/py/tests/test_vboot.py
>>  rename test/{ => py/tests}/vboot/sandbox-kernel.dts (100%)
>>  rename test/{ => py/tests}/vboot/sandbox-u-boot.dts (100%)
>>  rename test/{ => py/tests}/vboot/sign-configs-sha1.its (100%)
>>  rename test/{ => py/tests}/vboot/sign-configs-sha256.its (100%)
>>  rename test/{ => py/tests}/vboot/sign-images-sha1.its (100%)
>>  rename test/{ => py/tests}/vboot/sign-images-sha256.its (100%)
>>  delete mode 100644 test/vboot/.gitignore
>>  delete mode 100755 test/vboot/vboot_test.sh

[snip]

>
> Thanks for this refactor! If the comments related to the sh to Python
> are too nit-picky we can certainly change and expand the test harness
> within additional patches later.

Thanks for the high-quality review... I'm expecting that Stephen will
have a few things to say about how best to fit things into the pytest
stuff too. So I'll hold off a bit before respinning. But I want to
avoid any more expansion of the vboot shell script.

Regards,
Simon

On 07/03/2016 09:40 AM, Simon Glass wrote:
> Now that we have a suitable test framework we should move all tests into it.
> The vboot test is a suitable candidate. Rewrite it in Python and move the
> data files into an appropriate directory.

> diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py

> +# Copyright (c) 2013, Google Inc.

2013-2016?

> +@pytest.mark.buildconfigspec('fit_signature')
> +def test_vboot(u_boot_console):
> +    """Test verified boot signing with mkimage and verification with 'bootm'.
> +
> +    This works using sandbox only as it needs to update the device tree used
> +    by U-Boot to hold public keys from the signing process.

Since this works on sandbox, the function should be marked:

@pytest.mark.boardspec('sandbox')

> +    def dtc(dts):

> +        util.cmd(cons, 'dtc %s %s%s -O dtb '
> +                       '-o %s%s' % (dtc_args, datadir, dts, tmpdir, dtb))

For all the instances of util.cmd() in this file, it looks pretty easy 
to represent them as arrays rather than strings. Is implementing/using 
cmd() really necessary?

> +    def run_bootm(test_type, expect_string):

> +        cons.cleanup_spawn()
> +        cons.ensure_spawned()

That feels a bit too much like relying on internal details, especially 
as the docstring for cleanup_spawn() says "This is an internal function 
and should not be called directly." Can we introduce a new public 
function cons.restart_uboot() that's intended for public use? The 
implementation would just be the two lines above, but it would provide 
some encapsulation of the details.

> +        cons.log.action('%s: Test Verified Boot Run: %s' % (algo, test_type))
 > +        output = cons.run_command_list(
 > +            ['sb load hostfs - 100 %stest.fit' % tmpdir,
 > +             'fdt addr 100',
 > +             'bootm 100'])
 > +        assert(expect_string in output)

Would it make sense to do this instead:

with cons.log.section("Verified boot %s %s" % (algo, test_type)):
     output = ...
     assert ...

? That would nest each invocation of that command list into a separate 
collapsible section of the HTML log file.

> +    def test_with_algo(sha):
> +        """Test verified boot with the given hash algorithm
> +
> +        This is the main part of the test code. The same procedure is followed
> +        for both hashing algorithms.
> +
> +        Args:
> +            sha: Either 'sha1' or 'sha256', to select the algorithm to use
> +        """
> +        global algo
> +
> +        algo = sha

Why not just pass that parameter to the couple of functions that need it?

Certainly, having the various utility functions rely on variables from 
outer scopes is consistent with some other existing tests, but if you're 
going to do that, I'd suggest having this function not take the sha 
parameter, but instead pick up "algo" from an outer scope too?

> +        # Compile our device tree files for kernel and U-Boot
> +        dtc('sandbox-kernel.dts')
> +        dtc('sandbox-u-boot.dts')

I think that happens twice, and ends up doing an identical operation. 
Should those commands (and perhaps some others below too?) be moved 
outside the function into top-level setup code?

Also, is it necessary to repeat those commands if a previous test run 
already ran dtc? Here, dtc is an external utility so I don't think 
running it over and over is worthwhile. However, for some/all of the 
mkimage below, since mkimage presumably comes from the current U-Boot 
build, re-running it each time would actually test something about the 
current U-Boot source tree.

> +        # Build the FIT, but don't sign anything yet
> +        cons.log.action('%s: Test FIT with signed images' % algo)
> +        make_fit('sign-images-%s.its' % algo)
> +        run_bootm('unsigned images', 'dev-')

Perhaps rather than run_bootm() creating its own log section, the 
section should be created here, and wrap all 3 of those lines above?

> +        # Sign images with our dev keys
> +        sign_fit()
> +        run_bootm('signed images', 'dev+')
> +
> +        # Create a fresh .dtb without the public keys
> +        dtc('sandbox-u-boot.dts')

Doesn't this generate the same DTB as above? I don't see any 
FIT/binary/... include statements etc. in that .dts file.

> +        byte_list = ['%x' % (byte + 1)] + byte_list[1:]

byte_list[0] = '%x' % (byte + 1)

?

> +    cons = u_boot_console
> +    tmpdir = cons.config.result_dir + '/'
> +    tmp = tmpdir + 'vboot.tmp'
> +    datadir = 'test/py/tests/vboot/'

I suspect some of the files might usefully be placed into 
ubconfig.persistent_data_dir?

> +    fit = '%stest.fit' % tmpdir
> +    mkimage = cons.config.build_dir + '/tools/mkimage'
> +    fit_check_sign = cons.config.build_dir + '/tools/fit_check_sign'
> +    dtc_args = '-I dts -O dtb -i %s' % tmpdir
> +    dtb = '%ssandbox-u-boot.dtb' % tmpdir

I think all those filename concatenations would be clearer as '%s/%s' 
rather than placing the / into one of the strings.

> +    # Create an RSA key pair
> +    public_exponent = 65537
> +    util.cmd(cons, 'openssl genpkey -algorithm RSA -out %sdev.key '
> +                   '-pkeyopt rsa_keygen_bits:2048 '
> +                   '-pkeyopt rsa_keygen_pubexp:%d '
> +                   '2>/dev/null'  % (tmpdir, public_exponent))
> +
> +    # Create a certificate containing the public key
> +    util.cmd(cons, 'openssl req -batch -new -x509 -key %sdev.key -out '
> +                   '%sdev.crt' % (tmpdir, tmpdir))
> +
> +    # Create a number kernel image with zeroes
> +    with open('%stest-kernel.bin' % tmpdir, 'w') as fd:
> +        fd.write(5000 * chr(0))

Again, I suspect placing those into ubconfig.persistent_data_dir, and 
skipping those commands if the files already exist, would be beneficial. 
See u_boot_utils.py:PersistentRandomFile for a similar case.

> +    try:
> +        # We need to use our own device tree file. Remember to restore it
> +        # afterwards.
> +        old_dtb = cons.config.dtb
> +        cons.config.dtb = dtb
> +        test_with_algo('sha1')
> +        test_with_algo('sha256')
> +    finally:
> +        cons.config.dtb = old_dtb

I think that needs to call cons.restart_uboot() at the end of the 
finally: block, in order to switch back to the old DTB.

Better still would be to only mark the existing U-Boot instance as being 
in an error state, and defer restarting U-Boot to the next test that 
gets run. That way, U-Boot won't be needlessly respawned if this is the 
only/last test to be run.

On 07/03/2016 03:38 PM, Teddy Reed wrote:
> Hi Simon,
>
> On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
>> Now that we have a suitable test framework we should move all tests into it.
>> The vboot test is a suitable candidate. Rewrite it in Python and move the
>> data files into an appropriate directory.

>> +    datadir = 'test/py/tests/vboot/'
>> +    fit = '%stest.fit' % tmpdir
>> +    mkimage = cons.config.build_dir + '/tools/mkimage'
>> +    fit_check_sign = cons.config.build_dir + '/tools/fit_check_sign'
>> +    dtc_args = '-I dts -O dtb -i %s' % tmpdir
>> +    dtb = '%ssandbox-u-boot.dtb' % tmpdir
>> +    sig_node = '/configurations/conf@1/signature@1'
>
> If these variables are used throughout the tests like globals, should
> they be DATADIR, MKIMAGE, etc?

I'd prefer not to use all-caps variable names.

On Sun, Jul 03, 2016 at 09:40:46AM -0600, Simon Glass wrote:

> Now that we have a suitable test framework we should move all tests into it.
> The vboot test is a suitable candidate. Rewrite it in Python and move the
> data files into an appropriate directory.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!