[PATCH v2 3/3] binman: Add tests for etype encrypted

[Simon Glass]

Hi Christian,

On Tue, 4 Jul 2023 at 10:03, <christian.taedcke-oss at weidmueller.com> wrote:
>
> From: Christian Taedcke <christian.taedcke at weidmueller.com>
>
> Add tests to reach 100% code coverage for the added etype encrypted.
>
> Signed-off-by: Christian Taedcke <christian.taedcke at weidmueller.com>
> ---
>
> Changes in v2:
> - adapt tests for changed entry implementation
>
>  tools/binman/ftest.py                         | 52 +++++++++++++++++++
>  .../binman/test/282_encrypted_no_content.dts  | 15 ++++++
>  tools/binman/test/283_encrypted_no_algo.dts   | 19 +++++++
>  .../test/284_encrypted_invalid_iv_file.dts    | 23 ++++++++
>  .../binman/test/285_encrypted_missing_key.dts | 28 ++++++++++
>  .../binman/test/286_encrypted_key_source.dts  | 29 +++++++++++
>  tools/binman/test/287_encrypted_key_file.dts  | 29 +++++++++++
>  7 files changed, 195 insertions(+)
>  create mode 100644 tools/binman/test/282_encrypted_no_content.dts
>  create mode 100644 tools/binman/test/283_encrypted_no_algo.dts
>  create mode 100644 tools/binman/test/284_encrypted_invalid_iv_file.dts
>  create mode 100644 tools/binman/test/285_encrypted_missing_key.dts
>  create mode 100644 tools/binman/test/286_encrypted_key_source.dts
>  create mode 100644 tools/binman/test/287_encrypted_key_file.dts
>
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 43b4f850a6..d51139799f 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -94,6 +94,8 @@ ROCKCHIP_TPL_DATA     = b'rockchip-tpl'
>  TEST_FDT1_DATA        = b'fdt1'
>  TEST_FDT2_DATA        = b'test-fdt2'
>  ENV_DATA              = b'var1=1\nvar2="2"'
> +ENCRYPTED_IV_DATA     = b'123456'
> +ENCRYPTED_KEY_DATA    = b'1234567890123456'

Can you make that different, e.g. abcd, since at present one is a
subset of the other. Does the length matter? If not, shorter is better
for the second one, so we can visually look at the output.

>  PRE_LOAD_MAGIC        = b'UBSH'
>  PRE_LOAD_VERSION      = 0x11223344.to_bytes(4, 'big')
>  PRE_LOAD_HDR_SIZE     = 0x00001000.to_bytes(4, 'big')
> @@ -226,6 +228,10 @@ class TestFunctional(unittest.TestCase):
>          # Newer OP_TEE file in v1 binary format
>          cls.make_tee_bin('tee.bin')
>
> +        # test files for encrypted tests
> +        TestFunctional._MakeInputFile('encrypted-file.iv', ENCRYPTED_IV_DATA)
> +        TestFunctional._MakeInputFile('encrypted-file.key', ENCRYPTED_KEY_DATA)
> +
>          cls.comp_bintools = {}
>          for name in COMP_BINTOOLS:
>              cls.comp_bintools[name] = bintool.Bintool.create(name)
> @@ -6676,6 +6682,52 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>                                  ['fit'])
>          self.assertIn("Node '/fit': Missing tool: 'mkimage'", str(e.exception))
>
> +    def testEncryptedNoContent(self):

"""Test for missing content property"""

Please add a comment to following test functions

> +        with self.assertRaises(ValueError) as e:
> +            self._DoReadFileDtb('282_encrypted_no_content.dts')
> +        self.assertIn("Node '/binman/fit/images/u-boot/encrypted': Collection must have a 'content' property", str(e.exception))

Please wrap to 80cols. It is OK not to split strings though, but
otherwise, please keep to 80.

self.assertIn(
    "Node '/binman/fit/images/u-boot/encrypted': Collection must have
a 'content' property",
    str(e.exception))

same below

> +
> +    def testEncryptedNoAlgo(self):
> +        with self.assertRaises(ValueError) as e:
> +            self._DoReadFileDtb('283_encrypted_no_algo.dts')
> +        self.assertIn("Node '/binman/fit/images/u-boot/encrypted': 'encrypted' entry is missing properties: algo iv-filename", str(e.exception))
> +
> +    def testEncryptedInvalidIvfile(self):
> +        with self.assertRaises(ValueError) as e:
> +            self._DoReadFileDtb('284_encrypted_invalid_iv_file.dts')
> +        self.assertIn("Filename 'invalid-iv-file' not found in input path",
> +                      str(e.exception))
> +
> +    def testEncryptedMissingKey(self):
> +        with self.assertRaises(ValueError) as e:
> +            self._DoReadFileDtb('285_encrypted_missing_key.dts')
> +        self.assertIn("Node '/binman/fit/images/u-boot/encrypted': Provide either 'key-filename' or 'key-source'",
> +                      str(e.exception))
> +
> +    def testEncryptedKeySource(self):
> +        data = self._DoReadFileDtb('286_encrypted_key_source.dts')[0]
> +
> +        dtb = fdt.Fdt.FromData(data)
> +        dtb.Scan()
> +
> +        node = dtb.GetNode('/images/u-boot/cipher')
> +        self.assertEqual('algo-name', node.props['algo'].value)
> +        self.assertEqual('key-source-value', node.props['key-source'].value)
> +        self.assertEqual(ENCRYPTED_IV_DATA, tools.to_bytes(''.join(node.props['iv'].value)))
> +        self.assertNotIn('key', node.props)
> +
> +    def testEncryptedKeyFile(self):
> +        data = self._DoReadFileDtb('287_encrypted_key_file.dts')[0]
> +
> +        dtb = fdt.Fdt.FromData(data)
> +        dtb.Scan()
> +
> +        node = dtb.GetNode('/images/u-boot/cipher')
> +        self.assertEqual('algo-name', node.props['algo'].value)
> +        self.assertEqual(ENCRYPTED_IV_DATA, tools.to_bytes(''.join(node.props['iv'].value)))
> +        self.assertEqual(ENCRYPTED_KEY_DATA, b''.join(node.props['key'].value))
> +        self.assertNotIn('key-source', node.props)
> +
>
>  if __name__ == "__main__":
>      unittest.main()
> diff --git a/tools/binman/test/282_encrypted_no_content.dts b/tools/binman/test/282_encrypted_no_content.dts
> new file mode 100644
> index 0000000000..03f7ffee90
> --- /dev/null
> +++ b/tools/binman/test/282_encrypted_no_content.dts
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/dts-v1/;
> +
> +/ {
> +       binman {
> +               fit {
> +                       images {
> +                               u-boot {
> +                                       encrypted {
> +                                       };
> +                               };
> +                       };
> +               };
> +       };
> +};

Do these need to be in a FIT? It is OK to do this, but I wonder if it
is necessary for the purposes of your tests?

Regards,
Simon