Skip to content

Conversation

tomi-font
Copy link
Contributor

@tomi-font tomi-font commented May 23, 2025

See commits.

test_crypto: PR-808

@tomi-font tomi-font requested review from a team as code owners May 23, 2025 13:33
@NordicBuilder
Copy link
Contributor

NordicBuilder commented May 23, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
zephyr nrfconnect/sdk-zephyr@e653784 nrfconnect/sdk-zephyr@247ef30 (main) nrfconnect/[email protected]

All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@NordicBuilder
Copy link
Contributor

NordicBuilder commented May 23, 2025

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 10

Inputs:

Sources:

sdk-nrf: PR head: 886dab1a282a2253a9b4df379b27f185f59d830d
zephyr: PR head: 247ef302c2269d63b8957e5a346ab42a5f0b3bc8

more details

sdk-nrf:

PR head: 886dab1a282a2253a9b4df379b27f185f59d830d
merge base: 3502a8c820dd0ae29b8396ee1fe87c6386b49ac8
target head (main): 3502a8c820dd0ae29b8396ee1fe87c6386b49ac8
Diff

zephyr:

PR head: 247ef302c2269d63b8957e5a346ab42a5f0b3bc8
merge base: e6537849e886ca40b68269e0c5d39be66f09479d
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 (41)
CODEOWNERS
samples
│  ├── crypto
│  │  ├── persistent_key_usage
│  │  │  ├── CMakeLists.txt
│  │  │  ├── README.rst
│  │  │  ├── boards
│  │  │  │  ├── nrf52840dk_nrf52840.conf
│  │  │  │  ├── nrf5340dk_nrf5340_cpuapp.conf
│  │  │  │  ├── nrf54l15dk_nrf54l05_cpuapp.conf
│  │  │  │  ├── nrf54l15dk_nrf54l10_cpuapp.conf
│  │  │  │  ├── nrf54l15dk_nrf54l15_cpuapp.conf
│  │  │  │  ├── nrf54lm20pdk_nrf54lm20a_cpuapp.conf
│  │  │  │  ├── nrf54lv10dk_nrf5454lv10a_cpuapp.conf
│  │  │  │  ├── nrf9151dk_nrf9151.conf
│  │  │  │  ├── nrf9160dk_nrf9160.conf
│  │  │  │  │ nrf9161dk_nrf9161.conf
│  │  │  ├── sample.yaml
│  │  │  ├── src
│  │  │  │  ├── init.c
│  │  │  │  ├── init.h
│  │  │  │  ├── main.c
│  │  │  │  │ trusted_storage_init.h
scripts
│  ├── ci
│  │  │ tags.yaml
subsys
│  ├── Kconfig
│  ├── secure_storage
│  │  ├── CMakeLists.txt
│  │  ├── Kconfig
│  │  ├── compatibility
│  │  │  ├── Kconfig
│  │  │  ├── src
│  │  │  │  ├── its_store_settings_get.c
│  │  │  │  │ its_transform_tsbc.c
│  │  ├── src
│  │  │  │ its_transform_aead_get_key_huk.c
│  ├── trusted_storage
│  │  ├── Kconfig
│  │  ├── src
│  │  │  ├── aead
│  │  │  │  │ aead_crypt_psa_chachapoly.c
tests
│  ├── zephyr
│  │  ├── subsys
│  │  │  ├── secure_storage
│  │  │  │  ├── psa
│  │  │  │  │  ├── its
│  │  │  │  │  │  ├── CMakeLists.txt
│  │  │  │  │  │  ├── prj.conf
│  │  │  │  │  │  │ testcase.yaml
west.yml
zephyr
│  ├── subsys
│  │  ├── secure_storage
│  │  │  ├── CMakeLists.txt
│  │  │  ├── Kconfig.its_transform
│  │  │  ├── include
│  │  │  │  ├── internal
│  │  │  │  │  ├── zephyr
│  │  │  │  │  │  ├── secure_storage
│  │  │  │  │  │  │  ├── its
│  │  │  │  │  │  │  │  ├── common.h
│  │  │  │  │  │  │  │  │ transform.h
│  │  │  ├── src
│  │  │  │  ├── its
│  │  │  │  │  ├── store
│  │  │  │  │  │  ├── settings.c
│  │  │  │  │  │  │ zms.c
│  │  │  │  │  ├── transform
│  │  │  │  │  │  ├── aead.c
│  │  │  │  │  │  │ aead_get.c
│  ├── tests
│  │  ├── subsys
│  │  │  ├── secure_storage
│  │  │  │  ├── psa
│  │  │  │  │  ├── its
│  │  │  │  │  │  ├── src
│  │  │  │  │  │  │  │ custom_store.c

Outputs:

Toolchain

Version: 4aa3467a6d
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:4aa3467a6d_e85602c25f

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

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 4
    • sdk-zephyr test count: 621
  • ✅ Integration tests
    • ✅ test-fw-nrfconnect-nrf_crypto
Disabled integration tests
    • desktop52_verification
    • 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_cloud
    • test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • 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-proprietary_esb
    • test-fw-nrfconnect-ps-main
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-tfm
    • test-fw-nrfconnect-thread-main
    • test-low-level
    • test-sdk-audio
    • test-sdk-find-my
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-wifi
    • test-secdom-samples-public

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

Copy link

github-actions bot commented May 23, 2025

You can find the documentation preview for this PR here.

Preview links for modified nRF Connect SDK documents:

https://ncsdoc.z6.web.core.windows.net/PR-22491/nrf/samples/crypto/persistent_key_usage/README.html

@tomi-font tomi-font force-pushed the secure_storage_backward_compatibility branch 3 times, most recently from 671cf05 to 6f197c5 Compare May 28, 2025 09:15
@tomi-font tomi-font requested review from a team, PerMac, katgiadla and nordic-piks as code owners May 28, 2025 09:15
Copy link
Contributor

@PerMac PerMac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test automation part LGTM

@tomi-font tomi-font force-pushed the secure_storage_backward_compatibility branch from 6f197c5 to da6d2ff Compare May 28, 2025 10:49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should only be called (zephyr_library()) if it is used, so CONFIG_SECURE_STORAGE_TRUSTED_STORAGE_COMPATIBILITY is not set then this will generate an empty library which will throw a warning, library should be in the subsys/secure_storage/compatibility/CMakeLists.txt file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops. Addressed differently than you suggested as the conditions for there being source files aren't that straightforward. Unfortunately I couldn't seem to be able to fill the list in the child CMakeLists.txt and use it in the parent one (the list would be empty). Let me know if you have a better suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

help always goes last

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

experimental needs [EXPERIMENTAL] at the end of the choice text

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep selects together

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default x if SECURE_STORAGE_TRUSTED_STORAGE_COMPATIBILITY ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What for? SECURE_STORAGE_ITS_TRANSFORM_IMPLEMENTATION_TSBC already has depends on SECURE_STORAGE_TRUSTED_STORAGE_COMPATIBILITY, which is required so that SECURE_STORAGE_ITS_TRANSFORM_IMPLEMENTATION_TSBC cannot be chosen if SECURE_STORAGE_TRUSTED_STORAGE_COMPATIBILITY is not y (enclosing if is not enough, I did some experiments).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it makes it far easier to read and spot errors and is just good practice. Imagine if these 2 symbols were in different files for example

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prompt is always at top

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point

@tomi-font tomi-font force-pushed the secure_storage_backward_compatibility branch from da6d2ff to 8b18c51 Compare May 30, 2025 13:27
@tomi-font
Copy link
Contributor Author

Rebased and addressed @nordicjm's comments.

@tomi-font tomi-font requested a review from nordicjm May 30, 2025 13:29
@tomi-font tomi-font force-pushed the secure_storage_backward_compatibility branch from 8b18c51 to 6e32278 Compare June 2, 2025 13:45
@tomi-font tomi-font requested a review from a team as a code owner June 2, 2025 13:45
@github-actions github-actions bot added the doc-required PR must not be merged without tech writer approval. label Jun 2, 2025
@tomi-font
Copy link
Contributor Author

Fixed an issue with one addition in subsys/secure_storage/CMakeLists.txt, added changes to samples/crypto/persistent_key_usage, and rebased.

@tomi-font tomi-font requested a review from magnev June 2, 2025 13:47
@tomi-font
Copy link
Contributor Author

@nordicjm @ncs-aegir please review

Comment on lines +13 to +17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than this, config SECURE_STORAGE_ITS_TRANSFORM_IMPLEMENTATION_AEAD should just have depends on !SECURE_STORAGE_TRUSTED_STORAGE_COMPATIBILITY

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love to have a Kconfig-based solution but I went for this because I couldn't find one that worked. What you suggest would require a noup which I would rather not have.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it makes it far easier to read and spot errors and is just good practice. Imagine if these 2 symbols were in different files for example

Comment on lines 12 to 17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The ITS backend is either provided by TF-M or either the :ref:`secure_storage` subsystem or the :ref:`trusted_storage_readme` library when building applications without TF-M.
A persistent key becomes unusable when the ``psa_destroy_key`` function is called.
The ITS backend is provided in one of the following ways, depending on your configuration:
* Through TF-M using Internal Trusted Storage and Protected Storage services
* When building without TF-M: using either Zephyr's :ref:`secure_storage` subsystem or the :ref:`trusted_storage_readme` library
A persistent key becomes unusable when the ``psa_destroy_key`` function is called.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# When TF-M is not in use, the secure storage subsystem provides the PSA Secure Storage API.
# When TF-M is not in use, the Secure storage subsystem provides the PSA Secure Storage API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# When TF-M is not in use, the secure storage subsystem provides the PSA Secure Storage API.
# When TF-M is not in use, the Secure storage subsystem provides the PSA Secure Storage API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# When TF-M is not in use, the secure storage subsystem provides the PSA Secure Storage API.
# When TF-M is not in use, the Secure storage subsystem provides the PSA Secure Storage API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# When TF-M is not in use, the secure storage subsystem provides the PSA Secure Storage API.
# When TF-M is not in use, the Secure storage subsystem provides the PSA Secure Storage API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for "example" or "sample" in name. Most samples don't have this.

Suggested change
name: Persistent key usage example
name: Persistent key usage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. Forgot about that. Got distracted by how it was.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- ".*Example finished successfully!.*"
- ".*Sample finished successfully!*"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something that would need to be updated in all the crypto samples. Not changing this here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool "HUK library-derived keys"
bool "Keys derived from HUK library"

Or using HUK library?

Comment on lines 15 to 18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Enable this to make the secure storage subsystem compatible with
an existing installation that was previously using the trusted storage library.
It will allow the secure storage subsystem to operate and store entries
like the trusted storage library would.
Enable to make the Secure storage subsystem compatible with
an existing installation that was previously using the Trusted storage library.
This allows the Secure storage subsystem to operate and store entries
like the Trusted storage library would.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool "ITS transform module implementation compatible with trusted storage"
bool "ITS transform module implementation compatible with trusted storage"

Do you mean ITS with "trusted storage" or the trusted storage library?
Maybe:

Suggested change
bool "ITS transform module implementation compatible with trusted storage"
bool "Implementation of the ITS transform module, compatible with the Trusted storage library"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented this change except I had to make it shorter for it to fit within 100 (or so) characters.

@tomi-font tomi-font force-pushed the secure_storage_backward_compatibility branch 2 times, most recently from 164675e to c35551a Compare June 3, 2025 12:46
@tomi-font tomi-font requested a review from greg-fer June 3, 2025 12:46
Copy link
Contributor

@greg-fer greg-fer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but please address comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, missed this in the first round :/
NCS is a forbidden term and should not be used.

Suggested change
# Secure storage subsystem integration into NCS
# Secure storage subsystem integration into the nRF Connect SDK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this about Oberon?

Suggested change
* which holds all the active keys in the PSA Crypto core.
* which holds all the active keys in the Oberon PSA Core.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

@tomi-font tomi-font force-pushed the secure_storage_backward_compatibility branch from c35551a to 03e902a Compare June 4, 2025 09:28
tomi-font added 8 commits June 4, 2025 14:15
The ChaCha20-Poly1305 alg is needed, but the ChaCha20 key type also.

Signed-off-by: Tomi Fontanilles <[email protected]>
They cause issues when compiling on native_sim.

Signed-off-by: Tomi Fontanilles <[email protected]>
Previously was only on 54L15.

Signed-off-by: Tomi Fontanilles <[email protected]>
For downstream integration and expansion.

Signed-off-by: Tomi Fontanilles <[email protected]>
Allow the secure storage subsystem to be compatible with the trusted
storage library.
This is controlled by the top-level Kconfig option
CONFIG_SECURE_STORAGE_TRUSTED_STORAGE_COMPATIBILITY.

Signed-off-by: Tomi Fontanilles <[email protected]>
As trusted storage's TRUSTED_STORAGE_BACKEND_AEAD_KEY_DERIVE_FROM_HUK,
provide the possibility to derive keys using the HUK library through
CONFIG_SECURE_STORAGE_ITS_TRANSFORM_AEAD_KEY_PROVIDER_HUK_LIBRARY.

Signed-off-by: Tomi Fontanilles <[email protected]>
Extend the test scenarios found under
tests/subsys/secure_storage/psa/its with configurations tailored to NCS.
CONFIG_NRF_SECURITY and other relevant Kconfig options are enabled.

The primary intent is to test the backward-compatible implementation,
so all test scenarios except the ZMS one use it.

Signed-off-by: Tomi Fontanilles <[email protected]>
Make secure storage the default option on non-TF-M board targets.
Have test scenarios for both secure storage and trusted storage to
test both.
Reduce a bit the number of board targets in integration_platforms to
reduce CI load as some don't bring extra value when others are already
in there.

Signed-off-by: Tomi Fontanilles <[email protected]>
@tomi-font tomi-font force-pushed the secure_storage_backward_compatibility branch from 03e902a to 886dab1 Compare June 4, 2025 11:15
@NordicBuilder NordicBuilder removed the DNM label Jun 4, 2025
@rlubos rlubos merged commit 900df03 into nrfconnect:main Jun 4, 2025
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-required PR must not be merged without tech writer approval. manifest manifest-zephyr

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants