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

tpm: also parse TPM2TOOLS_TCTI and TEST_TCTI #872

Closed
wants to merge 1 commit into from

Conversation

THS-on
Copy link
Member

@THS-on THS-on commented Nov 22, 2024

This aligns envs for TCTI with ruts-tss-esapi and tpm2-tools.

Fixes: #871

This aligns envs for TCTI with ruts-tss-esapi and tpm2-tools.

Signed-off-by: Thore Sommer <[email protected]>
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.90%. Comparing base (2f7b3ad) to head (682ce88).
Report is 75 commits behind head on master.

Additional details and impacted files
Flag Coverage Δ
e2e-testsuite 59.90% <100.00%> (+2.31%) ⬆️
upstream-unit-tests 59.90% <100.00%> (+8.89%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
keylime/src/tpm.rs 71.60% <100.00%> (+5.41%) ⬆️

... and 9 files with indirect coverage changes

@ansasaki
Copy link
Contributor

ansasaki commented Nov 26, 2024

In my opinion, this is not correct and even confusing. The agent is not related to and does not use tpm2-tools in any way. It would be weird that by changing tpm2-tools configuration the agent is also affected.

The TCTI configuration should be sufficient, and I believe the confusion comes from missing proper documentation and better error messages.

@THS-on
Copy link
Member Author

THS-on commented Nov 26, 2024

Kinda agree, this just aligns it more with what was supported by the old agent. This options is still weird regardless, as its the only option that cannot be set via the config/env combintation. Ideally it would be KEYLIME_AGENT_TCTI and the default set as an option in the agent.conf. I can change it to that if you want.

@ansasaki
Copy link
Contributor

Kinda agree, this just aligns it more with what was supported by the old agent. This options is still weird regardless, as its the only option that cannot be set via the config/env combintation. Ideally it would be KEYLIME_AGENT_TCTI and the default set as an option in the agent.conf. I can change it to that if you want.

I think that making it a configuration option makes a lot of sense. Adding the tcti config option, which would result on KEYLIME_AGENT_TCTI env var being supported would be the ideal.

Unfortunately, we would have to still support the plain TCTI env var for compatibility. But it is no different from some other options we have.

@THS-on
Copy link
Member Author

THS-on commented Dec 3, 2024

Ok I'll close this PR then and send a new one to add this as an option + override if TCTI env is set.

@THS-on THS-on closed this Dec 3, 2024
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.

Also check TPM2TOOLS_TCTI as environment variable to set TCTI
4 participants