refactor!: move signature verification out of the CVM and into the SDKs - #1110
Conversation
|
Both points are fair. Pushed c8548a9 for the first. 1. READMEs demonstrating the anti-pattern — fixed. You are right that this was the worst place for the inconsistency to land. The prose said "must be the app id you expect, not whatever All four examples now make both anchors literals the caller owns: // Both anchors come from you, not from the CVM being checked.
appID, _ := hex.DecodeString("a9019d1b...")
kmsRoot, _ := hex.DecodeString("03...") // pinned, or read from the DstackKms contractand each gained a short paragraph naming what the example deliberately does not do, since a comment inside a snippet travels with a copy-paste but a caveat three paragraphs up does not:
2. recid 2/3 — no code change, but recorded. Agreed it is unreachable: it needs an ECDSA nonce whose I considered narrowing Rust/Go/JS to |
c8548a9 to
ca5711d
Compare
|
Rebased onto Conflicts: two, both from the same root cause. #1107's
The failure mode worth guarding against here is silently reverting the rename while resolving. It didn't happen: Re-verified against a simulator rebuilt from the rebased tree —
Live endpoint checks on the rebased agent: That third line is the one I actually cared about. It confirms #1107's rework still works through my resolution rather than having been quietly clobbered — a green build alone would not have told us that, since the old name is gone from the proto either way. |
Checking a signature needs no key material and no attestation. The agent's verdict also came back over the socket unattested, so a caller who believed the TEE was vouching for it was mistaken, and one who did not gained nothing over checking the signature locally. It was inbound attack surface for that non-benefit: attacker-supplied keys and signatures parsed inside the TEE on every call. Sign stays server-side, because it needs a key only the TEE holds. The RPC shipped in v0.5.6..v0.5.9, so this is a breaking change -- an SDK pinned at 0.5.x calling /Verify against a 0.6+ agent gets an unknown-method error. The SDKs verify locally instead; see the following commits. The method name is left documented in the proto so it is not reused for something else later.
Four independent ports of one byte-exact format is how this repository has shipped cross-language crypto drift before: the Go compose-hash helper HTML-escaped '<', so it hashed an app-compose differently from every other SDK, and that digest is what gets whitelisted on chain. So the format lives in one committed file, generated from the same primitives KMS and the guest agent actually use -- the RATLS HKDF, the dstack-kms-issued keccak preimage, RFC-6979 deterministic ECDSA -- and every SDK asserts against it. The generator re-derives and diffs on each run, so an intentional format change fails here first and the four SDK suites fail right after. The negative cases matter as much as the positive ones. secp256k1_high_s pins that the malleated (r, n-s) form of a valid signature must be rejected: k256 refuses it, but Python's cryptography and Go's decred secp256k1 accept it unless asked not to. wrong_kms_root_pubkey pins that a self-consistent chain anchored at a foreign root is refused, which is the check the whole chain rests on.
Replaces the client's verify() RPC wrapper with a standalone verify_signature.
A pure function of four byte strings had no business hanging off an object that
holds a socket connection.
Adds verify_signature_chain, which is new capability rather than a port. The
old RPC checked one signature against a public key the caller passed in, which
proves only that whoever holds that key signed the data -- it says nothing about
whose key it is. The chain walks all three links back to a KMS root key the
caller supplies: the payload signature, the app root attesting
"{purpose}:{hex(pubkey)}", and the KMS root attesting that app root for this
app_id. That last comparison is the entire point; without a root the caller
independently trusts, a chain is three signatures an attacker could have minted
themselves. The docs say so, and say the same about app_id, since AppInfo comes
from the CVM being checked.
High-S signatures are rejected explicitly rather than left to the backend's
default, so this cannot drift from the other three SDKs.
Mirrors the Rust SDK: verify_signature replaces the removed verify() RPC wrapper, and verify_signature_chain walks the chain back to a caller-supplied KMS root key. Both drive the shared vectors. No new dependencies -- cryptography covers Ed25519 and SECP256K1 (raw r||s converted to DER via encode_dss_signature), eth-keys covers the recoverable signatures in links two and three. cryptography does not enforce low-S, so the malleated (r, n-s) form of a valid signature is refused by an explicit check here rather than by the library. Without it this SDK would have accepted signatures the guest agent rejected.
Mirrors the Rust SDK: VerifySignature replaces the removed Verify() RPC wrapper, and VerifySignatureChain walks the chain back to a caller-supplied KMS root key. Both drive the shared vectors. No new module dependencies -- crypto/ed25519 from the standard library, and the decred secp256k1 package already vendored for the env-encrypt-pubkey verifier, whose keccak256 and recovery helpers are reused rather than duplicated. decred's ECDSA does not enforce low-S, so high-S is refused explicitly via ModNScalar.IsOverHalfOrder, which is exactly k256's predicate. One deliberate divergence is documented in the source: crypto/ed25519 offers no way to ask whether a public key is a canonical point, so a malformed ed25519 key is a false verdict here where Rust raises. Both refuse the signature.
Mirrors the Rust SDK: verifySignature replaces the removed verify() RPC wrapper, and verifySignatureChain walks the chain back to a caller-supplied KMS root key. Both drive the shared vectors. @noble/curves and @noble/hashes move from optional peer dependencies to real dependencies. verify.ts is reachable from the main entry point and needs them unconditionally, and node:crypto is not an option: it cannot verify a pre-hashed ECDSA digest, which secp256k1_prehashed requires. bun.lock is realigned with the new categorization. noble already defaults to lowS: true, which happens to match k256, but the flag is passed explicitly and high-S is detected up front with hasHighS() so a future change to that default cannot silently start accepting malleated signatures the other three SDKs reject.
The endpoint reference gains a note on why /Verify is gone rather than a silent gap, and the sections after it are renumbered. Drops the "not yet released" annotation from Sign and Verify in the curl and Rust references. It was stale: both shipped in v0.5.6. Leaving it would have made this removal look free when it is a breaking change. Also fixes the Sign response example, which was missing a comma and so was not valid JSON.
…i-pattern
Every README told the reader that app_id must be the value they expect rather
than whatever AppInfo reported, then showed an example passing client.info()'s
app_id straight into the verifier. The Go one put "pinned, or read from the
DstackKms contract" on the KMS root and left the app id unguarded on the line
above it.
Examples get copied verbatim into production far more often than the prose
above them gets read, so both anchors are now literals the caller owns, and each
example says explicitly what it is not doing and why feeding AppInfo back in
proves only that the CVM agrees with itself.
Also records, in the vector generator, that recovery ids 2 and 3 are the one
behaviour the shared vectors cannot pin: Rust, Go and JavaScript accept 0..3
while Python rejects 2 and 3 because eth_keys only models v in {0, 1}. Reaching
either needs an r that wrapped the curve order, so the divergence is
unreachable rather than latent -- but it should not live only in a review
thread.
ca5711d to
0e25636
Compare
What
Removes the guest-agent
VerifyRPC and moves signature verification into the four SDKs, where it also gains the check it was always missing: walking thesignature_chainback to a KMS root key the caller independently trusts.Why
Verifyshould never have been an RPCChecking a signature needs no key material and no attestation. The agent's verdict also came back over the socket unattested — so a caller who believed the TEE was vouching for it was mistaken, and a caller who did not gained nothing over checking the signature themselves. Meanwhile it parsed attacker-supplied keys and signatures inside the TEE on every call: inbound attack surface for a non-benefit.
Signstays server-side. It needs a key only the TEE holds.The RPC only existed because review #360 asked for a
Sign()counterpart. That parity requirement is real, but it is satisfied by an SDK-local function — it never needed to be an endpoint.Verifyshipped in v0.5.6 through v0.5.9. An SDK pinned at 0.5.x calling/Verifyagainst a 0.6+ guest agent now gets an unknown-method error.Note that
sdk/curl/api.mdandsdk/rust/README.mdboth labelled Sign/Verify "not yet released". That annotation was stale — this PR removes it. It would have made this look like a free cleanup when it is not.The client-side
verify()method is also gone from all four SDKs, replaced by a standalone function. A pure function of four byte strings had no business hanging off an object that holds a socket connection. Happy to add thin deprecated delegates back if you would rather stage that separately.New capability:
verify_signature_chainThe old RPC checked one signature against a public key the caller passed in — that proves only that whoever holds that key signed the data, not whose key it is. The chain verifier walks all three links:
[0]payloadr‖s[1]app root"{purpose}:{lowerhex(pubkey)}"r‖s‖recid[2]KMS root"dstack-kms-issued:" ‖ app_id(20B) ‖ app_root_pk(33B)r‖s‖recidLink 3 is the one that matters, and it is why the KMS root key is a caller-supplied parameter rather than something the SDK fetches. Read it from the KMS being verified and an attacker who can answer that query can also mint a self-consistent chain. The docs point at
DstackKms.kmsInfo().k256Pubkeyor a pinned value, and say the same aboutapp_id, sinceAppInfocomes from the CVM under test.This is deliberately not wired into the simulator-backed test suites: the only way to obtain a KMS root key from the simulator is to recover it from the chain being checked, which is exactly the self-anchoring anti-pattern the API warns against. Chain coverage lives in the vector suite.
The bug this would have shipped without shared vectors
k256rejects non-canonical high-S ECDSA signatures. Python'scryptographyand Go'sdecred/secp256k1accept them.A port done by eye would have silently reintroduced signature malleability in two of four languages — both
(r, s)and(r, n−s)validating the same message, so a signature stops being a unique identifier. It is now an explicit check in all four, pinned by a dedicated negative vector.This is not hypothetical for this repo: the Go compose-hash helper HTML-escaped
<and hashed app-composes differently from every other SDK, and that digest is what gets whitelisted on chain. Sosdk/tests/vectors/signature_chain.jsonis generated bydstack/guest-agent/tests/signature_chain_vectors.rsfrom the real KMS/guest-agent primitives, and all four SDKs assert against that one file. The generator re-derives and diffs on every run, so a format change fails there first.Dependencies
cryptography+eth-keysalready presentcrypto/ed25519+ already-vendored decred secp256k1, reusing the existing keccak/recovery helpersk256,ed25519-dalek(k256 already transitive viaalloy)@noble/curves/@noble/hashespromoted optional-peer → real deps.node:cryptocannot verify a pre-hashed ECDSA digest, whichsecp256k1_prehashedrequires.bun.lockrealignedVerification
sdk/run-tests.shend to end against a freshly built simulator —EXIT=0:dstack+tappd, including the three rewrittenTestSignAndVerify*ruff+mypycleanAgainst the rebuilt agent:
POST /Signstill returns a 3-element chain;POST /Verify→HTTP 404 {"error": "Service not found: Verify"}.prek run --all-filesandreuse lintboth pass. A proto/SDK sync audit found no leftover references and confirmed the other eightDstackGuestRPCs are still reachable in all four SDKs.Note for reviewers
sdk/js/bun.lockwas hand-edited — only the workspace dependency categorization changed, resolved package entries are untouched. bun was not available in this environment, so abun installto confirm would be welcome.