fix: correct policy test flows and policysigned auth session handling#27
Conversation
- Delete policy_workflows.rs and redistribute its tests:
- Move policynv tests (eq, neq, ult variants) into session_policy.rs
- Move parameter encryption tests into new parameter_encryption.rs
- Remove comments on unimplemented tests for getsessionauditdigest
- Add missing policy subcommand tests to session_policy.rs:
- policycphash (trial session with cpHash digest)
- policynamehash (trial session with nameHash digest)
- policytemplate (trial session with templateHash digest)
- policyduplicationselect (trial session with synthetic TPM names)
- policyauthorizenv (policy session with NV-stored policy)
- policyauthorize (full workflow: sign policy, verify, authorize)
- policysigned (trial session with signing key)
- Add new test files for previously untested subcommands:
- context.rs: contextsave/contextload roundtrip and readpublic verification
- sequence.rs: hashsequencestart/hmacsequencestart/sequenceupdate/
sequencecomplete with single-shot hash/hmac comparison
- duplicate_import.rs: verify duplicate fails on fixedTPM/fixedParent keys
- Extend hierarchy.rs with setcommandauditstatus and pcrallocate tests
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Takuma IMAMURA <209989118+hyperfinitism@users.noreply.github.com>
hyperfinitism
added
bug
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the policysigned command to utilize raw ESYS FFI calls via RawEsysContext and significantly expands the integration test suite, adding coverage for context management, hierarchy control, parameter encryption, and numerous policy subcommands. Review feedback identifies the use of unstable 'let chains' and potential memory leaks if I/O operations fail before allocated ESYS pointers are freed. Additionally, it is noted that input data for TPM buffers is currently subject to silent truncation, which should be addressed with proper length validation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7b4e41600
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if let Some(ref path) = self.ticket_out | ||
| && !ticket_ptr.is_null() | ||
| { | ||
| let ticket = &*ticket_ptr; |
Check failure
Code scanning / CodeQL
Access of invalid pointer High
Copilot Autofix
AI 6 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
There was a problem hiding this comment.
Pull request overview
This PR improves the integration-test coverage for TPM session/policy and related command flows, and updates the policysigned CLI command to use raw ESYS FFI calls as a workaround for an upstream tss-esapi issue.
Changes:
- Expanded and reorganized integration tests for policy subcommands (including
policysigned), plus added new test modules for sequences, context save/load, parameter encryption, and duplicate/import failure cases. - Added new hierarchy/admin integration tests (
setcommandauditstatus,pcrallocate). - Reimplemented
PolicySignedcommand execution using raw ESYS FFI viaRawEsysContext.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/session_policy.rs |
Adds many policy subcommand integration tests and updates the module doc coverage list. |
tests/sequence.rs |
New integration tests for hash/HMAC sequence start/update/complete flows. |
tests/policy_workflows.rs |
Removes the previous combined workflow tests (content moved/split into other files). |
tests/parameter_encryption.rs |
New integration tests for encrypted HMAC sessions (getrandom + nvwrite/nvread). |
tests/hierarchy.rs |
Adds integration tests for command audit status and PCR allocation. |
tests/duplicate_import.rs |
New negative tests ensuring duplicate fails on non-duplicatable keys. |
tests/context.rs |
New integration tests for contextsave/contextload roundtrips. |
src/cmd/policysigned.rs |
Switches PolicySigned implementation from tss-esapi wrapper to raw ESYS FFI calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| info!("policy signed succeeded"); | ||
| let timeout_data = if !timeout_ptr.is_null() { | ||
| let t = &*timeout_ptr; |
Check failure
Code scanning / CodeQL
Access of invalid pointer High
Copilot Autofix
AI 6 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| anyhow::bail!("Esys_PolicyGetDigest failed: 0x{rc:08x}"); | ||
| } | ||
| let data = if !digest_ptr.is_null() { | ||
| let d = &*digest_ptr; |
Check failure
Code scanning / CodeQL
Access of invalid pointer High
Copilot Autofix
AI 6 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
hyperfinitism
force-pushed
the
fix/policy-tests-and-policysigned-auth
branch
from
e572fba to
fc6abb2
Compare
The tss-esapi wrapper's policy_signed() calls required_session_1() which forces callers to set an auth session. Otherwise, the wrapper returns an error before calling the underlying ESYS function Esys_PolicySigned(). This violates the TCG TSS 2.0 ESAPI specification which defines all three session handles of Esys_PolicySigned() to be optional. For a workaround, this commit bypasses the tss-esapi wrapper and calls the raw ESYS FFI function directly. Once the patch is merged upstream, we can remove the workaround and use the wrapper again. Signed-off-by: Takuma IMAMURA <209989118+hyperfinitism@users.noreply.github.com>
hyperfinitism
force-pushed
the
fix/policy-tests-and-policysigned-auth
branch
from
fc6abb2 to
35c3fb2
Compare
This pull request introduces several new integration tests improving robustness and test coverage.
To work around a bug present in the upstream
tss-esapi, this PR also modifies thePolicySignedcommand to use raw Esys FFI calls directly. I have already submitted a pull request with a fix to the upstream. Once this patch is merged into the upstream, one can safely remove the workaround and use the wrapper again.Test Coverage
Of the 106 subcommands, 95 are tested. Test coverage is measured using
llvm-covv0.8.5: