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

Aperta
#1,301 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
5/5
Tempo stimato
Più di una settimana
Idoneità per principianti
30/100
Tipo di issue
Funzionalità
Chiarezza
Abbastanza chiara
Stato di attività
Attiva
Stack tecnologico
rust, typescript

Direzione di ricerca

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.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

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.

Lingua principale
Rust
Stelle
546
Fork
96
Merge medio
19h 22m
PR unite (30g)
109

Guida per i contributori

Apri la guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di Dstack-TEE/dstack

Tutte le issue di Dstack-TEE/dstack

Issue simili

Altre issue su Rust

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.