From 571d6e8357e2226acd0ae1f948ad59d732385b89 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 09:00:17 -0700 Subject: [PATCH 1/5] fix(sdk): finish the 0.6 cleanups -- v1 defaults, v1 naming, transport errors The leftovers named in #1116's description, from the #1094 prerelease checklist, plus one wire bug found on the way. Go's v1 `IssueCert` defaulted `usage_server_auth` to false where Rust, Python and JS default it to true, so the same argument-free call yielded a servable certificate in three languages and an unservable one in the fourth. v0's false default is kept and commented: that surface mirrors released 0.5.x behaviour rather than the better choice. `IssueCertResponseV1.asUint8Array` is removed rather than renamed. It existed to feed the key into the blockchain adapters, and v1 has no chain-flavoured surface -- `IssueCert` returns TLS material, PEM is what a TLS stack takes, and all four SDKs now return the PEM string and the chain and nothing else. The GPU bundle's accessor becomes `decodeEvidence()`, matching Python's and Rust's `decode_evidence`, since that one decodes wire hex rather than serving a chain flow. The JS transport ignored the HTTP status entirely -- its socket branch never parsed the status line -- so an agent with no `/v1` mount answered with an HTML 404 that reached the caller as "failed to parse response". Its socket branch also declared `Content-Length` as `payload.length`, a UTF-16 code unit count, while writing UTF-8: any request carrying a non-ASCII string was truncated on the wire and left the connection poisoned, which the v1 spec's promise that a `domain` may be any byte string made reachable by design. The Rust clients threw away the other half of the same information, since `error_for_status()` keeps the status and drops the body, and the unix crate's JSON helper insists on deserializing the error body too. Both now raise `HTTP : `, and the two duplicated Rust transport blocks became one `http_post`/`unix_post`/`http_error`. `TlsKeyOptions.path` is gone: it was declared but never sent, so callers who set it were silently ignored. Nothing on the wire or the success path changes except the corrected Content-Length; only failures read differently. Verified with sdk/run-tests.sh: Rust, Go, Python green and JS 144 passed, with the new tests asserting against real simulator responses -- a real HTML 404 from a socket with no v1 mount, and a real prpc 400 from EmitEvent. --- CHANGELOG.md | 5 + sdk/go/dstack/client.go | 6 +- sdk/go/dstack/client_v1.go | 6 +- sdk/go/dstack/client_v1_test.go | 56 ++++++ sdk/js/README.md | 11 +- sdk/js/src/__tests__/index-v1.test.ts | 42 ++-- sdk/js/src/__tests__/index.test.ts | 4 +- sdk/js/src/__tests__/send-rpc-request.test.ts | 127 ++++++++++++- sdk/js/src/index.ts | 30 +-- sdk/js/src/send-rpc-request.ts | 100 ++++++++-- sdk/rust/src/dstack_client.rs | 179 ++++++++++++++---- sdk/rust/src/dstack_client_v1.rs | 37 +--- sdk/rust/tests/test_client.rs | 24 +++ sdk/rust/tests/test_client_v1.rs | 20 ++ 14 files changed, 538 insertions(+), 109 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1dd592b2..cff89f955 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - sdk: the Go SDK's `gpu_policy` silently dropped sub-fields it did not declare on a decode/re-hash round trip; it now carries them through like the rest of `Requirements` - bound VMM-to-guest RPCs and guest-agent dependency calls to prevent stalled peers from retaining request resources indefinitely - sdk: the Go and Python compose-hash helpers silently dropped every app-compose field they did not declare, so `getComposeHash` returned a digest for an app-compose that was not the one being deployed — and that digest is what gets whitelisted on chain. The missing fields are named above; both now keep unrecognised keys as well, so a guest that gains a field before the SDK does still hashes correctly +- sdk: the JavaScript and Rust clients report a non-2xx answer with its status *and* the server's message. The JS transport ignored the status entirely, and its unix-socket branch never even parsed the status line, so an agent with no `/v1` mount — every pre-0.6 agent — answered a v1 call with an HTML 404 page that reached the caller as `failed to parse response`, which names neither the status nor the cause. The Rust clients discarded the other half: `error_for_status()` keeps the status line and throws away the body the agent puts its reason in, and on the unix path the crate's JSON helper insists on deserializing the *error* body too, so the same HTML page came back as a JSON parse failure. Both now raise `HTTP : ` — the prpc `error` field when the body is one, otherwise the body itself, bounded to a few hundred characters so an error page cannot become the whole message. The Python and Go SDKs already carried both. Only failures read differently: the wire, the success path and the timeout/abort behaviour are unchanged, and the agent's own explanations still arrive verbatim inside the error — `EmitEvent`'s removal message now as `HTTP 400: EmitEvent was removed in dstack 0.6.0…` ### Changed - kms: client certificates are authenticated by the attestation they carry rather than by their issuer. Rocket configures mutual TLS through rustls' `WebPkiClientVerifier`, which pins a CA — but an RA-TLS certificate is self-issued and carries its identity in a TEE quote, so there is nothing to chain to. `GetTempCaCert` bridged the gap by handing every caller a shared CA private key purely so the minted certificate would chain somewhere; the CA established nothing (its key is public by design, and the endpoint is unauthenticated) and the check that has always carried the meaning is the quote verification that runs afterwards. The KMS now hands rustls a verifier that requires an attestation and ignores the issuer. Nothing changes for callers: guests and KMS-to-KMS onboarding still mint their client certificates from the temp CA, and those are now accepted for the attestation they carry. What changes is that the TLS layer went from admitting any certificate signed by a public key to requiring an attested one, and that a self-issued certificate is now accepted — which is what lets callers be migrated off `GetTempCaCert` in a follow-up. `[rpc.tls.mutual]` is no longer the trust anchor and is dropped from `kms.toml` and the KMS config templates; leaving it in an existing deployment's config is inert. The gateway's `[tls.mutual]` is unaffected — it pins the KMS root CA, which is a real trust anchor @@ -53,9 +54,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - guest-agent: both unversioned surfaces are closed at exactly v0.5.11, and every new capability goes to `dstack.guest.v1` instead. Everything added to them after v0.5.11 never shipped in a release, so it is removed rather than frozen in: `AttestGpu` and `Attest`'s GPU-evidence field on the internal service, `AttestAppKey` and `Health` on the external one. All of those are v1 features now. No released client is affected -- both services are now byte-identical to v0.5.11 apart from doc comments and `reserved` statements holding the interim field numbers, so an unreleased `next` build in a dev environment cannot have one of them silently absorbed by a future field. "Frozen except for additions" is how all of them arrived in the first place - guest-agent: `GetQuote` is restricted to Intel TDX. It used to answer on every platform, returning an empty `quote` plus a `GetQuoteResponse.attestation` field carrying the versioned attestation — a shape only that one RPC produced, and one `Attest` already covers. Platforms without TDX now get an error telling them to call `Attest`, and the `attestation` field is gone from the RPC and from the Rust, Python, Go and JS SDKs. GCP Confidential VMs still get an answer — the gate is whether the platform has a TDX quote — but only the TDX half of one: `GetQuoteResponse` has no field for the vTPM quote GCP's verification also binds, so relying parties there want `Attest`, and the docs say so. `Tappd.TdxQuote`/`RawQuote` share the same backend path, so they fail closed there too instead of returning an empty quote - guest-agent: `Worker.GetAttestationForAppKey` is **retained**, unchanged and frozen, and v1 ships no counterpart. The method attests the key v0's KDF derives at path `vms` with purpose `signing`, and no v1 `GetKey(domain, algorithm)` can return that key -- different salt, different `info`, no `purpose` input -- so a v1 counterpart would have handed a pure-v1 app an attestation of a public key whose private half it could not obtain, which is worse than having no method because it looks like it works. A v1 app attests its own key instead: derive it at `/v1/GetKey`, commit the public key into `report_data`, call `/v1/Attest`, and serve the result to relying parties itself. That is strictly more capable, since the app chooses which key and which commitment format rather than being limited to the one the agent would derive. Legacy flows keep using the frozen method; it remains Intel TDX only, because it returns a `GetQuoteResponse` +- sdk: the Go SDK's v1 `IssueCert` defaults `usage_server_auth` to true, as the Rust, Python and JavaScript v1 clients already did. Go was the odd one out, so the same argument-free call produced a certificate that could serve TLS in three languages and one that could not in the fourth — and a certificate you cannot serve with is useless to most callers. `WithCertUsageServerAuth(false)` opts out. v0's `GetTlsKey` keeps its `false` default deliberately: that is what the released 0.5.x Go SDK sent, and `DstackClientV0` mirrors released behaviour rather than the better choice +- sdk: the JavaScript v1 `issueCert` response no longer carries a raw-bytes accessor. `asUint8Array()` is **removed rather than renamed**: it existed to feed the private key into the blockchain adapters, and v1 has no chain-flavoured surface. `IssueCert` returns TLS material, PEM is the form a TLS stack takes, and a caller who genuinely needs DER converts it with a standard library. The Rust, Python and Go v1 clients already returned the PEM string and the chain alone, so all four now agree. v0's `GetTlsKeyResponse.asUint8Array` is untouched — released API, and the viem and solana adapters depend on its truncating behaviour +- sdk: the JavaScript v1 GPU evidence bundle's `asUint8Array()` is renamed `decodeEvidence()`, matching Python's and Rust's `decode_evidence` and Go, which hands back the decoded `Evidence` bytes directly. The name now says what the bytes are — the vendor's evidence, hex off the wire and decoded byte-exact, because sha256 over precisely those bytes is what the measured `gpu-attestation` event commits to ### Removed +- sdk: `TlsKeyOptions.path` in the JavaScript SDK. `GetTlsKeyArgs` has no such field and `getTlsKey` never read it, so a caller who set it was silently ignored. Breaking at the type level only, and only for code whose value was already being discarded. `deriveKey`'s `path` is a real, deprecated Tappd-era parameter and stays; the Python, Rust and Go v0 TLS-key options never carried one - guest-agent: the `EmitEvent` RPC no longer records anything -- runtime RTMR3 events are system-owned in 0.6.0, so an app can no longer extend the measurement chain. The method itself stays on the unversioned path and always fails with an error naming the removal, rather than being deleted outright: a deleted method answers HTTP 404 `Service not found: EmitEvent`, which tells a 0.5.x caller nothing about why its events stopped being recorded, while the kept stub fails with a message naming the removal and pointing at `report_data`. **Breaking:** any app extending RTMR3 at runtime must stop; bind app data through `report_data` instead, which is what most callers wanted anyway diff --git a/sdk/go/dstack/client.go b/sdk/go/dstack/client.go index 3d2ebe6b8..03f0cb5a2 100644 --- a/sdk/go/dstack/client.go +++ b/sdk/go/dstack/client.go @@ -211,7 +211,11 @@ func NewDstackClientV0(opts ...DstackClientOption) *DstackClientV0 { // TlsKeyOption defines a function type for TLS key options type TlsKeyOption func(*tlsKeyOptions) -// tlsKeyOptions holds all the optional parameters for GetTlsKey +// tlsKeyOptions holds all the optional parameters for GetTlsKey. +// +// usageServerAuth stays false by default. That is what the released 0.5.x Go +// SDK sent, and this surface mirrors released behavior rather than the better +// choice -- v1's IssueCert defaults it to true. type tlsKeyOptions struct { subject string altNames []string diff --git a/sdk/go/dstack/client_v1.go b/sdk/go/dstack/client_v1.go index e5555f3cb..78a6cf7a4 100644 --- a/sdk/go/dstack/client_v1.go +++ b/sdk/go/dstack/client_v1.go @@ -263,7 +263,11 @@ func WithCertAppInfo(enabled bool) IssueCertV1Option { // is not derived from the app identity -- GetKey is the method that derives a // stable, attestable key. v0 called this GetTlsKey. func (c *DstackClientV1) IssueCert(ctx context.Context, options ...IssueCertV1Option) (*IssueCertV1Response, error) { - opts := &issueCertV1Options{} + // usageServerAuth starts true, matching the v1 default in the Rust, Python + // and JS SDKs: a certificate that cannot be served with is useless to most + // callers, and a v1 default that differs per language is a trap. Opt out + // with WithCertUsageServerAuth(false). + opts := &issueCertV1Options{usageServerAuth: true} for _, option := range options { option(opts) } diff --git a/sdk/go/dstack/client_v1_test.go b/sdk/go/dstack/client_v1_test.go index e6788a236..a3e4d1281 100644 --- a/sdk/go/dstack/client_v1_test.go +++ b/sdk/go/dstack/client_v1_test.go @@ -407,6 +407,62 @@ func TestV1IssueCertKeyIsFreshPerCall(t *testing.T) { } } +// The v1 default is usage_server_auth: true, the same as the Rust, Python and +// JS v1 clients -- a certificate the caller cannot serve with is useless to most +// of them. The default is only observable on the wire, so assert it there, and +// assert the opt-out reaches the wire too. +func TestV1IssueCertDefaultsToServerAuth(t *testing.T) { + var payload map[string]interface{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + t.Errorf("failed to decode request: %v", err) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"key":"","certificate_chain":[]}`)) + })) + defer server.Close() + + client := dstack.NewDstackClientV1(dstack.WithEndpoint(server.URL)) + ctx := context.Background() + + if _, err := client.IssueCert(ctx); err != nil { + t.Fatal(err) + } + if payload["usage_server_auth"] != true { + t.Errorf("expected usage_server_auth to default to true, got: %v", payload["usage_server_auth"]) + } + + if _, err := client.IssueCert(ctx, dstack.WithCertUsageServerAuth(false)); err != nil { + t.Fatal(err) + } + if payload["usage_server_auth"] != false { + t.Errorf("expected WithCertUsageServerAuth(false) to opt out, got: %v", payload["usage_server_auth"]) + } +} + +// v0 sent usage_server_auth: false when the caller said nothing, and that is +// what the released 0.5.x Go SDK did. The frozen surface keeps it, so the two +// defaults differ on purpose rather than by oversight. +func TestV0GetTlsKeyKeepsTheReleasedServerAuthDefault(t *testing.T) { + var payload map[string]interface{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + t.Errorf("failed to decode request: %v", err) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"key":"","certificate_chain":[]}`)) + })) + defer server.Close() + + client := dstack.NewDstackClientV0(dstack.WithEndpoint(server.URL)) + if _, err := client.GetTlsKey(context.Background()); err != nil { + t.Fatal(err) + } + if payload["usage_server_auth"] != false { + t.Errorf("expected the frozen surface to keep sending false, got: %v", payload["usage_server_auth"]) + } +} + // Version selection is by URL path alone: every v1 method must post under /v1. func TestV1MethodsPostUnderTheV1Prefix(t *testing.T) { paths := make(chan string, 1) diff --git a/sdk/js/README.md b/sdk/js/README.md index 6d010efb7..7706f2cd6 100644 --- a/sdk/js/README.md +++ b/sdk/js/README.md @@ -75,9 +75,10 @@ const cert = await client.issueCert({ }) cert.key // PEM-encoded private key cert.certificate_chain // PEM entries, leaf first -cert.asUint8Array(32) // the key as raw DER bytes ``` +PEM and nothing else: v0 attached a raw-bytes accessor to this response, but it was there to feed the key into the blockchain adapters, and v1 has no chain-flavored surface. This is TLS material, PEM is what a TLS stack takes, and DER is one standard-library call away if you want it. + Options: `subject`, `altNames`, `usageRaTls`, `usageServerAuth` (default `true`), `usageClientAuth` (default `false`), `withAppInfo`, `notBefore`, `notAfter` (Unix seconds). The key is freshly generated on every call and is not derived from the app identity — two identical requests return two unrelated keys. v0 called this `getTlsKey`, which named the by-product rather than the request. Use `getKey` for stable, attestable material. @@ -114,13 +115,13 @@ const { attestation } = await client.attest('app-state-snapshot') ```typescript const { attestation, boottime_gpu_evidence } = await client.attest('snapshot', true) for (const bundle of boottime_gpu_evidence) { - console.log(bundle.vendor, bundle.format, bundle.asUint8Array()) + console.log(bundle.vendor, bundle.format, bundle.decodeEvidence()) } ``` `boottime_gpu_evidence` is a list of the same `GpuEvidenceBundleV1` objects `attestGpu` returns, so one parser serves both; `format` is what tells them apart (`nvidia-nvattest-boottime-json-v1` here, `nvidia-nvattest-collect-evidence-json-v1` there). Absence is the empty list, not a sentinel: it is empty unless the flag was set and the guest has boot-time output. -That evidence is not bound to `reportData` — nvattest ran at boot against its own nonce. Bind it by replaying the runtime event log and comparing sha256 of the bytes `asUint8Array()` returns — exactly the bytes nvattest emitted, so do not parse and re-serialize the JSON — against `evidence_sha256` in the measured `gpu-attestation` event. +That evidence is not bound to `reportData` — nvattest ran at boot against its own nonce. Bind it by replaying the runtime event log and comparing sha256 of the bytes `decodeEvidence()` returns — exactly the bytes nvattest emitted, so do not parse and re-serialize the JSON — against `evidence_sha256` in the measured `gpu-attestation` event. ### `attestGpu(nonce)` @@ -129,13 +130,13 @@ Collect GPU evidence now, against a 32-byte nonce you choose. This answers "is t ```typescript const { bundles } = await client.attestGpu(crypto.randomBytes(32)) for (const bundle of bundles) { - console.log(bundle.vendor, bundle.format, bundle.asUint8Array()) + console.log(bundle.vendor, bundle.format, bundle.decodeEvidence()) } ``` The nonce must be exactly 32 bytes — SPDM fixes the length, and dstack applies no transform, so you can compare these bytes directly against the `eat_nonce` claim. Hash a longer challenge yourself. -Select a verifier from each bundle's `vendor` and `format`, then check the signature, certificate chain, measurements and embedded nonce. `evidence` is opaque and hex-encoded on the wire; `asUint8Array()` gives the vendor's bytes verbatim. It does not by itself bind the GPU to this CVM. +Select a verifier from each bundle's `vendor` and `format`, then check the signature, certificate chain, measurements and embedded nonce. `evidence` is opaque and hex-encoded on the wire; `decodeEvidence()` gives the vendor's bytes verbatim. It does not by itself bind the GPU to this CVM. ### `info()` diff --git a/sdk/js/src/__tests__/index-v1.test.ts b/sdk/js/src/__tests__/index-v1.test.ts index 720b7f2dc..a7a0abd28 100644 --- a/sdk/js/src/__tests__/index-v1.test.ts +++ b/sdk/js/src/__tests__/index-v1.test.ts @@ -48,14 +48,13 @@ describe('DstackClientV1', () => { expect(first.key).not.toBe(second.key) }) - it('should expose the key as a uint8array of the requested length', async () => { + it('should hand back the key as PEM and nothing else', async () => { const client = new DstackClientV1() - const result = await client.issueCert() - const full = result.asUint8Array() - const truncated = result.asUint8Array(32) - expect(full).toBeInstanceOf(Uint8Array) - expect(truncated.length).toBe(32) - expect(truncated.length).not.toBe(full.length) + const result = await client.issueCert() as any + // The raw-bytes accessor v0 carries fed the chain adapters, and v1 has no + // chain surface. All four SDKs return the PEM string alone here. + expect(result.asUint8Array).toBeUndefined() + expect(Object.keys(result).sort()).toEqual(['__name__', 'certificate_chain', 'key']) }) it('should reject a validity window that ends before it starts', async () => { @@ -116,6 +115,24 @@ describe('DstackClientV1', () => { expect(parent.key).not.toEqual(child.key) }) + // Over the unix socket the SDK writes the HTTP framing itself, and it used + // to declare `Content-Length` as the string's UTF-16 code-unit count while + // sending UTF-8 -- so a domain like this one arrived truncated and the + // surplus bytes were left in the stream. The simulator runs on that path. + it('should send a domain with multi-byte characters intact', async () => { + const client = new DstackClientV1() + const first = await client.getKey('café-storage', 'secp256k1') + expect(first.key.length).toBe(32) + + // Truncation would land on some prefix of the domain, and derivation binds + // the domain, so a mangled request cannot derive the same key twice the + // same way -- nor differ from a genuinely different domain. + const again = await client.getKey('café-storage', 'secp256k1') + expect(again.key).toEqual(first.key) + const truncated = await client.getKey('caf', 'secp256k1') + expect(truncated.key).not.toEqual(first.key) + }) + it('should accept an empty domain', async () => { const client = new DstackClientV1() const result = await client.getKey('', 'secp256k1') @@ -162,7 +179,7 @@ describe('DstackClientV1', () => { // Assigning one to the other is the assertion: one parser, both methods. const bundles: GpuEvidenceBundleV1[] = result.boottime_gpu_evidence for (const bundle of bundles) { - expect(bundle.asUint8Array()).toBeInstanceOf(Uint8Array) + expect(bundle.decodeEvidence()).toBeInstanceOf(Uint8Array) } }) @@ -183,9 +200,10 @@ describe('DstackClientV1', () => { it('should surface the agent failure when there is no GPU', async () => { const client = new DstackClientV1() // The simulator ships no nvattest, so this must fail fast and clearly - // rather than hang for the attestation timeout. + // rather than hang for the attestation timeout -- with the status the + // agent answered and its own explanation, not a parse error. await expect(() => client.attestGpu(new Uint8Array(32).fill(0xab))).rejects.toThrow( - 'GPU attestation' + /^HTTP 4\d\d: .*GPU attestation/ ) }) }) @@ -230,7 +248,7 @@ describe('DstackClientV1', () => { expect(bundle.format).toBe('nvidia-nvattest-boottime-json-v1') // Byte-exact: sha256 over these bytes is what `evidence_sha256` in the // measured `gpu-attestation` event commits to. - expect(Buffer.from(bundle.asUint8Array()).toString('utf8')).toBe(nvattest_output) + expect(Buffer.from(bundle.decodeEvidence()).toString('utf8')).toBe(nvattest_output) }) }) @@ -242,7 +260,7 @@ describe('DstackClientV1', () => { const on_demand: GpuEvidenceBundleV1 = collected.bundles[0] // Only `format` separates them, so one parser handles both. expect(on_demand.format).toBe('nvidia-nvattest-collect-evidence-json-v1') - expect(on_demand.asUint8Array()).toEqual(boottime.asUint8Array()) + expect(on_demand.decodeEvidence()).toEqual(boottime.decodeEvidence()) }) }) }) diff --git a/sdk/js/src/__tests__/index.test.ts b/sdk/js/src/__tests__/index.test.ts index e4ca2ab39..f86c5a221 100644 --- a/sdk/js/src/__tests__/index.test.ts +++ b/sdk/js/src/__tests__/index.test.ts @@ -274,8 +274,10 @@ describe('DstackClientV0', () => { const client = new DstackClientV0() // The 0.6.0 agent always fails this. A caller that gets a resolved promise // would believe the event was measured, which is the one wrong answer here. + // The agent answers 4xx, so the message arrives through the transport's + // status reporting rather than through the body check. await expect(() => client.emitEvent('test-event', 'payload')).rejects.toThrow( - 'EmitEvent was removed in dstack 0.6.0' + 'HTTP 400: EmitEvent was removed in dstack 0.6.0' ) }) }) diff --git a/sdk/js/src/__tests__/send-rpc-request.test.ts b/sdk/js/src/__tests__/send-rpc-request.test.ts index aa5a7de8e..d4ede7273 100644 --- a/sdk/js/src/__tests__/send-rpc-request.test.ts +++ b/sdk/js/src/__tests__/send-rpc-request.test.ts @@ -3,7 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 import { expect, describe, it, vi, beforeEach, afterEach } from 'vitest' -import { send_rpc_request, __version__ } from '../send-rpc-request' +import { send_rpc_request, parse_status_code, __version__ } from '../send-rpc-request' import http from 'http' import https from 'https' import net from 'net' @@ -13,6 +13,21 @@ vi.mock('http') vi.mock('https') vi.mock('net') +describe('parse_status_code', () => { + it('should read the code out of a status line', () => { + expect(parse_status_code('HTTP/1.1 200 OK')).toBe(200) + expect(parse_status_code('HTTP/1.1 404 Not Found')).toBe(404) + expect(parse_status_code('HTTP/1.0 500 Internal Server Error')).toBe(500) + }) + + it('should treat an unreadable status line as unsuccessful', () => { + // Not 2xx, so an answer nobody can classify is reported rather than + // silently passed off as a success. + expect(parse_status_code('')).toBe(0) + expect(parse_status_code('garbage')).toBe(0) + }) +}) + describe('send_rpc_request', () => { let mockHttpRequest: any let mockHttpsRequest: any @@ -143,6 +158,49 @@ describe('send_rpc_request', () => { await expect(send_rpc_request(endpoint, path, payload)).rejects.toThrow('connection failed') }) + it('should report a non-2xx with its status and the prpc error text', async () => { + mockRes.statusCode = 404 + mockHttpRequest.mockImplementation((url, options, callback) => { + callback(mockRes) + + const dataCallback = mockRes.on.mock.calls.find(call => call[0] === 'data')?.[1] + const endCallback = mockRes.on.mock.calls.find(call => call[0] === 'end')?.[1] + + if (dataCallback) dataCallback('{"error": "Service not found: GetKeyX"}') + if (endCallback) endCallback() + + return mockReq + }) + + await expect(send_rpc_request('http://localhost:3000', '/GetKeyX', '{}')) + .rejects.toThrow('HTTP 404: Service not found: GetKeyX') + }) + + it('should report a non-2xx with a bounded snippet when the body is not JSON', async () => { + // What a pre-0.6 agent answers a `/v1` call with: no `error` field, and a + // whole page of it. Without the status this reads as a parse failure. + const page = `${'

