[manuf] send RMA token hash from the host instead of generating on device by vbendeb · Pull Request #24203 · lowRISC/opentitan

vbendeb · 2024-08-01T23:14:48Z

what happened BRANCH=none BUG=b: TEST=manual Signed-off-by: Vadim Bendebury <vbendeb@google.com>

vbendeb · 2024-08-02T22:03:21Z

rebased and made sure that the previously failing tests pass. Had to move the RMA hash retrieval into manuf_personalize_device_secrets() to keep it in common code, not sure how clean this is from the overall SW architecture standpoiont

timothytrippel

Thanks @vbendeb ! Mostly looks good, just a couple of cleanups / improvments that will make FT provisioning flow in this repo useful during chip bringups.

sw/device/lib/testing/json/provisioning_data.h

timothytrippel · 2024-08-05T16:58:59Z

sw/device/silicon_creator/manuf/lib/personalize.c

+  // console.
+  LOG_INFO("Waiting For RMA Unlock Token Hash ...");
+
+  CHECK_STATUS_OK(UJSON_WITH_CRC(ujson_deserialize_rma_unlock_token_hash_t, uj,


I think we should leave the reading/deserialization of the token from the console to the actual test part (i.e., put it back in the ft_personalize.c file), to facilitate dependency injection of tokens during testing of this library. The personalize library should have no notion of a console uart, it should just be provided with the token hash and write it to OTP.

timothytrippel · 2024-08-05T17:01:03Z

sw/device/silicon_creator/manuf/lib/personalize.h

+status_t manuf_personalize_device_secrets(dif_flash_ctrl_state_t *flash_state,
+                                          const dif_lc_ctrl_t *lc_ctrl,
+                                          const dif_otp_ctrl_t *otp_ctrl,
+                                          ujson_t *uj);


the personalize lib should not be tied to the ujson console (see above comment); it should just take as input the hashed token

sw/device/silicon_creator/manuf/lib/personalize.c

sw/device/silicon_creator/manuf/lib/personalize_functest.c

timothytrippel · 2024-08-05T17:15:42Z

sw/host/provisioning/ft_lib/src/lib.rs

-    host_ecc_sk: PathBuf,
-    timeout: Duration,
-) -> Result<()> {
+fn process_rma_unlock_token(transport: &TransportWrapper, timeout: Duration) -> Result<()> {


Suggested change

fn process_rma_unlock_token(transport: &TransportWrapper, timeout: Duration) -> Result<()> {

fn send_rma_unlock_token(transport: &TransportWrapper, timeout: Duration) -> Result<()> {

timothytrippel · 2024-08-05T17:18:55Z

sw/host/provisioning/ft_lib/src/lib.rs

+    let mut token = ArrayVec::<u32, 4>::new();
+
+    token = (0..token.len())
+        .map(|_| rand::random::<u32>())


Rather than generate random tokens, could we make the token an input to the FT test harness (similar to how the host_sk was before), and put it in sw/device/silicon_creator/manuf/base/provisioning_inputs.bzl? The reason is because the FT test harness (sw/host/provisioning/ft/src/main.rs) will be used by benchtop provisioning flows during initial chip bringup, and we want the HSM in the provisioning infrastructure to generate the LC tokens and then just pass them along to the test harness as inputs.

timothytrippel · 2024-08-05T17:20:06Z

sw/host/provisioning/util_lib/src/lib.rs

+// Life cycle tokens are hashed using a keccak hashing algorithm. The result is
+// a 16 byte value represented as a vector of two u64s.


use doc comments here

Suggested change

// Life cycle tokens are hashed using a keccak hashing algorithm. The result is

// a 16 byte value represented as a vector of two u64s.

/// Life cycle tokens are hashed using a keccak hashing algorithm. The result is

/// a 16 byte value represented as a vector of two u64s.

Modified and re-tested, PTAL

vbendeb · 2024-08-05T17:33:41Z

Sure, @timothytrippel the comments make sense, I was not comfortable bringing the ujson object into personalize.c, but I ended up doing this because I wanted to keep the hash import code in one place and use it where needed (in two different tests now).

Is there a good location for this to avoid duplication?

timothytrippel · 2024-08-05T18:12:20Z

By "hash import code" do you mean the single call to CHECK_STATUS_OK(UJSON_WITH_CRC(ujson_deserialize_rma_unlock_token_hash_t, uj, &token_hash));? It is ok to duplicate this call across tests, it was duplicated when importing the host_sk struct before.

vbendeb · 2024-08-05T20:03:31Z

well, this and also printing the text prompt on the device console which triggers the host sending of the hash. The prompt is something which can easily get out of sync, if it is duplicated in multiple places.

…

On Mon, Aug 5, 2024 at 11:12 AM Timothy Trippel ***@***.***> wrote: By "hash import code" do you mean the single call to CHECK_STATUS_OK(UJSON_WITH_CRC(ujson_deserialize_rma_unlock_token_hash_t, uj, &token_hash));? It is ok to duplicate this call across tests, it was duplicated when importing the host_sk struct before. — Reply to this email directly, view it on GitHub <#24203 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNLPHYCED27FPATKLLGRFTZP66BXAVCNFSM6AAAAABL3PMYI2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRZGYZTCMBYGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

timothytrippel · 2024-08-05T20:46:36Z

Ah, the text prompt though is a contract betweeen the device code and host (test harness) code for a specific test. What is important is that the text prompt used in the personalize_functest device code, matches what is expected in its host test harness code, and likewise the text prompt used in the ft_personalize.c device code matches, what is expected in its matching host test harness code. Specifically, it is not required (and more importantly should not be required) that the synchronization text prompts used in one test match all other text prompts across all other tests that parse the same UJSON message immediately after. The text prompt used in the personalize_functest could be different than the text prompt used in the ft_personalize binary, or rather a test harness could choose not to have a synchronization message all together to improve performance. We want to maintain this flexibility across tests as we may want to optimize a specific test flow later without changing all other tests that do not need to be optimized, and may benefit from more explicit test synchronization console messages.

timothytrippel · 2024-08-06T17:28:54Z

sw/device/silicon_creator/manuf/lib/personalize.c

+  ;
+  LOG_INFO("Provisioning OTP SECRET2 and keymgr flash info pages ...");
+
+  // Perform OTP and flash info writes.
+  LOG_INFO("Provisioning OTP SECRET2 and keymgr flash info pages ...");


Suggested change

;

LOG_INFO("Provisioning OTP SECRET2 and keymgr flash info pages ...");

// Perform OTP and flash info writes.

LOG_INFO("Provisioning OTP SECRET2 and keymgr flash info pages ...");

We should try to keep console prints in the test itself, not in the libs, since we will want to optimize/remove console prints later on to optimize provisioning throughput. (Also, this looks accidentally duplicated.)

oops, not sure how I missed this, too many back and forths

timothytrippel · 2024-08-06T17:31:12Z

sw/device/silicon_creator/manuf/lib/personalize.c

-    const dif_otp_ctrl_t *otp_ctrl,
-    wrapped_rma_unlock_token_t *rma_unlock_token) {
+static status_t otp_partition_secret2_configure(const dif_otp_ctrl_t *otp_ctrl,
+                                                const lc_token_hash_t *hash) {


Suggested change

const lc_token_hash_t *hash) {

const lc_token_hash_t *rma_unlock_token_hash) {

nit: update "@param[out]" above: this is no longer an output

timothytrippel · 2024-08-06T17:32:43Z

sw/host/provisioning/ft/src/main.rs

    let _test_exit_token =
        hex_string_to_u32_arrayvec::<4>(opts.provisioning_data.test_exit_token.as_str())?;
-
+    let rma_uncock_token_hash =


Suggested change

let rma_uncock_token_hash =

let rma_unlock_token_hash =

timothytrippel · 2024-08-06T17:32:55Z

sw/host/provisioning/ft/src/main.rs

        &_perso_certgen_inputs,
        opts.timeout,
        opts.provisioning_data.ca_certificate,
+        &rma_uncock_token_hash,


Suggested change

&rma_uncock_token_hash,

&rma_unlock_token_hash,

timothytrippel · 2024-08-06T17:34:21Z

sw/host/tests/manuf/personalize_functest/src/main.rs

-    host_ecc_sk: PathBuf,
 }

 fn rma_unlock_token_export(opts: &Opts, transport: &TransportWrapper) -> Result<()> {


Suggested change

fn rma_unlock_token_export(opts: &Opts, transport: &TransportWrapper) -> Result<()> {

fn send_rma_unlock_token(opts: &Opts, transport: &TransportWrapper) -> Result<()> {

or something like that

Generating the RMA unlock token on the device and then transferring it to the host for future registering is pretty cumbersome: since the token must not be sent over the host connection in clear a whole ECDH key exchange and generating of an AES key on the target are required. It is much cleaner to have the token generated on the host and have just the hash sent to the device. No need to encrypt the hash, fewer messages sent back and forth. This also removes a lot of complexity from the device code. Tested by verifying that both personalize_functest and ft_provision_fpga test pass on the FPGA setup. Signed-off-by: Vadim Bendebury <vbendeb@google.com>

vbendeb · 2024-08-06T18:08:20Z

updated, PTAL

timothytrippel

LGTM, thanks @vbendeb !

github-actions · 2024-10-17T22:01:06Z

Successfully created backport PR for earlgrey_1.0.0:

Cherry-pick to earlgrey_1.0.0: [manuf] send RMA token hash from the host instead of generating on device #24806

vbendeb requested review from a team as code owners August 1, 2024 23:14

vbendeb requested review from jwnrt and moidx and removed request for a team August 1, 2024 23:14

moidx requested a review from timothytrippel August 1, 2024 23:32

timothytrippel removed the request for review from jwnrt August 2, 2024 00:26

timothytrippel changed the title ~~Rma token hash~~ [manuf] send RMA token hash from the host instead of generating on device Aug 2, 2024

[util_lib] move cShake hash calculation into the common location

0a791e2

what happened BRANCH=none BUG=b: TEST=manual Signed-off-by: Vadim Bendebury <vbendeb@google.com>

vbendeb requested a review from cfrantz as a code owner August 2, 2024 21:51

timothytrippel reviewed Aug 5, 2024

View reviewed changes

timothytrippel self-requested a review August 6, 2024 15:45

timothytrippel reviewed Aug 6, 2024

View reviewed changes

timothytrippel self-requested a review August 6, 2024 21:29

timothytrippel approved these changes Aug 6, 2024

View reviewed changes

timothytrippel merged commit e2e4c03 into lowRISC:master Aug 6, 2024

vbendeb deleted the rma-token-hash branch August 18, 2024 22:41

timothytrippel added the CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 label Oct 17, 2024

github-actions bot mentioned this pull request Oct 17, 2024

Cherry-pick to earlgrey_1.0.0: [manuf] send RMA token hash from the host instead of generating on device #24806

Merged

	fn process_rma_unlock_token(transport: &TransportWrapper, timeout: Duration) -> Result<()> {
	fn send_rma_unlock_token(transport: &TransportWrapper, timeout: Duration) -> Result<()> {

		// Life cycle tokens are hashed using a keccak hashing algorithm. The result is
		// a 16 byte value represented as a vector of two u64s.

	const lc_token_hash_t *hash) {
	const lc_token_hash_t *rma_unlock_token_hash) {

	fn rma_unlock_token_export(opts: &Opts, transport: &TransportWrapper) -> Result<()> {
	fn send_rma_unlock_token(opts: &Opts, transport: &TransportWrapper) -> Result<()> {

Conversation

vbendeb commented Aug 1, 2024 • edited by timothytrippel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vbendeb commented Aug 2, 2024

Uh oh!

timothytrippel left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

vbendeb commented Aug 5, 2024

Uh oh!

timothytrippel commented Aug 5, 2024

Uh oh!

vbendeb commented Aug 5, 2024 via email

Uh oh!

timothytrippel commented Aug 5, 2024

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!

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!

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!

vbendeb commented Aug 6, 2024

Uh oh!

timothytrippel left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vbendeb commented Aug 1, 2024 •

edited by timothytrippel

Loading