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.

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

[dice,cwt] Add CWT implementation #24835

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

tommychiu-github
Copy link
Contributor

Add some helper functions and a CDI_* builder manually.
Some of the changes in the builder will be replaced by a following RP on opentitantool > codegen feature.

@tommychiu-github tommychiu-github requested review from cfrantz and a team as code owners October 22, 2024 08:57
@tommychiu-github tommychiu-github requested review from jadephilipoom and removed request for a team October 22, 2024 08:57
@tommychiu-github
Copy link
Contributor Author

I unintentionally close the former PR (#24824) while resolving the conflicts.
Please move to this new one, sorry for the inconvinence. @timothytrippel @stevenchtsai

WRT to the dup of #24754, the thought is to have a reference implementation & output, for cross checking against the auto-codegen.
Moreover only the last commit will be replaced by the codegen output, the rest commits in this PR are requisites.

@pamaury
Copy link
Contributor

pamaury commented Oct 22, 2024

I am not sure I understand why you want to have a manual C implementation. If you want to have a reference implementation and output, I would suggest to follow the same approach as for the X509 certificates: have a reference rust implementation (easier to review in my opinion) and check that the rust and autogenerated C implementation yield the same results.

@timothytrippel timothytrippel requested review from pamaury and timothytrippel and removed request for jadephilipoom October 22, 2024 17:42
sw/device/silicon_creator/lib/base/util.h Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/base/util.h Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Outdated Show resolved Hide resolved
@timothytrippel
Copy link
Contributor

I am not sure I understand why you want to have a manual C implementation. If you want to have a reference implementation and output, I would suggest to follow the same approach as for the X509 certificates: have a reference rust implementation (easier to review in my opinion) and check that the rust and autogenerated C implementation yield the same results.

I agree with @pamaury here. The reference rust implementation would be more consistent with how the X.509 certs are implemented too, and we can reuse the rust implementation in host code to parse the device generated certs as well. Also there should be unittests generated for your device code as well to test its correctness. If this is really just a stepping stone until #24754 is implemented, and all the manually implemented code will be replaced shortly, could you please comment exactly the locations in the code that will be replaced by #24754?

@tommychiu-github
Copy link
Contributor Author

tommychiu-github commented Oct 23, 2024

I am not sure I understand why you want to have a manual C implementation. If you want to have a reference implementation and output, I would suggest to follow the same approach as for the X509 certificates: have a reference rust implementation (easier to review in my opinion) and check that the rust and autogenerated C implementation yield the same results.

I agree with @pamaury here. The reference rust implementation would be more consistent with how the X.509 certs are implemented too, and we can reuse the rust implementation in host code to parse the device generated certs as well. Also there should be unittests generated for your device code as well to test its correctness. If this is really just a stepping stone until #24754 is implemented, and all the manually implemented code will be replaced shortly, could you please comment exactly the locations in the code that will be replaced by #24754?

Thanks for the input.
Actually it'll be better to separate the utility changes from the manual CDI implementation, for easier review and merge flow.
Will keep only 682b8078b6f5761f6c2b489ef2cee23f698c42c6 in this PR, and move others to a new one (#24850).

And to align with the X509 implementation (codegen, unit test, verifier), I'll set the manual CDI implementation to "draft" for now, till we complete all the related changes.

@tommychiu-github tommychiu-github marked this pull request as draft October 23, 2024 09:03
@tommychiu-github tommychiu-github force-pushed the manual-cdi branch 2 times, most recently from 66a60c2 to e94fabf Compare October 24, 2024 09:03
@tommychiu-github
Copy link
Contributor Author

Verification result for this implementation.

$ hwtrust --verbose dice-chain DiceChain
Proper(
    Chain {
        Root public key: "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEDJ/ChN8wq5MokcByBnjRHC/Mjd5J\nH7kfpD4icOzchTCnGwusRivw4N/egP9Bm+yZae0LrspWkGe6mkJBrLEHvA==\n-----END PUBLIC KEY-----\n",
        DICE Certificate[0]: Payload {
            Issuer: "da517a972ab8b3772eb169507f593d66c7d1bc35a998ae1831c369ca75bc0f5e",
            Subject: "2cc11542a43c450dd06b930c2c9b9781758b16a5d6cc4bb4198798b0e1e67045",
            Mode: Normal,
            Code Hash: "1111111111111111111111111111111111111111111111111111111111111111",
            Config Hash: "7a7c35059d78b25665ac0a96ee30540484690e4f0b4d7c5bd9bf23217cb1edfb",
            Authority Hash: "c19a797fa1fd590cd2e5b42d1cf5f246e29b91684e2f87404b81dc345c7a56a0",
            Config Desc: ConfigDesc {
                component_name: None,
                component_instance_name: None,
                component_version: None,
                resettable: false,
                security_version: Some(
                    0,
                ),
                rkp_vm_marker: false,
                extensions: [],
            },
        },
        DICE Certificate[1]: Payload {
            Issuer: "2cc11542a43c450dd06b930c2c9b9781758b16a5d6cc4bb4198798b0e1e67045",
            Subject: "9b56d423c87a7116e4c51eb68a3aaecefded7faa1662642ba781dbe90e52fb9e",
            Mode: Normal,
            Code Hash: "3333333333333333333333333333333333333333333333333333333333333333",
            Config Hash: "7a7c35059d78b25665ac0a96ee30540484690e4f0b4d7c5bd9bf23217cb1edfb",
            Authority Hash: "c19a797fa1fd590cd2e5b42d1cf5f246e29b91684e2f87404b81dc345c7a56a0",
            Config Desc: ConfigDesc {
                component_name: None,
                component_instance_name: None,
                component_version: None,
                resettable: false,
                security_version: Some(
                    0,
                ),
                rkp_vm_marker: false,
                extensions: [],
            },
        },
    },
)
Success

@timothytrippel
Copy link
Contributor

@tommychiu-github What is the tool you used to verify this chain? the hwtrust tool? Is this tool something that could be vendored into the repo so that it could be used validate the DICE chain the FT provisioning flow test: https://cs.opensource.google/opentitan/opentitan/+/master:sw/host/provisioning/ft_lib/src/lib.rs;l=426?q=ft_lib%2F&ss=opentitan%2Fopentitan ?

@tommychiu-github
Copy link
Contributor Author

Yes, I'm using hwtrust tool from the Android source tree - https://source.corp.google.com/h/googleplex-android/platform/superproject/main/+/main:tools/security/remote_provisioning/hwtrust/src/;l=1
It should be possible to vendor in this module. Was there any example in the OT?

@tommychiu-github tommychiu-github force-pushed the manual-cdi branch 2 times, most recently from 9d570ec to b26205b Compare November 1, 2024 13:33
@tommychiu-github tommychiu-github changed the title [dice,cwt] Add maunal implementations for CDI_0/1 builder [dice,cwt] Add CWT implementation Nov 1, 2024
@timothytrippel
Copy link
Contributor

Should this PR be empty? or should be rebased on top of #24754 to use the autogenerated code? or will a new PR be opened to make use of the autogenerated CWT builder code in #24754 to implement theses functions:
https://cs.opensource.google/opentitan/opentitan/+/master:sw/device/silicon_creator/lib/cert/dice_cwt.c;l=142
https://cs.opensource.google/opentitan/opentitan/+/master:sw/device/silicon_creator/lib/cert/dice_cwt.c;l=152

@tommychiu-github @stevenchtsai ?

@tommychiu-github
Copy link
Contributor Author

Should this PR be empty? or should be rebased on top of #24754 to use the autogenerated code? or will a new PR be opened to make use of the autogenerated CWT builder code in #24754 to implement theses functions: https://cs.opensource.google/opentitan/opentitan/+/master:sw/device/silicon_creator/lib/cert/dice_cwt.c;l=142 https://cs.opensource.google/opentitan/opentitan/+/master:sw/device/silicon_creator/lib/cert/dice_cwt.c;l=152

@tommychiu-github @stevenchtsai ?

No, it should't be closed. However github do it for me when I try to "sync fork" from the upstrem.
Will reopen it after #24754 is landed.

Copy link
Contributor Author

@tommychiu-github tommychiu-github left a comment

Choose a reason for hiding this comment

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

The commit message will be fleshed out once all dependencies are merged.

@timothytrippel
Copy link
Contributor

I think you can update this PR and replace with autogen code now that it has been merged @tommychiu-github

@tommychiu-github tommychiu-github marked this pull request as ready for review November 12, 2024 15:46
@tommychiu-github
Copy link
Contributor Author

Update the latest offline verification result.

hwtrust --verbose dice-chain --allow-any-mode dice.bin 
Proper(
    Chain {
        Root public key: "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEDJ/ChN8wq5MokcByBnjRHC/Mjd5J\nH7kfpD4icOzchTCnGwusRivw4N/egP9Bm+yZae0LrspWkGe6mkJBrLEHvA==\n-----END PUBLIC KEY-----\n",
        DICE Certificate[0]: Payload {
            Issuer: "da517a972ab8b3772eb169507f593d66c7d1bc35a998ae1831c369ca75bc0f5e",
            Subject: "2cc11542a43c450dd06b930c2c9b9781758b16a5d6cc4bb4198798b0e1e67045",
            Mode: Debug,
            Code Hash: "1111111111111111111111111111111111111111111111111111111111111111",
            Config Hash: "7a7c35059d78b25665ac0a96ee30540484690e4f0b4d7c5bd9bf23217cb1edfb",
            Authority Hash: "c19a797fa1fd590cd2e5b42d1cf5f246e29b91684e2f87404b81dc345c7a56a0",
            Config Desc: ConfigDesc {
                component_name: None,
                component_instance_name: None,
                component_version: None,
                resettable: false,
                security_version: Some(
                    0,
                ),
                rkp_vm_marker: false,
                extensions: [],
            },
        },
        DICE Certificate[1]: Payload {
            Issuer: "2cc11542a43c450dd06b930c2c9b9781758b16a5d6cc4bb4198798b0e1e67045",
            Subject: "22d8e07c7ca0a9720bc2c29e437872941e4069cf112e60fa5aec0ad842351b2e",
            Mode: Debug,
            Code Hash: "3333333333333333333333333333333333333333333333333333333333333333",
            Config Hash: "b904d48b9a331c580cc7c511492711060fdcced8d6f0979a55766ace4d88ce99",
            Authority Hash: "c19a797fa1fd590cd2e5b42d1cf5f246e29b91684e2f87404b81dc345c7a56a0",
            Config Desc: ConfigDesc {
                component_name: None,
                component_instance_name: None,
                component_version: None,
                resettable: false,
                security_version: Some(
                    0,
                ),
                rkp_vm_marker: false,
                extensions: [
                    (
                        "-71006",
                        "Bytes([34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34])",
                    ),
                ],
            },
        },
    },
)
Success

@timothytrippel timothytrippel self-requested a review November 12, 2024 17:49
Copy link
Contributor

@timothytrippel timothytrippel left a comment

Choose a reason for hiding this comment

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

mostly, LGTM. A few things should be cleaned up first, then we can get this merged.

sw/device/silicon_creator/lib/cert/BUILD Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/BUILD Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/cert/dice_cwt.c Outdated Show resolved Hide resolved
Flesh out the CDI_0/1 generating APIs by the auto-gen CWT templates.

Signed-off-by: Tommy Chiu <[email protected]>
@timothytrippel
Copy link
Contributor

Thanks @tommychiu-github !

@timothytrippel timothytrippel merged commit 1d921f8 into lowRISC:master Nov 13, 2024
41 checks passed
@timothytrippel timothytrippel added the CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 label Nov 13, 2024
Copy link

Backport failed for earlgrey_1.0.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin earlgrey_1.0.0
git worktree add -d .worktree/backport-24835-to-earlgrey_1.0.0 origin/earlgrey_1.0.0
cd .worktree/backport-24835-to-earlgrey_1.0.0
git switch --create backport-24835-to-earlgrey_1.0.0
git cherry-pick -x f8a8be48c2104976943e1ff95ba7d8e0469040a3

@github-actions github-actions bot added the Manually CherryPick This PR should be manually cherry picked. label Nov 13, 2024
Copy link

Backport failed for earlgrey_1.0.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin earlgrey_1.0.0
git worktree add -d .worktree/backport-24835-to-earlgrey_1.0.0 origin/earlgrey_1.0.0
cd .worktree/backport-24835-to-earlgrey_1.0.0
git switch --create backport-24835-to-earlgrey_1.0.0
git cherry-pick -x f8a8be48c2104976943e1ff95ba7d8e0469040a3

@timothytrippel timothytrippel removed Manually CherryPick This PR should be manually cherry picked. CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 labels Nov 13, 2024
@tommychiu-github tommychiu-github deleted the manual-cdi branch November 15, 2024 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants