Authenticate Scout and DPU-agent with self-signed bearer JWTs - #4718
Authenticate Scout and DPU-agent with self-signed bearer JWTs#4718Sinck wants to merge 1 commit into
Conversation
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 — a real directory this agent owns, created 0700, refused if it holds anything else — since the directory is what closes the window between bind and the socket's own chmod, and only an actual socket is ever unlinked from it. 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>
Summary by CodeRabbit
WalkthroughThe change adds configurable node-auth JWTs across the API, DPU agent, Scout, FMDS, RPC clients, authentication middleware, local Unix-socket brokering, Helm deployments, and configuration files. Existing mTLS behavior remains the default. ChangesNode-auth contracts and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Scout
participant ForgeClient
participant API
participant NodeJwtValidator
Scout->>ForgeClient: Configure node JWT audience
ForgeClient->>ForgeClient: Mint or retrieve cached JWT
ForgeClient->>API: HTTPS request with bearer token
API->>NodeJwtValidator: Validate JWT and x5c chain
NodeJwtValidator-->>API: SPIFFE machine principal
API-->>ForgeClient: Authenticated response
sequenceDiagram
participant FMDS
participant SocketTokenSource
participant AgentLocalService
participant NodeJwtMinter
FMDS->>SocketTokenSource: Request cached token
SocketTokenSource->>AgentLocalService: GetNodeToken over Unix socket
AgentLocalService->>NodeJwtMinter: Mint current token
NodeJwtMinter-->>AgentLocalService: JWT and expiration
AgentLocalService-->>SocketTokenSource: Token response
SocketTokenSource-->>FMDS: Bearer token
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.72.0)Trivy execution failed: 2026-08-07T20:23:41Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: ansible scan error: fs filter error: fs filter error: walk error range error: stat .coderabbit-opengrep-fallback.4d10109b-565e-465b-b590-d30dbc0e73cc.yml: no such file or directory: range error: stat .coderabbit-opengrep-fallback.4d10109b-565e-465b-b590-d30dbc0e73cc.yml: no such file or directory 🔧 ast-grep (0.45.0)crates/rpc/build.rsast-grep timed out on this file Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/agent/src/lib.rs (1)
366-381: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGate the node-auth provider or document the TLS migration.
with_token_providerenforces TLS, andForgeTlsClient::buildrejects non-HTTPS URLs when a provider is present unlessDISABLE_TLS_ENFORCEMENTis set. The provider is attached even when[node_auth]is disabled, so plaintext deployments, including the example configuration, fail when they build a Forge client. Tests mask this withDISABLE_TLS_ENFORCEMENT. Gate the provider on node-auth configuration, or document this breaking change and update plaintext configurations.🤖 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 366 - 381, The Forge client always attaches the node-auth token provider, causing plaintext deployments to fail when node authentication is disabled. Update the ForgeClientConfig construction around NodeJwtMinter and with_token_provider to add the provider only when the agent’s node-auth configuration is enabled; otherwise preserve the client configuration without a token provider.
🧹 Nitpick comments (7)
crates/api-core/src/node_auth.rs (1)
53-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestrict these declarations to the crate.
node_authis a private module. The supplied production caller is insidecrates/api-core. Changepubtopub(crate)forNodeAuthError,NodeJwtValidator, andfrom_root_ca_fileunless an external caller requires wider visibility.As per coding guidelines, “keep declarations private by default, widening visibility only for actual callers.”
Also applies to: 99-131
🤖 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 53 - 60, Restrict the node authentication API to crate visibility: change NodeAuthError, NodeJwtValidator, and the from_root_ca_file method to pub(crate), preserving their existing behavior and signatures otherwise. Do not widen visibility unless an actual external caller requires it.Sources: Coding guidelines, Path instructions
crates/api-core/src/cfg/file.rs (2)
1985-1985: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
deny_unknown_fieldsonNodeAuthConfig.
SecretsConfigandCertificatesConfigin this file use#[serde(deny_unknown_fields)], and this file already carries tests asserting that a misspelled key fails to parse.[node_auth]has no such guard. A typo such asmtls_enabld = falseparses cleanly and leaves machine mTLS enabled, so the operator's intent to disable it is discarded without any signal.♻️ Proposed change
-#[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` at line 1985, Update the NodeAuthConfig definition to add Serde’s deny_unknown_fields guard, matching SecretsConfig and CertificatesConfig. Extend the existing configuration parsing tests to verify that an unknown or misspelled [node_auth] key is rejected.
3711-3727: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a scenario table for the validation cases.
The test invokes
validate()three times with differentNodeAuthConfiginputs. The repository convention prefers a table in this shape.Based on path instructions: "Use a table whenever two or more tests invoke the same operation with different inputs, but keep genuinely distinct tests standalone."
🤖 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 3711 - 3727, Refactor node_auth_rejects_all_methods_disabled into a scenario table covering both methods disabled, default configuration, and JWT-only configuration. Iterate over the cases and assert each NodeAuthConfig.validate() result against its expected outcome, preserving the existing assertions and test coverage.Source: Path instructions
crates/authn/src/middleware.rs (3)
683-698: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSuppressed machine certificates are invisible to metrics.
Trace the counting path.
rejectionscollects onlyErrresults fromtry_from_client_certificate. A machine certificate that maps successfully and is then dropped by this filter adds nothing toprincipalsand nothing torejections. At line 700 the request satisfiesprincipals.len() == minted_before, butrejections.first()isNone, soClientCertRejectednever fires.The consequence: after an operator sets
mtls_enabled = false, a node that has not yet migrated to bearer tokens loses its identity with only a per-request DEBUG line as evidence. No counter moves. The cutover is the exact moment when an operator needs a signal that machine certificates are still arriving.Consider emitting an Event with a bounded label for the suppression, so the remaining mTLS population is measurable before and during the cutover. This is deployment-safety tooling rather than a correctness fix, so it can also be deferred to a follow-up.
🤖 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 683 - 698, The machine-certificate suppression in the principal filtering path is not reflected in rejection metrics. Update the filter around try_from_client_certificate and the subsequent ClientCertRejected handling to emit a measurable Event with a bounded label whenever a SpiffeMachineIdentifier is dropped because machine_certs_enabled is false, while preserving the existing filtering behavior.
1001-1012: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe bearer service-identity arm has no test.
The tests exercise
Ok(SpiffeIdClass::Machine(_))thoroughly. The sibling arm at line 632, which producesPrincipal::SpiffeServiceIdentifier, is never reached by any test. A regression that swapped the two arms would pass the current suite.
spiffe_context()already declares/carbide-system/sa/as a service base path, so the addition is a single test using a service URI inFakeAuth.I can generate the test if that is useful.
🤖 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 1001 - 1012, Add a test alongside valid_bearer_token_yields_machine_principal covering a bearer token authenticated by FakeAuth with a service URI under the /carbide-system/sa/ base path. Assert principals_for returns Principal::SpiffeServiceIdentifier for the expected service identity, exercising the bearer service-identity branch.
57-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider narrowing the visibility of the two new fields.
with_bearer_authenticatorandwith_machine_certs_enabledare the only paths the listener uses to set these values. Thepubfields therefore widen the surface beyond the actual callers and permit a caller to bypass the builders. The pre-existingpubfields set a precedent, so this is a judgement call rather than a defect.As per coding guidelines: "Use the narrowest Rust visibility required by actual callers; do not use
pubto suppress dead-code warnings or widen production visibility solely for unit tests."🤖 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 57 - 65, Restrict the visibility of the new bearer_authenticator and machine_certs_enabled fields to the narrowest scope required by their callers, removing pub if external access is unnecessary. Keep with_bearer_authenticator and with_machine_certs_enabled as the supported configuration paths, and preserve the existing behavior and visibility of unrelated fields.Source: Coding guidelines
crates/agent/src/command_line.rs (1)
28-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider hosting this validator once in the
rpccrate.
crates/scout/src/cfg/command_line.rsLines 31-36 contain a byte-identicalnon_blank_audience. Both crates already depend onrpc, which ownsNODE_JWT_AUDIENCE. Placing the validator beside that constant keeps the two flags on one rule and prevents divergence when the rule changes, for example the trimming fix above.Based on learnings from the coding guidelines: "Prefer simple, explicit Rust code" and reuse before new code per the shared Engineering Guidelines.
🤖 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/command_line.rs` around lines 28 - 36, Move the shared non_blank_audience validator from the command-line crates into the rpc crate alongside NODE_JWT_AUDIENCE, expose it for reuse, and update both crates’ command-line argument definitions to reference that single implementation. Remove the duplicate local validators while preserving rejection of empty or whitespace-only values and the existing error 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.
Inline comments:
In `@crates/agent/src/command_line.rs`:
- Around line 31-36: Update non_blank_audience to return the trimmed audience
value after validation instead of the original input, so NodeJwtMinter produces
an aud claim matching the configured [node_auth] audience.
In `@crates/api-core/src/cfg/file.rs`:
- Around line 2025-2027: Update the audience handling in validate so the value
used by NodeJwtValidator::from_root_ca_file and Validation::set_audience is
normalized by trimming surrounding whitespace, or reject any value with leading
or trailing whitespace if validate must retain its &self signature. Preserve the
existing empty-audience error behavior.
In `@crates/api-core/src/dpf_services.rs`:
- Around line 692-707: Derive an effective, validated bearer-authentication
configuration before mandatory_services uses NodeAuthConfig, requiring TLS
termination and a non-whitespace audience; use it consistently for
dpu_agent_service and fmds_service. Update crates/api-core/src/dpf_services.rs
lines 692-707 accordingly,
bluefield/charts/nico-dpu-agent/templates/daemonset.yaml lines 134-139 to render
an optional validated audience only, and
bluefield/charts/nico-fmds/templates/daemonset.yaml lines 56-98 to enable token
mode only from that effective state. Document or reject the TLS requirement in
bluefield/charts/nico-fmds/values.yaml lines 16-26, and add configuration
scenarios covering enabled bearer auth with plaintext TLS and disabled auth with
whitespace-only audience.
In `@crates/api-core/src/listener.rs`:
- Around line 486-548: Update the refresh flow around
node_jwt_validator.refresh_roots and get_tls_acceptor so both refreshed trust
configurations are prepared before either is committed; retain the previous JWT
roots and TLS acceptor whenever either refresh fails, including TLS rebuild
failure after successful JWT reload. Ensure the next retry can rebuild both
together, and add coverage for this failure sequence.
- Around line 353-356: Update listener startup around get_tls_acceptor and the
node_jwt_validator match to build and validate the initial TLS acceptor before
installing bearer authentication. When node authentication is enabled, fail
startup if the acceptor cannot be created; only apply with_bearer_authenticator
when that usable acceptor exists, not merely when tls_config is present. Add
coverage using a valid root CA and an unreadable identity key to verify startup
failure and no plaintext bearer-authenticated listener.
In `@crates/host-support/src/agent_config.rs`:
- Around line 1128-1140: Update the test around ForgeSystemConfig::validate to
use the repository’s table-test helper, preferably check_cases or scenarios!
with Outcome, instead of manually looping over blank values. Represent the
empty, whitespace, and tab inputs as table cases while preserving the expected
validation error and node-auth-audience assertion.
In `@crates/rpc/src/forge_tls_client.rs`:
- Around line 100-105: Require any client configured with node_token_provider to
have enforce_tls enabled, unless DISABLE_TLS_ENFORCEMENT is set; apply this
validation across direct construction, with_token_provider, and HTTPS connection
setup so later mutation cannot bypass it. Preserve existing behavior for
non-token clients, and add a regression test that directly constructs a token
client with enforce_tls false and verifies it is rejected.
In `@rest-api/proto/core/src/v1/agent_local_nico.proto`:
- Around line 23-27: Update the GetNodeToken RPC documentation to state that the
agent may return a cached token and define the renewal threshold at which it
mints a fresh token. Clarify the remaining-validity guarantee callers can rely
on when choosing their refresh interval.
---
Outside diff comments:
In `@crates/agent/src/lib.rs`:
- Around line 366-381: The Forge client always attaches the node-auth token
provider, causing plaintext deployments to fail when node authentication is
disabled. Update the ForgeClientConfig construction around NodeJwtMinter and
with_token_provider to add the provider only when the agent’s node-auth
configuration is enabled; otherwise preserve the client configuration without a
token provider.
---
Nitpick comments:
In `@crates/agent/src/command_line.rs`:
- Around line 28-36: Move the shared non_blank_audience validator from the
command-line crates into the rpc crate alongside NODE_JWT_AUDIENCE, expose it
for reuse, and update both crates’ command-line argument definitions to
reference that single implementation. Remove the duplicate local validators
while preserving rejection of empty or whitespace-only values and the existing
error behavior.
In `@crates/api-core/src/cfg/file.rs`:
- Line 1985: Update the NodeAuthConfig definition to add Serde’s
deny_unknown_fields guard, matching SecretsConfig and CertificatesConfig. Extend
the existing configuration parsing tests to verify that an unknown or misspelled
[node_auth] key is rejected.
- Around line 3711-3727: Refactor node_auth_rejects_all_methods_disabled into a
scenario table covering both methods disabled, default configuration, and
JWT-only configuration. Iterate over the cases and assert each
NodeAuthConfig.validate() result against its expected outcome, preserving the
existing assertions and test coverage.
In `@crates/api-core/src/node_auth.rs`:
- Around line 53-60: Restrict the node authentication API to crate visibility:
change NodeAuthError, NodeJwtValidator, and the from_root_ca_file method to
pub(crate), preserving their existing behavior and signatures otherwise. Do not
widen visibility unless an actual external caller requires it.
In `@crates/authn/src/middleware.rs`:
- Around line 683-698: The machine-certificate suppression in the principal
filtering path is not reflected in rejection metrics. Update the filter around
try_from_client_certificate and the subsequent ClientCertRejected handling to
emit a measurable Event with a bounded label whenever a SpiffeMachineIdentifier
is dropped because machine_certs_enabled is false, while preserving the existing
filtering behavior.
- Around line 1001-1012: Add a test alongside
valid_bearer_token_yields_machine_principal covering a bearer token
authenticated by FakeAuth with a service URI under the /carbide-system/sa/ base
path. Assert principals_for returns Principal::SpiffeServiceIdentifier for the
expected service identity, exercising the bearer service-identity branch.
- Around line 57-65: Restrict the visibility of the new bearer_authenticator and
machine_certs_enabled fields to the narrowest scope required by their callers,
removing pub if external access is unnecessary. Keep with_bearer_authenticator
and with_machine_certs_enabled as the supported configuration paths, and
preserve the existing behavior and visibility of unrelated fields.
🪄 Autofix
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: 904eeb6d-94b3-404c-ac60-4b635a77ed0b
⛔ 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
| fn non_blank_audience(value: &str) -> Result<String, String> { | ||
| if value.trim().is_empty() { | ||
| return Err("node-auth audience must not be empty".to_string()); | ||
| } | ||
| Ok(value.to_string()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return the trimmed audience, not the raw value.
The validator rejects a blank value but accepts " nico-api " and returns it verbatim. That value is stamped into the aud claim by NodeJwtMinter and compared byte-for-byte against the API's [node_auth] audience. Every token is then rejected, which is exactly the silent mismatch this function documents at Line 29. Trim the value before returning it.
🔧 Proposed fix
fn non_blank_audience(value: &str) -> Result<String, String> {
- if value.trim().is_empty() {
+ let trimmed = value.trim();
+ if trimmed.is_empty() {
return Err("node-auth audience must not be empty".to_string());
}
- Ok(value.to_string())
+ Ok(trimmed.to_string())
}📝 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 non_blank_audience(value: &str) -> Result<String, String> { | |
| if value.trim().is_empty() { | |
| return Err("node-auth audience must not be empty".to_string()); | |
| } | |
| Ok(value.to_string()) | |
| } | |
| fn non_blank_audience(value: &str) -> Result<String, String> { | |
| let trimmed = value.trim(); | |
| if trimmed.is_empty() { | |
| return Err("node-auth audience must not be empty".to_string()); | |
| } | |
| Ok(trimmed.to_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/agent/src/command_line.rs` around lines 31 - 36, Update
non_blank_audience to return the trimmed audience value after validation instead
of the original input, so NodeJwtMinter produces an aud claim matching the
configured [node_auth] audience.
| if self.audience.trim().is_empty() { | ||
| return Err(eyre::eyre!("[node_auth] audience must not be empty")); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Store the trimmed audience, or a padded value silently rejects every token.
validate() accepts audience after trim(), but the field keeps its original spacing. NodeJwtValidator::from_root_ca_file passes the untrimmed value to Validation::set_audience, so an audience configured as " nico-api " passes startup validation and then rejects every token the fleet mints. The failure appears only as authentication rejections at runtime.
Normalize the value instead of only inspecting it.
🛠️ Proposed fix to normalize the audience during validation
- pub fn validate(&self) -> eyre::Result<()> {
+ pub fn validate(&mut self) -> eyre::Result<()> {
if !self.enabled && !self.mtls_enabled {
return Err(eyre::eyre!(
"[node_auth] enabled = false and mtls_enabled = false would leave nodes with no \
way to authenticate; enable at least one of bearer tokens or machine mTLS"
));
}
if !self.enabled {
// Remaining checks only constrain token validation.
return Ok(());
}
- if self.audience.trim().is_empty() {
+ let trimmed = self.audience.trim();
+ if trimmed.is_empty() {
return Err(eyre::eyre!("[node_auth] audience must not be empty"));
}
+ if trimmed.len() != self.audience.len() {
+ self.audience = trimmed.to_string();
+ }If a &self signature must be preserved, reject an audience with leading or trailing whitespace instead of accepting it.
📝 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 self.audience.trim().is_empty() { | |
| return Err(eyre::eyre!("[node_auth] audience must not be empty")); | |
| } | |
| let trimmed = self.audience.trim(); | |
| if trimmed.is_empty() { | |
| return Err(eyre::eyre!("[node_auth] audience must not be empty")); | |
| } | |
| if trimmed.len() != self.audience.len() { | |
| self.audience = trimmed.to_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/api-core/src/cfg/file.rs` around lines 2025 - 2027, Update the
audience handling in validate so the value used by
NodeJwtValidator::from_root_ca_file and Validation::set_audience is normalized
by trimming surrounding whitespace, or reject any value with leading or trailing
whitespace if validate must retain its &self signature. Preserve the existing
empty-audience error behavior.
| /// | ||
| /// `node_auth` mirrors the API's `[node_auth]` section: `enabled` switches | ||
| /// fmds to bearer tokens from the dpu-agent's local API, and `audience` is | ||
| /// templated onto the agent so both ends stamp/expect the same `aud` | ||
| /// (issue #355). | ||
| pub fn mandatory_services( | ||
| resolved: &DpfResolvedMandatoryServicesConfig, | ||
| bootstrap_ca: &DpfDpuAgentBootstrapCa, | ||
| node_auth: &NodeAuthConfig, | ||
| ) -> Vec<ServiceDefinition> { | ||
| let mut service_vec = vec![ | ||
| dts_service(&resolved.base.dts), | ||
| doca_hbn_service(&resolved.base.doca_hbn), | ||
| dhcp_server_service(&resolved.base.dhcp_server), | ||
| dpu_agent_service(&resolved.base.dpu_agent, bootstrap_ca), | ||
| fmds_service(&resolved.base.fmds), | ||
| dpu_agent_service(&resolved.base.dpu_agent, bootstrap_ca, &node_auth.audience), | ||
| fmds_service(&resolved.base.fmds, node_auth.enabled), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Derive token deployment mode from validated bearer-authentication state.
mandatory_services uses raw NodeAuthConfig values. This permits configurations where the deployment cannot authenticate.
The API listener refuses bearer authentication when it is not TLS-terminated. If node_auth.enabled = true with a plaintext listener, FMDS drops its mTLS credentials but the API rejects its bearer token. Also, when enabled = false, NodeAuthConfig::validate accepts a whitespace-only audience. Helm renders that value, and the DPU agent rejects the resulting --node-auth-audience argument at startup.
crates/api-core/src/dpf_services.rs#L692-L707: pass an effective bearer-authentication configuration only after startup validates TLS and the audience. Do not setuseNodeTokensfromenabledalone.bluefield/charts/nico-dpu-agent/templates/daemonset.yaml#L134-L139: accept an optional validated audience, so an inactive bearer mode does not render whitespace as a CLI argument.bluefield/charts/nico-fmds/templates/daemonset.yaml#L56-L98: enter token mode only when the effective configuration guarantees that the API listener accepts bearer tokens.bluefield/charts/nico-fmds/values.yaml#L16-L26: document the TLS requirement, or reject the invalid API configuration before deployment generation.
Add configuration scenarios for enabled bearer auth with a plaintext listener and disabled bearer auth with a whitespace-only audience.
📍 Affects 4 files
crates/api-core/src/dpf_services.rs#L692-L707(this comment)bluefield/charts/nico-dpu-agent/templates/daemonset.yaml#L134-L139bluefield/charts/nico-fmds/templates/daemonset.yaml#L56-L98bluefield/charts/nico-fmds/values.yaml#L16-L26
🤖 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 692 - 707, Derive an
effective, validated bearer-authentication configuration before
mandatory_services uses NodeAuthConfig, requiring TLS termination and a
non-whitespace audience; use it consistently for dpu_agent_service and
fmds_service. Update crates/api-core/src/dpf_services.rs lines 692-707
accordingly, bluefield/charts/nico-dpu-agent/templates/daemonset.yaml lines
134-139 to render an optional validated audience only, and
bluefield/charts/nico-fmds/templates/daemonset.yaml lines 56-98 to enable token
mode only from that effective state. Document or reject the TLS requirement in
bluefield/charts/nico-fmds/values.yaml lines 16-26, and add configuration
scenarios covering enabled bearer auth with plaintext TLS and disabled auth with
whitespace-only audience.
| match (&api_service.node_jwt_validator, tls_config.is_some()) { | ||
| (Some(node_jwt_validator), true) => { | ||
| tracing::info!(target: "node_auth", "node-auth: bearer token authentication enabled"); | ||
| layer.with_bearer_authenticator(node_jwt_validator.clone()) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not install bearer authentication unless TLS is active.
tls_config.is_some() only confirms TLS configuration. It does not confirm that get_tls_acceptor() succeeded. If the identity certificate or key cannot load, the listener takes the plaintext branch at lines 605-642 while this middleware still accepts bearer tokens.
When node authentication is enabled, fail startup if the initial TLS acceptor cannot be built. Also gate bearer-authenticator installation on a usable acceptor. Add a test with a valid root CA and an unreadable identity 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 `@crates/api-core/src/listener.rs` around lines 353 - 356, Update listener
startup around get_tls_acceptor and the node_jwt_validator match to build and
validate the initial TLS acceptor before installing bearer authentication. When
node authentication is enabled, fail startup if the acceptor cannot be created;
only apply with_bearer_authenticator when that usable acceptor exists, not
merely when tls_config is present. Add coverage using a valid root CA and an
unreadable identity key to verify startup failure and no plaintext
bearer-authenticated listener.
| let jwt_roots_reloaded = match node_jwt_validator.as_ref() { | ||
| None => true, | ||
| Some(node_jwt_validator) => { | ||
| let validator = node_jwt_validator.clone(); | ||
| let refreshed = tokio::task::Builder::new() | ||
| .name("node_jwt_validator refresh") | ||
| .spawn_blocking(move || validator.refresh_roots()) | ||
| .expect("Failed to spawn blocking task") | ||
| .await | ||
| .expect("task panicked"); | ||
| match refreshed { | ||
| Ok(()) => true, | ||
| Err(error) => { | ||
| tracing::warn!( | ||
| target: "node_auth", | ||
| %error, | ||
| "node-auth: could not reload JWT trust anchors; \ | ||
| keeping the previous TLS and token trust anchors" | ||
| ); | ||
| false | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| if jwt_roots_reloaded { | ||
| let refreshed_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"); | ||
|
|
||
| // `get_tls_acceptor` yields `None` for any failure — | ||
| // an identity PEM caught mid-write by cert-manager, an | ||
| // unreadable key, a CA bundle with nothing parsable in | ||
| // it. Assigning that straight through would drop the | ||
| // listener onto the plaintext branch below while the | ||
| // bearer authenticator, installed once at startup on | ||
| // the premise that this listener terminates TLS, keeps | ||
| // accepting node JWTs — putting them on the wire in the | ||
| // clear. Keep the working acceptor and retry instead: a | ||
| // stale-but-valid one beats no TLS, and rotation leaves | ||
| // ample overlap to pick the new material up. | ||
| match refreshed_acceptor { | ||
| Some(acceptor) => tls_acceptor = Some(acceptor), | ||
| None => { | ||
| // Retry on the next connection rather than | ||
| // waiting out another five-minute window. | ||
| initialize_tls_acceptor = true; | ||
| tracing::error!( | ||
| "could not rebuild the TLS acceptor; \ | ||
| keeping the previous one and retrying" | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Keep JWT and TLS trust anchors synchronized on refresh.
refresh_roots() replaces the JWT verifier before get_tls_acceptor() rebuilds the TLS acceptor. If the rebuild returns None, line 537 retains the old TLS acceptor but the JWT path already trusts the new root bundle.
Build both replacements before committing either one. If either build fails, retain both previous trust configurations. Add a test that forces TLS-acceptor rebuild failure after a successful JWT-root reload.
🤖 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 486 - 548, Update the refresh
flow around node_jwt_validator.refresh_roots and get_tls_acceptor so both
refreshed trust configurations are prepared before either is committed; retain
the previous JWT roots and TLS acceptor whenever either refresh fails, including
TLS rebuild failure after successful JWT reload. Ensure the next retry can
rebuild both together, and add coverage for this failure sequence.
| for blank in ["", " ", "\t"] { | ||
| let config = ForgeSystemConfig { | ||
| node_auth_audience: blank.to_string(), | ||
| ..ForgeSystemConfig::default() | ||
| }; | ||
| let err = config | ||
| .validate() | ||
| .expect_err("a blank audience must be rejected"); | ||
| assert!( | ||
| err.contains("node-auth-audience"), | ||
| "the error should name the field, got: {err}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the repository table-test helper.
Replace the manual loop with check_cases or scenarios! and Outcome. This test invokes the same fallible operation with multiple inputs.
As per coding guidelines, “Use a table whenever two or more tests invoke 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/host-support/src/agent_config.rs` around lines 1128 - 1140, Update the
test around ForgeSystemConfig::validate to use the repository’s table-test
helper, preferably check_cases or scenarios! with Outcome, instead of manually
looping over blank values. Represent the empty, whitespace, and tab inputs as
table cases while preserving the expected validation error and
node-auth-audience assertion.
Source: Coding guidelines
| /// Optional node-auth token provider (issue #355). When set, each request | ||
| /// carries an `Authorization: Bearer <jwt>` — either self-signed with the | ||
| /// client certificate's own private key ([`NodeJwtMinter`]) or fetched | ||
| /// from the dpu-agent's local API (`SocketTokenSource`). Independent of | ||
| /// mTLS: the channel may present a client cert, a token, or both. | ||
| pub node_token_provider: Option<Arc<dyn NodeTokenProvider>>, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce verified TLS for every bearer-token client.
Line 105 exposes node_token_provider, and enforce_tls is also public. A caller can configure a provider with enforce_tls: false, or disable enforcement after with_token_provider.
For an HTTPS URI, Lines 504-506 allow this configuration. Line 615 then selects DummyTlsVerifier. The client can send a bearer token to an endpoint that it did not authenticate.
Reject token clients when enforce_tls is false, unless DISABLE_TLS_ENFORCEMENT is set. Add a regression test that constructs this unsafe configuration directly.
Proposed fix
- if self.forge_client_config.node_token_provider.is_some()
- && uri.scheme() != Some(&tonic::codegen::http::uri::Scheme::HTTPS)
+ if self.forge_client_config.node_token_provider.is_some()
+ && (uri.scheme() != Some(&tonic::codegen::http::uri::Scheme::HTTPS)
+ || !self.forge_client_config.enforce_tls)
&& std::env::var("DISABLE_TLS_ENFORCEMENT").is_err()As per coding guidelines, “Design APIs to be hard to misuse.” As per path instructions, review Rust changes for behavior and security.
Also applies to: 498-512, 864-960
🤖 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 100 - 105, Require any
client configured with node_token_provider to have enforce_tls enabled, unless
DISABLE_TLS_ENFORCEMENT is set; apply this validation across direct
construction, with_token_provider, and HTTPS connection setup so later mutation
cannot bypass it. Preserve existing behavior for non-token clients, and add a
regression test that directly constructs a token client with enforce_tls false
and verifies it is rejected.
Sources: Coding guidelines, Path instructions
| // Returns the current node-auth bearer JWT, minted by the agent from the | ||
| // machine's client-certificate key (issue #355). Callers present it as | ||
| // `Authorization: Bearer <token>` to nico-api and must fetch a fresh one | ||
| // before `expires_at`. Only the agent ever touches the private key. | ||
| rpc GetNodeToken(GetNodeTokenRequest) returns (GetNodeTokenResponse); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the freshness guarantee of the returned token.
The comment instructs callers to fetch a fresh token before expires_at, but it does not state what the agent returns. The agent caches minted tokens, so a caller cannot determine from this contract whether a call made shortly before expiry yields the nearly-expired cached token or a renewed one. A caller that polls at expires_at - 5s and receives the cached token has no remaining margin.
State the cache behaviour and the renewal threshold, so callers can choose a safe refresh interval.
🤖 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 `@rest-api/proto/core/src/v1/agent_local_nico.proto` around lines 23 - 27,
Update the GetNodeToken RPC documentation to state that the agent may return a
cached token and define the renewal threshold at which it mints a fresh token.
Clarify the remaining-validity guarantee callers can rely on when choosing their
refresh interval.
1 participant
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.
There is no server-side signing key, no key storage, and no issuance or refresh
RPCs. Clients re-mint locally, and key rotation rides the existing
client-certificate renewal. The alternatives this displaces — a server-issued
design, a JWKS endpoint, a Kubernetes Secret for the machine key, and others —
are recorded in the design doc under "Designs not used", each with the reason
it lost.
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.
Review findings have been fixed rather than deferred. The root CA is published
to a key-free directory and mounted as a directory (an earlier
subPathmountexcluded the key but pinned the inode, so a rotation never reached a running
pod); the minter refuses to sign when the certificate and key on disk disagree,
which could otherwise happen mid-renewal and cache an unusable token; attaching
a token provider now implies server-certificate validation and a non-HTTPS
endpoint is refused outright, so a bearer token cannot leave the process in the
clear; a failed TLS acceptor rebuild keeps the previous acceptor instead of
serving plaintext while the bearer authenticator stays armed; and the agent's
socket directory must be a real directory it owns, with only an actual socket
ever unlinked from it.
This branch supersedes #4373, which was opened from a fork that is no longer in
use. The content is the same work plus the review fixes above.