Update DeviceKey document after root of trust refactoring #1244

tymoteuszblochmobica · 2020-02-24T15:25:01Z

DeviceKey class has been modified to not allow automatic Root of trust generation.
This requires docs/api/security/Devicekey.md document update.

ohadhawk · 2020-02-24T16:05:10Z

docs/api/security/Devicekey.md

-`device_inject_root_of_trust`: You must call this API once in the lifecycle of the device, before any call to key derivation, if the device does not support true random number generator (`DEVICE_TRNG` is not defined).
+DeviceKey class needs ROT ready to use  before derivation API first call. There are two options to achieve it:
+
+-Create device key using built-in random number generator 


@tymoteuszblochmobica tymoteuszblochmobicaI think you meant to have bulleted items, but instead the dash simply connects to the first word:

AnotherButler · 2020-02-24T19:29:00Z

docs/api/security/Devicekey.md

@@ -19,17 +19,49 @@ The characteristics required by this root of trust are:

 The DeviceKey feature keeps the root of trust key in internal storage, using the KVStore component. Internal storage provides protection from external physical attacks to the device.

-The root of trust is generated at the first use of DeviceKey if the true random number generator is available in the device. If no true random number generator is available, you must pass the injected root of trust key to the DeviceKey before you call the key derivation API.
+The root of trust must be created before its first use. Otherwise key  derivation API  will fail.


Does the user need to create it?

In development phase, probably yes. In production device it should be injected in the factory.

SeppoTakalo

Documentation looks OK.

Just a personal preference, I would prefer to use three backticks to format my codeblocks.

SeppoTakalo · 2020-02-25T09:19:10Z

docs/api/security/Devicekey.md

+            //success
+        } else {
+            //error
+        }


This code block does not seem to be aligned correctly.
Can you use the backticks, so it can be properly aligned?

SeppoTakalo

LGTM

SeppoTakalo · 2020-02-25T12:43:11Z

Oh.. you probably need to start the code snippets with

```C++ NOCI

Because otherwise the CI will try to build those, and fail as they are not complete.

Edit file for active voice.

AnotherButler

Thanks for the PR 👍 Please review my edits to make sure I didn't accidentally change the meaning of anything.

SeppoTakalo

I propose change to two sentences which are a bit misleading.
Otherwise looks OK.

Just a personal preference, do we need to shorten bits to just b? For example https://en.wikipedia.org/wiki/Advanced_Encryption_Standard uses AES is a variant of Rijndael which has a fixed block size of 128 bits, and a key size of 128, 192, or 256 bits. then later on, if they talk about key length they leave the bit out completely.

SeppoTakalo · 2020-02-27T12:26:44Z

docs/api/security/Devicekey.md

-
-The first way is used when device supports random number generator -  ` DEVICE_TRNG` is defined.
-Then  `generate_root_of_trust` must be called only once.
+Use this first method when the device supports a random number generator - when ` DEVICE_TRNG` is defined. Then, call `generate_root_of_trust` only once.


This is now misleading (as was the original as well). The Call of generate_root_of_trust() is the method of generating the key.
So maybe the sentence should be like:

When DEVICE_TRNG is defined, the device supports a random number generator and you may generate the key by calling generate_root_of_trust().
The call succeed only if the key does not already exist. Subsequent calls will fail, without modifying the existing key.

SeppoTakalo · 2020-02-27T12:27:40Z

docs/api/security/Devicekey.md

@@ -50,7 +47,7 @@ if(status == DEVICEKEY_SUCCESS) {
 }
 ```

-If `DEVICE_TRNG` is not defined  then key buffer must be filled manually and followed by `device_inject_root_of_trust` call. The example below shows an injection of a dummy key.  
+If `DEVICE_TRNG` is not defined, the key buffer must be filled manually and followed by `device_inject_root_of_trust` call. The example below shows an injection of a dummy key:


Similarly as above

