contracts: the documented app upgrade is add-only, so the withdrawn compose hash stays bootable with identical keys

Open
#1,297 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
5/5
Estimated time
Over a week
Newbie friendliness
25/100
Issue type
Feature
Clarity
Needs clarification
Activity status
Active
Tech stack
rust, solidity

Research direction

Start with contracts/DstackApp.sol, the upgrade tutorials in docs/tutorials/hello-world-app.md and docs/tutorials/gateway-build-configuration.md, and script/Manage.s.sol. Read .agent/CONTRACT-SCENARIOS.md and run the scenarios in dstack/kms/auth-eth/test/ScenarioWalk.t.sol. Done requires choosing a direction among documentation, tooling, or contract changes and defining behavior for version retirement and atomic revocation.

Written by the indexing model from the issue text.

Description

Label: DESIGN. No test fails against the current contracts; the tests below pass and pin present behaviour.

What the design currently is

DstackApp.allowedComposeHashes is a flat mapping(bytes32 => bool) (contracts/DstackApp.sol:25). There is no version counter, no ordering, and no supersession relation between two whitelisted hashes.

The upgrade call sequence every tutorial in the tree shows is a single call — docs/tutorials/hello-world-app.md:240 and docs/tutorials/gateway-build-configuration.md:513:

appOwner : app.addComposeHash(V2)

Resulting state: allowedComposeHashes = {V1: true, V2: true}. The withdrawn version is still bootable. Rollback is prevented by exactly one thing — the operator remembering a second transaction, removeComposeHash(V1), which no tutorial shows.

[PASS] test_S2_UpgradeLeavesTheOldVersionBootable() (gas: 319719)
       assertTrue(okOld, "WITHDRAWN version still boots -- no rollback protection")

[PASS] test_S2_RollbackIsOneOwnerTransactionAway() (gas: 330804)

This matters more than a stale-allowlist entry usually would, because compose_hash is not a key-derivation input. The only KDF contexts in the KMS are app_id and instance_id:

$ grep -n "context_data\|derive_dh_secret\|derive_k256_key" dstack/kms/src/main_service.rs
335:  let context_data = vec![app_id, b"app-ca"];
379:  let context_data = vec![&app_id[..], &instance_id[..], b"app-disk-crypt-key"];
384:  kdf::derive_dh_secret(&self.state.root_ca.key, &[&app_id[..], b"env-encrypt-key"])
391:  derive_k256_key(&self.state.k256_key, &app_id)

So a rolled-back CVM does not get a key — it gets the same disk key, env-decryption key, app CA and k256 wallet as the version that replaced it.

[PASS] test_S2_BothVersionsShareOneAppIdentity() (gas: 319481)

The second half of the same gap is that revocation has no batch and no pause. It is one owner transaction per whitelisted hash, each independently mineable, so a partially-applied revocation leaves the app authorized:

[PASS] test_S3_RevocationIsOneTransactionPerVersion_NoAtomicKillSwitch() (gas: 369465)
       assertFalse(hit, "no removeComposeHashes(bytes32[])");
       assertFalse(hit, "no pause()");
       assertFalse(hit, "no multicall(bytes[])");
       assertTrue(ok, "partially-applied revocation leaves the app authorized");

script/Manage.s.sol has a BatchKmsSetup for adding and no batch for removing.

Steelman

An additive allowlist is the right primitive for a system that runs many instances of an app concurrently. A real fleet rolls forward gradually: during a deployment, instances on both v1 and v2 need to boot, and canary and blue/green topologies keep two versions live indefinitely and on purpose. A contract that auto-retired the previous hash would break every one of those, and a "current version" pointer would force a design decision about how many versions may be live at once that operators should be making, not the contract.

mapping(bytes32 => bool) is also the cheapest possible representation, and the per-transaction model keeps each policy change individually auditable in PolicyChanged — a batch call collapses N decisions into one log entry unless the batch emits per-item events anyway.

Not making compose_hash a KDF input is likewise deliberate: an app that rotated its disk key on every compose change could not read its own persistent storage after an upgrade.

What it costs

The scenario it breaks is "upgrade", performed exactly as documented. The operator's intent is "v2 replaces v1"; the state they get is "v1 and v2 are both authorized"; and the difference is invisible unless they go looking. An attacker who can influence which image a CVM boots — or simply an operator re-running an old deployment manifest — gets a CVM on the withdrawn code with the production key material, not a fresh identity.

docs/specification.md §8 asks this question for removeOsImageHash (§8.3, "does not retroactively un-authorize already-running apps… is that intended?") and does not ask it for removeComposeHash.

Reachability: who — the app owner, by omission; credential — none for the gap itself (it is the absence of a second call), the app owner key to exploit deliberately; frequency — every upgrade.

Improvement direction

Redeployment status: the highest-value fix is documentation and needs no contract change at all. The contract options are DstackApp implementation upgrades behind existing proxies; one needs new storage past __gap, the others do not. Note that any DstackApp implementation fix reaches an app only when that app's owner upgrades, and never for an app that has called disableUpgrades() — which is why the docs option is not a consolation prize here.

Options:

  1. Docs + tooling, no contract change. Make every upgrade tutorial show addComposeHash(V2) and removeComposeHash(V1) as the two halves of one operation, and say explicitly that all whitelisted versions share one key hierarchy so a rollback is not a fresh identity. If vmm-cli or the deploy scripts ever drive an upgrade, have them prompt for or perform the retirement. Reaches every deployed app immediately.
  2. replaceComposeHash(bytes32 old, bytes32 new) (impl upgrade, no new storage). One transaction, atomic, emits both PolicyChanged entries. Makes the intended operation expressible without forcing it — addComposeHash stays for the deliberate multi-version case. Cheapest contract-side option and probably the best value.
  3. removeComposeHashes(bytes32[]) / addComposeHashes(bytes32[]) (impl upgrade, no new storage). Closes the O(n)-revocation half. Emit one PolicyChanged per item so the audit story is unchanged. Bounded-loop gas is the only real objection.
  4. An app-level pause() (impl upgrade, one new bool — packs into the existing slot 1 with _upgradesDisabled/allowAnyDevice, so still no __gap consumption). Gives an atomic kill switch independent of how many hashes are whitelisted. Needs a decision about who may un-pause.
  5. A monotonic version counter or supersession pointer (impl upgrade, new storage past __gap). Would let the contract express "v1 is retired" as data rather than absence, and could support a minimum-version floor. Highest review cost and it forecloses the concurrent-version topologies above unless the floor is opt-in. Listed for completeness; probably not worth it.

(1) plus (2) and (3) covers the scenario without changing what the contract permits.

Found during a scenario-driven review of the authorization contracts; full walk in .agent/CONTRACT-SCENARIOS.md (scenarios 2 and 3), tests in dstack/kms/auth-eth/test/ScenarioWalk.t.sol.

Dominant language
Rust
Stars
546
Forks
96
Avg merge
19h 22m
Merged PRs (30d)
109

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from Dstack-TEE/dstack

All issues in Dstack-TEE/dstack

Similar issues

More Rust issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.