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

Streamline unit-tests that use a TPM #4239

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

shjala
Copy link
Member

@shjala shjala commented Sep 11, 2024

  1. Create all the required keys for the TPM tests in the prep-and-test.sh before running the tests. This changes adds the creation of AIK key and also changes other keys creation method to creates all keys under Owner hierarchy rather than EK, just like we do in EVE.
  2. Add helper functions for simulate TPM.
  3. Streamline all the unit-tests that uses TPM, by removing the duplicated code and making all to use the sim tpm helper functions.

@github-actions github-actions bot requested a review from rucoder September 11, 2024 14:14
@shjala shjala force-pushed the tpm_tests_streamline branch from 697520c to 0e9453f Compare September 11, 2024 14:22
@OhmSpectator
Copy link
Member

image
Sorry =D

@OhmSpectator
Copy link
Member

In general, it would be nice to run the test in a container so that it can be used on any system. However, the preparation script currently works only on Ubuntu-based systems. But it's out of scope of the PR.

Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

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

Looks good. I realized too late that it was basically about just moving functions to shared files =D

tests/tpm/prep-and-test.sh Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Another question of curiosity. In the commit message, you say:

This changes adds the creation of AIK key and
also changes other keys creation method to creates all keys under Owner
hirearchy rather than EK, just like we do in EVE.

Can you please point me to where it is done in EVE?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here :

func createOtherKeys(override bool) error {
if err := etpm.CreateKey(log, etpm.TpmDevicePath, etpm.TpmEKHdl, tpm2.HandleEndorsement, etpm.DefaultEkTemplate, override); err != nil {
return fmt.Errorf("error in creating Endorsement key: %w ", err)
}
if err := etpm.CreateKey(log, etpm.TpmDevicePath, etpm.TpmSRKHdl, tpm2.HandleOwner, etpm.DefaultSrkTemplate, override); err != nil {
return fmt.Errorf("error in creating SRK key: %w ", err)
}
if err := etpm.CreateKey(log, etpm.TpmDevicePath, etpm.TpmAIKHdl, tpm2.HandleOwner, etpm.DefaultAikTemplate, override); err != nil {
return fmt.Errorf("error in creating Attestation key: %w ", err)
}
if err := etpm.CreateKey(log, etpm.TpmDevicePath, etpm.TpmQuoteKeyHdl, tpm2.HandleOwner, etpm.DefaultQuoteKeyTemplate, override); err != nil {
return fmt.Errorf("error in creating Quote key: %w ", err)
}
if err := etpm.CreateKey(log, etpm.TpmDevicePath, etpm.TpmEcdhKeyHdl, tpm2.HandleOwner, etpm.DefaultEcdhKeyTemplate, override); err != nil {
return fmt.Errorf("error in creating ECDH key: %w ", err)
}
return nil
}

btw, the ownerHandle in CreateKey is unused so don't and we don't create the EK under tpm2.HandleEndorsement, and tpm2.HandleOwner is hardcoded in the function. I want to clean that up for ages :D

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!
It may make sense to add a comment to the prep script that it refers to this code (and vice versa), especially if you have a plan to change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure.

pkg/pillar/evetpm/testhelper.go Show resolved Hide resolved
pkg/pillar/evetpm/testhelper.go Show resolved Hide resolved
Create all the required keys for the TPM tests in the prep-and-test.sh
before running the tests. This changes adds the creation of AIK key and
also changes other keys creation method to creates all keys under Owner
hirearchy rather than EK, just like we do in EVE.

Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
These functions can be used in all other tests that rely on a sim TPM.

Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
Streamline all the unit-tests that uses TPM, by removing the
duplicated code and making all to use the sim tpm helper functions.

Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
@shjala shjala force-pushed the tpm_tests_streamline branch from 0e9453f to ad620da Compare September 11, 2024 18:10
@shjala shjala requested a review from OhmSpectator September 11, 2024 18:29
@OhmSpectator OhmSpectator self-requested a review September 12, 2024 09:16
@OhmSpectator
Copy link
Member

@shjala

[yetus: pkg/pillar/cmd/tpmmgr/tpmmgr.go#L717](https://github.com/lf-edge/eve/pull/4239/files#annotation_26226706455)
codespell: hirarchy ==> hierarchy

The very last thing)

We create the keys in test script based on the tpmmgr code. Add a comment
and reminder, in case something changed.

Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
@shjala shjala force-pushed the tpm_tests_streamline branch from abbf712 to 5d5d5db Compare September 13, 2024 09:34
@shjala
Copy link
Member Author

shjala commented Sep 13, 2024

SPDX check glitching.

@OhmSpectator
Copy link
Member

OhmSpectator commented Sep 13, 2024

SPDX check glitching.

Yep... I also don't understand how it happened.
I guess it's just because the branch is not rebased on the latest master state.

@OhmSpectator OhmSpectator merged commit af37037 into lf-edge:master Sep 13, 2024
20 of 21 checks passed
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.

2 participants