auth-simple: duplicate app keys differing in case silently shadow the tighter entry, and a backend migration moves policy authority with no observable signal

オープン
#1,301 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る

まだ誰も着手していません。

評価

難易度
5/5
見積もり時間
1週間以上
初心者へのやさしさ
30/100
issue の種類
機能追加
明瞭さ
おおむね明確
活発さ
活発
技術スタック
rust, typescript

調査の方向性

Start with dstack/kms/auth-simple/index.ts, docs/auth-simple-operations.md, dstack/kms/auth-simple/migration.test.ts, and kms/src/main_service.rs, then read scenario 7 in .agent/CONTRACT-SCENARIOS.md. Done requires an agreed scope for duplicate-key handling and backend-posture reporting or documentation, with the relevant tests and operational guidance updated.

索引モデルが issue の本文から書いたものです。

説明

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

Two related gaps found while walking the "migrate one app between authorization backends" scenario. Both are off-chain and cheap to fix; neither needs any contract change.


1. Duplicate app keys differing only in case — the first entry wins

auth-simple resolves an app by scanning the config map and taking the first normalized match:

// dstack/kms/auth-simple/index.ts:173
const appConfig = Object.entries(config.apps).find(
  ([id]) => normalizeHex(id) === appId
)?.[1];

normalizeHex lowercases and prepends 0x, but JSON keeps "0x…AA" and "0x…aa" as two distinct object keys. So a config with both — which is exactly what happens when an operator pastes a checksummed address next to an existing lowercase one — silently resolves to whichever appears first in the file. A tightened entry placed below a stale permissive one never runs:

[PASS] DELTA 6: duplicate app keys differing only in case — the first entry wins
       // 0x…AA: composeHashes ['0xv1', '0xv-withdrawn']   (stale, added during debugging)
       // 0x…aa: composeHashes ['0xv1']                    (intended, tightened)
       expect(r.isAllowed).toBe(true);   // for '0xv-withdrawn'

The on-chain backends have no equivalent, because the key there is a Solidity address. This is auth-simple-specific.

Steelman. Case-insensitive matching is the right behaviour — hex addresses genuinely arrive in both forms, and normalizeHex is doing the correct thing. .find returning the first match is the obvious JS idiom, and the duplicate-key case is an operator error, not an input an attacker controls. Detecting it costs a pass over the map that the happy path does not need.

Cost. A silent config-authoring hazard in the one backend whose whole interface is a hand-edited JSON file, where the failure mode is "the tighter policy you wrote is ignored" rather than an error. docs/auth-simple-operations.md walks operators through iterative tightening of exactly this map.

Fix direction (no contract change; one commit in auth-simple):

  1. Normalize at load and reject duplicates. In loadConfig, build a Map keyed on normalizeHex(id) and throw — or log an error and fail closed — when two raw keys collide. Loudest, and it catches the error at the moment the operator makes it. Costs one pass per request, or zero if the config is cached (it is currently re-read per call, which is deliberate — see below).
  2. Normalize at load and take the most restrictive merge instead of the first. Avoids a hard failure but hides the mistake; probably worse.
  3. Validate in the zod schema with a superRefine on apps that rejects normalized-key collisions. Keeps the check with the rest of the config contract and is the smallest diff.

(3), with (1)'s fail-closed behaviour on violation.


2. A backend migration changes who holds app policy authority, and nothing downstream can tell

Walking the on-chain ↔ auth-simple migration end to end, two structural properties change that no per-field delta captures:

  • Authority moves. On-chain, an app's compose-hash allowlist is controlled by the app owner's key, on a contract the KMS operator does not own. Under auth-simple it is a JSON file on the KMS operator's filesystem. Migration transfers the app's policy authority to the infrastructure operator, and back, with no signal to the app or its users.
  • The audit trail disappears. PolicyChanged gives a permanent, third-party-verifiable record of every policy change. A config edit leaves no artifact outside the operator's host.

