feat(esapi): add missing TPM commands in integrity_collection_pcr#650
feat(esapi): add missing TPM commands in integrity_collection_pcr#650hyperfinitism wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds missing ESAPI command wrappers in the integrity_collection_pcr command set, and extends the integration test suite to exercise the new wrappers (with the known swtpm/libtpms limitations handled via #[ignore] and no_run examples).
Changes:
- Added
Contextwrappers forPCR_Event,PCR_Allocate,PCR_SetAuthPolicy, andPCR_SetAuthValueinintegrity_collection_pcr. - Added integration tests for the new wrappers; tests for
PCR_SetAuthPolicy/PCR_SetAuthValueare ignored due toswtpm/libtpmsbehavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tss-esapi/src/context/tpm_commands/integrity_collection_pcr.rs |
Adds the four missing PCR-related ESAPI wrappers and rustdoc examples (with no_run where applicable). |
tss-esapi/tests/integration_tests/context_tests/tpm_commands/integrity_collection_pcr_tests.rs |
Adds integration tests for the new PCR wrapper methods (with #[ignore] for commands unsupported by swtpm). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use crate::common::create_ctx_with_session; | ||
| use tss_esapi::{handles::PcrHandle, structures::MaxBuffer}; | ||
|
|
||
| #[test] | ||
| fn test_pcr_event() { | ||
| let mut context = create_ctx_with_session(); | ||
| let data = MaxBuffer::from_bytes(&[0x01, 0x02, 0x03, 0x04]).unwrap(); | ||
| context.pcr_event(PcrHandle::Pcr16, data).unwrap(); | ||
| } |
There was a problem hiding this comment.
To implement the suggested check, we must add a public getter to the DigestValues struct as DigestList does.
There was a problem hiding this comment.
To implement the suggested check, we must add a public getter to the
DigestValuesstruct asDigestListdoes.
Sounds like a good idea
There was a problem hiding this comment.
Addressed. Added public getter value() to DigestValues and implemented hash value check.
4393abd to
2b6768e
Compare
ionut-arm
left a comment
There was a problem hiding this comment.
Thanks! A few comments below
| size: event_data_bytes.len() as u16, | ||
| ..Default::default() | ||
| }; | ||
| event.buffer[..event_data_bytes.len()].copy_from_slice(event_data_bytes); |
There was a problem hiding this comment.
TPM2B_EVENT isn't guaranteed to be big enough for this (fixed at 1024, whereas MaxBuffer may be bigger). Any chance you can create a new native struct for EVENT and take that as a param instead?
There was a problem hiding this comment.
Addressed. Added the structures::Event buffer type for TPM2B_EVENT.
| &mut self, | ||
| auth_handle: AuthHandle, | ||
| pcr_allocation: PcrSelectionList, | ||
| ) -> Result<(bool, u32, u32, u32)> { |
There was a problem hiding this comment.
I think it's fairly unusual for us to return a tuple of primitives like that - seems like a simple struct that just holds these details would be a good way to make the result easier to consume.
There was a problem hiding this comment.
Addressed. Added PcrAllocateResult type.
| // Missing function: _TPM_Hash_Start | ||
| // Missing function: _TPM_Hash_Data | ||
| // Missing function: _TPM_Hash_End | ||
| // Missing function: _TPM_Hash_Start (platform-level indication, not an ESAPI command) |
There was a problem hiding this comment.
Good catch, please just remove these lines in this case.
| use crate::common::create_ctx_with_session; | ||
| use tss_esapi::{handles::PcrHandle, structures::MaxBuffer}; | ||
|
|
||
| #[test] | ||
| fn test_pcr_event() { | ||
| let mut context = create_ctx_with_session(); | ||
| let data = MaxBuffer::from_bytes(&[0x01, 0x02, 0x03, 0x04]).unwrap(); | ||
| context.pcr_event(PcrHandle::Pcr16, data).unwrap(); | ||
| } |
There was a problem hiding this comment.
To implement the suggested check, we must add a public getter to the
DigestValuesstruct asDigestListdoes.
Sounds like a good idea
Signed-off-by: Takuma IMAMURA <209989118+hyperfinitism@users.noreply.github.com>
2b6768e to
3011095
Compare
Signed-off-by: Takuma IMAMURA <209989118+hyperfinitism@users.noreply.github.com>
3011095 to
8317e1c
Compare
Added the following wrapper functions with integration tests for these commands: - pcr_event (ESAPI spec 11.3.52) - pcr_allocate (11.3.54) - pcr_set_auth_policy (11.3.55) - pcr_set_auth_value (11.3.56) To make implementation of pcr_allocate clean, added return type PcrAllocateResult. swtpm (libtpms) does not support PCR_SetAuthPolicy or PCR_SetAuthValue; these commands return TPM_RC_VALUE. So their integration tests are marked #[ignore], and their doc examples are marked no_run. Signed-off-by: Takuma IMAMURA <209989118+hyperfinitism@users.noreply.github.com>
8317e1c to
1a21adc
Compare
|
Thank you for your detailed review and suggestions. I have made the necessary changes. |
This pull request implements the following Esys wrapper the following wrapper functions with integration tests for these commands:
pcr_event(ESAPI spec 11.3.52)pcr_allocate(11.3.54)pcr_set_auth_policy(11.3.55)pcr_set_auth_value(11.3.56)These were extracted from #625.
Limitation
swtpm(libtpms) does not supportPCR_SetAuthPolicyorPCR_SetAuthValue; these commands always returnTPM_RC_VALUE. So their integration tests are marked#[ignore], and their doc examples are markedno_run.Reference: https://github.com/stefanberger/libtpms/blob/521c51073fe6f7c56023db78e56961fcaf7906e8/src/tpm2/TPMCmd/Platform/src/PlatformPcr.c