firmware_uefi: Refactor crypto to go through new crypto crate#3175
firmware_uefi: Refactor crypto to go through new crypto crate#3175smalis-msft merged 11 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors firmware_uefi authenticated-variable (PKCS#7) verification to go through the repository’s new support/crypto crate, consolidating OpenSSL usage behind a backend-agnostic API surface (with an OpenSSL implementation on Unix).
Changes:
- Switched UEFI auth-var verification from direct
opensslcalls tocrypto::pkcs7wrappers. - Introduced a new
crypto::pkcs7module (OpenSSL-backed) withPkcs7SignedDataandPkcs7CertStore. - Updated
firmware_uefifeature/dependencies and workspace lockfile accordingly.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
vm/devices/firmware/firmware_uefi/src/service/nvram/spec_services/auth_var_crypto.rs |
Replaces OpenSSL PKCS#7/X509 verification logic with crypto::pkcs7 usage. |
vm/devices/firmware/firmware_uefi/Cargo.toml |
Switches auth-var-verify-crypto feature dependency from openssl to crypto. |
support/crypto/src/pkcs7/ossl.rs |
Adds OpenSSL-backed implementation of PKCS#7 parsing, cert store, and verification. |
support/crypto/src/pkcs7/mod.rs |
Exposes the PKCS#7 API types and error wrapper. |
support/crypto/src/lib.rs |
Exports pkcs7 on Unix only. |
Cargo.lock |
Adds crypto dependency to firmware_uefi and removes direct openssl dependency. |
vm/devices/firmware/firmware_uefi/src/service/nvram/spec_services/auth_var_crypto.rs
Show resolved
Hide resolved
vm/devices/firmware/firmware_uefi/src/service/nvram/spec_services/auth_var_crypto.rs
Show resolved
Hide resolved
chris-oo
left a comment
chris-oo
left a comment
There was a problem hiding this comment.
can we not have uefi pass the flags or is it because we don't know how to rationalize those flags across crypto backends?
| uefi_mode: bool, | ||
| ) -> Result<bool, Pkcs7Error> { | ||
| if uefi_mode { | ||
| // TODO: set these flags through a better api once its clear how different backends handle similar adjustments |
There was a problem hiding this comment.
should uefi specify this? or not sure until you refactor/have more call sites? this seems odd to have this bool here / leaky abstraction
There was a problem hiding this comment.
I don't know how to rationalize these flags across different backends. Agreed it feels odd, but the crypto crate is not intended to be a general purpose abstraction, it is explictly specialized for openvmm use cases.
There was a problem hiding this comment.
I think I'm OK leaving it as-is in this PR, given your TODO comment, as long as we revisit this when you add more callsites or backends. Maybe instead of uefi_mode, we have a strict_mode that does the opposite?
There was a problem hiding this comment.
Also possible, I need to see what other backends look like. And yeah currently our only callsite is uefi, so...
| uefi_mode: bool, | ||
| ) -> Result<bool, Pkcs7Error> { | ||
| if uefi_mode { | ||
| // TODO: set these flags through a better api once its clear how different backends handle similar adjustments |
There was a problem hiding this comment.
I think I'm OK leaving it as-is in this PR, given your TODO comment, as long as we revisit this when you add more callsites or backends. Maybe instead of uefi_mode, we have a strict_mode that does the opposite?
| /// signature check fails. | ||
| pub fn verify( | ||
| &self, | ||
| store: Pkcs7CertStore, |
There was a problem hiding this comment.
It would be nice to take store by reference, but I understand why you take ownership here -- in practice, if the caller never reuses the store after calling verify it's probably OK.
There was a problem hiding this comment.
To both of your comments, this API is definitely subject to change in the future. Depending on how other backends do it I imagine it will evolve.
Fairly straightforward conversion, although there is an open design question about the flags needing to be passed to openssl.