[PATCH 1/2] doc swupdate.rst: clarify execution order for scripts

[Dominique Martinet]

Signed-off-by: Dominique Martinet <dominique...@atmark-techno.com>
---
Attempt at clarifying following discussion on the list:
https://groups.google.com/g/swupdate/c/Ih6Co_4CCq8/m/FmhCRWZtCQAJ

Rewriting someone's words is always difficult, feel free to adjust as
you see fit as I might not have kept your original intention through
very well.

doc/source/swupdate.rst | 45 ++++++++++++++++++++++++-----------------
1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
index 361f24840007..f53f4dbb18f5 100644
--- a/doc/source/swupdate.rst
+++ b/doc/source/swupdate.rst
@@ -80,7 +80,13 @@ General Overview

- Support for setting / erasing `EFI Boot Guard`_ variables

-- Support for preinstall scripts. They run before updating the images
+- Support for pre and post update commands run before the update starts
+ processing data and after the update has finished successfully.
+
+- Support for lua hooks, executed before any handler runs.
+
+- Support for preinstall scripts. They run after streamed handlers have
+ handled their data, and before regular handlers.

- Support for postinstall scripts. They run after updating the images.

@@ -345,24 +351,27 @@ A run of SWUpdate consists mainly of the following steps:
with the table in sw-description.
- Parse sw-description to determine which artefacts in the incoming SWU
are required. Not required artifacts are simply skipped.
- The meta file sw-description is declarative and executive. If "hooks"
- are defined in sw-description, they are executed at parse time. Hooks
- are executed before any artefact is evaluated and they are the best
- way to implement pre install functions.
+ If an "embedded-script" is defined, it is executed at this point
+ before parsing files.
+ If "hooks" are defined, they are executed as each file is parsed,
+ even if they will be skipped.
At the end of the parsing, SWUpdate builds an internal mapping for each artifact
to recognize which handler should be called for each of them.
-- Reads the cpio archive and proofs the CPIO checksum of each single file
- (this can be disabled in the configuration) and computes the sha256 sum if foreseen or if Signed Images
- are activated. SWUpdate stops if the archive is not complete verified.
-- check that all components described in sw-description are
- really in the cpio archive.
-- run the partitions handlers, if required.
-- runs pre-install scripts, if any. Please note: if artifacts are streamed, a preinstall script cannot be
- executed because it depends on the order the artifact (including the script) are packed into the CPIO archive.
- In this case, please use the "embedded-script" feature in sw-description to execute functions before any
- installation takes place.
-- if an artifact is found in SWU and it is marked with "installed-directly", proceed with installation,
- else extract it temporarily into TMPDIR.
+- runs the pre update command, if set
+- runs partition handlers, if required.
+- reads through the cpio archive one file at a time and either:
+ * execute handlers for each file marked as "installed-directly".
+ checksum is checked while the data is streamed to handler, and copy will
+ be marked as having failed if checksum was not correct failing the rest
+ of the install.
+ * copy other files to a temporary location while checking checksums,
+ stopping if there was a mismatch.
+- iterates through all `scripts` and call the corresponding
+ handler for pre-install scripts.
+ Please note: if artifacts are streamed, they will be extracted
+ before this runs. If earlier execution is required, please use
+ the "embedded-script" or hooks features to ensure code is run
+ before installation takes place.
- iterates through all `images` and call the corresponding
handler for installing on target.
- iterates through all `files` and call the corresponding
@@ -372,7 +381,7 @@ A run of SWUpdate consists mainly of the following steps:
- iterates through all `bootenv` and updates the bootloader environment.
- reports the status to the operator through the notification interface
(logging, traces) and through the progress interface.
-- if a postinstall command is foreseen (for example to reboot the device), call it.
+- runs the post update command, if set.

The first step that fails, stops the entire procedure and
an error is reported.
--
2.30.2

[Dominique Martinet]

