[PATCH 0/3] binman: Add support for externally encrypted blobs
From: Christian Taedcke christian.taedcke@weidmueller.com
This series adds the functionality to handle externally encrypted blobs to binman. It includes the functionality itself and the corresponding unit tests. The generated device tree structure is similar to the structure used in the already implemented cipher node in boot/image-cipher.c.
The following block shows an example on how to use this functionality. In the device tree that is parsed by binman a new node encrypted is used:
/ { binman { filename = "u-boot.itb"; fit { ... images { some-bitstream { ... image_bitstream: blob-ext { filename = "bitstream.bin"; }; encrypted { content = <&image_bitstream>; algo = "aes256-gcm"; key-name-hint = "keyname"; iv-filename = "bitstream.bin.iv"; key-filename = "bitstream.bin.key"; }; ...
This results in an generated fit image containing the following information:
\ { cipher { key-aes256-gcm-keyname { key = <0x...>; iv = <0x...>; }; };
images { ... some-bitstream { ... data = [...] cipher { algo = "aes256-gcm"; key-name-hint = "keyname"; }; }; ...
Christian Taedcke (3): binman: Add support for externally encrypted blobs binman: Allow cipher node as special section binman: Add tests for etype encrypted
tools/binman/etype/encrypted.py | 98 +++++++++++++++++++ tools/binman/etype/section.py | 2 +- tools/binman/ftest.py | 69 +++++++++++++ .../binman/test/282_encrypted_no_content.dts | 15 +++ tools/binman/test/283_encrypted_no_algo.dts | 19 ++++ .../test/284_encrypted_invalid_iv_file.dts | 22 +++++ tools/binman/test/285_encrypted.dts | 29 ++++++ tools/binman/test/286_encrypted_key_file.dts | 30 ++++++ .../test/287_encrypted_iv_name_hint.dts | 30 ++++++ 9 files changed, 313 insertions(+), 1 deletion(-) create mode 100644 tools/binman/etype/encrypted.py 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.dts create mode 100644 tools/binman/test/286_encrypted_key_file.dts create mode 100644 tools/binman/test/287_encrypted_iv_name_hint.dts
From: Christian Taedcke christian.taedcke@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@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@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 + - iv-name-hint: The name hint for the iv + - 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 + image). Optionally it also creates a cipher node in the root of the device + tree containg key and iv information. + """ + + 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') + + def ReadNode(self): + super().ReadNode() + + iv_filename = tools.get_input_filename(self._filename_iv) + self._iv = tools.read_file(iv_filename, binary=True) + + 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 + 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: + 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'')
Hi Christian,
On Tue, 27 Jun 2023 at 08:39, christian.taedcke-oss@weidmueller.com wrote:
From: Christian Taedcke christian.taedcke@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@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@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
- iv-name-hint: The name hint for the iv
what is the iv?
- 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.
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.
- """
- def __init__(self, section, etype, node):
# Put this here to allow entry-docs and help to work without libfdtglobal statefrom binman import statesuper().__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 itselfself.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 = Noneif 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 rootroot_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.
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_hintelse 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.
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 entryself.SetContents(b'')return True- def ProcessContents(self):
# ensure that linked content is not added to the device tree again from this entryreturn self.ProcessContentsUpdate(b'')-- 2.34.1
Regards, Simon
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@weidmueller.com wrote:
From: Christian Taedcke christian.taedcke@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@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@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 algorithmWhat 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?
- iv-name-hint: The name hint for the ivwhat is the iv?
Initialization Vector. Should i use the full name here?
- 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 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.
- """
- def __init__(self, section, etype, node):
# Put this here to allow entry-docs and help to work without libfdtglobal statefrom binman import statesuper().__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 itselfself.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 = Noneif 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 rootroot_node = self._node.parent.parent.parentThe 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.
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.
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_hintelse 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()?
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 entryself.SetContents(b'')return True- def ProcessContents(self):
# ensure that linked content is not added to the device tree again from this entryreturn self.ProcessContentsUpdate(b'')-- 2.34.1
Regards, Simon
Regards, Christian
Hi Christian,
On Fri, 30 Jun 2023 at 09:20, Taedcke, Christian christian.taedcke-oss@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@weidmueller.com wrote:
From: Christian Taedcke christian.taedcke@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@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@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 algorithmWhat 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?
- iv-name-hint: The name hint for the ivwhat 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.
- """
- def __init__(self, section, etype, node):
# Put this here to allow entry-docs and help to work without libfdtglobal statefrom binman import statesuper().__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 itselfself.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 = Noneif 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 rootroot_node = self._node.parent.parent.parentThe 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.
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_hintelse 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 entryself.SetContents(b'')return True- def ProcessContents(self):
# ensure that linked content is not added to the device tree again from this entryreturn self.ProcessContentsUpdate(b'')-- 2.34.1
Regards, Simon
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@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@weidmueller.com wrote:
From: Christian Taedcke christian.taedcke@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@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@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 algorithmWhat 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.
- iv-name-hint: The name hint for the ivwhat 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.
- """
- def __init__(self, section, etype, node):
# Put this here to allow entry-docs and help to work without libfdtglobal statefrom binman import statesuper().__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 itselfself.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 = Noneif 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 rootroot_node = self._node.parent.parent.parentThe 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...>; }; };
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)?
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_hintelse 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 entryself.SetContents(b'')return True- def ProcessContents(self):
# ensure that linked content is not added to the device tree again from this entryreturn self.ProcessContentsUpdate(b'')-- 2.34.1
Regards, Simon
Regards, Christian
Hi Christian,
On Fri, 30 Jun 2023 at 11:28, Taedcke, Christian christian.taedcke-oss@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@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@weidmueller.com wrote:
From: Christian Taedcke christian.taedcke@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@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@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 algorithmWhat 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 ivwhat 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 libfdtglobal statefrom binman import statesuper().__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 itselfself.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 = Noneif 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 rootroot_node = self._node.parent.parent.parentThe 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:
- 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
- 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_hintelse 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 entryself.SetContents(b'')return True- def ProcessContents(self):
# ensure that linked content is not added to the device tree again from this entryreturn self.ProcessContentsUpdate(b'')-- 2.34.1
Regards, Simon
Regards, Christian
Regards, Simon
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@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@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@weidmueller.com wrote:
From: Christian Taedcke christian.taedcke@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@weidmueller.com
tools/binman/etype/encrypted.py | 98 +++++++++++++++++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 tools/binman/etype/encrypted.pydiff --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@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 algorithmWhat 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 ivwhat 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 libfdtglobal statefrom binman import statesuper().__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 itselfself.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 = Noneif 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 rootroot_node = self._node.parent.parent.parentThe 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:
- 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
- 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_hintelse 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 entryself.SetContents(b'')return True- def ProcessContents(self):
# ensure that linked content is not added to the device tree again from this entryreturn self.ProcessContentsUpdate(b'')-- 2.34.1
Regards, Simon
Regards, Christian
Regards, Simon
Regards, Christian
From: Christian Taedcke christian.taedcke@weidmueller.com
The new encrypted etype generates a cipher node in the device tree that should not be evaluated by binman, but still be kept in the output device tree.
Signed-off-by: Christian Taedcke christian.taedcke@weidmueller.com ---
tools/binman/etype/section.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index c36edd1350..56abfd5129 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -178,7 +178,7 @@ class Entry_section(Entry): Returns: bool: True if the node is a special one, else False """ - return node.name.startswith('hash') or node.name.startswith('signature') + return node.name.startswith('hash') or node.name.startswith('signature') or node.name.startswith('cipher')
def ReadNode(self): """Read properties from the section node"""
On Tue, 27 Jun 2023 at 08:39, christian.taedcke-oss@weidmueller.com wrote:
From: Christian Taedcke christian.taedcke@weidmueller.com
The new encrypted etype generates a cipher node in the device tree that should not be evaluated by binman, but still be kept in the output device tree.
Signed-off-by: Christian Taedcke christian.taedcke@weidmueller.com
tools/binman/etype/section.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
From: Christian Taedcke christian.taedcke@weidmueller.com
Add tests to reach 100% code coverage for the added etype encrypted.
Signed-off-by: Christian Taedcke christian.taedcke@weidmueller.com ---
tools/binman/ftest.py | 69 +++++++++++++++++++ .../binman/test/282_encrypted_no_content.dts | 15 ++++ tools/binman/test/283_encrypted_no_algo.dts | 19 +++++ .../test/284_encrypted_invalid_iv_file.dts | 22 ++++++ tools/binman/test/285_encrypted.dts | 29 ++++++++ tools/binman/test/286_encrypted_key_file.dts | 30 ++++++++ .../test/287_encrypted_iv_name_hint.dts | 30 ++++++++ 7 files changed, 214 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.dts create mode 100644 tools/binman/test/286_encrypted_key_file.dts create mode 100644 tools/binman/test/287_encrypted_iv_name_hint.dts
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 43b4f850a6..3fb57e964e 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' 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,69 @@ fdt fdtmap Extract the devicetree blob from the fdtmap ['fit']) self.assertIn("Node '/fit': Missing tool: 'mkimage'", str(e.exception))
+ def testEncryptedNoContent(self): + with self.assertRaises(ValueError) as e: + self._DoReadFileDtb('282_encrypted_no_content.dts', update_dtb=True) + self.assertIn("Node '/binman/fit/images/u-boot/encrypted': Collection must have a 'content' property", str(e.exception)) + + def testEncryptedNoAlgo(self): + with self.assertRaises(ValueError) as e: + self._DoReadFileDtb('283_encrypted_no_algo.dts', update_dtb=True) + self.assertIn("Node '/binman/fit/images/u-boot/encrypted': 'encrypted' entry is missing properties: algo key-name-hint iv-filename", str(e.exception)) + + def testEncryptedInvalidIvfile(self): + with self.assertRaises(ValueError) as e: + self._DoReadFileDtb('284_encrypted_invalid_iv_file.dts', update_dtb=True) + self.assertIn("Filename 'invalid-iv-file' not found in input path", + str(e.exception)) + + def testEncryptedNoKey(self): + data = self._DoReadFileDtb('285_encrypted.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-name-hint-value', node.props['key-name-hint'].value) + self.assertEqual(ENCRYPTED_IV_DATA, tools.to_bytes(''.join(node.props['iv'].value))) + self.assertNotIn('iv-name-hint', node.props) + + node = dtb.GetNode('/cipher') + self.assertIsNone(node) + + def testEncryptedKeyFile(self): + data = self._DoReadFileDtb('286_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('key-name-hint-value', node.props['key-name-hint'].value) + self.assertEqual(ENCRYPTED_IV_DATA, tools.to_bytes(''.join(node.props['iv'].value))) + self.assertNotIn('iv-name-hint', node.props) + + node = dtb.GetNode('/cipher/key-algo-name-key-name-hint-value') + self.assertEqual(ENCRYPTED_KEY_DATA, b''.join(node.props['key'].value)) + self.assertNotIn('iv', node.props) + + def testEncryptedIvNameHint(self): + data = self._DoReadFileDtb('287_encrypted_iv_name_hint.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('iv-name-hint-value', node.props['iv-name-hint'].value) + self.assertEqual('key-name-hint-value', node.props['key-name-hint'].value) + self.assertNotIn('iv', node.props) + + node = dtb.GetNode('/cipher/key-algo-name-key-name-hint-value-iv-name-hint-value') + self.assertEqual(ENCRYPTED_IV_DATA, tools.to_bytes(''.join(node.props['iv'].value))) + self.assertNotIn('key', 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 { + }; + }; + }; + }; + }; +}; diff --git a/tools/binman/test/283_encrypted_no_algo.dts b/tools/binman/test/283_encrypted_no_algo.dts new file mode 100644 index 0000000000..71975c0116 --- /dev/null +++ b/tools/binman/test/283_encrypted_no_algo.dts @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/; + +/ { + binman { + fit { + images { + u-boot { + encrypted { + content = <&data>; + }; + + data: data { + }; + }; + }; + }; + }; +}; diff --git a/tools/binman/test/284_encrypted_invalid_iv_file.dts b/tools/binman/test/284_encrypted_invalid_iv_file.dts new file mode 100644 index 0000000000..cce307965c --- /dev/null +++ b/tools/binman/test/284_encrypted_invalid_iv_file.dts @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/; + +/ { + binman { + fit { + images { + u-boot { + encrypted { + content = <&data>; + algo = "some-algo"; + key-name-hint = "key"; + iv-filename = "invalid-iv-file"; + }; + + data: data { + }; + }; + }; + }; + }; +}; diff --git a/tools/binman/test/285_encrypted.dts b/tools/binman/test/285_encrypted.dts new file mode 100644 index 0000000000..ed5babf26e --- /dev/null +++ b/tools/binman/test/285_encrypted.dts @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + fit { + description = "test desc"; + + images { + u-boot { + blob: blob { + filename = "blobfile"; + }; + + encrypted { + content = <&blob>; + algo = "algo-name"; + key-name-hint = "key-name-hint-value"; + iv-filename = "encrypted-file.iv"; + }; + }; + }; + }; + }; +}; diff --git a/tools/binman/test/286_encrypted_key_file.dts b/tools/binman/test/286_encrypted_key_file.dts new file mode 100644 index 0000000000..56fdb24f9f --- /dev/null +++ b/tools/binman/test/286_encrypted_key_file.dts @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + fit { + description = "test desc"; + + images { + u-boot { + blob: blob { + filename = "blobfile"; + }; + + encrypted { + content = <&blob>; + algo = "algo-name"; + key-name-hint = "key-name-hint-value"; + iv-filename = "encrypted-file.iv"; + key-filename = "encrypted-file.key"; + }; + }; + }; + }; + }; +}; diff --git a/tools/binman/test/287_encrypted_iv_name_hint.dts b/tools/binman/test/287_encrypted_iv_name_hint.dts new file mode 100644 index 0000000000..06c0735e61 --- /dev/null +++ b/tools/binman/test/287_encrypted_iv_name_hint.dts @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + fit { + description = "test desc"; + + images { + u-boot { + blob: blob { + filename = "blobfile"; + }; + + encrypted { + content = <&blob>; + algo = "algo-name"; + iv-name-hint = "iv-name-hint-value"; + key-name-hint = "key-name-hint-value"; + iv-filename = "encrypted-file.iv"; + }; + }; + }; + }; + }; +};
On Tue, 27 Jun 2023 at 08:39, christian.taedcke-oss@weidmueller.com wrote:
From: Christian Taedcke christian.taedcke@weidmueller.com
Add tests to reach 100% code coverage for the added etype encrypted.
Signed-off-by: Christian Taedcke christian.taedcke@weidmueller.com
tools/binman/ftest.py | 69 +++++++++++++++++++ .../binman/test/282_encrypted_no_content.dts | 15 ++++ tools/binman/test/283_encrypted_no_algo.dts | 19 +++++ .../test/284_encrypted_invalid_iv_file.dts | 22 ++++++ tools/binman/test/285_encrypted.dts | 29 ++++++++ tools/binman/test/286_encrypted_key_file.dts | 30 ++++++++ .../test/287_encrypted_iv_name_hint.dts | 30 ++++++++ 7 files changed, 214 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.dts create mode 100644 tools/binman/test/286_encrypted_key_file.dts create mode 100644 tools/binman/test/287_encrypted_iv_name_hint.dts
Reviewed-by: Simon Glass sjg@chromium.org
participants (3)
-
christian.taedcke-oss＠weidmueller.com -
Simon Glass -
Taedcke, Christian