not found

'.repeat(200)}` + mockRes.statusCode = 404 + mockHttpRequest.mockImplementation((url, options, callback) => { + callback(mockRes) + + const dataCallback = mockRes.on.mock.calls.find(call => call[0] === 'data')?.[1] + const endCallback = mockRes.on.mock.calls.find(call => call[0] === 'end')?.[1] + + if (dataCallback) dataCallback(page) + if (endCallback) endCallback() + + return mockReq + }) + + const error = await send_rpc_request('http://localhost:3000', '/v1/Version', '{}') + .catch((err: Error) => err) + expect(error).toBeInstanceOf(Error) + expect((error as Error).message).toContain('HTTP 404: ') + expect((error as Error).message.length).toBeLessThan(page.length) + expect((error as Error).message.endsWith('...')).toBe(true) + }) + it('should handle invalid JSON response', async () => { const endpoint = 'http://localhost:3000' const path = '/api/test' @@ -181,6 +239,73 @@ describe('send_rpc_request', () => { expect(mockNetConnect).toHaveBeenCalledWith({ path: endpoint }, expect.any(Function)) }) + // The unix branch speaks HTTP by hand, so it is the only one that has to + // read the status off the wire itself -- and until it did, a 404 reached the + // caller as "failed to parse response". + it('should report a non-2xx read off the status line', async () => { + const body = '{"error": "Service not found: GetKeyX"}' + mockNetConnect.mockImplementation(() => { + setTimeout(() => { + const dataCallback = mockClient.on.mock.calls.find(call => call[0] === 'data')?.[1] + const endCallback = mockClient.on.mock.calls.find(call => call[0] === 'end')?.[1] + dataCallback( + `HTTP/1.1 404 Not Found\r\nContent-Type: application/json\r\nContent-Length: ${body.length}\r\n\r\n${body}` + ) + endCallback() + }, 0) + return mockClient + }) + + await expect(send_rpc_request('/tmp/socket', '/GetKeyX', '{}')) + .rejects.toThrow('HTTP 404: Service not found: GetKeyX') + }) + + it('should still resolve a 200 read off the status line', async () => { + const body = '{"version":"0.6.0"}' + mockNetConnect.mockImplementation(() => { + setTimeout(() => { + const dataCallback = mockClient.on.mock.calls.find(call => call[0] === 'data')?.[1] + const endCallback = mockClient.on.mock.calls.find(call => call[0] === 'end')?.[1] + dataCallback(`HTTP/1.1 200 OK\r\nContent-Length: ${body.length}\r\n\r\n${body}`) + endCallback() + }, 0) + return mockClient + }) + + await expect(send_rpc_request('/tmp/socket', '/Version', '{}')).resolves.toEqual({ + version: '0.6.0', + }) + }) + + // `payload.length` counts UTF-16 code units and the socket emits UTF-8, so a + // request with any non-ASCII field declared a body shorter than the one it + // sent: the agent parsed truncated JSON and the surplus bytes stayed in the + // stream. Both halves of the framing have to count bytes. + it('should declare the payload length in bytes, not code units', async () => { + const payload = JSON.stringify({ domain: 'café', algorithm: 'secp256k1' }) + expect(Buffer.byteLength(payload)).toBeGreaterThan(payload.length) + + const body = '{"ok":true}' + mockNetConnect.mockImplementation((options, callback) => { + // Deferred, as a real socket defers it: the connect handler writes + // through the `client` binding that createConnection is still returning. + setTimeout(() => { + callback() + const dataCallback = mockClient.on.mock.calls.find(call => call[0] === 'data')?.[1] + const endCallback = mockClient.on.mock.calls.find(call => call[0] === 'end')?.[1] + dataCallback(`HTTP/1.1 200 OK\r\nContent-Length: ${body.length}\r\n\r\n${body}`) + endCallback() + }, 0) + return mockClient + }) + + await send_rpc_request('/tmp/socket', '/v1/GetKey', payload) + + const written = mockClient.write.mock.calls.map((call: any[]) => call[0]) + expect(written).toContain(`Content-Length: ${Buffer.byteLength(payload)}\r\n`) + expect(written).not.toContain(`Content-Length: ${payload.length}\r\n`) + }) + it('should handle Unix socket connection errors', async () => { const endpoint = '/tmp/socket' const path = '/api/test' diff --git a/sdk/js/src/index.ts b/sdk/js/src/index.ts index a7c3926a5..a509973da 100644 --- a/sdk/js/src/index.ts +++ b/sdk/js/src/index.ts @@ -124,6 +124,9 @@ export function to_hex(data: string | Buffer | Uint8Array): string { return (data as Buffer).toString('hex'); } +// The v0 byte accessor, and v0's alone: the chain adapters feed it into seed +// derivation, so its truncating and zero-padding behaviour is load-bearing on +// the frozen surface. v1's IssueCert hands back PEM and nothing else. function x509key_to_uint8array(pem: string, max_length?: number) { const content = pem.replace(/-----BEGIN PRIVATE KEY-----/, '') .replace(/-----END PRIVATE KEY-----/, '') @@ -140,7 +143,6 @@ function x509key_to_uint8array(pem: string, max_length?: number) { } export interface TlsKeyOptions { - path?: string; subject?: string; altNames?: string[]; usageRaTls?: boolean; @@ -182,11 +184,11 @@ function throwOnRpcError(result: unknown): void { * which is the point of the wire message being shared. */ function to_gpu_evidence_bundles( - bundles: Array> | undefined, + bundles: Array> | undefined, ): GpuEvidenceBundleV1[] { return (bundles ?? []).map(bundle => Object.freeze({ ...bundle, - asUint8Array: () => new Uint8Array(Buffer.from(bundle.evidence, 'hex')), + decodeEvidence: () => new Uint8Array(Buffer.from(bundle.evidence, 'hex')), })) } @@ -594,12 +596,18 @@ export interface IssueCertOptionsV1 { export interface IssueCertResponseV1 { __name__: Readonly<'IssueCertResponseV1'> - /** The private key the agent generated for this certificate, PEM-encoded. */ + /** + * The private key the agent generated for this certificate, PEM-encoded. + * + * PEM and nothing else. v0 attached a raw-bytes accessor here, but it existed + * to feed the key into the blockchain adapters, and v1 has no chain-flavored + * surface: this is TLS material, PEM is what a TLS stack takes, and a caller + * who genuinely wants DER converts it with a standard library. The other + * three SDKs' v1 clients return the PEM string alone too. + */ key: string /** The certificate chain, leaf first, each entry PEM-encoded. */ certificate_chain: string[] - - asUint8Array: (max_length?: number) => Uint8Array } export interface GetKeyResponseV1 { @@ -626,7 +634,7 @@ export interface AttestResponseV1 { * * Not bound to `report_data`: nvattest ran at boot against its own nonce. * Bind it by replaying the runtime event log and comparing sha256 of the - * bytes `asUint8Array()` returns against `evidence_sha256` in the measured + * bytes `decodeEvidence()` returns against `evidence_sha256` in the measured * `gpu-attestation` event. */ boottime_gpu_evidence: GpuEvidenceBundleV1[] @@ -661,7 +669,7 @@ export interface GpuEvidenceBundleV1 { * measured `gpu-attestation` event, so parsing and re-serialising the JSON * breaks the comparison. */ - asUint8Array: () => Uint8Array + decodeEvidence: () => Uint8Array } export interface AttestGpuResponseV1 { @@ -786,10 +794,8 @@ export class DstackClientV1 { const result = await send_rpc_request<{ key: string, certificate_chain: string[] }>( this.endpoint, '/v1/IssueCert', JSON.stringify(raw)) throwOnRpcError(result) - const asUint8Array = (length?: number) => x509key_to_uint8array(result.key, length) return Object.freeze({ ...result, - asUint8Array, __name__: 'IssueCertResponseV1' as const, }) } @@ -847,7 +853,7 @@ export class DstackClientV1 { const payload = JSON.stringify({ report_data: hex, include_boottime_gpu_evidence }) const result = await send_rpc_request<{ attestation: string, - boottime_gpu_evidence?: Array>, + boottime_gpu_evidence?: Array>, }>(this.endpoint, '/v1/Attest', payload) throwOnRpcError(result) return Object.freeze({ @@ -874,7 +880,7 @@ export class DstackClientV1 { } const payload = JSON.stringify({ nonce: to_hex(nonce) }) const result = await send_rpc_request<{ - bundles?: Array>, + bundles?: Array>, }>(this.endpoint, '/v1/AttestGpu', payload) throwOnRpcError(result) return Object.freeze({ diff --git a/sdk/js/src/send-rpc-request.ts b/sdk/js/src/send-rpc-request.ts index 97805cfcb..b92716c35 100644 --- a/sdk/js/src/send-rpc-request.ts +++ b/sdk/js/src/send-rpc-request.ts @@ -8,6 +8,65 @@ import net from 'net' export const __version__ = "0.6.0" +/** + * How much of a server response an error may quote. + * + * An agent with no route for the path answers with an HTML page, and pasting + * a whole page into an exception message helps nobody. + */ +const MAX_ERROR_BODY = 300 + +function truncate(text: string): string { + return text.length > MAX_ERROR_BODY ? `${text.slice(0, MAX_ERROR_BODY)}...` : text +} + +/** + * What the server said, as far as it can be made out. + * + * A prpc handler that refuses answers `{"error": "..."}`, and that field is the + * only part worth showing. A request that never reached a handler -- a `/v1` + * call against a pre-0.6 agent -- comes back as an HTML error page instead, and + * then the raw body is the only clue there is. + */ +function serverErrorText(body: string): string { + const text = body.trim() + if (!text) { + return '(empty response body)' + } + try { + const parsed = JSON.parse(text) + if (parsed && typeof parsed === 'object' && typeof parsed.error === 'string') { + return truncate(parsed.error) + } + } catch { + // Not JSON, so the body is the message. + } + return truncate(text) +} + +function httpError(statusCode: number, body: string): Error { + return new Error(`HTTP ${statusCode}: ${serverErrorText(body)}`) +} + +function parseError(body: string): Error { + return new Error(`failed to parse response: ${truncate(body.trim())}`) +} + +function isSuccess(statusCode: number): boolean { + return statusCode >= 200 && statusCode < 300 +} + +/** + * Read the status code out of an HTTP status line (`HTTP/1.1 404 Not Found`). + * + * Exported for the unix-socket branch's tests: that branch speaks HTTP by hand, + * so nothing else parses this for it. An unreadable line yields 0, which is not + * a success code and so gets reported rather than passed off as one. + */ +export function parse_status_code(statusLine: string): number { + const code = Number.parseInt(statusLine.split(' ')[1], 10) + return Number.isNaN(code) ? 0 : code +} export function send_rpc_request(endpoint: string, path: string, payload: string, timeoutMs?: number): Promise { return new Promise((resolve, reject) => { @@ -28,6 +87,21 @@ export function send_rpc_request(endpoint: string, path: string, payloa } } + // Reporting the status is the whole point of reading it: a caller that only + // ever sees "failed to parse response" cannot tell a 404 from a corrupt + // body, and 404 is what a pre-0.6 agent answers every `/v1` call with. + const settle = (statusCode: number, body: string) => { + if (!isSuccess(statusCode)) { + safeReject(httpError(statusCode, body)) + return + } + try { + safeResolve(JSON.parse(body) as T) + } catch (error) { + safeReject(parseError(body)) + } + } + const timeout = setTimeout(() => { abortController.abort() safeReject(new Error('request timed out')) @@ -65,12 +139,7 @@ export function send_rpc_request(endpoint: string, path: string, payloa }) res.on('end', () => { cleanup() - try { - const result = JSON.parse(data) - safeResolve(result as T) - } catch (error) { - safeReject(new Error('failed to parse response')) - } + settle(res.statusCode ?? 0, data) }) }) @@ -90,7 +159,12 @@ export function send_rpc_request(endpoint: string, path: string, payloa client.write(`POST ${path} HTTP/1.1\r\n`) client.write(`Host: localhost\r\n`) client.write(`Content-Type: application/json\r\n`) - client.write(`Content-Length: ${payload.length}\r\n`) + // Byte length, not `payload.length`: JS strings count UTF-16 code units + // and the socket emits UTF-8, so any non-ASCII field -- a `getKey` + // domain, a certificate subject -- declared a body shorter than the one + // sent. The agent then parsed truncated JSON and the surplus bytes were + // left in the stream to corrupt whatever read next. + client.write(`Content-Length: ${Buffer.byteLength(payload)}\r\n`) client.write('\r\n') client.write(payload) }) @@ -98,6 +172,7 @@ export function send_rpc_request(endpoint: string, path: string, payloa let data = '' let headers: Record = {} let headersParsed = false + let statusCode = 0 let contentLength = 0 let bodyData = '' @@ -107,7 +182,9 @@ export function send_rpc_request(endpoint: string, path: string, payloa const headerEndIndex = data.indexOf('\r\n\r\n') if (headerEndIndex !== -1) { const headerLines = data.slice(0, headerEndIndex).split('\r\n') - headerLines.forEach(line => { + // The first line is the status line, not a header. + statusCode = parse_status_code(headerLines[0]) + headerLines.slice(1).forEach(line => { const [key, value] = line.split(': ') if (key && value) { headers[key.toLowerCase()] = value @@ -128,12 +205,7 @@ export function send_rpc_request(endpoint: string, path: string, payloa client.on('end', () => { cleanup() - try { - const result = JSON.parse(bodyData.slice(0, contentLength)) - safeResolve(result as T) - } catch (error) { - safeReject(new Error('failed to parse response')) - } + settle(statusCode, bodyData.slice(0, contentLength)) }) client.on('error', (error) => { diff --git a/sdk/rust/src/dstack_client.rs b/sdk/rust/src/dstack_client.rs index a0acdd9a0..eb94f5cf0 100644 --- a/sdk/rust/src/dstack_client.rs +++ b/sdk/rust/src/dstack_client.rs @@ -4,9 +4,9 @@ // // SPDX-License-Identifier: Apache-2.0 -use anyhow::Result; +use anyhow::{anyhow, Context, Result}; use hex::encode as hex_encode; -use http_client_unix_domain_socket::{ClientUnix, Method}; +use http_client_unix_domain_socket::{Body, ClientUnix, ErrorAndResponse, Method}; use reqwest::Client; use serde::{de::DeserializeOwned, Serialize}; use serde_json::{json, Value}; @@ -60,6 +60,102 @@ pub enum ClientKind { pub trait BaseClient {} +/// How much of a server response an error may quote. +/// +/// An agent with no route for the path answers with an HTML page, and pasting a +/// whole page into an error helps nobody. +const MAX_ERROR_BODY: usize = 512; + +fn truncate(text: &str) -> String { + match text.char_indices().nth(MAX_ERROR_BODY) { + Some((end, _)) => format!("{}...", &text[..end]), + None => text.to_string(), + } +} + +/// What the server said, as far as it can be made out. +/// +/// A prpc handler that refuses answers `{"error": "..."}` with a 4xx, and that +/// field is the only part worth showing. A request that never reached a handler +/// -- a `/v1` call against a pre-0.6 agent -- comes back as an HTML error page +/// instead, and then the raw body is the only clue there is. +fn server_error_text(body: &[u8]) -> String { + let Ok(text) = std::str::from_utf8(body) else { + return "(non-utf8 response body)".to_string(); + }; + let text = text.trim(); + if text.is_empty() { + return "(empty response body)".to_string(); + } + if let Ok(Value::Object(fields)) = serde_json::from_str::(text) { + if let Some(Value::String(message)) = fields.get("error") { + return truncate(message); + } + } + truncate(text) +} + +/// Turn a non-2xx response into an error naming both the status and the reason. +pub(crate) fn http_error(status: u16, body: &[u8]) -> anyhow::Error { + anyhow!("HTTP {status}: {}", server_error_text(body)) +} + +/// POST `payload` as JSON over TCP and return the raw response body. +pub(crate) async fn http_post( + base_url: &str, + path: &str, + payload: &S, +) -> Result> { + let url = format!( + "{}/{}", + base_url.trim_end_matches('/'), + path.trim_start_matches('/') + ); + let res = Client::new() + .post(&url) + .json(payload) + .header("Content-Type", "application/json") + .send() + .await?; + // Deliberately not `error_for_status()`: that discards the response body, + // which is exactly where the agent puts the reason it refused. + let status = res.status(); + let body = res.bytes().await?; + if !status.is_success() { + return Err(http_error(status.as_u16(), &body)); + } + Ok(body.to_vec()) +} + +/// POST `payload` as JSON over the guest-agent socket and return the raw body. +pub(crate) async fn unix_post( + endpoint: &str, + path: &str, + payload: &S, +) -> Result> { + let mut unix_client = ClientUnix::try_new(endpoint).await?; + // `send_request` rather than `send_request_json`: the JSON helper insists on + // deserializing the *error* body as well, so an HTML 404 from an agent that + // does not serve this path surfaces as a JSON parse failure rather than as + // the status that explains it. + let request = Body::from(serde_json::to_vec(payload)?); + match unix_client + .send_request( + path, + Method::POST, + &[("Content-Type", "application/json"), ("Host", "dstack")], + Some(request), + ) + .await + { + Ok((_status, body)) => Ok(body), + Err(ErrorAndResponse::ResponseUnsuccessful(status, body)) => { + Err(http_error(status.as_u16(), &body)) + } + Err(ErrorAndResponse::InternalError(err)) => Err(err.into()), + } +} + /// Client for the frozen v0 guest-agent surface. /// /// **Legacy.** New code should use [`crate::dstack_client_v1::DstackClientV1`], @@ -108,36 +204,11 @@ impl DstackClientV0 { path: &str, payload: &S, ) -> anyhow::Result { - match &self.client { - ClientKind::Http => { - let client = Client::new(); - let url = format!( - "{}/{}", - self.base_url.trim_end_matches('/'), - path.trim_start_matches('/') - ); - let res = client - .post(&url) - .json(payload) - .header("Content-Type", "application/json") - .send() - .await? - .error_for_status()?; - Ok(res.json().await?) - } - ClientKind::Unix => { - let mut unix_client = ClientUnix::try_new(&self.endpoint).await?; - let res = unix_client - .send_request_json::<_, _, Value>( - path, - Method::POST, - &[("Content-Type", "application/json"), ("Host", "dstack")], - Some(&payload), - ) - .await?; - Ok(res.1) - } - } + let body = match &self.client { + ClientKind::Http => http_post(&self.base_url, path, payload).await?, + ClientKind::Unix => unix_post(&self.endpoint, path, payload).await?, + }; + serde_json::from_slice(&body).context("failed to parse the response") } pub async fn get_key( @@ -261,3 +332,47 @@ impl DstackClientV0 { Ok(response) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn quotes_the_error_field_of_a_prpc_failure() { + let err = http_error(400, br#"{"error":"algorithm is not supported"}"#); + assert_eq!(err.to_string(), "HTTP 400: algorithm is not supported"); + } + + #[test] + fn quotes_the_body_when_it_is_not_a_prpc_failure() { + // What a pre-0.6 agent answers a `/v1` call with. + let err = http_error(404, b"\n404"); + assert_eq!( + err.to_string(), + "HTTP 404: \n404" + ); + } + + #[test] + fn bounds_the_quoted_body() { + let err = http_error(500, "x".repeat(MAX_ERROR_BODY * 2).as_bytes()); + // An HTML error page must not become the whole error message. + assert_eq!( + err.to_string().len(), + "HTTP 500: ".len() + MAX_ERROR_BODY + 3 + ); + assert!(err.to_string().ends_with("...")); + } + + #[test] + fn names_an_unreadable_body_rather_than_dropping_the_status() { + assert_eq!( + http_error(502, b"").to_string(), + "HTTP 502: (empty response body)" + ); + assert_eq!( + http_error(502, &[0xff, 0xfe]).to_string(), + "HTTP 502: (non-utf8 response body)" + ); + } +} diff --git a/sdk/rust/src/dstack_client_v1.rs b/sdk/rust/src/dstack_client_v1.rs index 3afa9edb2..32748d68d 100644 --- a/sdk/rust/src/dstack_client_v1.rs +++ b/sdk/rust/src/dstack_client_v1.rs @@ -16,14 +16,12 @@ use anyhow::{Context, Result}; use hex::encode as hex_encode; -use http_client_unix_domain_socket::{ClientUnix, Method}; -use reqwest::Client; use serde::{de::DeserializeOwned, Serialize}; -use serde_json::{json, Value}; +use serde_json::json; pub use dstack_sdk_types::dstack_v1::*; -use crate::dstack_client::{get_endpoint, BaseClient, ClientKind}; +use crate::dstack_client::{get_endpoint, http_post, unix_post, BaseClient, ClientKind}; /// Client for the v1 guest-agent surface. **This is the default client**, and /// what the unsuffixed [`crate::DstackClient`] names. @@ -68,32 +66,11 @@ impl DstackClientV1 { payload: &S, ) -> Result { let path = format!("/v1/{method}"); - match &self.client { - ClientKind::Http => { - let client = Client::new(); - let url = format!("{}{}", self.base_url.trim_end_matches('/'), path); - let res = client - .post(&url) - .json(payload) - .header("Content-Type", "application/json") - .send() - .await? - .error_for_status()?; - Ok(res.json().await?) - } - ClientKind::Unix => { - let mut unix_client = ClientUnix::try_new(&self.endpoint).await?; - let res = unix_client - .send_request_json::<_, _, Value>( - &path, - Method::POST, - &[("Content-Type", "application/json"), ("Host", "dstack")], - Some(&payload), - ) - .await?; - Ok(res.1) - } - } + let body = match &self.client { + ClientKind::Http => http_post(&self.base_url, &path, payload).await?, + ClientKind::Unix => unix_post(&self.endpoint, &path, payload).await?, + }; + serde_json::from_slice(&body).context("failed to parse the response") } /// Issue a certificate for this application. diff --git a/sdk/rust/tests/test_client.rs b/sdk/rust/tests/test_client.rs index bb84697dd..9084b4f84 100644 --- a/sdk/rust/tests/test_client.rs +++ b/sdk/rust/tests/test_client.rs @@ -195,3 +195,27 @@ async fn test_sign_then_verify_secp256k1_prehashed() { .valid ); } + +/// A refused prpc call answers 4xx with `{"error": "..."}`, and that message is +/// the whole value of the response -- `EmitEvent` exists in 0.6 only to explain +/// its own removal. Both the status and the agent's text have to reach the +/// caller, which `error_for_status()` and the unix client's default error both +/// used to throw away. +#[tokio::test] +async fn a_refused_call_reports_the_status_and_the_agent_message() { + let client = AsyncDstackClient::new(None); + let err = client + .emit_event("test-event".to_string(), b"payload".to_vec()) + .await + .unwrap_err(); + + let message = format!("{err:#}"); + assert!( + message.contains("HTTP 400"), + "expected the status in: {message}" + ); + assert!( + message.contains("EmitEvent was removed in dstack 0.6.0"), + "expected the agent's own explanation in: {message}" + ); +} diff --git a/sdk/rust/tests/test_client_v1.rs b/sdk/rust/tests/test_client_v1.rs index 6cd00d28e..b06adf874 100644 --- a/sdk/rust/tests/test_client_v1.rs +++ b/sdk/rust/tests/test_client_v1.rs @@ -229,3 +229,23 @@ async fn issue_cert_generates_a_fresh_key_per_call() { let second = client().issue_cert(config()).await.unwrap(); assert_ne!(first.key, second.key); } + +/// An agent that predates v1 has no `/v1` mount, so it answers with a plain +/// HTML 404 rather than a prpc error -- and that page is the only clue the +/// caller gets. The simulator's tappd socket serves no `/v1` either, so it +/// stands in for one here. +#[tokio::test] +async fn a_missing_v1_mount_is_reported_with_its_status() { + let endpoint = std::env::var("TAPPD_SIMULATOR_ENDPOINT") + .expect("TAPPD_SIMULATOR_ENDPOINT must point at the simulator"); + let err = DstackClientV1::new(Some(&endpoint)) + .version() + .await + .unwrap_err(); + + let message = format!("{err:#}"); + assert!( + message.starts_with("HTTP 404: "), + "expected the status and the server's page, got: {message}" + ); +} From 499c626d4e8289057579b78c37302192f4d75acf Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 09:58:26 -0700 Subject: [PATCH 2/5] fix(sdk-js): stop hand-writing HTTP over the guest agent socket An adversarial review found the Content-Length fix in the previous commit was half a fix. The write path counted bytes; the read path still compared that byte-counted `Content-Length` against a JS string's `.length`, which counts UTF-16 code units. For any response carrying a non-ASCII character the units never reach the bytes, so the client never decided the body was complete and waited out the agent's ten-second keep-alive instead. One accented character in an app-compose comment was enough to turn `info()` into a ten-second call and make `isReachable()` report a healthy agent as unreachable: measured at 15006ms against a request the agent answered in 9ms. The same branch also did `data += chunk`, which decodes each Buffer on its own, so a UTF-8 sequence split across two TCP reads became a replacement character on each side -- and the call resolved successfully, handing back quietly corrupted data. And with no `Content-Length` at all it sliced to nothing and reported "(empty response body)" for a body that was on the wire. All three are the same defect: a hand-written HTTP parser that confuses bytes with characters. It is gone. The unix branch now uses node's client over `socketPath`, which frames the request, de-chunks the response, and counts bytes where bytes are meant; both branches assemble responses as Buffers and decode once at the end. `agent: false` keeps the old one-connection-per-call behaviour -- node's default agent pools the socket, and an open socket keeps the process alive, so a script awaiting one `info()` would hang. The six unix tests that mocked `net.createConnection` are replaced by four that run against a real unix socket, including a multi-byte body served over a kept-alive connection and a UTF-8 sequence split across two writes. Neither defect was visible to the old fixtures, which fed the client a whole response as one ASCII string. For the same reason the remaining HTTP mocks now emit Buffers: node never emits strings there, and the string fixtures are what let this survive. Also from the review: the CHANGELOG claimed the success path was unchanged. It is not -- six JS v0 methods that never checked the response body used to resolve on a prpc failure and now reject, and the Rust unix path stops sending a duplicated Content-Type header. Both are stated now. Rust's truncation bound is renamed to say it counts characters, and Go's `WithCertUsageServerAuth` documents the default that the other three SDKs express in their signatures. --- CHANGELOG.md | 6 +- sdk/go/dstack/client_v1.go | 5 + sdk/js/src/__tests__/send-rpc-request.test.ts | 149 ++---------------- .../__tests__/send-rpc-request.unix.test.ts | 131 +++++++++++++++ sdk/js/src/send-rpc-request.ts | 119 ++++++-------- sdk/rust/src/dstack_client.rs | 14 +- 6 files changed, 208 insertions(+), 216 deletions(-) create mode 100644 sdk/js/src/__tests__/send-rpc-request.unix.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index cff89f955..2bd5ceb9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,7 +42,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - sdk: the Go SDK's `gpu_policy` silently dropped sub-fields it did not declare on a decode/re-hash round trip; it now carries them through like the rest of `Requirements` - bound VMM-to-guest RPCs and guest-agent dependency calls to prevent stalled peers from retaining request resources indefinitely - sdk: the Go and Python compose-hash helpers silently dropped every app-compose field they did not declare, so `getComposeHash` returned a digest for an app-compose that was not the one being deployed — and that digest is what gets whitelisted on chain. The missing fields are named above; both now keep unrecognised keys as well, so a guest that gains a field before the SDK does still hashes correctly -- sdk: the JavaScript and Rust clients report a non-2xx answer with its status *and* the server's message. The JS transport ignored the status entirely, and its unix-socket branch never even parsed the status line, so an agent with no `/v1` mount — every pre-0.6 agent — answered a v1 call with an HTML 404 page that reached the caller as `failed to parse response`, which names neither the status nor the cause. The Rust clients discarded the other half: `error_for_status()` keeps the status line and throws away the body the agent puts its reason in, and on the unix path the crate's JSON helper insists on deserializing the *error* body too, so the same HTML page came back as a JSON parse failure. Both now raise `HTTP : ` — the prpc `error` field when the body is one, otherwise the body itself, bounded to a few hundred characters so an error page cannot become the whole message. The Python and Go SDKs already carried both. Only failures read differently: the wire, the success path and the timeout/abort behaviour are unchanged, and the agent's own explanations still arrive verbatim inside the error — `EmitEvent`'s removal message now as `HTTP 400: EmitEvent was removed in dstack 0.6.0…` +- sdk: the JavaScript and Rust clients report a non-2xx answer with its status *and* the server's message. The JS transport ignored the status entirely, and its unix-socket branch never even parsed the status line, so an agent with no `/v1` mount — every pre-0.6 agent — answered a v1 call with an HTML 404 page that reached the caller as `failed to parse response`, which names neither the status nor the cause. The Rust clients discarded the other half: `error_for_status()` keeps the status line and throws away the body the agent puts its reason in, and on the unix path the crate's JSON helper insists on deserializing the *error* body too, so the same HTML page came back as a JSON parse failure. Both now raise `HTTP : ` — the prpc `error` field when the body is one, otherwise the body itself, bounded so an error page cannot become the whole message. Python and Go already reported both halves, in their own wording; this aligns JS with Rust rather than all four with each other. + + **This changes two things beyond the error text.** In JS, six v0 methods that never checked the response body — `getKey`, `getTlsKey`, `info`, `version`, `sign`, and `TappdClient.deriveKey` — used to *resolve* on a prpc failure, handing back an object whose every field was `undefined` next to an `error` string. They now reject. `version()`, whose own documentation says it throws against an agent too old to have the RPC, previously resolved there; it now does what it says. In Rust, the unix path stops sending a duplicated `Content-Type` header: the crate's JSON helper appended its own on top of the one the client passed, and dropping to the raw request removed it. + +- sdk: fix two encoding defects in the JavaScript transport, both of which corrupted or stalled real calls. The unix-socket branch declared `Content-Length` as `payload.length` — UTF-16 code units — while writing UTF-8, so a request carrying any non-ASCII field sent more bytes than it declared: the agent parsed truncated JSON and the surplus poisoned the connection. `getKey`'s `domain` is specified to accept any byte string a proto3 `string` can carry, so this was reachable by design rather than by accident. On the read path the same branch compared that byte-counted `Content-Length` against a JS string's `.length`, a condition a multi-byte body can never satisfy, so the client waited out the agent's ten-second keep-alive instead of returning — one accented character in an app-compose comment turned `info()` into a ten-second call and made `isReachable()` report a healthy agent as unreachable. That branch no longer speaks HTTP by hand: it uses node's client over `socketPath`, which frames the request, de-chunks the response, and counts bytes where bytes are meant. Responses on both branches are now assembled as bytes and decoded once, so a UTF-8 sequence split across two TCP reads no longer decodes to replacement characters on each side — which the old code did while resolving successfully, handing back quietly wrong data ### Changed - kms: client certificates are authenticated by the attestation they carry rather than by their issuer. Rocket configures mutual TLS through rustls' `WebPkiClientVerifier`, which pins a CA — but an RA-TLS certificate is self-issued and carries its identity in a TEE quote, so there is nothing to chain to. `GetTempCaCert` bridged the gap by handing every caller a shared CA private key purely so the minted certificate would chain somewhere; the CA established nothing (its key is public by design, and the endpoint is unauthenticated) and the check that has always carried the meaning is the quote verification that runs afterwards. The KMS now hands rustls a verifier that requires an attestation and ignores the issuer. Nothing changes for callers: guests and KMS-to-KMS onboarding still mint their client certificates from the temp CA, and those are now accepted for the attestation they carry. What changes is that the TLS layer went from admitting any certificate signed by a public key to requiring an attested one, and that a self-issued certificate is now accepted — which is what lets callers be migrated off `GetTempCaCert` in a follow-up. `[rpc.tls.mutual]` is no longer the trust anchor and is dropped from `kms.toml` and the KMS config templates; leaving it in an existing deployment's config is inert. The gateway's `[tls.mutual]` is unaffected — it pins the KMS root CA, which is a real trust anchor diff --git a/sdk/go/dstack/client_v1.go b/sdk/go/dstack/client_v1.go index 78a6cf7a4..10412fb0d 100644 --- a/sdk/go/dstack/client_v1.go +++ b/sdk/go/dstack/client_v1.go @@ -222,6 +222,11 @@ func WithCertUsageRaTls(usage bool) IssueCertV1Option { } // WithCertUsageServerAuth sets the server auth key usage. +// +// Defaults to true, matching the v1 default in the Rust, Python and JS SDKs; +// pass false to issue a certificate that cannot be used for server auth. The +// frozen v0 GetTlsKey defaults it to false instead, because that is what the +// released 0.5.x SDK sent. func WithCertUsageServerAuth(usage bool) IssueCertV1Option { return func(o *issueCertV1Options) { o.usageServerAuth = usage diff --git a/sdk/js/src/__tests__/send-rpc-request.test.ts b/sdk/js/src/__tests__/send-rpc-request.test.ts index d4ede7273..513d38a33 100644 --- a/sdk/js/src/__tests__/send-rpc-request.test.ts +++ b/sdk/js/src/__tests__/send-rpc-request.test.ts @@ -3,38 +3,19 @@ // SPDX-License-Identifier: Apache-2.0 import { expect, describe, it, vi, beforeEach, afterEach } from 'vitest' -import { send_rpc_request, parse_status_code, __version__ } from '../send-rpc-request' +import { send_rpc_request, __version__ } from '../send-rpc-request' import http from 'http' import https from 'https' -import net from 'net' // Mock the modules vi.mock('http') vi.mock('https') -vi.mock('net') - -describe('parse_status_code', () => { - it('should read the code out of a status line', () => { - expect(parse_status_code('HTTP/1.1 200 OK')).toBe(200) - expect(parse_status_code('HTTP/1.1 404 Not Found')).toBe(404) - expect(parse_status_code('HTTP/1.0 500 Internal Server Error')).toBe(500) - }) - - it('should treat an unreadable status line as unsuccessful', () => { - // Not 2xx, so an answer nobody can classify is reported rather than - // silently passed off as a success. - expect(parse_status_code('')).toBe(0) - expect(parse_status_code('garbage')).toBe(0) - }) -}) describe('send_rpc_request', () => { let mockHttpRequest: any let mockHttpsRequest: any - let mockNetConnect: any let mockReq: any let mockRes: any - let mockClient: any beforeEach(() => { // Reset all mocks @@ -58,17 +39,6 @@ describe('send_rpc_request', () => { vi.mocked(http).request = mockHttpRequest vi.mocked(https).request = mockHttpsRequest - - // Mock net connection - mockClient = { - write: vi.fn(), - end: vi.fn(), - on: vi.fn(), - destroy: vi.fn(), - } - - mockNetConnect = vi.fn(() => mockClient) - vi.mocked(net).createConnection = mockNetConnect }) afterEach(() => { @@ -90,7 +60,7 @@ describe('send_rpc_request', () => { const dataCallback = mockRes.on.mock.calls.find(call => call[0] === 'data')?.[1] const endCallback = mockRes.on.mock.calls.find(call => call[0] === 'end')?.[1] - if (dataCallback) dataCallback('{"result": "success"}') + if (dataCallback) dataCallback(Buffer.from('{"result": "success"}', 'utf8')) if (endCallback) endCallback() return mockReq @@ -128,7 +98,7 @@ describe('send_rpc_request', () => { const dataCallback = mockRes.on.mock.calls.find(call => call[0] === 'data')?.[1] const endCallback = mockRes.on.mock.calls.find(call => call[0] === 'end')?.[1] - if (dataCallback) dataCallback('{"result": "success"}') + if (dataCallback) dataCallback(Buffer.from('{"result": "success"}', 'utf8')) if (endCallback) endCallback() return mockReq @@ -166,7 +136,7 @@ describe('send_rpc_request', () => { const dataCallback = mockRes.on.mock.calls.find(call => call[0] === 'data')?.[1] const endCallback = mockRes.on.mock.calls.find(call => call[0] === 'end')?.[1] - if (dataCallback) dataCallback('{"error": "Service not found: GetKeyX"}') + if (dataCallback) dataCallback(Buffer.from('{"error": "Service not found: GetKeyX"}', 'utf8')) if (endCallback) endCallback() return mockReq @@ -187,7 +157,7 @@ describe('send_rpc_request', () => { const dataCallback = mockRes.on.mock.calls.find(call => call[0] === 'data')?.[1] const endCallback = mockRes.on.mock.calls.find(call => call[0] === 'end')?.[1] - if (dataCallback) dataCallback(page) + if (dataCallback) dataCallback(Buffer.from(page, 'utf8')) if (endCallback) endCallback() return mockReq @@ -212,7 +182,7 @@ describe('send_rpc_request', () => { const dataCallback = mockRes.on.mock.calls.find(call => call[0] === 'data')?.[1] const endCallback = mockRes.on.mock.calls.find(call => call[0] === 'end')?.[1] - if (dataCallback) dataCallback('invalid json') + if (dataCallback) dataCallback(Buffer.from('invalid json', 'utf8')) if (endCallback) endCallback() return mockReq @@ -222,108 +192,6 @@ describe('send_rpc_request', () => { }) }) - describe('Unix socket requests', () => { - it('should call createConnection with correct parameters', () => { - const endpoint = '/tmp/socket' - const path = '/api/test' - const payload = '{"test": "data"}' - - // Mock the socket connection to never complete - mockNetConnect.mockImplementation((options, callback) => { - return mockClient - }) - - // Start the request but don't wait for completion - send_rpc_request(endpoint, path, payload) - - expect(mockNetConnect).toHaveBeenCalledWith({ path: endpoint }, expect.any(Function)) - }) - - // The unix branch speaks HTTP by hand, so it is the only one that has to - // read the status off the wire itself -- and until it did, a 404 reached the - // caller as "failed to parse response". - it('should report a non-2xx read off the status line', async () => { - const body = '{"error": "Service not found: GetKeyX"}' - mockNetConnect.mockImplementation(() => { - setTimeout(() => { - const dataCallback = mockClient.on.mock.calls.find(call => call[0] === 'data')?.[1] - const endCallback = mockClient.on.mock.calls.find(call => call[0] === 'end')?.[1] - dataCallback( - `HTTP/1.1 404 Not Found\r\nContent-Type: application/json\r\nContent-Length: ${body.length}\r\n\r\n${body}` - ) - endCallback() - }, 0) - return mockClient - }) - - await expect(send_rpc_request('/tmp/socket', '/GetKeyX', '{}')) - .rejects.toThrow('HTTP 404: Service not found: GetKeyX') - }) - - it('should still resolve a 200 read off the status line', async () => { - const body = '{"version":"0.6.0"}' - mockNetConnect.mockImplementation(() => { - setTimeout(() => { - const dataCallback = mockClient.on.mock.calls.find(call => call[0] === 'data')?.[1] - const endCallback = mockClient.on.mock.calls.find(call => call[0] === 'end')?.[1] - dataCallback(`HTTP/1.1 200 OK\r\nContent-Length: ${body.length}\r\n\r\n${body}`) - endCallback() - }, 0) - return mockClient - }) - - await expect(send_rpc_request('/tmp/socket', '/Version', '{}')).resolves.toEqual({ - version: '0.6.0', - }) - }) - - // `payload.length` counts UTF-16 code units and the socket emits UTF-8, so a - // request with any non-ASCII field declared a body shorter than the one it - // sent: the agent parsed truncated JSON and the surplus bytes stayed in the - // stream. Both halves of the framing have to count bytes. - it('should declare the payload length in bytes, not code units', async () => { - const payload = JSON.stringify({ domain: 'café', algorithm: 'secp256k1' }) - expect(Buffer.byteLength(payload)).toBeGreaterThan(payload.length) - - const body = '{"ok":true}' - mockNetConnect.mockImplementation((options, callback) => { - // Deferred, as a real socket defers it: the connect handler writes - // through the `client` binding that createConnection is still returning. - setTimeout(() => { - callback() - const dataCallback = mockClient.on.mock.calls.find(call => call[0] === 'data')?.[1] - const endCallback = mockClient.on.mock.calls.find(call => call[0] === 'end')?.[1] - dataCallback(`HTTP/1.1 200 OK\r\nContent-Length: ${body.length}\r\n\r\n${body}`) - endCallback() - }, 0) - return mockClient - }) - - await send_rpc_request('/tmp/socket', '/v1/GetKey', payload) - - const written = mockClient.write.mock.calls.map((call: any[]) => call[0]) - expect(written).toContain(`Content-Length: ${Buffer.byteLength(payload)}\r\n`) - expect(written).not.toContain(`Content-Length: ${payload.length}\r\n`) - }) - - it('should handle Unix socket connection errors', async () => { - const endpoint = '/tmp/socket' - const path = '/api/test' - const payload = '{"test": "data"}' - - mockNetConnect.mockImplementation(() => { - mockClient.on.mockImplementation((event, callback) => { - if (event === 'error') { - setTimeout(() => callback(new Error('socket connection failed')), 0) - } - }) - return mockClient - }) - - await expect(send_rpc_request(endpoint, path, payload)).rejects.toThrow('socket connection failed') - }) - }) - describe('timeout functionality', () => { it('should use default timeout of 30 seconds', async () => { const endpoint = 'http://localhost:3000' @@ -406,7 +274,7 @@ describe('send_rpc_request', () => { const dataCallback = mockRes.on.mock.calls.find(call => call[0] === 'data')?.[1] const endCallback = mockRes.on.mock.calls.find(call => call[0] === 'end')?.[1] - if (dataCallback) dataCallback('{"result": "success"}') + if (dataCallback) dataCallback(Buffer.from('{"result": "success"}', 'utf8')) if (endCallback) endCallback() return mockReq @@ -431,7 +299,7 @@ describe('send_rpc_request', () => { const endCallback = mockRes.on.mock.calls.find(call => call[0] === 'end')?.[1] setTimeout(() => { - if (dataCallback) dataCallback('{"result": "success"}') + if (dataCallback) dataCallback(Buffer.from('{"result": "success"}', 'utf8')) if (endCallback) { endCallback() // First end endCallback() // Second end - should be ignored @@ -446,3 +314,4 @@ describe('send_rpc_request', () => { }) }) }) + diff --git a/sdk/js/src/__tests__/send-rpc-request.unix.test.ts b/sdk/js/src/__tests__/send-rpc-request.unix.test.ts new file mode 100644 index 000000000..b8107def5 --- /dev/null +++ b/sdk/js/src/__tests__/send-rpc-request.unix.test.ts @@ -0,0 +1,131 @@ +// SPDX-FileCopyrightText: © 2025 Phala Network +// +// SPDX-License-Identifier: Apache-2.0 + +// The unix-socket transport, against real sockets rather than mocks. +// +// Its sibling file mocks `http`/`https` to assert how the request is built. +// This one cannot: the defects it pins are in how a *response* is read, and a +// mock that hands the client a whole response as one string is exactly the +// fixture that hid them. Both bugs below were invisible to an ASCII fixture and +// both were live in a released transport. + +import { expect, describe, it, afterEach } from 'vitest' +import { send_rpc_request } from '../send-rpc-request' +import net from 'net' +import fs from 'fs' +import os from 'os' +import path from 'path' + +describe('unix socket transport', () => { + const servers: net.Server[] = [] + const socketPaths: string[] = [] + + afterEach(async () => { + await Promise.all( + servers.splice(0).map(server => new Promise(resolve => server.close(() => resolve()))), + ) + socketPaths.splice(0).forEach(socketPath => { + try { + fs.unlinkSync(socketPath) + } catch { + // Already gone with the server. + } + }) + }) + + /** + * A server that answers every request with `status` and `body`. + * + * It keeps the connection open afterwards, which is what a real agent does -- + * dstack ships `keep_alive = 10` in every Rocket config. Closing here would + * paper over a client that never decides the body is complete, since the + * close would end the response for it. + * + * `writes` splits the body into chosen chunks, to place a TCP boundary where + * a test needs one. + */ + async function serve( + status: string, + body: string, + writes: Buffer[] | null = null, + ): Promise { + const socketPath = path.join( + os.tmpdir(), + `dstack-rpc-${process.pid}-${socketPaths.length}-${Math.random().toString(36).slice(2)}.sock`, + ) + socketPaths.push(socketPath) + + const server = net.createServer(connection => { + connection.once('data', () => { + const payload = Buffer.from(body, 'utf8') + connection.write( + `HTTP/1.1 ${status}\r\nContent-Type: application/json\r\nContent-Length: ${payload.length}\r\n\r\n`, + ) + for (const chunk of writes ?? [payload]) { + connection.write(chunk) + } + }) + }) + servers.push(server) + await new Promise(resolve => server.listen(socketPath, () => resolve())) + return socketPath + } + + /** + * `Content-Length` is a byte count; a JS string's `.length` is UTF-16 code + * units. The transport this replaced compared the two, so for any body with a + * non-ASCII character the units stayed below the bytes forever and the client + * never decided the response was over -- it waited out the agent's keep-alive + * instead. One accented character in an app-compose comment was enough to + * turn `info()` into a ten-second call and `isReachable()` into `false` for a + * healthy agent. + */ + it('returns a multi-byte body promptly rather than waiting out keep-alive', async () => { + const socketPath = await serve('200 OK', JSON.stringify({ note: 'café ☕ 日本語' })) + + const started = Date.now() + const result = await send_rpc_request<{ note: string }>(socketPath, '/Info', '{}', 5000) + + expect(result.note).toBe('café ☕ 日本語') + expect(Date.now() - started).toBeLessThan(1000) + }) + + /** + * Appending a Buffer to a string decodes that chunk alone, so a UTF-8 + * sequence split across two TCP reads decodes to a replacement character on + * each side -- and the call still resolves, handing back corrupted data + * rather than failing. `info()`'s app-compose payload routinely spans reads. + */ + it('does not corrupt a UTF-8 sequence split across two reads', async () => { + const body = JSON.stringify({ note: 'aaa日bbb' }) + const payload = Buffer.from(body, 'utf8') + const cut = payload.indexOf(Buffer.from('日', 'utf8')) + 1 + const socketPath = await serve('200 OK', body, [ + payload.subarray(0, cut), + payload.subarray(cut), + ]) + + const result = await send_rpc_request<{ note: string }>(socketPath, '/Info', '{}', 5000) + + expect(result.note).toBe('aaa日bbb') + expect(result.note).not.toContain('�') + }) + + it('reports a non-2xx with its status and the prpc error text', async () => { + const socketPath = await serve( + '404 Not Found', + JSON.stringify({ error: 'Service not found: GetKeyX' }), + ) + + await expect(send_rpc_request(socketPath, '/GetKeyX', '{}', 5000)).rejects.toThrow( + 'HTTP 404: Service not found: GetKeyX', + ) + }) + + it('rejects when the socket does not exist', async () => { + const missing = path.join(os.tmpdir(), 'dstack-rpc-does-not-exist.sock') + + await expect(send_rpc_request(missing, '/Version', '{}', 5000)).rejects.toThrow() + }) +}) diff --git a/sdk/js/src/send-rpc-request.ts b/sdk/js/src/send-rpc-request.ts index b92716c35..1244db47d 100644 --- a/sdk/js/src/send-rpc-request.ts +++ b/sdk/js/src/send-rpc-request.ts @@ -4,7 +4,6 @@ import http from 'http' import https from 'https' -import net from 'net' export const __version__ = "0.6.0" @@ -56,18 +55,6 @@ function isSuccess(statusCode: number): boolean { return statusCode >= 200 && statusCode < 300 } -/** - * Read the status code out of an HTTP status line (`HTTP/1.1 404 Not Found`). - * - * Exported for the unix-socket branch's tests: that branch speaks HTTP by hand, - * so nothing else parses this for it. An unreadable line yields 0, which is not - * a success code and so gets reported rather than passed off as one. - */ -export function parse_status_code(statusLine: string): number { - const code = Number.parseInt(statusLine.split(' ')[1], 10) - return Number.isNaN(code) ? 0 : code -} - export function send_rpc_request(endpoint: string, path: string, payload: string, timeoutMs?: number): Promise { return new Promise((resolve, reject) => { const abortController = new AbortController() @@ -133,13 +120,17 @@ export function send_rpc_request(endpoint: string, path: string, payloa } const req = (url.protocol === 'https:' ? https : http).request(url, options, (res) => { - let data = '' + // Buffers, concatenated once at the end, rather than `data += chunk`. + // Appending a Buffer to a string decodes that chunk on its own, so a + // UTF-8 sequence split across two TCP reads becomes two replacement + // characters -- and the call still resolves, with corrupted data. + const chunks: Buffer[] = [] res.on('data', (chunk) => { - data += chunk + chunks.push(chunk) }) res.on('end', () => { cleanup() - settle(res.statusCode ?? 0, data) + settle(res.statusCode ?? 0, Buffer.concat(chunks).toString('utf8')) }) }) @@ -155,67 +146,55 @@ export function send_rpc_request(endpoint: string, path: string, payloa req.write(payload) req.end() } else { - const client = net.createConnection({ path: endpoint }, () => { - client.write(`POST ${path} HTTP/1.1\r\n`) - client.write(`Host: localhost\r\n`) - client.write(`Content-Type: application/json\r\n`) - // Byte length, not `payload.length`: JS strings count UTF-16 code units - // and the socket emits UTF-8, so any non-ASCII field -- a `getKey` - // domain, a certificate subject -- declared a body shorter than the one - // sent. The agent then parsed truncated JSON and the surplus bytes were - // left in the stream to corrupt whatever read next. - client.write(`Content-Length: ${Buffer.byteLength(payload)}\r\n`) - client.write('\r\n') - client.write(payload) - }) - - let data = '' - let headers: Record = {} - let headersParsed = false - let statusCode = 0 - let contentLength = 0 - let bodyData = '' - - client.on('data', (chunk) => { - data += chunk - if (!headersParsed) { - const headerEndIndex = data.indexOf('\r\n\r\n') - if (headerEndIndex !== -1) { - const headerLines = data.slice(0, headerEndIndex).split('\r\n') - // The first line is the status line, not a header. - statusCode = parse_status_code(headerLines[0]) - headerLines.slice(1).forEach(line => { - const [key, value] = line.split(': ') - if (key && value) { - headers[key.toLowerCase()] = value - } - }) - headersParsed = true - contentLength = parseInt(headers['content-length'] || '0', 10) - bodyData = data.slice(headerEndIndex + 4) - } - } else { - bodyData += chunk - } - - if (headersParsed && bodyData.length >= contentLength) { - client.end() - } - }) - - client.on('end', () => { - cleanup() - settle(statusCode, bodyData.slice(0, contentLength)) - }) + // `socketPath` rather than a hand-written request over `net`: node's HTTP + // client already frames the request, de-chunks the response, and knows + // that `Content-Length` counts bytes. The version this replaces did none + // of those on the read path -- it compared a byte count from the header + // against a JS string's UTF-16 length, so a response carrying one + // non-ASCII character never satisfied its own end condition and the call + // hung until the agent's keep-alive expired, ten seconds later. An + // app-compose with an accented character in a comment was enough. + const req = http.request( + { + socketPath: endpoint, + path, + method: 'POST', + // One connection per call, closed when the response ends. The code + // this replaces called `client.end()` explicitly; node's default + // agent instead pools the socket, and an open socket keeps the + // process alive -- a script that awaits one `info()` and returns + // would hang until the agent's keep-alive expired. + agent: false, + headers: { + Host: 'localhost', + 'Content-Type': 'application/json', + 'Content-Length': Buffer.byteLength(payload), + 'User-Agent': `dstack-sdk-js/${__version__}`, + }, + }, + (res) => { + const chunks: Buffer[] = [] + res.on('data', (chunk) => { + chunks.push(chunk) + }) + res.on('end', () => { + cleanup() + settle(res.statusCode ?? 0, Buffer.concat(chunks).toString('utf8')) + }) + }, + ) - client.on('error', (error) => { + req.on('error', (error) => { cleanup() safeReject(error) }) abortController.signal.addEventListener('abort', () => { - client.destroy() + req.destroy() }) + + req.write(payload) + req.end() } }) } diff --git a/sdk/rust/src/dstack_client.rs b/sdk/rust/src/dstack_client.rs index eb94f5cf0..dcf11b9c3 100644 --- a/sdk/rust/src/dstack_client.rs +++ b/sdk/rust/src/dstack_client.rs @@ -60,14 +60,18 @@ pub enum ClientKind { pub trait BaseClient {} -/// How much of a server response an error may quote. +/// How much of a server response an error may quote, in characters. /// /// An agent with no route for the path answers with an HTML page, and pasting a /// whole page into an error helps nobody. -const MAX_ERROR_BODY: usize = 512; +/// +/// Characters rather than bytes so the bound cannot land inside a multi-byte +/// sequence; the byte length that follows from it is larger, which is fine for +/// something whose only job is to stop an error message running away. +const MAX_ERROR_BODY_CHARS: usize = 512; fn truncate(text: &str) -> String { - match text.char_indices().nth(MAX_ERROR_BODY) { + match text.char_indices().nth(MAX_ERROR_BODY_CHARS) { Some((end, _)) => format!("{}...", &text[..end]), None => text.to_string(), } @@ -355,11 +359,11 @@ mod tests { #[test] fn bounds_the_quoted_body() { - let err = http_error(500, "x".repeat(MAX_ERROR_BODY * 2).as_bytes()); + let err = http_error(500, "x".repeat(MAX_ERROR_BODY_CHARS * 2).as_bytes()); // An HTML error page must not become the whole error message. assert_eq!( err.to_string().len(), - "HTTP 500: ".len() + MAX_ERROR_BODY + 3 + "HTTP 500: ".len() + MAX_ERROR_BODY_CHARS + 3 ); assert!(err.to_string().ends_with("...")); } From e94e13ae40651eff64e720327854b999315010bd Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 17:31:43 -0700 Subject: [PATCH 3/5] style(sdk-js): restore the single trailing newline end-of-file-fixer expects --- sdk/js/src/__tests__/send-rpc-request.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/sdk/js/src/__tests__/send-rpc-request.test.ts b/sdk/js/src/__tests__/send-rpc-request.test.ts index 513d38a33..15a2811cb 100644 --- a/sdk/js/src/__tests__/send-rpc-request.test.ts +++ b/sdk/js/src/__tests__/send-rpc-request.test.ts @@ -314,4 +314,3 @@ describe('send_rpc_request', () => { }) }) }) - From 2cf1b6158dfd4c75fc63c4f2b04ed4df18fe4f6f Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 19:39:51 -0700 Subject: [PATCH 4/5] fix(sdk): report timeouts as timeouts, and send Content-Type once Two defects the transport rework left in place. The JS timeout aborted the request before rejecting. `abort()` runs its listener synchronously, so `onAbort`'s `request aborted` always won the `isCompleted` race and `request timed out` was unreachable -- which is exactly the message `isReachable()` needs to tell a hung agent apart from any other failure. Rejecting first fixes it; the abort still runs, to destroy the socket. The test that claimed to cover this stubbed `global.setTimeout` so the callback ran before the abort listener was registered, an ordering that cannot happen at runtime, and passed against the broken transport. It is replaced by one against a real socket that accepts and never answers. `http_post` passed `Content-Type` on top of `json()`, and reqwest's `header()` appends rather than replaces, so the header went out twice. Rocket reads the first value and the agent never noticed, but RFC 9110 lets an intermediary refuse a request carrying the field twice. --- CHANGELOG.md | 4 +- sdk/js/src/__tests__/send-rpc-request.test.ts | 32 +++----------- .../__tests__/send-rpc-request.unix.test.ts | 42 +++++++++++++++++++ sdk/js/src/send-rpc-request.ts | 6 ++- sdk/rust/src/dstack_client.rs | 9 ++-- sdk/rust/tests/test_client_v1.rs | 30 ++++++++++--- 6 files changed, 82 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2bd5ceb9e..808b7e61d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,9 +44,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - sdk: the Go and Python compose-hash helpers silently dropped every app-compose field they did not declare, so `getComposeHash` returned a digest for an app-compose that was not the one being deployed — and that digest is what gets whitelisted on chain. The missing fields are named above; both now keep unrecognised keys as well, so a guest that gains a field before the SDK does still hashes correctly - sdk: the JavaScript and Rust clients report a non-2xx answer with its status *and* the server's message. The JS transport ignored the status entirely, and its unix-socket branch never even parsed the status line, so an agent with no `/v1` mount — every pre-0.6 agent — answered a v1 call with an HTML 404 page that reached the caller as `failed to parse response`, which names neither the status nor the cause. The Rust clients discarded the other half: `error_for_status()` keeps the status line and throws away the body the agent puts its reason in, and on the unix path the crate's JSON helper insists on deserializing the *error* body too, so the same HTML page came back as a JSON parse failure. Both now raise `HTTP : ` — the prpc `error` field when the body is one, otherwise the body itself, bounded so an error page cannot become the whole message. Python and Go already reported both halves, in their own wording; this aligns JS with Rust rather than all four with each other. - **This changes two things beyond the error text.** In JS, six v0 methods that never checked the response body — `getKey`, `getTlsKey`, `info`, `version`, `sign`, and `TappdClient.deriveKey` — used to *resolve* on a prpc failure, handing back an object whose every field was `undefined` next to an `error` string. They now reject. `version()`, whose own documentation says it throws against an agent too old to have the RPC, previously resolved there; it now does what it says. In Rust, the unix path stops sending a duplicated `Content-Type` header: the crate's JSON helper appended its own on top of the one the client passed, and dropping to the raw request removed it. + **This changes two things beyond the error text.** In JS, six v0 methods that never checked the response body — `getKey`, `getTlsKey`, `info`, `version`, `sign`, and `TappdClient.deriveKey` — used to *resolve* on a prpc failure, handing back an object whose every field was `undefined` next to an `error` string. They now reject. `version()`, whose own documentation says it throws against an agent too old to have the RPC, previously resolved there; it now does what it says. In Rust, neither transport sends a duplicated `Content-Type` header any more: on the unix path the crate's JSON helper appended its own on top of the one the client passed, and on the HTTP path `reqwest`'s `json()` did the same, since `header()` appends rather than replaces. Rocket reads the first value, so the agent never noticed, but a stricter intermediary is entitled to refuse a request carrying the field twice. -- sdk: fix two encoding defects in the JavaScript transport, both of which corrupted or stalled real calls. The unix-socket branch declared `Content-Length` as `payload.length` — UTF-16 code units — while writing UTF-8, so a request carrying any non-ASCII field sent more bytes than it declared: the agent parsed truncated JSON and the surplus poisoned the connection. `getKey`'s `domain` is specified to accept any byte string a proto3 `string` can carry, so this was reachable by design rather than by accident. On the read path the same branch compared that byte-counted `Content-Length` against a JS string's `.length`, a condition a multi-byte body can never satisfy, so the client waited out the agent's ten-second keep-alive instead of returning — one accented character in an app-compose comment turned `info()` into a ten-second call and made `isReachable()` report a healthy agent as unreachable. That branch no longer speaks HTTP by hand: it uses node's client over `socketPath`, which frames the request, de-chunks the response, and counts bytes where bytes are meant. Responses on both branches are now assembled as bytes and decoded once, so a UTF-8 sequence split across two TCP reads no longer decodes to replacement characters on each side — which the old code did while resolving successfully, handing back quietly wrong data +- sdk: fix two encoding defects in the JavaScript transport, both of which corrupted or stalled real calls. The unix-socket branch declared `Content-Length` as `payload.length` — UTF-16 code units — while writing UTF-8, so a request carrying any non-ASCII field sent more bytes than it declared: the agent parsed truncated JSON and the surplus poisoned the connection. `getKey`'s `domain` is specified to accept any byte string a proto3 `string` can carry, so this was reachable by design rather than by accident. On the read path the same branch compared that byte-counted `Content-Length` against a JS string's `.length`, a condition a multi-byte body can never satisfy, so the client waited out the agent's ten-second keep-alive instead of returning — one accented character in an app-compose comment turned `info()` into a ten-second call and made `isReachable()` report a healthy agent as unreachable. That branch no longer speaks HTTP by hand: it uses node's client over `socketPath`, which frames the request, de-chunks the response, and counts bytes where bytes are meant. Responses on both branches are now assembled as bytes and decoded once, so a UTF-8 sequence split across two TCP reads no longer decodes to replacement characters on each side — which the old code did while resolving successfully, handing back quietly wrong data. A timed-out call also says so: the timeout aborted the request before rejecting, and `abort()` runs its listener synchronously, so the abort's `request aborted` always won the race and `request timed out` was unreachable — leaving `isReachable()` unable to tell a hung agent from any other failure ### Changed - kms: client certificates are authenticated by the attestation they carry rather than by their issuer. Rocket configures mutual TLS through rustls' `WebPkiClientVerifier`, which pins a CA — but an RA-TLS certificate is self-issued and carries its identity in a TEE quote, so there is nothing to chain to. `GetTempCaCert` bridged the gap by handing every caller a shared CA private key purely so the minted certificate would chain somewhere; the CA established nothing (its key is public by design, and the endpoint is unauthenticated) and the check that has always carried the meaning is the quote verification that runs afterwards. The KMS now hands rustls a verifier that requires an attestation and ignores the issuer. Nothing changes for callers: guests and KMS-to-KMS onboarding still mint their client certificates from the temp CA, and those are now accepted for the attestation they carry. What changes is that the TLS layer went from admitting any certificate signed by a public key to requiring an attested one, and that a self-issued certificate is now accepted — which is what lets callers be migrated off `GetTempCaCert` in a follow-up. `[rpc.tls.mutual]` is no longer the trust anchor and is dropped from `kms.toml` and the KMS config templates; leaving it in an existing deployment's config is inert. The gateway's `[tls.mutual]` is unaffected — it pins the KMS root CA, which is a real trust anchor diff --git a/sdk/js/src/__tests__/send-rpc-request.test.ts b/sdk/js/src/__tests__/send-rpc-request.test.ts index 15a2811cb..8eba6444a 100644 --- a/sdk/js/src/__tests__/send-rpc-request.test.ts +++ b/sdk/js/src/__tests__/send-rpc-request.test.ts @@ -234,32 +234,12 @@ describe('send_rpc_request', () => { global.setTimeout = originalSetTimeout }) - it('should timeout and reject with correct error message', async () => { - const endpoint = 'http://localhost:3000' - const path = '/api/test' - const payload = '{"test": "data"}' - - // Mock real setTimeout to trigger timeout immediately - const originalSetTimeout = global.setTimeout - // @ts-ignore - global.setTimeout = vi.fn((callback, delay) => { - if (delay === 1) { - // Call timeout callback immediately for our test timeout - callback() - return 123 as any - } - return originalSetTimeout(callback, delay) - }) - - mockHttpRequest.mockImplementation(() => { - // Never complete the request - return mockReq - }) - - await expect(send_rpc_request(endpoint, path, payload, 1)).rejects.toThrow('request timed out') - - global.setTimeout = originalSetTimeout - }) + // The timeout *message* is pinned in `send-rpc-request.unix.test.ts`, + // against a real socket that accepts and never answers. Stubbing + // `setTimeout` here would run the callback before the abort listener is + // registered -- an ordering that cannot happen at runtime, and one that + // made the previous version of this test pass against a transport where + // `request timed out` was unreachable. }) describe('abort functionality', () => { diff --git a/sdk/js/src/__tests__/send-rpc-request.unix.test.ts b/sdk/js/src/__tests__/send-rpc-request.unix.test.ts index b8107def5..6ce65f0a2 100644 --- a/sdk/js/src/__tests__/send-rpc-request.unix.test.ts +++ b/sdk/js/src/__tests__/send-rpc-request.unix.test.ts @@ -20,8 +20,12 @@ import path from 'path' describe('unix socket transport', () => { const servers: net.Server[] = [] const socketPaths: string[] = [] + const connections: net.Socket[] = [] afterEach(async () => { + // Before closing: `server.close()` waits on live connections, and these + // servers deliberately hold theirs open the way a real agent does. + connections.splice(0).forEach(connection => connection.destroy()) await Promise.all( servers.splice(0).map(server => new Promise(resolve => server.close(() => resolve()))), ) @@ -57,6 +61,10 @@ describe('unix socket transport', () => { socketPaths.push(socketPath) const server = net.createServer(connection => { + connections.push(connection) + connection.on('error', () => { + // A client that gives up resets the connection; nothing to do. + }) connection.once('data', () => { const payload = Buffer.from(body, 'utf8') connection.write( @@ -72,6 +80,26 @@ describe('unix socket transport', () => { return socketPath } + /** A server that accepts the connection and then never answers. */ + async function serveSilent(): Promise { + const socketPath = path.join( + os.tmpdir(), + `dstack-rpc-${process.pid}-${socketPaths.length}-${Math.random().toString(36).slice(2)}.sock`, + ) + socketPaths.push(socketPath) + + const server = net.createServer(connection => { + connections.push(connection) + connection.on('error', () => { + // The client aborts on timeout, which arrives here as a reset. + }) + // Deliberately never answers. + }) + servers.push(server) + await new Promise(resolve => server.listen(socketPath, () => resolve())) + return socketPath + } + /** * `Content-Length` is a byte count; a JS string's `.length` is UTF-16 code * units. The transport this replaced compared the two, so for any body with a @@ -123,6 +151,20 @@ describe('unix socket transport', () => { ) }) + /** + * A timeout has to *say* it timed out. `abort()` fires its listener + * synchronously, so aborting before rejecting let `request aborted` win the + * race and the timeout message was unreachable -- leaving `isReachable()` + * unable to distinguish a hung agent from any other failure. + */ + it('reports a hung agent as a timeout rather than as an abort', async () => { + const socketPath = await serveSilent() + + await expect(send_rpc_request(socketPath, '/Info', '{}', 200)).rejects.toThrow( + 'request timed out', + ) + }) + it('rejects when the socket does not exist', async () => { const missing = path.join(os.tmpdir(), 'dstack-rpc-does-not-exist.sock') diff --git a/sdk/js/src/send-rpc-request.ts b/sdk/js/src/send-rpc-request.ts index 1244db47d..7e6c6707c 100644 --- a/sdk/js/src/send-rpc-request.ts +++ b/sdk/js/src/send-rpc-request.ts @@ -89,9 +89,13 @@ export function send_rpc_request(endpoint: string, path: string, payloa } } + // Reject *before* aborting. `abort()` runs `onAbort` synchronously, and the + // first `safeReject` to run is the message the caller sees -- aborting first + // means every timeout is reported as `request aborted`, which says nothing + // about why the call ended. The abort still happens, to destroy the socket. const timeout = setTimeout(() => { - abortController.abort() safeReject(new Error('request timed out')) + abortController.abort() }, timeoutMs || 30_000) // Default 30 seconds timeout const cleanup = () => { diff --git a/sdk/rust/src/dstack_client.rs b/sdk/rust/src/dstack_client.rs index dcf11b9c3..da72becbf 100644 --- a/sdk/rust/src/dstack_client.rs +++ b/sdk/rust/src/dstack_client.rs @@ -115,12 +115,9 @@ pub(crate) async fn http_post( base_url.trim_end_matches('/'), path.trim_start_matches('/') ); - let res = Client::new() - .post(&url) - .json(payload) - .header("Content-Type", "application/json") - .send() - .await?; + // `json()` sets `Content-Type` itself; adding it again would *append* rather + // than replace, putting the header on the wire twice. + let res = Client::new().post(&url).json(payload).send().await?; // Deliberately not `error_for_status()`: that discards the response body, // which is exactly where the agent puts the reason it refused. let status = res.status(); diff --git a/sdk/rust/tests/test_client_v1.rs b/sdk/rust/tests/test_client_v1.rs index b06adf874..155c503a2 100644 --- a/sdk/rust/tests/test_client_v1.rs +++ b/sdk/rust/tests/test_client_v1.rs @@ -20,7 +20,10 @@ async fn version_answers_on_the_v1_surface() { #[tokio::test] async fn get_key_returns_a_key_public_key_and_two_link_chain() { for algorithm in ["secp256k1", "ed25519"] { - let response = client().get_key("storage-encryption", algorithm).await.unwrap(); + let response = client() + .get_key("storage-encryption", algorithm) + .await + .unwrap(); // 32 raw bytes for both algorithms, hex-encoded on the wire. assert_eq!(response.decode_key().unwrap().len(), 32); @@ -34,14 +37,20 @@ async fn get_key_returns_a_key_public_key_and_two_link_chain() { #[tokio::test] async fn get_key_public_key_lengths_are_the_specified_ones() { - let secp = client().get_key("storage-encryption", "secp256k1").await.unwrap(); + let secp = client() + .get_key("storage-encryption", "secp256k1") + .await + .unwrap(); assert_eq!( secp.decode_public_key().unwrap().len(), 33, "SEC1 compressed" ); - let ed = client().get_key("storage-encryption", "ed25519").await.unwrap(); + let ed = client() + .get_key("storage-encryption", "ed25519") + .await + .unwrap(); assert_eq!(ed.decode_public_key().unwrap().len(), 32); } @@ -52,7 +61,10 @@ async fn get_key_rejects_an_empty_or_unknown_algorithm() { assert!(client().get_key("storage-encryption", "").await.is_err()); for algorithm in ["k256", "rsa", "secp256k1_prehashed"] { assert!( - client().get_key("storage-encryption", algorithm).await.is_err(), + client() + .get_key("storage-encryption", algorithm) + .await + .is_err(), "v1 accepted algorithm {algorithm:?}" ); } @@ -70,8 +82,14 @@ async fn different_domains_yield_different_keys() { /// KDF exists. #[tokio::test] async fn the_two_algorithms_never_share_key_material() { - let secp = client().get_key("storage-encryption", "secp256k1").await.unwrap(); - let ed = client().get_key("storage-encryption", "ed25519").await.unwrap(); + let secp = client() + .get_key("storage-encryption", "secp256k1") + .await + .unwrap(); + let ed = client() + .get_key("storage-encryption", "ed25519") + .await + .unwrap(); assert_ne!(secp.key, ed.key); } From c3b5f9614f037a22985a19ccfcfd575b00b4ab1d Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 19:55:35 -0700 Subject: [PATCH 5/5] test(sdk): pin the 501 an image without nvattest answers `AttestGpu` on an image that ships no nvattest answers 501, not 400 -- the request is well-formed and no retry of it will ever succeed, so a client that reads 4xx retries forever while one that reads 501 falls back. The agent started saying so in #1118; these tests still asserted the old 400, and the JS one failed against a `next` that had moved. Widened rather than relaxed. The JS assertion now names 501 and the agent's own words instead of matching any 4xx, and the Rust and Python tests -- which only checked that *something* failed -- pin the status too. That is this branch's own claim under test: every SDK reports a non-2xx response with both the server's error text and the HTTP status, and until now nothing proved it for the one status that tells a caller to stop trying. --- sdk/js/src/__tests__/index-v1.test.ts | 6 +++++- sdk/python/tests/test_client_v1.py | 11 +++++++++-- sdk/rust/tests/test_client_v1.rs | 11 +++++++++-- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/sdk/js/src/__tests__/index-v1.test.ts b/sdk/js/src/__tests__/index-v1.test.ts index a7a0abd28..030a1fa0c 100644 --- a/sdk/js/src/__tests__/index-v1.test.ts +++ b/sdk/js/src/__tests__/index-v1.test.ts @@ -202,8 +202,12 @@ describe('DstackClientV1', () => { // The simulator ships no nvattest, so this must fail fast and clearly // rather than hang for the attestation timeout -- with the status the // agent answered and its own explanation, not a parse error. + // + // 501 specifically, not any 4xx: the agent distinguishes "this image can + // never attest a GPU" from "your request was wrong", and a client that + // cannot see the difference retries a call that will never succeed. await expect(() => client.attestGpu(new Uint8Array(32).fill(0xab))).rejects.toThrow( - /^HTTP 4\d\d: .*GPU attestation/ + /^HTTP 501: .*GPU attestation is not available/ ) }) }) diff --git a/sdk/python/tests/test_client_v1.py b/sdk/python/tests/test_client_v1.py index cc0d051b0..ab405bd33 100644 --- a/sdk/python/tests/test_client_v1.py +++ b/sdk/python/tests/test_client_v1.py @@ -261,10 +261,17 @@ async def test_async_v1_attest_gpu_rejects_wrong_nonce_length(): def test_sync_v1_attest_gpu_reaches_the_agent(): - """No GPU in the simulator, so the agent's own refusal is the success signal.""" + """No GPU in the simulator, so the agent's own refusal is the success signal. + + 501 rather than 4xx, and the client must pass both the status and the + agent's own words through: a caller that sees 4xx retries a call this image + can never answer. + """ with pytest.raises(Exception) as excinfo: DstackClientV1().attest_gpu(NONCE) - assert "GPU attestation" in str(excinfo.value) + message = str(excinfo.value) + assert "501" in message, message + assert "GPU attestation is not available" in message, message def test_sync_v1_issue_cert(): diff --git a/sdk/rust/tests/test_client_v1.rs b/sdk/rust/tests/test_client_v1.rs index 155c503a2..bbddd60a9 100644 --- a/sdk/rust/tests/test_client_v1.rs +++ b/sdk/rust/tests/test_client_v1.rs @@ -159,8 +159,15 @@ async fn attest_gpu_validates_the_nonce_length() { ); } // A correctly sized nonce gets past the client and fails at the simulator, - // which has no GPU to attest. - assert!(client().attest_gpu(vec![0xab; 32]).await.is_err()); + // which has no GPU to attest. 501, not 4xx: the request was well-formed and + // no retry of it will ever succeed on an image that ships no nvattest. + let err = client() + .attest_gpu(vec![0xab; 32]) + .await + .expect_err("the simulator has no GPU to attest"); + let err = format!("{err:#}"); + assert!(err.contains("HTTP 501"), "{err}"); + assert!(err.contains("GPU attestation is not available"), "{err}"); } #[tokio::test]