igvmfilegen: migrate SNP ID block to serializer + add-snp-id-block command by mingweishih · Pull Request #3923 · microsoft/openvmm

mingweishih · 2026-07-10T22:29:41Z

Replace the in-tree signed_measurement module with the igvm crate's IgvmSerializer for launch-measurement computation, and move SNP ID block handling out of the build-time auto-write into an explicit add-snp-id-block subcommand.

Delete signed_measurement/{snp,tdx,vbs} and vbs_defs; compute launch digests via IgvmSerializer::measurement_for.
Add snp_id_block.rs: unsigned artifact emission, temp-key signing (dev/test), and out-of-band DER signature + X.509/SPKI public-key verification (production).
manifest emits -snp.idblock (raw SnpPspIdBlock) for SNP.
Add crypto::ecdsa::verify_prehash (ECDSA P-384 verify) across openssl, symcrypt, and BCrypt backends.
Extract measurement_diag.rs (launch-measurement diagnostics) and platform_mask.rs (compatibility-mask helpers).

github-actions · 2026-07-10T22:29:53Z

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

Copilot

Pull request overview

This PR refactors igvmfilegen to rely on the upstream igvm crate’s IgvmSerializer for launch-measurement computation, removes the in-tree signed-measurement implementations (including the vbs_defs crate), and introduces an explicit “add SNP ID block” flow with production (out-of-band) and dev/test (temporary key) signing modes. It also wires the new SNP ID-block signing payload artifact (*.idblock) through the Flowey build pipeline and extends the crypto crate with ECDSA public-key construction support needed for signature verification.

Changes:

Replace manual SNP/TDX/VBS measurement implementations with IgvmSerializer::measurement_for, and move measurement “diagnostic dressing” into measurement_diag.
Add add-snp-id-block command plus snp_id_block.rs to emit/verify/sign SNP ID blocks (including out-of-band DER signature + X.509/SPKI public key parsing).
Extend crypto::ecdsa with EcdsaPublicKey::new(...) and backend implementations, and propagate new *-snp.idblock artifact through Flowey outputs.

Reviewed changes

Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
vm/vbs_defs/src/lib.rs	Deleted VBS definition structs previously used by in-tree VBS measurement logic.
vm/vbs_defs/Cargo.toml	Removed the `vbs_defs` crate manifest.
vm/loader/igvmfilegen/src/snp_id_block.rs	New SNP ID block payload generation, signing, and out-of-band signature verification (with unit tests).
vm/loader/igvmfilegen/src/platform_mask.rs	New helpers to map `IgvmPlatformType` ↔ compatibility masks and derive short labels.
vm/loader/igvmfilegen/src/measurement_diag.rs	New per-platform diagnostic structure logging (VBS signed data / SNP ID block / TDX MRTD).
vm/loader/igvmfilegen/src/main.rs	Adds `add-snp-id-block` subcommand and switches manifest flow to `IgvmSerializer`-based measurements + sibling artifact emission.
vm/loader/igvmfilegen/src/file_loader.rs	Removes measurement generation from loader finalize path; tests updated to use `IgvmSerializer` measurements.
vm/loader/igvmfilegen/src/signed_measurement/mod.rs	Deleted legacy signed measurement module root.
vm/loader/igvmfilegen/src/signed_measurement/snp.rs	Deleted legacy SNP measurement + auto-injected temp-signed ID block logic.
vm/loader/igvmfilegen/src/signed_measurement/tdx.rs	Deleted legacy TDX MRTD computation implementation.
vm/loader/igvmfilegen/src/signed_measurement/vbs.rs	Deleted legacy VBS measurement implementation.
vm/loader/igvmfilegen/Cargo.toml	Updates deps for new functionality (der/x509-cert/bitfield-struct, dev-deps for crypto test helpers).
support/crypto/src/ecdsa/mod.rs	Adds public-key reconstruction API and tests for reimport/verify behavior.
support/crypto/src/ecdsa/ossl.rs	Implements OpenSSL backend public-key construction.
support/crypto/src/ecdsa/symcrypt.rs	Implements SymCrypt backend public-key construction.
support/crypto/src/ecdsa/win.rs	Implements BCrypt backend public-key construction and adds a Drop impl for owned public keys.
flowey/flowey_lib_hvlite/src/run_igvmfilegen.rs	Plumbs `*-snp.idblock` artifact path out of the igvmfilegen run.
flowey/flowey_lib_hvlite/src/build_openhcl_igvm_from_recipe.rs	Includes optional SNP ID-block payload in endorsements packaging.
flowey/flowey_lib_hvlite/src/_jobs/local_build_igvm.rs	Copies `openhcl-snp.idblock` to local build output dir when present.
Cargo.toml	Removes `vbs_defs` from workspace dependencies.
Cargo.lock	Locks new transitive deps (e.g. `corim`, `x509-cert` chain, `uuid`, sha2 versioning) due to feature/dependency changes.

Copilot

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.

Copilot

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.

github-actions · 2026-07-11T01:18:01Z

4 Petri tests failed (0 unstable)

smalis-msft · 2026-07-13T15:12:00Z

 "igvm_defs",
 "open-enum",
 "range_map_vec",
+ "sha2 0.10.9",


Can igvm update this to the same version we have here?

Also it's unfortunate that we need to pull this crate back in. But as long as it's only being used by igvmfilegen and not the loader I don't think it's that big a deal.

smalis-msft · 2026-07-13T15:18:55Z

+        // public view obtained by transmuting a keypair reference is never
+        // dropped as an `EcdsaPublicKeyInner`, so the keypair's handle is not
+        // double-freed.
+        if !self.handle.is_invalid() {


Remove the if (and the unnecessary comment above explaining how Drop works). We should be enforcing at a type level that we never have an invalid handle.

Should do this to the keypair's drop too.

smalis-msft · 2026-07-13T15:19:58Z

+        // Enforce the exact `Qx || Qy` length so all backends reject
+        // non-canonical encodings identically (the OpenSSL backend does the
+        // same before constructing its point).
+        if public_key.len() != curve.key_size() * 2 {


Consider if the key length check in each backend is really needed to be done ourselves, presumably each backend will be validating this too right?

smalis-msft · 2026-07-13T15:21:08Z

+
+        // Build a BCRYPT_ECCKEY_BLOB: { dwMagic: u32, cbKey: u32 } header (both
+        // little-endian) followed by Qx || Qy.
+        let mut blob = Vec::with_capacity(size_of::<BCRYPT_ECCKEY_BLOB>() + public_key.len());


Why does this need to be a vec instead of actually being a BCRYPT_ECCKEY_BLOB?

smalis-msft · 2026-07-13T15:23:38Z

+
+/// Extract the SubjectPublicKey bit string from an X.509 certificate (DER).
+fn cert_point(der: &[u8]) -> anyhow::Result<Vec<u8>> {
+    let cert = x509_cert::Certificate::from_der(der).context("parsing X.509 certificate")?;


We shouldn't be doing all this work here, crypto can already extract keys like this.

…mmand Replace the in-tree signed_measurement module with the igvm crate's IgvmSerializer for launch-measurement computation, and move SNP ID block handling out of the build-time auto-write into an explicit add-snp-id-block subcommand. - Delete signed_measurement/{snp,tdx,vbs} and vbs_defs; compute launch digests via IgvmSerializer::measurement_for. - Add snp_id_block.rs: unsigned artifact emission, temp-key signing (dev/test), and out-of-band DER signature + X.509/SPKI public-key verification (production). - manifest emits <base>-snp.idblock (raw SnpPspIdBlock) for SNP. - Add crypto::ecdsa::verify_prehash (ECDSA P-384 verify) across openssl, symcrypt, and BCrypt backends. - Extract measurement_diag.rs (launch-measurement diagnostics) and platform_mask.rs (compatibility-mask helpers). Signed-off-by: Ming-Wei Shih <mishih@microsoft.com>

Copilot

Pull request overview

Copilot reviewed 22 out of 23 changed files in this pull request and generated 1 comment.

+    // Refuse to double-add: a second ID block would make the file ambiguous.
+    // Callers wanting to re-sign must start from a file without one.


smalis-msft · 2026-07-15T17:45:02Z

+//! valid. Its presence signals the IGVM loader to set `id_block_en = 1`.
+
+use anyhow::Context;
+use der::Decode;


How much of the der code in here should be in crypto instead?

smalis-msft · 2026-07-15T17:46:40Z

+    /// Construct an ECDSA public key from a DER-encoded X.509 certificate,
+    /// using the certificate's `SubjectPublicKeyInfo`. Fails if the certificate
+    /// key is not an ECDSA key on `curve`.
+    pub fn from_x509_der(curve: EcdsaCurve, cert_der: &[u8]) -> Result<Self, EcdsaError> {


This method shouldn't exist, we should be using the already existing x509 public_key method. You'll have to change it to return an enum of possible key types.

smalis-msft · 2026-07-15T17:47:32Z

+    /// Construct an ECDSA public key from a DER-encoded `SubjectPublicKeyInfo`
+    /// (a bare public key, i.e. the `PUBLIC KEY` PEM type). Fails if the key is
+    /// not an ECDSA key on `curve`.
+    pub fn from_public_key_der(curve: EcdsaCurve, spki_der: &[u8]) -> Result<Self, EcdsaError> {


Presumably this also should be going through existing crypto parsing methods rather than having igvmfilegen do all the parsing and der work itself.

github-actions · 2026-07-15T18:41:00Z

2 Petri tests failed (2 unstable)

mingweishih requested a review from a team as a code owner July 10, 2026 22:29

Copilot AI review requested due to automatic review settings July 10, 2026 22:29

github-actions Bot added the unsafe Related to unsafe code label Jul 10, 2026

mingweishih changed the title ~~igvmfilegen: migrate SNP ID block to serializer + add-snp-id-block co…~~ igvmfilegen: migrate SNP ID block to serializer + add-snp-id-block command Jul 10, 2026

Copilot started reviewing on behalf of mingweishih July 10, 2026 22:30 View session

mingweishih requested review from chris-oo, jennagoddard, smalis-msft and stunes-ms July 10, 2026 22:32