feat: rotate BF4 service password#3858
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Summary by CodeRabbit
WalkthroughAdds site-wide BF4 DPU service credential storage and retrieval, Redfish service-account password rotation, BF4 product detection, and endpoint exploration integration during missing per-BMC credential recovery. ChangesBF4 service credential rotation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BmcEndpointExplorer
participant RedfishClient
participant CredentialClient
participant RedfishClientPool
BmcEndpointExplorer->>RedfishClient: Query Redfish product
RedfishClient-->>BmcEndpointExplorer: Return product
BmcEndpointExplorer->>CredentialClient: Get or create BF4 service password
CredentialClient-->>BmcEndpointExplorer: Return password
BmcEndpointExplorer->>RedfishClient: Rotate service password
RedfishClient->>RedfishClientPool: Change service account password
RedfishClientPool-->>RedfishClient: Return rotation result
RedfishClient-->>BmcEndpointExplorer: Return rotation result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/site-explorer/src/bmc_endpoint_explorer.rs (1)
826-900: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRetry BF4 service rotation after partial ingest failures.
The root credential is persisted before Lines 889-892 run. If service rotation fails, the next exploration takes the existing per-BMC credential path at Line 749 and never retries it, leaving the factory
servicepassword active indefinitely. Existing BF4 devices also bypass this migration. Move the BF4 detection/rotation into a recoverable path after root credentials are obtained, or persist explicit per-device completion state; add a regression test for retry after a failed rotation.🤖 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/site-explorer/src/bmc_endpoint_explorer.rs` around lines 826 - 900, The BF4 service-password rotation in the exploration flow must be retried when a prior attempt failed, including for existing devices that already have persisted root credentials. Update the path around set_sitewide_bmc_root_password, try_sitewide_bmc_root_credentials, and rotate_bf4_dpu_service_password so BF4 detection and rotation run after either credential source and remain recoverable on subsequent explorations; add a regression test covering a failed rotation followed by a retry.
🤖 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/site-explorer/src/credentials.rs`:
- Around line 156-168: Update the create-if-missing branch in the credentials
lookup flow around get_credentials and set_credentials to use an atomic
create/CAS operation, preventing concurrent writers from overwriting one
another. When creation reports an already-exists conflict, re-read the
credentials and return the stored password instead of the newly generated value;
preserve the existing return path for an actual successful create. Add a
concurrent regression test covering two callers racing to create the same
site-wide password.
---
Outside diff comments:
In `@crates/site-explorer/src/bmc_endpoint_explorer.rs`:
- Around line 826-900: The BF4 service-password rotation in the exploration flow
must be retried when a prior attempt failed, including for existing devices that
already have persisted root credentials. Update the path around
set_sitewide_bmc_root_password, try_sitewide_bmc_root_credentials, and
rotate_bf4_dpu_service_password so BF4 detection and rotation run after either
credential source and remain recoverable on subsequent explorations; add a
regression test covering a failed rotation followed by a retry.
🪄 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: 53f972ff-1c7b-43cd-8439-a587f1aaebdd
📒 Files selected for processing (7)
crates/bmc-explorer/src/lib.rscrates/redfish/src/libredfish/mod.rscrates/redfish/src/libredfish/test_support.rscrates/secrets/src/credentials.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/credentials.rscrates/site-explorer/src/redfish.rs
| match self.get_credentials(&key).await { | ||
| Ok(Credentials::UsernamePassword { password, .. }) => Ok(password), | ||
| Err(EndpointExplorationError::MissingCredentials { .. }) if create_if_missing => { | ||
| let password = Credentials::generate_password(); | ||
| self.set_credentials( | ||
| &key, | ||
| &Credentials::UsernamePassword { | ||
| username: "service".to_string(), | ||
| password: password.clone(), | ||
| }, | ||
| ) | ||
| .await?; | ||
| Ok(password) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make site-wide password creation atomic.
Concurrent BF4 ingests can both observe a missing key, generate different passwords, and overwrite each other through set_credentials. Each BMC may then receive a different password while Vault retains only the last write. Use an atomic create/CAS operation; on an already-exists conflict, re-read and return the stored password. Add a concurrent regression test.
🤖 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/site-explorer/src/credentials.rs` around lines 156 - 168, Update the
create-if-missing branch in the credentials lookup flow around get_credentials
and set_credentials to use an atomic create/CAS operation, preventing concurrent
writers from overwriting one another. When creation reports an already-exists
conflict, re-read the credentials and return the stored password instead of the
newly generated value; preserve the existing return path for an actual
successful create. Add a concurrent regression test covering two callers racing
to create the same site-wide password.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/site-explorer/src/bmc_endpoint_explorer.rs (2)
891-902: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRetry BF4 service rotation after partial first-ingest failures.
Root credentials are stored before service rotation. If rotation fails, the next run takes the existing-credentials path and generates a report without retrying it, leaving the device and Vault with mismatched service passwords. Track rotation completion or retry BF4 rotation in that path before reporting success.
🤖 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/site-explorer/src/bmc_endpoint_explorer.rs` around lines 891 - 902, Update the BF4 handling around generate_exploration_report to retry rotate_bf4_dpu_service_password when existing credentials indicate rotation was not completed, including runs following a partial first-ingest failure. Track or detect rotation completion before generating the report, while preserving the existing behavior for successfully rotated devices.
280-285: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake BF4 service-password creation atomic.
Concurrent first ingests can both observe a missing credential, generate different passwords, and program different devices before Vault retains only the last write. Use an atomic create-if-absent operation or shared lock, and return the committed value before updating any device.
🤖 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/site-explorer/src/bmc_endpoint_explorer.rs` around lines 280 - 285, The BF4 service-password flow around get_sitewide_bf4_dpu_service_password and set_bf4_dpu_service_password is vulnerable to concurrent first-ingest races. Make password creation atomic using an atomic create-if-absent operation or shared lock, return the committed password value, and only then program devices so every concurrent caller uses the same retained credential.
🤖 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.
Outside diff comments:
In `@crates/site-explorer/src/bmc_endpoint_explorer.rs`:
- Around line 891-902: Update the BF4 handling around
generate_exploration_report to retry rotate_bf4_dpu_service_password when
existing credentials indicate rotation was not completed, including runs
following a partial first-ingest failure. Track or detect rotation completion
before generating the report, while preserving the existing behavior for
successfully rotated devices.
- Around line 280-285: The BF4 service-password flow around
get_sitewide_bf4_dpu_service_password and set_bf4_dpu_service_password is
vulnerable to concurrent first-ingest races. Make password creation atomic using
an atomic create-if-absent operation or shared lock, return the committed
password value, and only then program devices so every concurrent caller uses
the same retained credential.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: de80766c-857f-463f-bdf2-3ea466f09717
📒 Files selected for processing (1)
crates/site-explorer/src/bmc_endpoint_explorer.rs
|
cc @spydaNVIDIA who is working other bmc creds changes |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/site-explorer/src/bmc_endpoint_explorer.rs (2)
862-897: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep BF4 service rotation retryable after root-credential persistence.
set_sitewide_bmc_root_passwordpersists the per-BMC root credentials before this rotation runs. Ifrotate_bf4_dpu_service_passwordfails, the next ingest takes the existing-credentials branch around Lines 746-803 and never re-enters BF4 detection or service rotation. The device can remain on its factory service password indefinitely. Run BF4 service convergence from an idempotent path that also handles already-persisted root credentials, or make the credential updates transactional.🤖 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/site-explorer/src/bmc_endpoint_explorer.rs` around lines 862 - 897, Ensure BF4 service-password rotation remains retryable after set_sitewide_bmc_root_password persists credentials. Update the existing-credentials ingest path, including the flow around try_sitewide_bmc_root_credentials and rotate_bf4_dpu_service_password, so already-persisted root credentials still trigger BF4 detection and service rotation on later ingests; preserve idempotent behavior and propagate rotation failures for future retries.
275-284: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake BF4 service-password creation atomic.
create_if_missing = truereaches a get-then-set implementation incrates/site-explorer/src/credentials.rs:148-172. Concurrent first ingests can therefore generate different passwords, overwrite the site-wide Vault entry, and leave one device configured with a password that is no longer stored. Use an atomic create-if-absent operation or a shared lock, then rotate devices with the winning stored value.🤖 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/site-explorer/src/bmc_endpoint_explorer.rs` around lines 275 - 284, Update rotate_bf4_dpu_service_password and the underlying get_sitewide_bf4_dpu_service_password create-if-missing flow to use an atomic create-if-absent operation or shared lock. Ensure concurrent first ingests converge on one winning site-wide Vault password, and configure each device with the value actually stored rather than a superseded generated value.
🤖 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.
Outside diff comments:
In `@crates/site-explorer/src/bmc_endpoint_explorer.rs`:
- Around line 862-897: Ensure BF4 service-password rotation remains retryable
after set_sitewide_bmc_root_password persists credentials. Update the
existing-credentials ingest path, including the flow around
try_sitewide_bmc_root_credentials and rotate_bf4_dpu_service_password, so
already-persisted root credentials still trigger BF4 detection and service
rotation on later ingests; preserve idempotent behavior and propagate rotation
failures for future retries.
- Around line 275-284: Update rotate_bf4_dpu_service_password and the underlying
get_sitewide_bf4_dpu_service_password create-if-missing flow to use an atomic
create-if-absent operation or shared lock. Ensure concurrent first ingests
converge on one winning site-wide Vault password, and configure each device with
the value actually stored rather than a superseded generated value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b46bb346-b2a9-4e40-aada-8777f5eb0547
📒 Files selected for processing (4)
crates/redfish/src/libredfish/test_support.rscrates/secrets/src/credentials.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/credentials.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/redfish/src/libredfish/test_support.rs
- crates/secrets/src/credentials.rs
- crates/site-explorer/src/credentials.rs
| /// Site-wide BF4 DPU BMC `service` account password | ||
| /// (`machines/bmc/site/bf4_service`). Written on first BF4 DPU ingestion; | ||
| /// distinct from the site-wide BMC root password. | ||
| SiteWideBf4Service, |
There was a problem hiding this comment.
Is it worth calling this SitewideBmcServiceCredentials ? I imagine that the BFs going forward will have a service account as well?
There was a problem hiding this comment.
The information that I have is that this is only for BF4. BF3s will continue to have one root account for both ssh and redfish. Let me confirm with someone
| BmcCredentialType::BmcForgeAdmin { bmc_mac_address } => Cow::from(format!( | ||
| "machines/bmc/{bmc_mac_address}/forge-admin-account" | ||
| )), | ||
| BmcCredentialType::SiteWideBf4Service => Cow::from("machines/bmc/site/bf4_service"), |
There was a problem hiding this comment.
similarly, maybe the path should be machines/bmc/site/service
| .map_err(|err| redact_password(err, new_password.as_str())) | ||
| .map_err(|err| redact_password(err, root_password.as_str())) | ||
| .map_err(RedfishClientCreationError::RedfishError)?; | ||
|
|
There was a problem hiding this comment.
Today, when we update the factory credentials of a DPU BMC, we add an entry per BMC mac into vault: machines/secret/bmc/DPU_BMC_MAC/root.
Similarly, I think we should add an entry per service account we configure (one for every BF4): machines/secrets/bmc/DPU_BMC_MAC/service
There was a problem hiding this comment.
This should help us down the line: it enables operators to configure custom passwords (in some edge cases we have had to do that in the past) and will make our lives easier when we want to support rotation of this credential.
| async fn rotate_bf4_dpu_service_password( | ||
| &self, | ||
| bmc_ip_address: SocketAddr, | ||
| root_credentials: &Credentials, |
There was a problem hiding this comment.
nit: rename root_credentials to factory_service_credentials
| }) | ||
| } | ||
|
|
||
| async fn rotate_bf4_dpu_service_password( |
There was a problem hiding this comment.
nit: function rename to rotate_dpu_service_password_from_factory_defaults may reflect the semantics of this function better
| Err(EndpointExplorationError::MissingCredentials { .. }) => { | ||
| // No per-BMC vault entry exists. Now try to: | ||
| // 1) Login with expected/factory credentials | ||
| // 2) Rotate the BMC root password to the sitewide root password |
There was a problem hiding this comment.
should we update this comment to reflect that we are also rotating the BMC service password on BF4s (and indicating that this account is required for SSH access to BF4 BMCs)
| }; | ||
|
|
||
| if is_bf4_dpu { | ||
| self.rotate_bf4_dpu_service_password(bmc_ip_address, &bmc_credentials) |
There was a problem hiding this comment.
Why are we passing in the bmc_credentials as the factory defaults?
| } | ||
|
|
||
| /// Rotate a BF4 DPU BMC's `service` account password using root credentials. | ||
| async fn set_bf4_dpu_service_password( |
There was a problem hiding this comment.
Worth adding an API/CLI endpoint to test configuring the service account's password?
3 participants
Description: NICo already ingests BF3 DPUs by logging in as root, rotating root to the site-wide BMC root password, and storing root credentials per BMC in Vault. BF4 needs the same root path for Redfish, plus a separate step to rotate the service account used for SSH.
What this PR does
On first ingest of a BF4 DPU BMC (no per-BMC Vault entry yet):
Detect BF4 via anonymous Redfish ServiceRoot.Product (B4240V / BlueField-4) using bmc_explorer::is_bf4_product.
Ingest with root — same as BF3: factory root:0penBmc → rotate to site-wide BMC root password → store root creds at machines/bmc/{mac}/root.
Rotate service account — while authenticated as root, call libredfish change_password_by_id("service", …) to move off factory Nvidia_12345!.
Store site-wide service password — a dedicated password (distinct from sitewide root) is created once at machines/bmc/site/bf4_service and reused for all BF4 DPUs at the site.
Subsequent Redfish exploration continues to use root credentials from the per-BMC Vault path. The rotated service password is stored sitewide for SSH use; it is not used for Redfish exploration today.
Type of Change
Testing