[PATCH 1/3] binman: Add support for externally encrypted blobs
Taedcke, Christian
christian.taedcke-oss at weidmueller.com
Tue Jul 4 10:54:20 CEST 2023
Hello Simon,
Am 30.06.2023 um 16:58 schrieb Simon Glass:
> Hi Christian,
>
> On Fri, 30 Jun 2023 at 11:28, Taedcke, Christian
> <christian.taedcke-oss at weidmueller.com> wrote:
>>
>> Hello Simon,
>>
>> Am 30.06.2023 um 10:57 schrieb Simon Glass:
>>> Hi Christian,
>>>
>>> On Fri, 30 Jun 2023 at 09:20, Taedcke, Christian
>>> <christian.taedcke-oss at weidmueller.com> wrote:
>>>>
>>>> Hello Simon,
>>>>
>>>> thank you for the initial review. Replies are below.
>>>>
>>>> Am 29.06.2023 um 21:09 schrieb Simon Glass:
>>>>> Hi Christian,
>>>>>
>>>>> On Tue, 27 Jun 2023 at 08:39, <christian.taedcke-oss at weidmueller.com> wrote:
>>>>>>
>>>>>> From: Christian Taedcke <christian.taedcke at weidmueller.com>
>>>>>>
>>>>>> This adds a new etype encrypted that is derived from collection.
>>>>>>
>>>>>> It creates a new cipher node in the related image similar to the
>>>>>> cipher node used by u-boot, see boot/image-cipher.c.
>>>>>> Optionally it creates a global /cipher node containing a key and iv
>>>>>> using the same nameing convention as used in boot/image-cipher.c.
>>>>>>
>>>>>> Signed-off-by: Christian Taedcke <christian.taedcke at weidmueller.com>
>>>>>> ---
>>>>>>
>>>>>> tools/binman/etype/encrypted.py | 98 +++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 98 insertions(+)
>>>>>> create mode 100644 tools/binman/etype/encrypted.py
>>>>>>
>>>>>> diff --git a/tools/binman/etype/encrypted.py b/tools/binman/etype/encrypted.py
>>>>>> new file mode 100644
>>>>>> index 0000000000..005ae56acf
>>>>>> --- /dev/null
>>>>>> +++ b/tools/binman/etype/encrypted.py
>>>>>> @@ -0,0 +1,98 @@
>>>>>> +# SPDX-License-Identifier: GPL-2.0+
>>>>>> +# Copyright 2023 Weidmüller Interface GmbH & Co. KG
>>>>>> +# Written by Christian Taedcke <christian.taedcke at weidmueller.com>
>>>>>> +#
>>>>>> +# Entry-type module for cipher information of encrypted blobs/images
>>>>>> +#
>>>>>> +
>>>>>> +from binman.etype.collection import Entry_collection
>>>>>> +from dtoc import fdt_util
>>>>>> +from u_boot_pylib import tools
>>>>>> +
>>>>>> +# This is imported if needed
>>>>>> +state = None
>>>>>> +
>>>>>> +
>>>>>> +class Entry_encrypted(Entry_collection):
>>>>>> + """Externally built encrypted binary blob
>>>>>> +
>>>>>> + If the file providing this blob is missing, binman can optionally ignore it
>>>>>> + and produce a broken image with a warning.
>>>>>> +
>>>>>> + In addition to the inherited 'collection' for Properties / Entry arguments:
>>>>>> + - algo: The encryption algorithm
>>>>>
>>>>> What possible values are available? Please list them
>>>>
>>>> Currently the evaluation of the generated cipher nodes is not
>>>> implemented in c code in upstream U-Boot. I use aes256-gcm and decrypt
>>>> the relevant blobs/images in board-specific code. We plan to also
>>>> upstream the c code for decryption later.
>>>>
>>>> I expect we will support at least aes[128/192/256]-cbc in the future,
>>>> since these are already implemented in software in U-Boot and in
>>>> addition aes256-gcm via a crypto driver.
>>>>
>>>> Since decryption is not implemented yet, i didn't want to restrict the
>>>> possible algos for now, since board-specific code could implement
>>>> decryption for any algorithm here that uses a key and iv (initialization
>>>> vector).
>>>>
>>>> Should i list aes[128/192/256]-cbc and aes256-gcm here or should i state
>>>> that the supported algorithms (for now) are board specific?
>>>
>>> Perhaps the correct answer is to say that nothing is supported yet,
>>> but future patches will add certain algorithms TBD?
>>
>> So something like this would be ok?
>>
>> - algo: The encryption algorithm. Currently no algorithm is supported
>> out-of-the-box. Certain algorithms will be added in future patches.
>
> Yes that seems OK to me.
>
>>
>>>
>>>>
>>>>>
>>>>>> + - iv-name-hint: The name hint for the iv
>>>>>
>>>>> what is the iv?
>>>>
>>>> Initialization Vector. Should i use the full name here?
>>>
>>> Yes, plus explain what it is or where it is documented.
>>>
>>>>
>>>>>
>>>>>> + - key-name-hint: The name hint for the key
>>>>>> + - iv-filename: The name of the file containing the iv
>>>>>> + - key-filename: The name of the file containing the key
>>>>>> +
>>>>>> + This entry creates a cipher node in the entries' parent node (i.e. the
>>>>>
>>>>> entry's
>>>>>
>>>>>> + image). Optionally it also creates a cipher node in the root of the device
>>>>>> + tree containg key and iv information.
>>>>>
>>>>> containing
>>>>>
>>>>> Overall I think this documentation needs to be expanded.
>>>>
>>>> Ok. I tried to explain the motivation in the cover letter, see
>>>> https://lists.denx.de/pipermail/u-boot/2023-June/521160.html
>>>>
>>>> Is the cover letter the wrong place for this (should i move the
>>>> motivation into the first commit message)?
>>>>
>>>> I will also try to improve the code documentation here.
>>>
>>> I mean in the docs for the entry itself (which ends up in
>>> entries.rst), since this is what people read.
>>>
>>> Yes it is good to comment the code as well, but it seems OK to me.
>>>
>>>>
>>>>>
>>>>> I wonder why this needs to be an entry type? Could the node be added
>>>>> to the description by the user, instead of the entry adding it? It
>>>>> feels a little strange to me, but perhaps I just need more info.
>>>>
>>>> This new entry type basically reads the files containing the
>>>> initialization vector and the key and puts it into the device tree. The
>>>> initialization vector normally changes whenever the encrypted blob changes.
>>>>
>>>> Having this as an entry type simplifies the build process of the
>>>> resulting image. Otherwise some external script would have to run during
>>>> the build process to patch the iv and key into the generated device tree.
>>>
>>> OK, so the 'cipher' node ends up providing information on how to
>>> decode the image. But why put it in the root node? Would it not be
>>> better to put it in the node next to the one with the encrypted data?
>>> People might encrypt several images separately.
>>
>> Having several encrypted images with different keys/ivs is supported.
>> For this different iv-name-hint and/or different key-name-hint values
>> must be used.
>>
>> I only added this global cipher node because this is done in the same
>> way in boot/image-cipher.c. I did not want to introduce a new structure.
>> But if it is ok, i could remove the cipher node below root (/cipher) and
>> move the key and iv property to the cipher node next to the one with the
>> encrypted data.
>> This would simplify the structure in the device tree and the code.
>> In that case i would remove the iv-name-hint, since it is no longer
>> used. But i would keep some kind of key-name-hint to transport the
>> information which kind of key should be used, see below for an example.
>
> That seems like a good idea to me.
>
>>
>>>>
>>>>>
>>>>>> + """
>>>>>> +
>>>>>> + def __init__(self, section, etype, node):
>>>>>> + # Put this here to allow entry-docs and help to work without libfdt
>>>>>> + global state
>>>>>> + from binman import state
>>>>>> +
>>>>>> + super().__init__(section, etype, node)
>>>>>> + # The property key-filename is not required, because some implementations use keys that
>>>>>> + # are not embedded in the device tree, but e.g. in the device itself
>>>>>> + self.required_props = ['algo', 'key-name-hint', 'iv-filename']
>>>>>> + self._algo = fdt_util.GetString(self._node, 'algo')
>>>>>> + self._iv_name_hint = fdt_util.GetString(self._node, 'iv-name-hint')
>>>>>> + self._key_name_hint = fdt_util.GetString(self._node, 'key-name-hint')
>>>>>> + self._filename_iv = fdt_util.GetString(self._node, 'iv-filename')
>>>>>> + self._filename_key = fdt_util.GetString(self._node, 'key-filename')
>>>>>
>>>>> Here you should set these variables to None. Move the reading to ReadNode()
>>>>>
>>>>> Sorry if there are counter-examples in the source code. But this is
>>>>> the correct way.
>>>>>
>>>>>> +
>>>>>> + def ReadNode(self):
>>>>>> + super().ReadNode()
>>>>>> +
>>>>>> + iv_filename = tools.get_input_filename(self._filename_iv)
>>>>>> + self._iv = tools.read_file(iv_filename, binary=True)
>>>>>
>>>>> Please only read the node in this method. Move file reading until
>>>>> where it is needed.
>>>>>
>>>>>> +
>>>>>> + self._key = None
>>>>>> + if self._filename_key:
>>>>>> + key_filename = tools.get_input_filename(self._filename_key)
>>>>>> + self._key = tools.read_file(key_filename, binary=True)
>>>>>> +
>>>>>> + def gen_entries(self):
>>>>>> + super().gen_entries()
>>>>>> +
>>>>>> + cipher_node = state.AddSubnode(self._node.parent, "cipher")
>>>>>> + cipher_node.AddString("algo", self._algo)
>>>>>> + cipher_node.AddString("key-name-hint", self._key_name_hint)
>>>>>> + if self._iv_name_hint:
>>>>>> + cipher_node.AddString("iv-name-hint", self._iv_name_hint)
>>>>>> + else:
>>>>>> + cipher_node.AddData("iv", self._iv)
>>>>>> +
>>>>>> + if self._key or self._iv_name_hint:
>>>>>> + # add cipher node in root
>>>>>> + root_node = self._node.parent.parent.parent
>>>>>
>>>>> The root node is self.GetImage()._node
>>>>>
>>>>> But why are you adding something to the root node? This seems quite strange.
>>>>
>>>> This is shown in the example in the cover letter. The generated device
>>>> tree looks like this:
>>>>
>>>> \ {
>>>> cipher {
>>>> key-aes256-gcm-keyname {
>>>> key = <0x...>;
>>>> iv = <0x...>;
>>>> };
>>>> };
>>>>
>>>> images {
>>>> ...
>>>> some-bitstream {
>>>> ...
>>>> data = [...]
>>>> cipher {
>>>> algo = "aes256-gcm";
>>>> key-name-hint = "keyname";
>>>> };
>>>> };
>>>> ...
>>>>
>>>> The cipher node right below the root node (may) contain the actually
>>>> used key and iv.
>>>> The cipher node below the images node just points to the node inside the
>>>> global cipher node. For this the values of the algo, key-name-hint and
>>>> optionally the iv-name-hint are used.
>>>> The actual key is found in /cipher/key-<algo>-<key-name-hint> or
>>>> /cipher/key-<algo>-<key-name-hint>-<iv-name-hint>. This is implemented
>>>> this way, because it is also used in boot/image-cipher.c.
>>>
>>> Oh so how about putting the 'cipher' node in some-bitstream instead,
>>> or even just add the encryption info to the 'cipher' node you have
>>> shown? Why does it need to be in the root node?
>>>
>>>>
>>>> Side note:
>>>> If the hardware/board already contains a key in some secure storage, it
>>>> is not necessary to put the key into the device tree. In that case the
>>>> property key-name-hint can be used to identify which key should be used
>>>> for decryption.
>>>
>>> OK. In that case, perhaps we should have a property indicating that
>>> the key is external, if that isn't obvious.
>>
>> It is not obvious now if the key is external, since it is just a magic
>> string. I think the key source must support these use-cases:
>>
>> 1. Key is embedded in the device tree. If we do not have a global
>> /cipher node anymore, there is no need for a key id. Basically having a
>> key property containing the key itself would be enough in that case.
>>
>> For key embedded in the device tree this would be:
>>
>> some-bitstream {
>> ...
>> data = [...]
>> cipher {
>> algo = "aes256-gcm";
>> iv = <0x...>;
>> key = <0x...>;
>> };
>> };
>
> That looks good
>
>>
>> 2. The key is not embedded in the device tree. In that case some kind of
>> id is needed to identify which key should be used. This id would be
>> specific to the hardware that provides the key. Some kind of string
>> would probably be good here.
>>
>> If the key from some external source is used it could be something like
>> this:
>>
>> some-bitstream {
>> ...
>> data = [...]
>> cipher {
>> algo = "aes256-gcm";
>> iv = <0x...>;
>> key-source = "some-external-key-1";
>> };
>> };
>>
>> Would this be ok (having either a key or key-source property)?
>
> Seems good. I don't know what to use for 'key-source' but perhaps it
> could be a phandle reference to the device that holds it? Or just a
> string?
There can be multiple keys stored in the same device. Because of that, I
used a string in the next version. We can continue the discussion there.
>
>>
>>>
>>>>
>>>>>
>>>>>> + name = "cipher"
>>>>>> + cipher_node = root_node.FindNode(name)
>>>>>> + if not cipher_node:
>>>>>> + cipher_node = state.AddSubnode(root_node, name)
>>>>>> + key_node_name = (
>>>>>> + f"key-{self._algo}-{self._key_name_hint}-{self._iv_name_hint}"
>>>>>> + if self._iv_name_hint
>>>>>> + else f"key-{self._algo}-{self._key_name_hint}"
>>>>>> + )
>>>>>> + key_node = cipher_node.FindNode(key_node_name)
>>>>>> + if not key_node:
>>>>>
>>>>> This behaviour is not clearly documented above.
>>>>
>>>> Should i document this in the class doc of this entry or in the method
>>>> doc of gen_entries()?
>>>
>>> The class doc has the 'user' documentation, so it should go there. You
>>> can copy in the info from your cover letter and also explain the
>>> naming of the key node.
>>>
>>> I might have a few more comments once this is updated.
>>>
>>>>>
>>>>>> + key_node = state.AddSubnode(cipher_node, key_node_name)
>>>>>> + if self._key:
>>>>>> + key_node.AddData("key", self._key)
>>>>>> + if self._iv_name_hint:
>>>>>> + key_node.AddData("iv", self._iv)
>>>>>> +
>>>>>> + def ObtainContents(self):
>>>>>> + # ensure that linked content is not added to the device tree again from this entry
>>>>>> + self.SetContents(b'')
>>>>>> + return True
>>>>>> +
>>>>>> + def ProcessContents(self):
>>>>>> + # ensure that linked content is not added to the device tree again from this entry
>>>>>> + return self.ProcessContentsUpdate(b'')
>>>>>> --
>>>>>> 2.34.1
>>>>>>
>>> Regards,
>>> Simon
>>
>> Regards,
>> Christian
>
> Regards,
> Simon
Regards,
Christian
More information about the U-Boot
mailing list