scripts: hid_configurator: Support pure ED25519 signature #19238

MarekPieta · 2024-12-04T08:18:47Z

Change adds support for pure ED25519 signature (used by nRF54L based devices that enable MCUboot hardware cryptography). imgtool from MCUboot upstream repository does not support this configuration, a dedicated imgtool version from sdk-mcuboot repository must be used.

Jira: NCSDK-30745

NordicBuilder · 2024-12-04T08:23:05Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 7

Inputs:

Sources:

sdk-nrf: PR head: 275c003822ce9d5d9a9930f3cef0807f41a1f26b

more details

sdk-nrf:

PR head: 275c003822ce9d5d9a9930f3cef0807f41a1f26b
merge base: cf777d12ccf26adb208b7d63fa157b7bb243d1dd
target head (main): 584e1c85cd2f058ee69b802bfd913b823e315b2c
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (2)

scripts
│  ├── hid_configurator
│  │  ├── README.rst
│  │  ├── modules
│  │  │  │ dfu.py

Outputs:

Toolchain

Version: b77d8c1312
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:b77d8c1312_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 87
✅ Integration tests
- ✅ desktop52_verification

Disabled integration tests

- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-boot
- test-fw-nrfconnect-chip
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf-iot_lwm2m
- test-fw-nrfconnect-nrf-iot_mosh
- test-fw-nrfconnect-nrf-iot_nrf_provisioning
- test-fw-nrfconnect-nrf-iot_positioning
- test-fw-nrfconnect-nrf-iot_samples
- test-fw-nrfconnect-nrf-iot_serial_lte_modem
- test-fw-nrfconnect-nrf-iot_thingy91
- test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread
- test-fw-nrfconnect-zigbee
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-find-my
- test-sdk-mcuboot
- test-sdk-pmic-samples
- test-sdk-sidewalk
- test-sdk-wifi
- test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

NordicBuilder · 2024-12-04T08:36:27Z

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.

nordicjm · 2024-12-04T10:04:10Z

scripts/hid_configurator/README.rst

+   .. parsed-literal::
+      :class: highlight
+
+      py -3 -m pip install -r requirements.txt
+      py -3 -m pip install .


not that I know about python but why can't it do as the build system does and use the in-tree version until a proper updated version is released? See https://github.com/nrfconnect/sdk-nrf/blob/main/cmake/sysbuild/image_signing.cmake#L52

This step actually installs the in-tree version of imgtool instead of version from PyPI. Python script by itself has no knowledge where sdk-mcuboot repository sources are on host computer - that's why I decided to install proper imgtool through pip. HID configurator scripts could be moved, so using a relative path might be problematic too.

The user should already have the correct version if they follow the NCS installation guide correctly. We could minimize this note and notify the user to double-check if they have the correct version of imgtool.

~~We can also add a reference to the NCS install documentation that covers how to install this tool:~~

https://docs.nordicsemi.com/bundle/ncs-latest/page/nrf/installation/install_ncs.html#system-wide_installation
(Step 4)

Which people should not be doing, the toolchain comes with a specific version of the tool, sure it's not the version that includes this but this will then pollute the version used by the toolchain going forward? Have asked some others if there is a better way of doing this

The nRF54L MCuboot's pure ED25519 signature requires custom imgtool version (not from PyPI; needs to be installed from source using sdk-mcuboot repo). Unless I missed something, the custom imgtool version is not installed together with our official toolchain. I am not sure if there is better way of using the custom imgtool in this context than manually installing from source (as currently suggested). Please propose alternative solutions if anything better comes to your mind.

I hope that this is only a temporary workaround and could be removed when mcu-tools/mcuboot#2063 is introduced to imgtool from PyPI. @de-nordic and @nvlsianpu, do you think there is a better way of doing it for now?

See how it is done e.g.

doc/internal/conf.py:sys.path.insert(0, str(NRF_BASE / "doc" / "_utils")) doc/internal/conf.py:sys.path.insert(0, str(ZEPHYR_BASE / "doc" / "_extensions")) doc/internal/conf.py:sys.path.insert(0, str(NRF_BASE / "doc" / "_extensions")) doc/kconfig/conf.py:sys.path.insert(0, str(NRF_BASE / "doc" / "_utils")) doc/kconfig/conf.py:sys.path.insert(0, str(ZEPHYR_BASE / "doc" / "_extensions"))

Then we need to know NRF_BASE/ZEPHYR_BASE in scope of the script (it might become problematic e.g. if someone would like to use script outside of the nRF Connect SDK as a standalone tool). Also, the sdk-mcuboot's imgtool's scripts would need to be located in a known place.

Please keep in mind that we use use image class exposed by the imgtool package (we do not call imgtool through CLI). Because of that, IMHO relying on packages might be cleaner approach - we build a SUIT-related package from source and install it manually already too.

Also please keep in mind that targets other than nRF54L do not use pure ED25519 signature, so they may still rely on the default imgtool version (extra step from the note could be basically skipped). Maybe we could explicitly comment on that and stick with using package built from source as a temporary workaround (we will use imgtool from PyPI as soon as it starts supporting the pure signature)?

@MarekPieta , I don't see @nordicjm blocking the change. I value input from various reviewers but this area is not his responsibility. Please keep that in mind.

I personally think that provided toolchain should cover all things needed. If it does not we should request it from team responsible for toolchain - check if we or pluto have a ticket. In the end this tool is not only our problem.

Since the timing is short I expect having the workaround is valid. It will require user interaction and is part of nrf_desktop tools doc only.

@MarekPieta, @nordicjm, command pip install . just installs Python's package from a source code, if that is what you want it is fine for me. I would only add a link to the repository where is imgtool (I don't see it in the readme file, and user may not know how to find it).

Also you can install directly from a git repository like so:
pip install git+ssh://[email protected]/myuser/foo.git@branch_or_tag#subdirectory=stackoverflow
Which could be more convenient for users.

Another thing, command like this could be more precise:

python -m pip install path/to/SomeProject

I provided source directory path starting from nRF Connect SDK root directory (should be straightforward to find it for an NCS user). I do not provide a complete path in command here as I cannot provide absolute path and relative path might change in case user moves HID configurator scripts locally. Would this approach be ok?

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst

kapi-no · 2024-12-04T10:31:02Z

scripts/hid_configurator/README.rst

+   .. parsed-literal::
+      :class: highlight
+
+      py -3 -m pip install -r requirements.txt
+      py -3 -m pip install .


The user should already have the correct version if they follow the NCS installation guide correctly. We could minimize this note and notify the user to double-check if they have the correct version of imgtool.

~~We can also add a reference to the NCS install documentation that covers how to install this tool:~~

https://docs.nordicsemi.com/bundle/ncs-latest/page/nrf/installation/install_ncs.html#system-wide_installation
(Step 4)

scripts/hid_configurator/modules/dfu.py

pdunaj · 2024-12-04T14:12:40Z

scripts/hid_configurator/README.rst

+   .. parsed-literal::
+      :class: highlight
+
+      py -3 -m pip install -r requirements.txt
+      py -3 -m pip install .


@MarekPieta , I don't see @nordicjm blocking the change. I value input from various reviewers but this area is not his responsibility. Please keep that in mind.

I personally think that provided toolchain should cover all things needed. If it does not we should request it from team responsible for toolchain - check if we or pluto have a ticket. In the end this tool is not only our problem.

Since the timing is short I expect having the workaround is valid. It will require user interaction and is part of nrf_desktop tools doc only.

scripts/hid_configurator/README.rst

Change adds support for pure ED25519 signature (used by nRF54L-based devices that enable MCUboot hardware cryptography). imgtool from MCUboot upstream repository does not support this configuration, a dedicated imgtool version from sdk-mcuboot repository must be used. Jira: NCSDK-30745 Signed-off-by: Marek Pieta <[email protected]> Signed-off-by: Pekka Niskanen <[email protected]> Signed-off-by: Divya Pillai <[email protected]>

stale

MarekPieta added this to the 2.9.0 milestone Dec 4, 2024

MarekPieta requested review from a team as code owners December 4, 2024 08:18

github-actions bot added the doc-required PR must not be merged without tech writer approval. label Dec 4, 2024

nordicjm reviewed Dec 4, 2024

View reviewed changes

MarekPieta force-pushed the hid_configurator_nrf54l_fix branch from cba5d47 to f8b2a38 Compare December 4, 2024 10:07

kapi-no reviewed Dec 4, 2024

View reviewed changes

MarekPieta requested a review from fundakol December 4, 2024 12:27

pdunaj approved these changes Dec 4, 2024

View reviewed changes

kapi-no approved these changes Dec 4, 2024

View reviewed changes

MarekPieta mentioned this pull request Dec 4, 2024

doc: Add release notes for the 2.9.0 release #19240

Open

5 tasks

MarekPieta force-pushed the hid_configurator_nrf54l_fix branch from f8b2a38 to c7f1b9a Compare December 4, 2024 14:54

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 4, 2024

MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 4, 2024

MarekPieta requested review from peknis, divipillai and nordicjm December 4, 2024 14:56

peknis previously requested changes Dec 4, 2024

View reviewed changes

scripts/hid_configurator/README.rst Outdated Show resolved Hide resolved

scripts/hid_configurator/README.rst Outdated Show resolved Hide resolved

scripts/hid_configurator/README.rst Outdated Show resolved Hide resolved

scripts/hid_configurator/README.rst Outdated Show resolved Hide resolved

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 4, 2024

MarekPieta force-pushed the hid_configurator_nrf54l_fix branch from 1c1876a to 5cc3f5c Compare December 4, 2024 15:29

MarekPieta requested a review from peknis December 4, 2024 15:29

MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 4, 2024

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 4, 2024

MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 4, 2024

divipillai approved these changes Dec 4, 2024

View reviewed changes

scripts/hid_configurator/README.rst Outdated Show resolved Hide resolved

scripts/hid_configurator/README.rst Outdated Show resolved Hide resolved

scripts/hid_configurator/README.rst Outdated Show resolved Hide resolved

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 4, 2024

MarekPieta force-pushed the hid_configurator_nrf54l_fix branch from 979b951 to 275c003 Compare December 4, 2024 15:55

carlescufi merged commit 6f444f7 into nrfconnect:main Dec 4, 2024
13 checks passed

MarekPieta deleted the hid_configurator_nrf54l_fix branch December 4, 2024 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripts: hid_configurator: Support pure ED25519 signature #19238

scripts: hid_configurator: Support pure ED25519 signature #19238

MarekPieta commented Dec 4, 2024

NordicBuilder commented Dec 4, 2024 •

edited

Loading

NordicBuilder commented Dec 4, 2024

nordicjm Dec 4, 2024

MarekPieta Dec 4, 2024 •

edited

Loading

kapi-no Dec 4, 2024 •

edited

Loading

nordicjm Dec 4, 2024

MarekPieta Dec 4, 2024

nordicjm Dec 4, 2024

MarekPieta Dec 4, 2024

pdunaj Dec 4, 2024

fundakol Dec 4, 2024 •

edited

Loading

MarekPieta Dec 4, 2024

kapi-no Dec 4, 2024 •

edited

Loading

pdunaj Dec 4, 2024

scripts: hid_configurator: Support pure ED25519 signature #19238

scripts: hid_configurator: Support pure ED25519 signature #19238

Conversation

MarekPieta commented Dec 4, 2024

NordicBuilder commented Dec 4, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

NordicBuilder commented Dec 4, 2024

Choose a reason for hiding this comment

MarekPieta Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

kapi-no Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fundakol Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kapi-no Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NordicBuilder commented Dec 4, 2024 •

edited

Loading

MarekPieta Dec 4, 2024 •

edited

Loading

kapi-no Dec 4, 2024 •

edited

Loading

fundakol Dec 4, 2024 •

edited

Loading

kapi-no Dec 4, 2024 •

edited

Loading