Hi Christian, On Fri, 30 Jun 2023 at 11:28, Taedcke, Christian <christian.taedcke-...@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-...@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-...@weidmueller.com> > >>> wrote: > >>>> > >>>> From: Christian Taedcke <christian.taed...@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.taed...@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.taed...@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? > > > > >> > >>> > >>>> + 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
