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-...@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?
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
