Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
61 changes: 60 additions & 1 deletion crates/health/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

use std::fmt::Debug;
use std::net::{IpAddr, SocketAddr};
use std::path::Path;
use std::path::{Path, PathBuf};
use std::time::Duration;

use figment::Figment;
Expand Down Expand Up @@ -84,13 +84,72 @@ pub struct EndpointSourcesConfig {

/// Static BMC endpoints
pub static_bmc_endpoints: Vec<StaticBmcEndpoint>,

/// Cluster inventory file source (file or cluster manager JSON RPC)
pub cluster: Configurable<ClusterEndpointSourceConfig>,
}

impl Default for EndpointSourcesConfig {
fn default() -> Self {
Self {
carbide_api: Configurable::Enabled(CarbideApiConnectionConfig::default()),
static_bmc_endpoints: Vec::new(),
cluster: Configurable::Disabled,
}
}
}

#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct ClusterEndpointSourceConfig {
/// Path to a JSON inventory file containing BMC endpoints and credentials for the cluster.
/// Used when `cluster_manager_url` is absent.
#[serde(default)]
pub inventory_path: PathBuf,

/// Cluster manager head-node URL (e.g. https://10.x.x.x:8081).
/// When set, inventory and credentials are fetched live via cluster manager JSON RPC
/// instead of reading `inventory_path`.
#[serde(default)]
pub cluster_manager_url: Option<url::Url>,

/// Cluster manager partition to read bmcsettings from (default: "base").
/// The cluster manager stores BMC username/password at partition level; this selects which partition.
#[serde(default = "default_cluster_manager_partition")]
pub cluster_manager_partition: String,

/// Fallback BMC username if cluster manager JSON RPC does not return one.
/// Cluster manager default is "bright" (set during head-node installation).
#[serde(default = "default_cluster_manager_username")]
pub default_username: String,

/// Fallback BMC password if cluster manager JSON RPC does not return one.
/// Must be set explicitly — no code-level default.
#[serde(default)]
pub default_password: Option<String>,

/// Optional BMC port override. None uses the BmcClient default (443/HTTPS).
#[serde(default)]
pub port: Option<u16>,
}
Comment on lines +102 to +134

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

Secret leakage via derived Debug/Serialize.

ClusterEndpointSourceConfig derives Debug and Serialize, and default_password: Option<String> is a plain field. Anywhere this config is logged with ?config or tracing::debug!(?cfg, ...) (or serialized for diagnostics), the password will be printed verbatim. Compare with StaticBmcEndpoint, which has a hand-written Debug impl (crates/health/src/config.rs:233-243) that deliberately omits username/password. This new struct breaks that established convention.

🔒 Suggested fix: redact the password in `Debug`
-#[derive(Debug, Clone, Serialize, Deserialize)]
+#[derive(Clone, Serialize, Deserialize)]
 #[serde(deny_unknown_fields)]
 pub struct ClusterEndpointSourceConfig {
     ...
 }
