opentitan icon indicating copy to clipboard operation
opentitan copied to clipboard

[manuf] send RMA token hash from the host instead of generating on device

Open vbendeb opened this issue 1 year ago • 1 comments

Fixes #24034.

vbendeb avatar Aug 01 '24 23:08 vbendeb

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

vbendeb avatar Aug 02 '24 22:08 vbendeb

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?

vbendeb avatar Aug 05 '24 17:08 vbendeb

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.

timothytrippel avatar Aug 05 '24 18:08 timothytrippel

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 https://github.com/lowRISC/opentitan/pull/24203#issuecomment-2269631080, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABNLPHYCED27FPATKLLGRFTZP66BXAVCNFSM6AAAAABL3PMYI2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRZGYZTCMBYGA . You are receiving this because you were mentioned.Message ID: @.***>

vbendeb avatar Aug 05 '24 20:08 vbendeb

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 avatar Aug 05 '24 20:08 timothytrippel

updated, PTAL

vbendeb avatar Aug 06 '24 18:08 vbendeb

Successfully created backport PR for earlgrey_1.0.0:

  • #24806

github-actions[bot] avatar Oct 17 '24 22:10 github-actions[bot]