DeviceKey Root of Trust generation refactored. #12385

tymoteuszblochmobica · 2020-02-06T19:00:16Z

Root of Trust is no longer automatically and silently created.

Summary of changes

Removed automatic creation of the Root Of Trust when DeviceKey::generate_derived_key(...) API is used.
User must generate or inject the key before first use of DeviceKey API.

Impact of changes

DeviceKey is not automatically generated anymore. This might impact test application which expect the key to be present, but do not explicitly generate it.

Migration actions required

Application developers must be aware that if their device does not have Root of Trust permanently injected, which is usually true in development phase, they must generate a key before first use of DeviceKey API. Two options exist for key generation:

Generate a key using TRNG sources by calling DeviceKey::device_generate_root_of_trust()
Inject a secure key by calling DeviceKey::device_inject_root_of_trust(...) in case the device does not have a TRNG available.

Documentation

ARMmbed/mbed-os-5-docs#1244

Release notes

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[x] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Reviewers

@SeppoTakalo
@michalpasztamobica
@Jammu Kekkonen
@teetak01
@janne Kiiskila

ciarmcom · 2020-02-06T20:00:37Z

@tymoteuszblochmobica, thank you for your changes.
@SeppoTakalo @michalpasztamobica @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

Teemu · 2020-02-06T20:04:02Z

Uhh I guess you meant @teetak01

SeppoTakalo · 2020-02-07T10:40:53Z

@yogpan01 and @JammuKekkonen This is now the change we talked about. This basically breaks existing client test applications which assume RoT to be autogenerated, and therefore should only be pushed to Mbed OS 6.0.

Please review and comment. To properly get this working, the new API has to be adopted in client test applications.

SeppoTakalo · 2020-02-07T10:43:20Z

features/device_key/source/DeviceKey.cpp

@@ -259,22 +246,22 @@ int DeviceKey::get_derived_key(uint32_t *ikey_buff, size_t ikey_size, const unsi
    return DEVICEKEY_SUCCESS;
 }

-int DeviceKey::generate_key_by_random(uint32_t *output, size_t size)
+int DeviceKey::device_generate_root_of_trust()


I know I proposed this name, but I'm starting to wonder.. is there and extra "device" in the DeviceKey::device_do_something...

Maybe the DeviceKey::generate_root_of_trust() should be enough.

SeppoTakalo

Change looks good. Just minor change requested.

This need OK from client team before merge.

SeppoTakalo · 2020-02-07T10:46:21Z

features/device_key/source/DeviceKey.cpp

@@ -283,7 +270,7 @@ int DeviceKey::generate_key_by_random(uint32_t *output, size_t size)

    mbedtls_entropy_free(entropy);
    delete entropy;
-
+    device_inject_root_of_trust(key_buff, actual_size);


Error code needs to be recorded from here as well. The injection phase can fail as rell, so

ret = device_inject_root_of_trust(key_buff, actual_size);

VeijoPesonen

Once the issues mentioned by Seppo have been addressed this looks ok from my point of view.

Pull request has been modified.

SeppoTakalo

Looks OK.

This now needs Client teams to approve, so once @teetak01 and @yogpan01 say OK, this is good to go in.

CI tests can be started already.

JammuKekkonen · 2020-02-07T14:00:07Z

features/device_key/source/DeviceKey.h

     */
-    int generate_key_by_random(uint32_t *output, size_t size);
+    int generate_root_of_trust();


Should be changed to public api. Seems like it could/should also be static.

Writing a unit test for this wouldn't hurt.

It is now changed to public.

static DeviceKey &get_instance()
{
// Use this implementation of singleton (Meyer's) rather than the one that allocates
// the instance on the heap, as it ensures destruction at program end (preventing warnings
// from memory checking tools, such as valgrind).
static DeviceKey instance;
return instance;
}

0xc0170 · 2020-02-07T15:24:53Z

CI started

mbed-ci · 2020-02-07T18:33:56Z

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

tymoteuszblochmobica · 2020-02-18T15:34:20Z

Rest of failing tests updated.
@0xc0170 could You restart ?

mbed-ci · 2020-02-26T13:21:31Z

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-IAR

mbed-ci · 2020-02-26T16:30:48Z

Test run: SUCCESS

Summary: 8 of 8 test jobs passed
Build number : 5
Build artifacts

0xc0170 · 2020-02-26T17:01:11Z

@tymoteuszblochmobica I added breaking change label. Can you update the release notes (no migration needed ?) ?

Also:

Possible Cloud Client.
Tests for CC will be added separately.

Is not saying much to a user, what real impact is there?

SeppoTakalo · 2020-02-27T09:39:22Z

@0xc0170 I updated the PR header, that should be now OK for release notes. Or do we need to add a separate title for that?