+
+impl std::fmt::Debug for ClusterEndpointSourceConfig {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("ClusterEndpointSourceConfig")
+            .field("inventory_path", &self.inventory_path)
+            .field("cluster_manager_url", &self.cluster_manager_url)
+            .field("cluster_manager_partition", &self.cluster_manager_partition)
+            .field("default_username", &self.default_username)
+            .field("default_password", &self.default_password.as_ref().map(|_| "<redacted>"))
+            .field("port", &self.port)
+            .finish()
+    }
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct ClusterEndpointSourceConfig {
/// Path to a JSON inventory file containing BMC endpoints and credentials for the cluster.
/// Used when `cluster_manager_url` is absent.
#[serde(default)]
pub inventory_path: PathBuf,
/// Cluster manager head-node URL (e.g. https://10.x.x.x:8081).
/// When set, inventory and credentials are fetched live via cluster manager JSON RPC
/// instead of reading `inventory_path`.
#[serde(default)]
pub cluster_manager_url: Option<url::Url>,
/// Cluster manager partition to read bmcsettings from (default: "base").
/// The cluster manager stores BMC username/password at partition level; this selects which partition.
#[serde(default = "default_cluster_manager_partition")]
pub cluster_manager_partition: String,
/// Fallback BMC username if cluster manager JSON RPC does not return one.
/// Cluster manager default is "bright" (set during head-node installation).
#[serde(default = "default_cluster_manager_username")]
pub default_username: String,
/// Fallback BMC password if cluster manager JSON RPC does not return one.
/// Must be set explicitly — no code-level default.
#[serde(default)]
pub default_password: Option<String>,
/// Optional BMC port override. None uses the BmcClient default (443/HTTPS).
#[serde(default)]
pub port: Option<u16>,
}
#[derive(Clone, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct ClusterEndpointSourceConfig {
/// Path to a JSON inventory file containing BMC endpoints and credentials for the cluster.
/// Used when `cluster_manager_url` is absent.
#[serde(default)]
pub inventory_path: PathBuf,
/// Cluster manager head-node URL (e.g. https://10.x.x.x:8081).
/// When set, inventory and credentials are fetched live via cluster manager JSON RPC
/// instead of reading `inventory_path`.
#[serde(default)]
pub cluster_manager_url: Option<url::Url>,
/// Cluster manager partition to read bmcsettings from (default: "base").
/// The cluster manager stores BMC username/password at partition level; this selects which partition.
#[serde(default = "default_cluster_manager_partition")]
pub cluster_manager_partition: String,
/// Fallback BMC username if cluster manager JSON RPC does not return one.
/// Cluster manager default is "bright" (set during head-node installation).
#[serde(default = "default_cluster_manager_username")]
pub default_username: String,
/// Fallback BMC password if cluster manager JSON RPC does not return one.
/// Must be set explicitly — no code-level default.
#[serde(default)]
pub default_password: Option<String>,
/// Optional BMC port override. None uses the BmcClient default (443/HTTPS).
#[serde(default)]
pub port: Option<u16>,
}
impl std::fmt::Debug for ClusterEndpointSourceConfig {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("ClusterEndpointSourceConfig")
.field("inventory_path", &self.inventory_path)
.field("cluster_manager_url", &self.cluster_manager_url)
.field("cluster_manager_partition", &self.cluster_manager_partition)
.field("default_username", &self.default_username)
.field("default_password", &self.default_password.as_ref().map(|_| "<redacted>"))
.field("port", &self.port)
.finish()
}
}
🤖 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/health/src/config.rs` around lines 102 - 134,
ClusterEndpointSourceConfig’s derived Debug implementation exposes
default_password in logs. Replace the derived Debug with a manual implementation
that redacts or omits default_password, following StaticBmcEndpoint’s custom
Debug pattern, while preserving the existing serialization behavior unless
separately required.

Comment on lines +104 to +134

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.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n## files\n'
git ls-files crates/health/src | sed -n '1,120p'

printf '\n## outline config.rs\n'
ast-grep outline crates/health/src/config.rs --view expanded || true

printf '\n## outline cluster.rs\n'
ast-grep outline crates/health/src/cluster.rs --view expanded || true

printf '\n## search validate and endpoint source references\n'
rg -n "ClusterEndpointSourceConfig|validate\\(|inventory_path|cluster_manager_url|read_from_file" crates/health/src -n

printf '\n## targeted read config.rs around relevant lines\n'
wc -l crates/health/src/config.rs crates/health/src/cluster.rs
sed -n '1,260p' crates/health/src/config.rs
printf '\n--- cluster.rs ---\n'
sed -n '1,260p' crates/health/src/cluster.rs

Repository: NVIDIA/infra-controller

Length of output: 24070


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n## config validation section\n'
sed -n '1290,1405p' crates/health/src/config.rs

printf '\n## cluster endpoint loader\n'
sed -n '260,360p' crates/health/src/endpoint/cluster.rs

printf '\n## config validation tests nearby\n'
sed -n '1680,1775p' crates/health/src/config.rs

Repository: NVIDIA/infra-controller

Length of output: 10538


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n## cluster config impls/tests\n'
rg -n "impl ClusterEndpointSourceConfig|cluster_manager_url|inventory_path|endpoint_sources\.cluster|Loaded cluster endpoints|Failed to read cluster inventory" crates/health/src/config.rs crates/health/src/endpoint -n

printf '\n## default endpoint sources config\n'
sed -n '80,160p' crates/health/src/config.rs

printf '\n## endpoint source config tests around cluster/static config\n'
sed -n '1460,1710p' crates/health/src/config.rs

Repository: NVIDIA/infra-controller

Length of output: 13386


Validate cluster endpoint source configuration before startup
Config::validate() does not enforce any requirement on ClusterEndpointSourceConfig, so enabling [endpoint_sources.cluster] with both inventory_path unset and cluster_manager_url = none falls through to an empty-path file read in crates/health/src/endpoint/cluster.rs. Add a ClusterEndpointSourceConfig::validate() and invoke it from Config::validate() to reject that case with a clear config 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/health/src/config.rs` around lines 104 - 134, Add
ClusterEndpointSourceConfig::validate() to require either a non-empty
inventory_path or a configured cluster_manager_url, returning a clear
configuration error when both are absent; then invoke this validation from
Config::validate() whenever the cluster endpoint source is enabled.


fn default_cluster_manager_partition() -> String {
"base".to_string()
}

fn default_cluster_manager_username() -> String {
"bright".to_string()
}

impl Default for ClusterEndpointSourceConfig {
fn default() -> Self {
Self {
inventory_path: PathBuf::default(),
cluster_manager_url: None,
cluster_manager_partition: default_cluster_manager_partition(),
default_username: default_cluster_manager_username(),
default_password: None,
port: None,
}
}
}
Expand Down
Loading
Loading