crypto: Add native windows backend for pkcs7, use it in uefi#3265
crypto: Add native windows backend for pkcs7, use it in uefi#3265smalis-msft wants to merge 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Add a Windows-native PKCS#7 verification backend to the crypto crate and make authenticated-variable verification in firmware_uefi unconditional (no longer feature-gated), while removing noisy tracing during verification failures.
Changes:
- Added
support/cryptoPKCS#7 Windows backend and selected backend by target OS. - Removed the
auth-var-verify-cryptofeature gating infirmware_uefiand downstream feature wiring (encryption). - Simplified PKCS#7 verification failure handling (removed trace-level logging).
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| vmm_core/vmotherboard/Cargo.toml | Removes encryption feature that previously toggled auth-var crypto verification. |
| vm/devices/firmware/firmware_uefi/src/service/nvram/spec_services/mod.rs | Removes feature-gated auth var crypto paths; makes the crypto error variants/structs unconditional. |
| vm/devices/firmware/firmware_uefi/src/service/nvram/spec_services/auth_var_crypto.rs | Removes crate-level cfg gating so module is always compiled. |
| vm/devices/firmware/firmware_uefi/fuzz/Cargo.toml | Drops auth-var-verify-crypto feature from fuzz target configuration. |
| vm/devices/firmware/firmware_uefi/Cargo.toml | Makes crypto/der non-optional dependencies; removes crypto feature flag. |
| support/crypto/src/pkcs7/win.rs | Introduces Windows CryptoAPI implementation for PKCS#7 parsing and verification. |
| support/crypto/src/pkcs7/ossl.rs | Removes trace logging on verification error and returns Ok(false) directly. |
| support/crypto/src/pkcs7/mod.rs | Selects PKCS#7 backend by OS (ossl on unix, win on windows). |
| support/crypto/src/lib.rs | Makes pkcs7 module available beyond unix-only builds. |
| support/crypto/Cargo.toml | Removes tracing dependency from the crypto crate. |
| openvmm/openvmm_entry/Cargo.toml | Removes encryption feature passthrough. |
| openvmm/openvmm/Cargo.toml | Removes encryption feature passthrough. |
| openhcl/underhill_core/Cargo.toml | Removes encryption feature from linux dependency feature set. |
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
| let mut policy_para: CERT_CHAIN_POLICY_PARA = unsafe { std::mem::zeroed() }; | ||
| policy_para.cbSize = size_of::<CERT_CHAIN_POLICY_PARA>() as u32; | ||
|
|
||
| if uefi_mode { |
There was a problem hiding this comment.
Yeah. This isn't new, openssl needs this flag too. Basically uefi requires some steps of the normal cert validation process be skipped. Couldn't think of a better name for it, but I didn't want to make it the default. Currently the one and only place we call into pkcs7 code passes true.
| }; | ||
|
|
||
| if verify_result.is_err() { | ||
| return Ok(false); |
There was a problem hiding this comment.
Do we really want to treat all errors as Ok(false)?
| let signer_cert = | ||
| unsafe { CertGetSubjectCertificateFromStore(msg_store.0, ENCODING, cert_info) }; | ||
| if signer_cert.is_null() { | ||
| return Ok(false); |
There was a problem hiding this comment.
Ditto here, is this the right return value?
| }; | ||
|
|
||
| if chain_result.is_err() { | ||
| return Ok(false); |
There was a problem hiding this comment.
Like this just can't be right...
Add a native win32 backend to the crypto crate for pkcs7. firmware_uefi is the only user of this algorithm currently, and this support allows us to remove a feature that previously made it optional.
Also remove some unhelpful tracing that AI snuck in previously.
This PR deserves some careful review, as I'm really not familiar with the win32 crypt APIs. It was mostly written by claude, but I did go read the docs afterwards to attempt to verify it and it looks right to me.