fix push-mode: cache UEFI event log bytes at startup to avoid post-privilege-drop failures#1223
Open
phaer wants to merge 2 commits intokeylime:masterfrom
Open
fix push-mode: cache UEFI event log bytes at startup to avoid post-privilege-drop failures#1223phaer wants to merge 2 commits intokeylime:masterfrom
phaer wants to merge 2 commits intokeylime:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds an optional in-memory cache for measured-boot (UEFI) log bytes in Changes
Sequence DiagramsequenceDiagram
participant Startup as Startup
participant PR as PrivilegedResources
participant FS as FileSystem
participant Cache as In-Memory Cache
participant SF as StructFiller
participant CI as ContextInfo
Startup->>PR: PrivilegedResources::new(measuredboot_ml_path)
PR->>FS: read(measuredboot_ml_path)
alt read ok
FS-->>PR: bytes
PR->>Cache: store bytes
else read error / missing
FS-->>PR: error / not found
PR->>Cache: set None
end
SF->>CI: collect_evidences(..., cached_uefi_bytes)
CI->>CI: uefi_log_content(log_path, cached_uefi_bytes)
alt cached_uefi_bytes present
CI-->>CI: base64(cached bytes)
else cached absent, log_path exists
CI->>FS: read log_path
FS-->>CI: base64(file bytes)
else neither
CI-->>CI: return empty string
end
CI-->>SF: EvidenceData (UEFI log)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
generate_uefi_log_evidence() re-opened the securityfs event log by path, failing with PermissionDenied after run_as drops privileges. Cache the raw bytes in PrivilegedResources at startup and pass them through collect_evidences(). The UEFI event log only contains boot-time events (up to ExitBootServices) so the cache is always valid. Signed-off-by: phaer <phaer@phaer.org>
privileged_resources: extend the three existing tests to also assert measuredboot_ml_bytes is None/Some with the correct content. context_info: extract uefi_log_content() as a TPM-free helper and add three unit tests: - test_cache_preferred_over_file: RED without the commit (reads file, gets different content than the cache) - test_cache_used_when_path_nonexistent: RED without the commit (tries to open a nonexistent file, returns an error) - test_file_fallback: regression test confirming the file path still works when no cache is provided Signed-off-by: phaer <phaer@phaer.org>
phaer
force-pushed
the
fix/uefi-log-privileged-fd
branch
from
b9ebc6b to
f0cb81e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keylime-push-model-agent/src/privileged_resources.rs`:
- Around line 147-161: If the measured-boot file exists but std::fs::read()
fails, do not silently set measuredboot_ml_bytes to None while leaving
measuredboot_ml_file present; instead make the cached bytes mandatory for the
push-model path by treating a read failure as an error: when
measuredboot_ml_path.exists() is true and reading it fails, return an Err (or
otherwise abort initialization) so struct_filler never forwards a None cache
into collect_evidences(); alternatively, if you prefer to suppress UEFI
evidence, ensure you clear measuredboot_ml_file when the read fails so
struct_filler/collect_evidences() will not attempt to reopen the log later. Use
the symbols measuredboot_ml_path, measuredboot_ml_bytes, measuredboot_ml_file,
struct_filler, and collect_evidences to locate and implement the change.
In `@keylime/src/context_info.rs`:
- Around line 790-839: Add a regression test that exercises the full plumbing by
calling collect_evidences() (the higher-level path used in production) with a
nonexistent log_path and a populated cached_uefi_bytes buffer and asserting the
returned evidence includes the base64-encoded UEFI log; specifically, create a
test that supplies Some("/nonexistent/uefi_log.bin") and Some(cache_bytes) to
collect_evidences (or the public entry that invokes struct_filler ->
uefi_log_content) and verify the resulting Evidences/struct contains the
expected base64 string so the test fails if cached_uefi_bytes are not forwarded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: db282b2f-ffd7-4bec-ae90-fe411b08f5d3
📒 Files selected for processing (6)
keylime-push-model-agent/src/attestation.rskeylime-push-model-agent/src/main.rskeylime-push-model-agent/src/privileged_resources.rskeylime-push-model-agent/src/state_machine.rskeylime-push-model-agent/src/struct_filler.rskeylime/src/context_info.rs
There was a problem hiding this comment.
♻️ Duplicate comments (2)
keylime/src/context_info.rs (1)
790-839: 🧹 Nitpick | 🔵 TrivialAdd one regression that exercises the
collect_evidences()cache plumbing.These tests only cover
uefi_log_content(). They still pass ifkeylime::context_info::ContextInfo::collect_evidences()stops passingcached_uefi_bytesintogenerate_uefi_log_evidence(). Please add one higher-level test with a nonexistentlog_pathand a populated cache that asserts the returnedEvidenceData::UefiLogcontains the cached base64 payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keylime/src/context_info.rs` around lines 790 - 839, Add a higher-level test that exercises ContextInfo::collect_evidences to ensure cached UEFI bytes are passed through to generate_uefi_log_evidence: create a ContextInfo (or builder) with log_path set to a nonexistent path and cached_uefi_bytes populated, call collect_evidences(), find the EvidenceData::UefiLog entry in the returned evidences, base64-encode the same cached bytes using base64::engine::general_purpose::STANDARD and assert the EvidenceData::UefiLog payload equals that encoded string; this will validate collect_evidences() actually forwards cached_uefi_bytes to generate_uefi_log_evidence rather than relying on on-disk reads.keylime-push-model-agent/src/privileged_resources.rs (1)
147-157:⚠️ Potential issue | 🟠 MajorMake the UEFI byte cache mandatory once the log is available.
If
std::fs::read()fails here,measuredboot_ml_filecan stay populated whilemeasuredboot_ml_bytesbecomesNone.keylime-push-model-agent/src/struct_filler.rs::FillerFromHardware::get_evidence_handling_request_finalonly forwardsmeasuredboot_ml_bytesintokeylime::context_info::ContextInfo::collect_evidences(), andkeylime/src/context_info.rs::uefi_log_content()then falls back to reopeninglog_path. That revives the same post-run_asPermissionDeniedpath this PR is meant to remove. Please either fail initialization here or suppress UEFI evidence by clearingmeasuredboot_ml_filetoo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keylime-push-model-agent/src/privileged_resources.rs` around lines 147 - 157, The cached UEFI bytes are being optionalized on read failure while the associated measuredboot_ml_file remains set, which allows later code paths (FillerFromHardware::get_evidence_handling_request_final -> ContextInfo::collect_evidences -> uefi_log_content) to attempt reopening the file with insufficient permissions; fix by making the UEFI cache mandatory or disabling the file reference on read error: either propagate an initialization error when std::fs::read(measuredboot_ml_path) fails (failing startup) or, if you prefer to suppress UEFI evidence, clear measuredboot_ml_file (set it to None) whenever measuredboot_ml_bytes is None so both the bytes and file handle stay consistent; update the code around measuredboot_ml_bytes and measuredboot_ml_file to enforce that invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@keylime-push-model-agent/src/privileged_resources.rs`:
- Around line 147-157: The cached UEFI bytes are being optionalized on read
failure while the associated measuredboot_ml_file remains set, which allows
later code paths (FillerFromHardware::get_evidence_handling_request_final ->
ContextInfo::collect_evidences -> uefi_log_content) to attempt reopening the
file with insufficient permissions; fix by making the UEFI cache mandatory or
disabling the file reference on read error: either propagate an initialization
error when std::fs::read(measuredboot_ml_path) fails (failing startup) or, if
you prefer to suppress UEFI evidence, clear measuredboot_ml_file (set it to
None) whenever measuredboot_ml_bytes is None so both the bytes and file handle
stay consistent; update the code around measuredboot_ml_bytes and
measuredboot_ml_file to enforce that invariant.
In `@keylime/src/context_info.rs`:
- Around line 790-839: Add a higher-level test that exercises
ContextInfo::collect_evidences to ensure cached UEFI bytes are passed through to
generate_uefi_log_evidence: create a ContextInfo (or builder) with log_path set
to a nonexistent path and cached_uefi_bytes populated, call collect_evidences(),
find the EvidenceData::UefiLog entry in the returned evidences, base64-encode
the same cached bytes using base64::engine::general_purpose::STANDARD and assert
the EvidenceData::UefiLog payload equals that encoded string; this will validate
collect_evidences() actually forwards cached_uefi_bytes to
generate_uefi_log_evidence rather than relying on on-disk reads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: de7056c9-f099-4821-ab01-949d75939ee7
📒 Files selected for processing (6)
keylime-push-model-agent/src/attestation.rskeylime-push-model-agent/src/main.rskeylime-push-model-agent/src/privileged_resources.rskeylime-push-model-agent/src/state_machine.rskeylime-push-model-agent/src/struct_filler.rskeylime/src/context_info.rs
Codecov Report❌ Patch coverage is Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
phaer
added a commit
to phaer/nixos-android-builder
that referenced
this pull request
Apr 13, 2026
Link each local patch to its upstream PR (or tracking issue) so the provenance of each is obvious from the package files alone: - 0001 elparsing → keylime/keylime#1878 - 0002 tpm-relevant-pcrs → keylime/keylime#1879 - 0003/0004 dual-mapping → keylime/keylime#1880 (issue, no PR yet) - keylime-agent 0001 → keylime/rust-keylime#1223
phaer
added a commit
to phaer/nixos-android-builder
that referenced
this pull request
Apr 13, 2026
Link each local patch to its upstream PR (or tracking issue) so the provenance of each is obvious from the package files alone: - 0001 elparsing → keylime/keylime#1878 - 0002 tpm-relevant-pcrs → keylime/keylime#1879 - 0003/0004 dual-mapping → keylime/keylime#1880 (issue, no PR yet) - keylime-agent 0001 → keylime/rust-keylime#1223
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
generate_uefi_log_evidence() was re-opening the securityfs UEFI event log by path on each attestation, which fails with PermissionDenied after run_as drops privileges in the push-model agent.
So we read and cache the raw event log bytes at startup, while still privileged. Since the UEFI event log only contains boot-time events (up to ExitBootServices), the cached bytes should stay valid for as long as the agent runs (and longer; until reboot).
Summary by CodeRabbit
New Features
Tests