Skip to content

fix(preingestion): create unified BFB with 0o600 perms #3688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
spydaNVIDIA wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(preingestion): create unified BFB with 0o600 perms #3688

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 97 additions & 24 deletions crates/preingestion-manager/src/bfb_rshim_copier.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use std::sync::Arc;
use std::time::Duration;

use carbide_secrets::credentials::{CredentialKey, CredentialReader, Credentials};
use tokio::fs::{self, File};
use tokio::fs::{self, File, OpenOptions};
use tokio::io::{AsyncReadExt, AsyncWriteExt};
use tokio::sync::Mutex;

Expand Down Expand Up @@ -159,31 +159,56 @@ impl BfbRshimCopier {
) -> Result<(), BfbRshimCopyError> {
let _lock = self.bfb_file_lock.lock().await;

if fs::metadata(UNIFIED_PREINGESTION_BFB_PATH).await.is_err() {
tracing::info!(
path = UNIFIED_PREINGESTION_BFB_PATH,
"Writing unified preingestion BFB"
);
Self::write_unified_preingestion_bfb(
PREINGESTION_BFB_PATH,
UNIFIED_PREINGESTION_BFB_PATH,
username,
password,
)
.await
}

/// Assemble the unified pre-ingestion BFB by concatenating the base BFB with
/// a `bf.cfg` blob carrying the BMC credentials.
///
/// 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.
Comment on lines +174 to +178

@coderabbitai coderabbitai Bot Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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)
PY

Repository: 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.rs

Repository: 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.

async fn write_unified_preingestion_bfb(
source_bfb_path: &str,
unified_bfb_path: &str,
username: &str,
password: &str,
) -> Result<(), BfbRshimCopyError> {
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)
Comment on lines +185 to +203

@coderabbitai coderabbitai Bot Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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.

.await
.map_err(|err| BfbRshimCopyError::Other {
details: format!("failed to create {unified_bfb_path}: {err}"),
})?;

let mut buffer = vec![0; 1024 * 1024].into_boxed_slice(); // 1 MB buffer

tracing::info!(path = UNIFIED_PREINGESTION_BFB_PATH, "Writing BFB payload");
tracing::info!(path = unified_bfb_path, "Writing BFB payload");
loop {
let n = preingestion_bfb.read(&mut buffer).await.map_err(|err| {
BfbRshimCopyError::Other {
Expand All @@ -197,17 +222,12 @@ impl BfbRshimCopier {

unified_bfb.write_all(&buffer[..n]).await.map_err(|err| {
BfbRshimCopyError::Other {
details: format!(
"failed to write BFB to {UNIFIED_PREINGESTION_BFB_PATH}: {err}"
),
details: format!("failed to write BFB to {unified_bfb_path}: {err}"),
}
})?;
}

tracing::info!(
path = UNIFIED_PREINGESTION_BFB_PATH,
"Writing bf.cfg payload"
);
tracing::info!(path = unified_bfb_path, "Writing bf.cfg payload");

@wminckler wminckler Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These logs seem kinda pointless in a production env since they don't have any machine info in them


unified_bfb
.write_all(bf_cfg_contents.as_bytes())
Expand All @@ -220,7 +240,7 @@ impl BfbRshimCopier {
.sync_all()
.await
.map_err(|err| BfbRshimCopyError::Other {
details: format!("failed to flush {UNIFIED_PREINGESTION_BFB_PATH}: {err}"),
details: format!("failed to flush {unified_bfb_path}: {err}"),
})?;
}

Expand Down Expand Up @@ -257,3 +277,56 @@ impl BfbRshimCopier {
})
}
}

#[cfg(test)]
mod tests {
use std::os::unix::fs::PermissionsExt;

use super::*;

/// The unified BFB embeds the BMC root password in cleartext, so the
/// assembled artifact must be readable only by its owner (`0o600`),
/// independent of the process umask.
#[tokio::test]
async fn write_unified_preingestion_bfb_creates_owner_only_artifact() {
let dir = tempfile::tempdir().expect("create tempdir");
let source_path = dir.path().join("preingestion.bfb");
let unified_path = dir.path().join("preingestion_unified_update.bfb");

let base_payload = b"base-bfb-payload";
fs::write(&source_path, base_payload)
.await
.expect("write source bfb");

BfbRshimCopier::write_unified_preingestion_bfb(
source_path.to_str().expect("utf-8 source path"),
unified_path.to_str().expect("utf-8 unified path"),
"root",
"s3cr3t-p@ssw0rd",
)
.await
.expect("write unified bfb");

let mode = fs::metadata(&unified_path)
.await
.expect("stat unified bfb")
.permissions()
.mode();
assert_eq!(
mode & 0o777,
0o600,
"unified BFB must be owner-only, got {:o}",
mode & 0o777
);

// The base BFB is concatenated with the bf.cfg blob carrying the creds.
let written = fs::read(&unified_path).await.expect("read unified bfb");
assert!(
written.starts_with(base_payload),
"unified BFB must start with the base BFB payload"
);
let bf_cfg = String::from_utf8_lossy(&written[base_payload.len()..]);
assert!(bf_cfg.contains("BMC_USER=\"root\""));
assert!(bf_cfg.contains("BMC_PASSWORD=\"s3cr3t-p@ssw0rd\""));
}
}
Loading