If DEVICE_TRNG is not defined, the key buffer must be filled manually by calling device_inject_root_of_trust(). The example below shows an injection of a dummy key:

Update phrasing as requested in comments for accuracy.

AnotherButler · 2020-02-28T17:26:02Z

@SeppoTakalo Thanks for the review and suggested changes 👍 Does this look better?

SeppoTakalo · 2020-03-04T11:25:58Z

docs/api/security/Devicekey.md

+
+Both cases requires injecting this key data to the KVStore reserved area.
+
+When `DEVICE_TRNG` is defined, the device supports a random number generator, and you may generate the key by calling `generate_root_of_trust()`. The call succeeds only if the key does not already exist. Subsequent calls will fail if you don't modify the existing key.


Subsequent calls will fail if you don't modify the existing key.

Sorry, I was probably unclear here. There is no way to modify the key, this sentence needs rewrite.

I tried to impress that you can only generate the key once. Modifying it, or regenerating it is not possible.

So subsequent call to this API will fail. And those calls don't modify the key.

Would it be clearer with:

The call succeeds only if the key does not already exist. Existing key cannot be changed.

SeppoTakalo

Just a minor change.
Otherwise looks OK.

Update phrasing as requested in comments for accuracy.

SeppoTakalo · 2020-03-05T09:12:22Z

Looks good now. Thanks @AnotherButler

tymoteuszblochmobica requested review from SeppoTakalo, iriark01, AnotherButler and JammuKekkonen February 24, 2020 15:25

ohadhawk reviewed Feb 24, 2020

View reviewed changes

tymoteuszblochmobica force-pushed the rot branch from 19fefc3 to c895f86 Compare February 24, 2020 16:29

AnotherButler reviewed Feb 24, 2020

View reviewed changes

SeppoTakalo approved these changes Feb 25, 2020

View reviewed changes

tymoteuszblochmobica force-pushed the rot branch from c895f86 to a6ad065 Compare February 25, 2020 10:59

SeppoTakalo approved these changes Feb 25, 2020

View reviewed changes

Update DeviceKey document after root of trust refactoring.

ab7f37b

tymoteuszblochmobica force-pushed the rot branch from a6ad065 to ab7f37b Compare February 25, 2020 16:58

Edit Devicekey.md

ea4fe5c

Edit file for active voice.

AnotherButler approved these changes Feb 26, 2020

View reviewed changes

tymoteuszblochmobica mentioned this pull request Feb 27, 2020

DeviceKey Root of Trust generation refactored. ARMmbed/mbed-os#12385

Merged

SeppoTakalo suggested changes Feb 27, 2020

View reviewed changes

Update phrasing in Devicekey.md

f081d82

Update phrasing as requested in comments for accuracy.

SeppoTakalo reviewed Mar 4, 2020

View reviewed changes

SeppoTakalo suggested changes Mar 4, 2020

View reviewed changes

Fix phrasing for Devicekey.md

47da069

Update phrasing as requested in comments for accuracy.

SeppoTakalo approved these changes Mar 5, 2020

View reviewed changes

JammuKekkonen approved these changes Mar 5, 2020

View reviewed changes

AnotherButler merged commit d2e1c9a into ARMmbed:development Mar 5, 2020


		Both cases requires injecting this key data to the KVStore reserved area.

		When `DEVICE_TRNG` is defined, the device supports a random number generator, and you may generate the key by calling `generate_root_of_trust()`. The call succeeds only if the key does not already exist. Subsequent calls will fail if you don't modify the existing key.

Update DeviceKey document after root of trust refactoring #1244

Update DeviceKey document after root of trust refactoring #1244

Uh oh!

Conversation

tymoteuszblochmobica commented Feb 24, 2020

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!

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!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo commented Feb 25, 2020

Uh oh!

AnotherButler left a comment

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

AnotherButler commented Feb 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo commented Mar 5, 2020

Uh oh!

Uh oh!

SeppoTakalo left a comment •

edited

Loading