swupdate has no need to recreate the same key from user input, so
generating key/iv from password/salt does not make sense.
Suggest using random data instead as has been suggested in
ac75ad653502 ("doc swupdate.rst: clarify execution order for scripts")

Signed-off-by: Dominique Martinet <dominique...@atmark-techno.com>
---

While I'm touching documentation, the encrypted image documentation has
been bugging me; generating keys from passphrase with a deprecated
method that gives a warning when users try it is not good.

Likewise, feel free to adjust or request changes.

doc/source/encrypted_images.rst | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/doc/source/encrypted_images.rst b/doc/source/encrypted_images.rst
index 867230996812..1dc974424a7c 100644
--- a/doc/source/encrypted_images.rst
+++ b/doc/source/encrypted_images.rst
@@ -9,22 +9,17 @@ bit key length but you may use other key lengths as well.
Building an Encrypted SWU Image
-------------------------------

-First, create a key via `openssl` which is part of the OpenSSL project.
+First, create a key; for aes-256-cbc we need 32 bytes of key and 16 bytes
+for an initialisation vector (IV).
A complete documentation can be found at the
`OpenSSL Website <https://www.openssl.org/docs/manmaster/man1/openssl.html>`_.

::

- openssl enc -aes-256-cbc -k <PASSPHRASE> -P -md sha1
-
-The key and initialization vector is generated based on the given ``<PASSPHRASE>``.
-The output of the above command looks like this:
-
-::
-
- salt=CE7B0488EFBF0D1B
- key=B78CC67DD3DC13042A1B575184D4E16D6A09412C242CE253ACEE0F06B5AD68FC
- iv =65D793B87B6724BB27954C7664F15FF3
+ openssl rand -hex 32
+ # key, for example 390ad54490a4a5f53722291023c19e08ffb5c4677a59e958c96ffa6e641df040
+ openssl rand -hex 16
+ # IV, for example d5d601bacfe13100b149177318ebc7a4

Then, encrypt an image using this information via

@@ -43,12 +38,11 @@ separated by whitespace, e.g., for above example values

::

- B78CC67DD3DC13042A1B575184D4E16D6A09412C242CE253ACEE0F06B5AD68FC 65D793B87B6724BB27954C7664F15FF3
+ 390ad54490a4a5f53722291023c19e08ffb5c4677a59e958c96ffa6e641df040 d5d601bacfe13100b149177318ebc7a4


-For earlier versions of SWUpdate it was falsely noted that passing the SALT as a
-3rd parameter would increase security. Key and IV are enough for maximum security,
-salt doesn't add any value.
+Previous versions of SWUpdate allowed for a salt as third word in key file,
+that was never actually used for aes and has been removed.

You should change the IV with every encryption, see CWE-329_. The ``ivt``
sw-description attribute overrides the key file's IV for one specific image.
--
2.30.2

[Stefano Babic]

Hi Dominique,

On 08.04.21 09:32, Dominique Martinet wrote:
> Signed-off-by: Dominique Martinet <dominique...@atmark-techno.com>
> ---
> Attempt at clarifying following discussion on the list:
> https://groups.google.com/g/swupdate/c/Ih6Co_4CCq8/m/FmhCRWZtCQAJ
>
> Rewriting someone's words is always difficult, feel free to adjust as
> you see fit as I might not have kept your original intention through
> very well.
>
> doc/source/swupdate.rst | 45 ++++++++++++++++++++++++-----------------
> 1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
> index 361f24840007..f53f4dbb18f5 100644
> --- a/doc/source/swupdate.rst
> +++ b/doc/source/swupdate.rst
> @@ -80,7 +80,13 @@ General Overview
>
> - Support for setting / erasing `EFI Boot Guard`_ variables
>
> -- Support for preinstall scripts. They run before updating the images
> +- Support for pre and post update commands run before the update starts
> + processing data and after the update has finished successfully.
> +
> +- Support for lua hooks, executed before any handler runs.
> +
> +- Support for preinstall scripts. They run after streamed handlers have
> + handled their data, and before regular handlers.

