From 1f7c6cda45f70d12b974ec71581431eae8db46c1 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 08:58:15 -0700 Subject: [PATCH 1/6] docs(guest-agent): fix v1 leftovers and pin the two untested v1 methods A post-merge audit of the v1 surface turned up five more comments in the species #1116 was already cleaning up, three documentation gaps, and two of the six v1 methods with no handler-level test. Comments naming things that do not exist: `GetAttestationForAppKey`'s doc still credited "the v1 method" with sharing its report data, and its vector test still said `WorkerV1` wraps those bytes in an attestation -- v1 ships neither. Four more spots wrote `WorkerV1` as a service name; the service is `Worker` in package `dstack.guest.v1`, and the proto says so explicitly. `AttestGpu`'s proto comment contrasted itself against `GpuInfo`, a method that exists on no surface: the real contrast is `Attest`'s `boottime_gpu_evidence`, which is the thing that returns a boot record. Documentation gaps: the spec pointed at `rpc_service_v1/keys.rs` for the private-key vectors and the v0-forgery regression test, both of which live in `ra-tls/src/api_v1.rs` -- a reader checking the normative bytes landed where they are not. The Errors table omitted the two validations `AttestGpu` performs. And the `Health` fail-open for an app that never opted into gating -- `healthy: true` meaning "nobody asked me to know" -- was pinned only by a test, in none of the three documents that describe the method. Tests: `IssueCert` and `AttestGpu` had no handler-level coverage, so nothing would have noticed the v1 handlers ceasing to route through the shared validity check or the attestor. Both are now asserted through the handler, and the on-demand GPU format tag is pinned like its boot-time counterpart. --- docs/app-health-checks.md | 6 ++ docs/guest-api-v1.md | 16 ++++- dstack/gateway/src/proxy/health_check.rs | 8 +-- .../guest-agent/rpc/proto/agent_rpc_v1.proto | 17 +++-- dstack/guest-agent/src/rpc_service.rs | 13 ++-- dstack/guest-agent/src/rpc_service_v1.rs | 70 ++++++++++++++++++- dstack/guest-agent/src/server.rs | 4 +- 7 files changed, 111 insertions(+), 23 deletions(-) diff --git a/docs/app-health-checks.md b/docs/app-health-checks.md index f0ae7182b..d330215d9 100644 --- a/docs/app-health-checks.md +++ b/docs/app-health-checks.md @@ -51,6 +51,12 @@ independent on purpose — a fleet of gateway nodes polling the same instance mu not multiply into that many container-runtime queries inside the CVM, and the RPC is served on the CVM's publicly reachable listener. +An app that never set `health_check` has no verdict to read: the agent runs no +monitor for it, and `/prpc/v1/Health` answers `healthy: true` with an empty +`unhealthy` to whoever asks. No gateway asks, so this only matters to a third +party calling the RPC directly — and there, `true` means "this app opted out of +health gating", not "the agent checked and the app is serving". + There are two sources, and the app picks one. ### Default: container healthchecks diff --git a/docs/guest-api-v1.md b/docs/guest-api-v1.md index a385837c3..7990d4a85 100644 --- a/docs/guest-api-v1.md +++ b/docs/guest-api-v1.md @@ -291,8 +291,10 @@ Generated with an app root key of The last row's domain is the five bytes `6b 00 3a 65 79`. It is there because a delimiter-joined encoding would mishandle it. -The same vectors are asserted in -`dstack/guest-agent/src/rpc_service_v1/keys.rs`. They describe deployed key +The same vectors are asserted in `dstack/ra-tls/src/api_v1.rs`, which pins the +private-key column next to the primitives that produce it; +`dstack/guest-agent/src/rpc_service_v1/keys.rs` pins the public-key column +through the type the handler actually serves. They describe deployed key material, so a diff against them is a bug in the derivation, not a stale fixture. ## The signature chain @@ -364,7 +366,7 @@ string, and since the two byte strings can never be equal, matching their keccak digests would require a preimage attack on keccak256. The regression test is `a_v0_claim_cannot_be_crafted_into_a_v1_claim` in -`dstack/guest-agent/src/rpc_service_v1/keys.rs`. It builds the strongest available +`dstack/ra-tls/src/api_v1.rs`. It builds the strongest available forgery, a `purpose` set to the v1 claim minus exactly the suffix v0 appends on its own, then asserts both that the byte strings differ and that the structural property holds. @@ -479,6 +481,12 @@ gateway nodes are polling. Only instances that opted in via `RegisterCvmRequest.health_check` are ever polled; see [Application health checks](./app-health-checks.md). +An app that never opted in answers `healthy: true` with an empty `unhealthy` +and no `error`, whatever its containers are doing: it declared itself +un-polled, so the agent computes no verdict and returns what the gateway would +have assumed. A caller other than the gateway must read that `true` as "nobody +asked me to know", not as a health check that passed. + ### public_tcbinfo on the external surface The v1 `Worker.Info` returns the same `InfoResponse` the internal surface returns, @@ -631,6 +639,8 @@ which is the authoritative description. | Empty or unrecognised `algorithm` | Error naming the accepted values | | Derived secp256k1 scalar out of range | Error; the caller picks another domain | | `report_data` longer than 64 bytes | Error | +| `nonce` not exactly 32 bytes | Error naming the required length | +| GPU attestation unavailable in this image | Error; `AttestGpu` has nothing to collect | | `not_before` not earlier than `not_after` | Error | | Unknown method on a mounted surface | HTTP 404, `Service not found: ` | | `/v1/...` on an agent that predates v1 | HTTP 404, no such mount | diff --git a/dstack/gateway/src/proxy/health_check.rs b/dstack/gateway/src/proxy/health_check.rs index 1ebf98434..42c4023f4 100644 --- a/dstack/gateway/src/proxy/health_check.rs +++ b/dstack/gateway/src/proxy/health_check.rs @@ -348,7 +348,7 @@ async fn poll_instance(ip: Ipv4Addr, agent_port: u16, timeout: Duration) -> Poll /// no real request can reach. Only the connection is unshared; the client /// itself is process-wide. See `http_client::ConnectionReuse`. fn prober_transport(ip: Ipv4Addr, agent_port: u16) -> http_client::prpc::PrpcClient { - // `/prpc/v1`, not `/prpc`: `Health` is a `WorkerV1` method. + // `/prpc/v1`, not `/prpc`: `Health` is a v1 `Worker` method. // // No released agent is affected. `Health` never shipped in a release, and a // pre-0.6 gateway does not poll, so the only guests that can be asked are @@ -518,9 +518,9 @@ mod tests { port } - /// The poller must ask the versioned surface. `Health` lives on `WorkerV1` - /// at `/prpc/v1`; the unversioned `Worker` at `/prpc` is closed at v0.5.11 - /// and has no such method, so a poller pointed there gets a 404 that + /// The poller must ask the versioned surface. `Health` lives on the v1 + /// `Worker` at `/prpc/v1`; the unversioned `Worker` at `/prpc` is closed at + /// v0.5.11 and has no such method, so a poller pointed there gets a 404 that /// `apply_hysteresis` eventually turns into "unhealthy" for every instance /// in the fleet at once. #[tokio::test] diff --git a/dstack/guest-agent/rpc/proto/agent_rpc_v1.proto b/dstack/guest-agent/rpc/proto/agent_rpc_v1.proto index 4ce01866a..42ea0b79f 100644 --- a/dstack/guest-agent/rpc/proto/agent_rpc_v1.proto +++ b/dstack/guest-agent/rpc/proto/agent_rpc_v1.proto @@ -99,11 +99,11 @@ service DstackGuest { // Collect GPU attestation evidence now, against a nonce the caller chooses. // // This answers "is the device I can talk to right now a genuine, CC-enabled - // NVIDIA GPU that signs my challenge", which `GpuInfo` cannot: that returns - // a record written at boot. Use it after anything that may have - // reinitialised the GPU -- a driver reload leaves a device that responds to - // NVML but can no longer attest -- and before submitting work you care - // about. + // NVIDIA GPU that signs my challenge", which `Attest`'s + // `boottime_gpu_evidence` cannot: that is the record nvattest wrote at boot, + // against its own nonce. Use this after anything that may have reinitialised + // the GPU -- a driver reload leaves a device that responds to NVML but can no + // longer attest -- and before submitting work you care about. // // Returns vendor-native evidence, not a local verdict, so a relying party // can appraise it with its own verifier. Evidence still does not bind the @@ -431,6 +431,13 @@ message HealthResponse { // False when the app's own health file says so or has gone stale, or -- when // no health file was declared -- when any container that declares a Compose // `healthcheck` is not running and healthy. + // + // True, unconditionally and with an empty `unhealthy`, for an app that never + // opted into health gating: it registered as "do not poll me", so no gateway + // asks, and anything that asks anyway gets the answer the gateway would have + // assumed. "Healthy" there means "nobody asked me to know", not "checked and + // fine" -- `RegisterCvmRequest.health_check` is what separates the two, and + // this method cannot report the difference. bool healthy = 1; // The containers that made `healthy` false, so an operator reading gateway // logs can tell which one is holding the instance out of rotation. Purely diff --git a/dstack/guest-agent/src/rpc_service.rs b/dstack/guest-agent/src/rpc_service.rs index de897695c..76fbed822 100644 --- a/dstack/guest-agent/src/rpc_service.rs +++ b/dstack/guest-agent/src/rpc_service.rs @@ -926,8 +926,9 @@ impl WorkerRpc for ExternalRpcHandler { /// `report_data`, call `/v1/Attest`, and serve the result to relying /// parties. See the `Worker` service comment in agent_rpc_v1.proto. /// - /// The report data comes from the same `app_key_report_data` the v1 method - /// uses, so both attest the same public key for a given algorithm. + /// The report data comes from `app_key_report_data`, the same commitment + /// the frozen `Sign` path's key uses, so what gets attested is the public + /// key of the key that actually signs. async fn get_attestation_for_app_key( self, request: GetAttestationForAppKeyRequest, @@ -1461,10 +1462,10 @@ pNs85uhOZE8z2jr8Pg== const SECP256K1_REPORT_DATA: &str = "dip1::secp256k1c-pk:A6t_JdVkVdMAocH3f1f20WGT6JzdntxcXimUtEax8zc9"; - /// The DIP-1 report data both external surfaces commit to. `WorkerV1` - /// wraps these exact bytes in an attestation and the frozen - /// `GetAttestationForAppKey` wraps them in a TDX quote, so pinning them - /// here pins both. + /// The DIP-1 report data the frozen `Worker.GetAttestationForAppKey` + /// commits to, wrapped in a TDX quote. Pinned so the commitment format a + /// relying party parses cannot drift; v1 has no counterpart method, so + /// this is the only surface these bytes appear on. #[tokio::test] async fn app_key_report_data_matches_its_vectors() { let (state, _guard) = setup_test_state().await; diff --git a/dstack/guest-agent/src/rpc_service_v1.rs b/dstack/guest-agent/src/rpc_service_v1.rs index 1ec7b1ca2..4ca697e6c 100644 --- a/dstack/guest-agent/src/rpc_service_v1.rs +++ b/dstack/guest-agent/src/rpc_service_v1.rs @@ -491,9 +491,9 @@ mod tests { } /// `Info` must not attest. Decoding identity costs a hardware quote and an - /// RTMR replay under a global lock, and `WorkerV1.Info` is served on the - /// public listener -- so doing it per call hands any caller that can route - /// to the CVM a way to monopolise the attestation path. + /// RTMR replay under a global lock, and the v1 `Worker.Info` is served on + /// the public listener -- so doing it per call hands any caller that can + /// route to the CVM a way to monopolise the attestation path. /// /// One decoded `AppIdentity`, shared by pointer, is what says it is cached. #[tokio::test] @@ -782,4 +782,68 @@ mod tests { ); assert_eq!(v1, &["Info", "Version", "Health"]); } + + /// `IssueCert` must reject an inverted validity window, and must do it + /// before generating a key or calling the KMS. + /// + /// The check lives in the shared `issue_cert_for_request`, which the frozen + /// `GetTlsKey` also calls, so nothing here would fail if the v1 handler + /// stopped routing through it -- which is exactly why the assertion is made + /// against the v1 handler rather than against the validator alone. + #[tokio::test] + async fn issue_cert_rejects_an_inverted_validity_window() { + let (state, _guard) = state().await; + let err = V1RpcHandler::new(state) + .issue_cert(IssueCertRequest { + subject: "example".to_string(), + not_before: Some(2_000), + not_after: Some(1_000), + ..Default::default() + }) + .await + .unwrap_err() + .to_string(); + assert!( + err.contains("not_before must be earlier than not_after"), + "{err}" + ); + } + + /// `AttestGpu` routes through the attestor, which validates the nonce + /// length before it touches a device. Pinned through the handler so a + /// future refactor cannot answer without asking the attestor at all. + /// + /// Formatted with `{:#}`, because the handler adds a context line and the + /// reason sits under it. That is also how the caller reads it: `ra_rpc` + /// encodes an error as `format!("{error:#}")`, so asserting on the flat + /// `to_string()` would test something no client ever sees. + #[tokio::test] + async fn attest_gpu_rejects_a_wrong_length_nonce() { + let (state, _guard) = state().await; + let err = V1RpcHandler::new(state) + .attest_gpu(AttestGpuRequest { + nonce: vec![0u8; 16], + }) + .await + .unwrap_err(); + let err = format!("{err:#}"); + assert!(err.contains("exactly 32 bytes"), "{err}"); + } + + /// The on-demand format tag, pinned like its boot-time counterpart. + /// + /// A consumer selects its verifier on `(vendor, format)`, so these two + /// strings are wire contract: the boot-time tag is already pinned by + /// `attest_returns_boot_time_gpu_evidence_only_when_asked`, and this is the + /// other half. Collecting real evidence needs a GPU, so the tags -- not a + /// live bundle -- are what a test can hold still. + #[test] + fn the_on_demand_gpu_format_tag_is_the_specified_one() { + assert_eq!(GPU_VENDOR, "nvidia"); + assert_eq!( + GPU_FORMAT_ON_DEMAND, + "nvidia-nvattest-collect-evidence-json-v1" + ); + assert_ne!(GPU_FORMAT_ON_DEMAND, GPU_FORMAT_BOOTTIME); + } } diff --git a/dstack/guest-agent/src/server.rs b/dstack/guest-agent/src/server.rs index aea536575..0021f4f9b 100644 --- a/dstack/guest-agent/src/server.rs +++ b/dstack/guest-agent/src/server.rs @@ -105,8 +105,8 @@ fn mount_internal(rocket: Rocket) -> Rocket { /// Mount the pRPC services the external listener serves. /// /// Same scheme as the internal socket, one level down: `/prpc/v0` is the frozen -/// v0.5.11 `Worker`, `/prpc/v1` is `WorkerV1`, and `/prpc` is the frozen surface -/// under its historical path. +/// v0.5.11 `Worker`, `/prpc/v1` is the v1 `Worker`, and `/prpc` is the frozen +/// surface under its historical path. /// /// The `trim` on the frozen mounts strips the service name a pre-0.6 client /// prefixes, so `/prpc/Worker.Info` and `/prpc/Info` both land on `Info`. From f7a5f0eda7071f788591839f0e695232577901ea Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 17:44:29 -0700 Subject: [PATCH 2/6] docs(guest-agent): correct how the frozen Verify rejects a high-S signature The comment said a malleated signature "fails to parse rather than verifying". Measured against k256 0.13.4: `Signature::from_slice` accepts the high-S bytes -- it only rejects an `r` or `s` outside `1..n`, and `n - s` is still in range -- and the rejection happens inside verification. So a caller gets HTTP 200 with `valid: false`, not the 400 a parse failure produces. The security answer is unchanged and the behaviour is right; only the explanation was wrong, and it was wrong about the status code, which is the part a client branches on. --- dstack/guest-agent/src/rpc_service.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/dstack/guest-agent/src/rpc_service.rs b/dstack/guest-agent/src/rpc_service.rs index 76fbed822..512d99d6e 100644 --- a/dstack/guest-agent/src/rpc_service.rs +++ b/dstack/guest-agent/src/rpc_service.rs @@ -710,10 +710,15 @@ impl DstackGuestRpc for InternalRpcHandler { /// Deprecated, kept for 0.5.x clients only. See the RPC's doc comment in /// agent_rpc.proto. /// - /// k256 rejects a non-canonical (high-S) signature outright, so a malleated - /// copy of a valid signature fails to parse rather than verifying. Keep it - /// that way: 0.5.x answered the same, and callers may be treating this - /// answer as a uniqueness check. + /// k256 rejects a non-canonical (high-S) signature, so a malleated copy of a + /// valid signature does not verify. Keep it that way: 0.5.x answered the + /// same, and callers may be treating this answer as a uniqueness check. + /// + /// The rejection is in the verification, not in the parsing -- `from_slice` + /// only rejects an `r` or `s` outside `1..n`, and a malleated `s` is still + /// in range. So a caller sees HTTP 200 with `valid: false`, not the 400 a + /// parse failure would produce. Same security answer, different status + /// code, and the status code is the part a client branches on. async fn verify(self, request: VerifyRequest) -> Result { let algorithm = normalize_algorithm(&request.algorithm); let valid = match algorithm { From 5bc22c88f40d6f40ac443beab1215dfc8684a1d2 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 18:23:47 -0700 Subject: [PATCH 3/6] test(guest-agent): pin that a malleated signature is a verdict, not a 400 The doc comment on the frozen `Verify` makes an observable claim: k256 rejects high-S inside the verification rather than in `from_slice`, so a malleated signature comes back as HTTP 200 with `valid: false`, not as the 400 a parse failure would produce. Nothing pinned it. The test builds the malleation by negating `s`, then checks it is the real thing before using it -- `normalize_s` is `Some` only for a high-S signature, and it must normalise back to exactly the signature that just verified -- so the assertions cannot pass against a no-op. Both halves of the claim are asserted: `.expect()` for the status code a 0.5.x client branches on, `!valid` for the security answer. A k256 upgrade that moved the check into parsing would keep the second and silently break the first. That is the drift the comment warns about. --- dstack/guest-agent/src/rpc_service.rs | 35 +++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/dstack/guest-agent/src/rpc_service.rs b/dstack/guest-agent/src/rpc_service.rs index 512d99d6e..726a29d25 100644 --- a/dstack/guest-agent/src/rpc_service.rs +++ b/dstack/guest-agent/src/rpc_service.rs @@ -1829,6 +1829,41 @@ pNs85uhOZE8z2jr8Pg== assert!(!response.valid); } + /// A malleated (high-S) signature must not verify, and must come back as a + /// verdict rather than as an error. + /// + /// Both halves matter. k256 rejects high-S inside the verification, not in + /// `from_slice`, so the caller sees HTTP 200 with `valid: false` -- not the + /// 400 a parse failure would produce, and the status code is the part a + /// 0.5.x client branches on. A k256 upgrade that moved the check into + /// parsing would keep the security answer and silently change the status, + /// which is what `.expect()` below is here to catch. + #[tokio::test] + async fn verify_rejects_a_malleated_secp256k1_signature() { + let data = b"test message for secp256k1".to_vec(); + let (state, _guard, signed) = sign_then_verify("secp256k1", data.clone()).await; + + // `sign` emits low-S, so negating `s` yields the other encoding of the + // same signature -- the one an unnormalised verifier would also accept. + let signature = K256Signature::from_slice(&signed.signature).unwrap(); + let malleated = K256Signature::from_scalars(signature.r(), -signature.s()).unwrap(); + // `normalize_s` is `Some` only for a high-S signature, so this asserts + // the malleation is real and is exactly the twin of what just verified. + assert_eq!(malleated.normalize_s().as_ref(), Some(&signature)); + + let response = InternalRpcHandler { state } + .verify(VerifyRequest { + algorithm: "secp256k1".to_string(), + data, + signature: malleated.to_vec(), + public_key: signed.public_key, + }) + .await + .expect("a malleated signature is a verdict, not an error"); + + assert!(!response.valid); + } + #[tokio::test] async fn verify_unsupported_algorithm_fails() { let (state, _guard) = setup_test_state().await; From f9f9c509f53ea5f65c3ba3d114cb7516fc0ed039 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 18:23:47 -0700 Subject: [PATCH 4/6] docs(guest-agent): tighten the v1 audit follow-up prose Three edits from review of the preceding two commits. `GetAttestationForAppKey`'s comment said the report data was "the same commitment the frozen `Sign` path's key uses". `Sign` uses a key, not a commitment. Say what is actually shared: the derivation -- same path, purpose and base algorithm -- so the attested public key is the one that signs. The Errors table now lists a row that no client can clear by changing its request. Since a 400 in this API means "the method ran and failed" rather than "your request was malformed", say so where the table says everything above is a 400, and say that the message in the body is what separates the two. `app-health-checks.md` restated the fail-open mechanism the proto comment and the spec already carry. Keep the reading an operator needs -- what `true` means for an app that opted out -- and drop the third copy of how it is computed. Also reflow the paragraph the `ra-tls` pointer fix left short of the fill. --- docs/app-health-checks.md | 10 +++++----- docs/guest-api-v1.md | 13 +++++++++---- dstack/guest-agent/src/rpc_service.rs | 6 +++--- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/docs/app-health-checks.md b/docs/app-health-checks.md index d330215d9..a3f9241c5 100644 --- a/docs/app-health-checks.md +++ b/docs/app-health-checks.md @@ -51,11 +51,11 @@ independent on purpose — a fleet of gateway nodes polling the same instance mu not multiply into that many container-runtime queries inside the CVM, and the RPC is served on the CVM's publicly reachable listener. -An app that never set `health_check` has no verdict to read: the agent runs no -monitor for it, and `/prpc/v1/Health` answers `healthy: true` with an empty -`unhealthy` to whoever asks. No gateway asks, so this only matters to a third -party calling the RPC directly — and there, `true` means "this app opted out of -health gating", not "the agent checked and the app is serving". +An app that never set `health_check` has no verdict to read: `/prpc/v1/Health` +answers `healthy: true` to whoever asks, whatever its containers are doing. No +gateway asks, so this only matters to a third party calling the RPC directly — +and there, `true` means "this app opted out of health gating", not "the agent +checked and the app is serving". There are two sources, and the app picks one. diff --git a/docs/guest-api-v1.md b/docs/guest-api-v1.md index 7990d4a85..a1026c5f6 100644 --- a/docs/guest-api-v1.md +++ b/docs/guest-api-v1.md @@ -366,10 +366,10 @@ string, and since the two byte strings can never be equal, matching their keccak digests would require a preimage attack on keccak256. The regression test is `a_v0_claim_cannot_be_crafted_into_a_v1_claim` in -`dstack/ra-tls/src/api_v1.rs`. It builds the strongest available -forgery, a `purpose` set to the v1 claim minus exactly the suffix v0 appends on -its own, then asserts both that the byte strings differ and that the structural -property holds. +`dstack/ra-tls/src/api_v1.rs`. It builds the strongest available forgery, a +`purpose` set to the v1 claim minus exactly the suffix v0 appends on its own, +then asserts both that the byte strings differ and that the structural property +holds. The two context tags are also distinct, so a derivation input can never be read as a claim: `dstack-guest-v1-key` for the KDF, `dstack-guest-v1-key-claim` for the @@ -649,6 +649,11 @@ Everything above the last two rows is an HTTP 400 with the message in the body. See [Status codes](#status-codes) for the full mapping. v1 does not default, coerce, or truncate a malformed request into a well-formed one. +A 400 here means "the method ran and failed", not "your request was malformed". +The GPU-unavailable row is a property of the image, not of the call: a client +that gets it should stop rather than retry with different arguments, and the +message in the body is the only thing that separates the two cases. + ## Migration from the unversioned API **v1 keys are different keys.** Deriving under the same name on `/v1` that an diff --git a/dstack/guest-agent/src/rpc_service.rs b/dstack/guest-agent/src/rpc_service.rs index 726a29d25..d60f589b6 100644 --- a/dstack/guest-agent/src/rpc_service.rs +++ b/dstack/guest-agent/src/rpc_service.rs @@ -931,9 +931,9 @@ impl WorkerRpc for ExternalRpcHandler { /// `report_data`, call `/v1/Attest`, and serve the result to relying /// parties. See the `Worker` service comment in agent_rpc_v1.proto. /// - /// The report data comes from `app_key_report_data`, the same commitment - /// the frozen `Sign` path's key uses, so what gets attested is the public - /// key of the key that actually signs. + /// The report data comes from `app_key_report_data`, which commits to the + /// key `Sign` derives -- same path, purpose and base algorithm -- so the + /// attested public key is the one that actually signs. async fn get_attestation_for_app_key( self, request: GetAttestationForAppKeyRequest, From 34ea773624f4c1447654817476abfd54d1e5255e Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 18:46:11 -0700 Subject: [PATCH 5/6] fix(guest-agent): answer 501, not 400, when the image cannot attest a GPU `AttestGpu` reported "GPU attestation is not available in this image" as an uncoded error, which `dispatch_prpc` turns into the generic 400. That tells a client its request was malformed. It was not: the request is well-formed, and no other request would succeed either, because the image ships no nvattest and will not grow one at runtime. A client branching on the status retries with different arguments forever instead of falling back. `ra_rpc::ErrorExt::with_code` already carries a chosen status through the transport, and `code_of` walks the whole error chain, so the code survives the handler's `.context("GPU attestation failed")`. 501 rather than 503 because the capability is absent for the lifetime of the CVM, not temporarily unavailable. A malformed nonce keeps the default 400 -- that one really is the caller's fault, and the two failures must not be indistinguishable. Safe to change now: `AttestGpu` is v1-only and never shipped in a release, so no deployed client is reading the old 400. `rpc_service_v1.rs` already asserts it is absent from both frozen surfaces. The availability probe moves from `nvattest::available()` to a `GpuAttestor` field holding the binary's path. Without that, a test for the unavailable answer would pass only on a host with no nvattest installed and would spawn a real collection against the host's GPUs anywhere else. The test fixture pins it to a path that cannot exist, so every guest-agent test sees the same answer. Documented in the proto comment, the spec's Errors and Status codes sections, and the curl API reference, which listed only 400 and 500. --- docs/guest-api-v1.md | 23 +++++---- .../guest-agent/rpc/proto/agent_rpc_v1.proto | 5 ++ dstack/guest-agent/src/gpu_attest.rs | 50 +++++++++++++++++-- dstack/guest-agent/src/rpc_service.rs | 7 ++- dstack/guest-agent/src/rpc_service_v1.rs | 25 ++++++++++ sdk/curl/api.md | 7 +++ 6 files changed, 103 insertions(+), 14 deletions(-) diff --git a/docs/guest-api-v1.md b/docs/guest-api-v1.md index a1026c5f6..85da408a8 100644 --- a/docs/guest-api-v1.md +++ b/docs/guest-api-v1.md @@ -103,10 +103,12 @@ returns JSON. | 404 | the server's own 404 page | No such mount: this agent has no surface at that path | | 404 | `{"error": "Service not found: "}` | The surface is mounted; it has no such method | | 400 | `{"error": ""}` | The method ran and failed | +| 501 | `{"error": ""}` | `AttestGpu` on an image that ships no GPU attestation | | other | `{"error": ""}` | A handler chose the status; the message says why | -A handler failure is a 400 with the error text in the body. v1 does not default, -coerce, or truncate a malformed request into a well-formed one. +A handler failure is a 400 with the error text in the body unless the handler +chose otherwise, and `AttestGpu` is the only v1 method that does. v1 does not +default, coerce, or truncate a malformed request into a well-formed one. ### Detecting an agent without v1 @@ -640,19 +642,20 @@ which is the authoritative description. | Derived secp256k1 scalar out of range | Error; the caller picks another domain | | `report_data` longer than 64 bytes | Error | | `nonce` not exactly 32 bytes | Error naming the required length | -| GPU attestation unavailable in this image | Error; `AttestGpu` has nothing to collect | +| GPU attestation unavailable in this image | HTTP 501; `AttestGpu` has nothing to collect | | `not_before` not earlier than `not_after` | Error | | Unknown method on a mounted surface | HTTP 404, `Service not found: ` | | `/v1/...` on an agent that predates v1 | HTTP 404, no such mount | -Everything above the last two rows is an HTTP 400 with the message in the body. -See [Status codes](#status-codes) for the full mapping. v1 does not default, -coerce, or truncate a malformed request into a well-formed one. +Every row that does not name a status is an HTTP 400 with the message in the +body. See [Status codes](#status-codes) for the full mapping. v1 does not +default, coerce, or truncate a malformed request into a well-formed one. -A 400 here means "the method ran and failed", not "your request was malformed". -The GPU-unavailable row is a property of the image, not of the call: a client -that gets it should stop rather than retry with different arguments, and the -message in the body is the only thing that separates the two cases. +The GPU row is a 501 rather than a 400 because it is a property of the image, +not of the call: the request is well-formed, and no other request would succeed +either. A client that reads 400 there retries with different arguments forever; +one that reads 501 stops and falls back to whatever it does without a GPU. A +malformed nonce is still the caller's fault and still a 400. ## Migration from the unversioned API diff --git a/dstack/guest-agent/rpc/proto/agent_rpc_v1.proto b/dstack/guest-agent/rpc/proto/agent_rpc_v1.proto index 42ea0b79f..1210fac58 100644 --- a/dstack/guest-agent/rpc/proto/agent_rpc_v1.proto +++ b/dstack/guest-agent/rpc/proto/agent_rpc_v1.proto @@ -108,6 +108,11 @@ service DstackGuest { // Returns vendor-native evidence, not a local verdict, so a relying party // can appraise it with its own verifier. Evidence still does not bind the // GPU to this TD; see `AttestGpuResponse.bundles`. + // + // An image that ships no GPU attestation answers HTTP 501, not the 400 every + // other v1 failure uses. The request is well-formed there and no other + // request would succeed either, so a caller can tell "fall back" from "fix + // your arguments" on the status alone. A malformed nonce stays a 400. rpc AttestGpu(AttestGpuRequest) returns (AttestGpuResponse) {} // Return this application's identity and measurements. diff --git a/dstack/guest-agent/src/gpu_attest.rs b/dstack/guest-agent/src/gpu_attest.rs index ced4dcd6f..6e54e9734 100644 --- a/dstack/guest-agent/src/gpu_attest.rs +++ b/dstack/guest-agent/src/gpu_attest.rs @@ -8,9 +8,11 @@ //! process and talks to the GPU through the driver. The gate below serialises //! collection so concurrent callers do not compete for the same devices. +use std::path::{Path, PathBuf}; use std::time::Duration; -use anyhow::{bail, Result}; +use anyhow::{anyhow, bail, Result}; +use ra_rpc::ErrorExt; use tokio::sync::Mutex; /// Serialises GPU evidence collection. @@ -20,13 +22,28 @@ use tokio::sync::Mutex; /// challenge is exactly the confusion this API exists to avoid. pub struct GpuAttestor { timeout: Duration, + /// The binary whose presence says this image can attest a GPU. + /// + /// A field rather than a call to `nvattest::available()` so a test can pin + /// the unavailable answer on a host that happens to have nvattest + /// installed -- otherwise that test would spawn a real collection against + /// whatever GPUs the host has. + nvattest: PathBuf, run_lock: Mutex<()>, } impl GpuAttestor { pub fn new() -> Self { + Self::with_nvattest_path(nvattest::NVATTEST) + } + + /// `pub(crate)` so the handler tests can build a state that is + /// deterministically without GPU support, whatever the host running + /// them has installed. + pub(crate) fn with_nvattest_path(path: impl AsRef) -> Self { Self { timeout: nvattest::DEFAULT_TIMEOUT, + nvattest: path.as_ref().to_path_buf(), run_lock: Mutex::new(()), } } @@ -40,8 +57,16 @@ impl GpuAttestor { nonce.len() ); } - if !nvattest::available() { - bail!("GPU attestation is not available in this image"); + if !self.nvattest.exists() { + // 501, not the 400 an uncoded error would report. The request is + // well-formed and no other request would succeed: this image ships + // no nvattest, so the capability is absent for the lifetime of the + // CVM. A caller that reads 400 retries with different arguments + // forever; one that reads 501 stops and falls back. + // + // Safe to say now because `AttestGpu` is v1-only and has never + // shipped in a release -- no client is reading 400 here today. + return Err(anyhow!("GPU attestation is not available in this image").with_code(501)); } // Held across the whole run, so a second caller waits rather than // starting a competing nvattest against the same devices. @@ -61,4 +86,23 @@ mod tests { let err = attestor.attest(&[0u8; 16]).await.unwrap_err().to_string(); assert!(err.contains("exactly 32 bytes"), "{err}"); } + + /// A malformed nonce is the caller's fault; an image without nvattest is + /// not. The two must not answer with the same status. + /// + /// The nonce here is well-formed, so this reaches the availability check + /// rather than stopping at the length check above. + #[tokio::test] + async fn an_image_without_nvattest_answers_not_implemented() { + let attestor = GpuAttestor::with_nvattest_path("/nonexistent/nvattest"); + let err = attestor.attest(&[0u8; 32]).await.unwrap_err(); + assert!( + err.to_string().contains("not available in this image"), + "{err}" + ); + assert_eq!(ra_rpc::code_of(&err), Some(501)); + + let malformed = attestor.attest(&[0u8; 16]).await.unwrap_err(); + assert_eq!(ra_rpc::code_of(&malformed), None, "a bad nonce stays a 400"); + } } diff --git a/dstack/guest-agent/src/rpc_service.rs b/dstack/guest-agent/src/rpc_service.rs index d60f589b6..b5eef9e77 100644 --- a/dstack/guest-agent/src/rpc_service.rs +++ b/dstack/guest-agent/src/rpc_service.rs @@ -1325,7 +1325,12 @@ pNs85uhOZE8z2jr8Pg== probe, }), health: None, - gpu_attestor: crate::gpu_attest::GpuAttestor::new(), + // Pinned to a path that cannot exist, so no test ever spawns a + // real collection against a host GPU and every test sees the + // same "this image cannot attest a GPU" answer. + gpu_attestor: crate::gpu_attest::GpuAttestor::with_nvattest_path( + "/nonexistent/nvattest", + ), app_root_signing_key: SigningKey::from_slice(&DUMMY_K256_KEY).ok(), identity: RwLock::new(None), identity_last_failure: Mutex::new(None), diff --git a/dstack/guest-agent/src/rpc_service_v1.rs b/dstack/guest-agent/src/rpc_service_v1.rs index 4ca697e6c..1e8a5b164 100644 --- a/dstack/guest-agent/src/rpc_service_v1.rs +++ b/dstack/guest-agent/src/rpc_service_v1.rs @@ -826,10 +826,35 @@ mod tests { }) .await .unwrap_err(); + assert_eq!( + ra_rpc::code_of(&err), + None, + "a malformed nonce is the caller's fault, so it keeps the default 400" + ); let err = format!("{err:#}"); assert!(err.contains("exactly 32 bytes"), "{err}"); } + /// An image that ships no nvattest must not answer like a malformed + /// request: the call is well-formed and no other call would succeed, so + /// the caller has to be able to tell "stop" from "try something else". + /// + /// Asserted through the handler because that is where `.context(..)` wraps + /// the attestor's error. `code_of` walks the whole chain for exactly this + /// reason -- a context layer that buried the code would silently put the + /// answer back to 400, and the message would still read correctly. + #[tokio::test] + async fn attest_gpu_reports_an_image_without_gpu_support_as_not_implemented() { + let (state, _guard) = state().await; + let err = V1RpcHandler::new(state) + .attest_gpu(AttestGpuRequest { + nonce: vec![0u8; 32], + }) + .await + .unwrap_err(); + assert_eq!(ra_rpc::code_of(&err), Some(501), "{err:#}"); + } + /// The on-demand format tag, pinned like its boot-time counterpart. /// /// A consumer selects its verifier on `(vendor, format)`, so these two diff --git a/sdk/curl/api.md b/sdk/curl/api.md index b755cf645..4e6c59f7f 100644 --- a/sdk/curl/api.md +++ b/sdk/curl/api.md @@ -331,6 +331,11 @@ the evidence signature, certificate chain, measurements, and embedded nonce. The agent does not appraise the evidence. Evidence does not by itself bind the GPU to this CVM. +> An image that ships no GPU attestation answers `501 Not Implemented`, not +> `400`. Retrying with a different nonce will not help; fall back to whatever +> your application does without a GPU. A nonce that is not exactly 32 bytes is +> still a `400`. + ### 8. Boot-time GPU evidence *(v1)* Returns the complete output NVIDIA `nvattest` produced during boot, as part of @@ -415,6 +420,8 @@ All endpoints may return the following HTTP status codes: - `200 OK`: Request successful - `400 Bad Request`: Invalid request parameters +- `501 Not Implemented`: The agent cannot serve this method in this image; only + `/v1/AttestGpu` answers this, and only when the image ships no GPU attestation - `500 Internal Server Error`: Server-side error Error responses will include a JSON body with error details: From 252076e50ac01fec7961a3dc3ea0cbf8a47334a6 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 18:56:08 -0700 Subject: [PATCH 6/6] refactor(ra-rpc): add bail! for failing with a chosen status code `return Err(anyhow!("...").with_code(501))` is four moving parts to say one thing, and it reads as an error construction rather than as a control-flow exit -- the two existing sites both wrote it differently, one importing `ErrorExt` and one calling it as a free function. `ra_rpc::bail!(501, "...")` expands to exactly that expression. The format arguments are forwarded to `anyhow::anyhow!` verbatim, so nothing new has to be learned, and the macro resolves `anyhow` through a hidden re-export so a caller does not need it in scope. The code is the first argument and is mandatory. An error with no status to choose should keep using `anyhow::bail!` and be reported as CODE_BAD_REQUEST, which is the correct default; requiring the code here means the two spellings say which kind of failure they are. `gpu_attest.rs` now shows both in one function -- a bad nonce bails plainly, a missing nvattest bails with 501. Converted both existing sites. The 413 path in `read_data` is already covered end to end by `status_codes::a_payload_over_the_limit_is_rejected`, so the macro reaching the transport is pinned without a new test; the unit test added here covers the part that test cannot see, that the code survives the `.context(..)` a handler adds on the way out. --- dstack/guest-agent/src/gpu_attest.rs | 5 +-- dstack/ra-rpc/src/lib.rs | 64 ++++++++++++++++++++++++++++ dstack/ra-rpc/src/rocket_helper.rs | 5 +-- 3 files changed, 67 insertions(+), 7 deletions(-) diff --git a/dstack/guest-agent/src/gpu_attest.rs b/dstack/guest-agent/src/gpu_attest.rs index 6e54e9734..9d8013a25 100644 --- a/dstack/guest-agent/src/gpu_attest.rs +++ b/dstack/guest-agent/src/gpu_attest.rs @@ -11,8 +11,7 @@ use std::path::{Path, PathBuf}; use std::time::Duration; -use anyhow::{anyhow, bail, Result}; -use ra_rpc::ErrorExt; +use anyhow::{bail, Result}; use tokio::sync::Mutex; /// Serialises GPU evidence collection. @@ -66,7 +65,7 @@ impl GpuAttestor { // // Safe to say now because `AttestGpu` is v1-only and has never // shipped in a release -- no client is reading 400 here today. - return Err(anyhow!("GPU attestation is not available in this image").with_code(501)); + ra_rpc::bail!(501, "GPU attestation is not available in this image"); } // Held across the whole run, so a second caller waits rather than // starting a competing nvattest against the same devices. diff --git a/dstack/ra-rpc/src/lib.rs b/dstack/ra-rpc/src/lib.rs index 40f6353a3..847563ec5 100644 --- a/dstack/ra-rpc/src/lib.rs +++ b/dstack/ra-rpc/src/lib.rs @@ -116,6 +116,45 @@ impl> ResultExt for std::result::Result { } } +/// Fail with a status code, the way `anyhow::bail!` fails without one. +/// +/// `ra_rpc::bail!(501, "...")` is `return Err(anyhow!("...").with_code(501))`. +/// The code is the first argument and is not optional: an error that does not +/// need to choose a status should use `anyhow::bail!` and be reported as +/// [`CODE_BAD_REQUEST`], which is the right default for "the caller sent +/// something wrong". Making the code mandatory keeps the two spellings honest +/// about which failure is which. +/// +/// Everything after the code is passed to `anyhow::anyhow!`, so the format +/// arguments are the ones you already know. +/// +/// ``` +/// fn load(id: &str) -> anyhow::Result<()> { +/// if id.is_empty() { +/// ra_rpc::bail!(404, "no app named {id:?}"); +/// } +/// Ok(()) +/// } +/// +/// let err = load("").unwrap_err(); +/// assert_eq!(ra_rpc::code_of(&err), Some(404)); +/// assert_eq!(err.to_string(), r#"no app named """#); +/// ``` +#[macro_export] +macro_rules! bail { + ($code:expr, $($arg:tt)*) => { + return ::core::result::Result::Err($crate::ErrorExt::with_code( + $crate::__private::anyhow!($($arg)*), + $code, + )) + }; +} + +#[doc(hidden)] +pub mod __private { + pub use anyhow::anyhow; +} + /// Extract the status code attached to `err`, if any. /// /// Walks the whole error chain and returns the outermost code, so a code @@ -295,6 +334,31 @@ mod coded_error_tests { assert_eq!(code_of(&err), Some(404)); } + /// The macro must produce exactly what the longhand does, including + /// through the `?` and `.context(..)` layers a handler adds on the way + /// out -- that is the whole reason a caller can trust the status it reads. + #[test] + fn bail_attaches_the_code_and_returns() { + fn inner(fail: bool) -> anyhow::Result { + if fail { + crate::bail!(501, "no {} in this image", "nvattest"); + } + Ok(7) + } + fn outer() -> anyhow::Result { + inner(true).context("collect evidence") + } + + assert_eq!(inner(false).unwrap(), 7, "the macro must not return early"); + + let err = outer().unwrap_err(); + assert_eq!(code_of(&err), Some(501)); + assert_eq!( + format!("{err:#}"), + "collect evidence: no nvattest in this image" + ); + } + #[test] fn a_code_survives_a_question_mark_boundary() { fn inner() -> anyhow::Result<()> { diff --git a/dstack/ra-rpc/src/rocket_helper.rs b/dstack/ra-rpc/src/rocket_helper.rs index 3e33c5288..82627e398 100644 --- a/dstack/ra-rpc/src/rocket_helper.rs +++ b/dstack/ra-rpc/src/rocket_helper.rs @@ -325,10 +325,7 @@ async fn read_data(data: Data<'_>, limit: ByteUnit) -> Result> { let stream = data.open(limit); let data = stream.into_bytes().await.context("failed to read data")?; if !data.is_complete() { - return Err(crate::ErrorExt::with_code( - anyhow::anyhow!("payload too large"), - crate::CODE_PAYLOAD_TOO_LARGE, - )); + crate::bail!(crate::CODE_PAYLOAD_TOO_LARGE, "payload too large"); } Ok(data.into_inner()) }