And the health endpoint cannot distinguish the two backends, because kmsContractAddr, chainId and appImplementation are free-form config values in auth-simple:

// dstack/kms/auth-simple/index.ts:44-48
kmsContractAddr: z.string().default('0x0000000000000000000000000000000000000000'),
chainId: z.number().default(0),
appImplementation: z.string().default('0x0000000000000000000000000000000000000000'),
[PASS] DELTA 5: the health endpoint can be made byte-identical to the on-chain backend
       expect(json.chainId).toBe(2035);
       expect(json.kmsContractAddr).toBe('0xA1B2C3D4E5F6071829304152637485960718293A');

GetInfo / GetMeta surface exactly these fields (kms/src/main_service.rs:471-473), so a migrated deployment can report the old contract address and chain id verbatim and nothing downstream can tell.

Steelman. auth-simple is a documented, supported, first-class backend — docs/auth-simple-operations.md exists for it — and choosing it is a legitimate deployment posture for air-gapped, single-tenant or pre-production settings where running a chain is not worth it. The config fields are surfaced because the KMS's response schema requires them; defaulting them to zero and letting an operator fill them in is the honest way to satisfy a shared schema from a backend that has no chain. Inventing a "backend kind" field also means every consumer has to learn to interpret it, and an operator who wants to misrepresent their posture can lie about any field you add.

The fail-closed behaviour is genuinely good and worth recording: an unreadable or missing config denies everything, matching the on-chain backend's behaviour when the RPC is unreachable. Both postures fail closed. Verified independently, not by comparison:

[PASS] DELTA 4: an unreadable config denies everything (fail-closed), like an unreachable RPC

Cost. An app cannot require on-chain governance as a condition of running, and a relying party cannot verify that the KMS it is trusting is governed by the contract it thinks it is. The posture is not attested, not measured and not reportable. Since the KMS's own app-compose is measured and its config is not, this is a property of the deployment that sits outside everything the attestation covers.

Related and already recorded, not re-reported here: K-e in AUDIT-BACKLOG.md (the compose-simple topology puts the whole authorization decision on an unauthenticated plain-HTTP channel), partly addressed by #1268; and the known advisoryIds / TCB-default divergences between the backends.

Fix direction (no contract change):

  1. Docs only. A "what changes when you migrate" section in docs/auth-simple-operations.md naming both properties: who controls the app allowlist, and that no audit trail survives. Zero cost, immediately useful, and it is missing today.
  2. Report the backend kind honestly. Add a non-overridable field to the health/GetInfo response — backend: "config" vs "ethereum" — set by the backend's own code rather than by config, and plumb it through GetMeta. Does not stop a determined operator from running a modified backend, but it removes the accidental case and gives an honest deployment something to point at. Small and additive; GetMeta already gained an optional field in #1268 (os_image_verification) by the same pattern.
  3. Measure the auth config. Include a hash of the resolved auth config in the KMS's own attested measurements, so the posture is inside the attestation rather than beside it. This is the only option that actually makes the posture verifiable, and it is much larger — it needs a decision about config mutability at runtime, which conflicts with the current re-read-per-request design that gives auth-simple its instant revocation latency.
  4. Refuse the impersonation case narrowly. Have auth-simple reject a config that sets a non-zero kmsContractAddr or chainId, since it has no chain to back them. Cheap, but it breaks any operator legitimately using those fields as documentation of a prior deployment; probably only worth it alongside (2).

(1) is required regardless; (2) is the cheap honest improvement; (3) is the real fix if the team wants the posture to be verifiable.


Found during a scenario-driven review of the authorization contracts; full walk in .agent/CONTRACT-SCENARIOS.md (scenario 7), tests in dstack/kms/auth-simple/migration.test.ts.

主要言語
Rust
スター
546
フォーク
96
平均マージ
19時間 22分
マージ済み PR(30日)
109

コントリビューションガイド

コントリビューションガイドを開く

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

Dstack-TEE/dstack のほかの issue

Dstack-TEE/dstack の issue をすべて見る

似ている issue

Rust の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。