Authenticate Scout and DPU-agent with self-signed bearer JWTs - #4373
Authenticate Scout and DPU-agent with self-signed bearer JWTs#4373wminckler wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughAdds configurable ES256 node-auth JWTs, API-side validation, bearer authentication middleware, a Unix-socket token broker for FMDS, and Helm wiring for DPU-agent and FMDS. Existing mTLS authentication remains configurable, with protected machine-key handling and tests/documentation. ChangesNode-auth JWT migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Scout
participant ForgeClient
participant CarbideAPI
participant AuthMiddleware
Scout->>ForgeClient: create request
ForgeClient->>ForgeClient: mint or retrieve ES256 JWT
ForgeClient->>CarbideAPI: send request with bearer token
CarbideAPI->>AuthMiddleware: process Authorization header
AuthMiddleware->>AuthMiddleware: validate x5c chain, claims, and SPIFFE identity
AuthMiddleware-->>CarbideAPI: attach node principal
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-30 14:00:48 UTC | Commit: 58b7b30 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
crates/api-core/src/node_auth.rs (2)
462-479: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTwo security-critical rejection paths remain untested: algorithm confusion and
submismatch.
RejectReason::AlgorithmandRejectReason::SubjectMismatchguard the module's central invariants — that only ES256 is honoured, and that identity derives from the certificate rather than from an attacker-supplied claim. Neither has a test, so a regression that relaxed either would pass CI. Both are cheap to add alongsideoverlong_lifetime_is_rejected: mint a token whosesubnames a different machine than the leaf's URI SAN, and one signed with a non-ES256 header.As per path instructions for
crates/**/*.rs, prefer "findings about behavior, concurrency, resource lifetimes, and missing tests over style-only comments".🤖 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/node_auth.rs` around lines 462 - 479, Add focused tests alongside overlong_lifetime_is_rejected covering both rejection paths: a token with a sub claim that differs from the leaf certificate’s URI SAN must be rejected, and a token signed with a non-ES256 algorithm header must also be rejected. Reuse the existing test_pki, mint_with, and NodeJwtValidator setup, and assert spiffe_id_from_bearer returns None for each case.Source: Path instructions
238-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider emitting a counted Event for bearer-token rejections, not just a DEBUG log.
Rejections here are the single signal that node auth is failing fleet-wide — precisely the CA-rotation scenario
refresh_rootsexists to prevent — yet they are observable only at DEBUG.RejectReasonis already a bounded enum, making it a natural#[label], and this crate's mTLS path sets the precedent withClientCertRejected/carbide_authn_client_cert_rejected_total. Keep the token and any error text in#[context].As per coding guidelines, "declare and emit a
carbide_instrument::Eventwhen an event deserves a count, rate, or duration" and "use bounded values (usually enums)" for labels.🤖 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/node_auth.rs` around lines 238 - 248, Update NodeJwtValidator::spiffe_id_from_bearer to declare and emit a counted carbide_instrument::Event when validate returns Err(reason). Use the bounded RejectReason as a label, and preserve the bearer token and error text as event context while retaining the existing None return behavior and debug log.Source: Coding guidelines
bluefield/charts/nico-fmds/templates/daemonset.yaml (1)
150-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCross-chart
certsDirconsistency is implicit, not enforced.
nico-agent-run's hostPath ({{ .Values.certsDir }}/run) must resolve to the exact same host path the co-locatednico-dpu-agentchart uses for its owncertsDir, since the socket is handed off via the shared host filesystem, not a programmatically-threaded value (unlikenode_auth.audience, whichdpf_services.rscentrally propagates to both). If an operator overridescertsDiron only one of the two charts, the socket path silently diverges.🤖 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 `@bluefield/charts/nico-fmds/templates/daemonset.yaml` around lines 150 - 160, Enforce that the nico-fmds certsDir and co-located nico-dpu-agent certsDir resolve identically before constructing the nico-agent-run hostPath in the daemonset template. Reuse the established shared configuration or validation mechanism to reject mismatched overrides, ensuring the socket is always mounted from the same host path used by nico-dpu-agent.crates/api-core/src/dpf_services.rs (1)
614-630: 🧹 Nitpick | 🔵 TrivialFleet-wide coupling between
node_auth.enabledand fmds's credential mode.
fmds_service(&resolved.base.fmds, node_auth.enabled)ties every fmds DaemonSet's authentication mode directly to the API's[node_auth] enabledflag. The moment an operator flipsnode_auth.enabled = trueon the API, all fmds instances fleet-wide immediately require a dpu-agent build that serves the local token-broker API (per the doc comment above), with no independent, per-rollout knob to stage this migration. If any node in the fleet is still running an older dpu-agent image without the local API, its fmds pod will be unable to fetch bearer tokens as soon as the API-side flag flips.Worth confirming this is an acceptable trade-off for the planned rollout sequencing (e.g., dpu-agent images upgraded fleet-wide before
node_auth.enabledis ever set), since there's no way to decouple the two oncenode_authis enabled.🤖 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/dpf_services.rs` around lines 614 - 630, Decouple fmds credential selection from the API-wide node_auth.enabled flag in mandatory_services. Add or reuse an independent fmds authentication setting and pass it to fmds_service, while retaining node_auth.enabled for dpu-agent/node-auth behavior so fmds rollout can be staged independently across the fleet.crates/api-core/src/cfg/file.rs (1)
1893-1974: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider
deny_unknown_fieldsonNodeAuthConfig.
CertificatesConfigandSecretsConfigin this same file enforcedeny_unknown_fieldsprecisely to catch operator typos (see theunknown field rejected.../rejects_misspelled_fieldtests).NodeAuthConfiggates node authentication entirely — a typo'd key (e.g.mtls_enableinstead ofmtls_enabled) would silently fall back to the default rather than error, potentially leaving nodes locked out or a site running with an unintended auth posture.🛡️ Proposed fix
-#[derive(Clone, Debug, Serialize, Deserialize)] +#[derive(Clone, Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct NodeAuthConfig {🤖 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/cfg/file.rs` around lines 1893 - 1974, Apply serde’s deny-unknown-fields attribute to NodeAuthConfig so misspelled or unsupported node-auth configuration keys fail deserialization instead of silently using defaults. Preserve the existing fields, defaults, and validate behavior, and add or update deserialization coverage for rejecting an unknown key such as mtls_enable.
🤖 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/agent/src/local_api.rs`:
- Around line 71-98: Update serve to set the socket parent directory permissions
to 0700 immediately after create_dir_all and before UnixListener::bind, while
preserving the existing socket-file permission restriction and error context.
Use the parent path already derived in serve and propagate any set_permissions
failure.
In `@crates/authn/src/middleware.rs`:
- Around line 627-634: Update both tracing::debug! calls in the bearer-token
SPIFFE parsing branches to keep stable message literals and record each error in
the structured error field using Display formatting. Preserve the existing
target and branch-specific messages while replacing direct {e} interpolation.
- Around line 264-268: Update bearer_token_from_headers to parse the
Authorization value with a case-insensitive Bearer scheme and tolerate optional
whitespace between the scheme and token. Preserve returning None for missing,
invalid, or tokenless headers, while continuing to trim surrounding token
whitespace.
In `@docs/design/machine-identity/node-auth-jwt.md`:
- Around line 206-216: Update the component map and “Open question” section to
describe the local token broker as implemented production functionality,
referencing agent `local_api.rs` with `GetNodeToken`, RPC
`node_token_socket.rs`, and FMDS wiring in `cfg.rs`/`main.rs`; remove stale
prototype, branch, and recommended-target wording. Correct the client opt-in row
to distinguish scout’s `with_node_jwt()` from agent’s direct shared
`NodeJwtMinter::with_audience(...)` and `.with_token_provider(...)` setup, and
retain only genuine open items such as the Kubernetes Secret alternative or
otelcol’s raw-key requirement.
In `@helm/charts/nico-api/files/carbide-api-config.toml`:
- Around line 44-45: Remove one of the consecutive [node_auth] table
declarations in helm/charts/nico-api/files/carbide-api-config.toml at lines
44-45 and apply the same removal in
deploy/nico-base/api/config-files/nico-api-config.toml at lines 46-47, leaving
exactly one [node_auth] header in each configuration source.
---
Nitpick comments:
In `@bluefield/charts/nico-fmds/templates/daemonset.yaml`:
- Around line 150-160: Enforce that the nico-fmds certsDir and co-located
nico-dpu-agent certsDir resolve identically before constructing the
nico-agent-run hostPath in the daemonset template. Reuse the established shared
configuration or validation mechanism to reject mismatched overrides, ensuring
the socket is always mounted from the same host path used by nico-dpu-agent.
In `@crates/api-core/src/cfg/file.rs`:
- Around line 1893-1974: Apply serde’s deny-unknown-fields attribute to
NodeAuthConfig so misspelled or unsupported node-auth configuration keys fail
deserialization instead of silently using defaults. Preserve the existing
fields, defaults, and validate behavior, and add or update deserialization
coverage for rejecting an unknown key such as mtls_enable.
In `@crates/api-core/src/dpf_services.rs`:
- Around line 614-630: Decouple fmds credential selection from the API-wide
node_auth.enabled flag in mandatory_services. Add or reuse an independent fmds
authentication setting and pass it to fmds_service, while retaining
node_auth.enabled for dpu-agent/node-auth behavior so fmds rollout can be staged
independently across the fleet.
In `@crates/api-core/src/node_auth.rs`:
- Around line 462-479: Add focused tests alongside overlong_lifetime_is_rejected
covering both rejection paths: a token with a sub claim that differs from the
leaf certificate’s URI SAN must be rejected, and a token signed with a non-ES256
algorithm header must also be rejected. Reuse the existing test_pki, mint_with,
and NodeJwtValidator setup, and assert spiffe_id_from_bearer returns None for
each case.
- Around line 238-248: Update NodeJwtValidator::spiffe_id_from_bearer to declare
and emit a counted carbide_instrument::Event when validate returns Err(reason).
Use the bounded RejectReason as a label, and preserve the bearer token and error
text as event context while retaining the existing None return behavior and
debug log.
🪄 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: 267fd218-4289-4c00-a1d7-fe330e0edc99
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockrest-api/proto/core/gen/v1/agent_local_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/agent_local_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (40)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/values.yamlbluefield/charts/nico-fmds/templates/daemonset.yamlbluefield/charts/nico-fmds/values.yamlcrates/agent/Cargo.tomlcrates/agent/example_agent_config.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/local_api.rscrates/agent/src/tests/common/mod.rscrates/api-core/src/api.rscrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/lib.rscrates/api-core/src/listener.rscrates/api-core/src/node_auth.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/builder.rscrates/api-core/src/test_support/default_config.rscrates/authn/Cargo.tomlcrates/authn/src/middleware.rscrates/fmds/src/cfg.rscrates/fmds/src/main.rscrates/host-support/src/agent_config.rscrates/host-support/src/registration.rscrates/host-support/test/min_agent_config/output.tomlcrates/rpc/Cargo.tomlcrates/rpc/build.rscrates/rpc/proto/agent_local.protocrates/rpc/src/forge_tls_client.rscrates/rpc/src/lib.rscrates/rpc/src/node_jwt.rscrates/rpc/src/node_token_socket.rscrates/rpc/src/protos/mod.rscrates/scout/src/cfg/command_line.rscrates/scout/src/client.rsdeploy/nico-base/api/config-files/nico-api-config.tomldocs/design/machine-identity/node-auth-jwt.mdhelm/charts/nico-api/files/carbide-api-config.tomlrest-api/proto/core/src/v1/agent_local_nico.proto
| /// Extracts the token from an `Authorization: Bearer <token>` header, if present. | ||
| fn bearer_token_from_headers(headers: &hyper::HeaderMap) -> Option<&str> { | ||
| let value = headers.get(hyper::header::AUTHORIZATION)?.to_str().ok()?; | ||
| value.strip_prefix("Bearer ").map(str::trim) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
RFC 6750 Bearer authentication scheme case-insensitive Authorization header
💡 Result:
The "Bearer" authentication scheme used in the HTTP Authorization header is case-insensitive [1][2]. Although RFC 6750 provides the ABNF definition credentials = "Bearer" 1*SP b64token [3][4], which uses a capitalized "Bearer," this does not mandate case sensitivity. According to ABNF specification (RFC 5234), literal strings defined in quotes are case-insensitive [2]. Furthermore, HTTP authentication schemes are fundamentally case-insensitive, as established by the HTTP authentication framework [1][2]. In practice, this has been a source of confusion and interoperability issues, leading some implementations to incorrectly enforce case sensitivity [5][6][1]. To resolve this ambiguity, the upcoming OAuth 2.1 specification explicitly clarifies that the "Bearer" scheme is case-insensitive, confirming that variations such as "bearer", "BEARER", and "bEaReR" are valid [1][7][2].
Citations:
- 1: case sensitivity of Bearer http authentication scheme oauth-wg/oauth-v2-1#166
- 2: https://auth0.com/blog/the-bearer-token-case/
- 3: https://www.rfc-editor.org/rfc/rfc6750.html
- 4: https://datatracker.ietf.org/doc/rfc6750/
- 5: https://sgeb.io/posts/fix-go-oauth2-case-sensitive-bearer-auth-headers/
- 6: Authentication scheme case-insensitivity spring-projects/spring-security#6228
- 7: https://www.mail-archive.com/oauth@ietf.org/msg24647.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file =="
cat -n crates/authn/src/middleware.rs | sed -n '250,285p'
echo
echo "== bearer token call sites =="
rg -n "bearer_token_from_headers|Authorization: Bearer|Bearer " crates -g '!target' -g '!**/node_modules/**'
echo
echo "== node jwt producer referenced in the original comment =="
cat -n crates/rpc/src/node_jwt.rs | sed -n '260,325p'Repository: NVIDIA/infra-controller
Length of output: 7791
Accept the Bearer scheme case-insensitively
Authorization parsing here is stricter than the HTTP auth-scheme grammar: Bearer is case-insensitive, and the separator may include more than one space. Lowercase or slightly different clients will be dropped silently. Accepting eq_ignore_ascii_case("Bearer") and tolerating optional whitespace would make this interoperable with essentially no complexity.
🤖 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/authn/src/middleware.rs` around lines 264 - 268, Update
bearer_token_from_headers to parse the Authorization value with a
case-insensitive Bearer scheme and tolerate optional whitespace between the
scheme and token. Preserve returning None for missing, invalid, or tokenless
headers, while continuing to trim surrounding token whitespace.
| Err(e) => { | ||
| tracing::debug!(target: "node_auth", "node-auth: bearer token SPIFFE id not recognized: {e}"); | ||
| } | ||
| }, | ||
| Err(e) => { | ||
| tracing::debug!(target: "node_auth", "node-auth: bearer token contained an unparsable SPIFFE URI: {e}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Interpolated error text in tracing messages breaks the structured-logging contract.
Both messages embed {e} directly, which destabilises the message literal and prevents the error from being filtered or aggregated as a field. node_auth.rs in this same feature already does it correctly with %reason.
♻️ Move the error into an `error` field
Err(e) => {
- tracing::debug!(target: "node_auth", "node-auth: bearer token SPIFFE id not recognized: {e}");
+ tracing::debug!(target: "node_auth", error = %e, "node-auth: bearer token SPIFFE id not recognized");
}
},
Err(e) => {
- tracing::debug!(target: "node_auth", "node-auth: bearer token contained an unparsable SPIFFE URI: {e}");
+ tracing::debug!(target: "node_auth", error = %e, "node-auth: bearer token contained an unparsable SPIFFE URI");
}As per coding guidelines, "Tracing messages must be stable human-readable string literals; put dynamic values in structured fields instead of interpolating them into messages", using % for Display and the documented error field name.
📝 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.
| Err(e) => { | |
| tracing::debug!(target: "node_auth", "node-auth: bearer token SPIFFE id not recognized: {e}"); | |
| } | |
| }, | |
| Err(e) => { | |
| tracing::debug!(target: "node_auth", "node-auth: bearer token contained an unparsable SPIFFE URI: {e}"); | |
| } | |
| } | |
| Err(e) => { | |
| tracing::debug!(target: "node_auth", error = %e, "node-auth: bearer token SPIFFE id not recognized"); | |
| } | |
| }, | |
| Err(e) => { | |
| tracing::debug!(target: "node_auth", error = %e, "node-auth: bearer token contained an unparsable SPIFFE URI"); | |
| } |
🤖 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/authn/src/middleware.rs` around lines 627 - 634, Update both
tracing::debug! calls in the bearer-token SPIFFE parsing branches to keep stable
message literals and record each error in the structured error field using
Display formatting. Preserve the existing target and branch-specific messages
while replacing direct {e} interpolation.
Source: Coding guidelines
58b7b30 to
ba1791b
Compare
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 (2)
crates/fmds/src/main.rs (1)
83-126: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSilently drops a configured
node_token_socketwhenroot_cais missing.If an operator sets
--node-token-socket/FMDS_NODE_TOKEN_SOCKETwithout--root-ca, the match falls to the generic_arm and phone_home is disabled entirely with a generic "no TLS credentials" warning — masking the actual misconfiguration (node-auth was requested but can't work withoutroot_cato validate the server).🐛 Proposed fix: surface the specific misconfiguration
let forge_client_config = match &options.root_ca { Some(root_ca) if client_cert.is_some() || options.node_token_socket.is_some() => { let mut config = ForgeClientConfig::new(root_ca.clone(), client_cert); if let Some(socket) = &options.node_token_socket { ... } Some(Arc::new(config)) } + None if options.node_token_socket.is_some() => { + eyre::bail!( + "--node-token-socket was provided without --root-ca; node-auth bearer tokens still require a root CA to validate the API server" + ) + } _ => { tracing::warn!( "No TLS credentials provided; phone_home to carbide-api will be unavailable" ); None } };🤖 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/fmds/src/main.rs` around lines 83 - 126, Update the forge_client_config match around options.root_ca and options.node_token_socket so configuring node-token authentication without root_ca is handled as an explicit misconfiguration rather than the generic no-credentials path. Emit a specific warning or error stating that node_token_socket requires root_ca, while preserving the existing configuration flow when root_ca is present.crates/rpc/src/node_jwt.rs (1)
142-199: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBlocking disk I/O on every request contradicts
NodeTokenProvider's own "never wait on I/O" contract.
NodeTokenProvider::currentis documented as needing to be non-blocking (Line 249:currentruns on the request path, so implementations must be non-blocking: return a cached token orNone, never wait on I/O"), yetNodeJwtMinter::current_with_expiryfalls through tomint(), which performs synchronousstd::fs::read/read_to_stringcalls. Sincemint()failures aren't negatively cached, a missing/unreadable cert (e.g. before registration completes) causes every outgoing request throughBearerAuthServiceto repeat the blocking read-and-fail cycle instead of backing off.Consider negative-caching failed mint attempts (short TTL) to bound retry frequency, and/or moving the disk read off the synchronous request path (mirroring the background-refresh design already used by
SocketTokenSource).♻️ Illustrative negative-cache sketch
pub fn current_with_expiry(&self) -> Option<(String, u64)> { let now = unix_now().ok()?; if let Ok(guard) = self.cached.read() && let Some(cached) = guard.as_ref() && cached.expires_at > now + REMINT_MARGIN_SECS { return Some((cached.token.clone(), cached.expires_at)); } + // Avoid hammering disk on every request while cert/key are + // missing/unreadable: skip re-minting until a short backoff elapses. + if let Ok(guard) = self.cached.read() + && let Some(cached) = guard.as_ref() + && cached.last_mint_failure_at.is_some_and(|t| now < t + MINT_RETRY_BACKOFF_SECS) + { + return None; + } match self.mint(now) {Also applies to: 244-255
🤖 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/rpc/src/node_jwt.rs` around lines 142 - 199, Update NodeJwtMinter::current_with_expiry so the request path never performs synchronous minting or disk I/O. Reuse the existing cached-token state to return a valid token, otherwise return None while scheduling or relying on background refresh; if immediate mint retries remain necessary, add a short-lived negative cache to suppress repeated failures. Preserve the current error logging and expiry behavior.
♻️ Duplicate comments (1)
crates/agent/src/local_api.rs (1)
71-90: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTOCTOU window on the socket directory remains unaddressed.
UnixListener::bind()creates the socket entry beforeset_permissions(0o600)runs on it, and the parent directory itself is never restricted. Anything with traversal access to the shared/opt/forge/runparent can connect during that window. This was already flagged in a prior review pass and the ordering here is unchanged.🔒 Harden the parent directory before bind
if let Some(parent) = std::path::Path::new(socket_path).parent() { std::fs::create_dir_all(parent) .wrap_err(format!("creating socket directory {}", parent.display()))?; + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700)) + .wrap_err(format!("restricting socket directory permissions on {}", parent.display()))?; + } }🤖 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/agent/src/local_api.rs` around lines 71 - 90, Update serve to restrict the socket parent directory before UnixListener::bind creates the socket, using restrictive directory permissions and preserving the existing error context. Keep the stale-socket removal and post-bind socket permission handling, while ensuring the parent-directory hardening occurs before binding.
🤖 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/rpc/src/forge_tls_client.rs`:
- Around line 187-195: Update `with_token_provider` to enable TLS certificate
validation as part of setting any bearer-token provider, ensuring the resulting
client cannot retain `DummyTlsVerifier` when token authentication is configured.
Preserve the provider assignment and fluent return behavior, and avoid requiring
callers to separately invoke `require_tls_enforcement()`.
---
Outside diff comments:
In `@crates/fmds/src/main.rs`:
- Around line 83-126: Update the forge_client_config match around
options.root_ca and options.node_token_socket so configuring node-token
authentication without root_ca is handled as an explicit misconfiguration rather
than the generic no-credentials path. Emit a specific warning or error stating
that node_token_socket requires root_ca, while preserving the existing
configuration flow when root_ca is present.
In `@crates/rpc/src/node_jwt.rs`:
- Around line 142-199: Update NodeJwtMinter::current_with_expiry so the request
path never performs synchronous minting or disk I/O. Reuse the existing
cached-token state to return a valid token, otherwise return None while
scheduling or relying on background refresh; if immediate mint retries remain
necessary, add a short-lived negative cache to suppress repeated failures.
Preserve the current error logging and expiry behavior.
---
Duplicate comments:
In `@crates/agent/src/local_api.rs`:
- Around line 71-90: Update serve to restrict the socket parent directory before
UnixListener::bind creates the socket, using restrictive directory permissions
and preserving the existing error context. Keep the stale-socket removal and
post-bind socket permission handling, while ensuring the parent-directory
hardening occurs before binding.
🪄 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: 925618da-9731-40dc-85cf-d1ae4b385f27
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockrest-api/proto/core/gen/v1/agent_local_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/agent_local_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (29)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/values.yamlbluefield/charts/nico-fmds/templates/daemonset.yamlbluefield/charts/nico-fmds/values.yamlcrates/agent/Cargo.tomlcrates/agent/example_agent_config.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/local_api.rscrates/agent/src/tests/common/mod.rscrates/api-core/src/dpf_services.rscrates/api-core/src/listener.rscrates/api-core/src/node_auth.rscrates/api-core/src/setup.rscrates/authn/src/middleware.rscrates/fmds/src/cfg.rscrates/fmds/src/main.rscrates/host-support/src/agent_config.rscrates/host-support/test/min_agent_config/output.tomlcrates/rpc/build.rscrates/rpc/proto/agent_local.protocrates/rpc/src/forge_tls_client.rscrates/rpc/src/lib.rscrates/rpc/src/node_jwt.rscrates/rpc/src/node_token_socket.rscrates/rpc/src/protos/mod.rscrates/scout/src/cfg/command_line.rscrates/scout/src/client.rsrest-api/proto/core/src/v1/agent_local_nico.proto
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/agent/src/tests/common/mod.rs
- crates/agent/example_agent_config.toml
- crates/scout/src/cfg/command_line.rs
- crates/authn/src/middleware.rs
| /// Attaches an explicit node-auth token provider — e.g. a pre-built | ||
| /// [`NodeJwtMinter`] the caller also serves through the agent's local | ||
| /// API, or a `SocketTokenSource` in a process that holds no key at all. | ||
| #[must_use] | ||
| pub fn with_token_provider(mut self, provider: Arc<dyn NodeTokenProvider>) -> Self { | ||
| self.node_token_provider = Some(provider); | ||
| self | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
with_token_provider should enforce TLS validation itself, not rely on callers to remember require_tls_enforcement().
Any bearer-token client implicitly trusts the token as its credential, so the server's certificate must always be validated — never left on DummyTlsVerifier. Today this only holds because crates/fmds/src/main.rs happens to chain .require_tls_enforcement() before .with_token_provider(...); a future caller that skips this pairing would silently get an unauthenticated TLS channel while believing bearer-token auth is protecting the connection. As per coding guidelines, "design APIs to be difficult to misuse and add abstractions only for real requirements."
🔒 Proposed fix: bake enforcement into the setter
#[must_use]
pub fn with_token_provider(mut self, provider: Arc<dyn NodeTokenProvider>) -> Self {
self.node_token_provider = Some(provider);
- self
+ self.require_tls_enforcement()
}📝 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.
| /// Attaches an explicit node-auth token provider — e.g. a pre-built | |
| /// [`NodeJwtMinter`] the caller also serves through the agent's local | |
| /// API, or a `SocketTokenSource` in a process that holds no key at all. | |
| #[must_use] | |
| pub fn with_token_provider(mut self, provider: Arc<dyn NodeTokenProvider>) -> Self { | |
| self.node_token_provider = Some(provider); | |
| self | |
| } | |
| /// Attaches an explicit node-auth token provider — e.g. a pre-built | |
| /// [`NodeJwtMinter`] the caller also serves through the agent's local | |
| /// API, or a `SocketTokenSource` in a process that holds no key at all. | |
| #[must_use] | |
| pub fn with_token_provider(mut self, provider: Arc<dyn NodeTokenProvider>) -> Self { | |
| self.node_token_provider = Some(provider); | |
| self.require_tls_enforcement() | |
| } |
🤖 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/rpc/src/forge_tls_client.rs` around lines 187 - 195, Update
`with_token_provider` to enable TLS certificate validation as part of setting
any bearer-token provider, ensuring the resulting client cannot retain
`DummyTlsVerifier` when token authentication is configured. Preserve the
provider assignment and fluent return behavior, and avoid requiring callers to
separately invoke `require_tls_enforcement()`.
Source: Coding guidelines
ba1791b to
d911e7b
Compare
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/api-core/src/dpf_services.rs`:
- Around line 664-668: Update the test invoking dpu_agent_helm_values to pass a
non-default node-auth audience and assert that the rendered Helm values contain
the same value at nodeAuth.audience, ensuring the argument is emitted under the
correct key.
In `@crates/host-support/src/agent_config.rs`:
- Around line 127-131: Validate node_auth_audience during AgentConfig::load_from
so whitespace-only or blank TOML values are rejected, matching the existing
non-blank validation used for the CLI flag. Apply the check to the deserialized
configuration before it can mint JWTs, while preserving
default_node_auth_audience behavior for omitted values.
🪄 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: 7a4e1b90-3a41-49d3-ad05-1fafae16dcf1
📒 Files selected for processing (17)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/values.yamlbluefield/charts/nico-fmds/templates/daemonset.yamlbluefield/charts/nico-fmds/values.yamlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/tests/common/mod.rscrates/api-core/src/dpf_services.rscrates/api-core/src/listener.rscrates/api-core/src/node_auth.rscrates/api-core/src/setup.rscrates/host-support/src/agent_config.rscrates/host-support/test/min_agent_config/output.tomlcrates/rpc/src/forge_tls_client.rscrates/rpc/src/node_jwt.rscrates/scout/src/cfg/command_line.rscrates/scout/src/client.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- bluefield/charts/nico-fmds/values.yaml
| dpu_agent_helm_values( | ||
| &default_dpu_agent_service(), | ||
| &policy, | ||
| ::rpc::node_jwt::NODE_JWT_AUDIENCE, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the rendered node-auth audience.
This test passes the new argument but never verifies nodeAuth.audience; a dropped or misspelled Helm key would still pass. Add an assertion using a non-default audience.
Proposed test
+#[test]
+fn dpu_agent_helm_values_include_node_auth_audience() {
+ let audience = "site-specific-api";
+ let values = dpu_agent_helm_values(
+ &default_dpu_agent_service(),
+ &DpfDpuAgentBootstrapCa::default(),
+ audience,
+ );
+
+ assert_eq!(values["nodeAuth"]["audience"], audience);
+}📝 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.
| dpu_agent_helm_values( | |
| &default_dpu_agent_service(), | |
| &policy, | |
| ::rpc::node_jwt::NODE_JWT_AUDIENCE, | |
| ) | |
| #[test] | |
| fn dpu_agent_helm_values_include_node_auth_audience() { | |
| let audience = "site-specific-api"; | |
| let values = dpu_agent_helm_values( | |
| &default_dpu_agent_service(), | |
| &DpfDpuAgentBootstrapCa::default(), | |
| audience, | |
| ); | |
| assert_eq!(values["nodeAuth"]["audience"], audience); | |
| } |
🤖 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/dpf_services.rs` around lines 664 - 668, Update the test
invoking dpu_agent_helm_values to pass a non-default node-auth audience and
assert that the rendered Helm values contain the same value at
nodeAuth.audience, ensuring the argument is emitted under the correct key.
| /// `aud` stamped on node-auth bearer JWTs (issue #355). Must match the | ||
| /// API's `[node_auth] audience`; a site that changes one must change the | ||
| /// other, or the API rejects every token this node mints. | ||
| #[serde(default = "default_node_auth_audience")] | ||
| pub node_auth_audience: String, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'node_auth_audience|ForgeSystemConfig|AgentConfig' crates/host-support crates/agentRepository: NVIDIA/infra-controller
Length of output: 19366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' crates/host-support/src/agent_config.rs
printf '\n---\n'
sed -n '1,220p' crates/agent/src/command_line.rs
printf '\n---\n'
sed -n '280,380p' crates/agent/src/lib.rs
printf '\n---\n'
rg -n 'non_blank_audience|trim\(\)\.is_empty|node_auth_audience|load_from\(' crates/host-support crates/agentRepository: NVIDIA/infra-controller
Length of output: 21440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,340p' crates/host-support/src/agent_config.rs
printf '\n---\n'
rg -n -C2 'struct NodeJwtMinter|with_audience|audience' crates -g '!target'Repository: NVIDIA/infra-controller
Length of output: 50379
Validate node_auth_audience on config load. AgentConfig::load_from accepts whitespace-only TOML values here, so a malformed file can still mint JWTs the API rejects. Apply the same non-blank check used by the CLI flag, or make this a validated type.
🤖 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/host-support/src/agent_config.rs` around lines 127 - 131, Validate
node_auth_audience during AgentConfig::load_from so whitespace-only or blank
TOML values are rejected, matching the existing non-blank validation used for
the CLI flag. Apply the check to the deserialized configuration before it can
mint JWTs, while preserving default_node_auth_audience behavior for omitted
values.
d911e7b to
11f4720
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
helm/charts/nico-api/files/carbide-api-config.toml (1)
44-61: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDuplicate
[node_auth]table header still present.Two consecutive
[node_auth]headers remain at the top of this block. TOML rejects redefining a table, so this can prevent the API config from parsing at all.🛠️ Proposed fix
[node_auth] -[node_auth] # Node (Scout / DPU-agent) authentication to the API.🤖 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 `@helm/charts/nico-api/files/carbide-api-config.toml` around lines 44 - 61, Remove the duplicate [node_auth] table header in the node authentication configuration, leaving exactly one header for the settings beginning with enabled and mtls_enabled so the TOML remains valid.crates/rpc/src/forge_tls_client.rs (1)
191-195: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
with_token_providerstill doesn't enforce TLS validation itself.Unchanged from the prior review: a bearer-token client implicitly trusts the token as its credential, so the server certificate must always be validated, never left on
DummyTlsVerifier. Today this only holds becausecrates/fmds/src/main.rshappens to chain.require_tls_enforcement()before.with_token_provider(...); a future caller that skips this pairing silently gets an unauthenticated TLS channel while believing bearer-token auth protects it.🔒 Proposed fix
#[must_use] pub fn with_token_provider(mut self, provider: Arc<dyn NodeTokenProvider>) -> Self { self.node_token_provider = Some(provider); - self + self.require_tls_enforcement() }🤖 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/rpc/src/forge_tls_client.rs` around lines 191 - 195, Update ForgeTlsClientBuilder::with_token_provider to enforce TLS certificate validation whenever a token provider is configured, rather than relying on callers to chain require_tls_enforcement separately. Preserve the existing provider assignment and builder return behavior, and ensure the method cannot leave DummyTlsVerifier active for bearer-token clients.crates/host-support/src/agent_config.rs (1)
121-131: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStill missing non-blank validation for
node_auth_audience.
node_auth_audiencedeserializes straight from TOML with no non-blank check, so a whitespace-only value in a config file mints tokens the API's[node_auth] audiencewill reject outright, rather than failing fast 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/host-support/src/agent_config.rs` around lines 121 - 131, The AgentConfig deserialization path must reject whitespace-only values for node_auth_audience instead of accepting them. Add non-blank validation to the node_auth_audience field’s existing TOML/config parsing flow, trimming or checking whitespace as appropriate while preserving valid audience values and returning a clear configuration error.
🧹 Nitpick comments (11)
crates/host-support/src/registration.rs (1)
349-351: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider
sync_all()instead offlush()for the persisted key.flush()on atokio::fs::Filedoes not push data to stable storage; a power loss right after registration could leave a truncated key file while the certificate is already written, requiring manual recovery.♻️ Suggested change
file.write_all(key).await?; - file.flush().await + file.sync_all().await🤖 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/host-support/src/registration.rs` around lines 349 - 351, Update the persisted-key write flow containing file.write_all and file.flush to call sync_all instead of flush, ensuring the key data is committed to stable storage before registration completes.bluefield/charts/nico-fmds/tests/node_tokens_test.yaml (1)
133-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the mTLS arguments in the default case. The default-mode test verifies the absence of token plumbing but not that
--client-cert/--client-keyare still rendered, which is the regression that would actually break existing deployments.♻️ Suggested additional assertions
- notContains: path: spec.template.spec.containers[0].args content: --node-token-socket=/opt/forge/run/agent.sock + - contains: + path: spec.template.spec.containers[0].args + content: --client-cert=/opt/forge/machine_cert.pem + - contains: + path: spec.template.spec.containers[0].args + content: --client-key=/opt/forge/machine_cert.key🤖 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 `@bluefield/charts/nico-fmds/tests/node_tokens_test.yaml` around lines 133 - 156, Extend the default-mode test “should keep the credentials mount and no pub volume by default” to assert that the rendered container arguments still include the mTLS `--client-cert` and `--client-key` options, using their expected credential paths. Preserve the existing assertions that token-mode plumbing is absent.crates/agent/src/local_api.rs (1)
74-76: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a relative-path edge case in the parent-directory handling. For a bare socket name (e.g.
agent.sock),Path::parent()yieldsSome("")andcreate_dir_all("")fails withENOENT, soservewould fail permanently in the agent's retry loop. Filtering empty parents keeps a dev/relative configuration usable.♻️ Suggested guard
- if let Some(parent) = std::path::Path::new(socket_path).parent() { + if let Some(parent) = std::path::Path::new(socket_path) + .parent() + .filter(|p| !p.as_os_str().is_empty()) + {🤖 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/agent/src/local_api.rs` around lines 74 - 76, Update the parent-directory handling around Path::new(socket_path).parent() so an empty parent for a bare relative socket name is skipped, while non-empty parents still pass to create_dir_all and retain the existing error context. Preserve the serve retry behavior and directory creation for paths that specify a parent.crates/rpc/src/node_token_socket.rs (1)
159-168: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPersistent token-fetch failure is only visible at
debuglevel. In token mode the bearer token is the sole client credential, so an unreachable agent socket silently degrades every request to unauthenticated. Consider escalating towarnafter a few consecutive failures (or logging the first failure atwarn) so a misconfigured deployment is diagnosable at default log levels.🤖 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/rpc/src/node_token_socket.rs` around lines 159 - 168, Escalate persistent token-fetch failures in the retry handling of the token-fetch loop around the Err(status) branch to warn-level logging, while retaining the existing retry behavior and context fields. Log the first failure or failures after a defined consecutive-failure threshold at warn so unreachable agent sockets are visible at default log levels.bluefield/charts/nico-dpu-agent/templates/daemonset.yaml (1)
134-139: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNil-safety:
.Values.nodeAuth.audiencepanics ifnodeAuthis explicitly nulled. The template already guardsbootstrapCawithdefault (dict)at line 1; applying the same treatment here keeps rendering robust against--set nodeAuth=nullor a values file that omits the map.♻️ Suggested guard
- {{- with .Values.nodeAuth.audience }} + {{- with get (default (dict) .Values.nodeAuth) "audience" }}🤖 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 `@bluefield/charts/nico-dpu-agent/templates/daemonset.yaml` around lines 134 - 139, Update the audience block in the DaemonSet template to access nodeAuth through a default empty dictionary, matching the existing bootstrapCa nil-safety pattern. Keep the current with guard and node-auth-audience argument behavior unchanged when nodeAuth.audience is configured.bluefield/charts/nico-dpu-agent/tests/node_auth_audience_test.yaml (1)
29-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
notContainsis an exact-element match, so it only proves the empty-valued flag is absent. The intent — "no--node-auth-audienceargument at all" — is currently carried by thelengthEqual: 5assertion, which will drift the next time an unrelated argument is added. Consider asserting the rendered args explicitly (equalon the list) instead of relying on a count.🤖 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 `@bluefield/charts/nico-dpu-agent/tests/node_auth_audience_test.yaml` around lines 29 - 34, The no-audience test should assert the complete rendered container argument list instead of relying on lengthEqual: 5, which can pass or fail due to unrelated arguments. Update the assertion near the existing notContains check in the node-auth audience test to use an explicit list equality that contains no --node-auth-audience argument while preserving the expected remaining arguments.bluefield/charts/nico-fmds/templates/daemonset.yaml (1)
131-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant variable re-declaration;
$rootCais unused here.$dirand$rootCaare already bound earlier in this template body, and thevolumeMountsblock only needs$dir.♻️ Suggested cleanup
- {{- $dir := .Values.certsDir | default "/opt/nico" }} - {{- $rootCa := .Values.rootCaFile | default "nico_root.pem" }} {{- if .Values.useNodeTokens }}🤖 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 `@bluefield/charts/nico-fmds/templates/daemonset.yaml` around lines 131 - 132, Remove the redundant `$rootCa` declaration from the volumeMounts block in the daemonset template, since it is already defined earlier and unused there. Retain the local `$dir` declaration because the block uses it.crates/api-core/src/cfg/file.rs (2)
1893-1913: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider
deny_unknown_fieldsonNodeAuthConfig.Unlike
CertificatesConfig/SecretsConfigin this same file,NodeAuthConfigdoesn't reject unknown TOML keys. Since every field here has a default, a typo (e.g.,enabld = true) silently falls back to the default rather than erroring — for a security-relevant auth toggle, that's a quiet downgrade rather than a loud failure.🛡️ Proposed fix
#[derive(Clone, Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct NodeAuthConfig {🤖 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/cfg/file.rs` around lines 1893 - 1913, Add serde’s deny_unknown_fields attribute to NodeAuthConfig, matching the strict deserialization behavior of CertificatesConfig and SecretsConfig, so misspelled or unsupported authentication settings fail configuration parsing instead of silently using defaults.
3573-3593: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd edge-case coverage for
validate().The current test only covers the enabled/mtls_enabled lockout combinations.
validate()also branches on emptyaudience,max_token_ttl_sec == 0, andmax_token_ttl_sec > NODE_AUTH_MAX_TOKEN_TTL_SEC— none of which are exercised.🤖 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/cfg/file.rs` around lines 3573 - 3593, Extend node_auth_rejects_all_methods_disabled to cover validate() branches for an empty audience, max_token_ttl_sec equal to zero, and max_token_ttl_sec above NODE_AUTH_MAX_TOKEN_TTL_SEC. Assert each configuration’s expected validation result while preserving the existing authentication-method combination checks.crates/rpc/src/node_jwt.rs (1)
240-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment misattached —
ec_encoding_keyis now undocumented.The block at lines 240-242 ("Builds an ES256 signing key from the client key PEM...") describes
ec_encoding_key(line 268) but is attached tokey_matches_certificate(line 249) instead, since there's no blank-line break. As written,key_matches_certificate's rustdoc opens with an unrelated sentence about signing keys, andec_encoding_keyships with no doc at all.📝 Proposed fix
-/// Builds an ES256 signing key from the client key PEM. Vault-issued machine -/// keys are SEC1 (`BEGIN EC PRIVATE KEY`), which `jsonwebtoken` cannot load -/// directly, so those are re-encoded to PKCS#8 first. -/// Whether `key_pem`'s public half is the one certified by `leaf_der`. +/// Whether `key_pem`'s public half is the one certified by `leaf_der`. /// /// Both encodings the node may hold are accepted: Vault issues SEC1 ("EC /// PRIVATE KEY") and renewal may leave PKCS#8. The comparison is on the /// uncompressed SEC1 point, which is exactly what an EC `SubjectPublicKeyInfo` /// carries. fn key_matches_certificate(key_pem: &str, leaf_der: &[u8]) -> Result<bool, NodeJwtError> { ... } +/// Builds an ES256 signing key from the client key PEM. Vault-issued machine +/// keys are SEC1 (`BEGIN EC PRIVATE KEY`), which `jsonwebtoken` cannot load +/// directly, so those are re-encoded to PKCS#8 first. fn ec_encoding_key(key_pem: &str) -> Result<EncodingKey, NodeJwtError> {🤖 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/rpc/src/node_jwt.rs` around lines 240 - 268, Move the opening signing-key Rustdoc from key_matches_certificate to ec_encoding_key, leaving key_matches_certificate documented only by the text describing certificate-key matching. Ensure ec_encoding_key has the full relevant documentation, including the SEC1 and PKCS#8 encoding details.crates/agent/src/lib.rs (1)
389-405: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftOwn and cancel the local API task.
This detached, infinite token-broker loop has no cancellation or join path during agent shutdown. Keep its handle in the service lifecycle, cancel it explicitly, and await it before exit so the socket server is shut down deterministically.
As per coding guidelines, “Join spawned background tasks through
JoinHandleorJoinSet” and “Use RAII cancellation for client-facing background services.”🤖 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/agent/src/lib.rs` around lines 389 - 405, The local API server task spawned in the agent startup lifecycle is detached and cannot be stopped during shutdown. Store the tokio::JoinHandle returned by the spawn alongside the service tasks, explicitly abort or cancel it during agent shutdown, and await its completion before exiting; preserve the existing retry loop and local_api::serve behavior.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@crates/host-support/src/agent_config.rs`:
- Around line 121-131: The AgentConfig deserialization path must reject
whitespace-only values for node_auth_audience instead of accepting them. Add
non-blank validation to the node_auth_audience field’s existing TOML/config
parsing flow, trimming or checking whitespace as appropriate while preserving
valid audience values and returning a clear configuration error.
In `@crates/rpc/src/forge_tls_client.rs`:
- Around line 191-195: Update ForgeTlsClientBuilder::with_token_provider to
enforce TLS certificate validation whenever a token provider is configured,
rather than relying on callers to chain require_tls_enforcement separately.
Preserve the existing provider assignment and builder return behavior, and
ensure the method cannot leave DummyTlsVerifier active for bearer-token clients.
In `@helm/charts/nico-api/files/carbide-api-config.toml`:
- Around line 44-61: Remove the duplicate [node_auth] table header in the node
authentication configuration, leaving exactly one header for the settings
beginning with enabled and mtls_enabled so the TOML remains valid.
---
Nitpick comments:
In `@bluefield/charts/nico-dpu-agent/templates/daemonset.yaml`:
- Around line 134-139: Update the audience block in the DaemonSet template to
access nodeAuth through a default empty dictionary, matching the existing
bootstrapCa nil-safety pattern. Keep the current with guard and
node-auth-audience argument behavior unchanged when nodeAuth.audience is
configured.
In `@bluefield/charts/nico-dpu-agent/tests/node_auth_audience_test.yaml`:
- Around line 29-34: The no-audience test should assert the complete rendered
container argument list instead of relying on lengthEqual: 5, which can pass or
fail due to unrelated arguments. Update the assertion near the existing
notContains check in the node-auth audience test to use an explicit list
equality that contains no --node-auth-audience argument while preserving the
expected remaining arguments.
In `@bluefield/charts/nico-fmds/templates/daemonset.yaml`:
- Around line 131-132: Remove the redundant `$rootCa` declaration from the
volumeMounts block in the daemonset template, since it is already defined
earlier and unused there. Retain the local `$dir` declaration because the block
uses it.
In `@bluefield/charts/nico-fmds/tests/node_tokens_test.yaml`:
- Around line 133-156: Extend the default-mode test “should keep the credentials
mount and no pub volume by default” to assert that the rendered container
arguments still include the mTLS `--client-cert` and `--client-key` options,
using their expected credential paths. Preserve the existing assertions that
token-mode plumbing is absent.
In `@crates/agent/src/lib.rs`:
- Around line 389-405: The local API server task spawned in the agent startup
lifecycle is detached and cannot be stopped during shutdown. Store the
tokio::JoinHandle returned by the spawn alongside the service tasks, explicitly
abort or cancel it during agent shutdown, and await its completion before
exiting; preserve the existing retry loop and local_api::serve behavior.
In `@crates/agent/src/local_api.rs`:
- Around line 74-76: Update the parent-directory handling around
Path::new(socket_path).parent() so an empty parent for a bare relative socket
name is skipped, while non-empty parents still pass to create_dir_all and retain
the existing error context. Preserve the serve retry behavior and directory
creation for paths that specify a parent.
In `@crates/api-core/src/cfg/file.rs`:
- Around line 1893-1913: Add serde’s deny_unknown_fields attribute to
NodeAuthConfig, matching the strict deserialization behavior of
CertificatesConfig and SecretsConfig, so misspelled or unsupported
authentication settings fail configuration parsing instead of silently using
defaults.
- Around line 3573-3593: Extend node_auth_rejects_all_methods_disabled to cover
validate() branches for an empty audience, max_token_ttl_sec equal to zero, and
max_token_ttl_sec above NODE_AUTH_MAX_TOKEN_TTL_SEC. Assert each configuration’s
expected validation result while preserving the existing authentication-method
combination checks.
In `@crates/host-support/src/registration.rs`:
- Around line 349-351: Update the persisted-key write flow containing
file.write_all and file.flush to call sync_all instead of flush, ensuring the
key data is committed to stable storage before registration completes.
In `@crates/rpc/src/node_jwt.rs`:
- Around line 240-268: Move the opening signing-key Rustdoc from
key_matches_certificate to ec_encoding_key, leaving key_matches_certificate
documented only by the text describing certificate-key matching. Ensure
ec_encoding_key has the full relevant documentation, including the SEC1 and
PKCS#8 encoding details.
In `@crates/rpc/src/node_token_socket.rs`:
- Around line 159-168: Escalate persistent token-fetch failures in the retry
handling of the token-fetch loop around the Err(status) branch to warn-level
logging, while retaining the existing retry behavior and context fields. Log the
first failure or failures after a defined consecutive-failure threshold at warn
so unreachable agent sockets are visible at default log levels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 61e22f6b-38e5-4b84-b45b-69cd5a0868d9
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockrest-api/proto/core/gen/v1/agent_local_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/agent_local_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (42)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/tests/node_auth_audience_test.yamlbluefield/charts/nico-dpu-agent/values.yamlbluefield/charts/nico-fmds/templates/daemonset.yamlbluefield/charts/nico-fmds/tests/node_tokens_test.yamlbluefield/charts/nico-fmds/values.yamlcrates/agent/Cargo.tomlcrates/agent/example_agent_config.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/local_api.rscrates/agent/src/tests/common/mod.rscrates/api-core/src/api.rscrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/lib.rscrates/api-core/src/listener.rscrates/api-core/src/node_auth.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/builder.rscrates/api-core/src/test_support/default_config.rscrates/authn/Cargo.tomlcrates/authn/src/middleware.rscrates/fmds/src/cfg.rscrates/fmds/src/main.rscrates/host-support/src/agent_config.rscrates/host-support/src/registration.rscrates/host-support/test/min_agent_config/output.tomlcrates/rpc/Cargo.tomlcrates/rpc/build.rscrates/rpc/proto/agent_local.protocrates/rpc/src/forge_tls_client.rscrates/rpc/src/lib.rscrates/rpc/src/node_jwt.rscrates/rpc/src/node_token_socket.rscrates/rpc/src/protos/mod.rscrates/scout/src/cfg/command_line.rscrates/scout/src/client.rsdeploy/nico-base/api/config-files/nico-api-config.tomldocs/design/machine-identity/node-auth-jwt.mdhelm/charts/nico-api/files/carbide-api-config.tomlrest-api/proto/core/src/v1/agent_local_nico.proto
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/host-support/test/min_agent_config/output.toml
- deploy/nico-base/api/config-files/nico-api-config.toml
- crates/agent/example_agent_config.toml
- crates/api-core/src/lib.rs
- crates/rpc/Cargo.toml
- docs/design/machine-identity/node-auth-jwt.md
11f4720 to
c487a0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
crates/host-support/src/registration.rs (1)
339-351: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider
sync_all()for the key.The create-with-mode then tighten-then-write ordering is right: the truncation window exposes only an empty file, and the secret bytes land after the chmod. One optional hardening —
flush()reaches the OS but not the platter, so an ungraceful reboot between the cert write and the key write can leave a valid certificate beside a truncated key.sync_all()before returning would make the pair recoverable without re-registration. Entirely optional, since the surrounding cert write makes the same trade-off.🤖 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/host-support/src/registration.rs` around lines 339 - 351, Optionally harden the key-write path by calling sync_all() on the opened file after write_all() and before returning from the surrounding function, while retaining the existing flush and permission ordering.bluefield/charts/nico-fmds/tests/node_tokens_test.yaml (2)
113-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: hoist the repeated
setblock to suite level.
image.repository/image.tagare set identically in all six cases. helm-unittest supports a suite-levelset:, which would leave each case setting only the values it actually varies (useNodeTokens,certsDir,rootCaFile) and make the intent of each case read at a glance.🤖 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 `@bluefield/charts/nico-fmds/tests/node_tokens_test.yaml` around lines 113 - 156, Hoist the repeated image.repository and image.tag values from the six test cases into a suite-level set block in the node token tests. Remove those duplicate image settings from each case, leaving only scenario-specific values such as useNodeTokens, certsDir, and rootCaFile.
60-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
nico-agent-runvolume, not just its mount.Lines 52-56 verify the mount and lines 60-66 verify the
nico-certs-pubvolume, but nothing asserts thatnico-agent-runis actually defined inspec.template.spec.volumes. A template edit that dropped the volume while keeping the mount yields a Pod spec the API server rejects, and this suite would still pass.💚 Close the gap
- contains: path: spec.template.spec.volumes content: name: nico-certs-pub hostPath: path: /opt/forge/pub type: DirectoryOrCreate + - contains: + path: spec.template.spec.volumes + content: + name: nico-agent-run + hostPath: + path: /opt/forge/run + type: DirectoryOrCreate🤖 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 `@bluefield/charts/nico-fmds/tests/node_tokens_test.yaml` around lines 60 - 66, Add a `contains` assertion in the node token test for `spec.template.spec.volumes` that verifies the `nico-agent-run` volume definition, alongside the existing `nico-certs-pub` assertion. Match the expected volume fields from the chart template so the test detects a mount without its corresponding volume.bluefield/charts/nico-dpu-agent/templates/daemonset.yaml (1)
134-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard against a nil
nodeAuth.
.Values.nodeAuth.audienceerrors outright if an operator setsnodeAuth: null(a common way to "unset" a block in an overlay). This chart already defends against exactly that forbootstrapCaon line 1 viadefault (dict); mirroring it here costs nothing.♻️ Defensive dereference
- {{- with .Values.nodeAuth.audience }} + {{- with (get (default (dict) .Values.nodeAuth) "audience") }}🤖 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 `@bluefield/charts/nico-dpu-agent/templates/daemonset.yaml` at line 134, Guard the nodeAuth lookup in the audience block by applying the chart’s existing default-dictionary pattern before accessing audience, so nodeAuth: null safely behaves as an unset block. Update the with expression around .Values.nodeAuth.audience while preserving the current audience rendering for configured values.crates/rpc/src/node_token_socket.rs (1)
224-281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the refresh transition, not just the steady states.
The four cases assert first-fetch, absent-socket, redaction, and margin rejection, but the re-fetch arithmetic in
refresh_loop(lines 149-157) — the part that decides when a cached token is replaced — has no coverage. A server whoseexpires_atadvances between calls would exercise it directly and guard thesaturating_sub/.max(RETRY_DELAY)boundary.🤖 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/rpc/src/node_token_socket.rs` around lines 224 - 281, The tests cover initial fetch and expiration handling but not token replacement during refresh. Add a test near the existing SocketTokenSource tests that serves a token with an advancing expires_at, waits for the refresh loop to replace the cached value, and asserts the newer token is returned; exercise the refresh_loop scheduling path including the saturating_sub and RETRY_DELAY minimum boundary.crates/api-core/src/node_auth.rs (2)
238-248: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNegative node-auth outcomes are DEBUG-only across both new decision points. The pre-existing mTLS path emits a counted
ClientCertRejectedEvent, but the two paths this PR adds — a bearer token failing validation, and a machine cert being gated away bymtls_enabled = false— leave nothing but a debug line. During the mTLS→JWT cutover these are precisely the signals distinguishing "migration proceeding" from "fleet locked out", and both stem from one missing instrumentation decision.
crates/api-core/src/node_auth.rs#L238-L248: emit acarbide_instrument::Eventalongside the debug log, withRejectReasonas a bounded#[label]and the error text in#[context].crates/authn/src/middleware.rs#L639-L666: count machine principals dropped by the gate, so a cutover with nodes still presenting certs is observable rather than inferred from DEBUG output.As per coding guidelines, "declare and emit a
carbide_instrument::Eventwhen an event deserves a count, rate, or duration", keeping#[label]values bounded and high-cardinality detail in#[context].🤖 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/node_auth.rs` around lines 238 - 248, In crates/api-core/src/node_auth.rs lines 238-248, update NodeJwtValidator::spiffe_id_from_bearer to emit a carbide_instrument::Event when validation fails, using a bounded #[label] for RejectReason and placing the error text in #[context] alongside the existing debug log. In crates/authn/src/middleware.rs lines 639-666, add event instrumentation for machine principals rejected because mtls_enabled is false, using the same bounded-label and contextual-detail conventions.Source: Coding guidelines
412-479: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winThe
sub-mismatch branch is untested.
SubjectMismatch(line 231-233) is what stops a node from claiming another machine's identity through an attacker-controlled claim — the single most consequential branch invalidate, and currently unexercised. A token minted from cert A but carrying cert B's SPIFFE URI insubwould pin it. An expired-token case would likewise close out theexppath, which today is only covered indirectly through the TTL cap.Given these cases share one operation with differing inputs, a table would fit: as per coding guidelines, "use tables whenever multiple tests call the same operation with different inputs".
🤖 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/node_auth.rs` around lines 412 - 479, Extend the token validation tests around spiffe_id_from_bearer to cover mismatched and expired claims: mint a token with one certificate while setting sub to another certificate’s SPIFFE URI and assert rejection, and create an otherwise valid token with an expired exp claim and assert rejection. Consolidate these cases with the existing same-operation tests using a table-driven test structure where practical, while preserving the current trusted, untrusted-CA, malformed, and lifetime-cap coverage.Source: Coding guidelines
crates/agent/src/local_api.rs (1)
156-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the socket-readiness poll loop.
The same bounded wait appears three times with only the predicate differing. A small
async fn wait_until(mut ready: impl FnMut() -> bool)(or await_for_socket(&Path)helper) would remove the repetition and keep the timing policy in one place.Also applies to: 178-183, 213-218
🤖 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/agent/src/local_api.rs` around lines 156 - 162, Extract the repeated bounded readiness polling loops around the current source and the corresponding sections into one async helper, such as wait_until or wait_for_socket, that accepts the differing readiness predicate. Centralize the 100-attempt loop and 20ms delay in that helper, then replace all three duplicated loops while preserving their existing success and timeout behavior.crates/authn/src/middleware.rs (1)
1102-1110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a table for the header-parsing cases.
Three inputs against one total function is the textbook
value_scenarios!shape, and it would make adding the lowercase-scheme and extra-whitespace cases a one-line change each.As per coding guidelines, use "
value_scenarios!for total operations" from thecarbide-test-supporttable-driven helpers.🤖 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/authn/src/middleware.rs` around lines 1102 - 1110, The bearer_token_from_headers_parses_scheme test currently repeats setup and assertions for each input; convert it to a carbide-test-support value_scenarios! table for this total operation. Define scenarios covering absent authorization, valid Bearer, and non-Bearer headers, while preserving the expected outputs and making future lowercase-scheme or whitespace cases easy to add.Source: Coding guidelines
bluefield/charts/nico-fmds/templates/daemonset.yaml (1)
91-98: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSource the node-token socket path from one value.
nico-fmdshardcodes{{ $dir }}/run/agent.sock, while the agent already exposeslocal_api_socket; wiring both to a shared value avoids drift if the bind path is overridden in a deployment.🤖 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 `@bluefield/charts/nico-fmds/templates/daemonset.yaml` around lines 91 - 98, Update the useNodeTokens argument block in the daemonset template to derive --node-token-socket from the agent’s existing local_api_socket value instead of hardcoding {{ $dir }}/run/agent.sock. Reuse the shared value while preserving the current argument behavior and other certificate paths.crates/api-core/src/cfg/file.rs (1)
3573-3593: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a scenario table here, and cover the remaining
validate()branches.Three invocations of the same operation with different inputs is exactly the table-driven case, and the
audience-empty andmax_token_ttl_secbounds branches currently have no coverage at all.♻️ Proposed table-driven form
- /// Disabling both bearer tokens and machine mTLS would lock every node out - /// of the API; validation must refuse the combination, and each mechanism - /// alone must pass. - #[test] - fn node_auth_rejects_all_methods_disabled() { - let both_off = NodeAuthConfig { - enabled: false, - mtls_enabled: false, - ..NodeAuthConfig::default() - }; - assert!(both_off.validate().is_err()); - - assert!(NodeAuthConfig::default().validate().is_ok()); - let jwt_only = NodeAuthConfig { - enabled: true, - mtls_enabled: false, - ..NodeAuthConfig::default() - }; - assert!(jwt_only.validate().is_ok()); - } + /// Disabling both bearer tokens and machine mTLS would lock every node out + /// of the API; validation must refuse that combination, accept either + /// mechanism alone, and reject nonsensical token constraints. + #[test] + fn node_auth_validation_contract() { + value_scenarios!( + run = |cfg: NodeAuthConfig| cfg.validate().is_ok(); + "accepted" { + NodeAuthConfig::default() => true, + NodeAuthConfig { enabled: true, mtls_enabled: false, ..NodeAuthConfig::default() } => true, + } + + "rejected" { + NodeAuthConfig { enabled: false, mtls_enabled: false, ..NodeAuthConfig::default() } => false, + NodeAuthConfig { enabled: true, audience: " ".to_string(), ..NodeAuthConfig::default() } => false, + NodeAuthConfig { enabled: true, max_token_ttl_sec: 0, ..NodeAuthConfig::default() } => false, + NodeAuthConfig { + enabled: true, + max_token_ttl_sec: NODE_AUTH_MAX_TOKEN_TTL_SEC + 1, + ..NodeAuthConfig::default() + } => false, + } + ); + }As per coding guidelines, "Use tables whenever multiple tests call the same operation with different inputs".
🤖 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/cfg/file.rs` around lines 3573 - 3593, Refactor node_auth_rejects_all_methods_disabled into a table-driven test covering the existing all-disabled, default, and JWT-only configurations, and add cases exercising the validate() branches for empty audience and max_token_ttl_sec boundary values. Invoke validate() through the scenario table and assert each expected result.Source: Coding guidelines
🤖 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/api-core/src/listener.rs`:
- Around line 485-498: Update the jwt_roots_reloaded branch in the listener
reload flow to replace tls_acceptor only when get_tls_acceptor returns Some;
retain the existing acceptor when the rebuild returns None. Leave the startup
path and existing task error propagation unchanged, preserving TLS for
subsequent connections after a failed reload.
In `@crates/fmds/src/main.rs`:
- Around line 102-118: Add a validation guard immediately before the
`forge_client_config` match: if `options.node_token_socket` is set while
`options.root_ca` is absent, fail loudly with an `eyre::bail!` error identifying
both flags. Keep the existing match behavior unchanged for valid credential
combinations.
In `@crates/rpc/src/node_jwt.rs`:
- Around line 281-292: Update the documentation for the
NodeTokenProvider::current contract to acknowledge that implementations may
perform the existing lightweight local token refresh/signing work, while
prohibiting blocking or network I/O on the request path. Keep the
cached-token/None behavior and the NodeJwtMinter and SocketTokenSource
references unchanged.
- Around line 240-249: Move the SEC1-to-PKCS#8 re-encoding documentation from
`key_matches_certificate` to `ec_encoding_key`, leaving only the certificate
public-key comparison documentation attached to `key_matches_certificate` and
ensuring `ec_encoding_key` has the appropriate encoding-related doc comment.
In `@crates/rpc/src/node_token_socket.rs`:
- Around line 110-118: Update NodeTokenSocket::current to recover from a
poisoned cached read lock using the repository’s into_inner() convention instead
of converting the error to None. Preserve the existing expiration filtering and
token-cloning behavior after obtaining the guard.
---
Nitpick comments:
In `@bluefield/charts/nico-dpu-agent/templates/daemonset.yaml`:
- Line 134: Guard the nodeAuth lookup in the audience block by applying the
chart’s existing default-dictionary pattern before accessing audience, so
nodeAuth: null safely behaves as an unset block. Update the with expression
around .Values.nodeAuth.audience while preserving the current audience rendering
for configured values.
In `@bluefield/charts/nico-fmds/templates/daemonset.yaml`:
- Around line 91-98: Update the useNodeTokens argument block in the daemonset
template to derive --node-token-socket from the agent’s existing
local_api_socket value instead of hardcoding {{ $dir }}/run/agent.sock. Reuse
the shared value while preserving the current argument behavior and other
certificate paths.
In `@bluefield/charts/nico-fmds/tests/node_tokens_test.yaml`:
- Around line 113-156: Hoist the repeated image.repository and image.tag values
from the six test cases into a suite-level set block in the node token tests.
Remove those duplicate image settings from each case, leaving only
scenario-specific values such as useNodeTokens, certsDir, and rootCaFile.
- Around line 60-66: Add a `contains` assertion in the node token test for
`spec.template.spec.volumes` that verifies the `nico-agent-run` volume
definition, alongside the existing `nico-certs-pub` assertion. Match the
expected volume fields from the chart template so the test detects a mount
without its corresponding volume.
In `@crates/agent/src/local_api.rs`:
- Around line 156-162: Extract the repeated bounded readiness polling loops
around the current source and the corresponding sections into one async helper,
such as wait_until or wait_for_socket, that accepts the differing readiness
predicate. Centralize the 100-attempt loop and 20ms delay in that helper, then
replace all three duplicated loops while preserving their existing success and
timeout behavior.
In `@crates/api-core/src/cfg/file.rs`:
- Around line 3573-3593: Refactor node_auth_rejects_all_methods_disabled into a
table-driven test covering the existing all-disabled, default, and JWT-only
configurations, and add cases exercising the validate() branches for empty
audience and max_token_ttl_sec boundary values. Invoke validate() through the
scenario table and assert each expected result.
In `@crates/api-core/src/node_auth.rs`:
- Around line 238-248: In crates/api-core/src/node_auth.rs lines 238-248, update
NodeJwtValidator::spiffe_id_from_bearer to emit a carbide_instrument::Event when
validation fails, using a bounded #[label] for RejectReason and placing the
error text in #[context] alongside the existing debug log. In
crates/authn/src/middleware.rs lines 639-666, add event instrumentation for
machine principals rejected because mtls_enabled is false, using the same
bounded-label and contextual-detail conventions.
- Around line 412-479: Extend the token validation tests around
spiffe_id_from_bearer to cover mismatched and expired claims: mint a token with
one certificate while setting sub to another certificate’s SPIFFE URI and assert
rejection, and create an otherwise valid token with an expired exp claim and
assert rejection. Consolidate these cases with the existing same-operation tests
using a table-driven test structure where practical, while preserving the
current trusted, untrusted-CA, malformed, and lifetime-cap coverage.
In `@crates/authn/src/middleware.rs`:
- Around line 1102-1110: The bearer_token_from_headers_parses_scheme test
currently repeats setup and assertions for each input; convert it to a
carbide-test-support value_scenarios! table for this total operation. Define
scenarios covering absent authorization, valid Bearer, and non-Bearer headers,
while preserving the expected outputs and making future lowercase-scheme or
whitespace cases easy to add.
In `@crates/host-support/src/registration.rs`:
- Around line 339-351: Optionally harden the key-write path by calling
sync_all() on the opened file after write_all() and before returning from the
surrounding function, while retaining the existing flush and permission
ordering.
In `@crates/rpc/src/node_token_socket.rs`:
- Around line 224-281: The tests cover initial fetch and expiration handling but
not token replacement during refresh. Add a test near the existing
SocketTokenSource tests that serves a token with an advancing expires_at, waits
for the refresh loop to replace the cached value, and asserts the newer token is
returned; exercise the refresh_loop scheduling path including the saturating_sub
and RETRY_DELAY minimum boundary.
🪄 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: f7817634-3115-42ad-8fd7-31aa1c02b0f0
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockrest-api/proto/core/gen/v1/agent_local_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/agent_local_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (43)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/tests/node_auth_audience_test.yamlbluefield/charts/nico-dpu-agent/values.yamlbluefield/charts/nico-fmds/templates/daemonset.yamlbluefield/charts/nico-fmds/tests/node_tokens_test.yamlbluefield/charts/nico-fmds/values.yamlcrates/agent/Cargo.tomlcrates/agent/example_agent_config.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/local_api.rscrates/agent/src/tests/common/mod.rscrates/api-core/src/api.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/lib.rscrates/api-core/src/listener.rscrates/api-core/src/node_auth.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/builder.rscrates/api-core/src/test_support/default_config.rscrates/authn/Cargo.tomlcrates/authn/src/middleware.rscrates/fmds/src/cfg.rscrates/fmds/src/main.rscrates/host-support/src/agent_config.rscrates/host-support/src/registration.rscrates/host-support/test/min_agent_config/output.tomlcrates/rpc/Cargo.tomlcrates/rpc/build.rscrates/rpc/proto/agent_local.protocrates/rpc/src/forge_tls_client.rscrates/rpc/src/lib.rscrates/rpc/src/node_jwt.rscrates/rpc/src/node_token_socket.rscrates/rpc/src/protos/mod.rscrates/scout/src/cfg/command_line.rscrates/scout/src/client.rsdeploy/nico-base/api/config-files/nico-api-config.tomldocs/design/machine-identity/node-auth-jwt.mdhelm/charts/nico-api/files/carbide-api-config.tomlrest-api/proto/core/src/v1/agent_local_nico.proto
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/api-core/src/test_support/builder.rs
- crates/authn/Cargo.toml
- bluefield/charts/nico-dpu-agent/tests/node_auth_audience_test.yaml
- bluefield/charts/nico-fmds/values.yaml
- crates/agent/example_agent_config.toml
- docs/design/machine-identity/node-auth-jwt.md
| if jwt_roots_reloaded { | ||
| tls_acceptor = tokio::task::Builder::new() | ||
| .name("get_tls_acceptor refresh") | ||
| .spawn_blocking({ | ||
| let tls_config = tls_config.clone(); | ||
| move || get_tls_acceptor(&tls_config) | ||
| }) | ||
| // Safety: spawn_blocking only returns Error if run outside the tokio runtime | ||
| .expect("Failed to spawn blocking task") | ||
| .await | ||
| // Safety: Awaiting a JoinHandle only fails if the task panicked, and we want to | ||
| // propagate panics | ||
| .expect("task panicked"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
A failed acceptor rebuild silently downgrades the TLS listener to plaintext — and node-auth makes that costly.
The reload ordering correctly protects the JWT-roots-fail case. The inverse is not protected: get_tls_acceptor returns Option, and on a mid-write identity file or unreadable key it yields None, which is assigned straight into tls_acceptor at line 486. The accept path then takes the else branch at line 555 and serves the connection in the clear.
With node-auth enabled this is no longer merely an availability problem. The (Some(_), false) guard at lines 345-349 is evaluated once at startup, so the middleware keeps its bearer authenticator installed and would validate tokens arriving over a plaintext connection — exactly the outcome that guard exists to prevent. The validator has also already advanced to the new roots, so the pair the surrounding comment works so hard to keep aligned ends up misaligned anyway.
Keeping the previous acceptor on a failed rebuild preserves the stated invariant in both directions.
🔒️ Preserve the previous acceptor when the rebuild fails
if jwt_roots_reloaded {
- tls_acceptor = tokio::task::Builder::new()
+ let rebuilt = tokio::task::Builder::new()
.name("get_tls_acceptor refresh")
.spawn_blocking({
let tls_config = tls_config.clone();
move || get_tls_acceptor(&tls_config)
})
// Safety: spawn_blocking only returns Error if run outside the tokio runtime
.expect("Failed to spawn blocking task")
.await
// Safety: Awaiting a JoinHandle only fails if the task panicked, and we want to
// propagate panics
.expect("task panicked");
+ match rebuilt {
+ Some(acceptor) => tls_acceptor = Some(acceptor),
+ // Never drop to plaintext on a transient read/parse
+ // failure: keep serving on the previous identity.
+ None => tracing::warn!(
+ "could not rebuild the TLS acceptor; keeping the previous one"
+ ),
+ }
}Note this leaves the startup path (line 291-295) unchanged; a None acceptor there for ApiListenMode::Tls deserves its own look, but it is outside this diff.
📝 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.
| if jwt_roots_reloaded { | |
| tls_acceptor = tokio::task::Builder::new() | |
| .name("get_tls_acceptor refresh") | |
| .spawn_blocking({ | |
| let tls_config = tls_config.clone(); | |
| move || get_tls_acceptor(&tls_config) | |
| }) | |
| // Safety: spawn_blocking only returns Error if run outside the tokio runtime | |
| .expect("Failed to spawn blocking task") | |
| .await | |
| // Safety: Awaiting a JoinHandle only fails if the task panicked, and we want to | |
| // propagate panics | |
| .expect("task panicked"); | |
| } | |
| if jwt_roots_reloaded { | |
| let rebuilt = tokio::task::Builder::new() | |
| .name("get_tls_acceptor refresh") | |
| .spawn_blocking({ | |
| let tls_config = tls_config.clone(); | |
| move || get_tls_acceptor(&tls_config) | |
| }) | |
| // Safety: spawn_blocking only returns Error if run outside the tokio runtime | |
| .expect("Failed to spawn blocking task") | |
| .await | |
| // Safety: Awaiting a JoinHandle only fails if the task panicked, and we want to | |
| // propagate panics | |
| .expect("task panicked"); | |
| match rebuilt { | |
| Some(acceptor) => tls_acceptor = Some(acceptor), | |
| // Never drop to plaintext on a transient read/parse | |
| // failure: keep serving on the previous identity. | |
| None => tracing::warn!( | |
| "could not rebuild the TLS acceptor; keeping the previous one" | |
| ), | |
| } | |
| } |
🤖 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/listener.rs` around lines 485 - 498, Update the
jwt_roots_reloaded branch in the listener reload flow to replace tls_acceptor
only when get_tls_acceptor returns Some; retain the existing acceptor when the
rebuild returns None. Leave the startup path and existing task error propagation
unchanged, preserving TLS for subsequent connections after a failed reload.
| let forge_client_config = match &options.root_ca { | ||
| Some(root_ca) if client_cert.is_some() || options.node_token_socket.is_some() => { | ||
| let mut config = ForgeClientConfig::new(root_ca.clone(), client_cert); | ||
| if let Some(socket) = &options.node_token_socket { | ||
| tracing::info!( | ||
| socket = %socket, | ||
| "fetching node-auth bearer tokens from the dpu-agent local API" | ||
| ); | ||
| // Token mode usually runs without a client cert, which would | ||
| // otherwise leave the channel on the dummy TLS verifier. The | ||
| // bearer token is the client credential; the server still has | ||
| // to prove itself against the root CA. | ||
| config = config | ||
| .require_tls_enforcement() | ||
| .with_token_provider(SocketTokenSource::spawn(socket.clone())); | ||
| } | ||
| Some(Arc::new(config)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
A --node-token-socket without --root-ca degrades silently.
Lines 87-101 rightly treat half a credential as a deployment bug and bail. The new guard on line 103, however, requires root_ca, so --node-token-socket supplied alone falls through to the generic "No TLS credentials provided" warning and phone_home is quietly disabled — the same class of misconfiguration, handled inconsistently. Failing loudly here would keep the two paths symmetric.
🛡️ Fail on a token socket configured without a trust anchor
let forge_client_config = match &options.root_ca {
Some(root_ca) if client_cert.is_some() || options.node_token_socket.is_some() => {Add before the match:
if options.root_ca.is_none() && options.node_token_socket.is_some() {
eyre::bail!("--node-token-socket was provided without --root-ca");
}📝 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.
| let forge_client_config = match &options.root_ca { | |
| Some(root_ca) if client_cert.is_some() || options.node_token_socket.is_some() => { | |
| let mut config = ForgeClientConfig::new(root_ca.clone(), client_cert); | |
| if let Some(socket) = &options.node_token_socket { | |
| tracing::info!( | |
| socket = %socket, | |
| "fetching node-auth bearer tokens from the dpu-agent local API" | |
| ); | |
| // Token mode usually runs without a client cert, which would | |
| // otherwise leave the channel on the dummy TLS verifier. The | |
| // bearer token is the client credential; the server still has | |
| // to prove itself against the root CA. | |
| config = config | |
| .require_tls_enforcement() | |
| .with_token_provider(SocketTokenSource::spawn(socket.clone())); | |
| } | |
| Some(Arc::new(config)) | |
| if options.root_ca.is_none() && options.node_token_socket.is_some() { | |
| eyre::bail!("--node-token-socket was provided without --root-ca"); | |
| } | |
| let forge_client_config = match &options.root_ca { | |
| Some(root_ca) if client_cert.is_some() || options.node_token_socket.is_some() => { | |
| let mut config = ForgeClientConfig::new(root_ca.clone(), client_cert); | |
| if let Some(socket) = &options.node_token_socket { | |
| tracing::info!( | |
| socket = %socket, | |
| "fetching node-auth bearer tokens from the dpu-agent local API" | |
| ); | |
| // Token mode usually runs without a client cert, which would | |
| // otherwise leave the channel on the dummy TLS verifier. The | |
| // bearer token is the client credential; the server still has | |
| // to prove itself against the root CA. | |
| config = config | |
| .require_tls_enforcement() | |
| .with_token_provider(SocketTokenSource::spawn(socket.clone())); | |
| } | |
| Some(Arc::new(config)) |
🤖 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/fmds/src/main.rs` around lines 102 - 118, Add a validation guard
immediately before the `forge_client_config` match: if
`options.node_token_socket` is set while `options.root_ca` is absent, fail
loudly with an `eyre::bail!` error identifying both flags. Keep the existing
match behavior unchanged for valid credential combinations.
| /// Builds an ES256 signing key from the client key PEM. Vault-issued machine | ||
| /// keys are SEC1 (`BEGIN EC PRIVATE KEY`), which `jsonwebtoken` cannot load | ||
| /// directly, so those are re-encoded to PKCS#8 first. | ||
| /// Whether `key_pem`'s public half is the one certified by `leaf_der`. | ||
| /// | ||
| /// Both encodings the node may hold are accepted: Vault issues SEC1 ("EC | ||
| /// PRIVATE KEY") and renewal may leave PKCS#8. The comparison is on the | ||
| /// uncompressed SEC1 point, which is exactly what an EC `SubjectPublicKeyInfo` | ||
| /// carries. | ||
| fn key_matches_certificate(key_pem: &str, leaf_der: &[u8]) -> Result<bool, NodeJwtError> { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc comment is attached to the wrong function.
The paragraph describing SEC1-to-PKCS#8 re-encoding documents ec_encoding_key (Line 268), but sits on key_matches_certificate, which then carries two unrelated doc blocks while ec_encoding_key is left undocumented.
🧹 Proposed relocation
-/// Builds an ES256 signing key from the client key PEM. Vault-issued machine
-/// keys are SEC1 (`BEGIN EC PRIVATE KEY`), which `jsonwebtoken` cannot load
-/// directly, so those are re-encoded to PKCS#8 first.
/// Whether `key_pem`'s public half is the one certified by `leaf_der`.
///
/// Both encodings the node may hold are accepted: Vault issues SEC1 ("EC
/// PRIVATE KEY") and renewal may leave PKCS#8. The comparison is on the
/// uncompressed SEC1 point, which is exactly what an EC `SubjectPublicKeyInfo`
/// carries.
fn key_matches_certificate(key_pem: &str, leaf_der: &[u8]) -> Result<bool, NodeJwtError> {Then, above Line 268:
+/// Builds an ES256 signing key from the client key PEM. Vault-issued machine
+/// keys are SEC1 (`BEGIN EC PRIVATE KEY`), which `jsonwebtoken` cannot load
+/// directly, so those are re-encoded to PKCS#8 first.
fn ec_encoding_key(key_pem: &str) -> Result<EncodingKey, NodeJwtError> {📝 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.
| /// Builds an ES256 signing key from the client key PEM. Vault-issued machine | |
| /// keys are SEC1 (`BEGIN EC PRIVATE KEY`), which `jsonwebtoken` cannot load | |
| /// directly, so those are re-encoded to PKCS#8 first. | |
| /// Whether `key_pem`'s public half is the one certified by `leaf_der`. | |
| /// | |
| /// Both encodings the node may hold are accepted: Vault issues SEC1 ("EC | |
| /// PRIVATE KEY") and renewal may leave PKCS#8. The comparison is on the | |
| /// uncompressed SEC1 point, which is exactly what an EC `SubjectPublicKeyInfo` | |
| /// carries. | |
| fn key_matches_certificate(key_pem: &str, leaf_der: &[u8]) -> Result<bool, NodeJwtError> { | |
| /// Whether `key_pem`'s public half is the one certified by `leaf_der`. | |
| /// | |
| /// Both encodings the node may hold are accepted: Vault issues SEC1 ("EC | |
| /// PRIVATE KEY") and renewal may leave PKCS#8. The comparison is on the | |
| /// uncompressed SEC1 point, which is exactly what an EC `SubjectPublicKeyInfo` | |
| /// carries. | |
| fn key_matches_certificate(key_pem: &str, leaf_der: &[u8]) -> Result<bool, NodeJwtError> { |
🤖 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/rpc/src/node_jwt.rs` around lines 240 - 249, Move the SEC1-to-PKCS#8
re-encoding documentation from `key_matches_certificate` to `ec_encoding_key`,
leaving only the certificate public-key comparison documentation attached to
`key_matches_certificate` and ensuring `ec_encoding_key` has the appropriate
encoding-related doc comment.
| /// A source of node-auth bearer tokens for outgoing requests. Implemented by | ||
| /// [`NodeJwtMinter`] (holds the key, signs locally) and | ||
| /// [`SocketTokenSource`](crate::node_token_socket::SocketTokenSource) | ||
| /// (fetches from the dpu-agent's local API — the caller never sees the key). | ||
| /// | ||
| /// `current` runs on the request path, so implementations must be non-blocking: | ||
| /// return a cached token or `None`, never wait on I/O. | ||
| pub trait NodeTokenProvider: Send + Sync + std::fmt::Debug { | ||
| /// Returns a currently-valid token, or `None` if one isn't available | ||
| /// (the request then proceeds with whatever else the channel carries). | ||
| fn current(&self) -> Option<String>; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The stated trait contract is stronger than the key-holding implementation delivers.
"never wait on I/O" is not what NodeJwtMinter::current does: on a cold or near-expiry cache it performs two synchronous std::fs::read calls plus an ECDSA signature, on the request thread. The cost is negligible (local files, roughly once per token lifetime), so the implementation is fine — the contract is what should be corrected, before someone writes a provider that genuinely blocks on a network call and believes they are within spec.
📝 Proposed wording
-/// `current` runs on the request path, so implementations must be non-blocking:
-/// return a cached token or `None`, never wait on I/O.
+/// `current` runs on the request path, so implementations must never block on
+/// network I/O or await a remote refresh: serve a cached token, or `None` and
+/// refresh out of band. Cheap local work on the re-mint path is acceptable —
+/// [`NodeJwtMinter`] re-reads its cert/key files and signs, roughly once per
+/// token lifetime.📝 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.
| /// A source of node-auth bearer tokens for outgoing requests. Implemented by | |
| /// [`NodeJwtMinter`] (holds the key, signs locally) and | |
| /// [`SocketTokenSource`](crate::node_token_socket::SocketTokenSource) | |
| /// (fetches from the dpu-agent's local API — the caller never sees the key). | |
| /// | |
| /// `current` runs on the request path, so implementations must be non-blocking: | |
| /// return a cached token or `None`, never wait on I/O. | |
| pub trait NodeTokenProvider: Send + Sync + std::fmt::Debug { | |
| /// Returns a currently-valid token, or `None` if one isn't available | |
| /// (the request then proceeds with whatever else the channel carries). | |
| fn current(&self) -> Option<String>; | |
| } | |
| /// A source of node-auth bearer tokens for outgoing requests. Implemented by | |
| /// [`NodeJwtMinter`] (holds the key, signs locally) and | |
| /// [`SocketTokenSource`](crate::node_token_socket::SocketTokenSource) | |
| /// (fetches from the dpu-agent's local API — the caller never sees the key). | |
| /// | |
| /// `current` runs on the request path, so implementations must never block on | |
| /// network I/O or await a remote refresh: serve a cached token, or `None` and | |
| /// refresh out of band. Cheap local work on the re-mint path is acceptable — | |
| /// [`NodeJwtMinter`] re-reads its cert/key files and signs, roughly once per | |
| /// token lifetime. | |
| pub trait NodeTokenProvider: Send + Sync + std::fmt::Debug { | |
| /// Returns a currently-valid token, or `None` if one isn't available | |
| /// (the request then proceeds with whatever else the channel carries). | |
| fn current(&self) -> Option<String>; | |
| } |
🤖 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/rpc/src/node_jwt.rs` around lines 281 - 292, Update the documentation
for the NodeTokenProvider::current contract to acknowledge that implementations
may perform the existing lightweight local token refresh/signing work, while
prohibiting blocking or network I/O on the request path. Keep the
cached-token/None behavior and the NodeJwtMinter and SocketTokenSource
references unchanged.
| fn current(&self) -> Option<String> { | ||
| let now = unix_now()?; | ||
| self.cached | ||
| .read() | ||
| .ok()? | ||
| .as_ref() | ||
| .filter(|(_, expires_at)| *expires_at > now + REFRESH_MARGIN_SECS / 2) | ||
| .map(|(token, _)| token.clone()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Recover from lock poisoning rather than degrading to “no token” permanently.
self.cached.read().ok()? converts a poisoned lock into an indefinite None, which silently strips the bearer header from every subsequent request. The rest of this feature (node_auth.rs, authn/src/middleware.rs) deliberately recovers with into_inner(); matching that keeps the failure mode consistent and non-terminal.
♻️ Align with the repository's poison-recovery convention
self.cached
.read()
- .ok()?
+ .unwrap_or_else(|poisoned| poisoned.into_inner())
.as_ref()📝 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.
| fn current(&self) -> Option<String> { | |
| let now = unix_now()?; | |
| self.cached | |
| .read() | |
| .ok()? | |
| .as_ref() | |
| .filter(|(_, expires_at)| *expires_at > now + REFRESH_MARGIN_SECS / 2) | |
| .map(|(token, _)| token.clone()) | |
| } | |
| fn current(&self) -> Option<String> { | |
| let now = unix_now()?; | |
| self.cached | |
| .read() | |
| .unwrap_or_else(|poisoned| poisoned.into_inner()) | |
| .as_ref() | |
| .filter(|(_, expires_at)| *expires_at > now + REFRESH_MARGIN_SECS / 2) | |
| .map(|(token, _)| token.clone()) | |
| } |
🤖 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/rpc/src/node_token_socket.rs` around lines 110 - 118, Update
NodeTokenSocket::current to recover from a poisoned cached read lock using the
repository’s into_inner() convention instead of converting the error to None.
Preserve the existing expiration filtering and token-cloning behavior after
obtaining the guard.
c487a0e to
a18fe9d
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4373.docs.buildwithfern.com/infra-controller |
|
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. |
8d886d3 to
bac16c4
Compare
NVIDIA#355) Nodes authenticate to the API with short-lived ES256 JWTs signed by the private key of their existing mTLS client certificate, carrying the cert chain in the token's x5c header. The API verifies that chain against the same root CA its TLS listener already trusts and maps the leaf's SPIFFE SAN through the existing SpiffeContext, so the machine principal and RBAC are unchanged. No new key material, no server-side signing key, no issuance or refresh RPCs. On a DPU only the dpu-agent holds the machine key. It serves AgentLocal/GetNodeToken over a unix socket, so co-located services get tokens rather than the key: token-mode fmds pods mount that socket plus a trust-anchor directory the agent publishes, and never reference the credentials volume. The socket's directory must be dedicated to it — created 0700, or refused if it holds anything else — since the directory is what closes the window between bind and the socket's own chmod. fmds token mode follows [node_auth] enabled, so with node-auth off the chart renders as before. Configuration is [node_auth]: enabled (accept bearer JWTs, requires a TLS listener), mtls_enabled (machine client-cert authn, disableable once the fleet presents tokens), audience and max_token_ttl_sec. Both switches off is rejected at startup. The audience must agree between the API and each node, and is validated on the node whether it arrives by flag or by config file. Bearer tokens never travel in the clear. The API refuses to accept them on a non-TLS listener and keeps its previous TLS acceptor if a rebuild fails, rather than falling back to plaintext while the authenticator stays armed. Clients enforce server-certificate validation whenever a token provider is attached, and refuse a non-HTTPS endpoint outright. Rotation is covered on both sides: the validator reloads its trust anchors on the listener's client-CA refresh, and the minter checks its key against the certified public key before signing, so neither CA rotation nor certificate renewal can lock a node out. The machine key is written 0600. Design doc: docs/design/machine-identity/node-auth-jwt.md Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bac16c4 to
e907b72
Compare
|
Superseded by #4718, opened from the fork this work now lives on. Same change plus the review fixes from this thread — the Thanks to everyone who reviewed here; the findings carried over rather than being lost. |
2 participants
Scout and the DPU-agent authenticate to the API with mTLS client certificates,
which means every process that needs to call the API must hold the machine's
private key. On a DPU that includes co-located DPF services such as fmds, so
the key gets mounted into more containers than strictly need it. Removing the
per-node key entirely is a longer road; this is the step that stops it from
spreading, and starts moving node auth off mTLS.
Nodes now sign short-lived (5 minute) ES256 JWTs with the private key of the
mTLS client certificate they already have, and carry the certificate chain in
the token's
x5cheader. The API verifies that chain against the same root CAits TLS listener already trusts for client certs, verifies the signature with
the verified leaf's key, enforces
exp/iat/audplus a bounded lifetime,and maps the leaf's SPIFFE URI SAN through the same
SpiffeContextas mTLScerts. A JWT and a client cert for the same machine therefore produce a
byte-identical principal, and RBAC is untouched.
This is the simple variant of the design: no server-side signing key, no key
storage, no issuance or refresh RPCs. Clients re-mint locally, and key rotation
rides the existing client-certificate renewal. An earlier server-issued design
is recorded in the design doc as the fallback if per-node keys are ever removed.
Because the token is a bearer credential rather than a channel credential, the
agent can also broker tokens to co-located services over a Unix socket. fmds
can then run without the machine key mounted at all, which is what closes the
key-spreading problem above.
Everything is off by default (
[node_auth] enabled = false) and the twomechanisms run side by side when it is enabled, so rollout order does not
matter: a server with node-auth disabled ignores the bearer header, and a node
that cannot mint yet simply sends no header.
Related issues
Fixes #355
Part of the Vault-elimination epic #195.
Type of Change
Breaking Changes
Testing
Chart tests (helm unittest) assert the security property of token mode
directly: the credentials directory is absent from the pod's volumes and from
both containers, with the mount counts pinned so it cannot be reintroduced
unnoticed. Note that CI's
helm-validatestep runslintandtemplateonly-- it does not execute chart tests -- so these were run locally via the
helmunittest/helm-unittestimage.Unit tests cover the validator end to end against a test PKI: a client-minted
token round-tripping to its certificate's SPIFFE URI, rejection of garbage,
missing chains, untrusted CAs and over-long lifetimes, a configured audience
round-tripping while the default is refused, client-CA rotation being honored
after a refresh, and a corrupt bundle leaving the previous trust anchors in
place. The authn middleware has tests for bearer principals with and without an
authenticator configured and for
mtls_enabled = falsesuppressing machinecert principals while leaving service certs alone. There are also tests for the
agent's local API socket (a key-less consumer obtaining a token through it, and
the socket being root-only) and for TLS enforcement surviving a token-only
client config.
Not manually exercised on hardware. The DPF token-mode path in particular
(chart mounts, the agent socket inside a DPU) has only been validated by
rendering the charts and by unit tests.
Additional Notes
Suggested reading order:
docs/design/machine-identity/node-auth-jwt.mdfirst —it covers the trust model, the new-DPU-to-first-authorized-call walkthrough, and
the JWT best-practice checklist — then
crates/api-core/src/node_auth.rsforvalidation and
crates/rpc/src/node_jwt.rsfor minting.Operational notes for reviewers:
[node_auth] audiencemust be kept in sync between the API and its nodes. TheAPI templates it onto DPF-deployed agents automatically; Scout and
non-DPF agents take it from their own config (
--node-auth-audience/[forge-system] node-auth-audience).enabled = falseandmtls_enabled = false,and refuses to accept bearer tokens on a non-TLS listener.
five-minute tick, so a client-CA rotation does not require an API restart.
One known follow-up, deliberately out of scope here:
parsing) and runs on every request, where mTLS amortizes the equivalent over
a long-lived connection. Tokens repeat for their whole 5-minute life, so
caching successful validations would remove nearly all of it. Filed as Cache validated node-auth JWTs to cut per-request verification cost #4388
with the measurement and the invalidation constraints.
Two issues raised in review during this PR are fixed rather than deferred: the
root CA is now published to a key-free directory and mounted as a directory (an
earlier
subPathmount excluded the key but pinned the inode, so a rotationnever reached a running pod), and the minter now refuses to sign when the
certificate and key on disk disagree, which could otherwise happen mid-renewal
and cache an unusable token.