RFC: Add OpenHCL telemetry for VMGS provisioning #2024
Conversation
| .map_err(|e| { | ||
| tracing::error!( | ||
| CVM_ALLOWED, | ||
| op_type = "VtpmKeysProvision", |
There was a problem hiding this comment.
It might be better to create enums for op_type and key_type
There was a problem hiding this comment.
I'll leave this as an open question for right now, and ask other reviewers: if we make these enums, where should they live? especially op_type.
| key_type = "AkPub", | ||
| bios_guid = self.bios_guid, | ||
| pub_key = self.ak_pub_hash, | ||
| success = true, |
There was a problem hiding this comment.
do you need to include this field?
There was a problem hiding this comment.
I think making it explicit is more in-line with the principles for our HCL telemetry -- this way, we can filter explicitly based on 'success' instead of implicitly based on log level. (I don't have a strong opinion about this though.)
stunes-ms
force-pushed
the
user/mikestunes/telemetry
branch
from
20afa23 to
44bfa47
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive telemetry logging for VMGS provisioning and TPM operations in OpenHCL. The goal is to provide structured, queryable logs that can track provisioning success and indicate whether Trusted Launch features are being enabled.
Key changes include:
- Added operation-specific telemetry logs with timing metrics across TPM and VMGS operations
- Enhanced logging for AK cert provisioning, NV read/write operations, and GSP callbacks
- Added BIOS GUID tracking to TPM device resources for better telemetry correlation
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/vmgs/vmgs/src/vmgs_impl.rs | Changed log level and added op_type for VMGS provisioning telemetry |
| vm/devices/tpm_resources/src/lib.rs | Added bios_guid field to TpmDeviceHandle for logging purposes |
| vm/devices/tpm/src/tpm_helper.rs | Added comprehensive telemetry for TPM NV read/write operations with timing |
| vm/devices/tpm/src/lib.rs | Added detailed AK cert and key provisioning telemetry with error handling |
| vm/devices/get/guest_emulation_transport/src/client.rs | Added GSP callback operation telemetry |
| openhcl/underhill_attestation/src/lib.rs | Added VMGS decryption telemetry with GSP type tracking |
| Various config files | Updated to pass bios_guid parameter and added dependencies |
vm/devices/tpm/src/lib.rs
Outdated
| TpmErrorKind::CreateAkPublic(e) | ||
| })?; | ||
|
|
||
| // TODO: make sure we're hashing the right thing |
There was a problem hiding this comment.
The TODO comment on line 603 indicates uncertainty about what is being hashed. This should be documented or the TODO resolved to clarify that the AK public key modulus is the intended data for hashing.
| // TODO: make sure we're hashing the right thing | |
| // Hash the AK public key modulus to derive its fingerprint. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This is still an open question; to be discussed soon
(Remaining open items: we have not yet settled the exact way to hash the AK public key.)
This is an RFC review request for adding telemetry (in the form of event markers in our logs) around operations of interest for VMGS provisioning and certain vTPM features (AKPub, AKCert, etc.). The goal of this approach is to have structured, easily queryable data that can show that provisioning is successful, and indicate whether Trusted Launch features (OpenHCL-provisioned VMGS, GSP key) are being enabled.
This PR is intended to match a corresponding legacy HCL change as closely as possible, given the differences in their logging implementations. In particular, the operation names (here, added as an
op_typeproperty on traces) and other metadata properties should match.