feat(nras): audit and measure device attestation#3543
Conversation
|
@coderabbitai full_review, thanks! |
|
ദ്ദി(˵ •̀ ᴗ •́ ˵ ) ✧ ✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
Summary by CodeRabbit
WalkthroughAdds ChangesAttestation observability
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SPDMController
participant Verifier
participant carbide_instrument
SPDMController->>Verifier: dispatch device attestation
Verifier-->>SPDMController: return attestation response
SPDMController->>carbide_instrument: emit AttestationPerformed
carbide_instrument-->>SPDMController: record device type and outcome
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3543.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/nras/src/client.rs (1)
88-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared "send + status-check + body" pattern used by both RED-wrapped calls.
attest_gpu's andfetch_jwks'sred::instrumentedclosures both do the same three steps — send/get, check status, extract text, and map non-OK intoNrasError::Communication— just with a differentRequestBuilder/URL source. A small shared async helper (e.g.async fn response_text_or_err(response: reqwest::Response, context: &str) -> Result<String, NrasError>) called from inside eachred::instrumented(...)block would remove the duplication and centralize any future change to the error-formatting/redaction logic (relevant given theConfig-debug-dump concern raised on the GPU path) so it can't silently drift between the two call sites.
crates/nras/src/client.rs#L88-L100: extract the send/status/text logic into a shared helper and call it from this closure.crates/nras/src/keystore.rs#L62-L75: call the same shared helper from this closure instead of duplicating the status-check/text logic.🤖 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/nras/src/client.rs` around lines 88 - 100, Extract the duplicated response handling into a shared async helper, such as response_text_or_err, that accepts a reqwest::Response and context, reads the body, validates an OK status, and returns NrasError::Communication consistently. Update the red::instrumented closure in crates/nras/src/client.rs#L88-L100 and the corresponding closure in crates/nras/src/keystore.rs#L62-L75 to call this helper, preserving each call’s existing request construction and context while centralizing error formatting and redaction.
🤖 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/spdm-controller/src/handler.rs`:
- Around line 326-365: The AttestationPerformed documentation overstates
coverage because perform_attestation returns MissingData before constructing or
emitting the event. Update the documentation to scope the audit claim to
dispatched verifier calls, or emit an appropriate event before each early
MissingData return while preserving the required labels and context.
---
Nitpick comments:
In `@crates/nras/src/client.rs`:
- Around line 88-100: Extract the duplicated response handling into a shared
async helper, such as response_text_or_err, that accepts a reqwest::Response and
context, reads the body, validates an OK status, and returns
NrasError::Communication consistently. Update the red::instrumented closure in
crates/nras/src/client.rs#L88-L100 and the corresponding closure in
crates/nras/src/keystore.rs#L62-L75 to call this helper, preserving each call’s
existing request construction and context while centralizing error formatting
and redaction.
🪄 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: 22e969bc-bed5-4743-9d5e-6a2377c30c6c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/nras/Cargo.tomlcrates/nras/src/client.rscrates/nras/src/keystore.rscrates/spdm-controller/Cargo.tomlcrates/spdm-controller/src/handler.rsdocs/observability/core_metrics.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/spdm-controller/src/handler.rs (1)
320-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the enum’s standard string conversion contract.
AttestedDeviceTypeis a bounded domain but only derivesLabelValue. Add the repository-standardDisplayandFromStrderives/implementations so its representation is consistently available beyond metric emission. As per coding guidelines, “For known finite sets of values in Rust, use enums … and derive their string representation withDisplay/FromStr.”🤖 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/spdm-controller/src/handler.rs` around lines 320 - 324, Update the AttestedDeviceType enum to derive or implement the repository-standard Display and FromStr conversions in addition to LabelValue, preserving the existing Gpu and Cx7 variants and their string representations.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/nras/src/client.rs`:
- Around line 88-100: In the attestation request within
crates/nras/src/client.rs lines 88-100, check the HTTP status immediately after
send, return a status-only NrasError::Communication for failures, and remove
both response-body and self.config data from the error; only read the body for
successful responses. Apply the same status-before-body and status-only error
handling in crates/nras/src/keystore.rs lines 62-75.
---
Nitpick comments:
In `@crates/spdm-controller/src/handler.rs`:
- Around line 320-324: Update the AttestedDeviceType enum to derive or implement
the repository-standard Display and FromStr conversions in addition to
LabelValue, preserving the existing Gpu and Cx7 variants and their string
representations.
🪄 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: e1fd7cdb-beff-4609-b8fa-c3b4b0e901a4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/nras/Cargo.tomlcrates/nras/src/client.rscrates/nras/src/keystore.rscrates/spdm-controller/Cargo.tomlcrates/spdm-controller/src/handler.rsdocs/observability/core_metrics.md
| let response_text = red::instrumented("nras", "attest_gpu", async { | ||
| let att_response = request.send().await?; | ||
| let status_code = att_response.status(); | ||
| let response_text = att_response.text().await?; | ||
| if status_code != reqwest::StatusCode::OK { | ||
| return Err(NrasError::Communication(format!( | ||
| "NRAS returned status code {} and message {}.\n Config is {:?}", | ||
| status_code, response_text, self.config | ||
| ))); | ||
| } | ||
| Ok(response_text) | ||
| }) | ||
| .await?; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Sanitize HTTP failures before handing them to RED logging. Both paths embed externally controlled response bodies in errors that red::instrumented emits at WARN; the attestation path also logs the complete configuration.
crates/nras/src/client.rs#L88-L100: check the status before reading the body, return a status-only error for failures, and removeself.configfrom the error.crates/nras/src/keystore.rs#L62-L75: check the status before reading the body and return a status-only error for failures.
📍 Affects 2 files
crates/nras/src/client.rs#L88-L100(this comment)crates/nras/src/keystore.rs#L62-L75
🤖 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/nras/src/client.rs` around lines 88 - 100, In the attestation request
within crates/nras/src/client.rs lines 88-100, check the HTTP status immediately
after send, return a status-only NrasError::Communication for failures, and
remove both response-body and self.config data from the error; only read the
body for successful responses. Apply the same status-before-body and status-only
error handling in crates/nras/src/keystore.rs lines 62-75.
nras -- GPU/DPU attestation, security-critical -- had neither logs nor metrics, so a remote verifier failure surfaced nowhere. This adds an audit trail and outbound measurement.
- The attestation call in `attest_gpu` runs through `red::instrumented("nras", "attest_gpu", ...)`, recording `carbide_external_call_duration_milliseconds{backend = "nras", operation, outcome}` and a WARN on transport failure -- reusing the outbound-RED family, so no new row for it.
- A new `carbide_attestation_total{device_type, outcome}` counter is the audit trail: emitted once per attestation at the spdm-controller dispatch, INFO on success and WARN on a failed verifier call. Labels are bounded to `device_type` and `outcome`; the machine and device ids ride as log-only context, and no attestation evidence, certificate, or nonce ever reaches a label or log.
- `attest_cx7` is not implemented yet, so a Cx7 attestation records `outcome = error` -- deliberately, to surface that a device we cannot yet attest was asked to.
This supports NVIDIA#3173
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
Thanks @coderabbitai:
|
|
@coderabbitai full_review, thanks! |
|
ദ്ദി(˵ •̀ ᴗ •́ ˵ ) ✧ ✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n '#\[derive\((.*LabelValue.*)\)\]' -A1 --type=rust | head -80Length of output: 217 🏁 Script executed: #!/bin/bash
rg -n 'LabelValue' --type=rust -B2 -A1 | head -150Length of output: 201 🏁 Script executed: #!/bin/bash
echo "Repo root:"; ls
echo "---"
fd handler.rs
echo "---"
rg -n "LabelValue" -lLength of output: 1494 🏁 Script executed: #!/bin/bash
fd instrument -t d
echo "---"
sed -n '1,80p' crates/spdm-controller/src/handler.rs | grep -n "AttestedDeviceType\|derive\|Label"
echo "---full context---"
grep -rn "AttestedDeviceType" crates/spdm-controller/src/handler.rs
echo "---search for derive Label across repo case-insensitive---"
rg -ni "derive.*label" crates -g '*.rs'Length of output: 6046
Both points look good — appreciate the thorough follow-up. (๑˃̵ᴗ˂̵)و |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
1 participant
nras -- GPU/DPU attestation, security-critical -- had neither logs nor metrics, so a remote verifier failure surfaced nowhere. This adds an audit trail and outbound measurement.
attest_gpu(and the required JWKS fetch) run throughred::instrumented("nras", op, ...), recordingcarbide_external_call_duration_milliseconds{backend = "nras", operation, outcome}and a WARN on failure. The status check is inside the instrumented future, so a non-success HTTP response recordsoutcome = error, not a falseok. Reuses the outbound-RED family, so no new rows.carbide_attestation_total{device_type, outcome}counter is the audit trail: emitted once per attestation at the spdm-controller dispatch, INFO on success and WARN on a failed verifier call. Labels are bounded todevice_typeandoutcome; the machine and device ids ride as log-only context, and no attestation evidence, certificate, or nonce ever reaches a label or log.attest_cx7is not implemented yet, so a Cx7 attestation recordsoutcome = error-- deliberately, to surface that a device we cannot yet attest was asked to.This supports #3173