Skip to content

refactor(redfish): move BMC set_bmc_root_password + add probe_bmc_vendor to RedfishClientPool for rotation reuse#3113

Merged
spydaNVIDIA merged 4 commits into
NVIDIA:mainfrom
spydaNVIDIA:rotate_creds
Jul 7, 2026
Merged

refactor(redfish): move BMC set_bmc_root_password + add probe_bmc_vendor to RedfishClientPool for rotation reuse#3113
spydaNVIDIA merged 4 commits into
NVIDIA:mainfrom
spydaNVIDIA:rotate_creds

Conversation

@spydaNVIDIA

Copy link
Copy Markdown
Contributor

refactor(redfish): move BMC set_bmc_root_password + add probe_bmc_vendor to RedfishClientPool for rotation reuse

Related issues

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Additional Notes

@spydaNVIDIA spydaNVIDIA requested review from a team and polarweasel as code owners July 2, 2026 23:21
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c106b3aa-c06c-40b5-963d-6290d70c86c1

📥 Commits

Reviewing files that changed from the base of the PR and between 960c30c and 62ea70f.

⛔ Files ignored due to path filters (5)
  • rest-api/flow/internal/nicoapi/gen/nico.pb.go is excluded by !**/*.pb.go, !**/gen/**, !rest-api/**/*.pb.go
  • rest-api/flow/internal/nicoapi/gen/nico_grpc.pb.go is excluded by !**/*.pb.go, !**/gen/**, !rest-api/**/*.pb.go, !rest-api/**/*_grpc.pb.go
  • rest-api/workflow-schema/schema/site-agent/workflows/v1/nico_nico.pb.go is excluded by !**/*.pb.go, !rest-api/**/*.pb.go
  • rest-api/workflow-schema/schema/site-agent/workflows/v1/nico_nico_grpc.pb.go is excluded by !**/*.pb.go, !rest-api/**/*.pb.go, !rest-api/**/*_grpc.pb.go
  • rest-api/workflow-schema/site-agent/workflows/v1/nico_nico.proto is excluded by !rest-api/workflow-schema/site-agent/workflows/v1/*_nico.proto
📒 Files selected for processing (22)
  • crates/admin-cli/src/bmc_machine/mod.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/args.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/cmd.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/mod.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/args.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/cmd.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/mod.rs
  • crates/api-core/src/api.rs
  • crates/api-core/src/auth/internal_rbac_rules.rs
  • crates/api-core/src/handlers/bmc_endpoint_explorer.rs
  • crates/redfish/src/libredfish/mod.rs
  • crates/redfish/src/libredfish/test_support.rs
  • crates/rpc/proto/forge.proto
  • crates/secrets/src/credentials.rs
  • crates/site-explorer/src/bmc_endpoint_explorer.rs
  • crates/site-explorer/src/endpoint_explorer.rs
  • crates/site-explorer/src/redfish.rs
  • crates/site-explorer/src/test_support/mock_endpoint_explorer.rs
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-probe-vendor.md
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-set-root-password.md
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine.md
  • rest-api/flow/internal/nicoapi/nicoproto/nico.proto
✅ Files skipped from review due to trivial changes (3)
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine.md
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-set-root-password.md
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-probe-vendor.md
🚧 Files skipped from review as they are similar to previous changes (19)
  • crates/admin-cli/src/bmc_machine/probe_vendor/cmd.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/mod.rs
  • crates/api-core/src/auth/internal_rbac_rules.rs
  • crates/site-explorer/src/test_support/mock_endpoint_explorer.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/mod.rs
  • crates/secrets/src/credentials.rs
  • crates/api-core/src/api.rs
  • crates/site-explorer/src/endpoint_explorer.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/args.rs
  • crates/admin-cli/src/bmc_machine/mod.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/cmd.rs
  • rest-api/flow/internal/nicoapi/nicoproto/nico.proto
  • crates/rpc/proto/forge.proto
  • crates/site-explorer/src/bmc_endpoint_explorer.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/args.rs
  • crates/site-explorer/src/redfish.rs
  • crates/api-core/src/handlers/bmc_endpoint_explorer.rs
  • crates/redfish/src/libredfish/test_support.rs
  • crates/redfish/src/libredfish/mod.rs

Summary by CodeRabbit

  • New Features
    • Added a command to set a BMC root password remotely.
    • Added a command to detect a BMC’s Redfish vendor.
  • Documentation
    • Updated the BMC machine command reference with the new subcommands and usage details.
  • Bug Fixes
    • Improved BMC handling to better resolve vendor-specific behavior and credential updates during management tasks.

Walkthrough

This PR adds BMC root-password setting and vendor probing end to end: proto and API contracts, Redfish and site-explorer plumbing, admin CLI subcommands, RBAC, and command documentation.

Changes

BMC Root Password & Vendor Probing

Layer / File(s) Summary
Proto contract and permissions
crates/rpc/proto/forge.proto, rest-api/flow/internal/nicoapi/nicoproto/nico.proto, crates/api-core/src/auth/internal_rbac_rules.rs
Adds SetBmcRootPassword and ProbeBmcVendor RPCs with their request/response messages and RBAC permissions.
Redfish vendor probing and password rotation
crates/redfish/src/libredfish/mod.rs, crates/redfish/src/libredfish/test_support.rs, crates/secrets/src/credentials.rs
Implements Redfish client-pool helpers, extends the simulator for vendor/password fallback behavior, and adds coverage for vendor dispatch and probing.
Site-explorer and handler wiring
crates/site-explorer/src/endpoint_explorer.rs, crates/site-explorer/src/redfish.rs, crates/site-explorer/src/bmc_endpoint_explorer.rs, crates/site-explorer/src/test_support/mock_endpoint_explorer.rs, crates/api-core/src/api.rs, crates/api-core/src/handlers/bmc_endpoint_explorer.rs
Extends the explorer interface, delegates Redfish operations, loads and persists per-BMC credentials, and adds API handlers that resolve requests and call the explorer.
Admin CLI commands
crates/admin-cli/src/bmc_machine/mod.rs, crates/admin-cli/src/bmc_machine/probe_vendor/*, crates/admin-cli/src/bmc_machine/set_root_password/*
Adds the bmc-machine command variants, argument parsing, request conversion, and command execution entrypoints.
Command documentation
docs/manuals/nico-admin-cli/commands/bmc-machine/*
Adds manual pages for the new subcommands and updates the subcommand index.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AdminCli
  participant Api
  participant BmcEndpointExplorer
  participant Vault
  participant RedfishClient

  AdminCli->>Api: SetBmcRootPassword(request)
  Api->>BmcEndpointExplorer: set_bmc_root_password(address, interface, new_password)
  BmcEndpointExplorer->>Vault: load current root credentials
  Vault-->>BmcEndpointExplorer: credentials
  BmcEndpointExplorer->>RedfishClient: probe_bmc_vendor(address, credentials)
  RedfishClient-->>BmcEndpointExplorer: vendor
  BmcEndpointExplorer->>RedfishClient: set_bmc_root_password(address, vendor, credentials, new_password)
  RedfishClient-->>BmcEndpointExplorer: Ok(())
  BmcEndpointExplorer->>Vault: persist new credentials
  BmcEndpointExplorer-->>Api: Ok(())
  Api-->>AdminCli: SetBmcRootPasswordResponse
Loading

Compact metadata

  • Related issues: Not specified in the provided context.
  • Related PRs: Not specified in the provided context.
  • Suggested labels: feature, bmc, redfish, admin-cli, documentation
  • Suggested reviewers: Not specified in the provided context.

Poem

A root password changes, vendor in sight,
The CLI speaks, and the API routes it right.
Redfish probes, vault stores the key,
Then docs describe the flow succinctly.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately captures the main refactor and the new probe_vendor helper in RedfishClientPool.
Description check ✅ Passed The description is clearly related to the Redfish refactor and vendor-probing work, even though much of it is template text.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-07-02 23:24:26 UTC | Commit: 4504c6d

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 7

🧹 Nitpick comments (4)
crates/admin-cli/src/bmc_machine/set_root_password/args.rs (1)

51-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate bmc_endpoint_request construction logic.

This From<Args> body is byte-for-byte identical (modulo the extra new_password field) to the one in crates/admin-cli/src/bmc_machine/probe_vendor/args.rs (lines 45-61). Consider extracting a shared helper (e.g. fn bmc_endpoint_request_from(ip: Option<String>, mac: Option<MacAddress>) -> Option<forgerpc::BmcEndpointRequest>) in a common module for both commands. This also gives a single place to fix the MAC-only bug noted above instead of two.

🤖 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/admin-cli/src/bmc_machine/set_root_password/args.rs` around lines 51 -
68, The `From<Args>` implementation for `SetBmcRootPasswordRequest` duplicates
the same `bmc_endpoint_request` building logic used by
`probe_vendor::args::From<Args>`, so factor that shared logic into a common
helper and reuse it from both command conversions. Introduce a reusable function
for constructing `forgerpc::BmcEndpointRequest` from the IP/MAC inputs, then
call it from `Args::from` here and in the probe vendor args conversion, keeping
`machine_id` and `new_password` handling local to `SetBmcRootPasswordRequest`.
docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-probe-vendor.md (1)

54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the generic BMC IP placeholder for --ip-address.

This flag targets the BMC's own IP (not a host-targeted flag), so it should use 192.0.2.10 rather than 192.0.2.20, which is reserved for host-targeted commands such as mlx-devices --host.

📝 Proposed fix
-nico-admin-cli bmc-machine probe-vendor --ip-address 192.0.2.20
+nico-admin-cli bmc-machine probe-vendor --ip-address 192.0.2.10

Based on learnings, "Use 192.0.2.10 as the canonical placeholder for the DPU/generic BMC IP address" while "192.0.2.20" is reserved for host-targeted flags.

🤖 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 `@docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-probe-vendor.md`
at line 54, Update the example for nico-admin-cli bmc-machine probe-vendor so
the --ip-address placeholder uses the generic BMC/DPU IP value 192.0.2.10
instead of 192.0.2.20. The issue is in the probe-vendor command example, and the
fix is to replace the host-targeted placeholder with the canonical BMC IP
placeholder consistently in this document.

Source: Learnings

docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-set-root-password.md (1)

60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Same BMC IP placeholder inconsistency as probe-vendor.md.

--ip-address targets the BMC directly here too, so 192.0.2.10 (generic/DPU BMC IP) should be used instead of 192.0.2.20 (reserved for host-targeted commands).

📝 Proposed fix
-nico-admin-cli bmc-machine set-root-password --ip-address 192.0.2.20 --new-password mynewpassword
+nico-admin-cli bmc-machine set-root-password --ip-address 192.0.2.10 --new-password mynewpassword

Based on learnings, "Use 192.0.2.10 as the canonical placeholder for the DPU/generic BMC IP address" while "192.0.2.20" is the host BMC placeholder.

🤖 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
`@docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-set-root-password.md`
at line 60, The BMC IP placeholder in the set-root-password example is
inconsistent with the canonical BMC-targeted placeholder. Update the example
under the set-root-password command so it uses the generic/DPU BMC address
placeholder 192.0.2.10 for the --ip-address argument, matching the convention
used for direct BMC commands. Keep the command text otherwise unchanged and
ensure this aligns with the placeholder usage in the related BMC docs, including
the set-root-password example.

Source: Learnings

crates/api-core/src/handlers/bmc_endpoint_explorer.rs (1)

988-1055: 📐 Maintainability & Code Quality | 🔵 Trivial

Consider adding table-driven tests for the new request→response adapters.

No tests are visible for set_bmc_root_password or probe_bmc_vendor in this diff. Both are straightforward request-validation/adapter functions well-suited to table-driven coverage of the various validate_and_complete_bmc_endpoint_request / resolve_bmc_interface failure paths (missing machine, IP/MAC mismatch, missing BMC IP, etc.).

As per path instructions, "prefer table-driven tests for functions that map inputs to outputs/errors (useful for request->response adapters introduced by the RPC contract)."

🤖 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/api-core/src/handlers/bmc_endpoint_explorer.rs` around lines 988 -
1055, Add table-driven tests for the new request-to-response adapter handlers
`set_bmc_root_password` and `probe_bmc_vendor`, since the diff adds
validation/translation logic without visible coverage. Exercise the main success
path plus the failure cases from `validate_and_complete_bmc_endpoint_request`
and `resolve_bmc_interface`, including missing machine ID, IP/MAC mismatch, and
missing BMC IP. Keep the tests focused on the adapter behavior and expected
mapped responses/errors so regressions in these request handlers are caught.

Source: Path instructions

🤖 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/admin-cli/src/bmc_machine/probe_vendor/args.rs`:
- Around line 32-33: The `probe-vendor` help text currently shows a MAC-only
example even though `probe_vendor` always populates
`bmc_endpoint_request.ip_address` and the API requires it. Update the example in
`args.rs` to include `--ip-address`, or remove the `--mac-address`-only example
entirely, so the usage shown by `ProbeVendorArgs` matches the actual
`probe-vendor` behavior.

In `@crates/admin-cli/src/bmc_machine/set_root_password/args.rs`:
- Around line 34-36: The `set_root_password::args` example currently suggests
`--mac-address` can be used alone, but the `BmcEndpointRequest` built by this
flow still ends up with an empty `ip_address` and is rejected. Update the
`set_root_password` argument handling so the BMC IP is resolved before
constructing the request, using the MAC-based lookup path in the relevant
`BmcEndpointRequest`/handler flow, or remove the MAC-only example if that path
is not supported.

In `@crates/admin-cli/src/bmc_machine/set_root_password/cmd.rs`:
- Around line 22-26: The `set_root_password` function is passing the local
`Args` type directly into `set_bmc_root_password`, but the RPC client expects a
`::rpc::forge::SetBmcRootPasswordRequest`. Update the call in
`set_root_password` to convert `args` using its existing `Into` conversion
before invoking `api_client.0.set_bmc_root_password`, so the request type
matches what the client method requires.

In `@crates/api-core/src/handlers/bmc_endpoint_explorer.rs`:
- Around line 1012-1015: The handler logic in the endpoint explorer is
collapsing all EndpointExplorationError cases into CarbideError::internal, which
loses the user-input vs system-failure distinction. Update the error mapping in
the root password flow and the other affected handler to inspect the underlying
EndpointExplorationError and return the appropriate CarbideError variant for bad
password/missing credential cases versus genuine BMC/network failures. Follow
the same pattern used by map_redfish_client_creation_error and map_redfish_error
so callers can reliably distinguish actionable 4xx-like errors from internal
5xx-like errors.
- Around line 988-993: The request logging in set_bmc_root_password is exposing
the plaintext new_password because log_request_data(&request) logs the full
Debug payload before redaction. Update this path to use
log_request_data_redacted(...) or first call into_inner() and log a redacted
copy of rpc::SetBmcRootPasswordRequest so the sensitive field is never written
to logs.

In `@crates/rpc/proto/forge.proto`:
- Around line 7897-7905: The SetBmcRootPasswordRequest currently logs sensitive
data because set_bmc_root_password calls log_request_data(&request), which
captures the full Debug output including new_password. Update the
bmc_endpoint_explorer::set_bmc_root_password path to use the redacted logging
helper or explicitly omit/filter new_password before logging, so the request
still has context without exposing the plaintext BMC root password.

In `@crates/site-explorer/src/bmc_endpoint_explorer.rs`:
- Around line 1215-1240: The `set_bmc_root_password` flow can leave the BMC
rotated but the vault stale if `credential_client.set_bmc_root_credentials`
fails after the device password change succeeds. Update this path in
`set_bmc_root_password` to handle post-rotation persistence failures explicitly,
either by retrying the credential write with backoff or by returning a dedicated
error such as `EndpointExplorationError::CredentialPersistFailedAfterRotation`.
Make sure the error includes enough context to distinguish this from a generic
explorer failure so operators can quickly identify and remediate the
partial-failure case.

---

Nitpick comments:
In `@crates/admin-cli/src/bmc_machine/set_root_password/args.rs`:
- Around line 51-68: The `From<Args>` implementation for
`SetBmcRootPasswordRequest` duplicates the same `bmc_endpoint_request` building
logic used by `probe_vendor::args::From<Args>`, so factor that shared logic into
a common helper and reuse it from both command conversions. Introduce a reusable
function for constructing `forgerpc::BmcEndpointRequest` from the IP/MAC inputs,
then call it from `Args::from` here and in the probe vendor args conversion,
keeping `machine_id` and `new_password` handling local to
`SetBmcRootPasswordRequest`.

In `@crates/api-core/src/handlers/bmc_endpoint_explorer.rs`:
- Around line 988-1055: Add table-driven tests for the new request-to-response
adapter handlers `set_bmc_root_password` and `probe_bmc_vendor`, since the diff
adds validation/translation logic without visible coverage. Exercise the main
success path plus the failure cases from
`validate_and_complete_bmc_endpoint_request` and `resolve_bmc_interface`,
including missing machine ID, IP/MAC mismatch, and missing BMC IP. Keep the
tests focused on the adapter behavior and expected mapped responses/errors so
regressions in these request handlers are caught.

In
`@docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-probe-vendor.md`:
- Line 54: Update the example for nico-admin-cli bmc-machine probe-vendor so the
--ip-address placeholder uses the generic BMC/DPU IP value 192.0.2.10 instead of
192.0.2.20. The issue is in the probe-vendor command example, and the fix is to
replace the host-targeted placeholder with the canonical BMC IP placeholder
consistently in this document.

In
`@docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-set-root-password.md`:
- Line 60: The BMC IP placeholder in the set-root-password example is
inconsistent with the canonical BMC-targeted placeholder. Update the example
under the set-root-password command so it uses the generic/DPU BMC address
placeholder 192.0.2.10 for the --ip-address argument, matching the convention
used for direct BMC commands. Keep the command text otherwise unchanged and
ensure this aligns with the placeholder usage in the related BMC docs, including
the set-root-password example.
🪄 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: f547ec4d-ef1f-407e-97bd-f6d1f3e3640d

📥 Commits

Reviewing files that changed from the base of the PR and between 12ef832 and 4504c6d.

📒 Files selected for processing (20)
  • crates/admin-cli/src/bmc_machine/mod.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/args.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/cmd.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/mod.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/args.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/cmd.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/mod.rs
  • crates/api-core/src/api.rs
  • crates/api-core/src/handlers/bmc_endpoint_explorer.rs
  • crates/redfish/src/libredfish/mod.rs
  • crates/redfish/src/libredfish/test_support.rs
  • crates/rpc/proto/forge.proto
  • crates/secrets/src/credentials.rs
  • crates/site-explorer/src/bmc_endpoint_explorer.rs
  • crates/site-explorer/src/endpoint_explorer.rs
  • crates/site-explorer/src/redfish.rs
  • crates/site-explorer/src/test_support/mock_endpoint_explorer.rs
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-probe-vendor.md
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-set-root-password.md
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine.md

Comment thread crates/admin-cli/src/bmc_machine/probe_vendor/args.rs
Comment thread crates/admin-cli/src/bmc_machine/set_root_password/args.rs
Comment thread crates/admin-cli/src/bmc_machine/set_root_password/cmd.rs
Comment thread crates/api-core/src/handlers/bmc_endpoint_explorer.rs
Comment on lines +1012 to +1015
api.endpoint_explorer
.set_bmc_root_password(bmc_addr, &machine_interface, &req.new_password)
.await
.map_err(|e| CarbideError::internal(e.to_string()))?;

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 | 🟠 Major | 🏗️ Heavy lift

Blanket CarbideError::internal mapping loses the invalid-argument vs system-error distinction.

Both new handlers collapse every EndpointExplorationError (e.g. bad current password/Unauthorized, missing vault entry/MissingCredentials, genuine network/BMC failure) into CarbideError::internal. Callers (admin-cli, other services) can no longer distinguish "you supplied the wrong current password" (a 4xx-like, user-actionable condition) from "the BMC/network is down" (a genuine 5xx). This is the exact "avoid — constructing Status directly, bypassing NicoError error mapping" anti-pattern flagged in the referenced STYLE_GUIDE, applied in spirit to error variant selection.

Consider mapping the underlying EndpointExplorationError to the appropriate CarbideError variant (similar to how map_redfish_client_creation_error/map_redfish_error differentiate causes elsewhere in redfish.rs) instead of a single catch-all .internal().

As per path instructions, "Prefer NicoError-based error construction inside handlers... choose the NicoError variant based on whether failures are user-input/invalid-argument (4xx-like) vs internal/system issues."

Also applies to: 1044-1048

🤖 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/api-core/src/handlers/bmc_endpoint_explorer.rs` around lines 1012 -
1015, The handler logic in the endpoint explorer is collapsing all
EndpointExplorationError cases into CarbideError::internal, which loses the
user-input vs system-failure distinction. Update the error mapping in the root
password flow and the other affected handler to inspect the underlying
EndpointExplorationError and return the appropriate CarbideError variant for bad
password/missing credential cases versus genuine BMC/network failures. Follow
the same pattern used by map_redfish_client_creation_error and map_redfish_error
so callers can reliably distinguish actionable 4xx-like errors from internal
5xx-like errors.

Source: Path instructions

Comment thread crates/rpc/proto/forge.proto
Comment on lines +1215 to +1240
// Resolve the dispatch vendor `set_bmc_root_password` branches on using
// the current credentials, then set the new password on the device.
let vendor = self
.redfish_client
.probe_bmc_vendor(bmc_ip_address, current_credentials.clone())
.await?;
let new_credentials = self
.set_bmc_root_password(
bmc_ip_address,
vendor,
current_credentials,
new_password.to_string(),
)
.await?;

// Persist the new per-device credential so NICo can still reach the BMC.
// Deliberately does NOT record rotation convergence (unlike
// `set_bmc_root_credentials`): this is an out-of-band set, so the
// credential-rotation engine will reassert the site-wide password on
// its next pass rather than treating this device as converged.
self.credential_client
.set_bmc_root_credentials(bmc_mac_address, &new_credentials)
.await?;

Ok(())
}

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.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Partial-failure risk: BMC lockout if vault persistence fails after device rotation.

Lines 1221-1237 perform two dependent external writes: rotate the password on the physical BMC, then persist the new credential to the vault. If the vault write in Line 1236 fails (network blip, vault outage) after the on-device rotation in Line 1222 already succeeded, the stored credential becomes stale and every subsequent get_bmc_root_credentials call will authenticate with the wrong password — effectively locking NICo out of that BMC until an operator manually intervenes.

Consider retrying the vault persistence step with backoff before propagating the error, or emitting a distinct, actionable error/alert (e.g. EndpointExplorationError::CredentialPersistFailedAfterRotation) so operators can immediately recognize and remediate this specific failure mode rather than treating it as a generic explorer error.

🤖 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 1215 - 1240,
The `set_bmc_root_password` flow can leave the BMC rotated but the vault stale
if `credential_client.set_bmc_root_credentials` fails after the device password
change succeeds. Update this path in `set_bmc_root_password` to handle
post-rotation persistence failures explicitly, either by retrying the credential
write with backoff or by returning a dedicated error such as
`EndpointExplorationError::CredentialPersistFailedAfterRotation`. Make sure the
error includes enough context to distinguish this from a generic explorer
failure so operators can quickly identify and remediate the partial-failure
case.

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

🔍 Container Scan Summary

Service Total Critical High Medium Low Other
boot-artifacts-aarch64 3 0 0 3 0 0
boot-artifacts-x86_64 3 0 0 3 0 0
forge-admin-cli-x86_64 270 13 33 91 7 126
machine-validation-runner 771 37 219 281 40 194
machine_validation 771 37 219 281 40 194
machine_validation-aarch64 771 37 219 281 40 194
nvmetal-carbide 771 37 219 281 40 194
TOTAL 3360 161 909 1221 167 902

Per-CVE detail lives in the per-service grype-* artifacts (JSON + SARIF). Severity counts only — no CVE IDs published here.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🤖 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 `@rest-api/flow/internal/nicoapi/nicoproto/nico.proto`:
- Around line 7889-7899: `SetBmcRootPasswordRequest` is being logged with its
full Debug output, which exposes `new_password` in structured logs. Update the
BMC password handling in `bmc_endpoint_explorer` so the request is logged
through `log_request_data_redacted` or another non-secret logging path before
`new_password` is read. Keep only safe fields such as `bmc_endpoint_request` and
`machine_id`, and ensure the redacted logging is used at the
`set_bmc_root_password` request handling site.
🪄 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: 86c84f2c-f43f-4473-868b-f24efdee7956

📥 Commits

Reviewing files that changed from the base of the PR and between 4504c6d and fe25adb.

⛔ Files ignored due to path filters (5)
  • rest-api/flow/internal/nicoapi/gen/nico.pb.go is excluded by !**/*.pb.go, !**/gen/**, !rest-api/**/*.pb.go
  • rest-api/flow/internal/nicoapi/gen/nico_grpc.pb.go is excluded by !**/*.pb.go, !**/gen/**, !rest-api/**/*.pb.go, !rest-api/**/*_grpc.pb.go
  • rest-api/workflow-schema/schema/site-agent/workflows/v1/nico_nico.pb.go is excluded by !**/*.pb.go, !rest-api/**/*.pb.go
  • rest-api/workflow-schema/schema/site-agent/workflows/v1/nico_nico_grpc.pb.go is excluded by !**/*.pb.go, !rest-api/**/*.pb.go, !rest-api/**/*_grpc.pb.go
  • rest-api/workflow-schema/site-agent/workflows/v1/nico_nico.proto is excluded by !rest-api/workflow-schema/site-agent/workflows/v1/*_nico.proto
📒 Files selected for processing (21)
  • crates/admin-cli/src/bmc_machine/mod.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/args.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/cmd.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/mod.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/args.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/cmd.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/mod.rs
  • crates/api-core/src/api.rs
  • crates/api-core/src/handlers/bmc_endpoint_explorer.rs
  • crates/redfish/src/libredfish/mod.rs
  • crates/redfish/src/libredfish/test_support.rs
  • crates/rpc/proto/forge.proto
  • crates/secrets/src/credentials.rs
  • crates/site-explorer/src/bmc_endpoint_explorer.rs
  • crates/site-explorer/src/endpoint_explorer.rs
  • crates/site-explorer/src/redfish.rs
  • crates/site-explorer/src/test_support/mock_endpoint_explorer.rs
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-probe-vendor.md
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-set-root-password.md
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine.md
  • rest-api/flow/internal/nicoapi/nicoproto/nico.proto
✅ Files skipped from review due to trivial changes (3)
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-set-root-password.md
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-probe-vendor.md
  • docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine.md
🚧 Files skipped from review as they are similar to previous changes (17)
  • crates/secrets/src/credentials.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/cmd.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/mod.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/args.rs
  • crates/site-explorer/src/endpoint_explorer.rs
  • crates/admin-cli/src/bmc_machine/probe_vendor/cmd.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/mod.rs
  • crates/api-core/src/api.rs
  • crates/site-explorer/src/test_support/mock_endpoint_explorer.rs
  • crates/admin-cli/src/bmc_machine/mod.rs
  • crates/admin-cli/src/bmc_machine/set_root_password/args.rs
  • crates/rpc/proto/forge.proto
  • crates/redfish/src/libredfish/test_support.rs
  • crates/redfish/src/libredfish/mod.rs
  • crates/site-explorer/src/bmc_endpoint_explorer.rs
  • crates/api-core/src/handlers/bmc_endpoint_explorer.rs
  • crates/site-explorer/src/redfish.rs

Comment thread rest-api/flow/internal/nicoapi/nicoproto/nico.proto
@spydaNVIDIA spydaNVIDIA force-pushed the rotate_creds branch 2 times, most recently from 4a4e408 to 7b81995 Compare July 6, 2026 19:00

@polarweasel polarweasel left a comment

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.

Minor docs changes

@spydaNVIDIA spydaNVIDIA requested a review from polarweasel July 6, 2026 19:57
@spydaNVIDIA spydaNVIDIA enabled auto-merge (squash) July 7, 2026 16:03
@spydaNVIDIA spydaNVIDIA merged commit 39722db into NVIDIA:main Jul 7, 2026
119 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request Jul 8, 2026
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants