IGVMfilegen: Sign hash of ID block with temporary key#3832
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the IGVM file generator’s SNP measurement path to additionally emit an IgvmDirectiveHeader::SnpIdBlock directive by generating a temporary ECDSA P-384 keypair and signing the SHA-384 hash of the SNP ID block. To support this, the measurement generation APIs are adjusted to allow mutation of the directive list, and symcrypt is added as a dependency for ECC signing/key export.
Changes:
- Emit a temporarily-signed SNP ID block directive during SNP measurement generation (using SymCrypt ECDSA P-384).
- Adjust measurement-generation interfaces to take a mutable directive vector so the SNP path can append directives.
- Add
symcryptas a dependency forigvmfilegen.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| vm/loader/igvmfilegen/src/signed_measurement/snp.rs | Adds temp key generation + ECDSA signature/public key export and pushes a SnpIdBlock directive during SNP measurement generation. |
| vm/loader/igvmfilegen/src/file_loader.rs | Updates the measurement-generation trait and call sites to pass a mutable directives Vec (enabling SNP to append directives). |
| vm/loader/igvmfilegen/Cargo.toml | Adds symcrypt as a direct dependency for the new SNP signing code. |
| // Generate a temporary key and sign the ID block hash. | ||
| let (id_key_signature, id_public_key) = sign_id_block_with_temp_key(&psp_id_block)?; | ||
| directive_headers.push(IgvmDirectiveHeader::SnpIdBlock { | ||
| compatibility_mask: DEFAULT_COMPATIBILITY_MASK, | ||
| author_key_enabled: 0, | ||
| reserved: [0; 3], |
There was a problem hiding this comment.
yes this is expected for non release builds
There was a problem hiding this comment.
You should make sure Ben is aware of this as it might impact his reproducible build work
There was a problem hiding this comment.
@benhillis fyi the IGVM file for non-release\shipping builds will contain an ID block for SNP VMs signed with a temp key. This only impacts the igvm file not the build itself
|
Can you check if this change can also be done with |
|
yes I'll look into that In reply to: 4836364193 |
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
| .map_err(|e| Error::TempSigning(format!("EcdsaKeyPair::generate: {e}")))?; | ||
|
|
||
| // Hash the ID block with SHA-384. | ||
| let mut hash = Sha384::new(); |
There was a problem hiding this comment.
Don't forget my other comment on teams about moving to use crypto-provided hashing too, but that can come in a follow up.
| sys::EcdsaKeyPairInner::generate(curve).map(Self) | ||
| } | ||
|
|
||
| /// Sign a pre-computed hash value. Returns the signature as `r || s` |
There was a problem hiding this comment.
Would it be more user-friendly if this took the raw data and did the hashing internally? We have a HashAlgorithm enum for exactly this use case, and do similar for other algorithms.
jennagoddard
changed the title
igvmfilegen enables `crypto`'s `native` + `vendored` features, which vendor-builds OpenSSL from source on Linux. The vendored build runs OpenSSL's `./Configure` perl script, needing the OpenSSL headers and a perl install with the standard modules. Ubuntu's base `perl` ships them (so GitHub CI is green), but minimal Fedora/Azure Linux images don't, failing with `Can't locate FindBin.pm`. This adds `openssl-devel` + `perl` as pre-build deps for the igvmfilegen node, mirroring `build_vmgstool.rs` which already does this for the same crypto feature combo. No-op on non-Linux hosts. Surfaced by a downstream Azure Linux OneBranch build of igvmfilegen going red after #3832 added the vendored crypto dependency. --------- Co-authored-by: OpenVMM Team <openvmm@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
No description provided.