Since ARMmbed/mbed-os#12385: 0e7a53c DeviceKey Root of Trust generation refactored. the Root of Trust is not automatically generated anymore. We need to generate or inject one explicitly. This commit also improves the readability of the existing code for the injection of Root of Trust.

Since ARMmbed/mbed-os#12385: 0e7a53c DeviceKey Root of Trust generation refactored. the Root of Trust is not automatically generated anymore. We need to generate or inject one explicitly. This commit also improves the readability of the existing code for the injection of Root of Trust. Fixes ARMmbed#71

ciarmcom requested review from michalpasztamobica, SeppoTakalo and a team February 6, 2020 20:00

ciarmcom added the needs: review label Feb 6, 2020

SeppoTakalo added the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Feb 7, 2020

SeppoTakalo requested review from JammuKekkonen, teetak01, yogpan01 and VeijoPesonen February 7, 2020 10:39

SeppoTakalo reviewed Feb 7, 2020

View reviewed changes

SeppoTakalo previously requested changes Feb 7, 2020

View reviewed changes

mergify bot added needs: work and removed needs: review labels Feb 7, 2020

VeijoPesonen approved these changes Feb 7, 2020

View reviewed changes

tymoteuszblochmobica force-pushed the rot branch from 31cfc3e to 1938190 Compare February 7, 2020 13:40

SeppoTakalo previously approved these changes Feb 7, 2020

View reviewed changes

SeppoTakalo added needs: CI needs: review and removed needs: work labels Feb 7, 2020

JammuKekkonen suggested changes Feb 7, 2020

View reviewed changes

mergify bot removed the needs: review label Feb 7, 2020

mergify bot added needs: work and removed needs: CI labels Feb 7, 2020

SeppoTakalo approved these changes Feb 26, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Feb 26, 2020

mergify bot added needs: work and removed needs: CI labels Feb 26, 2020

SeppoTakalo added ready for merge and removed needs: work labels Feb 26, 2020

0xc0170 merged commit 67e9502 into ARMmbed:master Feb 26, 2020

mergify bot removed the ready for merge label Feb 26, 2020

0xc0170 added BREAKING-CHANGE ready for merge labels Feb 26, 2020

mergify bot removed the ready for merge label Feb 26, 2020

VeijoPesonen mentioned this pull request Mar 3, 2020

Refactor TDB internal bounds computation #11629

Merged

This was referenced Mar 3, 2020

KVStore greentea tests broken on some Cypress targets after #12385 #12565

Closed

CY8CPROTO_062_4343W failed on kvstore Greentea tests #12564

Closed

boomer41 mentioned this pull request Mar 23, 2020

Add check for return code in DeviceKey to avoid injecting invalid ROT #12667

Merged

teetak01 mentioned this pull request May 6, 2020

mbed-cloud-client-example no longer works with kvstore FILESYSTEM #12921

Closed

moshe-shahar mentioned this pull request Sep 10, 2020

PSA force generate ROT #13596

Closed

LDong-Arm mentioned this pull request Nov 26, 2020

Generate DeviceKey Root of Trust if SecureStore is enabled ARMmbed/mbed-os-example-kvstore#66

Merged

This was referenced Apr 15, 2021

Fix DeviceKey documentation regarding RoT #14550

Merged

DeviceKey Root of Trust not generated ARMmbed/mbed-os-example-devicekey#71

Closed

LDong-Arm mentioned this pull request Apr 15, 2021

Generate DeviceKey Root of Trust, update Mbed CLI 2 support ARMmbed/mbed-os-example-devicekey#72

Merged

DeviceKey Root of Trust generation refactored. #12385

DeviceKey Root of Trust generation refactored. #12385

Uh oh!

Conversation

tymoteuszblochmobica commented Feb 6, 2020 • edited by SeppoTakalo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Release notes

Pull request type

Test results

Reviewers

@SeppoTakalo @michalpasztamobica @Jammu Kekkonen @teetak01 @janne Kiiskila

Uh oh!

ciarmcom commented Feb 6, 2020

Uh oh!

Teemu commented Feb 6, 2020

Uh oh!

SeppoTakalo commented Feb 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeijoPesonen left a comment

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Feb 7, 2020

Uh oh!

mbed-ci commented Feb 7, 2020

Uh oh!

tymoteuszblochmobica commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbed-ci commented Feb 26, 2020

Uh oh!

mbed-ci commented Feb 26, 2020

Uh oh!

0xc0170 commented Feb 26, 2020

Uh oh!

SeppoTakalo commented Feb 27, 2020

Uh oh!

Uh oh!

tymoteuszblochmobica commented Feb 6, 2020 •

edited by SeppoTakalo

Loading

@SeppoTakalo
@michalpasztamobica
@Jammu Kekkonen
@teetak01
@janne Kiiskila

tymoteuszblochmobica commented Feb 18, 2020 •

edited

Loading