CvmUtil and Vmgstool changes for CPSinTEE and VMGS provenance work#2773
CvmUtil and Vmgstool changes for CPSinTEE and VMGS provenance work#2773canfikret wants to merge 19 commits intomicrosoft:mainfrom
Conversation
…TPM simulator for online unsealing
There was a problem hiding this comment.
Pull request overview
This PR adds a new cvmutil workspace tool and expands TPM helper/protocol support to enable Ubuntu/Canonical interoperability (SRK template + sealed-key import/export flows) and to provide a socket-based vTPM interface for external TPM tooling.
Changes:
- Add new
vm/cvmutilCLI/tooling crate (vTPM blob management, sealed-key marshaling, and a TPM socket server). - Extend
tpm_libwith SRK public template support and make some helper APIs/constants public for reuse. - Modify TPM protocol struct visibility and TPM2B buffer deserialization behavior for “compatibility” parsing.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/devices/tpm/tpm_protocol/src/tpm20proto.rs | Adds TPM2B deserialization “compat” path + makes several TPM structs’ fields public. |
| vm/devices/tpm/tpm_lib/src/lib.rs | Exposes RSA constants, makes import/load public, adds srk_pub_template(). |
| vm/devices/tpm/tpm_device/src/lib.rs | Attempts to expose TPM proto/helper modules + adds handle/NV constants (currently breaks build). |
| vm/cvmutil/src/main.rs | New CLI with multiple subcommands for blob creation, SRK template export, sealing/unsealing, import/export, socket server. |
| vm/cvmutil/src/marshal.rs | Implements marshaling/parsing for Canonical-style sealed key formats and AF split. |
| vm/cvmutil/src/vtpm_helper.rs | Builds an in-memory TPM engine helper around ms-tpm-20-ref for the tool. |
| vm/cvmutil/src/vtpm_sock_server.rs | Implements a TCP socket server intended to be compatible with TPM simulator clients. |
| vm/cvmutil/Cargo.toml | Introduces the new crate and its dependencies (currently has an invalid workspace dep). |
| Cargo.toml | Adds vm/cvmutil to workspace members and workspace dependencies. |
| Cargo.lock | Adds lock entries for the new cvmutil package. |
| // Common issue: size field includes/excludes padding or structure overhead | ||
| if bytes.len() >= start && (bytes.len() - start) <= MAX_DIGEST_BUFFER_SIZE { | ||
| let actual_data_size = bytes.len() - start; | ||
| tracing::debug!( | ||
| "TPM2B_PRIVATE compatibility: expected {} bytes, got {} bytes, using actual size {}", | ||
| size, bytes.len() - start, actual_data_size | ||
| ); | ||
|
|
||
| let mut buffer = [0u8; MAX_DIGEST_BUFFER_SIZE]; | ||
| buffer[..actual_data_size].copy_from_slice(&bytes[start..]); | ||
|
|
||
| return Some(Self { | ||
| size: (actual_data_size as u16).into(), | ||
| buffer, | ||
| }); |
There was a problem hiding this comment.
This change makes Tpm2bBuffer::deserialize accept inputs where the declared size exceeds the available bytes by silently truncating to bytes.len() - start. That relaxes TPM wire-format validation and can mask malformed/truncated data (and potentially enable weird parsing differentials). Consider keeping strict length validation, or gate this behind a very narrow compatibility rule (documented with exact expected delta) rather than accepting any shorter buffer.
| // Common issue: size field includes/excludes padding or structure overhead | |
| if bytes.len() >= start && (bytes.len() - start) <= MAX_DIGEST_BUFFER_SIZE { | |
| let actual_data_size = bytes.len() - start; | |
| tracing::debug!( | |
| "TPM2B_PRIVATE compatibility: expected {} bytes, got {} bytes, using actual size {}", | |
| size, bytes.len() - start, actual_data_size | |
| ); | |
| let mut buffer = [0u8; MAX_DIGEST_BUFFER_SIZE]; | |
| buffer[..actual_data_size].copy_from_slice(&bytes[start..]); | |
| return Some(Self { | |
| size: (actual_data_size as u16).into(), | |
| buffer, | |
| }); | |
| // Common issue: size field includes/excludes padding or structure overhead. | |
| // | |
| // To avoid masking arbitrary truncation, only accept *small* and explicitly | |
| // bounded mismatches between the declared size and the available bytes. | |
| const TPM2B_SIZE_COMPAT_MAX_DELTA: usize = 2; | |
| if bytes.len() >= start { | |
| let available = bytes.len() - start; | |
| // Only consider compatibility when the declared size is larger than what | |
| // we actually have, and the difference is within the small, known delta. | |
| if (size as usize) > available && available <= MAX_DIGEST_BUFFER_SIZE { | |
| let delta = (size as usize) - available; | |
| if delta <= TPM2B_SIZE_COMPAT_MAX_DELTA { | |
| tracing::debug!( | |
| "TPM2B_PRIVATE compatibility: expected {} bytes, got {} bytes (delta {}), using actual size {}", | |
| size, | |
| available, | |
| delta, | |
| available | |
| ); | |
| let mut buffer = [0u8; MAX_DIGEST_BUFFER_SIZE]; | |
| buffer[..available].copy_from_slice(&bytes[start..]); | |
| return Some(Self { | |
| size: (available as u16).into(), | |
| buffer, | |
| }); | |
| } | |
| } |
| pub const TPM_RSA_SRK_HANDLE: ReservedHandle = ReservedHandle::new(TPM20_HT_PERSISTENT, 0x01); | ||
| const TPM_AZURE_AIK_HANDLE: ReservedHandle = ReservedHandle::new(TPM20_HT_PERSISTENT, 0x03); | ||
| const TPM_GUEST_SECRET_HANDLE: ReservedHandle = ReservedHandle::new(TPM20_HT_PERSISTENT, 0x04); |
There was a problem hiding this comment.
These new ReservedHandle constants won’t compile as written because ReservedHandle and TPM20_HT_PERSISTENT are not in scope in this file. Either fully-qualify them (e.g. tpm_protocol::tpm20proto::ReservedHandle) or add the necessary use statements (and consider whether these constants should live in tpm_protocol instead of being duplicated here).
| pub const TPM_RSA_SRK_HANDLE: ReservedHandle = ReservedHandle::new(TPM20_HT_PERSISTENT, 0x01); | |
| const TPM_AZURE_AIK_HANDLE: ReservedHandle = ReservedHandle::new(TPM20_HT_PERSISTENT, 0x03); | |
| const TPM_GUEST_SECRET_HANDLE: ReservedHandle = ReservedHandle::new(TPM20_HT_PERSISTENT, 0x04); | |
| pub const TPM_RSA_SRK_HANDLE: tpm20proto::ReservedHandle = | |
| tpm20proto::ReservedHandle::new(tpm20proto::TPM20_HT_PERSISTENT, 0x01); | |
| const TPM_AZURE_AIK_HANDLE: tpm20proto::ReservedHandle = | |
| tpm20proto::ReservedHandle::new(tpm20proto::TPM20_HT_PERSISTENT, 0x03); | |
| const TPM_GUEST_SECRET_HANDLE: tpm20proto::ReservedHandle = | |
| tpm20proto::ReservedHandle::new(tpm20proto::TPM20_HT_PERSISTENT, 0x04); |
| pub fn create(payload: &[u8]) -> Self { | ||
| use sha2::{Digest, Sha256}; | ||
|
|
||
| // Use Canonical's approach: target 128KB minimum size | ||
| let min_size = 128 * 1024; // 128KB like Canonical | ||
| let stripes = (min_size / payload.len()).max(1) + 1; | ||
|
|
There was a problem hiding this comment.
AfSplitData::create divides by payload.len() to compute stripes. If payload is empty, this will panic with division-by-zero. Handle the empty-payload case explicitly (return an error or define a minimum block size/stripes).
| fn handle_tpm_data_client( | ||
| stream: TcpStream, | ||
| tpm_engine: Arc<Mutex<TpmEngineHelper>>, | ||
| _nv_accessor: Arc<Mutex<impl std::any::Any + Send>>, |
There was a problem hiding this comment.
handle_tpm_data_client uses impl Trait nested inside Arc<Mutex<...>> (Arc<Mutex<impl Any + Send>>), which is not allowed and will not compile. Use a generic type parameter for the function, or use a concrete type (e.g. Arc<Mutex<Vec<u8>>>) since the caller passes Arc<Mutex<Vec<u8>>>.
| fn handle_tpm_data_client( | |
| stream: TcpStream, | |
| tpm_engine: Arc<Mutex<TpmEngineHelper>>, | |
| _nv_accessor: Arc<Mutex<impl std::any::Any + Send>>, | |
| fn handle_tpm_data_client<N: std::any::Any + Send>( | |
| stream: TcpStream, | |
| tpm_engine: Arc<Mutex<TpmEngineHelper>>, | |
| _nv_accessor: Arc<Mutex<N>>, |
| /// Seal data to SRK using TPM-standard format compatible with Ubuntu secboot. | ||
| fn seal_data_to_srk(srk_pub_path: &str, input_file: &str, output_file: &str) { | ||
| use marshal::{AfSplitData, CURRENT_METADATA_VERSION, KEY_DATA_HEADER}; | ||
| use std::fs; | ||
|
|
||
| tracing::info!("Creating TPM-standard sealed key compatible with Ubuntu secboot"); | ||
| tracing::info!("Reading input data from: {}", input_file); | ||
| let input_data = fs::read(input_file).expect("failed to read input file"); | ||
| tracing::info!("Input data size: {} bytes", input_data.len()); | ||
|
|
||
| // Create minimal TPM structures for a sealed data object | ||
| // We'll create a simple keyedobject that contains the sealed data | ||
|
|
||
| // 1. Create a minimal TPM2B_PRIVATE containing our data | ||
| let key_private = Tpm2bBuffer::new(&input_data).expect("input data too large for TPM2B buffer"); | ||
|
|
||
| // 2. Create a minimal TPM2B_PUBLIC for the sealed object | ||
| // Use the SRK public key template but mark it as a data object | ||
| let srk_template = tpm_helper::srk_pub_template().expect("failed to create SRK template"); | ||
| let mut sealed_template = srk_template; |
There was a problem hiding this comment.
seal_data_to_srk takes srk_pub_path but never uses it, so the --seal operation ignores the user-provided SRK public key and instead always uses srk_pub_template(). Either remove the unused argument or actually load/parse srk_pub_path and seal against that public area.
| // if bytes.len() < end { | ||
| // return None; | ||
| // } | ||
|
|
There was a problem hiding this comment.
Commented-out code was left in deserialize (the old if bytes.len() < end { return None; } block). Please remove the commented block to keep the parsing logic unambiguous and maintainable.
| // if bytes.len() < end { | |
| // return None; | |
| // } |
| pub mod tpm20proto; | ||
| pub mod tpm_helper; |
There was a problem hiding this comment.
tpm_device declares pub mod tpm20proto; / pub mod tpm_helper; but there are no corresponding tpm20proto.rs/tpm_helper.rs files in this crate, so the crate will fail to compile. If the intent is to re-export tpm_protocol::tpm20proto and helper APIs, use pub use ... or add the missing module files.
| pub mod tpm20proto; | |
| pub mod tpm_helper; | |
| pub use tpm_lib::tpm20proto; | |
| pub use tpm_lib::tpm_helper; |
| // Read size (2 bytes, LITTLE endian to match our export format) | ||
| if data.len() < 10 { | ||
| return Err(io::Error::new(io::ErrorKind::InvalidData, "Data too short for size")); | ||
| } | ||
| // Read size (4 bytes, LITTLE endian to match our export format) | ||
| if data.len() < 12 { | ||
| return Err(io::Error::new(io::ErrorKind::InvalidData, "Data too short for size")); | ||
| } | ||
| let size = u32::from_le_bytes([data[8], data[9], data[10], data[11]]); |
There was a problem hiding this comment.
AfSplitData::from_bytes has inconsistent/duplicated size validation (checks for 10 bytes then immediately for 12 bytes) and a misindented comment suggesting a 2-byte size while actually reading a 4-byte size. This is easy to misread and could hide real format bugs; please remove the dead check and make the header parsing/comments match the actual serialized format.
| running: Arc<AtomicBool>, | ||
| ) { | ||
| use std::io::Write; | ||
| let peer_addr = stream.peer_addr().unwrap_or_else(|_| "unknown".parse().unwrap()); |
There was a problem hiding this comment.
The fallback in peer_addr().unwrap_or_else(|_| "unknown".parse().unwrap()) will still panic because "unknown" is not a valid SocketAddr. Use an Option<SocketAddr>/Result and log it, or fall back to a string without parsing.
| let peer_addr = stream.peer_addr().unwrap_or_else(|_| "unknown".parse().unwrap()); | |
| let peer_addr = match stream.peer_addr() { | |
| Ok(addr) => addr.to_string(), | |
| Err(e) => { | |
| tracing::warn!("Failed to get peer address: {}", e); | |
| "unknown".to_string() | |
| } | |
| }; |
| // if the vtpm file exists, delet it and create a new one | ||
| if std::path::Path::new(&path).exists() { | ||
| tracing::info!( | ||
| "vTPM file already exists. Deleting the existing file and creating a new one." | ||
| ); | ||
| fs::remove_file(&path).expect("failed to delete existing vtpm file"); | ||
| } |
There was a problem hiding this comment.
Typo in comment: "delet it" -> "delete it".
|
Ples update the pr title with |
|
@mingweishih, @stunes-ms, FYI. Thanks. |
CvmUtil and VmgsTool changes for supporting provenance and confidential provisioning service in TEE.