[PATCH 3/3] binman: add test for sign option

[Sean Anderson]

On 4/10/22 6:37 PM, Alper Nebi Yasak wrote:
> I initially ignored this test because I didn't like the implementation
> anyway, but I disagree with some of Sean's comments here so I wanted to
> add on what I think.
> 
> On 08/04/2022 22:26, Ivan Mikhaylov wrote:
>> On Fri, 2022-04-08 at 11:39 -0400, Sean Anderson wrote:
>>> On 3/21/22 5:43 PM, Ivan Mikhaylov wrote:
>>>> Add the test which provides sequence of actions:
>>>>   1. create the image from binman dts
>>>>   2. create public and private keys
>>>>   3. add public key into dtb with fdt_add_pubkey
>>>>   4. sign image with new sign option
>>>>   5. check with fit_check_sign
>>>>
>>>> Signed-off-by: Ivan Mikhaylov <ivan.mikhaylov at siemens.com>
>>>> ---
>>>>  tools/binman/ftest.py              | 42 +++++++++++++++++++
>>>>  tools/binman/test/225_fit_sign.dts | 67
>>>> ++++++++++++++++++++++++++++++
>>>>  2 files changed, 109 insertions(+)
>>>>  create mode 100644 tools/binman/test/225_fit_sign.dts
>>>>
>>>> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
>>>> index 8f00db6945..8a3d3720c4 100644
>>>> --- a/tools/binman/ftest.py
>>>> +++ b/tools/binman/ftest.py
>>>> @@ -3088,6 +3088,48 @@ class TestFunctional(unittest.TestCase):
>>>>          self.assertEqual(len(U_BOOT_DATA), entry.contents_size)
>>>>          self.assertEqual(len(U_BOOT_DATA), entry.size)
>>>>  
>>>> +    def testSignSimple(self):
>>>> +        """Test signing a single file"""
>>>> +
>>>> +        data = self._DoReadFileRealDtb('225_fit_sign.dts')
>>>> +
>>>> +        updated_fname = tools.GetOutputFilename('image-
>>>> updated.bin')
>>>> +        tools.WriteFile(updated_fname, data)
>>>> +
>>>> +        outdir = os.path.join(self._indir, 'extract')
>>>> +        einfos = control.ExtractEntries(updated_fname, None,
>>>> outdir, [])
>>>> +
>>>> +        dtb = tools.GetOutputFilename('source.dtb')
>>>> +        private_key = tools.GetOutputFilename('test_key.key')
>>>> +        public_key = tools.GetOutputFilename('test_key.crt')
>>>> +        fit = tools.GetOutputFilename('fit.fit')
>>>> +        key_dir = tools.GetOutputDir()
>>>> +
>>>> +        def check_sign(fit, key):
>>>
>>> please inline this, since it is only called once
>>>
>>>> +            try:
>>>> +                tools.Run('fit_check_sign', '-k', key, '-f', fit)
>>>> +            except:
>>>> +                return False
>>>
>>> you can just do a bare tools.Run() and if an exception is raised it
>>> will
>>> cause the test to fail.
>> 
>> Ok, good to know.
> 
> It will cause a test 'error' instead of a 'failure'. You should keep the
> try-except, but call self.fail(...) in the except case with a reasonable
> error message. Nesting multiple try blocks is a bit ugly, so you could
> keep the single-use function.
> 
> It would be even better if you added more cases testing various signing
> conditions, with this as a helper function in the outer scope that all
> of them can use.
> 
> You could extend it to check if the signature nodes in the updated
> fdtmap match the arguments passed to 'binman sign' (but your current
> implementation doesn't update those nodes).
> 
>>>
>>>> +            return True
>>>> +
>>>> +        is_signed = False
>>>> +        try:
>>>> +            tools.Run('openssl', 'req', '-batch' , '-newkey',
>>>> 'rsa:4096',
>>>> +                      '-sha256', '-new',  '-nodes',  '-x509', '-
>>>> keyout',
>>>> +                      private_key, '-out', public_key)
>>>> +            tools.Run('fdt_add_pubkey', '-a', 'sha256,rsa4096', '-
>>>> k', key_dir,
>>>> +                      '-n', 'test_key', dtb)
>>>> +            with test_util.capture_sys_output() as (stdout,
>>>> stderr):
>>>> +                # do sign with private key
>>>> +                self._DoBinman('sign', '-i', updated_fname, '-k',
>>>> private_key,
>>>> +                               '-a', 'sha256,rsa4096', '-f', fit,
>>>> 'fit')
>>>> +                is_signed = check_sign(fit, dtb)
>>>> +        finally:
>>>> +            shutil.rmtree(key_dir)
>>>> +
>>>> +        self.assertEqual(is_signed, True)
>>>
>>> so no need for this assert here
>>>> +
>>>> +
>>>>      def _RunReplaceCmd(self, entry_name, data, decomp=True,
>>>> allow_resize=True,
>>>>                         dts='132_replace.dts'):
>>>>          """Replace an entry in an image
> 
> New tests usually are added at the end of this file. IIRC they should be
> ordered by their test dts' number.
> 
>>>> diff --git a/tools/binman/test/225_fit_sign.dts
>>>> b/tools/binman/test/225_fit_sign.dts
>>>> new file mode 100644
>>>> index 0000000000..2bfa826106
>>>> --- /dev/null
>>>> +++ b/tools/binman/test/225_fit_sign.dts
>>>> @@ -0,0 +1,67 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +
>>>> +/dts-v1/;
>>>> +
>>>> +/ {
>>>> +       #address-cells = <1>;
>>>> +       #size-cells = <1>;
>>>> +
>>>> +       binman {
>>>
>>> I don't really understand what's going on in this test case.
> 
> Binman mocks most entries' data during tests, the test images aren't
> meaningful or usable beyond what's in the python test case code. In this
> case, we only need a 'fit' entry with a signature we can run 'binman
> sign' on, and a 'fdtmap' so we can recognize that the built image has
> that 'fit' entry in it. The rest is just fluff.

I was primarily confused by the duplicate/extraneous binaries also
present. In addition to being tests, these device trees also function as
examples, so I think it is desirable for them to be somewhat sane.

--Sean