Skip to content
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 noup] Migrate Operational Keys from mbedTLS to PSA ITS #374

Merged
merged 2 commits into from
Jan 25, 2024
Merged

[nrf noup] Migrate Operational Keys from mbedTLS to PSA ITS #374

merged 2 commits into from
Jan 25, 2024

Conversation

ArekBalysNordic
Copy link
Contributor

@ArekBalysNordic ArekBalysNordic commented Jan 4, 2024 •
edited
Loading

  • Extended the OperationalKeystore API by a mechanism for migration of operational keys stored in one Operational Keystore implementation to another.
  • Extended the OperationalKeystore API by exporting keypair for Fabric.
  • Added Unit tests to PersistentStorageOpKeyStore and PSAOpKeystore regarding the new OperationalKeystore for migration and exporting OpKeys.
  • Added unit tests for the created export and migrate methods.
  • Emitting a new ChipEvent if the key has been migrated wrongly.

@NordicBuilder NordicBuilder mentioned this pull request Jan 4, 2024
Merged
LuDuda
LuDuda approved these changes Jan 5, 2024
View reviewed changes
Copy link
Collaborator

@LuDuda LuDuda left a comment

Choose a reason for hiding this comment

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

What about to contributing this to upstream as well, to get more reviews?

src/crypto/PSAOperationalKeystore.cpp Outdated
VerifyOrExit(status == PSA_SUCCESS, error = CHIP_ERROR_INTERNAL);

bbuf.Put(input.ConstBytes(), mPublicKey.Length());
VerifyOrExit(bbuf.Fit(), error = CHIP_ERROR_NO_MEMORY);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens with key that is imported at line 166 if this error occurs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it should destroy a private key if the error occurs because we need to have a valid keypair. I will add it.

@ArekBalysNordic
Copy link
Contributor Author

What about to contributing this to upstream as well, to get more reviews?

Sure, why not, we can start pushing PSA changes into upstream. :)

@ArekBalysNordic ArekBalysNordic force-pushed the crypto_migration branch 7 times, most recently from b6394cc to 236acea Compare January 9, 2024 15:32
Damian-Nordic
Damian-Nordic approved these changes Jan 9, 2024
View reviewed changes
src/crypto/PSAOperationalKeystore.cpp Outdated
ReturnErrorOnFailure(reader.ExitContainer(containerType));
ReturnErrorOnFailure(reader.VerifyEndOfContainer());

if (keyData.size() == kP256_PrivateKey_Length + kP256_PublicKey_Length)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would error out if that happens

@ArekBalysNordic ArekBalysNordic force-pushed the crypto_migration branch 2 times, most recently from 93d9f1d to 35f1c18 Compare January 15, 2024 11:04
@ArekBalysNordic
Copy link
Contributor Author

I've changed the way to migrate Operational Keys. Now the keys are migrated in the Server::Init method, just after getting info about FabricTable. If the key cannot be migrated, then the new ChipEvent is posted and the Server::Init method doesn't fail. I've added two methods to OperationalKeystore API to migrate and export keys, and implemented both virtual methods for PersistentOperationalKeystore and PSAOperationalKeystore. And also I've written tests for migration and exporting.

Please review those changes before I will go upstream with them.

@ArekBalysNordic ArekBalysNordic force-pushed the crypto_migration branch 3 times, most recently from 92cc164 to 8e039d9 Compare January 15, 2024 14:11
Damian-Nordic
Damian-Nordic reviewed Jan 16, 2024
View reviewed changes
Copy link
Contributor

@Damian-Nordic Damian-Nordic left a comment

Choose a reason for hiding this comment

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

Good work with tests. Found some inconsistencies that you can look into.

src/app/server/Server.cpp Outdated Show resolved Hide resolved
src/include/platform/CHIPDeviceEvent.h Outdated Show resolved Hide resolved
src/crypto/OperationalKeystore.h Outdated Show resolved Hide resolved
src/crypto/PSAOperationalKeystore.cpp Show resolved Hide resolved
src/crypto/PSAOperationalKeystore.cpp Outdated Show resolved Hide resolved
src/crypto/PersistentStorageOperationalKeystore.cpp Outdated
// Load up the operational key structure from storage
uint16_t size = static_cast<uint16_t>(buf.Capacity());
CHIP_ERROR err = storage->SyncGetKeyValue(DefaultStorageKeyAllocator::FabricOpKey(fabricIndex).KeyName(), buf.Bytes(), size);
if (err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the PSA migration function you seem to assume that for already migrated fabric CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND will be returned, but due to this code this will likely be CHIP_ERROR_INVALID_FABRIC_INDEX, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to return the CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND error for the Export method and keep CHIP_ERROR_INVALID_FABRIC_INDEX for the Signing method

src/crypto/PersistentStorageOperationalKeystore.cpp Outdated Show resolved Hide resolved
src/crypto/tests/TestPSAOpKeyStore.cpp Outdated Show resolved Hide resolved
src/crypto/tests/TestPSAOpKeyStore.cpp Outdated Show resolved Hide resolved
src/crypto/tests/TestPersistentStorageOpKeyStore.cpp Show resolved Hide resolved
Damian-Nordic
Damian-Nordic approved these changes Jan 16, 2024
View reviewed changes
src/crypto/PSAOperationalKeystore.cpp Outdated Show resolved Hide resolved
@ArekBalysNordic ArekBalysNordic force-pushed the crypto_migration branch 2 times, most recently from ffbfd48 to 8886e9d Compare January 16, 2024 14:32
@ArekBalysNordic ArekBalysNordic force-pushed the crypto_migration branch 3 times, most recently from 063d14c to 78bc9bd Compare January 22, 2024 15:05
@ArekBalysNordic
[nrf fromlist][crypto] Extend the OperationalKeystore API to allow mi…
e886ae8 
…gration

- Extended the OperationalKeystore API by mechanism for migration of
operational keys stored in one Operational Keystore implementation
to another.

- Extended the OperationalKeystore API by exporting keypair for Fabric.

- Added Unit tests to PersistentStorageOpKeyStore and PSAOpKeystore regarding
the new OperationalKeystore for migration and exporting OpKeys.

Added first unit tests, created export method

Aligned NXP and Silabs platform to the recent OperationalKeystore changes.
@ArekBalysNordic
[nrf noup] Move initializing PSA to Application
b3aa01d 
Initialize PSA in the Application side instead of Matter Server.
@ArekBalysNordic ArekBalysNordic merged commit 288b64a into nrfconnect:master Jan 25, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Damian-Nordic Damian-Nordic Damian-Nordic approved these changes

@kkasperczyk-no kkasperczyk-no Awaiting requested review from kkasperczyk-no

@markaj-nordic markaj-nordic Awaiting requested review from markaj-nordic

@LuDuda LuDuda Awaiting requested review from LuDuda

Assignees
No one assigned
Labels
app crypto
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ArekBalysNordic @LuDuda @Damian-Nordic