guest-agent: fix v1 leftovers and pin the two untested v1 methods

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: `/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.
 
 ### Default: container healthchecks

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: <Method>"}` | The surface is mounted; it has no such method |
 | 400 | `{"error": "<message>"}` | The method ran and failed |
+| 501 | `{"error": "<message>"}` | `AttestGpu` on an image that ships no GPU attestation |
 | other | `{"error": "<message>"}` | 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
 
@@ -291,8 +293,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,10 +368,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/guest-agent/src/rpc_service_v1/keys.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
@@ -479,6 +483,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,13 +641,21 @@ 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 | 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: <Method>` |
 | `/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.
+
+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
 

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]

dstack/guest-agent/rpc/proto/agent_rpc_v1.proto

@@ -99,15 +99,20 @@ 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
   // 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.
@@ -431,6 +436,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

dstack/guest-agent/src/gpu_attest.rs

@@ -8,6 +8,7 @@
 //! 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};
@@ -20,13 +21,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<Path>) -> Self {
         Self {
             timeout: nvattest::DEFAULT_TIMEOUT,
+            nvattest: path.as_ref().to_path_buf(),
             run_lock: Mutex::new(()),
         }
     }
@@ -40,8 +56,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.
+            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.
@@ -61,4 +85,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");
+    }
 }

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<VerifyResponse> {
         let algorithm = normalize_algorithm(&request.algorithm);
         let valid = match algorithm {
@@ -926,8 +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 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`, 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,
@@ -1319,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),
@@ -1461,10 +1472,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;
@@ -1823,6 +1834,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;