Skip to content

dasharo-security/tpm-support.robot: Refactor TPM version and support … #507

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

Merged
merged 8 commits into from
Feb 4, 2025

Conversation

SebastianCzapla
Copy link
Contributor

…tests

Currently, TPM Support and TPM Version test are split into different tests groups. Merging of version and support checks into one allows us to skip few extra reboot cycles during testing. Additionally, it provides opportunity to add coverage for TPM 1.2

@miczyg1
Copy link
Contributor

miczyg1 commented Sep 18, 2024

It is not entirely redundant. See: #495 (comment)

The test to verify TPM version in firmware should check if it is TPM 1.2 or 2.0

@SebastianCzapla SebastianCzapla force-pushed the tpm_version_support_refactor branch 2 times, most recently from 8534884 to 12b0d62 Compare September 25, 2024 09:35
@SebastianCzapla SebastianCzapla marked this pull request as ready for review September 25, 2024 09:36
@SebastianCzapla SebastianCzapla force-pushed the tpm_version_support_refactor branch 2 times, most recently from 98700f0 to 5b01d14 Compare September 25, 2024 10:03
@SebastianCzapla
Copy link
Contributor Author

This branch was rebased onto this #487
Here's a log from dasharo-security test suite run with this patch applied.

dasharo-security.tar.gz

@SebastianCzapla SebastianCzapla force-pushed the tpm_version_support_refactor branch from 5b01d14 to 31bc60d Compare September 30, 2024 14:00
@SebastianCzapla SebastianCzapla force-pushed the tpm_version_support_refactor branch 4 times, most recently from 8bab4c2 to a5c2700 Compare October 15, 2024 14:44
@SebastianCzapla
Copy link
Contributor Author

Updated and rebased as per suggestions.
Using Setup Menu to detect TPM has a tradeoff, it no longer is automated test for laptop testing. Is this an acceptable tradeoff? @miczyg1

Also, when testing this solution, I found an issue with OptiPlex platform Dasharo/dasharo-issues#1091

@miczyg1
Copy link
Contributor

miczyg1 commented Oct 16, 2024

Updated and rebased as per suggestions. Using Setup Menu to detect TPM has a tradeoff, it no longer is automated test for laptop testing. Is this an acceptable tradeoff? @miczyg1

Also, when testing this solution, I found an issue with OptiPlex platform Dasharo/dasharo-issues#1091

I gave you 3 options and it looks like you chose maybe the worst one. Of course this is not a good tradeoff from laptops perspective. But, was this test even being run on laptops if it only supports SSH tests over OS?

Looks like option 1 is the best, but again it would be limited to UEFI payload only.

Option 2 is the best since it uses coreboot log alone to determine the TPM chip. Based on the TPM chip we may derive what TPM version it is. But this requires the most work.

@SebastianCzapla SebastianCzapla force-pushed the tpm_version_support_refactor branch 2 times, most recently from 0e885b2 to 39fc118 Compare November 13, 2024 13:29
@SebastianCzapla SebastianCzapla force-pushed the tpm_version_support_refactor branch 2 times, most recently from 40a9e27 to 4550b3a Compare November 28, 2024 15:34
@SebastianCzapla
Copy link
Contributor Author

Made a change to use TPM chip as a base for validation. I also kept old log based detection, as a fallback mechanism in case chip detection fails (it happens on Optiplex due to log truncation). Should there be a warning that this mechanism was used? At the moment it only logs to console.

Some TPM variables for various platforms are not set, but I tested it on few laptops and it seems to work correctly.

@SebastianCzapla SebastianCzapla force-pushed the tpm_version_support_refactor branch from 128948a to 010bdab Compare January 20, 2025 12:33
@SebastianCzapla SebastianCzapla force-pushed the tpm_version_support_refactor branch 2 times, most recently from f611b87 to f1138d7 Compare January 23, 2025 13:17
@SebastianCzapla SebastianCzapla force-pushed the tpm_version_support_refactor branch 2 times, most recently from 816be6f to 2a074ad Compare January 27, 2025 11:20
Copy link
Contributor

@miczyg1 miczyg1 left a comment

Choose a reason for hiding this comment

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

Still don't quite agree on the names of the keywords. They both check logs in cbmem, but doesn't explicitly say which log.

@SebastianCzapla
Copy link
Contributor Author

@miczyg1 Fixed the names as per suggestion. Additionally, I've introduced small change to get-robot-variables.sh; I changed the usage of path to usage of a variable that holds this path, as it was also missing. Retested after all the changes, here's a log

@SebastianCzapla SebastianCzapla force-pushed the tpm_version_support_refactor branch from d35e121 to 083f57b Compare February 4, 2025 13:49
@miczyg1
Copy link
Contributor

miczyg1 commented Feb 4, 2025

@SebastianCzapla it just needs a rebase before it can be merged fast-forward

…tests

This commit introduces two new variables, EXPECTED_TPM_CHIP and
EXPECTED_TPM_VERSION. Additionally, refactor few keywords and tests
within tpm-support.robot

Signed-off-by: Sebastian Czapla <[email protected]>
Signed-off-by: Sebastian Czapla <[email protected]>
@SebastianCzapla SebastianCzapla force-pushed the tpm_version_support_refactor branch from 083f57b to 7cd6900 Compare February 4, 2025 14:09
@SebastianCzapla
Copy link
Contributor Author

@miczyg1 Done

@miczyg1 miczyg1 merged commit 7cd6900 into develop Feb 4, 2025
1 of 3 checks passed
@miczyg1 miczyg1 deleted the tpm_version_support_refactor branch February 4, 2025 14:16
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.

5 participants