Ok

>
> - Support for postinstall scripts. They run after updating the images.
>
> @@ -345,24 +351,27 @@ A run of SWUpdate consists mainly of the following steps:
> with the table in sw-description.
> - Parse sw-description to determine which artefacts in the incoming SWU
> are required. Not required artifacts are simply skipped.
> - The meta file sw-description is declarative and executive. If "hooks"
> - are defined in sw-description, they are executed at parse time. Hooks
> - are executed before any artefact is evaluated and they are the best
> - way to implement pre install functions.

Should we drop the whole stuff above ? This is not described later.

The point is the buggy CPIO archive distributed with Linux distro (this
does not affect OE). cpio in distros does not set the checksum when a
single file is bigger as 2GB. Having this note (maybe rewritten ?) is
important for debian users.

> + If an "embedded-script" is defined, it is executed at this point
> + before parsing files.

This is not correct. The embedded-script is loaded, but it is executed
when the parser hits a hook, so while an entry is parsed.

Entries could be also not a file - see for example partition, or toggler
(ssbl_handler for example). Use a generic name here (entry or whatever)
- in fact, it is foreseen that some handlers have no image / file
associated to it. (NO_DATA_HANDLER).

> + If "hooks" are defined, they are executed as each file is parsed,
> + even if they will be skipped.

Well, the hook can decide if the file is skipped. So the entry is
parsed, the hook is called, and then it is decided if it is skipped.

> At the end of the parsing, SWUpdate builds an internal mapping for each artifact
> to recognize which handler should be called for each of them.
> -- Reads the cpio archive and proofs the CPIO checksum of each single file
> - (this can be disabled in the configuration) and computes the sha256 sum if foreseen or if Signed Images
> - are activated. SWUpdate stops if the archive is not complete verified.

Agree, this is wrong - it assumes that SWUpdate reads an ertifact,
computes the sha256 and stops before installing. But in this way, it is
duty to save temporary the data. SWUpdate works on the fly, and
decryption / decompression / hash computation is done with buffers in
memory on the fly. Nice to remove.

> -- check that all components described in sw-description are
> - really in the cpio archive.
> -- run the partitions handlers, if required.
> -- runs pre-install scripts, if any. Please note: if artifacts are streamed, a preinstall script cannot be
> - executed because it depends on the order the artifact (including the script) are packed into the CPIO archive.
> - In this case, please use the "embedded-script" feature in sw-description to execute functions before any
> - installation takes place.
> -- if an artifact is found in SWU and it is marked with "installed-directly", proceed with installation,
> - else extract it temporarily into TMPDIR.
> +- runs the pre update command, if set
> +- runs partition handlers, if required.
> +- reads through the cpio archive one file at a time and either:
> + * execute handlers for each file marked as "installed-directly".
> + checksum is checked while the data is streamed to handler, and copy will
> + be marked as having failed if checksum was not correct failing the rest
> + of the install.
> + * copy other files to a temporary location while checking checksums,
> + stopping if there was a mismatch.

ok

> +- iterates through all `scripts` and call the corresponding
> + handler for pre-install scripts.
> + Please note: if artifacts are streamed, they will be extracted
> + before this runs. If earlier execution is required, please use
> + the "embedded-script" or hooks features to ensure code is run
> + before installation takes place.
> - iterates through all `images` and call the corresponding
> handler for installing on target.
> - iterates through all `files` and call the corresponding
> @@ -372,7 +381,7 @@ A run of SWUpdate consists mainly of the following steps:
> - iterates through all `bootenv` and updates the bootloader environment.
> - reports the status to the operator through the notification interface
> (logging, traces) and through the progress interface.
> -- if a postinstall command is foreseen (for example to reboot the device), call it.
> +- runs the post update command, if set.
>
> The first step that fails, stops the entire procedure and
> an error is reported.
>

