-
Notifications
You must be signed in to change notification settings - Fork 1.5k
suit: Build system changes needed for encryption #19681
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
suit: Build system changes needed for encryption #19681
Conversation
|
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: a56204b98b73062d988d17be31c8e94fd2960a78 more detailssdk-nrf:
suit-generator:
Github labels
List of changed files detected by CI (13)Outputs:ToolchainVersion: 11349092be Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
|
You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds. Note: This comment is automatically posted by the Documentation Publish GitHub Action. |
Memory footprint analysis revealed the following potential issuessample.matter.template.debug[nrf7002dk/nrf5340/cpuapp]: High ROM usage: 911938[B] - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic) Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-19681/19) |
nordicjm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apply changes throughout whole PR
cmake/sysbuild/suit.cmake
Outdated
| ----------------------------------------------------------- | ||
| --- WARNING: Using default file-based basic KMS implentation for encryption. --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the widths should match for the header and footer with the message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
cmake/sysbuild/suit.cmake
Outdated
| message(WARNING " | ||
| ----------------------------------------------------------- | ||
| --- WARNING: Using default file-based basic KMS implentation for encryption. --- | ||
| --- It should not be used for production unless the build is performed in --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typos need fixing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
sysbuild/Kconfig.suit
Outdated
| Python script called to generate encryption artifacts for images inside a SUIT envelope. | ||
| See the help message for the default encryption script to see what arguments the script | ||
| must accept. | ||
| default "${ZEPHYR_NRF_MODULE_DIR}/../modules/lib/suit-generator/ncs/encrypt_script.py" if SUIT_ENVELOPE_ENCRYPT_SCRIPT_DEFAULT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the actual module name instead of a path from the nrf module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, changed
0fcaa3b to
b40b0a3
Compare
sysbuild/Kconfig.suit
Outdated
| choice SUIT_ENVELOPE_KMS_SCRIPT | ||
| prompt "Select the used SUIT KMS script to be used by the encryption script" | ||
| help | ||
| This is the "internal" python script that is called by the encryption script to perform the cryptographic operations, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1x tab followed by 2x spaces for help text indent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fixed
sysbuild/Kconfig.suit
Outdated
|
|
||
| config SUIT_ENVELOPE_KMS_SCRIPT_PATH | ||
| string "Location of SUIT KMS script" | ||
| default "${ZEPHYR_SUIT_GENERATOR_MODULE_DIR}/ncs/basic_kms.py" if SUIT_ENVELOPE_KMS_SCRIPT_BASIC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this a kconfig?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SUIT_ENVELOPE_KMS_SCRIPT choice is supposed to be extended, adding more possible values to SUIT_ENVELOPE_KMS_SCRIPT_PATH - in a similar way like
sdk-nrf/sysbuild/Kconfig.netcore
Line 142 in cac49ac
| default "${ZEPHYR_NRF_MODULE_DIR}/samples/nrf5340/empty_net_core" if NETCORE_EMPTY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The netcore part is used to specify an image, not a file, the file part of signing is done like so: https://github.com/zephyrproject-rtos/zephyr/blob/main/CMakeLists.txt#L2027 which then uses this file by default: https://github.com/zephyrproject-rtos/zephyr/blob/main/cmake/mcuboot.cmake but in ncs is overridden here: https://github.com/nrfconnect/sdk-nrf/blob/main/sysbuild/CMakeLists.txt#L293 or https://github.com/nrfconnect/sdk-nrf/blob/main/sysbuild/CMakeLists.txt#L336
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the misunderstanding was cleared offline, I will fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fixed
e5c4dd9 to
97a7f9e
Compare
2c5fdd4 to
36e9c11
Compare
nordicjm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit. can be fixed in future
36e9c11 to
5677cb8
Compare
dad9fb9 to
011a835
Compare
011a835 to
91e4de6
Compare
This commit contains changes to the SUIT build system allowing for encrypting images for update. Signed-off-by: Artur Hadasz <[email protected]>
91e4de6 to
a56204b
Compare
This commit contains changes to the SUIT build system allowing for encrypting images for update.