Skip to content

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

Description

@kvinwang

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    P0Highest priority: review or decide before anything else in the audit batch

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions