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.

Sign up for GitHub

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

Jump to bottom

nrf_security: Add key revocation for SICR keys #19710

Open

Vge0rge wants to merge 4 commits into nrfconnect:main from Vge0rge:platform_key_revocation

+88 −30

Contributor

Vge0rge commented Dec 26, 2024

Adds key revocation for the platform keys stored in SICR for nRF54H20.

github-actions bot added the changelog-entry-required label

Contributor

NordicBuilder commented Dec 26, 2024 •

edited

Loading

CI Information

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

Inputs:

Sources:

sdk-nrf: PR head: e4eb37445c63cf4bbf3785eff13179fc6b7c8a42

more details

sdk-nrf:

PR head: e4eb37445c63cf4bbf3785eff13179fc6b7c8a42
merge base: cac49ac985664e9357e1c30691ce1091470c528b
target head (main): d899e22404f40e028cc7c1cbf71930e06e321cd3
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 (3)

subsys
│  ├── nrf_security
│  │  ├── src
│  │  │  ├── drivers
│  │  │  │  ├── cracen
│  │  │  │  │  ├── cracenpsa
│  │  │  │  │  │  ├── src
│  │  │  │  │  │  │  ├── key_management.c
│  │  │  │  │  │  │  ├── platform_keys
│  │  │  │  │  │  │  │  ├── platform_keys.c
│  │  │  │  │  │  │  │  │ platform_keys.h

Outputs:

Toolchain

Version: 342151af73
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:342151af73_912848a074

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

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 1629
❌ Integration tests
- ✅ test-fw-nrfconnect-chip
- ❌ test-fw-nrfconnect-nrf_crypto
- ✅ test-fw-nrfconnect-tfm
- ✅ test-sdk-find-my
- ✅ test-sdk-sidewalk
- ✅ test-sdk-dfu
- ⚠️ test-fw-nrfconnect-nrf-iot_cloud

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-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-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-thread
- test-fw-nrfconnect-zigbee
- test-low-level
- test-sdk-audio
- 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

Vge0rge force-pushed the platform_key_revocation branch from 4fdb848 to caca435 Compare

December 30, 2024 15:32

Vge0rge marked this pull request as ready for review

December 30, 2024 15:32

Vge0rge requested a review from a team as a code owner

December 30, 2024 15:32

tomi-font reviewed

View reviewed changes

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/platform_keys/platform_keys.c Outdated Show resolved Hide resolved

Vge0rge force-pushed the platform_key_revocation branch from caca435 to 4ed1318 Compare

January 2, 2025 10:56


          nrf_security: Refactor writing of SICR keys

1d269a6

This refactors how SICR keys are writen into MRAM. This refactors the
code but it should not change any funcionality. The purpose of this
is to make the MRAM writing part reusable so it can be used by the
revocation functionality later.

Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>

Vge0rge force-pushed the platform_key_revocation branch from 4ed1318 to 8008df6 Compare

January 6, 2025 15:33

maciejpietras requested review from tomchy and SylwesterKonczyk

January 7, 2025 15:13

Vge0rge force-pushed the platform_key_revocation branch from 8008df6 to 14c4ee8 Compare

January 8, 2025 11:53


          nrf_security: Add platform key revocation support for SICR keys

dcd079a

Adds support of key revocation using the psa_destroy_key API.
The value 0xfa50 is used in the key type in order to mark an
revoked key.

The return code PSA_ERROR_NOT_PERMITTED is returned for
revoked keys for all the functions in the PSA crypto driver
wrapper. This error code seems OK since it mentions
platform specific policies for not permitted an operation.

Ref: NCSDK-30076

Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>

Vge0rge force-pushed the platform_key_revocation branch from 14c4ee8 to dcd079a Compare

January 14, 2025 12:33

tomi-font reviewed

View reviewed changes

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/platform_keys/platform_keys.c Show resolved Hide resolved

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/platform_keys/platform_keys.c

Comment on lines +569 to 570

		if (type == INVALID \|\| type == REVOKED) {
		return 0;

Contributor

tomi-font Jan 16, 2025

Just wondering, wouldn't we want to return a more clear error than a 0 size?

Contributor Author

Vge0rge Jan 16, 2025

I wanted to avoid doing that in this same PR to make it more clear to follow.

But I have this PR which updates this function and returns psa_status_t:
#19731

Since likely the 19731 will probably get merged first I will just rebase the current PR. So no need to do something now here I think.

Contributor

tomi-font Jan 16, 2025

Ah right, I even approved it, but didn't quite remember it. IMO it would have been plenty fine to have a single PR as long as the commits are separate.

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/platform_keys/platform_keys.c Outdated

Comment on lines 702 to 717

-              		write_sicr_key_to_mram(key, sicr_attr, encrypted_key, key_buffer_size);
+              		write_sicr_key_to_mram(&key, sicr_attr, encrypted_key, key_buffer_size);
               	} else {
-              		write_sicr_key_to_mram(key, sicr_attr, key_buffer, key_buffer_size);
+              		write_sicr_key_to_mram(&key, sicr_attr, key_buffer, key_buffer_size);

Contributor

tomi-font Jan 16, 2025

better squash that into the first commit

Contributor Author

Vge0rge Jan 16, 2025

Good catch, thanks! It seems that I messed up my fixup commit. Will update

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/platform_keys/platform_keys.c Outdated Show resolved Hide resolved

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/platform_keys/platform_keys.c Outdated Show resolved Hide resolved

Vge0rge and others added 2 commits

January 16, 2025 10:25


          Update subsys/nrf_security/src/drivers/cracen/cracenpsa/src/platform_…

9063b3f

…keys/platform_keys.c

Co-authored-by: Tomi Fontanilles <tomi.f@tutanota.com>


          Update subsys/nrf_security/src/drivers/cracen/cracenpsa/src/platform_…

e4eb374

…keys/platform_keys.c

Co-authored-by: Tomi Fontanilles <tomi.f@tutanota.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog-entry-required