bootutil: Refactor encrypted.c to have the same logic as image_validate #1658

RaphaelDupont · 2023-03-22T15:45:23Z

Currently encrypted.c contains all the implementation of the functions inside enc_key.h for each configuration (RSA, EC256 and X25519).

The purpose of this pr is to adopt a logic similar to the file organization of image_validate to make it easier to add other configurations.

Note:
I'm not satisfied with the current state of fake_rng because it loses its static attribute.
I think putting it in encrypted_priv.h could solve this issue but I'm not sure if it is a good way to solve it.

davidvincze

Thank you for this patch. I think it's really useful.

A few initial comments and:
May I ask you to add the MCUBOOT_ENC_IMAGES and MCUBOOT_ENCRYPT_<...> config macros to the samples/mcuboot_config/mcuboot_config.template.h file with a short description? Thanks.
This is a bit out of the scope of this PR but it kind of belongs here.

boot/bootutil/src/encrypted_priv.h

boot/bootutil/src/encrypt_kw.c

boot/bootutil/src/encrypt_misc.c

boot/bootutil/src/encrypted_priv.h

boot/bootutil/src/encrypted.c

boot/bootutil/src/encrypted_priv.h

boot/bootutil/src/encrypt_ec256.c

boot/bootutil/src/encrypted_priv.h

boot/bootutil/src/encrypt_kw.c

davidvincze

Thank you for the updates!

There is a type in your last commit message, can you update that with a force push?
And please rebase your patches if required.
Sorry, I didn't intend the previous comment as a separate one, but as part of the review.

samples/mcuboot_config/mcuboot_config.template.h

davidvincze

Thanks for the updates.

davidvincze · 2023-06-06T13:56:05Z

@nvlsianpu , @de-nordic, espressif and zephyr related files are directly affected (new files introoduced) in this change, could you approve this change if you think it won't cause any build issues?

de-nordic · 2023-06-06T15:30:47Z

@nvlsianpu , @de-nordic, espressif and zephyr related files are directly affected (new files introoduced) in this change, could you approve this change if you think it won't cause any build issues?

Let me test that with our code for Zephyr.
We lack espressif expert voice, but this is not unique to this PR, and I do not really know how to address this issue.

davidvincze · 2023-06-09T12:26:27Z

@nvlsianpu , @de-nordic, espressif and zephyr related files are directly affected (new files introoduced) in this change, could you approve this change if you think it won't cause any build issues?

Let me test that with our code for Zephyr. We lack espressif expert voice, but this is not unique to this PR, and I do not really know how to address this issue.

I guess we'll definitely have at least some kind of espressif voice if it breaks. 😅

davidvincze · 2023-06-09T13:59:29Z

@RaphaelDupont may I ask you to rebase the PR?
New patches have been merged recently and there are conflicting modifications in boot/bootutil/src/encrypted.c.

RaphaelDupont · 2023-06-13T08:35:56Z

@RaphaelDupont may I ask you to rebase the PR? New patches have been merged recently and there are conflicting modifications in boot/bootutil/src/encrypted.c.

The branch is now up-to-date.
Do you have other suggestions to make ?
Regarding the way some files are included for example.

de-nordic

There is some squashing needed.
I do not think that we need "bootutil : Reformat code according to suggestions" commits that look like review fixes on main.

RaphaelDupont · 2023-06-16T07:32:47Z

There is some squashing needed. I do not think that we need "bootutil : Reformat code according to suggestions" commits that look like review fixes on main.

Done

RaphaelDupont · 2023-07-28T13:36:32Z

Did you find other issues with this PR ?

de-nordic · 2023-08-01T15:32:08Z

I have run encryption and signature tests for ECDSA, RSA and 25519 against latest Zephyr and they seem to boot fine.
I am still trying to run test against sdk-nrf.

de-nordic · 2023-06-20T11:10:10Z

boot/bootutil/src/encrypt_ec256.c

+
+    if ((rc = mbedtls_asn1_get_tag(p, end, &len,
+                    MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE)) != 0) {
+        return -1;


No hardcoded errors. Give them defines that can be used by caller.

I think it can be a subject for an all other PR.
The error values of parse_ec256_enckey are hardcoded but they are also not used.

rc = parse_ec256_enckey(&cp, cpend, private_key); if (rc) { return -1; }

The EC256 configuration is not the only one affected by this problem.

It would be a good opportinunity to discuss if MCUBoot should define its own error values or just propagate the one
it receives from the crypto module it uses (for example : Mbedtls).

It would be a good opportinunity to discuss if MCUBoot should define its own error values or just propagate the one it receives from the crypto module it uses (for example : Mbedtls).

The entire IT industry should figure it out, it is really annoying to be forced to debug where did the -EINVAL comes from, because everything returns it for some reason or an other.

I checked how parse_ec256_enckey return value is handled on the main branch and the error is propagated when there is one.
So it is a change I made when I did the refactoring...
I reverted this.

Currently encrypted.c contains all the implementation of the functions inside enc_key.h for each configuration (RSA, EC256 and X25519). The purpose of this pr is to adopt a logic similar to the file organization of image_validate to make it easier to add other confifurations. Signed-off-by: Raphael Dupont <raphael7dup@gmail.com>

Signed-off-by: Raphael Dupont <raphael7dup@gmail.com>

github-actions · 2024-06-12T01:39:00Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

RaphaelDupont force-pushed the bootutil-refacto-crypto-usage branch from e8248a8 to c2a61b9 Compare March 28, 2023 06:43

RaphaelDupont marked this pull request as ready for review March 28, 2023 06:46

RaphaelDupont force-pushed the bootutil-refacto-crypto-usage branch 3 times, most recently from 18ff956 to 77271c3 Compare April 25, 2023 06:58

de-nordic added the crypto Encryption support label May 4, 2023

de-nordic assigned d3zd3z and davidvincze May 4, 2023

RaphaelDupont force-pushed the bootutil-refacto-crypto-usage branch 2 times, most recently from 966c014 to 83a3dca Compare May 11, 2023 07:02

davidvincze reviewed May 17, 2023

View reviewed changes

RaphaelDupont force-pushed the bootutil-refacto-crypto-usage branch from fbf94f2 to 09632c9 Compare May 23, 2023 11:17

RaphaelDupont requested a review from davidvincze May 30, 2023 16:50

davidvincze reviewed Jun 5, 2023

View reviewed changes

boot/bootutil/src/encrypted_priv.h Show resolved Hide resolved

boot/bootutil/src/encrypt_kw.c Outdated Show resolved Hide resolved

RaphaelDupont force-pushed the bootutil-refacto-crypto-usage branch from 1431ed7 to b64726d Compare June 5, 2023 07:46

davidvincze reviewed Jun 5, 2023

View reviewed changes

samples/mcuboot_config/mcuboot_config.template.h Outdated Show resolved Hide resolved

RaphaelDupont force-pushed the bootutil-refacto-crypto-usage branch 2 times, most recently from 43e1315 to 3ec049c Compare June 5, 2023 10:01

davidvincze approved these changes Jun 6, 2023

View reviewed changes

RaphaelDupont force-pushed the bootutil-refacto-crypto-usage branch 2 times, most recently from 2ef7d40 to d9f6c93 Compare June 12, 2023 14:33

de-nordic self-requested a review June 15, 2023 15:44

de-nordic requested changes Jun 15, 2023

View reviewed changes

RaphaelDupont force-pushed the bootutil-refacto-crypto-usage branch 2 times, most recently from 573b6ce to ae793d4 Compare June 15, 2023 16:12

RaphaelDupont force-pushed the bootutil-refacto-crypto-usage branch from ae793d4 to 22f5fc7 Compare June 15, 2023 16:15

RaphaelDupont requested a review from de-nordic June 20, 2023 08:53

RaphaelDupont force-pushed the bootutil-refacto-crypto-usage branch 2 times, most recently from 9a3b8e6 to 3e4179a Compare June 28, 2023 08:57

RaphaelDupont force-pushed the bootutil-refacto-crypto-usage branch from 3e4179a to 60ac9d2 Compare July 3, 2023 09:27

RaphaelDupont force-pushed the bootutil-refacto-crypto-usage branch from 60ac9d2 to b41822a Compare July 24, 2023 14:25

de-nordic approved these changes Aug 3, 2023

View reviewed changes

RaphaelDupont added 3 commits August 3, 2023 14:18

samples: Add a section on encryption in mcuboot_config.template.h

dc19ab0

Signed-off-by: Raphael Dupont <raphael7dup@gmail.com>

docs: Add note on encrypted.c refactoring

996b66e

Signed-off-by: Raphael Dupont <raphael7dup@gmail.com>

RaphaelDupont force-pushed the bootutil-refacto-crypto-usage branch from b41822a to 996b66e Compare August 3, 2023 13:11

d3zd3z self-requested a review December 14, 2023 15:18

github-actions bot added the stale label Jun 12, 2024

github-actions bot closed this Jun 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootutil: Refactor encrypted.c to have the same logic as image_validate #1658

bootutil: Refactor encrypted.c to have the same logic as image_validate #1658

RaphaelDupont commented Mar 22, 2023

davidvincze left a comment •

edited

Loading

davidvincze left a comment •

edited

Loading

davidvincze left a comment

davidvincze commented Jun 6, 2023

de-nordic commented Jun 6, 2023

davidvincze commented Jun 9, 2023

davidvincze commented Jun 9, 2023

RaphaelDupont commented Jun 13, 2023

de-nordic left a comment

RaphaelDupont commented Jun 16, 2023

RaphaelDupont commented Jul 28, 2023

de-nordic commented Aug 1, 2023

de-nordic Jun 20, 2023

RaphaelDupont Aug 3, 2023

de-nordic Aug 3, 2023

RaphaelDupont Aug 3, 2023

github-actions bot commented Jun 12, 2024

bootutil: Refactor encrypted.c to have the same logic as image_validate #1658

bootutil: Refactor encrypted.c to have the same logic as image_validate #1658

Conversation

RaphaelDupont commented Mar 22, 2023

davidvincze left a comment • edited Loading

Choose a reason for hiding this comment

davidvincze left a comment • edited Loading

Choose a reason for hiding this comment

davidvincze left a comment

Choose a reason for hiding this comment

davidvincze commented Jun 6, 2023

de-nordic commented Jun 6, 2023

davidvincze commented Jun 9, 2023

davidvincze commented Jun 9, 2023

RaphaelDupont commented Jun 13, 2023

de-nordic left a comment

Choose a reason for hiding this comment

RaphaelDupont commented Jun 16, 2023

RaphaelDupont commented Jul 28, 2023

de-nordic commented Aug 1, 2023

de-nordic Jun 20, 2023

Choose a reason for hiding this comment

RaphaelDupont Aug 3, 2023

Choose a reason for hiding this comment

de-nordic Aug 3, 2023

Choose a reason for hiding this comment

RaphaelDupont Aug 3, 2023

Choose a reason for hiding this comment

github-actions bot commented Jun 12, 2024

davidvincze left a comment •

edited

Loading

davidvincze left a comment •

edited

Loading