Best regards,
Stefano Babic

--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=====================================================================

[Dominique Martinet]

Stefano Babic wrote on Thu, Apr 08, 2021 at 06:29:04PM +0200:
> > - Support for postinstall scripts. They run after updating the images.
> > @@ -345,24 +351,27 @@ A run of SWUpdate consists mainly of the following steps:
> > with the table in sw-description.
> > - Parse sw-description to determine which artefacts in the incoming SWU
> > are required. Not required artifacts are simply skipped.
> > - The meta file sw-description is declarative and executive. If "hooks"
> > - are defined in sw-description, they are executed at parse time. Hooks
> > - are executed before any artefact is evaluated and they are the best
> > - way to implement pre install functions.
>
> Should we drop the whole stuff above ? This is not described later.
>
> The point is the buggy CPIO archive distributed with Linux distro (this does
> not affect OE). cpio in distros does not set the checksum when a single file
> is bigger as 2GB. Having this note (maybe rewritten ?) is important for
> debian users.

Sorry, I am not quite sure what exactly you are referring to?
In my head the whole few points below are part of parsing sw-description
and decribe execution happen while parsing it before looking at other
files; so they replace the above statement.
If you think this is not clear it might need an extra line?

I am also not sure what you mean with the cpio archive in linux distros;
I removed that the cpio checksum is checked below as well as the
sha256sum that might be worth keeping, was it that?

> > + If an "embedded-script" is defined, it is executed at this point
> > + before parsing files.
>
> This is not correct. The embedded-script is loaded, but it is executed when
> the parser hits a hook, so while an entry is parsed.

Yes and no. the embedded-script is fully evaluated at this point before
hooks are hit. So if you just define functions then nothing happens
until hooks happen, but if you have declarating stuff then they will
just appear to have been executed e.g. the following sw-description will
print "embed" once when evaluating the embed script; then move on to
parsing sub items and run check_hook twice (once for each file) :

software = {
embedded-script = "
require(\"swupdate\")

function check_hook(image)
swupdate.trace(\"test hook\")
return true, image
end

swupdate.trace(\"embed\")"
...
files: (
{ ... hook = "check_hook"; ... },
{ ... hook = "check_hook"; ... },
);
};


I found about this "feature" will writing the documentation.

> Entries could be also not a file - see for example partition, or toggler
> (ssbl_handler for example). Use a generic name here (entry or whatever) - in
> fact, it is foreseen that some handlers have no image / file associated to
> it. (NO_DATA_HANDLER).

Yes for this, sorry for wording.

> > + If "hooks" are defined, they are executed as each file is parsed,
> > + even if they will be skipped.
>
> Well, the hook can decide if the file is skipped. So the entry is parsed,
> the hook is called, and then it is decided if it is skipped.

Yes; I understand how this works, but there are other skip mechanisms
(e.g. version comparison) and people might be surprised if hooks are
executed even if install-if-different is set and version match installed
software for example.
The hook could decide to install anyway, so it makes sense to do it that
way; it just needs mentioning.

Help with the wording is appreciated if you think this is confusing.



I will send v2 after reply

--
Dominique

[Stefano Babic]

On 09.04.21 02:12, Dominique Martinet wrote:
> Stefano Babic wrote on Thu, Apr 08, 2021 at 06:29:04PM +0200:
>>> - Support for postinstall scripts. They run after updating the images.
>>> @@ -345,24 +351,27 @@ A run of SWUpdate consists mainly of the following steps:
>>> with the table in sw-description.
>>> - Parse sw-description to determine which artefacts in the incoming SWU
>>> are required. Not required artifacts are simply skipped.
>>> - The meta file sw-description is declarative and executive. If "hooks"
>>> - are defined in sw-description, they are executed at parse time. Hooks
>>> - are executed before any artefact is evaluated and they are the best
>>> - way to implement pre install functions.
>>
>> Should we drop the whole stuff above ? This is not described later.
>>
>> The point is the buggy CPIO archive distributed with Linux distro (this does
>> not affect OE). cpio in distros does not set the checksum when a single file
>> is bigger as 2GB. Having this note (maybe rewritten ?) is important for
>> debian users.
>
> Sorry, I am not quite sure what exactly you are referring to?
> In my head the whole few points below are part of parsing sw-description
> and decribe execution happen while parsing it before looking at other
> files; so they replace the above statement.

Yes - the information above is at the wrong place, so I agree it should
be removed. I do not know where to put it back, the topic comes more as
once on ML.

> If you think this is not clear it might need an extra line?

No, I reread, this part is for parsing and the issue related to CPIO is
not part of parsing.

>
> I am also not sure what you mean with the cpio archive in linux distros;
> I removed that the cpio checksum is checked below as well as the
> sha256sum that might be worth keeping, was it that?

SWUpdate uses cpio "new ascii format with checksum". Checksum is very
weak, but it still something. However, cpio (a very old program)
delivered with Distros (Debian, Fedora, ...) has a bug and the checksum
is set to zero if the file is bigger than 2GB. When SWUpdate chekcs it,
it raises the error.

On OE this is fixed in meta-swupdate. There is quite no CPIO maintainer,
I sent the patch to cpio ML, but this list is quite dead. There are a
large set of patches that are simply ignored. So if you are not using OE
or buildroot but Debian, you have to patch CPIO yourself or configure
SWUpdate to ignore CPIO checksum.

>
>>> + If an "embedded-script" is defined, it is executed at this point
>>> + before parsing files.
>>
>> This is not correct. The embedded-script is loaded, but it is executed when
>> the parser hits a hook, so while an entry is parsed.
>
> Yes and no. the embedded-script is fully evaluated at this point before
> hooks are hit. So if you just define functions then nothing happens
> until hooks happen, but if you have declarating stuff then they will
> just appear to have been executed e.g.

Oh yes, that is correct - as in any script. It is also a use case.

> the following sw-description will
> print "embed" once when evaluating the embed script; then move on to
> parsing sub items and run check_hook twice (once for each file) :
>
> software = {
> embedded-script = "
> require(\"swupdate\")
>
> function check_hook(image)
> swupdate.trace(\"test hook\")
> return true, image
> end
>
> swupdate.trace(\"embed\")"
> ...
> files: (
> { ... hook = "check_hook"; ... },
> { ... hook = "check_hook"; ... },
> );
> };
>
>
> I found about this "feature" will writing the documentation.

Agree - it is a "hidden" feature, but it is correct what you say.

>
>> Entries could be also not a file - see for example partition, or toggler
>> (ssbl_handler for example). Use a generic name here (entry or whatever) - in
>> fact, it is foreseen that some handlers have no image / file associated to
>> it. (NO_DATA_HANDLER).
>
> Yes for this, sorry for wording.
>
>>> + If "hooks" are defined, they are executed as each file is parsed,
>>> + even if they will be skipped.
>>
>> Well, the hook can decide if the file is skipped. So the entry is parsed,
>> the hook is called, and then it is decided if it is skipped.
>
> Yes; I understand how this works, but there are other skip mechanisms
> (e.g. version comparison) and people might be surprised if hooks are
> executed even if install-if-different is set and version match installed
> software for example.

Well, this is part of the parsing. Anyway it is ok for me if you think
that enforcing this makes things clearer.

> The hook could decide to install anyway, so it makes sense to do it that
> way; it just needs mentioning.

Fine.

>
> Help with the wording is appreciated if you think this is confusing.

I guess I cannot do a lot. My phrases looked confusing to you (and I
admit, one reason is that some additional text was added in later patch
without checking if the whole makes sense), so appreciated that you want
to fix it. It will be nice to have more comments to check if what we are
writing is just clear to us or there is a common understanding.

>
>
>
> I will send v2 after reply
>