firmware_uefi: add telemetry on secureboot configuration - #4100
firmware_uefi: add telemetry on secureboot configuration#4100maheeraeron wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
There are correctness issues to address (notably missing size_of/size_of_val scope and misleading/absent telemetry in some cases) plus missing unit test coverage for the new signature parsing/comparison logic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds Secure Boot telemetry for firmware_uefi by comparing the PK/KEK/db/dbx variables loaded in NVRAM against the baseline Secure Boot template (when present), and emitting trace events with revision/config metadata.
Changes:
- Introduces a baseline template revision string (
BASELINE_REVISION) and re-exports it viafirmware_uefi_resources. - Parses the base template into
BaseTemplateVarsup-front and adds Secure Boot signature parsing + set-diff logic to evaluate missing baseline signatures. - Adds
cvm_tracingusage for allowed/confidential telemetry fields and exposes template signature access viaBaseTemplateVars::signatures().
File summaries
| File | Description |
|---|---|
| vm/devices/firmware/hyperv_secure_boot_templates/src/lib.rs | Adds a baseline revision constant for built-in Secure Boot templates. |
| vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs | Adds Secure Boot configuration telemetry and signature parsing/comparison helpers; refactors base template parsing flow. |
| vm/devices/firmware/firmware_uefi/Cargo.toml | Adds cvm_tracing dependency for telemetry tagging. |
| vm/devices/firmware/firmware_uefi_resources/src/lib.rs | Re-exports BASELINE_REVISION for consumers. |
| vm/devices/firmware/firmware_uefi_custom_vars/src/lib.rs | Adds BaseTemplateVars::signatures() accessor needed for telemetry evaluation. |
| Cargo.lock | Records the new cvm_tracing dependency in the lockfile. |
Review details
Comments suppressed due to low confidence (1)
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:342
- The warn message "base secure boot template variable is missing" is misleading: in this branch the variable exists, but
missing_entriesindicates some baseline signatures are absent. Consider updating the wording to reflect missing signatures rather than a missing variable.
"base secure boot template variable is missing"
- Files reviewed: 5/6 changed files
- Comments generated: 4
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The updated nvram/mod.rs introduces compile-breaking missing imports and has a couple of correctness/clarity and coverage gaps that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (5)
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:33
size_of/size_of_valare used in this module (e.g.,signature_list_payloadandhandle_nvram_command_inner) but are not imported or qualified, which will not compile.
use std::borrow::Cow;
use std::collections::BTreeSet;
use std::fmt::Debug;
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:331
- This log message indicates the whole variable is "present", but the check is specifically that baseline signatures are not missing (it doesn't assert equality, and extra entries may exist). Updating the message would make the telemetry less ambiguous.
missing_entries,
loaded_variable_bytes,
"base secure boot template variable is present"
);
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:237
- The new secure-boot telemetry path adds non-trivial parsing logic (
collect_signature_set/signature_list_payload) and comparison behavior inreport_secure_boot_configuration, but the test module only exercises first-boot injection/delta parsing. Adding unit tests for auth-header stripping and signature-set comparisons would help prevent regressions.
async fn report_secure_boot_configuration(
&mut self,
base_template_vars: Option<&BaseTemplateVars>,
custom_uefi_config_present: bool,
) {
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:128
base_template_jsonis parsed intoBaseTemplateVars(and then cloned) unconditionally on every non-restore cold boot, even when NVRAM is not empty andinject_vars_on_first_bootwill return early. This adds avoidable JSON parsing/allocation work to boot paths where the base template is only needed for first-boot injection and/or secure boot telemetry.
let base_template_vars = base_template_json
.map(|template_json| {
hyperv_uefi_custom_vars_json::parse_template_json(template_json.as_bytes())
})
.transpose()
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:343
- This log message says the variable is "missing", but
missing_entries > 0means the variable was loaded and parsed successfully and is only missing some baseline signatures. Consider wording it as missing baseline signatures to avoid misinterpretation.
loaded_entries,
missing_entries,
loaded_variable_bytes,
"base secure boot template variable is missing"
);
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
Two new telemetry log messages are misleading (they describe variable presence/absence rather than baseline signature match/mismatch), which can cause incorrect interpretation by telemetry consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:348
- The warn-level telemetry message says the baseline secure-boot variable is missing, but the code is reporting that some baseline signatures are missing from the loaded variable (the variable itself was loaded earlier). This wording can mislead telemetry/alerting.
"baseline secure boot template variable is missing"
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:336
- The info-level telemetry message says the baseline secure-boot variable is present, but at this point the variable is already known to exist; the check is actually that no baseline signatures are missing from the loaded variable. This wording can mislead telemetry consumers.
This issue also appears on line 348 of the same file.
"baseline secure boot template variable is present"
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The change is additive (telemetry + helpers), includes targeted unit tests for the new parsing/diff logic, and does not alter Secure Boot enforcement behavior.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
ca0ffb7 to
de654ae
Compare
| impl SecureBootConfigReport<'_> { | ||
| fn log(&self) { | ||
| if self.missing_entries == 0 { | ||
| tracing::info!(CVM_ALLOWED, ?self); |
There was a problem hiding this comment.
This is the important part
There was a problem hiding this comment.
Hmm, do we need to trace this as a warning if missing entries is non-zero? OR should we just always log this to info, and azsec just needs to be interested in nonzero missing entries to flag a VM's variable, along with other heuristics from the report?
There was a problem hiding this comment.
I'm thinking as a platform that supports custom uefi deltas, customer's shouldn't expect a warning in their secure boot report. So, I think this should actually just be an info log, and its up to consumers to derive conclusions on the report's details per variable. What do others think?
There was a problem hiding this comment.
🟡 Not ready to approve
Telemetry logging includes at least one misleading message and a small unreachable-path implementation detail that should be corrected to avoid confusing downstream telemetry consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:268
baseline_revisionis computed as eitherBASELINE_REVISIONor "none", but the function returns early when there is no baseline template/signatures; as written, the "none" branch is unreachable and can confuse future readers.
// Get the baseline revision
let baseline_revision = if base_template_vars.is_some() {
BASELINE_REVISION
} else {
"none"
};
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:299
- When the variable exists but its data is empty (
Ok((_, data))withdata.is_empty()), the warning says it is "missing in NVRAM". That’s misleading (the variable is present but empty) and makes it hard to distinguish corruption/empty-state from a true NOT_FOUND case.
let loaded_variable = match self.services.get_variable_ucs2(vendor, name).await {
Ok((_, data)) if !data.is_empty() => data,
Ok((_, data)) => {
tracing::warn!(
CVM_ALLOWED,
variable_name,
loaded_variable_bytes = data.len(),
"loaded secure boot template variable is missing in NVRAM"
);
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
It introduces avoidable per-boot JSON parsing/allocation overhead by parsing the base template unconditionally even when it isn’t needed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:152
base_template_jsonis parsed unconditionally inNvramServices::neweven when NVRAM is not empty (not first boot) and Secure Boot is disabled, which adds avoidable JSON parsing and allocations on every boot. Previously the parse was deferred until the first-boot injection path.
if !is_restoring {
let base_template_vars = base_template_json
.map(|template_json| {
hyperv_uefi_custom_vars_json::parse_template_json(template_json.as_bytes())
})
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The telemetry logic is covered by targeted unit tests and prior review items appear addressed, with the remaining note being a non-blocking performance optimization suggestion.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:205
FinalVars::resolve(base_template_vars.cloned(), ...)clones the entireBaseTemplateVars(including potentially large Secure Boot cert/db/dbx data) before injection. On first boot with Secure Boot disabled this clone looks unnecessary, and even when enabled it can transiently double memory/allocations.
Consider restructuring so the base template is moved into FinalVars::resolve when it’s only needed for injection (e.g., make inject_initial_vars take Option<BaseTemplateVars> and only clone in the caller when you also need to retain the template for telemetry).
let custom_template_delta = custom_uefi_json
.map(|json| hyperv_uefi_custom_vars_json::parse_delta_json(json.as_bytes()))
.transpose()
.map_err(NvramSetupError::LoadCustomUefiJson)?;
let final_vars = FinalVars::resolve(base_template_vars.cloned(), custom_template_delta)
.map_err(NvramSetupError::ApplyCustomTemplate)?;
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The changes are internally consistent, include targeted unit tests for the new parsing/comparison helpers, and no functional correctness issues were identified during review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:265
- The warning emitted when no baseline signatures are available is a bit misleading:
base_template_vars.and_then(BaseTemplateVars::signatures)can beNoneeither because there is no baseline template or because the template exists but has nosignaturesfield. The message also says "secureboot" without a space. Consider logging this as "no baseline secure boot signatures available" and includebaseline_revisionas a structured field for easier telemetry queries.
let Some(signatures) = base_template_vars.and_then(BaseTemplateVars::signatures) else {
tracing::warn!(
CVM_ALLOWED,
"no baseline secure boot template available; skipping secureboot configuration report"
);
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Adds logic to evaluate each authenticated secure boot variable against the baseline template, if applicable, and reports telemetry based on the results and other secure boot configuration parameters