fix: canonicalize TPM EK NV fallback#3434
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughTPM EK certificate fallback now uses structured NV index metadata, probes known indices, and returns one canonical certificate. RSA certificates are preferred in table order, with ECC used when no RSA certificate is readable. Tests cover both selection paths. ChangesTPM EK certificate fallback
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/host-support/src/hardware_enumeration/tpm.rs (2)
155-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse structured tracing fields instead of string interpolation.
nv_index.nameandnv_index.indexare interpolated into the message string rather than passed as structured fields, breaking logfmt-style parsing for this log line.As per coding guidelines, "tracing messages should pass common values as structured fields rather than interpolating them into strings" and "Services must emit logs in logfmt syntax".
🧾 Proposed fix
Ok(cert) => { tracing::info!( - "Read TPM EK certificate {} from NV index {}", - nv_index.name, - nv_index.index + name = nv_index.name, + index = nv_index.index, + "Read TPM EK certificate from NV index" ); certs.push((*nv_index, cert)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/host-support/src/hardware_enumeration/tpm.rs` around lines 155 - 170, Update the tracing::info! call in the TPM EK certificate enumeration loop to pass nv_index.name and nv_index.index as structured tracing fields, while keeping the message static and preserving the existing log content.Sources: Coding guidelines, Path instructions
421-478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a scenario with multiple readable non-RSA certs.
Current cases cover multi-RSA tie-break and single-ECC fallback, but nothing exercises
first_non_rsa_cert.is_none()guarding against a second readable non-RSA cert overwriting the first (e.g., two readable ECC entries at different table positions, no RSA readable). Worth locking this branch in explicitly.As per coding guidelines, "Use table-driven tests for functions mapping inputs to outputs, errors, or observable results."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/host-support/src/hardware_enumeration/tpm.rs` around lines 421 - 478, Add a table-driven test alongside the existing TPM certificate selection scenarios covering two readable ECC/non-RSA certificates at different NV indices with no readable RSA certificate. Configure the fake runner so both reads succeed and assert the selection yields the first ECC certificate, exercising the first_non_rsa_cert guard without changing the existing RSA-preference cases.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/host-support/src/hardware_enumeration/tpm.rs`:
- Around line 155-170: Update the tracing::info! call in the TPM EK certificate
enumeration loop to pass nv_index.name and nv_index.index as structured tracing
fields, while keeping the message static and preserving the existing log
content.
- Around line 421-478: Add a table-driven test alongside the existing TPM
certificate selection scenarios covering two readable ECC/non-RSA certificates
at different NV indices with no readable RSA certificate. Configure the fake
runner so both reads succeed and assert the selection yields the first ECC
certificate, exercising the first_non_rsa_cert guard without changing the
existing RSA-preference cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1c3fe55b-837f-4ad7-8208-75db9599de6a
📒 Files selected for processing (1)
crates/host-support/src/hardware_enumeration/tpm.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
1 participant
Summary
tpm2_getekcertificate.Root Cause
The prior fallback was intended to work around
tpm2_getekcertificatefailing on systems with many NV entries, but it changed identity semantics by concatenating all readable EK certs. Upstreamtpm2_getekcertificatemay read multiple NV certs internally, but its default stdout behavior emits only one certificate, preferring RSA for backward compatibility.Upstream Check
Checked
tpm2-tools/tools/tpm2_getekcertificate.con upstreammaster:ek_index_mapsuses the same index order, andprocess_outputwrites one RSA certificate first for backward compatibility. With default stdout output, additional NV certs are not concatenated.Validation
cargo fmt --package carbide-host-support --check— passed on Linux.git diff --check— passed.cargo test -p carbide-host-support hardware_enumeration::tpm— passed on Linux: 5 passed, 0 failed.