fix(preingestion): create unified BFB with 0o600 perms#3688
Open
spydaNVIDIA wants to merge 1 commit into
Open
Conversation
Contributor
WalkthroughThe BFB copier now builds unified pre-ingestion artifacts through a path-parameterized writer, creates them with ChangesUnified BFB assembly
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BFB_Copier
participant Base_BFB
participant Unified_BFB
BFB_Copier->>Unified_BFB: Create or truncate with mode 0o600
BFB_Copier->>Base_BFB: Read base BFB payload
Base_BFB-->>BFB_Copier: Return BFB bytes
BFB_Copier->>Unified_BFB: Write base payload and bf.cfg credentials
Unified_BFB-->>BFB_Copier: Flush assembled artifact
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
2 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/preingestion-manager/src/bfb_rshim_copier.rs`:
- Around line 185-203: Remove the destination-existence guard around the unified
BFB assembly so every invocation regenerates credentials for the current
machine. Update the write flow near the unified artifact creation and the
corresponding logic around the later referenced section to write complete
content to a fresh temporary file with mode 0o600, then atomically rename it
over the destination. Extend the relevant test to pre-populate stale credentials
and permissions, invoke the copier, and verify the destination is fully replaced
with current content and secure permissions.
- Around line 174-178: Update the artifact creation flow described near the
bf.cfg security comment to call set_permissions(0o600) on the opened file before
writing the payload, rather than relying on open’s .mode(0o600) alone. Add a
regression case using a restrictive umask and verify the resulting artifact
remains owner-readable and writable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c76aa5cd-db78-41d8-aceb-adb12f3be995
📒 Files selected for processing (1)
crates/preingestion-manager/src/bfb_rshim_copier.rs
Comment on lines
+174
to
+178
| /// The `bf.cfg` blob embeds the BMC root password in cleartext, so the | ||
| /// artifact is created with `0o600` (owner read/write only). The mode is | ||
| /// applied atomically by `open` at creation time rather than being left to | ||
| /// the process umask, so the password is never momentarily readable by other | ||
| /// local users/processes. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import os
import stat
import tempfile
path = tempfile.mktemp()
previous = os.umask(0o777)
try:
fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
os.close(fd)
finally:
os.umask(previous)
print(oct(stat.S_IMODE(os.stat(path).st_mode)))
os.unlink(path)
PYRepository: NVIDIA/infra-controller
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file before reading targeted sections.
ast-grep outline crates/preingestion-manager/src/bfb_rshim_copier.rs --view expanded || true
echo '--- lines 150-230 ---'
sed -n '150,230p' crates/preingestion-manager/src/bfb_rshim_copier.rs
echo '--- lines 290-340 ---'
sed -n '290,340p' crates/preingestion-manager/src/bfb_rshim_copier.rsRepository: NVIDIA/infra-controller
Length of output: 5797
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any other uses of the same file-creation pattern.
rg -n "set_permissions|OpenOptionsExt|mode\\(0o600\\)|umask|bf.cfg|rshim" crates/preingestion-manager/src/bfb_rshim_copier.rs crates/preingestion-manager/src -g '!**/target/**'Repository: NVIDIA/infra-controller
Length of output: 7300
Set the artifact mode explicitly after open. .mode(0o600) is still filtered by the process umask, so the file may not end up owner read/write (for example, it can land as 0o000 under a restrictive umask). Call set_permissions(0o600) on the opened file before writing the payload, and add a regression case with a restrictive umask.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/preingestion-manager/src/bfb_rshim_copier.rs` around lines 174 - 178,
Update the artifact creation flow described near the bf.cfg security comment to
call set_permissions(0o600) on the opened file before writing the payload,
rather than relying on open’s .mode(0o600) alone. Add a regression case using a
restrictive umask and verify the resulting artifact remains owner-readable and
writable.
Comment on lines
+185
to
+203
| if fs::metadata(unified_bfb_path).await.is_err() { | ||
| tracing::info!(path = unified_bfb_path, "Writing unified preingestion BFB"); | ||
| let bf_cfg_contents = format!( | ||
| "BMC_USER=\"{username}\"\nBMC_PASSWORD=\"{password}\"\nBMC_REBOOT=\"yes\"\nCEC_REBOOT=\"yes\"\n" | ||
| ); | ||
|
|
||
| let mut preingestion_bfb = File::open(PREINGESTION_BFB_PATH).await.map_err(|err| { | ||
| BfbRshimCopyError::Other { | ||
| details: format!("failed to open {PREINGESTION_BFB_PATH}: {err}"), | ||
| } | ||
| })?; | ||
|
|
||
| let mut unified_bfb = | ||
| File::create(UNIFIED_PREINGESTION_BFB_PATH) | ||
| let mut preingestion_bfb = | ||
| File::open(source_bfb_path) | ||
| .await | ||
| .map_err(|err| BfbRshimCopyError::Other { | ||
| details: format!("failed to create {UNIFIED_PREINGESTION_BFB_PATH}: {err}"), | ||
| details: format!("failed to open {source_bfb_path}: {err}"), | ||
| })?; | ||
|
|
||
| let mut unified_bfb = OpenOptions::new() | ||
| .write(true) | ||
| .create(true) | ||
| .truncate(true) | ||
| .mode(0o600) | ||
| .open(unified_bfb_path) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Always replace the destination; the existence check reuses stale credentials.
After the first call, Line 185 skips assembly entirely. Because callers provide machine-specific BMC credentials, subsequent machines receive the first artifact’s password; partial or insecure existing files are also accepted.
Atomically replace the artifact using a fresh 0o600 temporary file and rename it into place. Extend the test by pre-populating the destination with stale credentials and permissions, then verify complete replacement.
Also applies to: 290-330
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/preingestion-manager/src/bfb_rshim_copier.rs` around lines 185 - 203,
Remove the destination-existence guard around the unified BFB assembly so every
invocation regenerates credentials for the current machine. Update the write
flow near the unified artifact creation and the corresponding logic around the
later referenced section to write complete content to a fresh temporary file
with mode 0o600, then atomically rename it over the destination. Extend the
relevant test to pre-populate stale credentials and permissions, invoke the
copier, and verify the destination is fully replaced with current content and
secure permissions.
wminckler
reviewed
Jul 17, 2026
| path = UNIFIED_PREINGESTION_BFB_PATH, | ||
| "Writing bf.cfg payload" | ||
| ); | ||
| tracing::info!(path = unified_bfb_path, "Writing bf.cfg payload"); |
Contributor
There was a problem hiding this comment.
These logs seem kinda pointless in a production env since they don't have any machine info in them
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
2 participants
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.
The unified pre-ingestion BFB embeds the BMC root password in cleartext via its bf.cfg blob, but was created with File::create (0o666 & ~umask, typically 0644), leaving the secret world-readable to any local user/process.
Create the artifact with OpenOptions::mode(0o600) so permissions are set atomically at creation and independent of umask. Extract the assembly logic into a testable helper and add a unit test asserting the artifact is owner-only.
Related issues
#3060
Type of Change
Breaking Changes
Testing
Additional Notes