From 2c5c409edd1a572850873d97908c3193208474b4 Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 10 Sep 2026 13:08:10 +0200 Subject: [PATCH 01/48] =?UTF-8?q?docs(encryption-suites):=20spec=20?= =?UTF-8?q?=E2=80=94=20harden=20vault=20key-material=20guards=20(#673)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenSpec change harden-vault-key-material-guards: proposal, design, tasks, and spec deltas for a verified master-password proof (VaultKeyProof) gating the irreversible key-material operations, plus a migration abort route. Reproduced end-to-end against development; findings 1 and 2 documented in the proposal. Spec only — implementation lands in a separate commit. Refs #673 Assisted-by: ClaudeCode:claude-opus-5 --- .../.openspec.yaml | 2 + .../design.md | 162 +++++++ .../plan.json | 434 ++++++++++++++++++ .../proposal.md | 61 +++ .../specs/emergency-access/spec.md | 23 + .../specs/encryption-suites/spec.md | 142 ++++++ .../specs/vault-key-proof/spec.md | 121 +++++ .../harden-vault-key-material-guards/tasks.md | 75 +++ 8 files changed, 1020 insertions(+) create mode 100644 openspec/changes/harden-vault-key-material-guards/.openspec.yaml create mode 100644 openspec/changes/harden-vault-key-material-guards/design.md create mode 100644 openspec/changes/harden-vault-key-material-guards/plan.json create mode 100644 openspec/changes/harden-vault-key-material-guards/proposal.md create mode 100644 openspec/changes/harden-vault-key-material-guards/specs/emergency-access/spec.md create mode 100644 openspec/changes/harden-vault-key-material-guards/specs/encryption-suites/spec.md create mode 100644 openspec/changes/harden-vault-key-material-guards/specs/vault-key-proof/spec.md create mode 100644 openspec/changes/harden-vault-key-material-guards/tasks.md diff --git a/openspec/changes/harden-vault-key-material-guards/.openspec.yaml b/openspec/changes/harden-vault-key-material-guards/.openspec.yaml new file mode 100644 index 000000000..1ea7e36f4 --- /dev/null +++ b/openspec/changes/harden-vault-key-material-guards/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-09 diff --git a/openspec/changes/harden-vault-key-material-guards/design.md b/openspec/changes/harden-vault-key-material-guards/design.md new file mode 100644 index 000000000..c04d92363 --- /dev/null +++ b/openspec/changes/harden-vault-key-material-guards/design.md @@ -0,0 +1,162 @@ +# Design — harden-vault-key-material-guards + +## Context + +Under ADR-003 (always-E2E) the server holds ciphertext and an AES-wrapped private key, never the master password. That makes *reading* the vault cryptographically gated. It leaves *writing* gated only by the Nextcloud session, because writing a secret needs nothing but the owner's public key — which is by design. + +Issue #395 shows what that costs on the paths that write **key material** rather than secrets. `EncryptionSuiteController::compromiseRecovery()` accepts an attacker's own keypair and proves nothing about the suite being replaced; `updatePrivateKey()` overwrites the envelope in place behind an ownership check; `MigrationController::complete()` marks the old suite `compromised`; `EmergencyAccessController::destroy()` deletes the recovery envelope. None can be undone: `EncryptionSuiteService::reinstateSuite()` accepts `revoked` and refuses `compromised`, and there is no abort route. + +Keepiq already has both halves of the mechanism this needs, unconnected: + +- **Server**: `lib/Middleware/JwtAuthMiddleware.php` + `PlatformIntegrationRegistrar.php:64` establish the app-middleware pattern (`beforeController` throws, `afterException` renders JSON). `tests/Unit/Controller/RateLimitAttributesTest.php` establishes attribute-coverage testing as a build guard. +- **Client**: `src/crypto/reauth.js` derives the AES key from a freshly entered master password, decrypts the private-key envelope to prove knowledge, and discards every derived key immediately. Its own header states that the control is *advisory* because only the client sees the result. + +This change connects them. + +## Goals / Non-Goals + +**Goals** + +- No irreversible operation on vault contents or key material succeeds without a **server-verified** proof of master-password knowledge +- The guard is declarative and reusable: a future destructive route opts in with one attribute, and forgetting the attribute fails the build +- Every wedged migration has a route back to a working vault (abort) +- No new runtime dependency, no new table, no new cache requirement + +**Non-Goals** + +- Gating *every* write on the master password. See D3 — a blanket rule would be strictly less safe than a targeted one +- Recovering a vault whose owner has genuinely forgotten the master password. Rotation exists for a key that may be *exposed*, not for a password that was *forgotten*; the lost-password route is administrator revocation and is deliberately deferred to a follow-up change +- Defending against a client that keylogs the master-password field. No client-side-rooted E2E system can, and `reauth.js` already says so +- Retrofitting the three existing advisory `verifyMasterPassword()` gates. Follow-up change + +## Decisions + +### D1: The proof is a signature over a server-issued challenge, verified against the stored public key + +`GET /api/v1/suites/{id}/proof-challenge` returns a nonce. The client decrypts the private-key envelope with the freshly entered master password, re-imports the PKCS#8 bytes with `['sign']` usage, signs, discards the key, and sends the signature in `X-Keepiq-Key-Proof`. The middleware verifies it against the suite's stored public key. + +The server can do this because it already holds the public key and the certificate. It cannot verify anything about the *plaintext* — and does not need to. Possession of the private key implies possession of the master password, because the private key exists only inside an AES envelope keyed by PBKDF2-SHA256 over that password. + +This is what closes finding 2 in the proposal. Gating `complete()` alone is insufficient because an attacker can commit garbage ciphertext and complete with zero failures; gating the *entry* to rotation stops every downstream variant, including that one. `complete()` is still guarded, as defence in depth, but it is not where the fix lives. + +### D2: Signature, never decryption — this is load-bearing + +The obvious alternative is a decrypt challenge: the server encrypts a nonce to the suite public key and the client returns the plaintext. **This must not be used.** The session `CryptoKey` (`src/crypto/rsa.js:61-66`) is imported: + +```js +crypto.subtle.importKey('pkcs8', keyData, + { name: 'RSA-OAEP', hash: 'SHA-256' }, + false, // extractable = false — security critical + ['decrypt'], // decrypt only +) +``` + +Non-extractable, and decrypt-only. So: + +| challenge design | satisfiable by an unlocked tab | satisfiable by XSS in that tab | +|---|---|---| +| "decrypt this nonce" | yes | yes | +| "sign this nonce" | no | no | + +A decrypt challenge is satisfiable by the long-lived session key, which means XSS in an unlocked tab defeats it. Signing requires re-importing the raw PKCS#8 bytes with `['sign']` usage, and those bytes exist only for the instant `decryptPrivateKey()` (`src/crypto/aes.js:79`) returns them — which requires the password. `extractable: false` is precisely what makes the proof unforgeable from a live session, and it only pays off if the proof is a signature. + +Anyone tempted to simplify this later should read this decision first. + +### D3: The guard is a step-up gate on irreversible operations, not a blanket write gate + +Producing a signature requires the raw private key. Gating every write therefore means either a password prompt plus ~1s of PBKDF2 (600k rounds) on every secret created, or holding a signing-capable key in memory for the session. + +The second undoes `extractable: false` and hands XSS the exact capability the guard exists to deny. A blanket rule would make the app **less** safe than a targeted one. The enforceable invariant is therefore: + +> No irreversible operation on vault contents or key material without a server-verified proof of the master password — produced at the moment the user enters it, and discarded immediately. + +Reading is already cryptographically gated and needs nothing added. Creating a secret needs only the public key and stays ungated. + +### D4: The attribute carries the binding, so the middleware stays route-agnostic + +A proof that authorises "some operation" is replayable onto a different operation. Binding the signature to the request is what prevents that — but the middleware cannot see the request body. `Request::decodeContent()` reads `php://input` via `file_get_contents`, `json_decode`s it and **discards the raw string**; `getContent()` is `protected`; and `IRequest`'s entire public surface is `getHeader / getParam / getParams / ...` with no raw-body accessor. Re-reading `php://input` from app code would bypass the injectable `inputStream` the Request is constructed with, making the guard the one part untestable in an isolated PHPUnit run. + +So the attribute declares the binding and the middleware reads named parameters: + +```php +#[VaultKeyProofRequired(binds: ['publicKey', 'encryptedPrivateKey'])] +public function compromiseRecovery(string $publicKey, string $encryptedPrivateKey): JSONResponse + +#[VaultKeyProofRequired(binds: ['encryptedPrivateKey'], subject: 'routeParam:id')] +public function updatePrivateKey(string $id, string $encryptedPrivateKey): JSONResponse +``` + +- `binds` — request parameters the proof commits to, hashed individually in declared order +- `subject` — whose public key verifies: `'active'` (the session user's active suite, default) or `'routeParam:'` + +Signed payload: `nonce || sha256(param_1) || ... || sha256(param_n)`. + +Three things fall out. There is no canonicalisation problem — only named scalar parameters, hashed individually, so `crypto.subtle` and PHP never have to agree on JSON key ordering, number formatting or unicode normalisation. The binding is legible at the route rather than buried in the middleware. And the proof travels as a header, so no guarded controller signature grows a `?string $proof` it never reads. + +### D5: The nonce is stateless, because the binding makes single-use unnecessary + +`nonce = base64(random) . '.' . HMAC(instance secret, random | uid | purpose | exp)`. The middleware verifies the HMAC and the expiry; no storage, no table. + +Replay is not a gap here. Because the signature commits to the operation's parameters, a captured proof only ever re-authorises the byte-identical operation: for `compromiseRecovery` that is the victim's own successor key, for `updatePrivateKey` it is re-setting the envelope already in place. `purpose` binds the challenge to one route, so a proof for one guarded operation cannot be presented to another. + +Deliberately **not** `ICacheFactory`: without a configured distributed cache Nextcloud returns a null cache, and a nonce store that silently forgets would break the flow on a default install. + +### D6: Abort terminates a migration only while nothing has been committed + +The reachable states, given a migration A -> B: + +``` + A ──────────────▶ B n of m records already re-encrypted to B + migration + + abort + revoke B ⇒ those n records unreadable ✗ + abort + keep B active ⇒ the other m-n stranded on A ✗ + abort only while n = 0 ✓ +``` + +The third rule is both defensible and sufficient: an attacker commits nothing, because producing valid re-encrypted ciphertext requires the plaintext and therefore the master password. Once any record has been committed the remedy is resume, not abort, and the refusal names the count. + +Abort sets `aborted`, clears failure accounting, revokes the unused successor suite, leaves the old suite `active`, releases the write lock, and dispatches a new `SuiteMigrationAbortedEvent` so `SuiteMigrationStartedListener`'s locked SecretRequests are released. It **must not** dispatch `SuiteMigrationCompletedEvent` — that is what `EmergencyAccessSuiteRotationListener` consumes to invalidate the recovery envelopes, and abort exists to avoid exactly that loss. + +Abort carries **no** `#[VaultKeyProofRequired]`, deliberately. It is restorative: it returns the vault to the old suite, still `active`. An attacker aborting a victim's legitimate rotation is a nuisance the victim can simply redo, whereas a proof requirement on abort would leave a wedged vault wedged. + +### D7: Coverage is guarded by a test, because attribute guards fail open by omission + +The failure mode of every declarative guard is the route that forgets it: nothing errors, the guard is simply absent. Notably, NC's own `PasswordConfirmationMiddleware` shows the same shape from the inside — `canConfirmPassword()`, the `SCOPE_SKIP_PASSWORD_VALIDATION` token scope and an `excludedUserBackEnds` list for SAML each `return;` and the guard disappears rather than failing. + +`RateLimitAttributesTest` already solves this locally for `#[AnonRateLimit]`: enumerate the routes that must carry an attribute, assert by reflection that each does. `VaultKeyProofAttributesTest` does the same for the destructive list, so a new destructive route without the guard turns the build red. + +Our guard has no equivalent bypass to make: it never consults the auth backend, so it behaves identically on SSO, app-password and ordinary sessions. + +### D8: Verification lives in a service, not in the middleware + +`VaultKeyProofService` owns challenge issuance and signature verification; the middleware owns attribute dispatch, subject resolution, parameter collection and the 403. This keeps the crypto unit-testable without the app framework, and mirrors how `JwtAuthMiddleware` delegates to `JwtAuthService`. + +The 403 body carries `error: 'key_proof_required'` so a client can tell "fetch a challenge and retry" from a dead end, the same way `migration_incomplete` and `migration_in_progress` are already distinguishable. + +## Risks / Trade-offs + +- **`updatePrivateKey` is the hot path.** It is the routine master-password change, so the guard lands on a flow users hit regularly. Mitigated by the fact that the flow already holds the old password in order to re-wrap the envelope — the proof is free at that moment. If the flow is ever changed to derive the new envelope without materialising the old key, the guard breaks; the spec scenario pins this +- **Breaking API change on four routes.** Deliberate, and cheap only because the app is pre-production. Any out-of-tree client of those routes must be updated +- **A user who has forgotten the master password can no longer rotate.** This is the correct behaviour, not a regression — but it means the lost-password route (administrator revocation, with the emergency-access warnings from #395) is now load-bearing and must not be deferred indefinitely +- **PBKDF2 cost on the guarded flows.** ~1s per proof at 600k rounds. Acceptable on operations a user performs a handful of times; unacceptable per-write, which is D3 +- **Proof of possession is not proof of intent.** A user tricked into typing their master password into a hostile flow still produces a valid proof. The guard raises the bar from "a cookie" to "the password", which is the stated goal, and no further + +## Migration Plan + +No data migration. `aborted` is a new value in a plain `string` status column, so no schema change and no `` bump. + +Ordering matters for the rollout, because the guard is a breaking change to routes the shipped frontend calls: + +1. Attribute, service, middleware, challenge endpoint, registration — inert until a route opts in +2. `abort` route and its event — independently useful, unblocks any already-wedged migration +3. Client `proveMasterPassword()` and the four call sites +4. Apply `#[VaultKeyProofRequired]` to the four routes, plus `VaultKeyProofAttributesTest` + +Steps 3 and 4 must land together, or in that order, or the frontend breaks against its own backend. Migrations already `in_progress` when this deploys are unaffected: the guard applies to starting a rotation and to completing one, and `abort` gives any migration wedged by a pre-fix attempt a way out. + +## Open Questions + +- Should `complete()` keep the `acceptUnrecoverable` acknowledgement now that a key proof is required? It no longer carries the security weight (finding 2), but it is still the mechanism that makes losing a record a decision the owner made rather than a side-effect. Recommendation: keep both; they answer different questions +- The lost-password route (administrator revocation with the "this deletes emergency access" warning, and refusing to revoke while a usable emergency contact exists) is specced in #395 but deliberately out of scope here. It should be the immediate follow-up, because this change makes it the only remaining route for a forgotten password +- `#395` also observes that **any** completed rotation costs the user their emergency access, since `invalidateForGrantorRotation()` fires on `SuiteMigrationCompletedEvent`. Confirmed: `CompromiseRecoveryForm.vue` never mentions emergency access, so users silently lose their break-glass path after a routine key change. Not fixed here — worth its own change alongside the lost-password route diff --git a/openspec/changes/harden-vault-key-material-guards/plan.json b/openspec/changes/harden-vault-key-material-guards/plan.json new file mode 100644 index 000000000..a24b04d7f --- /dev/null +++ b/openspec/changes/harden-vault-key-material-guards/plan.json @@ -0,0 +1,434 @@ +{ + "change": "harden-vault-key-material-guards", + "project": "keepiq", + "repo": "ConductionNL/keepiq", + "base_branch": "development", + "feature_branch": "feature/673/harden-vault-key-material-guards", + "created": "2026-09-10", + "tracking_issue": 673, + "tasks": [ + { + "id": 1, + "num": "1.1", + "title": "Create `lib/Attribute/VaultKeyProofRequired.php`: `#[Attribute(Attribute::TARGET_METHOD)]`, constructor `array $binds = []`, `string $subject = 'active'`; SPDX header per `contribute/HowToApplyALicense.md`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 2, + "num": "1.2", + "title": "Create `lib/Service/VaultKeyProofService.php` with `issueChallenge(string $userId, string $purpose): array` returning `{nonce, expiresAt}` \u2014 nonce is `base64(ISecureRandom bytes) . '.' . HMAC(instance secret, random|uid|purpose|exp)`; no storage", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 3, + "num": "1.3", + "title": "Implement `VaultKeyProofService::verify(string $nonce, string $signature, string $publicKeyPem, string $userId, string $purpose, array $boundValues): void` \u2014 validate the HMAC, validate the expiry, rebuild the payload as `nonce || sha256(v1) || \u2026 || sha256(vn)` in declared order, verify with `openssl_verify` against the stored public key; throw a typed exception on every failure path with no distinction leaked to the caller", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 4, + "num": "1.4", + "title": "Do NOT use `ICacheFactory` for challenge state (design D5 \u2014 a null cache on a default install would make the guarded flows unusable). Assert this in review", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 5, + "num": "1.5", + "title": "Create `lib/Middleware/VaultKeyProofMiddleware.php` following `JwtAuthMiddleware`: `beforeController` reads the attribute via `new ReflectionMethod($controller, $methodName)`, resolves the subject suite (`'active'` \u2192 the session user's active suite via `EncryptionSuiteService::getActiveSuite`; `'routeParam:'` \u2192 `IRequest::getParam`), collects the bound values via `IRequest::getParam`, reads `X-Keepiq-Key-Proof`, and delegates to the service", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 6, + "num": "1.6", + "title": "Implement `afterException` returning `403` with `['error' => 'key_proof_required', 'message' => \u2026]`; re-throw anything that is not the guard's own exception, as `JwtAuthMiddleware` does", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 7, + "num": "1.7", + "title": "Middleware MUST NOT consult `IUserSession` backends, token scopes or `IPasswordConfirmationBackend` \u2014 no SSO/app-password carve-out (spec: *the guard is not waived*). Add an explanatory comment citing the NC `PasswordConfirmationMiddleware` bypasses this deliberately does not copy", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 8, + "num": "1.8", + "title": "Register in `lib/AppInfo/PlatformIntegrationRegistrar.php` alongside `JwtAuthMiddleware::class`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 9, + "num": "1.9", + "title": "Run phpcs/phpstan/phpmd \u2014 watch `CouplingBetweenObjects` on the middleware; keep crypto in the service, which is also what makes it unit-testable", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 10, + "num": "2.1", + "title": "Add `proofChallenge(string $id)` to `EncryptionSuiteController` (`#[NoAdminRequired]`), returning `{nonce, expiresAt}` for the calling user and the requested purpose; validate suite ownership", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 11, + "num": "2.2", + "title": "Accept the purpose as a request parameter constrained to a known set (one per guarded operation); reject an unknown purpose", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 12, + "num": "2.3", + "title": "Register `['name' => 'encryptionSuite#proofChallenge', 'url' => '/api/v1/suites/{id}/proof-challenge', 'verb' => 'GET']` in `appinfo/routes.php`, before the SPA catch-all wildcard", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 13, + "num": "2.4", + "title": "The challenge endpoint itself MUST NOT carry `#[VaultKeyProofRequired]` \u2014 assert in the coverage test that it is on the deliberate-exclusion list", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 14, + "num": "3.1", + "title": "Add `MigrationService::abortMigration(string $migrationId): array` \u2014 refuse unless `in_progress`; refuse when any record has been committed to the new suite, reporting the count and pointing at resume; idempotent by status like `completeMigration`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 15, + "num": "3.2", + "title": "On success: set status `aborted`, leave the old suite `active` and its records untouched, revoke the successor suite via `EncryptionSuiteService::revokeSuite`, clear failure accounting via `workService->clearFailureAccounting`, release the write lock", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 16, + "num": "3.3", + "title": "Create `SuiteMigrationAbortedEvent` and a listener that unlocks the SecretRequests locked by `SuiteMigrationStartedListener`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 17, + "num": "3.4", + "title": "**Must not** dispatch `SuiteMigrationCompletedEvent` \u2014 that is what `EmergencyAccessSuiteRotationListener` consumes to invalidate recovery envelopes. Add a regression test asserting envelopes survive an abort", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 18, + "num": "3.5", + "title": "Add `MigrationController::abort(string $id)` (`#[NoAdminRequired]`) with the existing `requireOwnMigration` ownership check; **no** `#[VaultKeyProofRequired]` (design D6 \u2014 abort is restorative)", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 19, + "num": "3.6", + "title": "Register `['name' => 'migration#abort', 'url' => '/api/v1/migrations/{id}/abort', 'verb' => 'POST']`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 20, + "num": "3.7", + "title": "Update the refusal text in `EncryptionSuiteController::compromiseRecovery()` so the promised \"abort\" now names a route that exists", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 21, + "num": "3.8", + "title": "Add an abort control to `src/components/MigrationResumeBanner.vue`, shown only while abort is still available, with copy stating that the old suite stays intact", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 22, + "num": "4.1", + "title": "Add `proveMasterPassword(encryptedPrivateKey, masterPassword, nonce, boundValues)` to `src/crypto/reauth.js`: decrypt the envelope via `decryptPrivateKey` (`src/crypto/aes.js:79`), re-import the PKCS#8 bytes with `['sign']` usage, sign `nonce || sha256(v1) || \u2026`, return the signature", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 23, + "num": "4.2", + "title": "Discard the derived AES key, the raw PKCS#8 bytes and the signing key immediately after signing; never return, store or cache them (spec: *the signing key does not outlive the proof*). Keep `verifyMasterPassword` as-is for the three existing advisory call sites \u2014 they are out of scope for this change", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 24, + "num": "4.3", + "title": "Confirm the signing key is imported with `['sign']` only and is NOT the session `CryptoKey`; add a unit test asserting the session key (`src/crypto/rsa.js:61-66`) remains non-extractable and `['decrypt']`-only", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 25, + "num": "4.4", + "title": "Add a shared client helper that fetches a challenge, prompts for the master password, produces the proof, and sets the `X-Keepiq-Key-Proof` header \u2014 so the four call sites do not each re-implement it", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 26, + "num": "4.5", + "title": "Wire `src/components/CompromiseRecoveryForm.vue` (recovery start, and the completion call) through the helper", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 27, + "num": "4.6", + "title": "Wire the routine master-password change flow through the helper; verify the old private key is materialised at that point (design \"Risks\" \u2014 if it is not, stop and raise before proceeding)", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 28, + "num": "4.7", + "title": "Wire the emergency-contact delete action in `src/views/EmergencyAccessView.vue` / `src/store/modules/emergencyAccess.js` through the helper", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 29, + "num": "4.8", + "title": "Handle `403 key_proof_required` as \"re-enter your master password and retry\", not as a terminal error", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 30, + "num": "5.1", + "title": "`EncryptionSuiteController::compromiseRecovery` \u2192 `#[VaultKeyProofRequired(binds: ['publicKey', 'encryptedPrivateKey'])]`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 31, + "num": "5.2", + "title": "`EncryptionSuiteController::updatePrivateKey` \u2192 `#[VaultKeyProofRequired(binds: ['encryptedPrivateKey'], subject: 'routeParam:id')]`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 32, + "num": "5.3", + "title": "`MigrationController::complete` \u2192 `#[VaultKeyProofRequired]` (defence in depth; the acknowledgement stays \u2014 they answer different questions)", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 33, + "num": "5.4", + "title": "`EmergencyAccessController::destroy` \u2192 `#[VaultKeyProofRequired]`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 34, + "num": "5.5", + "title": "Create `tests/Unit/Controller/VaultKeyProofAttributesTest.php` in the shape of `RateLimitAttributesTest`: a provider enumerating the four methods with their expected `binds` and `subject`, asserting each by reflection; plus a deliberate-exclusion list (abort, proof-challenge) with the reason recorded per entry", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 35, + "num": "6.1", + "title": "`tests/Unit/Service/VaultKeyProofServiceTest.php`: valid proof passes; wrong key fails; altered bound value fails; altered nonce fails; expired nonce fails (injected `ITimeFactory`); wrong purpose fails; proof for one parameter set rejected against another", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 36, + "num": "6.2", + "title": "`tests/Unit/Middleware/VaultKeyProofMiddlewareTest.php`: attribute absent \u2192 pass-through; attribute present without header \u2192 403 `key_proof_required`; `subject: 'active'` and `'routeParam:id'` both resolve; `afterException` re-throws foreign exceptions", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 37, + "num": "6.3", + "title": "Cross-implementation round-trip: sign with WebCrypto in a JS test, verify with `openssl_verify` in PHPUnit (config rule: *test cross-implementation encryption round-trips*)", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 38, + "num": "6.4", + "title": "`MigrationServiceTest`: abort on an untouched migration; abort refused after a commit with the count reported; abort idempotent by status; emergency-access envelopes unchanged after abort; completed event NOT dispatched", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 39, + "num": "6.5", + "title": "`EncryptionSuiteControllerTest` / `MigrationControllerTest` / `EmergencyAccessControllerTest`: the four guarded routes refuse without a proof and proceed with one", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 40, + "num": "6.6", + "title": "Regression test for the bypass in proposal finding 2: committing ciphertext for every record and completing MUST now be refused at the recovery entry point without a proof", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 41, + "num": "6.7", + "title": "Frontend unit tests for `proveMasterPassword` (signature verifies against the suite public key; keys discarded) and for the 403 retry path", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 42, + "num": "7.1", + "title": "Run the hydra gates locally: route-auth (two new routes), no-admin-idor, gate-16 spec-coverage, gate-113 exclusion-evidence (every `@e2e exclude` in this change carries a reason). Note the known pre-existing `no-admin-idor` debt on `development` is not introduced here", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 43, + "num": "7.2", + "title": "Confirm gate-110 does not apply (no migration added) \u2014 if a migration is introduced after all, bump `appinfo/info.xml` `` from `0.3.1`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 44, + "num": "7.3", + "title": "Document the guard in `docs/ARCHITECTURE.md`: the attribute contract, the sign-not-decrypt rationale (design D2), and the rule that a new destructive route must be added to `VaultKeyProofAttributesTest`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 45, + "num": "7.4", + "title": "Every commit carries `Assisted-by: ClaudeCode:claude-opus-5`. Do NOT add `Signed-off-by` \u2014 only the human contributor certifies the DCO", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 46, + "num": "7.5", + "title": "The PR description discloses AI tool use, in the contributor's own words, and links issue #395", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 47, + "num": "7.6", + "title": "Before opening: re-read #395's \"Verification status\" \u2014 the chain was never executed end to end. Reproduce the lockout on a throwaway account against pre-fix code, then confirm the same steps are refused post-fix. This is the issue's own first task and it is still outstanding", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + } + ] +} diff --git a/openspec/changes/harden-vault-key-material-guards/proposal.md b/openspec/changes/harden-vault-key-material-guards/proposal.md new file mode 100644 index 000000000..aa325e08d --- /dev/null +++ b/openspec/changes/harden-vault-key-material-guards/proposal.md @@ -0,0 +1,61 @@ +## Why + +Issue #395 (`ConductionNL/keepiq`, 2026-08-21) reports that an attacker holding **only an authenticated Nextcloud session** for a Keepiq user can permanently destroy that user's access to their entire vault — including their pre-arranged emergency-access recovery — without learning a single secret. + +The asymmetry is the point. Keepiq is zero-knowledge: a session alone does not let anyone read the vault, because decryption needs the master password, which the server never holds. A stolen session is therefore normally *not* game-over. These paths turn it into one, for destruction rather than disclosure. + +Stated as the invariant that is currently violated: + +> Reading the vault requires the master password. Destroying it requires a cookie. + +Re-verified on `development` @ `c1cac29c`, four independent paths reach permanent loss from a session alone: + +| Path | Effect | Reversible today | Gate today | +|---|---|---|---| +| `PUT /api/v1/suites/{id}/private-key` | overwrites the private-key envelope in place | only via emergency access | ownership | +| `POST /api/v1/suites/compromise-recovery` | mints a successor suite under attacker-supplied key material, write-locks the vault | **no — no abort route exists** | ownership | +| `POST /api/v1/migrations/{id}/secrets/{secretId}` | writes client-supplied ciphertext verbatim over the original | no | ownership | +| `POST /api/v1/migrations/{id}/complete` | marks the old suite `compromised`, invalidates emergency access | no | acknowledgement count (see below) | +| `DELETE /api/v1/emergency-access/contacts/{id}` | deletes the recovery envelope — the only survivor of row 1 | no | ownership | + +Three findings beyond the filed issue, established while tracing it: + +1. **`updatePrivateKey` is a one-request version of the same lockout.** Its only gate is `validateOwnership()`. Posting a garbage envelope means the master password no longer decrypts anything, and the private key existed *only* as that envelope. Fewer steps than the filed chain, no acknowledgement, no audit trail, and no write-lock guard — so it works mid-migration too. +2. **The acknowledgement gate in `complete()` can be bypassed entirely.** The filed chain reports per-record failures, which forces the `acceptUnrecoverable` handshake. An attacker need not: `reEncryptSecret()` accepts client-supplied ciphertext and `commitSecret()` writes it verbatim, and the round-trip verification the spec names is performed in the *browser* — the server structurally cannot repeat it under ADR-003. Committing garbage for every record yields zero failures, so completion succeeds with no acknowledgement at all. **Hardening `complete()` therefore does not close the hole; the gate has to be at the entry to rotation.** +3. **The safety net is removable by the same authority.** `EmergencyAccessController::destroy()` is session-only, so the attack sequence is *delete the emergency contacts, then lock out*. + +Two facts make this cheap to fix correctly rather than expensively: + +- **The spec already assumes the gate exists.** `encryption-suites` -> *Master Password Change — Compromise Recovery* reads "AND provides their old master password and a new master password". The old password *is* collected; it is consumed entirely client-side, so the server never observes any consequence of it. This change does not introduce new policy — it makes the server able to verify what the spec already claims. +- **The client-side half is already written.** `src/crypto/reauth.js` implements master-password re-authentication and documents its own limitation: *"a 're-auth' gate is a CLIENT-SIDE proof of knowledge... The control is advisory against a tampered client."* It is already used by `AccountDeletionDialog.vue:199`, `CxpTransferDialog.vue:365` and `ExportDialog.vue:349`. The upgrade is a return type: a boolean the client consumes becomes a signature the **server** verifies. + +`#392` (`d475d00d`, *refuse a plain create when the owner already has an active suite*) closed one milder instance of the same theme and does not address any path above. + +## What Changes + +- Introduce a reusable, attribute-driven guard — `#[VaultKeyProofRequired]` plus `VaultKeyProofMiddleware` — that refuses a request unless it carries a signature, made with the private key of the owner's EncryptionSuite, over a server-issued challenge bound to the operation's own parameters. Because the private key is only obtainable by decrypting its envelope with the master password, this is a server-verifiable proof of the master password +- Add a challenge endpoint (`GET /api/v1/suites/{id}/proof-challenge`) issuing a stateless, expiring, HMAC-authenticated nonce +- Apply the guard to `compromiseRecovery`, `updatePrivateKey`, `complete` and the emergency-contact `destroy` route +- Add the **abort** route that `compromiseRecovery()`'s own error message already promises ("Resume or **abort** that migration before starting another") but which does not exist in `appinfo/routes.php` or `MigrationController`. Abort is permitted only while no record has been committed, releases the write lock, revokes the unused successor suite, and leaves the old suite `active` +- Add an attribute-coverage test in the shape of the existing `RateLimitAttributesTest`, asserting every route on the destructive list carries the guard — so a future destructive route that forgets it fails the build rather than failing open +- Extend `src/crypto/reauth.js` with `proveMasterPassword()`, returning a signature instead of a boolean, and wire the four guarded flows to fetch a challenge and send the proof header + +Explicitly **not** in scope: retrofitting the three existing advisory `verifyMasterPassword()` call sites (export, CXP transfer, account deletion) onto the middleware. That is a clean follow-up once the guard exists, and folding it in here would roughly double the diff for an unrelated concern (see AGENTS.md on PR size). + +## Capabilities + +### New Capabilities +- `vault-key-proof`: A server-verified proof of master-password knowledge, expressed as a signature over a server-issued challenge made with the owner's suite private key, applied declaratively to controller methods via a PHP attribute and enforced by app middleware. Covers challenge issuance and expiry, the binding of a proof to the parameters of the operation it authorises, the signature-over-decryption requirement, and the fail-closed coverage guarantee + +### Modified Capabilities +- `encryption-suites`: compromise recovery and private-key replacement require a verified key proof; a migration gains an abort terminal state and the route that reaches it; the "always has a way to terminate" requirement gains the abort escape it currently lacks +- `emergency-access`: deleting an emergency contact requires a verified key proof, since it destroys the only recovery path that survives a private-key overwrite + +## Impact + +- **Database**: none. `SuiteMigration::$status` is a plain `string` column (`lib/Db/SuiteMigration.php:115`), so the new `aborted` terminal value needs no schema change — and therefore no migration and no `` bump for gate-110. The stateless nonce design adds no table +- **Backend**: new `lib/Attribute/VaultKeyProofRequired.php`, `lib/Middleware/VaultKeyProofMiddleware.php`, `lib/Service/VaultKeyProofService.php`; new `abort` action on `MigrationController` and `SuiteMigrationAbortedEvent`; challenge endpoint on `EncryptionSuiteController`; middleware registered in `PlatformIntegrationRegistrar` alongside the existing `JwtAuthMiddleware` +- **Frontend**: `src/crypto/reauth.js` gains `proveMasterPassword()`; `CompromiseRecoveryForm.vue`, the routine password-change flow, the emergency-contact delete action and the migration-completion call each fetch a challenge and send the proof header; a new abort control on `MigrationResumeBanner.vue` +- **API**: two new endpoints (`proof-challenge`, `abort`); four existing routes begin requiring the `X-Keepiq-Key-Proof` header and answer `403 {"error": "key_proof_required"}` without it. Breaking for any client of those four routes — acceptable and deliberate while the app carries its pre-production disclaimers +- **Security**: this is the whole point of the change. The guard resists a stolen session, a leaked app password, and XSS in an *already-unlocked* tab — the last because the session `CryptoKey` is imported non-extractable and `['decrypt']`-only (`src/crypto/rsa.js:61-66`), so it cannot produce a signature. See `design.md` D2, which is load-bearing and must not be "simplified" to a decrypt-based challenge +- **Cross-app**: none. Every guarded route is session-authenticated (`#[NoAdminRequired]`, owner derived from `IUserSession`). Application-owned suites hold no server-side envelope at all (`EncryptionSuiteProvisioningService` stores `encryptedPrivateKey: ''`) and authenticate via `JwtAuthMiddleware` on `ApplicationApiController` routes, which this change does not touch. OpenConnector is unaffected diff --git a/openspec/changes/harden-vault-key-material-guards/specs/emergency-access/spec.md b/openspec/changes/harden-vault-key-material-guards/specs/emergency-access/spec.md new file mode 100644 index 000000000..19cede6b1 --- /dev/null +++ b/openspec/changes/harden-vault-key-material-guards/specs/emergency-access/spec.md @@ -0,0 +1,23 @@ +## MODIFIED Requirements + +### Requirement: Revoke Emergency Contact +The grantor MUST be able to revoke an emergency contact at any time. Revocation MUST delete the recovery envelope and cancel any pending request, and a revoked contact MUST NOT be able to break glass until re-designated (which rebuilds a fresh envelope). + +Revocation MUST require a verified key proof (see the `vault-key-proof` capability). The recovery envelope is the only copy of the grantor's private key that survives a replacement of the stored envelope, which makes it the last recovery path out of an account lockout. An attacker holding the grantor's session would otherwise be able to delete the safety net first and destroy the vault second, using the same authority for both. + +The requirement is on the grantor-initiated revocation of a designated contact. Envelope clearing that follows from suite revocation or rotation is a consequence of those operations, is governed by *Envelope Invalidation on Key Change*, and is not separately gated here. + +#### Scenario: Revoked contact cannot break glass +@e2e exclude State-machine/authorization contract — covered by PHPUnit EmergencyAccessServiceTest (designate/request/decline/approve-by-timeout + the approved+grantee release gate with identical wrong-state/wrong-caller refusal). A live Playwright run of the DOM flow is deferred: the worktree is not deployed and deploying to the shared dev instance is prohibited. +- **GIVEN** A has designated B as an emergency contact +- **WHEN** A revokes B +- **THEN** the recovery envelope MUST be deleted and any pending request cancelled +- **AND** B MUST be unable to initiate or complete a break-glass request until re-designated + +#### Scenario: Revocation without a key proof is refused +@e2e exclude Middleware enforcement on a session-authenticated route; not DOM-observable. Covered by PHPUnit on the middleware and the attribute-coverage test. +- **GIVEN** A has designated B as an emergency contact +- **AND** an authenticated session for A +- **WHEN** revocation is requested without a verified key proof +- **THEN** the system MUST refuse with `403` and `error: key_proof_required` +- **AND** the recovery envelope MUST be unchanged and still usable diff --git a/openspec/changes/harden-vault-key-material-guards/specs/encryption-suites/spec.md b/openspec/changes/harden-vault-key-material-guards/specs/encryption-suites/spec.md new file mode 100644 index 000000000..b99428ca3 --- /dev/null +++ b/openspec/changes/harden-vault-key-material-guards/specs/encryption-suites/spec.md @@ -0,0 +1,142 @@ +## MODIFIED Requirements + +### Requirement: Master Password Change — Routine +The system MUST allow a user to change their master password for routine hygiene reasons. In this case, the RSA key pair MUST remain unchanged — only the AES wrapping of the private key changes. + +Replacing the stored private-key envelope MUST require a verified key proof (see the `vault-key-proof` capability). The envelope is the only copy of the private key, so a request that replaces it with material the owner cannot open destroys the vault in a single call; an ownership check alone is therefore insufficient authority. + +The proof MUST be bound to the submitted envelope, and MUST be verified against the public key of the suite named in the route. + +The flow already holds the current master password in order to derive the old AES key, so the proof imposes no additional prompt: the raw private key is materialised at exactly the moment the signature must be produced. An implementation that re-wraps the envelope without materialising the old private key would be unable to produce the proof and MUST NOT be adopted. + +#### Scenario: Routine password change +@e2e exclude The password-change form is rendered inside the user-settings dialog; verifying that AES key re-wrapping succeeded requires reading back the encrypted private-key blob — a crypto-API assertion, not DOM-observable. The form's UI surface is captured in user-settings::user-opens-settings. +- GIVEN a user provides their current master password and a new master password +- AND the new master password meets the configured strength floor +- WHEN the change is submitted +- THEN the system MUST decrypt the private key using the current AES-derived key +- AND re-encrypt it using the new AES-derived key +- AND store the updated blob +- AND no secrets are affected + +#### Scenario: Envelope replacement without a key proof is refused +@e2e exclude Middleware enforcement on a session-authenticated route; not DOM-observable. Covered by PHPUnit on the middleware and the attribute-coverage test. +- **GIVEN** an authenticated session for the suite owner +- **WHEN** a replacement private-key envelope is submitted without a verified key proof +- **THEN** the system MUST refuse with `403` and `error: key_proof_required` +- **AND** the stored envelope MUST be unchanged + +### Requirement: Master Password Change — Compromise Recovery +When a user indicates their master password has been compromised, the system MUST initiate a full key rotation: a new RSA key pair is generated, all secrets are re-encrypted, and the old EncryptionSuite is flagged as compromised. + +Initiating compromise recovery MUST require a verified key proof over the **old** suite's private key (see the `vault-key-proof` capability). The proof MUST be bound to the submitted successor public key and successor private-key envelope. + +Without it, the operation proves nothing about the suite it replaces: any holder of the owner's session can submit their own key pair, become the write target by suite resolution, and reach a terminal state that locks the old suite. Every downstream variant of that attack — reporting records unrecoverable, or committing ciphertext the owner cannot open — is reachable only through this entry point, so this is where the gate belongs. Gating completion alone is insufficient, because a caller who commits ciphertext for every record produces zero failures and needs no acknowledgement. + +Requiring the proof does not obstruct legitimate recovery: rotation exists for a key that may be **exposed**, not for a password that was **forgotten**, so a user rotating still knows their master password. A user who has genuinely lost it MUST be routed to administrator revocation instead, which produces an empty vault and is not a recovery. + +#### Scenario: Compromise recovery initiated +@e2e exclude Verifying RSA key pair generation, SuiteMigration record creation, and write-lock application requires inspecting server-side crypto state — not DOM-observable. The recovery UI form renders in the user-settings dialog and its presence is captured in user-settings::user-opens-settings. +- GIVEN a user selects "my master password was leaked" as the reason for changing their password +- AND provides their old master password and a new master password +- WHEN the change is submitted +- THEN the system MUST generate a new RSA key pair and EncryptionSuite +- AND create a SuiteMigration record with status `in_progress` +- AND apply a write lock to the account (no create/update operations on secrets) +- AND lock all pending SecretRequests (see secret-requests spec) +- AND begin migrating all secrets from the old suite to the new suite + +#### Scenario: Recovery without proof of the old key is refused +@e2e exclude Middleware enforcement on a session-authenticated route; not DOM-observable. Covered by PHPUnit on the middleware and the attribute-coverage test. +- **GIVEN** an authenticated session for a user with an active EncryptionSuite +- **WHEN** compromise recovery is requested with key material not accompanied by a verified proof over the existing suite's private key +- **THEN** the system MUST refuse with `403` and `error: key_proof_required` +- **AND** MUST NOT create a successor suite, a migration record, or a write lock + +### Requirement: A Migration Always Has A Way To Terminate + +A migration MUST always have a way to terminate. Completion is therefore gated on rows nobody has attempted, NOT on every row still bound to `old_suite_id`. The two are different situations and conflating them makes the write lock inescapable: a record that can never be re-encrypted would hold the migration open forever, leaving the owner permanently unable to write to their own vault. + +Termination MUST be reachable in both directions. Completion carries the migration forward to the new suite; **abort** returns it to the old suite. A migration that can only be completed is not terminable in the sense this requirement intends, because the only available exit is the destructive one — which is what made a hostile or abandoned rotation unrecoverable. + +A row is **unaccounted for** when it is still bound to `old_suite_id` and its owning secret carries no `migration_error`. The system MUST refuse to terminate a migration while any unaccounted-for row exists, because terminating locks the old suite and would take every un-reached row down with it. The refusal MUST name the remaining count and point at resuming, and MUST point at aborting when aborting is still available. + +A row that was attempted and recorded a failure MUST NOT block termination. Terminating with such rows present MUST require an explicit acknowledgement from the client stating how many records it accepts losing, and the count MUST match what the server observes; an absent or mismatched acknowledgement MUST be refused. This makes locking a secret out of the vault a decision the owner made, never a side-effect of a client calling completion — a run in which every record failed would otherwise silently lock an owner out of everything. + +Completion MUST additionally require a verified key proof (see the `vault-key-proof` capability). The acknowledgement establishes that the owner accepts the loss; the proof establishes that the caller is the owner. These answer different questions and the system MUST require both. + +Only a failure to decrypt the EXISTING ciphertext with the old key may be recorded as a per-record failure. A re-encryption that does not survive its round-trip check MUST NOT be recorded, because the original decrypted successfully and is therefore readable: the fault lies in the new key material, it will recur on every record, and the run MUST stop instead. It follows that finalisation can only ever remove access from rows that were already unreadable under the old key. + +#### Scenario: Unattempted rows refuse termination and point at resuming + +@e2e exclude Server-side query and status transition; covered by PHPUnit on the completion path. +- **GIVEN** a migration whose client stopped before processing every record, leaving rows with no `migration_error` +- **WHEN** completion is requested +- **THEN** the server MUST refuse, MUST leave the old suite `active`, and MUST keep the migration `in_progress` +- **AND** the refusal MUST report how many records remain and state that the migration can be resumed + +#### Scenario: An unrecoverable record does not trap the vault + +@e2e exclude Terminal status transition and suite locking are server-side; covered by PHPUnit on the completion path. +- **GIVEN** a migration in which every remaining row on `old_suite_id` has a recorded `migration_error` +- **WHEN** completion is requested WITHOUT an acknowledgement +- **THEN** the server MUST refuse and MUST state how many records would lose access +- **WHEN** completion is requested WITH an acknowledgement matching that count and a verified key proof +- **THEN** the migration MUST terminate as `completed_with_errors`, the old suite MUST be locked, and the write lock MUST be released +- **AND** the response MUST identify the secrets that lost access + +#### Scenario: A round-trip failure halts rather than sacrificing the record + +@e2e exclude Injected at the crypto layer; no DOM path induces it. Covered by unit tests of the migration pipeline. +- **GIVEN** a record whose existing ciphertext decrypts correctly but whose re-encryption does not survive the round-trip check +- **WHEN** the migration processes that record +- **THEN** the failure MUST NOT be recorded as a per-record migration failure +- **AND** the run MUST stop so the new key material can be investigated +- **AND** records already committed MUST remain valid, each having been verified before its own commit + +## ADDED Requirements + +### Requirement: A Migration Can Be Aborted Before Any Record Moves + +The system MUST provide a route to abort a migration in progress, and `compromise-recovery`'s refusal message MUST NOT name a remedy that does not exist. + +Abort MUST be permitted only while no record has been committed to the new suite. Once any record has moved, the two available outcomes both lose data — revoking the successor strands what has moved, keeping it active strands what has not — so the system MUST refuse to abort, MUST name the number of records already committed, and MUST point at resuming instead. + +Restricting abort this way is sufficient for the case it exists to remedy: producing valid re-encrypted ciphertext requires the plaintext, and therefore the master password, so a caller who cannot prove possession of the old key can never have committed a record. + +On abort the system MUST: + +- set the migration to the terminal status `aborted` +- leave the old EncryptionSuite `active`, and leave every record bound to it untouched +- revoke the successor suite, which by definition holds nothing +- release the write lock and unlock the SecretRequests locked when the migration started +- clear the migration's failure accounting, so a later migration does not inherit a stale acknowledgement threshold + +Abort MUST NOT dispatch the migration-completed event. That event is what invalidates the owner's emergency-access recovery envelopes, and abort exists precisely to avoid that loss. + +Abort MUST NOT require a key proof. It is restorative — it returns the vault to a suite that is still `active` and readable — and requiring proof of a key would leave a wedged vault wedged, including one wedged by a rotation the owner never authorised. A caller who aborts another user's legitimate rotation causes a nuisance the owner can simply repeat, which is not comparable to permanent loss. + +#### Scenario: Aborting an untouched migration restores the old suite + +@e2e exclude Terminal status transition, suite status and write-lock release are server-side. Covered by PHPUnit on the abort path. +- **GIVEN** a migration `in_progress` with no record committed to the new suite +- **WHEN** abort is requested by the owner +- **THEN** the migration MUST become `aborted` +- **AND** the old suite MUST remain `active` with every record still bound to it +- **AND** the successor suite MUST be revoked +- **AND** the write lock MUST be released and locked SecretRequests MUST be unlocked + +#### Scenario: Aborting after records have moved is refused + +@e2e exclude Server-side query and status transition. Covered by PHPUnit on the abort path. +- **GIVEN** a migration in which at least one record has been committed to the new suite +- **WHEN** abort is requested +- **THEN** the system MUST refuse, MUST keep the migration `in_progress` +- **AND** MUST report how many records have already been committed and state that the migration can be resumed + +#### Scenario: Abort does not destroy emergency access + +@e2e exclude Event dispatch and listener side effects are server-side. Covered by PHPUnit asserting the completed event is not dispatched and envelopes are unchanged. +- **GIVEN** an owner with a designated emergency contact and a migration `in_progress` with no record committed +- **WHEN** the migration is aborted +- **THEN** the emergency-access recovery envelopes MUST be unchanged and still usable diff --git a/openspec/changes/harden-vault-key-material-guards/specs/vault-key-proof/spec.md b/openspec/changes/harden-vault-key-material-guards/specs/vault-key-proof/spec.md new file mode 100644 index 000000000..b66551c9a --- /dev/null +++ b/openspec/changes/harden-vault-key-material-guards/specs/vault-key-proof/spec.md @@ -0,0 +1,121 @@ +## ADDED Requirements + +### Requirement: Irreversible Operations Require A Verified Key Proof + +The system MUST refuse any operation that can render vault contents or key material permanently unreadable unless the request carries a **key proof**: a signature, made with the private key of the owner's EncryptionSuite, over a challenge the server issued. + +The server MUST verify the signature against the public key it already stores for the subject suite. Because a suite's private key exists only inside an AES envelope keyed by PBKDF2-SHA256 over the master password, a verified proof establishes that the caller knows the master password. A Nextcloud session alone MUST NOT be sufficient authority for any such operation. + +The guard MUST be declared on the controller method via a `#[VaultKeyProofRequired]` attribute and enforced by middleware, so that the requirement is legible at the route and cannot be satisfied by controller code that forgets to call it. + +A request missing or failing the proof MUST be refused with `403` and a machine-readable `error` of `key_proof_required`, so a client can distinguish "obtain a challenge and retry" from a terminal failure. + +The guard MUST NOT consult the authentication backend, and MUST NOT be waived for SSO sessions, app passwords, or any token scope. Its authority derives from key material, not from how the session was established. + +#### Scenario: A session without a proof is refused + +@e2e exclude Middleware dispatch and signature verification are server-side; a DOM flow cannot present a request with the proof header withheld. Covered by PHPUnit on the middleware and service. +- **GIVEN** an authenticated session for a user who owns an active EncryptionSuite +- **WHEN** a guarded operation is requested without a key proof +- **THEN** the system MUST refuse with `403` and `error: key_proof_required` +- **AND** MUST NOT perform any part of the operation + +#### Scenario: A valid proof admits the operation + +@e2e exclude Requires signing with raw private-key bytes held only transiently in JS memory; not observable or triggerable via Playwright DOM. Covered by PHPUnit plus a cross-implementation round-trip test. +- **GIVEN** a challenge issued for the caller and the operation +- **AND** a signature over that challenge made with the subject suite's private key +- **WHEN** the guarded operation is requested carrying that proof +- **THEN** the system MUST verify the signature against the stored public key and proceed + +#### Scenario: The guard is not waived for SSO or app-password sessions + +@e2e exclude Requires provisioning an SSO or app-password session against a live instance. Covered by PHPUnit asserting the middleware reads no token scope and no user backend. +- **GIVEN** a session established by SSO, or authenticated with an app password +- **WHEN** a guarded operation is requested without a key proof +- **THEN** the system MUST refuse exactly as for an ordinary session + +### Requirement: The Proof Is A Signature, Never A Decryption + +The proof MUST be a signature produced with the subject suite's private key. The system MUST NOT accept, as proof, the decryption of a server-issued ciphertext. + +The browser holds the unlocked session key as a WebCrypto `CryptoKey` imported non-extractable with `['decrypt']` usage only. A decryption challenge would therefore be satisfiable by any unlocked tab, and so by script injected into one, which would defeat the guard for the attacker it most needs to stop. Signing requires re-importing the private key with `['sign']` usage from raw PKCS#8 bytes, which are obtainable only by decrypting the envelope with a freshly entered master password. + +The client MUST derive the signing key at the moment the master password is entered and MUST discard it immediately after signing. It MUST NOT retain a signing-capable key for the duration of the session, because doing so would grant injected script the capability this requirement exists to withhold. + +#### Scenario: An unlocked session cannot produce a proof by itself + +@e2e exclude The in-memory CryptoKey and its usage flags cannot be inspected via Playwright DOM. Covered by unit tests of the client crypto module asserting the session key is imported with `['decrypt']` only. +- **GIVEN** a vault unlocked in the browser, with the session `CryptoKey` in memory +- **WHEN** a key proof is required and the master password has not been re-entered +- **THEN** the client MUST be unable to produce a signature from the session key +- **AND** MUST prompt for the master password + +#### Scenario: The signing key does not outlive the proof + +@e2e exclude JavaScript memory lifetime is not observable via Playwright DOM. Covered by unit tests asserting the derived key is not returned, stored, or retained after signing. +- **GIVEN** the user has entered their master password to authorise a guarded operation +- **WHEN** the signature has been produced +- **THEN** the client MUST discard the derived AES key and the signing key +- **AND** MUST NOT place either in `localStorage`, `sessionStorage`, or a store that outlives the operation + +### Requirement: A Proof Is Bound To The Operation It Authorises + +A key proof MUST commit to the parameters of the operation it authorises, so that a captured proof cannot be replayed onto a different operation. + +The `#[VaultKeyProofRequired]` attribute MUST declare which request parameters the proof binds to, and the signed payload MUST be the challenge followed by the digest of each declared parameter, hashed individually in the declared order. The attribute MUST also declare which suite's public key verifies the proof: by default the caller's active suite, or a suite named by a route parameter. + +Binding MUST NOT be expressed as a digest over the whole request body. The framework decodes a JSON body and discards the raw bytes, so a whole-body digest would require re-reading the input stream outside the request abstraction, and would additionally require client and server to agree on a canonical serialisation. + +A challenge MUST additionally be bound to a single purpose, so that a proof obtained for one guarded operation cannot be presented to another. + +#### Scenario: A proof does not transfer to a different operation + +@e2e exclude Server-side signature verification against a bound payload; not DOM-observable. Covered by PHPUnit on the middleware. +- **GIVEN** a valid proof issued and signed for one guarded operation +- **WHEN** it is presented to a different guarded operation +- **THEN** the system MUST refuse it + +#### Scenario: A proof does not transfer to different parameters + +@e2e exclude As above. Covered by PHPUnit on the middleware. +- **GIVEN** a valid proof bound to a set of request parameters +- **WHEN** the same proof is presented with any bound parameter altered +- **THEN** the system MUST refuse it + +### Requirement: Challenges Are Stateless And Expiring + +The system MUST issue key-proof challenges through an endpoint that requires only an authenticated session, and MUST NOT require server-side storage to verify them. + +A challenge MUST carry a random component and MUST be authenticated with the instance secret over that component, the caller, the purpose, and an expiry. The system MUST reject an expired or unauthenticated challenge. + +The system MUST NOT depend on a distributed cache to hold challenge state. Nextcloud returns a null cache when none is configured, and a challenge store that silently forgets would make the guarded flows unusable on a default installation. + +Single-use enforcement is NOT required, because a proof is bound to its operation's parameters and therefore replays only ever re-authorise the byte-identical operation. + +#### Scenario: An expired challenge is refused + +@e2e exclude Time-dependent server-side verification; not DOM-observable. Covered by PHPUnit with an injected time factory. +- **GIVEN** a challenge whose expiry has passed +- **WHEN** a proof over it is presented +- **THEN** the system MUST refuse the request with `error: key_proof_required` + +#### Scenario: A forged challenge is refused + +@e2e exclude Server-side HMAC verification; not DOM-observable. Covered by PHPUnit. +- **GIVEN** a challenge not issued by this instance, or altered after issue +- **WHEN** a proof over it is presented +- **THEN** the system MUST refuse the request + +### Requirement: Guard Coverage Is Enforced By Test + +Because a declarative guard fails open when it is omitted, the system MUST carry a test that enumerates every operation required to be guarded and asserts, by reflection, that each carries `#[VaultKeyProofRequired]` with the expected binding and subject. + +Adding a route that can render vault contents or key material permanently unreadable without adding it to that enumeration MUST be treated as a defect in this requirement, not as an accepted gap. + +#### Scenario: A guarded route that loses its attribute fails the build + +@e2e exclude Attribute reflection over controller methods; the middleware itself needs a running instance to produce a 403, which is out of scope for an isolated PHPUnit run — the same rationale documented for `RateLimitAttributesTest`. +- **GIVEN** the enumeration of operations required to carry a key proof +- **WHEN** any enumerated method does not carry `#[VaultKeyProofRequired]`, or carries it with an unexpected binding or subject +- **THEN** the test suite MUST fail diff --git a/openspec/changes/harden-vault-key-material-guards/tasks.md b/openspec/changes/harden-vault-key-material-guards/tasks.md new file mode 100644 index 000000000..b8ea301a8 --- /dev/null +++ b/openspec/changes/harden-vault-key-material-guards/tasks.md @@ -0,0 +1,75 @@ +## 0. Read First — Ordering Constraint + +The guard is a breaking change to four routes the shipped frontend already calls. Sections 1–3 are inert (nothing opts in yet). **Section 5 must not land before section 4**, or the frontend breaks against its own backend. + +No database migration: `SuiteMigration::$status` is a plain `string` column (`lib/Db/SuiteMigration.php:115`), so the new `aborted` value needs no schema change and no `` bump — gate-110 does not apply to this change. If that assumption changes, revisit before merging. + +Section 3 (abort) is independently useful and can be split into its own PR if the whole change grows too large for one review — it has no dependency on sections 1, 2, 4 or 5. + +## 1. Backend — The Guard Primitive + +- [ ] 1.1 Create `lib/Attribute/VaultKeyProofRequired.php`: `#[Attribute(Attribute::TARGET_METHOD)]`, constructor `array $binds = []`, `string $subject = 'active'`; SPDX header per `contribute/HowToApplyALicense.md` +- [ ] 1.2 Create `lib/Service/VaultKeyProofService.php` with `issueChallenge(string $userId, string $purpose): array` returning `{nonce, expiresAt}` — nonce is `base64(ISecureRandom bytes) . '.' . HMAC(instance secret, random|uid|purpose|exp)`; no storage +- [ ] 1.3 Implement `VaultKeyProofService::verify(string $nonce, string $signature, string $publicKeyPem, string $userId, string $purpose, array $boundValues): void` — validate the HMAC, validate the expiry, rebuild the payload as `nonce || sha256(v1) || … || sha256(vn)` in declared order, verify with `openssl_verify` against the stored public key; throw a typed exception on every failure path with no distinction leaked to the caller +- [ ] 1.4 Do NOT use `ICacheFactory` for challenge state (design D5 — a null cache on a default install would make the guarded flows unusable). Assert this in review +- [ ] 1.5 Create `lib/Middleware/VaultKeyProofMiddleware.php` following `JwtAuthMiddleware`: `beforeController` reads the attribute via `new ReflectionMethod($controller, $methodName)`, resolves the subject suite (`'active'` → the session user's active suite via `EncryptionSuiteService::getActiveSuite`; `'routeParam:'` → `IRequest::getParam`), collects the bound values via `IRequest::getParam`, reads `X-Keepiq-Key-Proof`, and delegates to the service +- [ ] 1.6 Implement `afterException` returning `403` with `['error' => 'key_proof_required', 'message' => …]`; re-throw anything that is not the guard's own exception, as `JwtAuthMiddleware` does +- [ ] 1.7 Middleware MUST NOT consult `IUserSession` backends, token scopes or `IPasswordConfirmationBackend` — no SSO/app-password carve-out (spec: *the guard is not waived*). Add an explanatory comment citing the NC `PasswordConfirmationMiddleware` bypasses this deliberately does not copy +- [ ] 1.8 Register in `lib/AppInfo/PlatformIntegrationRegistrar.php` alongside `JwtAuthMiddleware::class` +- [ ] 1.9 Run phpcs/phpstan/phpmd — watch `CouplingBetweenObjects` on the middleware; keep crypto in the service, which is also what makes it unit-testable + +## 2. Backend — Challenge Endpoint + +- [ ] 2.1 Add `proofChallenge(string $id)` to `EncryptionSuiteController` (`#[NoAdminRequired]`), returning `{nonce, expiresAt}` for the calling user and the requested purpose; validate suite ownership +- [ ] 2.2 Accept the purpose as a request parameter constrained to a known set (one per guarded operation); reject an unknown purpose +- [ ] 2.3 Register `['name' => 'encryptionSuite#proofChallenge', 'url' => '/api/v1/suites/{id}/proof-challenge', 'verb' => 'GET']` in `appinfo/routes.php`, before the SPA catch-all wildcard +- [ ] 2.4 The challenge endpoint itself MUST NOT carry `#[VaultKeyProofRequired]` — assert in the coverage test that it is on the deliberate-exclusion list + +## 3. Backend — Abort (independently mergeable) + +- [ ] 3.1 Add `MigrationService::abortMigration(string $migrationId): array` — refuse unless `in_progress`; refuse when any record has been committed to the new suite, reporting the count and pointing at resume; idempotent by status like `completeMigration` +- [ ] 3.2 On success: set status `aborted`, leave the old suite `active` and its records untouched, revoke the successor suite via `EncryptionSuiteService::revokeSuite`, clear failure accounting via `workService->clearFailureAccounting`, release the write lock +- [ ] 3.3 Create `SuiteMigrationAbortedEvent` and a listener that unlocks the SecretRequests locked by `SuiteMigrationStartedListener` +- [ ] 3.4 **Must not** dispatch `SuiteMigrationCompletedEvent` — that is what `EmergencyAccessSuiteRotationListener` consumes to invalidate recovery envelopes. Add a regression test asserting envelopes survive an abort +- [ ] 3.5 Add `MigrationController::abort(string $id)` (`#[NoAdminRequired]`) with the existing `requireOwnMigration` ownership check; **no** `#[VaultKeyProofRequired]` (design D6 — abort is restorative) +- [ ] 3.6 Register `['name' => 'migration#abort', 'url' => '/api/v1/migrations/{id}/abort', 'verb' => 'POST']` +- [ ] 3.7 Update the refusal text in `EncryptionSuiteController::compromiseRecovery()` so the promised "abort" now names a route that exists +- [ ] 3.8 Add an abort control to `src/components/MigrationResumeBanner.vue`, shown only while abort is still available, with copy stating that the old suite stays intact + +## 4. Frontend — Producing the Proof + +- [ ] 4.1 Add `proveMasterPassword(encryptedPrivateKey, masterPassword, nonce, boundValues)` to `src/crypto/reauth.js`: decrypt the envelope via `decryptPrivateKey` (`src/crypto/aes.js:79`), re-import the PKCS#8 bytes with `['sign']` usage, sign `nonce || sha256(v1) || …`, return the signature +- [ ] 4.2 Discard the derived AES key, the raw PKCS#8 bytes and the signing key immediately after signing; never return, store or cache them (spec: *the signing key does not outlive the proof*). Keep `verifyMasterPassword` as-is for the three existing advisory call sites — they are out of scope for this change +- [ ] 4.3 Confirm the signing key is imported with `['sign']` only and is NOT the session `CryptoKey`; add a unit test asserting the session key (`src/crypto/rsa.js:61-66`) remains non-extractable and `['decrypt']`-only +- [ ] 4.4 Add a shared client helper that fetches a challenge, prompts for the master password, produces the proof, and sets the `X-Keepiq-Key-Proof` header — so the four call sites do not each re-implement it +- [ ] 4.5 Wire `src/components/CompromiseRecoveryForm.vue` (recovery start, and the completion call) through the helper +- [ ] 4.6 Wire the routine master-password change flow through the helper; verify the old private key is materialised at that point (design "Risks" — if it is not, stop and raise before proceeding) +- [ ] 4.7 Wire the emergency-contact delete action in `src/views/EmergencyAccessView.vue` / `src/store/modules/emergencyAccess.js` through the helper +- [ ] 4.8 Handle `403 key_proof_required` as "re-enter your master password and retry", not as a terminal error + +## 5. Apply The Guard (must not precede section 4) + +- [ ] 5.1 `EncryptionSuiteController::compromiseRecovery` → `#[VaultKeyProofRequired(binds: ['publicKey', 'encryptedPrivateKey'])]` +- [ ] 5.2 `EncryptionSuiteController::updatePrivateKey` → `#[VaultKeyProofRequired(binds: ['encryptedPrivateKey'], subject: 'routeParam:id')]` +- [ ] 5.3 `MigrationController::complete` → `#[VaultKeyProofRequired]` (defence in depth; the acknowledgement stays — they answer different questions) +- [ ] 5.4 `EmergencyAccessController::destroy` → `#[VaultKeyProofRequired]` +- [ ] 5.5 Create `tests/Unit/Controller/VaultKeyProofAttributesTest.php` in the shape of `RateLimitAttributesTest`: a provider enumerating the four methods with their expected `binds` and `subject`, asserting each by reflection; plus a deliberate-exclusion list (abort, proof-challenge) with the reason recorded per entry + +## 6. Tests + +- [ ] 6.1 `tests/Unit/Service/VaultKeyProofServiceTest.php`: valid proof passes; wrong key fails; altered bound value fails; altered nonce fails; expired nonce fails (injected `ITimeFactory`); wrong purpose fails; proof for one parameter set rejected against another +- [ ] 6.2 `tests/Unit/Middleware/VaultKeyProofMiddlewareTest.php`: attribute absent → pass-through; attribute present without header → 403 `key_proof_required`; `subject: 'active'` and `'routeParam:id'` both resolve; `afterException` re-throws foreign exceptions +- [ ] 6.3 Cross-implementation round-trip: sign with WebCrypto in a JS test, verify with `openssl_verify` in PHPUnit (config rule: *test cross-implementation encryption round-trips*) +- [ ] 6.4 `MigrationServiceTest`: abort on an untouched migration; abort refused after a commit with the count reported; abort idempotent by status; emergency-access envelopes unchanged after abort; completed event NOT dispatched +- [ ] 6.5 `EncryptionSuiteControllerTest` / `MigrationControllerTest` / `EmergencyAccessControllerTest`: the four guarded routes refuse without a proof and proceed with one +- [ ] 6.6 Regression test for the bypass in proposal finding 2: committing ciphertext for every record and completing MUST now be refused at the recovery entry point without a proof +- [ ] 6.7 Frontend unit tests for `proveMasterPassword` (signature verifies against the suite public key; keys discarded) and for the 403 retry path + +## 7. Gates and Documentation + +- [ ] 7.1 Run the hydra gates locally: route-auth (two new routes), no-admin-idor, gate-16 spec-coverage, gate-113 exclusion-evidence (every `@e2e exclude` in this change carries a reason). Note the known pre-existing `no-admin-idor` debt on `development` is not introduced here +- [ ] 7.2 Confirm gate-110 does not apply (no migration added) — if a migration is introduced after all, bump `appinfo/info.xml` `` from `0.3.1` +- [ ] 7.3 Document the guard in `docs/ARCHITECTURE.md`: the attribute contract, the sign-not-decrypt rationale (design D2), and the rule that a new destructive route must be added to `VaultKeyProofAttributesTest` +- [ ] 7.4 Every commit carries `Assisted-by: ClaudeCode:claude-opus-5`. Do NOT add `Signed-off-by` — only the human contributor certifies the DCO +- [ ] 7.5 The PR description discloses AI tool use, in the contributor's own words, and links issue #395 +- [ ] 7.6 Before opening: re-read #395's "Verification status" — the chain was never executed end to end. Reproduce the lockout on a throwaway account against pre-fix code, then confirm the same steps are refused post-fix. This is the issue's own first task and it is still outstanding From 455fb5405ab46306b1988b6f9ec7a933c20c89ed Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 10 Sep 2026 13:08:37 +0200 Subject: [PATCH 02/48] =?UTF-8?q?docs(encryption-suites):=20spec=20?= =?UTF-8?q?=E2=80=94=20migrate=20emergency=20access=20on=20rotation=20(#67?= =?UTF-8?q?4)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenSpec change migrate-emergency-access-on-rotation: proposal, design, tasks, and spec deltas. A compromise-recovery rotation re-envelopes each reachable emergency contact under the new key (buildRecoveryEnvelope with the new private key + the grantee's current certificate) and invalidates only the residual, correcting the spec's claim that the owner cannot re-wrap it alone. Spec only — implementation lands separately. Refs #674 Assisted-by: ClaudeCode:claude-opus-5 --- .../.openspec.yaml | 2 + .../design.md | 60 +++++ .../plan.json | 236 ++++++++++++++++++ .../proposal.md | 39 +++ .../specs/emergency-access/spec.md | 33 +++ .../specs/encryption-suites/spec.md | 72 ++++++ .../tasks.md | 49 ++++ 7 files changed, 491 insertions(+) create mode 100644 openspec/changes/migrate-emergency-access-on-rotation/.openspec.yaml create mode 100644 openspec/changes/migrate-emergency-access-on-rotation/design.md create mode 100644 openspec/changes/migrate-emergency-access-on-rotation/plan.json create mode 100644 openspec/changes/migrate-emergency-access-on-rotation/proposal.md create mode 100644 openspec/changes/migrate-emergency-access-on-rotation/specs/emergency-access/spec.md create mode 100644 openspec/changes/migrate-emergency-access-on-rotation/specs/encryption-suites/spec.md create mode 100644 openspec/changes/migrate-emergency-access-on-rotation/tasks.md diff --git a/openspec/changes/migrate-emergency-access-on-rotation/.openspec.yaml b/openspec/changes/migrate-emergency-access-on-rotation/.openspec.yaml new file mode 100644 index 000000000..e8cda9e50 --- /dev/null +++ b/openspec/changes/migrate-emergency-access-on-rotation/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-10 diff --git a/openspec/changes/migrate-emergency-access-on-rotation/design.md b/openspec/changes/migrate-emergency-access-on-rotation/design.md new file mode 100644 index 000000000..58943b043 --- /dev/null +++ b/openspec/changes/migrate-emergency-access-on-rotation/design.md @@ -0,0 +1,60 @@ +# Design — migrate-emergency-access-on-rotation + +## Context + +Six stores bind ciphertext to an EncryptionSuite. Five are migrated in the browser during compromise recovery: each record's ciphertext is decrypted with the old private key and re-encrypted to the new one, committed one row per request, and the completion gate refuses until nothing remains on `old_suite_id`. The sixth — `keepiq_emergency_contacts` — is the exception: its recovery envelope is not migrated but *invalidated* by `EmergencyAccessSuiteRotationListener` when `SuiteMigrationCompletedEvent` fires. + +The stated reason is that the owner "cannot re-wrap it alone". That is true of the *old* envelope, whose plaintext is the old private key sealed to the grantee's certificate — opening it needs the grantee's key. It is false of the operation actually wanted: minting a *new* envelope. `buildRecoveryEnvelope(privateKeyPem, granteeCertificatePem)` needs only the grantor's private key and the grantee's public certificate, and during a rotation the owner has both — the new private key is generated locally at `initiateCompromiseRecovery` and the grantee certificate is fetchable. Emergency contacts can therefore migrate like the other stores; the only structural difference is that the job *builds* a value from the new key rather than *transforming* an existing ciphertext. + +## Goals / Non-Goals + +**Goals** +- A compromise-recovery rotation preserves emergency access for every contact whose grantee is still reachable +- The residual — contacts whose grantee cannot be reached — is invalidated as today, but surfaced so the owner can re-designate, instead of lost silently +- No schema change, no new trust assumption + +**Non-Goals** +- Touching the routine master-password-change flow. It keeps the same key pair, so the escrowed private key stays valid and envelopes keep opening; there is nothing to migrate and the invalidation listener does not fire for it +- Migrating a contact to a grantee who has no active suite. The envelope has nowhere to be sealed; that contact is residual by definition +- Preserving the *grantee* side of emergency access when the grantee rotates. That is governed by `invalidateForGranteeRevocation` and is out of scope + +## Decisions + +### D1: Re-envelope in the migration loop, mirroring attachment-grant re-wrap + +Emergency contacts join the migration work list. For each contact still bound to the old suite, the browser fetches the grantee's current certificate, calls `buildRecoveryEnvelope(newPrivateKeyPem, granteeCert)`, and commits the fresh envelope to a migration endpoint that sets `recovery_envelope` and re-points `grantor_suite_id` to the new suite, leaving `state = granted`. + +This reuses the loop, the per-record commit shape, and the server-side owner/suite scoping the other stores already have. The job differs only in its producer: attachment grants decrypt the old wrapped key and re-wrap it; emergency contacts ignore the old envelope entirely and build a new one from the new private key. Both end at "a row that used to point at the old suite now points at the new one, with material only the owner could have produced". + +### D2: Best-effort migrate, listener sweeps the residual — no completion-gate change + +The five migrated stores gate completion: the run cannot finalise while any of their rows remains on the old suite. Emergency contacts are deliberately **not** added to that gate. + +The reason is that a contact can be legitimately un-migratable — the grantee left the instance, or revoked their suite, so there is no certificate to seal to. Gating completion on such a row would trap the vault exactly the way the *A Migration Always Has A Way To Terminate* requirement forbids. So emergency contacts stay outside the gate: the loop migrates every reachable one, and `invalidateForGrantorRotation()` runs at completion as it does today — but now finds only the residual, because the migrated contacts already left the old suite. The listener keeps its current code; its meaning narrows from "invalidate all" to "invalidate whatever the loop could not carry". + +This is the least invasive correct design: no new column, no `migration_error` analogue for contacts, no change to the gate or its progress denominator. The trade-off is that a re-envelope that fails transiently (grantee cert briefly unfetchable) is swept into the residual rather than retried to exhaustion — acceptable, because the residual is re-designatable and the failure mode is "prompt to re-establish", never data loss. + +### D3: The completion summary carries the residual, and the form acts on it + +Today the loss is silent. With this change the completion response reports which contacts were invalidated rather than migrated, and `CompromiseRecoveryForm.vue` prompts the owner to re-designate exactly those. A rotation with all grantees reachable prompts nothing; a rotation with an unreachable grantee explains which one and why. + +### D4: Bind to the grantee's current certificate, and let that be a feature + +The new envelope seals to whatever certificate `getGranteeCertificate()` returns now, which may differ from the one the old envelope used if the grantee has since rotated. This is correct: an envelope sealed to a grantee's stale key would be unopenable by that grantee anyway. Re-enveloping on the grantor's rotation therefore also repairs staleness introduced by the grantee's own rotation, for free. + +## Risks / Trade-offs + +- **A grantee reachable at migration time but not later.** No worse than today: the envelope is valid when built, and any later grantee-side change is handled by the existing grantee-revocation invalidation. Not this change's concern +- **Transient cert-fetch failure demotes a contact to residual.** The owner is prompted to re-designate one contact they did not need to; a nuisance, not a loss. If it proves common, D2 could gain a bounded retry without changing the model +- **Two rotations in quick succession.** The first migrates the envelope to suite B; the second (B→C) re-reads contacts bound to B and migrates again. The work list is derived from `grantor_suite_id`, so this composes without special handling +- **The listener now means something narrower than its name.** `invalidateForGrantorRotation` will mostly invalidate nothing. Worth a comment at the call site so a future reader does not "fix" the apparent no-op + +## Migration Plan + +No data migration. Existing contacts keep working; the first rotation after this ships migrates their envelopes instead of dropping them. A rotation already in progress when this deploys completes under the old behaviour (invalidate) — acceptable, and the owner is prompted to re-designate, which is the pre-change status quo. + +## Open Questions + +- **Gate or sweep?** D2 chooses sweep (no completion-gate change). The alternative — make emergency contacts a gated store with an explicit "invalidate this one" acknowledgement, like the per-record failure path for secrets — is more uniform but needs a contact-level accounting field and touches the gate. Recommendation: ship the sweep; revisit only if the residual needs auditing beyond a re-designation prompt +- **Where does the client read the contacts to process?** DECIDED: the filtered read — the client reads the existing emergency-access index and selects `grantorSuiteId === oldSuiteId`, rather than widening `getWork`. This keeps `getWork`'s `totalRemaining` and the completion gate entirely untouched, which matters because emergency contacts are deliberately not gated (D2). Extending `getWork` was the uniform-looking alternative but would have put a non-gating list inside the endpoint whose whole output feeds the gate. +- **Attachments-style verification?** DECIDED: a shape check, not a round-trip. The other stores verify by decrypting what they just wrote, but an emergency envelope can only be opened by the grantee, so the grantor cannot round-trip it. The server therefore asserts the envelope parses, carries the expected `v`/`alg`, and declares a `granteeSuiteId` matching the grantee's current active suite. This catches a malformed or misaddressed envelope; it cannot catch a well-formed envelope sealed to the wrong plaintext, which is inherent to the trust model and no worse than initial designation, which has the same limit. diff --git a/openspec/changes/migrate-emergency-access-on-rotation/plan.json b/openspec/changes/migrate-emergency-access-on-rotation/plan.json new file mode 100644 index 000000000..2b1de5a60 --- /dev/null +++ b/openspec/changes/migrate-emergency-access-on-rotation/plan.json @@ -0,0 +1,236 @@ +{ + "change": "migrate-emergency-access-on-rotation", + "project": "keepiq", + "repo": "ConductionNL/keepiq", + "base_branch": "development", + "feature_branch": "feature/674/migrate-emergency-access-on-rotation", + "created": "2026-09-10", + "tracking_issue": 674, + "tasks": [ + { + "id": 1, + "num": "1.1", + "title": "Add a read the client can use to enumerate the rotating owner's emergency contacts still bound to the old suite: reuse `EmergencyContactMapper::findByGrantorSuite($oldSuiteId)` filtered to the migration owner, returning `id`, `granteeUserId`, and `state` (exclude already-`invalidated`). Prefer the existing emergency-access index over widening `getWork`, so the completion gate and its progress denominator are untouched", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 2, + "num": "1.2", + "title": "Add a migration re-point endpoint (e.g. `POST /api/v1/migrations/{id}/emergency-contacts/{contactId}`) accepting a fresh `recoveryEnvelope`; it MUST set `recovery_envelope`, set `grantor_suite_id` to the migration's new suite, keep `state = granted`, and clear any `invalidated_reason`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 3, + "num": "1.3", + "title": "Enforce scoping identically to the other migration writes: refuse unless the contact's current `grantor_suite_id` is the migration's `old_suite_id` and the contact's grantor is the migration owner (resolve the acting user via `OCP\\IUserSession`)", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 4, + "num": "1.4", + "title": "Validate the submitted envelope's shape server-side as far as is possible without the grantee's key: it MUST parse, carry the expected `v`/`alg`, and its declared `granteeSuiteId` MUST match the grantee's current active suite (a shape check, not a round-trip \u2014 only the grantee can open it)", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 5, + "num": "1.5", + "title": "Register the route in `appinfo/routes.php` before the SPA catch-all wildcard", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 6, + "num": "1.6", + "title": "Add a comment at the `invalidateForGrantorRotation()` call site noting it is now a **residual sweep**: after the loop it finds only contacts the migration could not carry (grantee unreachable). Do not \"optimise away\" the apparent no-op", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 7, + "num": "2.1", + "title": "In `initiateCompromiseRecovery` (`src/store/modules/encryptionSuite.js`), after the new key pair is generated and before/within the migration loop, fetch the owner's emergency contacts on the old suite (1.1)", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 8, + "num": "2.2", + "title": "For each contact: fetch the grantee's current certificate via `getGranteeCertificate(granteeUserId)`; on success call `buildRecoveryEnvelope(newPrivateKeyPem, granteeCert)` and POST it to the re-point endpoint (1.2). `newPrivateKeyPem` is already materialised in this function \u2014 reuse it, do not re-derive", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 9, + "num": "2.3", + "title": "On a grantee with no active certificate (fetch throws / returns none), do NOT commit: leave the contact bound to the old suite so the completion sweep invalidates it, and collect it into a `residualContacts` list", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 10, + "num": "2.4", + "title": "Treat a transient re-point failure as residual for this run (the contact is re-designatable); do not halt the migration on it \u2014 emergency contacts are outside the completion gate", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 11, + "num": "2.5", + "title": "The raw new private key PEM MUST stay in the existing rotation scope and MUST NOT be persisted or logged; only envelope ciphertext crosses the wire (ADR-003)", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 12, + "num": "3.1", + "title": "Include `residualContacts` (grantee display names) in the migration outcome returned by `initiateCompromiseRecovery`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 13, + "num": "3.2", + "title": "In `CompromiseRecoveryForm.vue`, on completion, prompt the owner to re-establish exactly the residual contacts; show nothing about emergency access when every contact migrated", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 14, + "num": "3.3", + "title": "Use `@conduction/nextcloud-vue` components and the NL Design System double-fallback CSS pattern, consistent with the rest of the form", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 15, + "num": "4.1", + "title": "Unit test the re-point endpoint: re-points `grantor_suite_id` to the new suite, keeps `state = granted`, clears `invalidated_reason`; refuses when the contact is on a different suite or owned by another user; rejects a malformed envelope and a `granteeSuiteId` that does not match the grantee's current suite", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 16, + "num": "4.2", + "title": "Unit test the residual sweep: after the loop, `invalidateForGrantorRotation(oldSuiteId)` invalidates only contacts still on the old suite; a migrated contact (now on the new suite) is untouched", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 17, + "num": "4.3", + "title": "Frontend unit test: a reachable grantee yields a `buildRecoveryEnvelope(newPrivateKeyPem, cert)` call and a commit; an unreachable grantee yields no commit and a residual entry", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 18, + "num": "4.4", + "title": "Cross-implementation sanity: an envelope built in JS parses under the server's shape check (config rule: test cross-implementation round-trips as far as the trust model allows)", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 19, + "num": "4.5", + "title": "Regression: a rotation with all grantees reachable prompts no re-designation and leaves no contact invalidated (the behaviour this change fixes)", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 20, + "num": "4.6", + "title": "Two-rotations-in-succession: a contact migrated A\u2192B is then migrated B\u2192C, found each time via `grantor_suite_id`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 21, + "num": "5.1", + "title": "Run the hydra gates locally: route-auth (one new route), no-admin-idor (the re-point endpoint is owner-scoped by construction), gate-16 spec-coverage, gate-113 exclusion-evidence (every `@e2e exclude` carries a reason)", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 22, + "num": "5.2", + "title": "Confirm gate-110 does not apply (no migration). If a schema change is introduced after all, bump `appinfo/info.xml` `` from `0.3.1`", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 23, + "num": "5.3", + "title": "Update `docs/ARCHITECTURE.md` where it describes suite migration: emergency contacts are a migrated store, and `invalidateForGrantorRotation` is a residual sweep", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 24, + "num": "5.4", + "title": "Every commit carries `Assisted-by: ClaudeCode:claude-opus-5`; no `Signed-off-by` (only the human certifies the DCO)", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 25, + "num": "5.5", + "title": "PR description discloses AI tool use in the contributor's own words and links the `harden-vault-key-material-guards` change whose open question this resolves", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + } + ] +} diff --git a/openspec/changes/migrate-emergency-access-on-rotation/proposal.md b/openspec/changes/migrate-emergency-access-on-rotation/proposal.md new file mode 100644 index 000000000..cd84f5e0c --- /dev/null +++ b/openspec/changes/migrate-emergency-access-on-rotation/proposal.md @@ -0,0 +1,39 @@ +## Why + +A compromise-recovery rotation silently destroys the user's emergency-access recovery. `EmergencyAccessSuiteRotationListener` fires on `SuiteMigrationCompletedEvent` and calls `invalidateForGrantorRotation()`, which clears the recovery envelope of every emergency contact bound to the old suite. So the one pre-arranged break-glass path a careful user set up is gone after a routine key change, and `CompromiseRecoveryForm.vue` never tells them to re-establish it (verified — the form has no emergency-access copy). This was raised as an open question in the `harden-vault-key-material-guards` change and is the natural fix for it. + +The `encryption-suites` spec presents this destruction as unavoidable. The *Migration Covers Every Suite-Bound Store* requirement says of `keepiq_emergency_contacts`: + +> "the rotating owner cannot re-wrap it alone; the grantor MUST be prompted to re-establish emergency access." + +**That justification is wrong**, and the code proves it. The recovery envelope is built by `buildRecoveryEnvelope(privateKeyPem, granteeCertificatePem)` (`src/crypto/emergencyEnvelope.js`). Re-wrapping the *old* envelope would indeed need the grantee's key — but nobody needs to re-wrap the old one. During a rotation the owner mints a *fresh* envelope escrowing the **new** private key, and both inputs are already in hand: + +- `newPrivateKeyPem` — generated in the browser at the top of `initiateCompromiseRecovery` (`src/store/modules/encryptionSuite.js:186`), the same value that seals every other store in the migration; +- the grantee's current certificate — fetchable via `getGranteeCertificate()`, exactly as initial designation fetches it. + +This is byte-for-byte the operation designation already performs, and structurally identical to the attachment-grant disposition one row up in the same table ("Re-wrap the rotating owner's own grants under the new suite"). Emergency contacts are simply one more suite-bound store that can **migrate** rather than being invalidated. + +Scope is compromise recovery only. A routine master-password change keeps the same RSA key pair and only re-wraps the AES envelope, so the escrowed private key is unchanged and existing recovery envelopes still open — routine change neither invalidates nor needs to migrate them, and the invalidation listener does not fire for it. + +## What Changes + +- Migrate emergency-access recovery envelopes as part of compromise-recovery migration: for each of the rotating owner's emergency contacts still bound to the old suite whose grantee has a usable certificate, the browser builds a fresh recovery envelope escrowing the **new** private key, wrapped to the grantee's current certificate, and re-points the contact to the new suite — leaving its `granted` state intact +- Correct the *Migration Covers Every Suite-Bound Store* disposition for `keepiq_emergency_contacts` from "Invalidate, unchanged" to "Re-envelope under the new key where the grantee is reachable; invalidate only the residual" +- Keep `invalidateForGrantorRotation()` as a **fallback sweep**: after migration, it now finds only the contacts that could not be re-enveloped (grantee has no active suite / left the instance), which are exactly the ones that genuinely must be invalidated +- Surface the residual: where any contact was invalidated rather than migrated, prompt the owner to re-designate that specific contact — replacing today's silent, total loss with a targeted, explained one +- Do **not** change the routine master-password-change flow, which does not rotate the key pair + +## Capabilities + +### Modified Capabilities +- `encryption-suites`: the *Migration Covers Every Suite-Bound Store* requirement gains emergency contacts as a migrated store rather than an invalidated one, with a defined residual disposition +- `emergency-access`: *Envelope Invalidation on Key Change* changes from "rotation invalidates every envelope" to "rotation re-envelopes under the new key where possible and invalidates only the residual" + +## Impact + +- **Database**: none. Re-enveloping reuses the existing `recovery_envelope` and `grantor_suite_id` columns of `keepiq_emergency_contacts`; no schema change, no migration, no `` bump +- **Backend**: `getWork` (or a sibling read) exposes the owner's emergency contacts still bound to the old suite, with the `granteeUserId` needed to fetch the certificate; a migration commit endpoint accepts a fresh envelope and re-points `grantor_suite_id` to the new suite while keeping `state = granted`; `EmergencyAccessSuiteRotationListener` is unchanged in code but now runs as a residual sweep. Owner/suite scoping enforced server-side exactly as the other migration writes are +- **Frontend**: `initiateCompromiseRecovery` builds a new envelope per reachable contact using `buildRecoveryEnvelope(newPrivateKeyPem, granteeCert)` and commits it in the migration loop; the completion summary lists any residual contacts to re-designate; `CompromiseRecoveryForm.vue` renders that prompt +- **Security**: unchanged trust model. The new envelope escrows the new private key and is wrapped to the grantee's public certificate; the raw private key exists only transiently in the browser, and only ciphertext crosses the wire (ADR-003). Binding to the grantee's *current* certificate is strictly more correct than the old envelope, which may have escrowed a key the grantee has since rotated away from +- **Cross-app**: none +- **Dependency note**: composes with `harden-vault-key-material-guards` but does not require it. That change gates *destructive* operations; this one makes a *legitimate* rotation preserve emergency access. Landing this resolves that change's third open question diff --git a/openspec/changes/migrate-emergency-access-on-rotation/specs/emergency-access/spec.md b/openspec/changes/migrate-emergency-access-on-rotation/specs/emergency-access/spec.md new file mode 100644 index 000000000..9446fb866 --- /dev/null +++ b/openspec/changes/migrate-emergency-access-on-rotation/specs/emergency-access/spec.md @@ -0,0 +1,33 @@ +## MODIFIED Requirements + +### Requirement: Envelope Invalidation on Key Change +Because the recovery envelope escrows the grantor's private key as of designation, a change to that key MUST be reflected in the envelopes bound to it. + +When the grantor's EncryptionSuite is rotated (compromise recovery), the system MUST migrate each affected recovery envelope where the grantee is reachable: it MUST build a fresh envelope escrowing the grantor's **new** private key, sealed to the grantee's current certificate, and re-point the contact to the new suite while preserving its `granted` state. A contact whose grantee has no active certificate to seal to (the grantee left the instance or revoked their suite) cannot be migrated; the system MUST invalidate that residual contact and MUST prompt the grantor to re-establish it. The grantor MUST NOT be required to open the old envelope to do any of this — building a new envelope needs only the new private key, which the grantor holds during rotation, and the grantee's public certificate. + +Migrating rather than invalidating is possible because the recovery envelope is rebuilt, not re-wrapped: `buildRecoveryEnvelope` takes the grantor's private key and the grantee's public certificate, both of which the grantor has mid-rotation. Sealing to the grantee's *current* certificate is also more correct than preserving the old envelope, which may escrow a key the grantee has since rotated away from. + +When the grantor's EncryptionSuite is revoked, existing recovery envelopes MUST be cleared. Revocation is not a key rotation and produces no new key to migrate to; this is unchanged. Likewise, if a grantee's EncryptionSuite is revoked, envelopes encrypted to that grantee MUST be invalidated; this is unchanged. + +#### Scenario: Suite rotation migrates a reachable contact +@e2e exclude Server-side re-point plus client-side envelope construction; verifying the migrated envelope opens requires the grantee's key in a second browser context. Covered by PHPUnit on the re-point endpoint and unit tests of the envelope builder. +- **GIVEN** A has an emergency contact B whose EncryptionSuite is active +- **AND** a recovery envelope escrowing A's current private key +- **WHEN** A performs compromise recovery and rotates their EncryptionSuite +- **THEN** the system MUST build a fresh recovery envelope escrowing A's new private key, sealed to B's current certificate +- **AND** re-point the contact to A's new suite with its state still `granted` +- **AND** MUST NOT prompt A to re-establish B + +#### Scenario: Suite rotation invalidates only the unreachable residual +@e2e exclude Server-side listener sweep after the migration loop; covered by PHPUnit (contacts remaining on the old suite are invalidated) and the completion-summary assertion. +- **GIVEN** A has emergency contacts B (active suite) and C (no active suite) +- **WHEN** A performs compromise recovery and rotates their EncryptionSuite +- **THEN** B MUST be migrated to the new suite +- **AND** C MUST be invalidated +- **AND** A MUST be prompted to re-establish C specifically + +#### Scenario: Suite revocation clears envelopes +@e2e exclude Server-side suite rotation/revocation listener contract — covered by PHPUnit (invalidateForGrantorRotation/clearForGrantorRevocation/invalidateForGranteeRevocation + invalidated audit). Live UI run deferred (worktree not deployed). +- **GIVEN** A has one or more emergency contacts with recovery envelopes +- **WHEN** A's EncryptionSuite is revoked +- **THEN** the recovery envelopes MUST be cleared diff --git a/openspec/changes/migrate-emergency-access-on-rotation/specs/encryption-suites/spec.md b/openspec/changes/migrate-emergency-access-on-rotation/specs/encryption-suites/spec.md new file mode 100644 index 000000000..48bbc6b76 --- /dev/null +++ b/openspec/changes/migrate-emergency-access-on-rotation/specs/encryption-suites/spec.md @@ -0,0 +1,72 @@ +## MODIFIED Requirements + +### Requirement: Migration Covers Every Suite-Bound Store + +The Suite Migration requirement speaks of migrating "all secrets". Because a user's ciphertext is bound to an EncryptionSuite in six separate stores, a migration that walks `keepiq_secrets` alone silently strands the other five. The system MUST therefore treat compromise-recovery migration as complete only when every suite-bound store has been given its disposition. Outstanding work MUST be derivable server-side from the data itself — rows still bound to `old_suite_id` — rather than from a client-reported count, so that a resumed migration knows what remains without trusting the browser. + +The disposition of each store is fixed as follows. All fields listed as re-encrypted are stored as RSA ciphertext; plaintext columns (`name`, `url`, `folder_id`, `requested_fields`) are organisational metadata and MUST NOT be touched. + +| Store | Suite-bound content | Disposition | +|-------|---------------------|-------------| +| `keepiq_secrets` | `key`, `login`, `additional_fields` | Re-encrypt under the new suite; re-point `encryption_suite_id` | +| `keepiq_secret_versions` | `key`, `login`, `additional_fields` (own `encryption_suite_id`) | Re-encrypt the bounded window fixed by the `secret-version-history` spec (head plus the N most recent versions, default 5); drop older versions | +| `keepiq_attachment_grants` | `wrapped_file_key` (RSA-wrapped per-file AES key) | Re-wrap the rotating owner's own grants under the new suite. Grants belonging to other recipients MUST NOT be altered | +| `keepiq_secret_requests` | No ciphertext of its own; `encryption_suite_id` selects the certificate used to encrypt future submissions | Lock for the duration of the migration, then unlock and re-point to the new suite | +| `keepiq_link_shares` | `encrypted_secret_snapshot` | Revoke (cascade), unchanged from current behaviour | +| `keepiq_emergency_contacts` | `recovery_envelope` | Re-envelope under the new key where the grantee is reachable, then invalidate only the residual. For each contact still bound to the old suite whose grantee has an active certificate, the browser builds a fresh envelope escrowing the **new** private key sealed to that certificate and re-points `grantor_suite_id` to the new suite, keeping `state = granted`. A contact whose grantee has no active suite is invalidated and the grantor is prompted to re-establish it. This is not a re-wrap of the old envelope — `buildRecoveryEnvelope` needs only the new private key (held during rotation) and the grantee's public certificate (see the `emergency-access` spec) | + +Re-encryption of `keepiq_secrets`, `keepiq_secret_versions` and `keepiq_attachment_grants` MUST happen in the browser under the same rules as ordinary migration: the old private key decrypts and the new public key encrypts, both as WebCrypto `CryptoKey` objects, and only ciphertext crosses the wire. Emergency contacts are the one migrated store not produced by decrypt-then-re-encrypt: the browser builds a fresh recovery envelope from the new private key and the grantee's fetched certificate, so no old-key decrypt is involved. Unlike the three re-encrypted stores, emergency contacts MUST NOT gate completion — a contact whose grantee is unreachable can never be re-enveloped, and gating on it would make the write lock inescapable; such contacts are swept into invalidation at completion instead. RSA has a per-chunk plaintext cap (446 bytes at RSA-4096), so every value MUST be re-chunked against the new key rather than having its existing chunk framing reused. + +Owner and suite scoping MUST be enforced server-side on every re-encryption write, resolving the acting user through the Nextcloud `OCP\IUserSession` the surrounding controllers already use: a write MUST be refused unless the target row's current `encryption_suite_id` is the migration's `old_suite_id` and the row is owned by the migration's owner. + +#### Scenario: Attachment grants survive the rotation + +@e2e exclude Attachment-grant re-wrapping is verified by unwrapping the file key with the new private key — a WebCrypto/DB assertion with no DOM surface; covered by unit tests of the migration driver and PHPUnit on the re-point endpoint. +- **GIVEN** a user owns a secret with an encrypted attachment, and their own attachment grant holds the file key wrapped under their old suite +- **WHEN** compromise recovery migration completes +- **THEN** the owner's grant MUST hold the same file key re-wrapped under the new suite and the shared ciphertext blob MUST NOT be re-uploaded or duplicated +- **AND** grants held by other recipients of that attachment MUST be unchanged + +#### Scenario: Version history migrates within its bounded window + +@e2e exclude Version-history migration is asserted on stored ciphertext and row counts; the version list UI shows only counts, so the migration itself is not DOM-observable. Covered by PHPUnit and migration-driver unit tests. +- **GIVEN** a secret with a head and 12 prior versions, and a migration window of 5 +- **WHEN** compromise recovery migration completes +- **THEN** the head and the 5 most recent versions MUST be re-encrypted under the new suite and re-pointed +- **AND** the 7 older versions MUST be deleted +- **AND** the user MUST be told that older version history was dropped + +#### Scenario: Secret requests are locked and re-pointed, not stranded + +@e2e exclude The lock/re-point transition is server-side request state; the fill-in page's "temporarily unavailable" surface belongs to the secret-requests spec. Covered by PHPUnit on the request lifecycle. +- **GIVEN** a user has pending SecretRequests when they declare their master password compromised +- **WHEN** the migration starts +- **THEN** those requests MUST be set to `locked` and the fill-in link MUST report the request as temporarily unavailable +- **WHEN** the migration terminates +- **THEN** those requests MUST be unlocked and their `encryption_suite_id` MUST be the new suite + +#### Scenario: A store left unprocessed blocks completion + +@e2e exclude Outstanding-work detection is a server-side query with no DOM representation beyond the aggregate progress indicator; covered by PHPUnit on the completion endpoint. +- **GIVEN** a migration in which the attachment-grant pass has not yet run, so grants remain bound to `old_suite_id` +- **WHEN** the client requests completion of the migration +- **THEN** the server MUST refuse to mark the migration terminal +- **AND** the migration MUST remain `in_progress` with the write lock held + +#### Scenario: A reachable emergency contact is re-enveloped, not invalidated + +@e2e exclude Client builds the envelope and the server re-points the row; verifying the envelope opens needs the grantee's key in a second context. Covered by PHPUnit on the re-point endpoint and unit tests of the envelope builder. +- **GIVEN** a rotating owner with an emergency contact whose grantee has an active suite +- **WHEN** the migration processes emergency contacts +- **THEN** a fresh recovery envelope escrowing the new private key MUST be built and the contact re-pointed to the new suite with `state = granted` +- **AND** the contact MUST NOT be invalidated +- **AND** the completion MUST NOT gate on that contact + +#### Scenario: An unreachable emergency contact does not trap the vault + +@e2e exclude Server-side listener sweep after the loop; covered by PHPUnit asserting the residual is invalidated and completion still terminates. +- **GIVEN** a rotating owner with an emergency contact whose grantee has no active suite +- **WHEN** the migration processes emergency contacts and then completes +- **THEN** that contact MUST be invalidated by the completion sweep +- **AND** completion MUST NOT be blocked by it +- **AND** the owner MUST be prompted to re-establish that specific contact diff --git a/openspec/changes/migrate-emergency-access-on-rotation/tasks.md b/openspec/changes/migrate-emergency-access-on-rotation/tasks.md new file mode 100644 index 000000000..ec8b1ccbf --- /dev/null +++ b/openspec/changes/migrate-emergency-access-on-rotation/tasks.md @@ -0,0 +1,49 @@ +## 0. Read First — Scope and Ordering + +Scope is **compromise-recovery rotation only**. The routine master-password change keeps the same RSA key pair, so escrowed private keys stay valid and this change does not touch that flow (`changePassword` / `updatePrivateKey`). + +No database migration: re-enveloping reuses the existing `recovery_envelope` and `grantor_suite_id` columns of `keepiq_emergency_contacts`. No schema change, no `` bump — gate-110 does not apply. If that assumption changes, revisit. + +Composes with `harden-vault-key-material-guards` but does not depend on it. Landing this resolves that change's third open question (rotation silently costing emergency access). + +Design fork still open (see design.md): the client may read the contacts to migrate either from an extended `getWork` or from the existing emergency-access index filtered by `grantorSuiteId`. Tasks below assume the **filtered read** (smaller blast radius on the completion gate); if `getWork` is chosen instead, 1.1 and 2.2 move accordingly. + +## 1. Backend — Re-point Endpoint and Read + +- [ ] 1.1 Add a read the client can use to enumerate the rotating owner's emergency contacts still bound to the old suite: reuse `EmergencyContactMapper::findByGrantorSuite($oldSuiteId)` filtered to the migration owner, returning `id`, `granteeUserId`, and `state` (exclude already-`invalidated`). Prefer the existing emergency-access index over widening `getWork`, so the completion gate and its progress denominator are untouched +- [ ] 1.2 Add a migration re-point endpoint (e.g. `POST /api/v1/migrations/{id}/emergency-contacts/{contactId}`) accepting a fresh `recoveryEnvelope`; it MUST set `recovery_envelope`, set `grantor_suite_id` to the migration's new suite, keep `state = granted`, and clear any `invalidated_reason` +- [ ] 1.3 Enforce scoping identically to the other migration writes: refuse unless the contact's current `grantor_suite_id` is the migration's `old_suite_id` and the contact's grantor is the migration owner (resolve the acting user via `OCP\IUserSession`) +- [ ] 1.4 Validate the submitted envelope's shape server-side as far as is possible without the grantee's key: it MUST parse, carry the expected `v`/`alg`, and its declared `granteeSuiteId` MUST match the grantee's current active suite (a shape check, not a round-trip — only the grantee can open it) +- [ ] 1.5 Register the route in `appinfo/routes.php` before the SPA catch-all wildcard +- [ ] 1.6 Add a comment at the `invalidateForGrantorRotation()` call site noting it is now a **residual sweep**: after the loop it finds only contacts the migration could not carry (grantee unreachable). Do not "optimise away" the apparent no-op + +## 2. Frontend — Build and Commit the New Envelopes + +- [ ] 2.1 In `initiateCompromiseRecovery` (`src/store/modules/encryptionSuite.js`), after the new key pair is generated and before/within the migration loop, fetch the owner's emergency contacts on the old suite (1.1) +- [ ] 2.2 For each contact: fetch the grantee's current certificate via `getGranteeCertificate(granteeUserId)`; on success call `buildRecoveryEnvelope(newPrivateKeyPem, granteeCert)` and POST it to the re-point endpoint (1.2). `newPrivateKeyPem` is already materialised in this function — reuse it, do not re-derive +- [ ] 2.3 On a grantee with no active certificate (fetch throws / returns none), do NOT commit: leave the contact bound to the old suite so the completion sweep invalidates it, and collect it into a `residualContacts` list +- [ ] 2.4 Treat a transient re-point failure as residual for this run (the contact is re-designatable); do not halt the migration on it — emergency contacts are outside the completion gate +- [ ] 2.5 The raw new private key PEM MUST stay in the existing rotation scope and MUST NOT be persisted or logged; only envelope ciphertext crosses the wire (ADR-003) + +## 3. Frontend — Surface the Residual + +- [ ] 3.1 Include `residualContacts` (grantee display names) in the migration outcome returned by `initiateCompromiseRecovery` +- [ ] 3.2 In `CompromiseRecoveryForm.vue`, on completion, prompt the owner to re-establish exactly the residual contacts; show nothing about emergency access when every contact migrated +- [ ] 3.3 Use `@conduction/nextcloud-vue` components and the NL Design System double-fallback CSS pattern, consistent with the rest of the form + +## 4. Tests + +- [ ] 4.1 Unit test the re-point endpoint: re-points `grantor_suite_id` to the new suite, keeps `state = granted`, clears `invalidated_reason`; refuses when the contact is on a different suite or owned by another user; rejects a malformed envelope and a `granteeSuiteId` that does not match the grantee's current suite +- [ ] 4.2 Unit test the residual sweep: after the loop, `invalidateForGrantorRotation(oldSuiteId)` invalidates only contacts still on the old suite; a migrated contact (now on the new suite) is untouched +- [ ] 4.3 Frontend unit test: a reachable grantee yields a `buildRecoveryEnvelope(newPrivateKeyPem, cert)` call and a commit; an unreachable grantee yields no commit and a residual entry +- [ ] 4.4 Cross-implementation sanity: an envelope built in JS parses under the server's shape check (config rule: test cross-implementation round-trips as far as the trust model allows) +- [ ] 4.5 Regression: a rotation with all grantees reachable prompts no re-designation and leaves no contact invalidated (the behaviour this change fixes) +- [ ] 4.6 Two-rotations-in-succession: a contact migrated A→B is then migrated B→C, found each time via `grantor_suite_id` + +## 5. Gates and Documentation + +- [ ] 5.1 Run the hydra gates locally: route-auth (one new route), no-admin-idor (the re-point endpoint is owner-scoped by construction), gate-16 spec-coverage, gate-113 exclusion-evidence (every `@e2e exclude` carries a reason) +- [ ] 5.2 Confirm gate-110 does not apply (no migration). If a schema change is introduced after all, bump `appinfo/info.xml` `` from `0.3.1` +- [ ] 5.3 Update `docs/ARCHITECTURE.md` where it describes suite migration: emergency contacts are a migrated store, and `invalidateForGrantorRotation` is a residual sweep +- [ ] 5.4 Every commit carries `Assisted-by: ClaudeCode:claude-opus-5`; no `Signed-off-by` (only the human certifies the DCO) +- [ ] 5.5 PR description discloses AI tool use in the contributor's own words and links the `harden-vault-key-material-guards` change whose open question this resolves From ea787dc3f1a86cb82bec64d60229a337e2bf1ad9 Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 10 Sep 2026 13:42:31 +0200 Subject: [PATCH 03/48] docs(encryption-suites): reconcile 673 open questions with #674 The "any completed rotation silently costs emergency access" open question is resolved by #674 (migrate-emergency-access-on-rotation), which re-envelopes reachable contacts under the new key. The lost-password route's destructive mechanics (the revocation warning and the refuse-while-a-usable-contact-exists gate) are likewise folded into #674; this note records where each piece now lives. Refs #673 Assisted-by: ClaudeCode:claude-opus-5 --- openspec/changes/harden-vault-key-material-guards/design.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openspec/changes/harden-vault-key-material-guards/design.md b/openspec/changes/harden-vault-key-material-guards/design.md index c04d92363..45d2e3ca1 100644 --- a/openspec/changes/harden-vault-key-material-guards/design.md +++ b/openspec/changes/harden-vault-key-material-guards/design.md @@ -158,5 +158,5 @@ Steps 3 and 4 must land together, or in that order, or the frontend breaks again ## Open Questions - Should `complete()` keep the `acceptUnrecoverable` acknowledgement now that a key proof is required? It no longer carries the security weight (finding 2), but it is still the mechanism that makes losing a record a decision the owner made rather than a side-effect. Recommendation: keep both; they answer different questions -- The lost-password route (administrator revocation with the "this deletes emergency access" warning, and refusing to revoke while a usable emergency contact exists) is specced in #395 but deliberately out of scope here. It should be the immediate follow-up, because this change makes it the only remaining route for a forgotten password -- `#395` also observes that **any** completed rotation costs the user their emergency access, since `invalidateForGrantorRotation()` fires on `SuiteMigrationCompletedEvent`. Confirmed: `CompromiseRecoveryForm.vue` never mentions emergency access, so users silently lose their break-glass path after a routine key change. Not fixed here — worth its own change alongside the lost-password route +- The lost-password route (administrator revocation as the only way back to a working vault once this guard blocks a forgotten-password rotation) is out of scope here, and is now partly in place around it: a plain create after revocation already works via #392, and the destruction warning plus the refuse-while-a-usable-emergency-contact-exists enforcement have been folded into #674 (`migrate-emergency-access-on-rotation`). What remains genuinely open is only whether any further UI is needed to walk a forgotten-password user through revoke -> recreate; the destructive mechanics are covered +- ~~`#395` also observes that **any** completed rotation costs the user their emergency access, since `invalidateForGrantorRotation()` fires on `SuiteMigrationCompletedEvent`.~~ **RESOLVED by #674** (`migrate-emergency-access-on-rotation`): rotation now re-envelopes each reachable emergency contact under the new key instead of dropping it, and only a contact whose grantee is unreachable is invalidated — with the owner prompted to re-designate that one specifically. The silent break-glass loss after a routine key change is gone From 8965d7b961eead1557f555fd90142b5382267ebe Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 10 Sep 2026 13:57:54 +0200 Subject: [PATCH 04/48] docs(emergency-access): fold the destructive-revocation warning into #674 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends this change to carry #395's lost-password-route safeguard: revoking a user suite still clears its emergency envelopes, but must now warn plainly (secrets gone, emergency access deleted, accessor must retrieve first while the suite is active), refuse while a usable emergency contact exists unless an explicit override is given, and surface the count of usable contacts (never identities). Belongs here because the guard in #673 makes revocation the only forgotten-password route, and the clearing is emergency-access lifecycle on a suite key-state transition — the surface this change owns. Adds spec scenarios (refuse-without-override, proceed-with-override), a design decision D5, a tasks section 4b, and proposal/impact notes. Refs #674 Assisted-by: ClaudeCode:claude-opus-5 --- .../design.md | 6 +++ .../plan.json | 46 +++++++++++++++++-- .../proposal.md | 4 ++ .../specs/emergency-access/spec.md | 23 +++++++++- .../tasks.md | 9 +++- 5 files changed, 81 insertions(+), 7 deletions(-) diff --git a/openspec/changes/migrate-emergency-access-on-rotation/design.md b/openspec/changes/migrate-emergency-access-on-rotation/design.md index 58943b043..8972d9b07 100644 --- a/openspec/changes/migrate-emergency-access-on-rotation/design.md +++ b/openspec/changes/migrate-emergency-access-on-rotation/design.md @@ -42,6 +42,12 @@ Today the loss is silent. With this change the completion response reports which The new envelope seals to whatever certificate `getGranteeCertificate()` returns now, which may differ from the one the old envelope used if the grantee has since rotated. This is correct: an envelope sealed to a grantee's stale key would be unopenable by that grantee anyway. Re-enveloping on the grantor's rotation therefore also repairs staleness introduced by the grantee's own rotation, for free. +### D5: Revocation still clears emergency access — but never silently + +Rotation migrates emergency access (D1); revocation cannot, because it produces no new key to seal to. So revocation keeps clearing the envelopes — but clearing is destructive and irreversible, and revocation is the last-resort route for an owner who lost their master password, i.e. the owner most likely to still need their contact. The safeguard makes the clear a knowing choice: warn plainly, refuse while a usable contact exists unless an explicit override is given, and surface the *count* of usable contacts (never identities — those stay grantor-private) so the administrator can decide. The retrieve-first ordering (accessor pulls the secrets while the suite is still `active`) is the whole point, and it is enforceable rather than merely documented. + +This is folded in here rather than in `harden-vault-key-material-guards` because it is emergency-access-lifecycle behaviour on a suite key-state transition — the same surface D1 already touches — and because the guard change is what makes revocation the only forgotten-password route, so the safeguard is its natural companion. + ## Risks / Trade-offs - **A grantee reachable at migration time but not later.** No worse than today: the envelope is valid when built, and any later grantee-side change is handled by the existing grantee-revocation invalidation. Not this change's concern diff --git a/openspec/changes/migrate-emergency-access-on-rotation/plan.json b/openspec/changes/migrate-emergency-access-on-rotation/plan.json index 2b1de5a60..2fe92ef71 100644 --- a/openspec/changes/migrate-emergency-access-on-rotation/plan.json +++ b/openspec/changes/migrate-emergency-access-on-rotation/plan.json @@ -189,8 +189,8 @@ }, { "id": 21, - "num": "5.1", - "title": "Run the hydra gates locally: route-auth (one new route), no-admin-idor (the re-point endpoint is owner-scoped by construction), gate-16 spec-coverage, gate-113 exclusion-evidence (every `@e2e exclude` carries a reason)", + "num": "4b.1", + "title": "On the user-suite revoke path, before clearing, count the owner's usable (non-invalidated) emergency contacts via `EmergencyContactMapper::findByGrantorSuite` / grantor lookup; refuse the revocation when the count is > 0 and no override is supplied, returning that count (never identities)", "status": "pending", "spec_ref": null, "acceptance_criteria": [], @@ -198,6 +198,42 @@ }, { "id": 22, + "num": "4b.2", + "title": "Add an explicit `override`/`acceptEmergencyAccessLoss` parameter to the revoke endpoint; with it, revocation proceeds and `clearForGrantorRevocation` runs as today", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 23, + "num": "4b.3", + "title": "Surface the destruction warning in the revoke UI: secrets permanently unreadable + vault rebuilt from scratch; emergency access deleted; if an accessor exists they MUST retrieve secrets first while the suite is still `active`. Use `@conduction/nextcloud-vue` + NL Design System double-fallback CSS", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 24, + "num": "4b.4", + "title": "Tests: revoke refused with the usable-contact count when a contact exists and no override; revoke proceeds and clears with the override; count is returned without identities; no-contact case revokes unchanged", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 25, + "num": "5.1", + "title": "Run the hydra gates locally: route-auth (the re-point route, plus the revoke override param), no-admin-idor (the re-point endpoint is owner-scoped by construction), gate-16 spec-coverage, gate-113 exclusion-evidence (every `@e2e exclude` carries a reason)", + "status": "pending", + "spec_ref": null, + "acceptance_criteria": [], + "files_likely_affected": [] + }, + { + "id": 26, "num": "5.2", "title": "Confirm gate-110 does not apply (no migration). If a schema change is introduced after all, bump `appinfo/info.xml` `` from `0.3.1`", "status": "pending", @@ -206,7 +242,7 @@ "files_likely_affected": [] }, { - "id": 23, + "id": 27, "num": "5.3", "title": "Update `docs/ARCHITECTURE.md` where it describes suite migration: emergency contacts are a migrated store, and `invalidateForGrantorRotation` is a residual sweep", "status": "pending", @@ -215,7 +251,7 @@ "files_likely_affected": [] }, { - "id": 24, + "id": 28, "num": "5.4", "title": "Every commit carries `Assisted-by: ClaudeCode:claude-opus-5`; no `Signed-off-by` (only the human certifies the DCO)", "status": "pending", @@ -224,7 +260,7 @@ "files_likely_affected": [] }, { - "id": 25, + "id": 29, "num": "5.5", "title": "PR description discloses AI tool use in the contributor's own words and links the `harden-vault-key-material-guards` change whose open question this resolves", "status": "pending", diff --git a/openspec/changes/migrate-emergency-access-on-rotation/proposal.md b/openspec/changes/migrate-emergency-access-on-rotation/proposal.md index cd84f5e0c..7c780c2d2 100644 --- a/openspec/changes/migrate-emergency-access-on-rotation/proposal.md +++ b/openspec/changes/migrate-emergency-access-on-rotation/proposal.md @@ -15,12 +15,15 @@ This is byte-for-byte the operation designation already performs, and structural Scope is compromise recovery only. A routine master-password change keeps the same RSA key pair and only re-wraps the AES envelope, so the escrowed private key is unchanged and existing recovery envelopes still open — routine change neither invalidates nor needs to migrate them, and the invalidation listener does not fire for it. +This change also carries the destructive-revocation safeguard from #395's lost-password route. It belongs here rather than in the guard change (`harden-vault-key-material-guards`): once that guard blocks a forgotten-password rotation, administrator revocation becomes the only way back to a working vault, and revocation *deletes* emergency access — so the warning and the ordering gate are emergency-access-lifecycle behaviour, adjacent to the rotation-migration this change already owns. + ## What Changes - Migrate emergency-access recovery envelopes as part of compromise-recovery migration: for each of the rotating owner's emergency contacts still bound to the old suite whose grantee has a usable certificate, the browser builds a fresh recovery envelope escrowing the **new** private key, wrapped to the grantee's current certificate, and re-points the contact to the new suite — leaving its `granted` state intact - Correct the *Migration Covers Every Suite-Bound Store* disposition for `keepiq_emergency_contacts` from "Invalidate, unchanged" to "Re-envelope under the new key where the grantee is reachable; invalidate only the residual" - Keep `invalidateForGrantorRotation()` as a **fallback sweep**: after migration, it now finds only the contacts that could not be re-enveloped (grantee has no active suite / left the instance), which are exactly the ones that genuinely must be invalidated - Surface the residual: where any contact was invalidated rather than migrated, prompt the owner to re-designate that specific contact — replacing today's silent, total loss with a targeted, explained one +- Fold in the destructive-revocation safeguard for the lost-password route: revoking a user suite still clears its emergency envelopes, but the system now MUST warn plainly (secrets gone, emergency access **deleted**, accessor must retrieve first while the suite is active), MUST refuse while a usable emergency contact exists unless an explicit override is given, and MUST surface the count of usable contacts (never identities) so the administrator can choose. Today `clearForGrantorRevocation` deletes them silently - Do **not** change the routine master-password-change flow, which does not rotate the key pair ## Capabilities @@ -35,5 +38,6 @@ Scope is compromise recovery only. A routine master-password change keeps the sa - **Backend**: `getWork` (or a sibling read) exposes the owner's emergency contacts still bound to the old suite, with the `granteeUserId` needed to fetch the certificate; a migration commit endpoint accepts a fresh envelope and re-points `grantor_suite_id` to the new suite while keeping `state = granted`; `EmergencyAccessSuiteRotationListener` is unchanged in code but now runs as a residual sweep. Owner/suite scoping enforced server-side exactly as the other migration writes are - **Frontend**: `initiateCompromiseRecovery` builds a new envelope per reachable contact using `buildRecoveryEnvelope(newPrivateKeyPem, granteeCert)` and commits it in the migration loop; the completion summary lists any residual contacts to re-designate; `CompromiseRecoveryForm.vue` renders that prompt - **Security**: unchanged trust model. The new envelope escrows the new private key and is wrapped to the grantee's public certificate; the raw private key exists only transiently in the browser, and only ciphertext crosses the wire (ADR-003). Binding to the grantee's *current* certificate is strictly more correct than the old envelope, which may have escrowed a key the grantee has since rotated away from +- **Revocation path**: the user-suite revoke flow gains the usable-contact check and the override parameter; the warning copy lives in the settings dialog. `clearForGrantorRevocation` is unchanged in effect (still clears on the override path) but no longer reachable silently - **Cross-app**: none - **Dependency note**: composes with `harden-vault-key-material-guards` but does not require it. That change gates *destructive* operations; this one makes a *legitimate* rotation preserve emergency access. Landing this resolves that change's third open question diff --git a/openspec/changes/migrate-emergency-access-on-rotation/specs/emergency-access/spec.md b/openspec/changes/migrate-emergency-access-on-rotation/specs/emergency-access/spec.md index 9446fb866..862a9623f 100644 --- a/openspec/changes/migrate-emergency-access-on-rotation/specs/emergency-access/spec.md +++ b/openspec/changes/migrate-emergency-access-on-rotation/specs/emergency-access/spec.md @@ -7,7 +7,13 @@ When the grantor's EncryptionSuite is rotated (compromise recovery), the system Migrating rather than invalidating is possible because the recovery envelope is rebuilt, not re-wrapped: `buildRecoveryEnvelope` takes the grantor's private key and the grantee's public certificate, both of which the grantor has mid-rotation. Sealing to the grantee's *current* certificate is also more correct than preserving the old envelope, which may escrow a key the grantee has since rotated away from. -When the grantor's EncryptionSuite is revoked, existing recovery envelopes MUST be cleared. Revocation is not a key rotation and produces no new key to migrate to; this is unchanged. Likewise, if a grantee's EncryptionSuite is revoked, envelopes encrypted to that grantee MUST be invalidated; this is unchanged. +When the grantor's EncryptionSuite is revoked, existing recovery envelopes MUST be cleared. Revocation is not a key rotation and produces no new key to migrate to, so unlike rotation there is nothing to migrate the envelope to. But clearing is destructive and irreversible — `clearForGrantorRevocation` deletes the rows outright — and revocation of a user suite is the last-resort route for an owner who has lost their master password, exactly the owner most likely to still need their emergency contact. The system MUST therefore treat this clearing as a decision the acting administrator makes knowingly, not a silent side effect: + +- Before revoking a user suite that has a usable (non-invalidated) emergency contact, the system MUST warn plainly that every secret becomes permanently unreadable and the vault is rebuilt from scratch, that the designated emergency access is **deleted** along with it, and that if an emergency accessor exists they MUST retrieve the old secrets first, while the old suite is still `active`. +- The system MUST refuse the revocation while a usable emergency contact exists, unless the caller supplies an explicit override. The refusal MUST surface the **count** of usable contacts so the administrator can choose — never their identities, which stay grantor-private. Today the deletion is silent and the count is not surfaced; that is the gap this closes. +- With the override, revocation proceeds and clears the envelopes as before. The ordering is enforceable, not merely documented. + +Likewise, if a grantee's EncryptionSuite is revoked, envelopes encrypted to that grantee MUST be invalidated; this is unchanged. #### Scenario: Suite rotation migrates a reachable contact @e2e exclude Server-side re-point plus client-side envelope construction; verifying the migrated envelope opens requires the grantee's key in a second browser context. Covered by PHPUnit on the re-point endpoint and unit tests of the envelope builder. @@ -26,6 +32,21 @@ When the grantor's EncryptionSuite is revoked, existing recovery envelopes MUST - **AND** C MUST be invalidated - **AND** A MUST be prompted to re-establish C specifically +#### Scenario: Revocation refuses while a usable emergency contact exists +@e2e exclude Server-side guard on the revoke path; covered by PHPUnit asserting revocation is refused and the usable-contact count is returned. Live UI run deferred. +- **GIVEN** A has a usable (non-invalidated) emergency contact +- **WHEN** an administrator revokes A's suite without an override +- **THEN** the system MUST refuse and MUST report the count of usable emergency contacts +- **AND** MUST NOT clear any recovery envelope or change the suite status +- **AND** MUST NOT disclose the contact's identity + +#### Scenario: Revocation proceeds with an explicit override and warns +@e2e exclude Server-side guard plus the destructive clear; covered by PHPUnit on the revoke path with the override flag. The warning copy is asserted in the settings-dialog component test. +- **GIVEN** A has a usable emergency contact and the administrator has been shown the destruction warning +- **WHEN** the administrator revokes A's suite with the explicit override +- **THEN** the suite MUST be revoked and the recovery envelopes cleared +- **AND** the warning MUST have stated that emergency access is deleted and that an accessor must retrieve secrets first while the suite is still active + #### Scenario: Suite revocation clears envelopes @e2e exclude Server-side suite rotation/revocation listener contract — covered by PHPUnit (invalidateForGrantorRotation/clearForGrantorRevocation/invalidateForGranteeRevocation + invalidated audit). Live UI run deferred (worktree not deployed). - **GIVEN** A has one or more emergency contacts with recovery envelopes diff --git a/openspec/changes/migrate-emergency-access-on-rotation/tasks.md b/openspec/changes/migrate-emergency-access-on-rotation/tasks.md index ec8b1ccbf..5692632bd 100644 --- a/openspec/changes/migrate-emergency-access-on-rotation/tasks.md +++ b/openspec/changes/migrate-emergency-access-on-rotation/tasks.md @@ -40,9 +40,16 @@ Design fork still open (see design.md): the client may read the contacts to migr - [ ] 4.5 Regression: a rotation with all grantees reachable prompts no re-designation and leaves no contact invalidated (the behaviour this change fixes) - [ ] 4.6 Two-rotations-in-succession: a contact migrated A→B is then migrated B→C, found each time via `grantor_suite_id` +## 4b. Destructive-Revocation Safeguard (lost-password route) + +- [ ] 4b.1 On the user-suite revoke path, before clearing, count the owner's usable (non-invalidated) emergency contacts via `EmergencyContactMapper::findByGrantorSuite` / grantor lookup; refuse the revocation when the count is > 0 and no override is supplied, returning that count (never identities) +- [ ] 4b.2 Add an explicit `override`/`acceptEmergencyAccessLoss` parameter to the revoke endpoint; with it, revocation proceeds and `clearForGrantorRevocation` runs as today +- [ ] 4b.3 Surface the destruction warning in the revoke UI: secrets permanently unreadable + vault rebuilt from scratch; emergency access deleted; if an accessor exists they MUST retrieve secrets first while the suite is still `active`. Use `@conduction/nextcloud-vue` + NL Design System double-fallback CSS +- [ ] 4b.4 Tests: revoke refused with the usable-contact count when a contact exists and no override; revoke proceeds and clears with the override; count is returned without identities; no-contact case revokes unchanged + ## 5. Gates and Documentation -- [ ] 5.1 Run the hydra gates locally: route-auth (one new route), no-admin-idor (the re-point endpoint is owner-scoped by construction), gate-16 spec-coverage, gate-113 exclusion-evidence (every `@e2e exclude` carries a reason) +- [ ] 5.1 Run the hydra gates locally: route-auth (the re-point route, plus the revoke override param), no-admin-idor (the re-point endpoint is owner-scoped by construction), gate-16 spec-coverage, gate-113 exclusion-evidence (every `@e2e exclude` carries a reason) - [ ] 5.2 Confirm gate-110 does not apply (no migration). If a schema change is introduced after all, bump `appinfo/info.xml` `` from `0.3.1` - [ ] 5.3 Update `docs/ARCHITECTURE.md` where it describes suite migration: emergency contacts are a migrated store, and `invalidateForGrantorRotation` is a residual sweep - [ ] 5.4 Every commit carries `Assisted-by: ClaudeCode:claude-opus-5`; no `Signed-off-by` (only the human certifies the DCO) From 3bc32e97ea95c3c194ff913617e0cc0c9cd838a2 Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 10 Sep 2026 14:28:21 +0200 Subject: [PATCH 05/48] docs(encryption-suites): abort deletes the successor, not revokes it (#673) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correcting the abort spec discovered during implementation: revoking the unused successor suite would run EncryptionSuiteRevokedListener, which for a user suite sweeps the owner's incoming ShareTargets and promotes their delegations — destroying real state over a migration the abort exists to undo. The successor is brand-new and empty, so it is deleted outright. Refs #673 Assisted-by: ClaudeCode:claude-opus-5 --- .../specs/encryption-suites/spec.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openspec/changes/harden-vault-key-material-guards/specs/encryption-suites/spec.md b/openspec/changes/harden-vault-key-material-guards/specs/encryption-suites/spec.md index b99428ca3..8f59694cf 100644 --- a/openspec/changes/harden-vault-key-material-guards/specs/encryption-suites/spec.md +++ b/openspec/changes/harden-vault-key-material-guards/specs/encryption-suites/spec.md @@ -108,7 +108,7 @@ On abort the system MUST: - set the migration to the terminal status `aborted` - leave the old EncryptionSuite `active`, and leave every record bound to it untouched -- revoke the successor suite, which by definition holds nothing +- discard the successor suite by **deleting** it — created moments ago, it holds no ciphertext and has no shares or emergency contacts, so it is removed outright. It MUST NOT be revoked through the ordinary suite-revocation path: that path treats a revoked *user* suite as a lost identity and cascades a share-target sweep and delegation promotion, which would destroy the owner's incoming shares over a migration the abort exists to undo - release the write lock and unlock the SecretRequests locked when the migration started - clear the migration's failure accounting, so a later migration does not inherit a stale acknowledgement threshold @@ -123,7 +123,7 @@ Abort MUST NOT require a key proof. It is restorative — it returns the vault t - **WHEN** abort is requested by the owner - **THEN** the migration MUST become `aborted` - **AND** the old suite MUST remain `active` with every record still bound to it -- **AND** the successor suite MUST be revoked +- **AND** the successor suite MUST be deleted (not revoked, which would cascade the user-suite revocation side effects) - **AND** the write lock MUST be released and locked SecretRequests MUST be unlocked #### Scenario: Aborting after records have moved is refused From 49ff8c23fcf8bb0cf8b31c1185b6f360bf3becb0 Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 10 Sep 2026 14:29:36 +0200 Subject: [PATCH 06/48] feat(encryption-suites): add the migration abort route (#673) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The abort route the compromiseRecovery refusal already promises but that did not exist — the remedy named in the error message. It is the non-destructive terminal: completion carries the vault forward to the new suite and marks the old one compromised; abort carries it back to the old suite, which stays active and readable. Abort is permitted only while no record has been committed to the new suite (MigrationWorkService::countCommitted). Once a record has moved, both outcomes lose data, so the migration stays in_progress and the caller is pointed at resuming — a 409 carrying the committed count. This restriction is also exactly what makes abort safe against the session-only lockout: producing a valid re-encrypted record needs the master password, so a hostile session that never held it can never have committed one and can always be aborted away. On success: status -> aborted, the successor suite is deleted (not revoked, which would cascade the user-suite lost-identity teardown), failure accounting is cleared, the write lock is released, and SuiteMigrationAbortedEvent fires — NOT SuiteMigrationCompletedEvent, so the terminal cascade (compromise-flagging, link-share revocation, emergency-access invalidation) never runs. Its one listener unlocks the SecretRequests locked at start, keeping them on the old suite. Frontend: an "Abort and keep my old key" control on the resume banner plus the abortMigration store action. Backend + store fully unit-tested (abort restores/deletes; refused-after-commit with count; idempotent; aborted-not- completed event); phpmd clean; prettier clean. Refs #673 Assisted-by: ClaudeCode:claude-opus-5 --- appinfo/routes.php | 1 + lib/AppInfo/SuiteLifecycleEventRegistrar.php | 11 ++ lib/Controller/MigrationController.php | 50 ++++++ lib/Event/SuiteMigrationAbortedEvent.php | 79 ++++++++++ .../MigrationAbortRefusedException.php | 62 ++++++++ .../SuiteMigrationAbortedListener.php | 97 ++++++++++++ lib/Service/MigrationService.php | 115 ++++++++++++++ lib/Service/MigrationWorkService.php | 48 ++++++ .../plan.json | 32 ++-- .../harden-vault-key-material-guards/tasks.md | 20 +-- src/components/MigrationResumeBanner.vue | 51 +++++++ src/store/modules/encryptionSuite.js | 36 +++++ .../Controller/MigrationControllerTest.php | 56 +++++++ tests/Unit/Service/MigrationServiceTest.php | 142 ++++++++++++++++++ tests/store/encryptionSuite.spec.js | 68 +++++++++ 15 files changed, 842 insertions(+), 26 deletions(-) create mode 100644 lib/Event/SuiteMigrationAbortedEvent.php create mode 100644 lib/Exception/MigrationAbortRefusedException.php create mode 100644 lib/Listener/SuiteMigrationAbortedListener.php diff --git a/appinfo/routes.php b/appinfo/routes.php index ad636e242..72e36a4d8 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -49,6 +49,7 @@ // Migration tracking. ['name' => 'migration#getStatus', 'url' => '/api/v1/migrations/status', 'verb' => 'GET'], ['name' => 'migration#complete', 'url' => '/api/v1/migrations/{id}/complete', 'verb' => 'POST'], + ['name' => 'migration#abort', 'url' => '/api/v1/migrations/{id}/abort', 'verb' => 'POST'], // Compromise-recovery migration work loop. One record per request: the // browser decrypts with the old private key, re-encrypts under the new one, diff --git a/lib/AppInfo/SuiteLifecycleEventRegistrar.php b/lib/AppInfo/SuiteLifecycleEventRegistrar.php index a9063c35b..6731b95b4 100644 --- a/lib/AppInfo/SuiteLifecycleEventRegistrar.php +++ b/lib/AppInfo/SuiteLifecycleEventRegistrar.php @@ -24,12 +24,14 @@ namespace OCA\Keepiq\AppInfo; use OCA\Keepiq\Event\EncryptionSuiteRevokedEvent; +use OCA\Keepiq\Event\SuiteMigrationAbortedEvent; use OCA\Keepiq\Event\SuiteMigrationCompletedEvent; use OCA\Keepiq\Event\SuiteMigrationStartedEvent; use OCA\Keepiq\Listener\EmergencyAccessSuiteRevocationListener; use OCA\Keepiq\Listener\EmergencyAccessSuiteRotationListener; use OCA\Keepiq\Listener\EncryptionSuiteRevokedListener; use OCA\Keepiq\Listener\SuiteCompromiseListener; +use OCA\Keepiq\Listener\SuiteMigrationAbortedListener; use OCA\Keepiq\Listener\SuiteMigrationCompletedListener; use OCA\Keepiq\Listener\SuiteMigrationStartedListener; use OCP\AppFramework\Bootstrap\IRegistrationContext; @@ -70,6 +72,15 @@ public function register(IRegistrationContext $context): void { listener: SuiteMigrationCompletedListener::class ); + // Abort: release the SecretRequests locked at start, keeping them on the + // old suite. Deliberately bound ONLY to this listener — none of the + // terminal-cascade listeners above may react to an abort, since nothing + // migrated and the old suite stays active. + $context->registerEventListener( + event: SuiteMigrationAbortedEvent::class, + listener: SuiteMigrationAbortedListener::class + ); + // Implement-user-sharing §8 — sharing-graph reactions to suite // revocation and post-migration possibly-compromised flagging. $context->registerEventListener( diff --git a/lib/Controller/MigrationController.php b/lib/Controller/MigrationController.php index 993ddfdd1..1997b65c7 100644 --- a/lib/Controller/MigrationController.php +++ b/lib/Controller/MigrationController.php @@ -25,6 +25,7 @@ use OCA\Keepiq\AppInfo\Application; use OCA\Keepiq\Db\SuiteMigration; use OCA\Keepiq\Exception\ForbiddenException; +use OCA\Keepiq\Exception\MigrationAbortRefusedException; use OCA\Keepiq\Exception\MigrationIncompleteException; use OCA\Keepiq\Exception\NotFoundException; use OCA\Keepiq\Service\EncryptionSuiteService; @@ -172,6 +173,55 @@ public function complete(string $id, bool $hasErrors = false, ?int $acceptUnreco }//end try }//end complete() + /** + * Abort a migration, returning the vault to the old suite. + * + * The endpoint the `compromiseRecovery` refusal already tells users to use. + * Non-destructive: it discards the unused successor and leaves the old suite + * active. Permitted only while no record has been committed to the new suite; + * once records have moved the server refuses and points at resuming. + * + * @param string $id The migration ID + * + * @NoAdminRequired + * + * @return JSONResponse + * + * @spec openspec/specs/encryption-suites/spec.md#requirement-a-migration-can-be-aborted-before-any-record-moves + */ + #[NoAdminRequired] + public function abort(string $id): JSONResponse { + $user = $this->userSession->getUser(); + if ($user === null) { + return new JSONResponse(data: ['message' => 'Unauthorized'], statusCode: Http::STATUS_UNAUTHORIZED); + } + + try { + $this->requireOwnMigration(migrationId: $id, userId: $user->getUID()); + + $result = $this->migrationService->abortMigration(migrationId: $id); + return new JSONResponse(data: $result); + } catch (ForbiddenException $e) { + return new JSONResponse(data: ['message' => $e->getMessage()], statusCode: Http::STATUS_FORBIDDEN); + } catch (MigrationAbortRefusedException $e) { + // The migration is intact and resumable — a record has already + // moved, so abort would lose data. Distinct from a generic fault so + // the client offers "resume", not "try abort again". + return new JSONResponse( + data: [ + 'error' => 'migration_abort_refused', + 'message' => $e->getMessage(), + 'committed' => $e->getCommitted(), + ], + statusCode: Http::STATUS_CONFLICT + ); + } catch (NotFoundException $e) { + return new JSONResponse(data: ['message' => $e->getMessage()], statusCode: Http::STATUS_NOT_FOUND); + } catch (Exception $e) { + return new JSONResponse(data: ['message' => $e->getMessage()], statusCode: Http::STATUS_BAD_REQUEST); + }//end try + }//end abort() + /** * List the records still bound to the migration's old suite. * diff --git a/lib/Event/SuiteMigrationAbortedEvent.php b/lib/Event/SuiteMigrationAbortedEvent.php new file mode 100644 index 000000000..78dafb163 --- /dev/null +++ b/lib/Event/SuiteMigrationAbortedEvent.php @@ -0,0 +1,79 @@ + + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\Keepiq\Event; + +use OCP\EventDispatcher\Event; + +/** + * Fired when a compromise-recovery migration is aborted with nothing migrated. + */ +class SuiteMigrationAbortedEvent extends Event { + /** + * Constructor. + * + * @param string $oldSuiteId The suite the vault returns to (still active) + * @param string $newSuiteId The discarded successor suite's id + * @param string $migrationId The aborted migration's id + * + * @return void + */ + public function __construct( + private string $oldSuiteId, + private string $newSuiteId, + private string $migrationId, + ) { + parent::__construct(); + }//end __construct() + + /** + * The suite the vault returns to. + * + * @return string + */ + public function getOldSuiteId(): string { + return $this->oldSuiteId; + }//end getOldSuiteId() + + /** + * The discarded successor suite's id. + * + * @return string + */ + public function getNewSuiteId(): string { + return $this->newSuiteId; + }//end getNewSuiteId() + + /** + * The aborted migration's id. + * + * @return string + */ + public function getMigrationId(): string { + return $this->migrationId; + }//end getMigrationId() +}//end class diff --git a/lib/Exception/MigrationAbortRefusedException.php b/lib/Exception/MigrationAbortRefusedException.php new file mode 100644 index 000000000..43eef89f9 --- /dev/null +++ b/lib/Exception/MigrationAbortRefusedException.php @@ -0,0 +1,62 @@ + + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\Keepiq\Exception; + +use RuntimeException; + +/** + * Thrown when a migration has moved records and can no longer be aborted. + */ +class MigrationAbortRefusedException extends RuntimeException { + /** + * How many records have already been committed to the new suite. + * + * @var integer + */ + private int $committed = 0; + + /** + * Record the committed count to surface to the caller. + * + * @param integer $committed The number of records already on the new suite + * + * @return self + */ + public function withCommitted(int $committed): self { + $this->committed = $committed; + return $this; + }//end withCommitted() + + /** + * The number of records already committed to the new suite. + * + * @return integer + */ + public function getCommitted(): int { + return $this->committed; + }//end getCommitted() +}//end class diff --git a/lib/Listener/SuiteMigrationAbortedListener.php b/lib/Listener/SuiteMigrationAbortedListener.php new file mode 100644 index 000000000..4da92b110 --- /dev/null +++ b/lib/Listener/SuiteMigrationAbortedListener.php @@ -0,0 +1,97 @@ + + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\Keepiq\Listener; + +use OCA\Keepiq\Event\SuiteMigrationAbortedEvent; +use OCA\Keepiq\Service\SecretRequestSuiteLockService; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventListener; +use Psr\Log\LoggerInterface; +use Throwable; + +/** + * Unlock SecretRequests, keeping the old suite, when a migration is aborted. + * + * @implements IEventListener + */ +class SuiteMigrationAbortedListener implements IEventListener { + /** + * Constructor. + * + * @param SecretRequestSuiteLockService $secretRequestService The SecretRequest suite-lock service + * @param LoggerInterface $logger The logger + * + * @return void + */ + public function __construct( + private SecretRequestSuiteLockService $secretRequestService, + private LoggerInterface $logger, + ) { + }//end __construct() + + /** + * Handle the event. + * + * @param Event $event The event + * + * @return void + * + * @spec openspec/specs/encryption-suites/spec.md#requirement-a-migration-can-be-aborted-before-any-record-moves + */ + public function handle(Event $event): void { + if (($event instanceof SuiteMigrationAbortedEvent) === false) { + return; + } + + try { + // Unlock the requests locked at start, keeping them on the OLD + // suite: passing the old id as both arguments re-points them to the + // suite they are already on (a no-op update) and flips their status + // back to pending. The new suite is being discarded, so it must not + // become their target. + $unlocked = $this->secretRequestService->unlockAndUpdateSuite( + $event->getOldSuiteId(), + $event->getOldSuiteId() + ); + $this->logger->info( + 'Keepiq: unlocked SecretRequests after migration abort, kept on the old suite', + [ + 'oldSuiteId' => $event->getOldSuiteId(), + 'unlocked' => $unlocked, + ] + ); + } catch (Throwable $e) { + $this->logger->error( + 'Keepiq: SuiteMigrationAbortedListener failed: ' . $e->getMessage(), + ['exception' => $e] + ); + } + }//end handle() +}//end class diff --git a/lib/Service/MigrationService.php b/lib/Service/MigrationService.php index 1005a6e2e..3b5a20cbe 100644 --- a/lib/Service/MigrationService.php +++ b/lib/Service/MigrationService.php @@ -25,8 +25,10 @@ use OCA\Keepiq\Db\EncryptionSuiteMapper; use OCA\Keepiq\Db\SuiteMigration; use OCA\Keepiq\Db\SuiteMigrationMapper; +use OCA\Keepiq\Event\SuiteMigrationAbortedEvent; use OCA\Keepiq\Event\SuiteMigrationCompletedEvent; use OCA\Keepiq\Event\SuiteMigrationStartedEvent; +use OCA\Keepiq\Exception\MigrationAbortRefusedException; use OCA\Keepiq\Exception\MigrationIncompleteException; use OCP\AppFramework\Db\DoesNotExistException; use OCP\EventDispatcher\IEventDispatcher; @@ -222,6 +224,119 @@ public function completeMigration( ); }//end completeMigration() + /** + * Abort a compromise-recovery migration, returning the vault to the old suite. + * + * The abort route the `compromiseRecovery` refusal message already promises. + * It is the non-destructive terminal: completion carries the vault FORWARD to + * the new suite and marks the old one compromised; abort carries it BACK to + * the old suite, which stays `active` and readable. + * + * Abort is permitted ONLY while no record has been committed to the new + * suite. Once a record has moved, the two possible outcomes both lose data — + * discarding the successor strands what has moved, keeping it strands what has + * not — so the migration stays `in_progress` and the caller is pointed at + * resuming. This restriction is also exactly sufficient for the case abort + * exists to remedy: producing a valid re-encrypted record requires the + * plaintext, hence the master password, so a hostile session that never held + * it can never have committed a record and can always be aborted away. + * + * Idempotent by status, like completeMigration: a retried abort on an already + * terminal migration is a no-op, not a second teardown. + * + * @param string $migrationId The migration to abort + * + * @return array The terminal migration plus an `aborted` flag + * + * @throws MigrationAbortRefusedException When a record has already been committed + * + * @spec openspec/specs/encryption-suites/spec.md#requirement-a-migration-can-be-aborted-before-any-record-moves + */ + public function abortMigration(string $migrationId): array { + $migration = $this->mapper->findById($migrationId); + + if ($migration->getStatus() !== 'in_progress') { + $this->logger->info( + 'Keepiq: abortMigration called on an already-terminated migration; ignoring', + ['migrationId' => $migrationId, 'status' => $migration->getStatus()] + ); + + return [ + 'status' => $migration->getStatus(), + 'aborted' => false, + 'alreadyTerminated' => true, + ]; + } + + $ownerId = $this->resolveOwnerId(suiteId: $migration->getOldSuiteId()); + + // The one gate: nothing may have moved to the new suite yet. + $committed = 0; + if ($ownerId !== null) { + $committed = $this->workService->countCommitted(migration: $migration, ownerId: $ownerId); + } + + if ($committed > 0) { + throw (new MigrationAbortRefusedException( + message: sprintf( + '%d record(s) have already been re-encrypted to the new suite, so this ' + . 'migration can no longer be aborted without losing data. Resume it to finish, ' + . 'or complete it.', + $committed + ) + ))->withCommitted($committed); + } + + // Terminal, but the RESTORATIVE terminal. The old suite is untouched and + // stays active; the successor — created empty moments ago and never + // written to — is discarded. It is DELETED rather than revoked on + // purpose: revoking a user suite runs the lost-identity cascade + // (EncryptionSuiteRevokedListener sweeps the owner's incoming + // ShareTargets and promotes their delegations), which would destroy real + // state over a migration the abort exists to undo. + $migration->setStatus('aborted'); + $migration->setCompletedAt(new DateTime()); + $this->mapper->update($migration); + + try { + $successor = $this->suiteMapper->findById($migration->getNewSuiteId()); + $this->suiteMapper->delete($successor); + } catch (DoesNotExistException) { + // Already gone — nothing to discard. + $this->logger->warning( + 'Keepiq: successor suite already absent during abort', + ['migrationId' => $migrationId, 'newSuiteId' => $migration->getNewSuiteId()] + ); + } + + $this->workService->clearFailureAccounting(migration: $migration); + + // NOT SuiteMigrationCompletedEvent: that event runs the terminal cascade + // (compromise-flagging, link-share revocation, emergency-access + // invalidation) which must never fire for an abort. The aborted event + // carries the single reaction abort needs — releasing the SecretRequests + // that SuiteMigrationStartedListener locked, keeping them on the old + // suite — handled by SuiteMigrationAbortedListener. + $this->eventDispatcher?->dispatchTyped( + new SuiteMigrationAbortedEvent( + oldSuiteId: $migration->getOldSuiteId(), + newSuiteId: $migration->getNewSuiteId(), + migrationId: $migration->getId(), + ) + ); + + $this->logger->info( + "Keepiq: Compromise recovery aborted for migration {$migrationId}; vault returned to the old suite", + ['oldSuiteId' => $migration->getOldSuiteId()] + ); + + return ( + $migration->jsonSerialize() + [ + 'aborted' => true, + ] + ); + }//end abortMigration() + /** * Run everything that must happen — and must be allowed — before a * migration may be marked terminal. diff --git a/lib/Service/MigrationWorkService.php b/lib/Service/MigrationWorkService.php index 0426fbff3..a2fd2a5f8 100644 --- a/lib/Service/MigrationWorkService.php +++ b/lib/Service/MigrationWorkService.php @@ -63,6 +63,13 @@ * one generic entry point would mean passing the store as a parameter on a * per-object write path, which the change's design rejected as an IDOR * footgun (hydra-gate-no-admin-idor). + * @SuppressWarnings(PHPMD.ExcessiveClassLength) The length is the same three + * near-parallel per-store pairs (count / list / commit / drop), each with the + * per-store owner-scoping guard that must not be shared. The class sat just + * under the threshold; countCommitted — the mirror of countOutstanding needed + * by the abort gate, and dependent on the same three mappers only this class + * holds — tipped it over. Splitting the suite-bound stores into their own + * services is a separate refactor, not part of the abort change. * * @spec openspec/specs/encryption-suites/spec.md#requirement-migration-covers-every-suite-bound-store */ @@ -318,6 +325,47 @@ public function countUnrecoverable(SuiteMigration $migration): int { return $this->failureMapper->countByMigration(migrationId: $migration->getId()); }//end countUnrecoverable() + /** + * How many of the owner's records have been committed to the NEW suite. + * + * The successor suite is created empty at the start of a migration, so any + * of the owner's suite-bound rows now pointing at it is a record the + * migration has moved. This is the mirror of countOutstanding(), which + * counts what still sits on the OLD suite. It is what decides whether a + * migration may still be aborted: abort is only safe while nothing has + * moved, because once a record is on the new suite, discarding that suite + * would strand it and keeping it would strand everything still on the old + * one. + * + * @param SuiteMigration $migration The migration + * @param string $ownerId The owner's user id + * + * @return integer + * + * @spec openspec/specs/encryption-suites/spec.md#requirement-a-migration-can-be-aborted-before-any-record-moves + */ + public function countCommitted(SuiteMigration $migration, string $ownerId): int { + $newSuiteId = $migration->getNewSuiteId(); + + $secrets = $this->secretMapper->countBySuiteForOwner( + encryptionSuiteId: $newSuiteId, + ownerType: 'user', + ownerId: $ownerId + ); + $versions = $this->versionMapper->countBySuiteForOwner( + encryptionSuiteId: $newSuiteId, + ownerType: 'user', + ownerId: $ownerId + ); + $grants = $this->grantMapper->countBySuiteForRecipient( + encryptionSuiteId: $newSuiteId, + recipientType: 'user', + recipientId: $ownerId + ); + + return ($secrets + $versions + $grants); + }//end countCommitted() + /** * Drop this migration's failure accounting. * diff --git a/openspec/changes/harden-vault-key-material-guards/plan.json b/openspec/changes/harden-vault-key-material-guards/plan.json index a24b04d7f..2ccf380d5 100644 --- a/openspec/changes/harden-vault-key-material-guards/plan.json +++ b/openspec/changes/harden-vault-key-material-guards/plan.json @@ -127,8 +127,8 @@ { "id": 14, "num": "3.1", - "title": "Add `MigrationService::abortMigration(string $migrationId): array` \u2014 refuse unless `in_progress`; refuse when any record has been committed to the new suite, reporting the count and pointing at resume; idempotent by status like `completeMigration`", - "status": "pending", + "title": "Added `MigrationService::abortMigration(string $migrationId): array` \u2014 refuses unless `in_progress` (idempotent no-op otherwise); refuses via `MigrationAbortRefusedException` (mapped to 409) when `MigrationWorkService::countCommitted` finds any record on the new suite, reporting the count and pointing at resume", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -136,8 +136,8 @@ { "id": 15, "num": "3.2", - "title": "On success: set status `aborted`, leave the old suite `active` and its records untouched, revoke the successor suite via `EncryptionSuiteService::revokeSuite`, clear failure accounting via `workService->clearFailureAccounting`, release the write lock", - "status": "pending", + "title": "On success: sets status `aborted`, leaves the old suite `active` and its records untouched, **DELETES** the successor suite via `suiteMapper->delete` (NOT `revokeSuite` \u2014 revoking a user suite cascades the lost-identity share-target sweep + delegation promotion; discovered during implementation, spec/design corrected), clears failure accounting, and releases the write lock (derived from the now-terminal migration)", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -145,8 +145,8 @@ { "id": 16, "num": "3.3", - "title": "Create `SuiteMigrationAbortedEvent` and a listener that unlocks the SecretRequests locked by `SuiteMigrationStartedListener`", - "status": "pending", + "title": "Created `SuiteMigrationAbortedEvent` + `SuiteMigrationAbortedListener` (registered in `SuiteLifecycleEventRegistrar`) that unlocks the SecretRequests locked at start via `unlockAndUpdateSuite(old, old)`, keeping them on the old suite", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -154,8 +154,8 @@ { "id": 17, "num": "3.4", - "title": "**Must not** dispatch `SuiteMigrationCompletedEvent` \u2014 that is what `EmergencyAccessSuiteRotationListener` consumes to invalidate recovery envelopes. Add a regression test asserting envelopes survive an abort", - "status": "pending", + "title": "Does **not** dispatch `SuiteMigrationCompletedEvent`. `MigrationServiceTest::testAbortDispatchesAbortedEventNotCompleted` asserts the aborted event fires and the completed event does not \u2014 the completed event is the only thing `EmergencyAccessSuiteRotationListener` consumes, so this is the unit-level proof envelopes survive an abort", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -163,8 +163,8 @@ { "id": 18, "num": "3.5", - "title": "Add `MigrationController::abort(string $id)` (`#[NoAdminRequired]`) with the existing `requireOwnMigration` ownership check; **no** `#[VaultKeyProofRequired]` (design D6 \u2014 abort is restorative)", - "status": "pending", + "title": "Added `MigrationController::abort(string $id)` (`#[NoAdminRequired]`) with the existing `requireOwnMigration` check; no `#[VaultKeyProofRequired]` (design D6 \u2014 abort is restorative)", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -172,8 +172,8 @@ { "id": 19, "num": "3.6", - "title": "Register `['name' => 'migration#abort', 'url' => '/api/v1/migrations/{id}/abort', 'verb' => 'POST']`", - "status": "pending", + "title": "Registered `migration#abort` \u2192 `POST /api/v1/migrations/{id}/abort`", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -181,8 +181,8 @@ { "id": 20, "num": "3.7", - "title": "Update the refusal text in `EncryptionSuiteController::compromiseRecovery()` so the promised \"abort\" now names a route that exists", - "status": "pending", + "title": "The `compromiseRecovery` refusal already reads \"Resume or **abort** that migration before starting another\" \u2014 that promised route now exists, so the wording is backed rather than broken. Left as-is", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -190,8 +190,8 @@ { "id": 21, "num": "3.8", - "title": "Add an abort control to `src/components/MigrationResumeBanner.vue`, shown only while abort is still available, with copy stating that the old suite stays intact", - "status": "pending", + "title": "Added an \"Abort and keep my old key\" control to `MigrationResumeBanner.vue` (shown while the banner is expanded), plus the `abortMigration` store action and its vitest coverage (success clears the banner; a 409 refusal keeps it and surfaces the message)", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] diff --git a/openspec/changes/harden-vault-key-material-guards/tasks.md b/openspec/changes/harden-vault-key-material-guards/tasks.md index b8ea301a8..a9b575948 100644 --- a/openspec/changes/harden-vault-key-material-guards/tasks.md +++ b/openspec/changes/harden-vault-key-material-guards/tasks.md @@ -25,16 +25,16 @@ Section 3 (abort) is independently useful and can be split into its own PR if th - [ ] 2.3 Register `['name' => 'encryptionSuite#proofChallenge', 'url' => '/api/v1/suites/{id}/proof-challenge', 'verb' => 'GET']` in `appinfo/routes.php`, before the SPA catch-all wildcard - [ ] 2.4 The challenge endpoint itself MUST NOT carry `#[VaultKeyProofRequired]` — assert in the coverage test that it is on the deliberate-exclusion list -## 3. Backend — Abort (independently mergeable) - -- [ ] 3.1 Add `MigrationService::abortMigration(string $migrationId): array` — refuse unless `in_progress`; refuse when any record has been committed to the new suite, reporting the count and pointing at resume; idempotent by status like `completeMigration` -- [ ] 3.2 On success: set status `aborted`, leave the old suite `active` and its records untouched, revoke the successor suite via `EncryptionSuiteService::revokeSuite`, clear failure accounting via `workService->clearFailureAccounting`, release the write lock -- [ ] 3.3 Create `SuiteMigrationAbortedEvent` and a listener that unlocks the SecretRequests locked by `SuiteMigrationStartedListener` -- [ ] 3.4 **Must not** dispatch `SuiteMigrationCompletedEvent` — that is what `EmergencyAccessSuiteRotationListener` consumes to invalidate recovery envelopes. Add a regression test asserting envelopes survive an abort -- [ ] 3.5 Add `MigrationController::abort(string $id)` (`#[NoAdminRequired]`) with the existing `requireOwnMigration` ownership check; **no** `#[VaultKeyProofRequired]` (design D6 — abort is restorative) -- [ ] 3.6 Register `['name' => 'migration#abort', 'url' => '/api/v1/migrations/{id}/abort', 'verb' => 'POST']` -- [ ] 3.7 Update the refusal text in `EncryptionSuiteController::compromiseRecovery()` so the promised "abort" now names a route that exists -- [ ] 3.8 Add an abort control to `src/components/MigrationResumeBanner.vue`, shown only while abort is still available, with copy stating that the old suite stays intact +## 3. Backend — Abort (independently mergeable) — IMPLEMENTED + +- [x] 3.1 Added `MigrationService::abortMigration(string $migrationId): array` — refuses unless `in_progress` (idempotent no-op otherwise); refuses via `MigrationAbortRefusedException` (mapped to 409) when `MigrationWorkService::countCommitted` finds any record on the new suite, reporting the count and pointing at resume +- [x] 3.2 On success: sets status `aborted`, leaves the old suite `active` and its records untouched, **DELETES** the successor suite via `suiteMapper->delete` (NOT `revokeSuite` — revoking a user suite cascades the lost-identity share-target sweep + delegation promotion; discovered during implementation, spec/design corrected), clears failure accounting, and releases the write lock (derived from the now-terminal migration) +- [x] 3.3 Created `SuiteMigrationAbortedEvent` + `SuiteMigrationAbortedListener` (registered in `SuiteLifecycleEventRegistrar`) that unlocks the SecretRequests locked at start via `unlockAndUpdateSuite(old, old)`, keeping them on the old suite +- [x] 3.4 Does **not** dispatch `SuiteMigrationCompletedEvent`. `MigrationServiceTest::testAbortDispatchesAbortedEventNotCompleted` asserts the aborted event fires and the completed event does not — the completed event is the only thing `EmergencyAccessSuiteRotationListener` consumes, so this is the unit-level proof envelopes survive an abort +- [x] 3.5 Added `MigrationController::abort(string $id)` (`#[NoAdminRequired]`) with the existing `requireOwnMigration` check; no `#[VaultKeyProofRequired]` (design D6 — abort is restorative) +- [x] 3.6 Registered `migration#abort` → `POST /api/v1/migrations/{id}/abort` +- [x] 3.7 The `compromiseRecovery` refusal already reads "Resume or **abort** that migration before starting another" — that promised route now exists, so the wording is backed rather than broken. Left as-is +- [x] 3.8 Added an "Abort and keep my old key" control to `MigrationResumeBanner.vue` (shown while the banner is expanded), plus the `abortMigration` store action and its vitest coverage (success clears the banner; a 409 refusal keeps it and surfaces the message) ## 4. Frontend — Producing the Proof diff --git a/src/components/MigrationResumeBanner.vue b/src/components/MigrationResumeBanner.vue index 666e8ff6a..7e9202562 100644 --- a/src/components/MigrationResumeBanner.vue +++ b/src/components/MigrationResumeBanner.vue @@ -54,6 +54,32 @@ {{ t('keepiq', 'Unlock your vault first, then resume.') }}

+ +
+ + {{ + busy + ? t('keepiq', 'Aborting…') + : t('keepiq', 'Abort and keep my old key') + }} + + + {{ + t( + 'keepiq', + 'Discards the new key and unlocks your vault under the old one. Only possible while nothing has been re-encrypted yet.', + ) + }} + +
+

{{ progressLabel }}

@@ -222,6 +248,31 @@ export default { this.busy = false } }, + + /** + * Abort the migration, returning the vault to the old key. + * + * On a server refusal (records already moved) the message says so and + * the banner stays, pointing the user at resuming instead. + * + * @return {Promise} + * @spec openspec/specs/encryption-suites/spec.md#requirement-a-migration-can-be-aborted-before-any-record-moves + */ + async onAbort() { + this.busy = true + this.error = null + + try { + await useEncryptionSuiteStore().abortMigration() + } catch (e) { + this.error = + e?.response?.data?.message + || e?.message + || this.t('keepiq', 'Could not abort the rotation.') + } finally { + this.busy = false + } + }, }, } diff --git a/src/store/modules/encryptionSuite.js b/src/store/modules/encryptionSuite.js index 35306a051..55b720d2b 100644 --- a/src/store/modules/encryptionSuite.js +++ b/src/store/modules/encryptionSuite.js @@ -957,5 +957,41 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { this.migrationStatus = null } }, + + /** + * Abort an interrupted migration, returning the vault to the old suite. + * + * The non-destructive escape from a rotation the user does not want to + * finish: it discards the unused new key and unlocks the vault under the + * old key, which never stopped being valid. The server refuses (409) if + * any record has already moved to the new suite — at that point resuming + * is the only safe route — so this surfaces that as an error for the + * banner rather than pretending it succeeded. + * + * @return {Promise} The server's terminal result. + * @spec openspec/specs/encryption-suites/spec.md#requirement-a-migration-can-be-aborted-before-any-record-moves + */ + async abortMigration() { + await this.fetchMigrationStatus() + if (this.migrationStatus === null) { + throw new Error('There is no migration to abort') + } + + const migrationId = this.migrationStatus.id + try { + const { data } = await axios.post( + generateUrl( + `/apps/keepiq/api/v1/migrations/${migrationId}/abort`, + ), + ) + return data + } finally { + // Whether it aborted or was refused, re-read the authoritative + // state so the banner reflects reality (cleared on success, still + // present with its remaining count on a refusal). + await this.fetchMigrationStatus() + await this.fetchMigrationRemaining() + } + }, }, }) diff --git a/tests/Unit/Controller/MigrationControllerTest.php b/tests/Unit/Controller/MigrationControllerTest.php index 08372ec40..e81561cf1 100644 --- a/tests/Unit/Controller/MigrationControllerTest.php +++ b/tests/Unit/Controller/MigrationControllerTest.php @@ -25,6 +25,7 @@ use OCA\Keepiq\Db\Secret; use OCA\Keepiq\Db\SuiteMigration; use OCA\Keepiq\Exception\ForbiddenException; +use OCA\Keepiq\Exception\MigrationAbortRefusedException; use OCA\Keepiq\Exception\MigrationIncompleteException; use OCA\Keepiq\Exception\NotFoundException; use OCA\Keepiq\Service\EncryptionSuiteService; @@ -245,6 +246,61 @@ public function testCompleteWithErrors(): void { $this->assertSame('completed_with_errors', $response->getData()['status']); }//end testCompleteWithErrors() + /** + * Abort delegates and returns the terminal result. + * + * @return void + */ + public function testAbortReturnsResult(): void { + $this->arrangeOwnMigration(); + + $aborted = new SuiteMigration(); + $aborted->setId('migr-1'); + $aborted->setStatus('aborted'); + $this->migrationService->method('abortMigration') + ->with('migr-1') + ->willReturn($aborted->jsonSerialize() + ['aborted' => true]); + + $response = $this->controller->abort('migr-1'); + + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + $this->assertTrue($response->getData()['aborted']); + $this->assertSame('aborted', $response->getData()['status']); + }//end testAbortReturnsResult() + + /** + * Abort after a record has moved is a 409 that reports the committed count + * and points at resuming, not a generic fault. + * + * @return void + */ + public function testAbortRefusedReturns409WithCommittedCount(): void { + $this->arrangeOwnMigration(); + + $this->migrationService->method('abortMigration') + ->willThrowException((new MigrationAbortRefusedException('records moved'))->withCommitted(2)); + + $response = $this->controller->abort('migr-1'); + + $this->assertSame(Http::STATUS_CONFLICT, $response->getStatus()); + $this->assertSame('migration_abort_refused', $response->getData()['error']); + $this->assertSame(2, $response->getData()['committed']); + }//end testAbortRefusedReturns409WithCommittedCount() + + /** + * Abort refuses another user's migration and never reaches the service. + * + * @return void + */ + public function testAbortForbiddenForAnotherUsersMigration(): void { + $this->arrangeForeignMigration(); + $this->migrationService->expects($this->never())->method('abortMigration'); + + $response = $this->controller->abort('migr-1'); + + $this->assertSame(Http::STATUS_FORBIDDEN, $response->getStatus()); + }//end testAbortForbiddenForAnotherUsersMigration() + /** * A missing migration is 404 on complete, as on every sibling endpoint. * diff --git a/tests/Unit/Service/MigrationServiceTest.php b/tests/Unit/Service/MigrationServiceTest.php index 794ba4822..c3d64a199 100644 --- a/tests/Unit/Service/MigrationServiceTest.php +++ b/tests/Unit/Service/MigrationServiceTest.php @@ -8,12 +8,16 @@ use OCA\Keepiq\Db\EncryptionSuiteMapper; use OCA\Keepiq\Db\SuiteMigration; use OCA\Keepiq\Db\SuiteMigrationMapper; +use OCA\Keepiq\Event\SuiteMigrationAbortedEvent; +use OCA\Keepiq\Event\SuiteMigrationCompletedEvent; +use OCA\Keepiq\Exception\MigrationAbortRefusedException; use OCA\Keepiq\Exception\MigrationIncompleteException; use OCA\Keepiq\Service\EncryptionSuiteService; use OCA\Keepiq\Service\LinkShareService; use OCA\Keepiq\Service\MigrationService; use OCA\Keepiq\Service\MigrationWorkService; use OCA\Keepiq\Service\WriteLockService; +use OCP\EventDispatcher\IEventDispatcher; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; @@ -185,6 +189,144 @@ public function testIsNotWriteLockedWhenNoMigration(): void { $this->assertFalse($this->service->isWriteLocked('user', 'testuser')); }//end testIsNotWriteLockedWhenNoMigration() + /** + * Aborting an untouched migration restores the old suite and discards the + * successor by DELETING it (never revoking, which would cascade the + * user-suite revocation side effects). + * + * @return void + */ + public function testAbortRestoresOldSuiteAndDeletesTheSuccessor(): void { + $migration = $this->arrangeAbortableMigration(committed: 0); + + $this->migrationMapper->expects($this->once())->method('update'); + // The successor is DELETED, not revoked — revokeSuite must never run. + $this->suiteMapper->expects($this->once())->method('delete'); + $this->suiteService->expects($this->never())->method('revokeSuite'); + $this->workService->expects($this->once())->method('clearFailureAccounting'); + + $result = $this->service->abortMigration('migration-1'); + + $this->assertTrue($result['aborted']); + $this->assertSame('aborted', $migration->getStatus()); + }//end testAbortRestoresOldSuiteAndDeletesTheSuccessor() + + /** + * Once a record has been committed to the new suite, abort is refused and + * reports the committed count; nothing is torn down. + * + * @return void + */ + public function testAbortRefusedAfterARecordHasBeenCommitted(): void { + $this->arrangeAbortableMigration(committed: 3); + + $this->migrationMapper->expects($this->never())->method('update'); + $this->suiteMapper->expects($this->never())->method('delete'); + + try { + $this->service->abortMigration('migration-1'); + $this->fail('Expected MigrationAbortRefusedException'); + } catch (MigrationAbortRefusedException $e) { + $this->assertSame(3, $e->getCommitted()); + } + }//end testAbortRefusedAfterARecordHasBeenCommitted() + + /** + * Aborting an already-terminated migration is a no-op, not a second + * teardown — mirrors completeMigration's idempotency guard. + * + * @return void + */ + public function testAbortingAnAlreadyTerminatedMigrationIsANoOp(): void { + $migration = new SuiteMigration(); + $migration->setId('migration-1'); + $migration->setOldSuiteId('old-suite'); + $migration->setNewSuiteId('new-suite'); + $migration->setStatus('completed'); + $this->migrationMapper->method('findById')->willReturn($migration); + + $this->migrationMapper->expects($this->never())->method('update'); + $this->suiteMapper->expects($this->never())->method('delete'); + + $result = $this->service->abortMigration('migration-1'); + + $this->assertFalse($result['aborted']); + $this->assertTrue($result['alreadyTerminated']); + }//end testAbortingAnAlreadyTerminatedMigrationIsANoOp() + + /** + * Abort dispatches SuiteMigrationAbortedEvent and NEVER + * SuiteMigrationCompletedEvent — the latter runs the terminal cascade + * (compromise-flagging, link-share revocation, emergency-access + * invalidation) that must not fire when nothing migrated. + * + * @return void + */ + public function testAbortDispatchesAbortedEventNotCompleted(): void { + $dispatcher = $this->createMock(IEventDispatcher::class); + $service = new MigrationService( + mapper: $this->migrationMapper, + suiteMapper: $this->suiteMapper, + suiteService: $this->suiteService, + linkShareService: $this->linkShareService, + workService: $this->workService, + writeLockService: $this->writeLockService, + logger: $this->createMock(LoggerInterface::class), + eventDispatcher: $dispatcher, + ); + $this->arrangeAbortableMigration(committed: 0); + + $dispatched = []; + $dispatcher->method('dispatchTyped')->willReturnCallback( + static function (object $event) use (&$dispatched): void { + $dispatched[] = $event::class; + } + ); + + $service->abortMigration('migration-1'); + + $this->assertContains(SuiteMigrationAbortedEvent::class, $dispatched); + $this->assertNotContains(SuiteMigrationCompletedEvent::class, $dispatched); + }//end testAbortDispatchesAbortedEventNotCompleted() + + /** + * Wire an in-progress, abortable migration: the old suite resolves to an + * owner and the successor is a distinct suite so the delete path is + * exercised. `committed` sets what countCommitted reports. + * + * @param int $committed How many records countCommitted should report + * + * @return SuiteMigration + */ + private function arrangeAbortableMigration(int $committed): SuiteMigration { + $migration = new SuiteMigration(); + $migration->setId('migration-1'); + $migration->setOldSuiteId('old-suite'); + $migration->setNewSuiteId('new-suite'); + $migration->setStatus('in_progress'); + $this->migrationMapper->method('findById')->willReturn($migration); + + $old = new EncryptionSuite(); + $old->setId('old-suite'); + $old->setOwnerType('user'); + $old->setOwnerId('alice'); + + $successor = new EncryptionSuite(); + $successor->setId('new-suite'); + $successor->setOwnerType('user'); + $successor->setOwnerId('alice'); + + $this->suiteMapper->method('findById')->willReturnCallback( + static function (string $id) use ($old, $successor): EncryptionSuite { + return ($id === 'new-suite') ? $successor : $old; + } + ); + + $this->workService->method('countCommitted')->willReturn($committed); + + return $migration; + }//end arrangeAbortableMigration() + /** * Wire an in-progress migration whose old suite resolves to an owner, so * the owner-scoped terminal work and the completion gate both engage. diff --git a/tests/store/encryptionSuite.spec.js b/tests/store/encryptionSuite.spec.js index 5e94d0797..06a6e6340 100644 --- a/tests/store/encryptionSuite.spec.js +++ b/tests/store/encryptionSuite.spec.js @@ -139,3 +139,71 @@ describe('useEncryptionSuiteStore — revocation', () => { // the server deliberately has no endpoint listing who holds a suite, so // candidates are named by Nextcloud's sharee search and PROBED // (tests/store/share.recipients.spec.js). + +describe('useEncryptionSuiteStore — abort migration', () => { + beforeEach(() => { + setActivePinia(createPinia()) + vi.restoreAllMocks() + }) + + it('POSTs the abort to the in-progress migration and clears state on success', async () => { + // status GET first resolves in-progress, then 'none' after the abort. + const statuses = [ + { data: { status: 'in_progress', id: 'migr-1', oldSuiteId: 'old' } }, + { data: { status: 'none' } }, + ] + vi.spyOn(axios, 'get').mockImplementation(async (url) => { + if (url.endsWith('/migrations/status')) { + return statuses.shift() ?? { data: { status: 'none' } } + } + // fetchMigrationRemaining hits /work + return { data: { totalRemaining: 0 } } + }) + const post = vi.spyOn(axios, 'post').mockResolvedValue({ + data: { id: 'migr-1', status: 'aborted', aborted: true }, + }) + + const store = useEncryptionSuiteStore() + const result = await store.abortMigration() + + expect(post).toHaveBeenCalledWith( + expect.stringContaining('/migrations/migr-1/abort'), + ) + expect(result.aborted).toBe(true) + // State re-read afterwards and the banner cleared. + expect(store.migrationStatus).toBeNull() + }) + + it('surfaces a server refusal (records already moved) as a throw and keeps the banner', async () => { + vi.spyOn(axios, 'get').mockImplementation(async (url) => { + if (url.endsWith('/migrations/status')) { + return { + data: { status: 'in_progress', id: 'migr-1', oldSuiteId: 'old' }, + } + } + return { data: { totalRemaining: 4 } } + }) + vi.spyOn(axios, 'post').mockRejectedValue({ + response: { + status: 409, + data: { error: 'migration_abort_refused', committed: 2 }, + }, + }) + + const store = useEncryptionSuiteStore() + await expect(store.abortMigration()).rejects.toMatchObject({ + response: { data: { committed: 2 } }, + }) + // The migration is still there — abort did not clear it. + expect(store.migrationStatus).not.toBeNull() + }) + + it('refuses to abort when there is no migration', async () => { + vi.spyOn(axios, 'get').mockResolvedValue({ data: { status: 'none' } }) + const post = vi.spyOn(axios, 'post') + + const store = useEncryptionSuiteStore() + await expect(store.abortMigration()).rejects.toThrow(/no migration to abort/) + expect(post).not.toHaveBeenCalled() + }) +}) From bb0e61c8cfbbd32e3c89079f7bf5eb5c64feac28 Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 10 Sep 2026 15:10:16 +0200 Subject: [PATCH 07/48] feat(vault-key-proof): server-verified master-password guard (#673) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The core of the fix for the session-only lockout (#395). A destructive operation on vault key material now requires a VaultKeyProof: a signature, made with the caller's suite private key, over a server-issued challenge bound to the operation's own parameters. The private key is obtainable only by decrypting its envelope with the master password, so a verified proof is a server-verifiable proof of the master password — a stolen session, a leaked app password, or XSS in an unlocked tab no longer suffices, because the session key is non-extractable and decrypt-only and so cannot sign. - `#[VaultKeyProofRequired(binds, subject, purpose)]` declares the guard on a method; the binding lives on the attribute because the middleware cannot read the request body (the framework decodes JSON and drops the raw bytes), so the proof commits to NAMED parameters, hashed individually in order. - `VaultKeyProofMiddleware` enforces it: reads the attribute by reflection, resolves the subject suite, collects the bound params, delegates to the service, and maps a failure to 403 `key_proof_required`. It consults no auth backend and honours no token scope, so it is not waived for SSO/app-password sessions — its authority is key material, not the login method. - `VaultKeyProofService` issues a STATELESS, expiring, HMAC-authenticated nonce (no ICacheFactory — a null cache on a default install would break the flow) and verifies an RSASSA-PKCS1-v1_5 SHA-256 signature. Replay is a non-issue because the signature commits to the operation's parameters. - Challenge endpoint `GET /api/v1/suites/{id}/proof-challenge` (ungated). - Guard applied to compromiseRecovery, updatePrivateKey, complete, and the emergency-contact destroy. `VaultKeyProofAttributesTest` enumerates them and fails the build if one drops the attribute (a declarative guard fails open by omission), with a documented exclusion list (challenge, abort). Service crypto and middleware dispatch fully unit-tested (valid verifies; wrong key / altered value / tampered nonce / expired / wrong purpose / wrong user / missing all refused; attribute dispatch, subject resolution, foreign suite, 403 mapping). phpmd clean. The client half (proveMasterPassword + wiring the four flows) lands next — until then the guarded routes 403 by design. Refs #673 Assisted-by: ClaudeCode:claude-opus-5 --- appinfo/routes.php | 1 + lib/AppInfo/PlatformIntegrationRegistrar.php | 6 + lib/Attribute/VaultKeyProofRequired.php | 92 +++++++ lib/Controller/EmergencyAccessController.php | 7 + lib/Controller/EncryptionSuiteController.php | 59 +++++ lib/Controller/MigrationController.php | 7 + lib/Exception/KeyProofRequiredException.php | 35 +++ lib/Middleware/VaultKeyProofMiddleware.php | 222 ++++++++++++++++ lib/Service/VaultKeyProofService.php | 249 ++++++++++++++++++ .../plan.json | 40 +-- .../harden-vault-key-material-guards/tasks.md | 40 +-- .../EncryptionSuiteControllerTest.php | 10 + .../VaultKeyProofAttributesTest.php | 157 +++++++++++ .../VaultKeyProofMiddlewareTest.php | 184 +++++++++++++ .../Unit/Service/VaultKeyProofServiceTest.php | 165 ++++++++++++ 15 files changed, 1234 insertions(+), 40 deletions(-) create mode 100644 lib/Attribute/VaultKeyProofRequired.php create mode 100644 lib/Exception/KeyProofRequiredException.php create mode 100644 lib/Middleware/VaultKeyProofMiddleware.php create mode 100644 lib/Service/VaultKeyProofService.php create mode 100644 tests/Unit/Controller/VaultKeyProofAttributesTest.php create mode 100644 tests/Unit/Middleware/VaultKeyProofMiddlewareTest.php create mode 100644 tests/Unit/Service/VaultKeyProofServiceTest.php diff --git a/appinfo/routes.php b/appinfo/routes.php index 72e36a4d8..63ef76ca6 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -39,6 +39,7 @@ ['name' => 'encryptionSuite#revoke', 'url' => '/api/v1/suites/{id}/revoke', 'verb' => 'POST'], ['name' => 'encryptionSuite#reinstate', 'url' => '/api/v1/suites/{id}/reinstate', 'verb' => 'POST'], ['name' => 'encryptionSuite#compromiseRecovery','url' => '/api/v1/suites/compromise-recovery', 'verb' => 'POST'], + ['name' => 'encryptionSuite#proofChallenge', 'url' => '/api/v1/suites/{id}/proof-challenge', 'verb' => 'GET'], // CA management (admin-only). ['name' => 'cACertificate#getStatus', 'url' => '/api/v1/ca/status', 'verb' => 'GET'], diff --git a/lib/AppInfo/PlatformIntegrationRegistrar.php b/lib/AppInfo/PlatformIntegrationRegistrar.php index 2c21c1faf..3337b5e86 100644 --- a/lib/AppInfo/PlatformIntegrationRegistrar.php +++ b/lib/AppInfo/PlatformIntegrationRegistrar.php @@ -23,6 +23,7 @@ namespace OCA\Keepiq\AppInfo; use OCA\Keepiq\Middleware\JwtAuthMiddleware; +use OCA\Keepiq\Middleware\VaultKeyProofMiddleware; use OCA\Keepiq\Notification\KeepiqNotifier; use OCA\Keepiq\Search\SecretSearchProvider; use OCP\AppFramework\Bootstrap\IRegistrationContext; @@ -63,5 +64,10 @@ public function register(IRegistrationContext $context): void { // controllers pass through untouched. $context->registerMiddleware(JwtAuthMiddleware::class); + // The vault-key-proof middleware. Runs for every controller but acts + // only on methods carrying #[VaultKeyProofRequired]; every other method + // passes through untouched. + $context->registerMiddleware(VaultKeyProofMiddleware::class); + }//end register() }//end class diff --git a/lib/Attribute/VaultKeyProofRequired.php b/lib/Attribute/VaultKeyProofRequired.php new file mode 100644 index 000000000..5ab5e4672 --- /dev/null +++ b/lib/Attribute/VaultKeyProofRequired.php @@ -0,0 +1,92 @@ + + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\Keepiq\Attribute; + +use Attribute; + +/** + * Require a verified vault-key proof on the annotated controller method. + */ +#[Attribute(Attribute::TARGET_METHOD)] +class VaultKeyProofRequired { + /** + * Constructor. + * + * @param string[] $binds Request parameter names the proof commits to, in + * the order they are hashed into the signed payload. + * Empty means the proof binds to the challenge alone. + * @param string $subject Whose public key verifies the proof: + * 'active' (default) — the caller's active suite; + * 'routeParam:' — the suite named by that route + * parameter. + * @param string $purpose A stable public identifier for this operation. A + * challenge is bound to one purpose, so a proof + * obtained for one guarded operation cannot be + * presented to another. The client requests its + * challenge with the same string. + * + * @return void + */ + public function __construct( + private array $binds = [], + private string $subject = 'active', + private string $purpose = '', + ) { + }//end __construct() + + /** + * The request parameter names the proof binds to, in payload order. + * + * @return string[] + */ + public function getBinds(): array { + return $this->binds; + }//end getBinds() + + /** + * How the subject suite is resolved. + * + * @return string + */ + public function getSubject(): string { + return $this->subject; + }//end getSubject() + + /** + * The stable purpose identifier this operation's challenge is bound to. + * + * @return string + */ + public function getPurpose(): string { + return $this->purpose; + }//end getPurpose() +}//end class diff --git a/lib/Controller/EmergencyAccessController.php b/lib/Controller/EmergencyAccessController.php index 502a77b0e..50b1bcbde 100644 --- a/lib/Controller/EmergencyAccessController.php +++ b/lib/Controller/EmergencyAccessController.php @@ -30,9 +30,11 @@ use InvalidArgumentException; use OCA\Keepiq\AppInfo\Application; +use OCA\Keepiq\Attribute\VaultKeyProofRequired; use OCA\Keepiq\Exception\ForbiddenException; use OCA\Keepiq\Exception\NotFoundException; use OCA\Keepiq\Service\EmergencyAccessService; +use OCA\Keepiq\Service\VaultKeyProofService; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\JSONResponse; @@ -192,6 +194,11 @@ public function create( * @spec openspec/changes/add-emergency-access/specs/emergency-access/spec.md#requirement-revoke-emergency-contact */ #[NoAdminRequired] + #[VaultKeyProofRequired( + binds: ['id'], + subject: 'active', + purpose: VaultKeyProofService::PURPOSE_EMERGENCY_DESTROY + )] public function destroy(string $id): JSONResponse { $userId = $this->requireUserId(); if ($userId === null) { diff --git a/lib/Controller/EncryptionSuiteController.php b/lib/Controller/EncryptionSuiteController.php index b5031ec4b..0e6c5f01f 100644 --- a/lib/Controller/EncryptionSuiteController.php +++ b/lib/Controller/EncryptionSuiteController.php @@ -25,8 +25,10 @@ use InvalidArgumentException; use OCA\Keepiq\AppInfo\Application; use OCA\Keepiq\Exception\ConflictException; +use OCA\Keepiq\Attribute\VaultKeyProofRequired; use OCA\Keepiq\Service\EncryptionSuiteService; use OCA\Keepiq\Service\MigrationService; +use OCA\Keepiq\Service\VaultKeyProofService; use OCA\Keepiq\Settings\AdminSettings; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting; @@ -57,6 +59,7 @@ public function __construct( private EncryptionSuiteService $suiteService, private MigrationService $migrationService, private IUserSession $userSession, + private VaultKeyProofService $proofService, private ?\OCA\Keepiq\Service\PasskeyService $passkeyService = null, ) { parent::__construct(appName: Application::APP_ID, request: $request); @@ -230,6 +233,11 @@ public function create( * @spec openspec/changes/retrofit-2026-05-25-doriath-coverage/tasks.md#task-2 */ #[NoAdminRequired] + #[VaultKeyProofRequired( + binds: ['encryptedPrivateKey'], + subject: 'routeParam:id', + purpose: VaultKeyProofService::PURPOSE_UPDATE_PRIVATE_KEY + )] public function updatePrivateKey(string $id, string $encryptedPrivateKey): JSONResponse { try { $suite = $this->suiteService->getSuite($id); @@ -342,6 +350,11 @@ public function reinstate(string $id): JSONResponse { * @spec openspec/changes/implement-link-sharing/tasks.md#5.2 */ #[NoAdminRequired] + #[VaultKeyProofRequired( + binds: ['publicKey', 'encryptedPrivateKey'], + subject: 'active', + purpose: VaultKeyProofService::PURPOSE_COMPROMISE_RECOVERY + )] public function compromiseRecovery( string $publicKey, string $encryptedPrivateKey, @@ -442,6 +455,52 @@ public function compromiseRecovery( }//end try }//end compromiseRecovery() + /** + * Issue a vault-key-proof challenge for one of the guarded operations. + * + * Returns a stateless, expiring nonce the client signs with its suite + * private key to authorise a destructive operation. Requires only a session + * and that the caller own the named suite; it is NOT itself guarded, since a + * challenge grants nothing on its own. + * + * @param string $id The caller's suite the proof will be made with + * @param string|null $purpose The operation the challenge authorises + * + * @NoAdminRequired + * + * @return JSONResponse + * + * @spec openspec/changes/harden-vault-key-material-guards/specs/vault-key-proof/spec.md#requirement-challenges-are-stateless-and-expiring + */ + #[NoAdminRequired] + public function proofChallenge(string $id, ?string $purpose = null): JSONResponse { + $user = $this->userSession->getUser(); + if ($user === null) { + return new JSONResponse(data: ['message' => 'Unauthorized'], statusCode: Http::STATUS_UNAUTHORIZED); + } + + if ($purpose === null || in_array($purpose, VaultKeyProofService::ALLOWED_PURPOSES, true) === false) { + return new JSONResponse( + data: ['message' => 'Unknown or missing proof purpose'], + statusCode: Http::STATUS_BAD_REQUEST + ); + } + + try { + $suite = $this->suiteService->getSuite($id); + $this->validateOwnership(suite: $suite); + } catch (Exception $e) { + return new JSONResponse( + data: ['message' => $e->getMessage()], + statusCode: Http::STATUS_NOT_FOUND + ); + } + + return new JSONResponse( + data: $this->proofService->issueChallenge(userId: $user->getUID(), purpose: $purpose) + ); + }//end proofChallenge() + /** * Validate that the current user owns the suite (or is admin). * diff --git a/lib/Controller/MigrationController.php b/lib/Controller/MigrationController.php index 1997b65c7..16dc7988e 100644 --- a/lib/Controller/MigrationController.php +++ b/lib/Controller/MigrationController.php @@ -23,6 +23,7 @@ use Exception; use OCA\Keepiq\AppInfo\Application; +use OCA\Keepiq\Attribute\VaultKeyProofRequired; use OCA\Keepiq\Db\SuiteMigration; use OCA\Keepiq\Exception\ForbiddenException; use OCA\Keepiq\Exception\MigrationAbortRefusedException; @@ -31,6 +32,7 @@ use OCA\Keepiq\Service\EncryptionSuiteService; use OCA\Keepiq\Service\MigrationService; use OCA\Keepiq\Service\MigrationWorkService; +use OCA\Keepiq\Service\VaultKeyProofService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; @@ -115,6 +117,11 @@ public function getStatus(): JSONResponse { * @spec openspec/changes/retrofit-2026-05-25-doriath-coverage/tasks.md#task-4 */ #[NoAdminRequired] + #[VaultKeyProofRequired( + binds: ['id'], + subject: 'active', + purpose: VaultKeyProofService::PURPOSE_COMPLETE_MIGRATION + )] public function complete(string $id, bool $hasErrors = false, ?int $acceptUnrecoverable = null): JSONResponse { $user = $this->userSession->getUser(); if ($user === null) { diff --git a/lib/Exception/KeyProofRequiredException.php b/lib/Exception/KeyProofRequiredException.php new file mode 100644 index 000000000..6f4f54401 --- /dev/null +++ b/lib/Exception/KeyProofRequiredException.php @@ -0,0 +1,35 @@ + + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\Keepiq\Exception; + +use RuntimeException; + +/** + * Thrown when a required vault-key proof is absent or does not verify. + */ +class KeyProofRequiredException extends RuntimeException { +}//end class diff --git a/lib/Middleware/VaultKeyProofMiddleware.php b/lib/Middleware/VaultKeyProofMiddleware.php new file mode 100644 index 000000000..040324179 --- /dev/null +++ b/lib/Middleware/VaultKeyProofMiddleware.php @@ -0,0 +1,222 @@ + + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\Keepiq\Middleware; + +use OCA\Keepiq\Attribute\VaultKeyProofRequired; +use OCA\Keepiq\Db\EncryptionSuite; +use OCA\Keepiq\Exception\KeyProofRequiredException; +use OCA\Keepiq\Service\EncryptionSuiteService; +use OCA\Keepiq\Service\VaultKeyProofService; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\JSONResponse; +use OCP\AppFramework\Middleware; +use OCP\IRequest; +use OCP\IUserSession; +use ReflectionMethod; +use Throwable; + +/** + * Enforce #[VaultKeyProofRequired] on the annotated controller methods. + */ +class VaultKeyProofMiddleware extends Middleware { + /** + * The header carrying the base64 signature. + */ + private const HEADER_PROOF = 'X-Keepiq-Key-Proof'; + + /** + * The header echoing the challenge the proof was made over. + */ + private const HEADER_NONCE = 'X-Keepiq-Key-Proof-Nonce'; + + /** + * Constructor. + * + * @param IRequest $request The HTTP request + * @param IUserSession $userSession The session, for the acting user + * @param EncryptionSuiteService $suiteService Resolves the subject suite + * @param VaultKeyProofService $proofService Verifies the proof + * + * @return void + */ + public function __construct( + private IRequest $request, + private IUserSession $userSession, + private EncryptionSuiteService $suiteService, + private VaultKeyProofService $proofService, + ) { + }//end __construct() + + /** + * Verify the proof before a guarded method runs. + * + * @param Controller $controller The controller about to run + * @param string $methodName The method about to run + * + * @return void + * + * @throws KeyProofRequiredException When the guard is not satisfied + */ + public function beforeController($controller, $methodName): void { + $attribute = $this->attributeFor(controller: $controller, methodName: $methodName); + if ($attribute === null) { + return; + } + + $user = $this->userSession->getUser(); + if ($user === null) { + // No session at all is an authentication problem, not a proof one; + // the framework's own auth handling has already refused, but guard + // against a null here rather than dereferencing it. + throw new KeyProofRequiredException(message: 'Not authenticated'); + } + + $userId = $user->getUID(); + $certificate = $this->subjectCertificate(attribute: $attribute, userId: $userId); + + $boundValues = []; + foreach ($attribute->getBinds() as $name) { + $boundValues[] = (string)$this->request->getParam($name, ''); + } + + $this->proofService->verify( + nonce: $this->request->getHeader(self::HEADER_NONCE), + signatureB64: $this->request->getHeader(self::HEADER_PROOF), + certificatePem: $certificate, + userId: $userId, + purpose: $attribute->getPurpose(), + boundValues: $boundValues, + ); + }//end beforeController() + + /** + * Translate a failed guard into a 403 the client can act on. + * + * @param Controller $controller The controller + * @param string $methodName The method + * @param Throwable $exception The raised exception + * + * @return JSONResponse + * + * @throws Throwable When the exception is not the guard's own (re-thrown) + */ + public function afterException($controller, $methodName, Throwable $exception): JSONResponse { + if (($exception instanceof KeyProofRequiredException) === false) { + throw $exception; + } + + return new JSONResponse( + data: [ + 'error' => 'key_proof_required', + 'message' => $exception->getMessage(), + ], + statusCode: Http::STATUS_FORBIDDEN + ); + }//end afterException() + + /** + * The #[VaultKeyProofRequired] attribute on the method, or null. + * + * @param Controller $controller The controller + * @param string $methodName The method + * + * @return VaultKeyProofRequired|null + */ + private function attributeFor($controller, string $methodName): ?VaultKeyProofRequired { + $reflection = new ReflectionMethod($controller, $methodName); + $attributes = $reflection->getAttributes(VaultKeyProofRequired::class); + if ($attributes === []) { + return null; + } + + return $attributes[0]->newInstance(); + }//end attributeFor() + + /** + * Resolve the certificate whose public key verifies the proof. + * + * @param VaultKeyProofRequired $attribute The guard declaration + * @param string $userId The acting user + * + * @return string The subject suite's certificate PEM + * + * @throws KeyProofRequiredException When the subject suite cannot be resolved + */ + private function subjectCertificate(VaultKeyProofRequired $attribute, string $userId): string { + $subject = $attribute->getSubject(); + + try { + $suite = $this->resolveSubjectSuite(subject: $subject, userId: $userId); + } catch (KeyProofRequiredException $e) { + throw $e; + } catch (Throwable $e) { + throw new KeyProofRequiredException(message: 'No subject suite to verify against'); + } + + $certificate = $suite->getCertificate(); + if ($certificate === null || $certificate === '') { + throw new KeyProofRequiredException(message: 'Subject suite has no certificate'); + } + + return $certificate; + }//end subjectCertificate() + + /** + * Resolve the subject suite from the attribute's declaration. + * + * @param string $subject The subject declaration ('active' or 'routeParam:') + * @param string $userId The acting user + * + * @return EncryptionSuite + * + * @throws KeyProofRequiredException When a named suite is not the caller's own + */ + private function resolveSubjectSuite(string $subject, string $userId): EncryptionSuite { + if (str_starts_with($subject, 'routeParam:') === false) { + return $this->suiteService->getActiveSuite(ownerType: 'user', ownerId: $userId); + } + + $paramName = substr($subject, strlen('routeParam:')); + $suite = $this->suiteService->getSuite((string)$this->request->getParam($paramName, '')); + + // The proof must be over the OWNER's own key; a suite belonging to + // someone else (or to an application) can never be the subject of a + // user's self-service proof. + if ($suite->getOwnerType() !== 'user' || $suite->getOwnerId() !== $userId) { + throw new KeyProofRequiredException(message: 'Subject suite is not yours'); + } + + return $suite; + }//end resolveSubjectSuite() +}//end class diff --git a/lib/Service/VaultKeyProofService.php b/lib/Service/VaultKeyProofService.php new file mode 100644 index 000000000..a116161d9 --- /dev/null +++ b/lib/Service/VaultKeyProofService.php @@ -0,0 +1,249 @@ + + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\Keepiq\Service; + +use OCA\Keepiq\Exception\KeyProofRequiredException; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\IConfig; +use OCP\Security\ISecureRandom; + +/** + * Stateless issuance and verification of vault-key proofs. + */ +class VaultKeyProofService { + /** + * How long a challenge is valid, in seconds. + */ + private const TTL = 300; + + /** + * Stable public purpose identifiers. Both the guarded method's attribute and + * the client's challenge request name one of these, and the challenge is + * bound to it, so a proof for one operation cannot be presented to another. + */ + public const PURPOSE_COMPROMISE_RECOVERY = 'compromise-recovery'; + public const PURPOSE_UPDATE_PRIVATE_KEY = 'update-private-key'; + public const PURPOSE_COMPLETE_MIGRATION = 'complete-migration'; + public const PURPOSE_EMERGENCY_DESTROY = 'emergency-access-destroy'; + + /** + * The purposes a challenge may be issued for. + */ + public const ALLOWED_PURPOSES = [ + self::PURPOSE_COMPROMISE_RECOVERY, + self::PURPOSE_UPDATE_PRIVATE_KEY, + self::PURPOSE_COMPLETE_MIGRATION, + self::PURPOSE_EMERGENCY_DESTROY, + ]; + + /** + * Constructor. + * + * @param IConfig $config The system config, for the instance secret + * @param ISecureRandom $secureRandom The challenge randomness source + * @param ITimeFactory $timeFactory The clock, injected for testable expiry + * + * @return void + */ + public function __construct( + private IConfig $config, + private ISecureRandom $secureRandom, + private ITimeFactory $timeFactory, + ) { + }//end __construct() + + /** + * Issue a challenge for a caller and a purpose. + * + * @param string $userId The caller's user id + * @param string $purpose The operation the challenge authorises + * + * @return array{nonce:string,expiresAt:int} + */ + public function issueChallenge(string $userId, string $purpose): array { + $expiresAt = ($this->timeFactory->getTime() + self::TTL); + + $payload = $this->b64url((string)json_encode([ + 'r' => base64_encode($this->secureRandom->generate(18)), + 'u' => $userId, + 'p' => $purpose, + 'e' => $expiresAt, + ])); + + $nonce = $payload . '.' . $this->mac($payload); + + return ['nonce' => $nonce, 'expiresAt' => $expiresAt]; + }//end issueChallenge() + + /** + * Verify a proof, or throw. + * + * Every failure path throws the same KeyProofRequiredException with no + * indication of which check failed, so a caller learns only pass/fail. + * + * @param string $nonce The challenge the client echoed back + * @param string $signatureB64 The base64 signature over the bound payload + * @param string $certificatePem The subject suite's certificate (its public key) + * @param string $userId The caller, which the challenge must name + * @param string $purpose The operation, which the challenge must name + * @param string[] $boundValues The request parameter values the proof commits to + * + * @return void + * + * @throws KeyProofRequiredException When the proof is absent, stale, mis-bound or invalid + */ + public function verify( + string $nonce, + string $signatureB64, + string $certificatePem, + string $userId, + string $purpose, + array $boundValues, + ): void { + $claims = $this->authenticateNonce(nonce: $nonce); + + if (($claims['u'] ?? null) !== $userId || ($claims['p'] ?? null) !== $purpose) { + throw new KeyProofRequiredException(message: 'Challenge does not match this operation'); + } + + if ((int)($claims['e'] ?? 0) < $this->timeFactory->getTime()) { + throw new KeyProofRequiredException(message: 'Challenge has expired'); + } + + $publicKey = openssl_pkey_get_public($certificatePem); + if ($publicKey === false) { + throw new KeyProofRequiredException(message: 'Subject public key unreadable'); + } + + $signature = base64_decode($signatureB64, true); + if ($signature === false) { + throw new KeyProofRequiredException(message: 'Malformed proof'); + } + + $verified = openssl_verify( + $this->signedMessage(nonce: $nonce, boundValues: $boundValues), + $signature, + $publicKey, + OPENSSL_ALGO_SHA256 + ); + + if ($verified !== 1) { + throw new KeyProofRequiredException(message: 'Proof does not verify'); + } + }//end verify() + + /** + * The exact string a valid proof signs: the challenge, then the SHA-256 of + * each bound value in declared order, one per line. The client builds the + * identical string, so only named scalar parameters cross the language + * boundary — no JSON-canonicalisation agreement is needed. + * + * @param string $nonce The challenge + * @param string[] $boundValues The bound request-parameter values, in order + * + * @return string + */ + public function signedMessage(string $nonce, array $boundValues): string { + $lines = [$nonce]; + foreach ($boundValues as $value) { + $lines[] = hash('sha256', (string)$value); + } + + return implode("\n", $lines); + }//end signedMessage() + + /** + * Recover and authenticate a challenge's claims, or throw. + * + * @param string $nonce The challenge string + * + * @return array + * + * @throws KeyProofRequiredException When the challenge is absent or forged + */ + private function authenticateNonce(string $nonce): array { + if ($nonce === '') { + throw new KeyProofRequiredException(message: 'No challenge presented'); + } + + $parts = explode('.', $nonce); + if (count($parts) !== 2) { + throw new KeyProofRequiredException(message: 'Malformed challenge'); + } + + [$payload, $mac] = $parts; + if (hash_equals($this->mac($payload), $mac) === false) { + throw new KeyProofRequiredException(message: 'Challenge failed authentication'); + } + + $json = base64_decode(strtr($payload, '-_', '+/'), true); + if ($json === false) { + throw new KeyProofRequiredException(message: 'Unreadable challenge'); + } + + $claims = json_decode($json, true); + if (is_array($claims) === false) { + throw new KeyProofRequiredException(message: 'Unreadable challenge'); + } + + return $claims; + }//end authenticateNonce() + + /** + * The HMAC of a payload under the instance secret, base64url-encoded. + * + * @param string $payload The base64url payload + * + * @return string + */ + private function mac(string $payload): string { + $secret = $this->config->getSystemValueString('secret', ''); + return $this->b64url(hash_hmac('sha256', $payload, $secret, true)); + }//end mac() + + /** + * URL-safe, unpadded base64. + * + * @param string $raw The raw bytes + * + * @return string + */ + private function b64url(string $raw): string { + return rtrim(strtr(base64_encode($raw), '+/', '-_'), '='); + }//end b64url() +}//end class diff --git a/openspec/changes/harden-vault-key-material-guards/plan.json b/openspec/changes/harden-vault-key-material-guards/plan.json index 2ccf380d5..35e09c167 100644 --- a/openspec/changes/harden-vault-key-material-guards/plan.json +++ b/openspec/changes/harden-vault-key-material-guards/plan.json @@ -11,7 +11,7 @@ "id": 1, "num": "1.1", "title": "Create `lib/Attribute/VaultKeyProofRequired.php`: `#[Attribute(Attribute::TARGET_METHOD)]`, constructor `array $binds = []`, `string $subject = 'active'`; SPDX header per `contribute/HowToApplyALicense.md`", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -20,7 +20,7 @@ "id": 2, "num": "1.2", "title": "Create `lib/Service/VaultKeyProofService.php` with `issueChallenge(string $userId, string $purpose): array` returning `{nonce, expiresAt}` \u2014 nonce is `base64(ISecureRandom bytes) . '.' . HMAC(instance secret, random|uid|purpose|exp)`; no storage", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -29,7 +29,7 @@ "id": 3, "num": "1.3", "title": "Implement `VaultKeyProofService::verify(string $nonce, string $signature, string $publicKeyPem, string $userId, string $purpose, array $boundValues): void` \u2014 validate the HMAC, validate the expiry, rebuild the payload as `nonce || sha256(v1) || \u2026 || sha256(vn)` in declared order, verify with `openssl_verify` against the stored public key; throw a typed exception on every failure path with no distinction leaked to the caller", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -38,7 +38,7 @@ "id": 4, "num": "1.4", "title": "Do NOT use `ICacheFactory` for challenge state (design D5 \u2014 a null cache on a default install would make the guarded flows unusable). Assert this in review", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -47,7 +47,7 @@ "id": 5, "num": "1.5", "title": "Create `lib/Middleware/VaultKeyProofMiddleware.php` following `JwtAuthMiddleware`: `beforeController` reads the attribute via `new ReflectionMethod($controller, $methodName)`, resolves the subject suite (`'active'` \u2192 the session user's active suite via `EncryptionSuiteService::getActiveSuite`; `'routeParam:'` \u2192 `IRequest::getParam`), collects the bound values via `IRequest::getParam`, reads `X-Keepiq-Key-Proof`, and delegates to the service", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -56,7 +56,7 @@ "id": 6, "num": "1.6", "title": "Implement `afterException` returning `403` with `['error' => 'key_proof_required', 'message' => \u2026]`; re-throw anything that is not the guard's own exception, as `JwtAuthMiddleware` does", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -65,7 +65,7 @@ "id": 7, "num": "1.7", "title": "Middleware MUST NOT consult `IUserSession` backends, token scopes or `IPasswordConfirmationBackend` \u2014 no SSO/app-password carve-out (spec: *the guard is not waived*). Add an explanatory comment citing the NC `PasswordConfirmationMiddleware` bypasses this deliberately does not copy", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -74,7 +74,7 @@ "id": 8, "num": "1.8", "title": "Register in `lib/AppInfo/PlatformIntegrationRegistrar.php` alongside `JwtAuthMiddleware::class`", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -83,7 +83,7 @@ "id": 9, "num": "1.9", "title": "Run phpcs/phpstan/phpmd \u2014 watch `CouplingBetweenObjects` on the middleware; keep crypto in the service, which is also what makes it unit-testable", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -92,7 +92,7 @@ "id": 10, "num": "2.1", "title": "Add `proofChallenge(string $id)` to `EncryptionSuiteController` (`#[NoAdminRequired]`), returning `{nonce, expiresAt}` for the calling user and the requested purpose; validate suite ownership", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -101,7 +101,7 @@ "id": 11, "num": "2.2", "title": "Accept the purpose as a request parameter constrained to a known set (one per guarded operation); reject an unknown purpose", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -110,7 +110,7 @@ "id": 12, "num": "2.3", "title": "Register `['name' => 'encryptionSuite#proofChallenge', 'url' => '/api/v1/suites/{id}/proof-challenge', 'verb' => 'GET']` in `appinfo/routes.php`, before the SPA catch-all wildcard", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -119,7 +119,7 @@ "id": 13, "num": "2.4", "title": "The challenge endpoint itself MUST NOT carry `#[VaultKeyProofRequired]` \u2014 assert in the coverage test that it is on the deliberate-exclusion list", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -272,7 +272,7 @@ "id": 30, "num": "5.1", "title": "`EncryptionSuiteController::compromiseRecovery` \u2192 `#[VaultKeyProofRequired(binds: ['publicKey', 'encryptedPrivateKey'])]`", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -281,7 +281,7 @@ "id": 31, "num": "5.2", "title": "`EncryptionSuiteController::updatePrivateKey` \u2192 `#[VaultKeyProofRequired(binds: ['encryptedPrivateKey'], subject: 'routeParam:id')]`", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -290,7 +290,7 @@ "id": 32, "num": "5.3", "title": "`MigrationController::complete` \u2192 `#[VaultKeyProofRequired]` (defence in depth; the acknowledgement stays \u2014 they answer different questions)", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -299,7 +299,7 @@ "id": 33, "num": "5.4", "title": "`EmergencyAccessController::destroy` \u2192 `#[VaultKeyProofRequired]`", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -308,7 +308,7 @@ "id": 34, "num": "5.5", "title": "Create `tests/Unit/Controller/VaultKeyProofAttributesTest.php` in the shape of `RateLimitAttributesTest`: a provider enumerating the four methods with their expected `binds` and `subject`, asserting each by reflection; plus a deliberate-exclusion list (abort, proof-challenge) with the reason recorded per entry", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -317,7 +317,7 @@ "id": 35, "num": "6.1", "title": "`tests/Unit/Service/VaultKeyProofServiceTest.php`: valid proof passes; wrong key fails; altered bound value fails; altered nonce fails; expired nonce fails (injected `ITimeFactory`); wrong purpose fails; proof for one parameter set rejected against another", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -326,7 +326,7 @@ "id": 36, "num": "6.2", "title": "`tests/Unit/Middleware/VaultKeyProofMiddlewareTest.php`: attribute absent \u2192 pass-through; attribute present without header \u2192 403 `key_proof_required`; `subject: 'active'` and `'routeParam:id'` both resolve; `afterException` re-throws foreign exceptions", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] diff --git a/openspec/changes/harden-vault-key-material-guards/tasks.md b/openspec/changes/harden-vault-key-material-guards/tasks.md index a9b575948..6cdb6541a 100644 --- a/openspec/changes/harden-vault-key-material-guards/tasks.md +++ b/openspec/changes/harden-vault-key-material-guards/tasks.md @@ -8,22 +8,22 @@ Section 3 (abort) is independently useful and can be split into its own PR if th ## 1. Backend — The Guard Primitive -- [ ] 1.1 Create `lib/Attribute/VaultKeyProofRequired.php`: `#[Attribute(Attribute::TARGET_METHOD)]`, constructor `array $binds = []`, `string $subject = 'active'`; SPDX header per `contribute/HowToApplyALicense.md` -- [ ] 1.2 Create `lib/Service/VaultKeyProofService.php` with `issueChallenge(string $userId, string $purpose): array` returning `{nonce, expiresAt}` — nonce is `base64(ISecureRandom bytes) . '.' . HMAC(instance secret, random|uid|purpose|exp)`; no storage -- [ ] 1.3 Implement `VaultKeyProofService::verify(string $nonce, string $signature, string $publicKeyPem, string $userId, string $purpose, array $boundValues): void` — validate the HMAC, validate the expiry, rebuild the payload as `nonce || sha256(v1) || … || sha256(vn)` in declared order, verify with `openssl_verify` against the stored public key; throw a typed exception on every failure path with no distinction leaked to the caller -- [ ] 1.4 Do NOT use `ICacheFactory` for challenge state (design D5 — a null cache on a default install would make the guarded flows unusable). Assert this in review -- [ ] 1.5 Create `lib/Middleware/VaultKeyProofMiddleware.php` following `JwtAuthMiddleware`: `beforeController` reads the attribute via `new ReflectionMethod($controller, $methodName)`, resolves the subject suite (`'active'` → the session user's active suite via `EncryptionSuiteService::getActiveSuite`; `'routeParam:'` → `IRequest::getParam`), collects the bound values via `IRequest::getParam`, reads `X-Keepiq-Key-Proof`, and delegates to the service -- [ ] 1.6 Implement `afterException` returning `403` with `['error' => 'key_proof_required', 'message' => …]`; re-throw anything that is not the guard's own exception, as `JwtAuthMiddleware` does -- [ ] 1.7 Middleware MUST NOT consult `IUserSession` backends, token scopes or `IPasswordConfirmationBackend` — no SSO/app-password carve-out (spec: *the guard is not waived*). Add an explanatory comment citing the NC `PasswordConfirmationMiddleware` bypasses this deliberately does not copy -- [ ] 1.8 Register in `lib/AppInfo/PlatformIntegrationRegistrar.php` alongside `JwtAuthMiddleware::class` -- [ ] 1.9 Run phpcs/phpstan/phpmd — watch `CouplingBetweenObjects` on the middleware; keep crypto in the service, which is also what makes it unit-testable +- [x] 1.1 Create `lib/Attribute/VaultKeyProofRequired.php`: `#[Attribute(Attribute::TARGET_METHOD)]`, constructor `array $binds = []`, `string $subject = 'active'`; SPDX header per `contribute/HowToApplyALicense.md` +- [x] 1.2 Create `lib/Service/VaultKeyProofService.php` with `issueChallenge(string $userId, string $purpose): array` returning `{nonce, expiresAt}` — nonce is `base64(ISecureRandom bytes) . '.' . HMAC(instance secret, random|uid|purpose|exp)`; no storage +- [x] 1.3 Implement `VaultKeyProofService::verify(string $nonce, string $signature, string $publicKeyPem, string $userId, string $purpose, array $boundValues): void` — validate the HMAC, validate the expiry, rebuild the payload as `nonce || sha256(v1) || … || sha256(vn)` in declared order, verify with `openssl_verify` against the stored public key; throw a typed exception on every failure path with no distinction leaked to the caller +- [x] 1.4 Do NOT use `ICacheFactory` for challenge state (design D5 — a null cache on a default install would make the guarded flows unusable). Assert this in review +- [x] 1.5 Create `lib/Middleware/VaultKeyProofMiddleware.php` following `JwtAuthMiddleware`: `beforeController` reads the attribute via `new ReflectionMethod($controller, $methodName)`, resolves the subject suite (`'active'` → the session user's active suite via `EncryptionSuiteService::getActiveSuite`; `'routeParam:'` → `IRequest::getParam`), collects the bound values via `IRequest::getParam`, reads `X-Keepiq-Key-Proof`, and delegates to the service +- [x] 1.6 Implement `afterException` returning `403` with `['error' => 'key_proof_required', 'message' => …]`; re-throw anything that is not the guard's own exception, as `JwtAuthMiddleware` does +- [x] 1.7 Middleware MUST NOT consult `IUserSession` backends, token scopes or `IPasswordConfirmationBackend` — no SSO/app-password carve-out (spec: *the guard is not waived*). Add an explanatory comment citing the NC `PasswordConfirmationMiddleware` bypasses this deliberately does not copy +- [x] 1.8 Register in `lib/AppInfo/PlatformIntegrationRegistrar.php` alongside `JwtAuthMiddleware::class` +- [x] 1.9 Run phpcs/phpstan/phpmd — watch `CouplingBetweenObjects` on the middleware; keep crypto in the service, which is also what makes it unit-testable ## 2. Backend — Challenge Endpoint -- [ ] 2.1 Add `proofChallenge(string $id)` to `EncryptionSuiteController` (`#[NoAdminRequired]`), returning `{nonce, expiresAt}` for the calling user and the requested purpose; validate suite ownership -- [ ] 2.2 Accept the purpose as a request parameter constrained to a known set (one per guarded operation); reject an unknown purpose -- [ ] 2.3 Register `['name' => 'encryptionSuite#proofChallenge', 'url' => '/api/v1/suites/{id}/proof-challenge', 'verb' => 'GET']` in `appinfo/routes.php`, before the SPA catch-all wildcard -- [ ] 2.4 The challenge endpoint itself MUST NOT carry `#[VaultKeyProofRequired]` — assert in the coverage test that it is on the deliberate-exclusion list +- [x] 2.1 Add `proofChallenge(string $id)` to `EncryptionSuiteController` (`#[NoAdminRequired]`), returning `{nonce, expiresAt}` for the calling user and the requested purpose; validate suite ownership +- [x] 2.2 Accept the purpose as a request parameter constrained to a known set (one per guarded operation); reject an unknown purpose +- [x] 2.3 Register `['name' => 'encryptionSuite#proofChallenge', 'url' => '/api/v1/suites/{id}/proof-challenge', 'verb' => 'GET']` in `appinfo/routes.php`, before the SPA catch-all wildcard +- [x] 2.4 The challenge endpoint itself MUST NOT carry `#[VaultKeyProofRequired]` — assert in the coverage test that it is on the deliberate-exclusion list ## 3. Backend — Abort (independently mergeable) — IMPLEMENTED @@ -49,16 +49,16 @@ Section 3 (abort) is independently useful and can be split into its own PR if th ## 5. Apply The Guard (must not precede section 4) -- [ ] 5.1 `EncryptionSuiteController::compromiseRecovery` → `#[VaultKeyProofRequired(binds: ['publicKey', 'encryptedPrivateKey'])]` -- [ ] 5.2 `EncryptionSuiteController::updatePrivateKey` → `#[VaultKeyProofRequired(binds: ['encryptedPrivateKey'], subject: 'routeParam:id')]` -- [ ] 5.3 `MigrationController::complete` → `#[VaultKeyProofRequired]` (defence in depth; the acknowledgement stays — they answer different questions) -- [ ] 5.4 `EmergencyAccessController::destroy` → `#[VaultKeyProofRequired]` -- [ ] 5.5 Create `tests/Unit/Controller/VaultKeyProofAttributesTest.php` in the shape of `RateLimitAttributesTest`: a provider enumerating the four methods with their expected `binds` and `subject`, asserting each by reflection; plus a deliberate-exclusion list (abort, proof-challenge) with the reason recorded per entry +- [x] 5.1 `EncryptionSuiteController::compromiseRecovery` → `#[VaultKeyProofRequired(binds: ['publicKey', 'encryptedPrivateKey'])]` +- [x] 5.2 `EncryptionSuiteController::updatePrivateKey` → `#[VaultKeyProofRequired(binds: ['encryptedPrivateKey'], subject: 'routeParam:id')]` +- [x] 5.3 `MigrationController::complete` → `#[VaultKeyProofRequired]` (defence in depth; the acknowledgement stays — they answer different questions) +- [x] 5.4 `EmergencyAccessController::destroy` → `#[VaultKeyProofRequired]` +- [x] 5.5 Create `tests/Unit/Controller/VaultKeyProofAttributesTest.php` in the shape of `RateLimitAttributesTest`: a provider enumerating the four methods with their expected `binds` and `subject`, asserting each by reflection; plus a deliberate-exclusion list (abort, proof-challenge) with the reason recorded per entry ## 6. Tests -- [ ] 6.1 `tests/Unit/Service/VaultKeyProofServiceTest.php`: valid proof passes; wrong key fails; altered bound value fails; altered nonce fails; expired nonce fails (injected `ITimeFactory`); wrong purpose fails; proof for one parameter set rejected against another -- [ ] 6.2 `tests/Unit/Middleware/VaultKeyProofMiddlewareTest.php`: attribute absent → pass-through; attribute present without header → 403 `key_proof_required`; `subject: 'active'` and `'routeParam:id'` both resolve; `afterException` re-throws foreign exceptions +- [x] 6.1 `tests/Unit/Service/VaultKeyProofServiceTest.php`: valid proof passes; wrong key fails; altered bound value fails; altered nonce fails; expired nonce fails (injected `ITimeFactory`); wrong purpose fails; proof for one parameter set rejected against another +- [x] 6.2 `tests/Unit/Middleware/VaultKeyProofMiddlewareTest.php`: attribute absent → pass-through; attribute present without header → 403 `key_proof_required`; `subject: 'active'` and `'routeParam:id'` both resolve; `afterException` re-throws foreign exceptions - [ ] 6.3 Cross-implementation round-trip: sign with WebCrypto in a JS test, verify with `openssl_verify` in PHPUnit (config rule: *test cross-implementation encryption round-trips*) - [ ] 6.4 `MigrationServiceTest`: abort on an untouched migration; abort refused after a commit with the count reported; abort idempotent by status; emergency-access envelopes unchanged after abort; completed event NOT dispatched - [ ] 6.5 `EncryptionSuiteControllerTest` / `MigrationControllerTest` / `EmergencyAccessControllerTest`: the four guarded routes refuse without a proof and proceed with one diff --git a/tests/Unit/Controller/EncryptionSuiteControllerTest.php b/tests/Unit/Controller/EncryptionSuiteControllerTest.php index 3cbdfb99b..694fa609d 100644 --- a/tests/Unit/Controller/EncryptionSuiteControllerTest.php +++ b/tests/Unit/Controller/EncryptionSuiteControllerTest.php @@ -26,6 +26,7 @@ use OCA\Keepiq\Exception\ConflictException; use OCA\Keepiq\Service\EncryptionSuiteService; use OCA\Keepiq\Service\MigrationService; +use OCA\Keepiq\Service\VaultKeyProofService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Http; use OCP\IRequest; @@ -61,6 +62,13 @@ class EncryptionSuiteControllerTest extends TestCase { */ private MigrationService&MockObject $migrationService; + /** + * The mocked vault-key-proof service. + * + * @var VaultKeyProofService&MockObject + */ + private VaultKeyProofService&MockObject $proofService; + /** * The mocked user session. * @@ -80,6 +88,7 @@ protected function setUp(): void { $this->suiteService = $this->createMock(originalClassName: EncryptionSuiteService::class); $this->migrationService = $this->createMock(originalClassName: MigrationService::class); $this->userSession = $this->createMock(originalClassName: IUserSession::class); + $this->proofService = $this->createMock(originalClassName: VaultKeyProofService::class); $user = $this->createMock(originalClassName: IUser::class); $user->method('getUID')->willReturn('testuser'); @@ -90,6 +99,7 @@ protected function setUp(): void { suiteService: $this->suiteService, migrationService: $this->migrationService, userSession: $this->userSession, + proofService: $this->proofService, ); }//end setUp() diff --git a/tests/Unit/Controller/VaultKeyProofAttributesTest.php b/tests/Unit/Controller/VaultKeyProofAttributesTest.php new file mode 100644 index 000000000..3d612a4a5 --- /dev/null +++ b/tests/Unit/Controller/VaultKeyProofAttributesTest.php @@ -0,0 +1,157 @@ + + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\Keepiq\Tests\Unit\Controller; + +use OCA\Keepiq\Attribute\VaultKeyProofRequired; +use OCA\Keepiq\Controller\EmergencyAccessController; +use OCA\Keepiq\Controller\EncryptionSuiteController; +use OCA\Keepiq\Controller\MigrationController; +use OCA\Keepiq\Service\VaultKeyProofService; +use PHPUnit\Framework\TestCase; +use ReflectionMethod; + +/** + * Tests for #[VaultKeyProofRequired] coverage on the destructive endpoints. + */ +class VaultKeyProofAttributesTest extends TestCase { + /** + * Each destructive operation with its expected binds, subject and purpose. + * + * @return array + */ + public static function guardedMethodsProvider(): array { + return [ + 'compromise recovery' => [ + EncryptionSuiteController::class, + 'compromiseRecovery', + ['publicKey', 'encryptedPrivateKey'], + 'active', + VaultKeyProofService::PURPOSE_COMPROMISE_RECOVERY, + ], + 'update private key' => [ + EncryptionSuiteController::class, + 'updatePrivateKey', + ['encryptedPrivateKey'], + 'routeParam:id', + VaultKeyProofService::PURPOSE_UPDATE_PRIVATE_KEY, + ], + 'complete migration' => [ + MigrationController::class, + 'complete', + ['id'], + 'active', + VaultKeyProofService::PURPOSE_COMPLETE_MIGRATION, + ], + 'destroy emergency contact' => [ + EmergencyAccessController::class, + 'destroy', + ['id'], + 'active', + VaultKeyProofService::PURPOSE_EMERGENCY_DESTROY, + ], + ]; + }//end guardedMethodsProvider() + + /** + * @dataProvider guardedMethodsProvider + * + * @param class-string $class The controller class + * @param string $method The method name + * @param string[] $binds The expected bound parameters + * @param string $subject The expected subject resolution + * @param string $purpose The expected purpose + * + * @return void + */ + public function testDestructiveMethodCarriesTheGuard( + string $class, + string $method, + array $binds, + string $subject, + string $purpose, + ): void { + $attributes = (new ReflectionMethod($class, $method)) + ->getAttributes(VaultKeyProofRequired::class); + + $this->assertCount( + 1, + $attributes, + "$class::$method must carry exactly one #[VaultKeyProofRequired]" + ); + + $attribute = $attributes[0]->newInstance(); + $this->assertSame($binds, $attribute->getBinds(), "$class::$method binds"); + $this->assertSame($subject, $attribute->getSubject(), "$class::$method subject"); + $this->assertSame($purpose, $attribute->getPurpose(), "$class::$method purpose"); + }//end testDestructiveMethodCarriesTheGuard() + + /** + * Methods deliberately NOT guarded, with the reason each is safe. + * + * @return array + */ + public static function deliberatelyUnguardedProvider(): array { + return [ + 'proof challenge' => [ + EncryptionSuiteController::class, + 'proofChallenge', + 'Issuing a challenge grants nothing on its own; guarding it would be circular.', + ], + 'abort migration' => [ + MigrationController::class, + 'abort', + 'Abort is restorative — it returns the vault to the still-active old suite. ' + . 'Requiring a proof would leave a vault wedged by an unauthorised rotation wedged.', + ], + ]; + }//end deliberatelyUnguardedProvider() + + /** + * @dataProvider deliberatelyUnguardedProvider + * + * @param class-string $class The controller class + * @param string $method The method name + * @param string $reason Why it is safe unguarded (documentation) + * + * @return void + */ + public function testDeliberatelyUnguardedMethodHasNoGuard( + string $class, + string $method, + string $reason, + ): void { + $attributes = (new ReflectionMethod($class, $method)) + ->getAttributes(VaultKeyProofRequired::class); + + $this->assertCount(0, $attributes, "$class::$method must stay unguarded — $reason"); + }//end testDeliberatelyUnguardedMethodHasNoGuard() +}//end class diff --git a/tests/Unit/Middleware/VaultKeyProofMiddlewareTest.php b/tests/Unit/Middleware/VaultKeyProofMiddlewareTest.php new file mode 100644 index 000000000..41ff5e374 --- /dev/null +++ b/tests/Unit/Middleware/VaultKeyProofMiddlewareTest.php @@ -0,0 +1,184 @@ + pass-through, present -> verify), + * subject resolution (active vs routeParam, foreign suite refused), and the + * afterException mapping (guard exception -> 403 key_proof_required; foreign + * exception re-thrown). The signature crypto itself lives in + * VaultKeyProofServiceTest; here the service is mocked. + * + * @category Test + * @package OCA\Keepiq\Tests\Unit\Middleware + * + * @author Conduction Development Team + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\Keepiq\Tests\Unit\Middleware; + +use OCA\Keepiq\Attribute\VaultKeyProofRequired; +use OCA\Keepiq\Db\EncryptionSuite; +use OCA\Keepiq\Exception\KeyProofRequiredException; +use OCA\Keepiq\Middleware\VaultKeyProofMiddleware; +use OCA\Keepiq\Service\EncryptionSuiteService; +use OCA\Keepiq\Service\VaultKeyProofService; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http; +use OCP\IRequest; +use OCP\IUser; +use OCP\IUserSession; +use PHPUnit\Framework\TestCase; +use RuntimeException; + +/** + * A controller fixture exposing guarded and unguarded methods. + */ +class GuardFixtureController extends Controller { + #[VaultKeyProofRequired(binds: ['encryptedPrivateKey'], subject: 'active', purpose: 'compromise-recovery')] + public function guardedActive(): void { + } + + #[VaultKeyProofRequired(subject: 'routeParam:id', purpose: 'update-private-key')] + public function guardedRouteParam(): void { + } + + public function unguarded(): void { + } +}//end class + +/** + * Tests for VaultKeyProofMiddleware. + */ +class VaultKeyProofMiddlewareTest extends TestCase { + private IRequest $request; + private IUserSession $userSession; + private EncryptionSuiteService $suiteService; + private VaultKeyProofService $proofService; + private VaultKeyProofMiddleware $middleware; + private GuardFixtureController $controller; + + /** + * @return void + */ + protected function setUp(): void { + parent::setUp(); + + $this->request = $this->createMock(IRequest::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->suiteService = $this->createMock(EncryptionSuiteService::class); + $this->proofService = $this->createMock(VaultKeyProofService::class); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('alice'); + $this->userSession->method('getUser')->willReturn($user); + + $this->middleware = new VaultKeyProofMiddleware( + request: $this->request, + userSession: $this->userSession, + suiteService: $this->suiteService, + proofService: $this->proofService, + ); + + $this->controller = new GuardFixtureController('keepiq', $this->request); + }//end setUp() + + public function testAnUnguardedMethodIsPassedThrough(): void { + $this->proofService->expects($this->never())->method('verify'); + $this->middleware->beforeController($this->controller, 'unguarded'); + $this->addToAssertionCount(1); + }//end testAnUnguardedMethodIsPassedThrough() + + public function testActiveSubjectResolvesAndTheProofIsVerifiedWithTheBoundValues(): void { + $this->suiteService->method('getActiveSuite') + ->with('user', 'alice') + ->willReturn($this->suiteWithCertificate('CERT-PEM')); + + $this->request->method('getHeader')->willReturnMap([ + ['X-Keepiq-Key-Proof-Nonce', 'the-nonce'], + ['X-Keepiq-Key-Proof', 'the-sig'], + ]); + $this->request->method('getParam')->willReturnMap([ + ['encryptedPrivateKey', '', 'ENVELOPE'], + ]); + + $this->proofService->expects($this->once())->method('verify') + ->with( + 'the-nonce', + 'the-sig', + 'CERT-PEM', + 'alice', + 'compromise-recovery', + ['ENVELOPE'], + ); + + $this->middleware->beforeController($this->controller, 'guardedActive'); + }//end testActiveSubjectResolvesAndTheProofIsVerifiedWithTheBoundValues() + + public function testRouteParamSubjectRefusesAForeignSuite(): void { + $foreign = $this->suiteWithCertificate('CERT-PEM'); + $foreign->setOwnerType('user'); + $foreign->setOwnerId('bob'); + $this->suiteService->method('getSuite')->willReturn($foreign); + $this->request->method('getParam')->willReturnMap([['id', '', 'suite-x']]); + + $this->proofService->expects($this->never())->method('verify'); + $this->expectException(KeyProofRequiredException::class); + $this->middleware->beforeController($this->controller, 'guardedRouteParam'); + }//end testRouteParamSubjectRefusesAForeignSuite() + + public function testAFailedProofPropagatesAsTheGuardException(): void { + $this->suiteService->method('getActiveSuite')->willReturn($this->suiteWithCertificate('CERT-PEM')); + $this->request->method('getHeader')->willReturn(''); + $this->request->method('getParam')->willReturn(''); + $this->proofService->method('verify') + ->willThrowException(new KeyProofRequiredException('nope')); + + $this->expectException(KeyProofRequiredException::class); + $this->middleware->beforeController($this->controller, 'guardedActive'); + }//end testAFailedProofPropagatesAsTheGuardException() + + public function testAfterExceptionMapsTheGuardExceptionTo403(): void { + $response = $this->middleware->afterException( + $this->controller, + 'guardedActive', + new KeyProofRequiredException('need a proof') + ); + + $this->assertSame(Http::STATUS_FORBIDDEN, $response->getStatus()); + $this->assertSame('key_proof_required', $response->getData()['error']); + }//end testAfterExceptionMapsTheGuardExceptionTo403() + + public function testAfterExceptionRethrowsAForeignException(): void { + $this->expectException(RuntimeException::class); + $this->middleware->afterException( + $this->controller, + 'guardedActive', + new RuntimeException('something else') + ); + }//end testAfterExceptionRethrowsAForeignException() + + /** + * A user-owned suite carrying the given certificate. + * + * @param string $certificate The certificate PEM + * + * @return EncryptionSuite + */ + private function suiteWithCertificate(string $certificate): EncryptionSuite { + $suite = new EncryptionSuite(); + $suite->setId('suite-alice'); + $suite->setOwnerType('user'); + $suite->setOwnerId('alice'); + $suite->setCertificate($certificate); + return $suite; + }//end suiteWithCertificate() +}//end class diff --git a/tests/Unit/Service/VaultKeyProofServiceTest.php b/tests/Unit/Service/VaultKeyProofServiceTest.php new file mode 100644 index 000000000..c9c2a71cb --- /dev/null +++ b/tests/Unit/Service/VaultKeyProofServiceTest.php @@ -0,0 +1,165 @@ + + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\Keepiq\Tests\Unit\Service; + +use OCA\Keepiq\Exception\KeyProofRequiredException; +use OCA\Keepiq\Service\VaultKeyProofService; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\IConfig; +use OCP\Security\ISecureRandom; +use PHPUnit\Framework\TestCase; + +/** + * Tests for VaultKeyProofService. + */ +class VaultKeyProofServiceTest extends TestCase { + private VaultKeyProofService $service; + private int $now = 1000; + private string $privateKeyPem = ''; + private string $publicKeyPem = ''; + private string $otherPublicKeyPem = ''; + + private const PURPOSE = 'compromise-recovery'; + + /** + * @return void + */ + protected function setUp(): void { + parent::setUp(); + + $config = $this->createMock(IConfig::class); + $config->method('getSystemValueString')->willReturn('the-instance-secret'); + + $random = $this->createMock(ISecureRandom::class); + $random->method('generate')->willReturn('deterministic-random'); + + $time = $this->createMock(ITimeFactory::class); + $time->method('getTime')->willReturnCallback(fn () => $this->now); + + $this->service = new VaultKeyProofService( + config: $config, + secureRandom: $random, + timeFactory: $time, + ); + + [$this->privateKeyPem, $this->publicKeyPem] = $this->makeKeypair(); + [, $this->otherPublicKeyPem] = $this->makeKeypair(); + }//end setUp() + + public function testAValidProofVerifies(): void { + $nonce = $this->service->issueChallenge('alice', self::PURPOSE)['nonce']; + $sig = $this->sign($this->service->signedMessage($nonce, ['pub', 'env']), $this->privateKeyPem); + + $this->expectNotToPerformAssertions(); + $this->service->verify($nonce, $sig, $this->publicKeyPem, 'alice', self::PURPOSE, ['pub', 'env']); + }//end testAValidProofVerifies() + + public function testAProofSignedWithTheWrongKeyIsRefused(): void { + $nonce = $this->service->issueChallenge('alice', self::PURPOSE)['nonce']; + $sig = $this->sign($this->service->signedMessage($nonce, ['pub', 'env']), $this->privateKeyPem); + + $this->expectException(KeyProofRequiredException::class); + $this->service->verify($nonce, $sig, $this->otherPublicKeyPem, 'alice', self::PURPOSE, ['pub', 'env']); + }//end testAProofSignedWithTheWrongKeyIsRefused() + + public function testAnAlteredBoundValueIsRefused(): void { + $nonce = $this->service->issueChallenge('alice', self::PURPOSE)['nonce']; + $sig = $this->sign($this->service->signedMessage($nonce, ['pub', 'env']), $this->privateKeyPem); + + $this->expectException(KeyProofRequiredException::class); + // Same signature, but the server sees a different bound value. + $this->service->verify($nonce, $sig, $this->publicKeyPem, 'alice', self::PURPOSE, ['pub', 'TAMPERED']); + }//end testAnAlteredBoundValueIsRefused() + + public function testATamperedNonceIsRefused(): void { + $nonce = $this->service->issueChallenge('alice', self::PURPOSE)['nonce']; + $sig = $this->sign($this->service->signedMessage($nonce, []), $this->privateKeyPem); + + $tampered = $nonce . 'x'; + $this->expectException(KeyProofRequiredException::class); + $this->service->verify($tampered, $sig, $this->publicKeyPem, 'alice', self::PURPOSE, []); + }//end testATamperedNonceIsRefused() + + public function testAnExpiredChallengeIsRefused(): void { + $nonce = $this->service->issueChallenge('alice', self::PURPOSE)['nonce']; + $sig = $this->sign($this->service->signedMessage($nonce, []), $this->privateKeyPem); + + // Advance well past the 300s TTL. + $this->now += 10000; + + $this->expectException(KeyProofRequiredException::class); + $this->service->verify($nonce, $sig, $this->publicKeyPem, 'alice', self::PURPOSE, []); + }//end testAnExpiredChallengeIsRefused() + + public function testAProofForAnotherPurposeIsRefused(): void { + $nonce = $this->service->issueChallenge('alice', self::PURPOSE)['nonce']; + $sig = $this->sign($this->service->signedMessage($nonce, []), $this->privateKeyPem); + + $this->expectException(KeyProofRequiredException::class); + $this->service->verify($nonce, $sig, $this->publicKeyPem, 'alice', 'update-private-key', []); + }//end testAProofForAnotherPurposeIsRefused() + + public function testAProofForAnotherUserIsRefused(): void { + $nonce = $this->service->issueChallenge('alice', self::PURPOSE)['nonce']; + $sig = $this->sign($this->service->signedMessage($nonce, []), $this->privateKeyPem); + + $this->expectException(KeyProofRequiredException::class); + $this->service->verify($nonce, $sig, $this->publicKeyPem, 'mallory', self::PURPOSE, []); + }//end testAProofForAnotherUserIsRefused() + + public function testAMissingProofIsRefused(): void { + $this->expectException(KeyProofRequiredException::class); + $this->service->verify('', '', $this->publicKeyPem, 'alice', self::PURPOSE, []); + }//end testAMissingProofIsRefused() + + /** + * Sign a message with RSASSA-PKCS1-v1_5 SHA-256, the same scheme WebCrypto + * produces, and return it base64-encoded as the client would send it. + * + * @param string $message The message to sign + * @param string $privateKeyPem The signer's private key + * + * @return string + */ + private function sign(string $message, string $privateKeyPem): string { + openssl_sign($message, $signature, $privateKeyPem, OPENSSL_ALGO_SHA256); + return base64_encode($signature); + }//end sign() + + /** + * Generate an RSA keypair, returned as [privatePem, publicPem]. + * + * @return array{0:string,1:string} + */ + private function makeKeypair(): array { + $res = openssl_pkey_new([ + 'private_key_bits' => 2048, + 'private_key_type' => OPENSSL_KEYTYPE_RSA, + ]); + openssl_pkey_export($res, $privatePem); + $publicPem = openssl_pkey_get_details($res)['key']; + return [$privatePem, $publicPem]; + }//end makeKeypair() +}//end class From 2d445d4839ae79c66a9a1ca12fc322ed5af9a1d8 Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 10 Sep 2026 16:07:30 +0200 Subject: [PATCH 08/48] feat(vault-key-proof): client proof + wire the password-in-hand flows (#673) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The client half of the guard. `proveMasterPassword` (reauth.js) decrypts the suite envelope with the freshly entered master password, re-imports the PKCS#8 bytes for SIGNING (RSASSA-PKCS1-v1_5 SHA-256 — a distinct capability from the session key, which is non-extractable and decrypt-only and so cannot sign), signs the challenge bound to the operation's parameters, and discards every derived key. `keyProof.js` fetches a challenge and returns the two proof headers, so the four flows do not each re-implement it. Wired the flows that already hold the master password, so they keep working against the now-guarded routes: - compromise-recovery START — proof over the OLD key (old password) bound to the new key material; - migration COMPLETE on the initiate path — proof over the NEW key (new password) bound to the migration id; - routine password change (updatePrivateKey) — proof over the current key (old password) bound to the new envelope; the old key is already materialised there, so no extra prompt. Tested: proveMasterPassword round-trips under RSASSA-PKCS1-v1_5 (verifies over the exact server-rebuilt message; wrong password throws before signing; a changed bound value fails verification), and the session key is pinned non-extractable / decrypt-only. Store tests still green. prettier + eslint clean (0 errors). REMAINING (tracked in tasks §4.7-4.8): the emergency-contact delete and the resume-path completion both need a master-password prompt at the point of action (no password in hand there), plus the 403 re-enter-and-retry UX. Until those land, those two paths return 403 by design. Refs #673 Assisted-by: ClaudeCode:claude-opus-5 --- .../plan.json | 16 +- .../harden-vault-key-material-guards/tasks.md | 16 +- src/crypto/keyProof.js | 68 +++++++++ src/crypto/reauth.js | 84 +++++++++++ src/store/modules/encryptionSuite.js | 65 ++++++++- tests/vitest/proveMasterPassword.spec.js | 138 ++++++++++++++++++ 6 files changed, 366 insertions(+), 21 deletions(-) create mode 100644 src/crypto/keyProof.js create mode 100644 tests/vitest/proveMasterPassword.spec.js diff --git a/openspec/changes/harden-vault-key-material-guards/plan.json b/openspec/changes/harden-vault-key-material-guards/plan.json index 35e09c167..67f5306d2 100644 --- a/openspec/changes/harden-vault-key-material-guards/plan.json +++ b/openspec/changes/harden-vault-key-material-guards/plan.json @@ -200,7 +200,7 @@ "id": 22, "num": "4.1", "title": "Add `proveMasterPassword(encryptedPrivateKey, masterPassword, nonce, boundValues)` to `src/crypto/reauth.js`: decrypt the envelope via `decryptPrivateKey` (`src/crypto/aes.js:79`), re-import the PKCS#8 bytes with `['sign']` usage, sign `nonce || sha256(v1) || \u2026`, return the signature", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -209,7 +209,7 @@ "id": 23, "num": "4.2", "title": "Discard the derived AES key, the raw PKCS#8 bytes and the signing key immediately after signing; never return, store or cache them (spec: *the signing key does not outlive the proof*). Keep `verifyMasterPassword` as-is for the three existing advisory call sites \u2014 they are out of scope for this change", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -218,7 +218,7 @@ "id": 24, "num": "4.3", "title": "Confirm the signing key is imported with `['sign']` only and is NOT the session `CryptoKey`; add a unit test asserting the session key (`src/crypto/rsa.js:61-66`) remains non-extractable and `['decrypt']`-only", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -227,7 +227,7 @@ "id": 25, "num": "4.4", "title": "Add a shared client helper that fetches a challenge, prompts for the master password, produces the proof, and sets the `X-Keepiq-Key-Proof` header \u2014 so the four call sites do not each re-implement it", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -236,7 +236,7 @@ "id": 26, "num": "4.5", "title": "Wire `src/components/CompromiseRecoveryForm.vue` (recovery start, and the completion call) through the helper", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -245,7 +245,7 @@ "id": 27, "num": "4.6", "title": "Wire the routine master-password change flow through the helper; verify the old private key is materialised at that point (design \"Risks\" \u2014 if it is not, stop and raise before proceeding)", - "status": "pending", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -253,7 +253,7 @@ { "id": 28, "num": "4.7", - "title": "Wire the emergency-contact delete action in `src/views/EmergencyAccessView.vue` / `src/store/modules/emergencyAccess.js` through the helper", + "title": "REMAINING \u2014 wire the emergency-contact delete through the helper. Needs a master-password prompt at the delete point (the delete action has no password in hand), so it is a UI change, not just a store change", "status": "pending", "spec_ref": null, "acceptance_criteria": [], @@ -262,7 +262,7 @@ { "id": 29, "num": "4.8", - "title": "Handle `403 key_proof_required` as \"re-enter your master password and retry\", not as a terminal error", + "title": "REMAINING \u2014 handle `403 key_proof_required` as a re-enter-and-retry prompt. Also covers the resume path's completion: resume asks only for the OLD password, but completion's proof is over the NEW suite key, so a current-password prompt is needed there too", "status": "pending", "spec_ref": null, "acceptance_criteria": [], diff --git a/openspec/changes/harden-vault-key-material-guards/tasks.md b/openspec/changes/harden-vault-key-material-guards/tasks.md index 6cdb6541a..68323a159 100644 --- a/openspec/changes/harden-vault-key-material-guards/tasks.md +++ b/openspec/changes/harden-vault-key-material-guards/tasks.md @@ -38,14 +38,14 @@ Section 3 (abort) is independently useful and can be split into its own PR if th ## 4. Frontend — Producing the Proof -- [ ] 4.1 Add `proveMasterPassword(encryptedPrivateKey, masterPassword, nonce, boundValues)` to `src/crypto/reauth.js`: decrypt the envelope via `decryptPrivateKey` (`src/crypto/aes.js:79`), re-import the PKCS#8 bytes with `['sign']` usage, sign `nonce || sha256(v1) || …`, return the signature -- [ ] 4.2 Discard the derived AES key, the raw PKCS#8 bytes and the signing key immediately after signing; never return, store or cache them (spec: *the signing key does not outlive the proof*). Keep `verifyMasterPassword` as-is for the three existing advisory call sites — they are out of scope for this change -- [ ] 4.3 Confirm the signing key is imported with `['sign']` only and is NOT the session `CryptoKey`; add a unit test asserting the session key (`src/crypto/rsa.js:61-66`) remains non-extractable and `['decrypt']`-only -- [ ] 4.4 Add a shared client helper that fetches a challenge, prompts for the master password, produces the proof, and sets the `X-Keepiq-Key-Proof` header — so the four call sites do not each re-implement it -- [ ] 4.5 Wire `src/components/CompromiseRecoveryForm.vue` (recovery start, and the completion call) through the helper -- [ ] 4.6 Wire the routine master-password change flow through the helper; verify the old private key is materialised at that point (design "Risks" — if it is not, stop and raise before proceeding) -- [ ] 4.7 Wire the emergency-contact delete action in `src/views/EmergencyAccessView.vue` / `src/store/modules/emergencyAccess.js` through the helper -- [ ] 4.8 Handle `403 key_proof_required` as "re-enter your master password and retry", not as a terminal error +- [x] 4.1 Add `proveMasterPassword(encryptedPrivateKey, masterPassword, nonce, boundValues)` to `src/crypto/reauth.js`: decrypt the envelope via `decryptPrivateKey` (`src/crypto/aes.js:79`), re-import the PKCS#8 bytes with `['sign']` usage, sign `nonce || sha256(v1) || …`, return the signature +- [x] 4.2 Discard the derived AES key, the raw PKCS#8 bytes and the signing key immediately after signing; never return, store or cache them (spec: *the signing key does not outlive the proof*). Keep `verifyMasterPassword` as-is for the three existing advisory call sites — they are out of scope for this change +- [x] 4.3 Confirm the signing key is imported with `['sign']` only and is NOT the session `CryptoKey`; add a unit test asserting the session key (`src/crypto/rsa.js:61-66`) remains non-extractable and `['decrypt']`-only +- [x] 4.4 Add a shared client helper that fetches a challenge, prompts for the master password, produces the proof, and sets the `X-Keepiq-Key-Proof` header — so the four call sites do not each re-implement it +- [x] 4.5 Wire `src/components/CompromiseRecoveryForm.vue` (recovery start, and the completion call) through the helper +- [x] 4.6 Wire the routine master-password change flow through the helper; verify the old private key is materialised at that point (design "Risks" — if it is not, stop and raise before proceeding) +- [ ] 4.7 REMAINING — wire the emergency-contact delete through the helper. Needs a master-password prompt at the delete point (the delete action has no password in hand), so it is a UI change, not just a store change +- [ ] 4.8 REMAINING — handle `403 key_proof_required` as a re-enter-and-retry prompt. Also covers the resume path's completion: resume asks only for the OLD password, but completion's proof is over the NEW suite key, so a current-password prompt is needed there too ## 5. Apply The Guard (must not precede section 4) diff --git a/src/crypto/keyProof.js b/src/crypto/keyProof.js new file mode 100644 index 000000000..332ba223b --- /dev/null +++ b/src/crypto/keyProof.js @@ -0,0 +1,68 @@ +/** + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * Client helper for VaultKeyProof: fetch a challenge for a guarded operation, + * sign it with the master password, and return the headers the guarded request + * must carry. The four destructive flows use this rather than each re-fetching + * and re-signing, and so that the header names live in exactly one place. + * + * The master password is used only to sign and is never sent; the proof headers + * carry the challenge and the signature, nothing else. See the vault-key-proof + * spec and src/crypto/reauth.js#proveMasterPassword. + */ + +import axios from '@nextcloud/axios' +import { generateUrl } from '@nextcloud/router' +import { proveMasterPassword } from './reauth.js' + +/** The header carrying the challenge the proof was made over. */ +export const HEADER_NONCE = 'X-Keepiq-Key-Proof-Nonce' +/** The header carrying the base64 signature. */ +export const HEADER_PROOF = 'X-Keepiq-Key-Proof' + +/** + * The stable purpose identifiers, matching VaultKeyProofService::PURPOSE_*. + */ +export const PROOF_PURPOSE = { + COMPROMISE_RECOVERY: 'compromise-recovery', + UPDATE_PRIVATE_KEY: 'update-private-key', + COMPLETE_MIGRATION: 'complete-migration', + EMERGENCY_DESTROY: 'emergency-access-destroy', +} + +/** + * Build the proof headers for a guarded request. + * + * @param {object} params The proof parameters. + * @param {string} params.suiteId The suite the proof is made with (for the challenge URL). + * @param {string} params.purpose One of PROOF_PURPOSE. + * @param {string} params.encryptedPrivateKey The subject suite's stored AES envelope. + * @param {string} params.masterPassword The freshly entered master password. + * @param {string[]} [params.boundValues] The request parameters the proof commits to, in order. + * @return {Promise>} Headers to merge into the guarded request. + */ +export async function buildKeyProofHeaders({ + suiteId, + purpose, + encryptedPrivateKey, + masterPassword, + boundValues = [], +}) { + const { data } = await axios.get( + generateUrl(`/apps/keepiq/api/v1/suites/${suiteId}/proof-challenge`), + { params: { purpose } }, + ) + + const signature = await proveMasterPassword( + encryptedPrivateKey, + masterPassword, + data.nonce, + boundValues, + ) + + return { + [HEADER_NONCE]: data.nonce, + [HEADER_PROOF]: signature, + } +} diff --git a/src/crypto/reauth.js b/src/crypto/reauth.js index d4d900f88..b2927eb50 100644 --- a/src/crypto/reauth.js +++ b/src/crypto/reauth.js @@ -19,6 +19,90 @@ import { decryptPrivateKey } from './aes.js' +/** + * The exact string the server verifies (VaultKeyProofService::signedMessage): + * the challenge, then the lowercase-hex SHA-256 of each bound value in the + * declared order, one per line. Only named scalar values cross the boundary, so + * no JSON-canonicalisation agreement is needed. + * + * @param {string} nonce The server-issued challenge. + * @param {string[]} boundValues The bound request-parameter values, in order. + * @return {Promise} The UTF-8 bytes of the message to sign. + */ +async function signedMessageBytes(nonce, boundValues) { + const encoder = new TextEncoder() + const lines = [nonce] + for (const value of boundValues) { + const digest = await crypto.subtle.digest( + 'SHA-256', + encoder.encode(String(value ?? '')), + ) + const hex = Array.from(new Uint8Array(digest)) + .map((b) => b.toString(16).padStart(2, '0')) + .join('') + lines.push(hex) + } + return encoder.encode(lines.join('\n')) +} + +/** + * Produce a VaultKeyProof: a signature over a server-issued challenge, made + * with the suite private key, proving master-password knowledge to the server. + * + * This is a SIGNATURE, never a decryption — the session key is decrypt-only, so + * only a key re-derived from the freshly entered master password can sign, which + * is exactly the guarantee the server relies on. The derived AES key, the PEM, + * and the signing key are all discarded the moment signing resolves; nothing is + * returned but the signature, and nothing is stored. + * + * @param {string} encryptedPrivateKey The stored AES envelope for the subject suite. + * @param {string} masterPassword The freshly entered master password. + * @param {string} nonce The challenge from GET /suites/{id}/proof-challenge. + * @param {string[]} boundValues The request parameters the proof commits to, in order. + * @return {Promise} The base64 signature to send as X-Keepiq-Key-Proof. + */ +export async function proveMasterPassword( + encryptedPrivateKey, + masterPassword, + nonce, + boundValues = [], +) { + // Decrypt the envelope with the master password to recover the PKCS#8 PEM. + // A wrong password throws here (AES-GCM tag mismatch) before anything signs. + const pem = await decryptPrivateKey(encryptedPrivateKey, masterPassword) + + const pkcs8 = Uint8Array.from( + atob( + pem + .replace(/-----BEGIN PRIVATE KEY-----/, '') + .replace(/-----END PRIVATE KEY-----/, '') + .replace(/-----BEGIN RSA PRIVATE KEY-----/, '') + .replace(/-----END RSA PRIVATE KEY-----/, '') + .replace(/\s/g, ''), + ), + (c) => c.charCodeAt(0), + ) + + // Re-import for SIGNING specifically — a distinct capability from the + // session key, which is imported non-extractable and ['decrypt'] only. + const signingKey = await crypto.subtle.importKey( + 'pkcs8', + pkcs8, + { name: 'RSASSA-PKCS1-v1_5', hash: 'SHA-256' }, + false, + ['sign'], + ) + + const message = await signedMessageBytes(nonce, boundValues) + const signature = await crypto.subtle.sign( + 'RSASSA-PKCS1-v1_5', + signingKey, + message, + ) + + return btoa(String.fromCharCode(...new Uint8Array(signature))) +} + /** * Verify the master password by attempting to decrypt the private-key blob. * diff --git a/src/store/modules/encryptionSuite.js b/src/store/modules/encryptionSuite.js index 55b720d2b..979f61f56 100644 --- a/src/store/modules/encryptionSuite.js +++ b/src/store/modules/encryptionSuite.js @@ -8,6 +8,7 @@ import { importPrivateKey, importPublicKey, } from '../../crypto/index.js' +import { buildKeyProofHeaders, PROOF_PURPOSE } from '../../crypto/keyProof.js' import { createMigrationRunner } from '../../migration/driver.js' import { MIGRATION_STORES } from '../../migration/pipeline.js' import { onVaultLock, useSessionStore } from './session.js' @@ -151,12 +152,24 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { newPassword, ) + // Replacing the envelope is guarded: prove possession of the current + // key (the old master password) over the new envelope. The old key is + // already materialised above, so this adds no extra prompt. + const proof = await buildKeyProofHeaders({ + suiteId: session.suiteId, + purpose: PROOF_PURPOSE.UPDATE_PRIVATE_KEY, + encryptedPrivateKey: session.encryptedPrivateKey, + masterPassword: oldPassword, + boundValues: [newEncryptedPk], + }) + // Update on server. await axios.put( generateUrl( `/apps/keepiq/api/v1/suites/${session.suiteId}/private-key`, ), { encryptedPrivateKey: newEncryptedPk }, + { headers: proof }, ) session.encryptedPrivateKey = newEncryptedPk @@ -203,12 +216,25 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { this.migrationFailures = [] this.migrationDroppedVersions = 0 + // Starting a rotation is guarded: prove possession of the OLD suite + // key (i.e. the old master password) over the submitted new key + // material, so a stolen session cannot begin a hostile rotation. + const session = useSessionStore() + const startProof = await buildKeyProofHeaders({ + suiteId: session.suiteId, + purpose: PROOF_PURPOSE.COMPROMISE_RECOVERY, + encryptedPrivateKey: session.encryptedPrivateKey, + masterPassword: oldPassword, + boundValues: [publicKeyPem, newEncryptedPk], + }) + const response = await axios.post( generateUrl('/apps/keepiq/api/v1/suites/compromise-recovery'), { publicKey: publicKeyPem, encryptedPrivateKey: newEncryptedPk, }, + { headers: startProof }, ) this.migrationStatus = response.data.migration @@ -224,7 +250,6 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { // The key material is the pair generated above, so this is the same // binding createSuite performs on first-time setup, not a // re-derivation from anything the server sent. - const session = useSessionStore() session.cryptoKey = await importPrivateKey(newPrivateKeyPem) session.encryptedPrivateKey = newEncryptedPk session.certificate = response.data.newSuite?.certificate ?? null @@ -255,7 +280,21 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { // for the terminal step. The premature complete() that used to sit // here reported success five lines after initiating, before a single // record had been touched. - await this.finaliseMigration(response.data.migration.id, outcome) + // Completion is guarded too. The session is now bound to the NEW + // suite, so the proof is made with the new key and the new master + // password (both in hand here), bound to the migration id. + const completeProof = await buildKeyProofHeaders({ + suiteId: session.suiteId, + purpose: PROOF_PURPOSE.COMPLETE_MIGRATION, + encryptedPrivateKey: newEncryptedPk, + masterPassword: newPassword, + boundValues: [response.data.migration.id], + }) + await this.finaliseMigration( + response.data.migration.id, + outcome, + completeProof, + ) return outcome }, @@ -651,16 +690,22 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { * * @param {string} migrationId The migration ID. * @param {object} outcome The run outcome from runMigration. + * @param {Record|null} proofHeaders Vault-key-proof headers for completion. * @return {Promise<{finalised: boolean, needsAcknowledgement: boolean, message: string|null}>} * Whether the migration terminated, and why not if it did not. * @spec openspec/changes/restore-suite-migration-loop/specs/encryption-suites/spec.md#requirement-migration-covers-every-suite-bound-store */ - async finaliseMigration(migrationId, outcome) { + async finaliseMigration(migrationId, outcome, proofHeaders = null) { this.migrationNeedsAcknowledgement = false this.migrationBlockedMessage = null try { - await this.completeMigration(migrationId, outcome.failed > 0) + await this.completeMigration( + migrationId, + outcome.failed > 0, + null, + proofHeaders, + ) return { finalised: true, needsAcknowledgement: false, @@ -741,10 +786,16 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { * @param {string} migrationId The migration ID. * @param {boolean} hasErrors Whether any record failed. * @param {number|null} acceptUnrecoverable Losses the user has accepted. + * @param {Record|null} proofHeaders Vault-key-proof headers for completion. * @return {Promise} The completion response body. * @spec openspec/changes/restore-suite-migration-loop/specs/encryption-suites/spec.md#requirement-a-migration-always-has-a-way-to-terminate */ - async completeMigration(migrationId, hasErrors, acceptUnrecoverable = null) { + async completeMigration( + migrationId, + hasErrors, + acceptUnrecoverable = null, + proofHeaders = null, + ) { try { const body = { hasErrors } // The server refuses to finalise a migration that would cost the @@ -755,11 +806,15 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { body.acceptUnrecoverable = acceptUnrecoverable } + // Completing marks the old suite compromised, so it carries a + // vault-key proof (defence in depth alongside the acknowledgement). + const config = proofHeaders ? { headers: proofHeaders } : {} const { data } = await axios.post( generateUrl( `/apps/keepiq/api/v1/migrations/${migrationId}/complete`, ), body, + config, ) this.migrationDroppedVersions = data.droppedVersions ?? this.migrationDroppedVersions diff --git a/tests/vitest/proveMasterPassword.spec.js b/tests/vitest/proveMasterPassword.spec.js new file mode 100644 index 000000000..6022e5161 --- /dev/null +++ b/tests/vitest/proveMasterPassword.spec.js @@ -0,0 +1,138 @@ +/** + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * Tests for proveMasterPassword — the client half of the vault-key proof. A + * signature it produces must verify, under RSASSA-PKCS1-v1_5 SHA-256, against + * the suite public key over the exact message the server rebuilds (challenge + + * per-bound-value SHA-256). A wrong password must throw before signing, and a + * changed bound value must make the signature fail — the binding the server + * relies on. Also pins that the SESSION key stays decrypt-only, so it can never + * be the thing that signs. + */ + +import { describe, expect, it } from 'vitest' +import { encryptPrivateKey } from '../../src/crypto/aes.js' +import { proveMasterPassword } from '../../src/crypto/reauth.js' +import { importPrivateKey } from '../../src/crypto/rsa.js' + +/** + * Generate an RSA keypair and return the private key as PKCS#8 PEM plus the + * public key as a verify-capable CryptoKey. + * + * @return {Promise<{privateKeyPem: string, publicKey: CryptoKey}>} The pair. + */ +async function makePair() { + const pair = await crypto.subtle.generateKey( + { + name: 'RSA-PSS', + modulusLength: 2048, + publicExponent: new Uint8Array([1, 0, 1]), + hash: 'SHA-256', + }, + true, + ['sign', 'verify'], + ) + const pkcs8 = new Uint8Array( + await crypto.subtle.exportKey('pkcs8', pair.privateKey), + ) + const privateKeyPem = + '-----BEGIN PRIVATE KEY-----\n' + + btoa(String.fromCharCode(...pkcs8)) + .match(/.{1,64}/g) + .join('\n') + + '\n-----END PRIVATE KEY-----' + + // Re-import the public key for the RSASSA-PKCS1-v1_5 scheme the proof uses. + const spki = new Uint8Array( + await crypto.subtle.exportKey('spki', pair.publicKey), + ) + const publicKey = await crypto.subtle.importKey( + 'spki', + spki, + { name: 'RSASSA-PKCS1-v1_5', hash: 'SHA-256' }, + false, + ['verify'], + ) + + return { privateKeyPem, publicKey } +} + +/** + * Rebuild the signed message exactly as VaultKeyProofService::signedMessage: + * challenge, then hex SHA-256 of each bound value, one per line. + * + * @param {string} nonce The challenge. + * @param {string[]} boundValues The bound values, in order. + * @return {Promise} The message bytes. + */ +async function rebuildMessage(nonce, boundValues) { + const enc = new TextEncoder() + const lines = [nonce] + for (const v of boundValues) { + const d = await crypto.subtle.digest('SHA-256', enc.encode(String(v))) + lines.push( + Array.from(new Uint8Array(d)) + .map((b) => b.toString(16).padStart(2, '0')) + .join(''), + ) + } + return enc.encode(lines.join('\n')) +} + +describe('proveMasterPassword', () => { + it('produces a signature that verifies over the bound message', async () => { + const { privateKeyPem, publicKey } = await makePair() + const envelope = await encryptPrivateKey(privateKeyPem, 'master-pw') + const nonce = 'challenge.mac' + const bound = ['PUBLIC', 'ENVELOPE'] + + const sigB64 = await proveMasterPassword(envelope, 'master-pw', nonce, bound) + const signature = Uint8Array.from(atob(sigB64), (c) => c.charCodeAt(0)) + + const ok = await crypto.subtle.verify( + 'RSASSA-PKCS1-v1_5', + publicKey, + signature, + await rebuildMessage(nonce, bound), + ) + expect(ok).toBe(true) + }) + + it('throws on a wrong password, before any signature is produced', async () => { + const { privateKeyPem } = await makePair() + const envelope = await encryptPrivateKey(privateKeyPem, 'master-pw') + + await expect( + proveMasterPassword(envelope, 'WRONG', 'challenge.mac', []), + ).rejects.toBeDefined() + }) + + it('binds to the values: a changed bound value fails verification', async () => { + const { privateKeyPem, publicKey } = await makePair() + const envelope = await encryptPrivateKey(privateKeyPem, 'master-pw') + const nonce = 'challenge.mac' + + const sigB64 = await proveMasterPassword(envelope, 'master-pw', nonce, ['A']) + const signature = Uint8Array.from(atob(sigB64), (c) => c.charCodeAt(0)) + + const ok = await crypto.subtle.verify( + 'RSASSA-PKCS1-v1_5', + publicKey, + signature, + await rebuildMessage(nonce, ['TAMPERED']), + ) + expect(ok).toBe(false) + }) +}) + +describe('the session key cannot sign', () => { + it('imports the session private key non-extractable and decrypt-only', async () => { + const { privateKeyPem } = await makePair() + const sessionKey = await importPrivateKey(privateKeyPem) + + expect(sessionKey.extractable).toBe(false) + expect(sessionKey.usages).toEqual(['decrypt']) + expect(sessionKey.usages).not.toContain('sign') + }) +}) From 0c65767843bdef62170facbf8ea04ce92fc3d63c Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 10 Sep 2026 16:40:38 +0200 Subject: [PATCH 09/48] =?UTF-8?q?feat(vault-key-proof):=20wire=20the=20eme?= =?UTF-8?q?rgency=20and=20completion=20flows=20(#673,=20=C2=A74.7-4.8)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finishes the client wiring so all four guarded flows work end to end. Emergency-contact delete (§4.7): `emergencyAccess.revoke(id, masterPassword)` builds a proof (subject active, bound to the contact id) and the delete carries it. `EmergencyAccessView` gained a master-password confirm dialog — deleting a contact destroys its recovery envelope, so it must prove the master password, which is why a session alone can no longer do it. Completion (§4.8): completion's proof is now over the OLD (retiring) key rather than the new one, via a new middleware subject `migrationOldSuite` that resolves the migration's old suite. Both suites are active at completion so 'active' was ambiguous, and — the point — the old key is the one BOTH the initiate and resume paths already hold the password for, so a resumed run finalises with no extra prompt. The "Finish anyway" acknowledgement path builds the proof from the retained (or re-entered) old password, and the form re-shows the password field on a `key_proof_required` refusal — the re-enter-and-retry UX. Coverage and middleware tests updated for the new subject; the middleware gains SuiteMigrationMapper to resolve the old suite. Backend + frontend tests green (79 PHP incl. the new migrationOldSuite resolution test; 22 frontend incl. proveMasterPassword). phpmd clean; prettier + eslint 0 errors. Refs #673 Assisted-by: ClaudeCode:claude-opus-5 --- lib/Controller/MigrationController.php | 2 +- lib/Middleware/VaultKeyProofMiddleware.php | 47 +++++++-- .../plan.json | 8 +- .../harden-vault-key-material-guards/tasks.md | 4 +- src/components/CompromiseRecoveryForm.vue | 32 ++++++- src/store/modules/emergencyAccess.js | 21 +++- src/store/modules/encryptionSuite.js | 62 ++++++++++-- src/views/EmergencyAccessView.vue | 96 ++++++++++++++++++- .../VaultKeyProofAttributesTest.php | 2 +- .../VaultKeyProofMiddlewareTest.php | 29 ++++++ .../components/CompromiseRecoveryForm.spec.js | 9 +- 11 files changed, 275 insertions(+), 37 deletions(-) diff --git a/lib/Controller/MigrationController.php b/lib/Controller/MigrationController.php index 16dc7988e..bfd1e9b14 100644 --- a/lib/Controller/MigrationController.php +++ b/lib/Controller/MigrationController.php @@ -119,7 +119,7 @@ public function getStatus(): JSONResponse { #[NoAdminRequired] #[VaultKeyProofRequired( binds: ['id'], - subject: 'active', + subject: 'migrationOldSuite', purpose: VaultKeyProofService::PURPOSE_COMPLETE_MIGRATION )] public function complete(string $id, bool $hasErrors = false, ?int $acceptUnrecoverable = null): JSONResponse { diff --git a/lib/Middleware/VaultKeyProofMiddleware.php b/lib/Middleware/VaultKeyProofMiddleware.php index 040324179..09e80ebc6 100644 --- a/lib/Middleware/VaultKeyProofMiddleware.php +++ b/lib/Middleware/VaultKeyProofMiddleware.php @@ -34,6 +34,7 @@ use OCA\Keepiq\Attribute\VaultKeyProofRequired; use OCA\Keepiq\Db\EncryptionSuite; +use OCA\Keepiq\Db\SuiteMigrationMapper; use OCA\Keepiq\Exception\KeyProofRequiredException; use OCA\Keepiq\Service\EncryptionSuiteService; use OCA\Keepiq\Service\VaultKeyProofService; @@ -67,6 +68,7 @@ class VaultKeyProofMiddleware extends Middleware { * @param IUserSession $userSession The session, for the acting user * @param EncryptionSuiteService $suiteService Resolves the subject suite * @param VaultKeyProofService $proofService Verifies the proof + * @param SuiteMigrationMapper $migrationMapper Resolves a migration's old suite * * @return void */ @@ -75,6 +77,7 @@ public function __construct( private IUserSession $userSession, private EncryptionSuiteService $suiteService, private VaultKeyProofService $proofService, + private SuiteMigrationMapper $migrationMapper, ) { }//end __construct() @@ -203,20 +206,48 @@ private function subjectCertificate(VaultKeyProofRequired $attribute, string $us * @throws KeyProofRequiredException When a named suite is not the caller's own */ private function resolveSubjectSuite(string $subject, string $userId): EncryptionSuite { - if (str_starts_with($subject, 'routeParam:') === false) { - return $this->suiteService->getActiveSuite(ownerType: 'user', ownerId: $userId); + if ($subject === 'migrationOldSuite') { + // Completion proves the OLD key, not the new one: at completion both + // suites are active so 'active' is ambiguous, and the old key is the + // one both the initiate and resume clients already hold the password + // for. Resolve it from the migration named by the route's `id`. + $migration = $this->migrationMapper->findById((string)$this->request->getParam('id', '')); + return $this->assertOwned( + suite: $this->suiteService->getSuite($migration->getOldSuiteId()), + userId: $userId + ); } - $paramName = substr($subject, strlen('routeParam:')); - $suite = $this->suiteService->getSuite((string)$this->request->getParam($paramName, '')); + if (str_starts_with($subject, 'routeParam:') === true) { + $paramName = substr($subject, strlen('routeParam:')); + return $this->assertOwned( + suite: $this->suiteService->getSuite((string)$this->request->getParam($paramName, '')), + userId: $userId + ); + } + + return $this->assertOwned( + suite: $this->suiteService->getActiveSuite(ownerType: 'user', ownerId: $userId), + userId: $userId + ); + }//end resolveSubjectSuite() - // The proof must be over the OWNER's own key; a suite belonging to - // someone else (or to an application) can never be the subject of a - // user's self-service proof. + /** + * Assert the resolved suite is the caller's own; a proof is always over the + * owner's key, never another user's or an application's. + * + * @param EncryptionSuite $suite The resolved suite + * @param string $userId The acting user + * + * @return EncryptionSuite + * + * @throws KeyProofRequiredException When the suite is not the caller's + */ + private function assertOwned(EncryptionSuite $suite, string $userId): EncryptionSuite { if ($suite->getOwnerType() !== 'user' || $suite->getOwnerId() !== $userId) { throw new KeyProofRequiredException(message: 'Subject suite is not yours'); } return $suite; - }//end resolveSubjectSuite() + }//end assertOwned() }//end class diff --git a/openspec/changes/harden-vault-key-material-guards/plan.json b/openspec/changes/harden-vault-key-material-guards/plan.json index 67f5306d2..dd0fa436a 100644 --- a/openspec/changes/harden-vault-key-material-guards/plan.json +++ b/openspec/changes/harden-vault-key-material-guards/plan.json @@ -253,8 +253,8 @@ { "id": 28, "num": "4.7", - "title": "REMAINING \u2014 wire the emergency-contact delete through the helper. Needs a master-password prompt at the delete point (the delete action has no password in hand), so it is a UI change, not just a store change", - "status": "pending", + "title": "Wired the emergency-contact delete through the helper: `emergencyAccess.revoke(id, masterPassword)` builds a proof (subject active, bound to the contact id) and `EmergencyAccessView` gained a master-password confirm dialog before it", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] @@ -262,8 +262,8 @@ { "id": 29, "num": "4.8", - "title": "REMAINING \u2014 handle `403 key_proof_required` as a re-enter-and-retry prompt. Also covers the resume path's completion: resume asks only for the OLD password, but completion's proof is over the NEW suite key, so a current-password prompt is needed there too", - "status": "pending", + "title": "Completion's proof is now over the OLD key (new middleware subject `migrationOldSuite`), which both the initiate and resume paths already hold the password for \u2014 so resume-completion needs no new prompt. The acknowledgement (\"Finish anyway\") path builds the proof from the retained/re-entered old password, and the form re-shows the password field on a `key_proof_required` refusal", + "status": "done", "spec_ref": null, "acceptance_criteria": [], "files_likely_affected": [] diff --git a/openspec/changes/harden-vault-key-material-guards/tasks.md b/openspec/changes/harden-vault-key-material-guards/tasks.md index 68323a159..30866fba0 100644 --- a/openspec/changes/harden-vault-key-material-guards/tasks.md +++ b/openspec/changes/harden-vault-key-material-guards/tasks.md @@ -44,8 +44,8 @@ Section 3 (abort) is independently useful and can be split into its own PR if th - [x] 4.4 Add a shared client helper that fetches a challenge, prompts for the master password, produces the proof, and sets the `X-Keepiq-Key-Proof` header — so the four call sites do not each re-implement it - [x] 4.5 Wire `src/components/CompromiseRecoveryForm.vue` (recovery start, and the completion call) through the helper - [x] 4.6 Wire the routine master-password change flow through the helper; verify the old private key is materialised at that point (design "Risks" — if it is not, stop and raise before proceeding) -- [ ] 4.7 REMAINING — wire the emergency-contact delete through the helper. Needs a master-password prompt at the delete point (the delete action has no password in hand), so it is a UI change, not just a store change -- [ ] 4.8 REMAINING — handle `403 key_proof_required` as a re-enter-and-retry prompt. Also covers the resume path's completion: resume asks only for the OLD password, but completion's proof is over the NEW suite key, so a current-password prompt is needed there too +- [x] 4.7 Wired the emergency-contact delete through the helper: `emergencyAccess.revoke(id, masterPassword)` builds a proof (subject active, bound to the contact id) and `EmergencyAccessView` gained a master-password confirm dialog before it +- [x] 4.8 Completion's proof is now over the OLD key (new middleware subject `migrationOldSuite`), which both the initiate and resume paths already hold the password for — so resume-completion needs no new prompt. The acknowledgement ("Finish anyway") path builds the proof from the retained/re-entered old password, and the form re-shows the password field on a `key_proof_required` refusal ## 5. Apply The Guard (must not precede section 4) diff --git a/src/components/CompromiseRecoveryForm.vue b/src/components/CompromiseRecoveryForm.vue index e2ec08caa..8184e58ee 100644 --- a/src/components/CompromiseRecoveryForm.vue +++ b/src/components/CompromiseRecoveryForm.vue @@ -112,13 +112,23 @@ + + +
{{ t('keepiq', 'Try these again') }} {{ n( @@ -233,6 +243,8 @@ export default { result: null, /** @type {string|null} Retained so a retry can resume without re-asking. */ activeOldPassword: null, + /** @type {boolean} Show the re-auth field when completion needs a fresh proof. */ + needsReauth: false, } }, @@ -484,13 +496,29 @@ export default { // server counts distinct records currently failed and compares // with a strict `===`. Sending the list length made every click // refused and left the vault write-locked with no way out. - await store.acceptMigrationLosses(store.migrationStatus?.id) + // + // Completion carries a vault-key proof over the OLD key. The old + // password is retained from the run when it started here; on a + // resumed run it is not, so the field below is re-shown. + await store.acceptMigrationLosses( + store.migrationStatus?.id, + this.activeOldPassword || this.oldPassword, + ) + this.needsReauth = false this.result = { ...(this.result ?? { migrated: 0, droppedVersions: 0 }), failures: this.unrecoverable, } this.phase = 'terminal' } catch (e) { + // A guard refusal (or a missing password) means: re-enter and + // retry, not a dead end. Surface the password field. + if ( + e?.code === 'key_proof_required' + || e?.response?.data?.error === 'key_proof_required' + ) { + this.needsReauth = true + } this.error = this.describe(e) } finally { this.loading = false diff --git a/src/store/modules/emergencyAccess.js b/src/store/modules/emergencyAccess.js index defdb16c3..ff8248117 100644 --- a/src/store/modules/emergencyAccess.js +++ b/src/store/modules/emergencyAccess.js @@ -23,6 +23,7 @@ import { openRecoveryEnvelope, } from '../../crypto/emergencyEnvelope.js' import { decryptPrivateKey } from '../../crypto/index.js' +import { buildKeyProofHeaders, PROOF_PURPOSE } from '../../crypto/keyProof.js' import { useSessionStore } from './session.js' export const useEmergencyAccessStore = defineStore('emergencyAccess', { @@ -151,13 +152,29 @@ export const useEmergencyAccessStore = defineStore('emergencyAccess', { /** * Revoke an emergency contact (grantor). * + * Deleting a contact destroys its recovery envelope — the only break-glass + * path that survives a private-key overwrite — so it carries a vault-key + * proof: the caller must prove the master password. The password is used + * only to sign and is never sent. + * * @param {string} id The relationship ID. + * @param {string} masterPassword The current master password, for the proof. * @return {Promise} - * @spec openspec/changes/add-emergency-access/specs/emergency-access/spec.md#requirement-revoke-emergency-contact + * @spec openspec/changes/harden-vault-key-material-guards/specs/emergency-access/spec.md#requirement-revoke-emergency-contact */ - async revoke(id) { + async revoke(id, masterPassword) { + const session = useSessionStore() + const headers = await buildKeyProofHeaders({ + suiteId: session.suiteId, + purpose: PROOF_PURPOSE.EMERGENCY_DESTROY, + encryptedPrivateKey: session.encryptedPrivateKey, + masterPassword, + boundValues: [id], + }) + await axios.delete( generateUrl(`/apps/keepiq/api/v1/emergency-access/contacts/${id}`), + { headers }, ) await this.fetchContacts() }, diff --git a/src/store/modules/encryptionSuite.js b/src/store/modules/encryptionSuite.js index 979f61f56..954cfe34b 100644 --- a/src/store/modules/encryptionSuite.js +++ b/src/store/modules/encryptionSuite.js @@ -280,14 +280,15 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { // for the terminal step. The premature complete() that used to sit // here reported success five lines after initiating, before a single // record had been touched. - // Completion is guarded too. The session is now bound to the NEW - // suite, so the proof is made with the new key and the new master - // password (both in hand here), bound to the migration id. + // Completion is guarded too, and its proof is over the OLD key — + // the suite being retired — not the new one. That is the key both + // this initiate path and the resume path already hold the password + // for (oldPassword), so completion needs no extra prompt on either. const completeProof = await buildKeyProofHeaders({ - suiteId: session.suiteId, + suiteId: response.data.migration.oldSuiteId, purpose: PROOF_PURPOSE.COMPLETE_MIGRATION, - encryptedPrivateKey: newEncryptedPk, - masterPassword: newPassword, + encryptedPrivateKey: response.data.oldEncryptedPrivateKey, + masterPassword: oldPassword, boundValues: [response.data.migration.id], }) await this.finaliseMigration( @@ -751,11 +752,16 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { * called from an affirmative user action. * * @param {string} migrationId The migration ID. + * @param {string} oldPassword The old master password, to prove the retiring key. * @param {number} acceptUnrecoverable How many losses the user accepted. * @return {Promise} The completion response. * @spec openspec/changes/restore-suite-migration-loop/specs/secrets/spec.md#requirement-possibly-compromised-flag-lifecycle */ - async acceptMigrationLosses(migrationId, acceptUnrecoverable = null) { + async acceptMigrationLosses( + migrationId, + oldPassword, + acceptUnrecoverable = null, + ) { // Defaults to the server's own number. A caller may still pass one // explicitly, but the stored value is what the server asked for and // is therefore what it will accept. @@ -768,7 +774,35 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { ) } - const data = await this.completeMigration(migrationId, true, accepted) + // Completing is guarded, and the proof is over the OLD (retiring) + // key. Without the old password we cannot build it, so ask for it + // rather than sending a request the server will refuse. + if (!oldPassword) { + const err = new Error( + 'Re-enter your master password to finish the rotation.', + ) + err.code = 'key_proof_required' + throw err + } + const { data: oldSuite } = await axios.get( + generateUrl( + `/apps/keepiq/api/v1/suites/${this.migrationStatus.oldSuiteId}`, + ), + ) + const proof = await buildKeyProofHeaders({ + suiteId: this.migrationStatus.oldSuiteId, + purpose: PROOF_PURPOSE.COMPLETE_MIGRATION, + encryptedPrivateKey: oldSuite.privateKey, + masterPassword: oldPassword, + boundValues: [migrationId], + }) + + const data = await this.completeMigration( + migrationId, + true, + accepted, + proof, + ) this.migrationNeedsAcknowledgement = false this.migrationRequiredAcknowledgement = null this.migrationBlockedMessage = null @@ -931,7 +965,17 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { newPrivateKey: session.cryptoKey, }) - await this.finaliseMigration(migrationId, outcome) + // Completion's proof is over the OLD key, which resume already holds + // the password for — so a resumed run finalises without any extra + // prompt, exactly like the initiate path. + const completeProof = await buildKeyProofHeaders({ + suiteId: this.migrationStatus.oldSuiteId, + purpose: PROOF_PURPOSE.COMPLETE_MIGRATION, + encryptedPrivateKey: oldSuite.privateKey, + masterPassword: oldPassword, + boundValues: [migrationId], + }) + await this.finaliseMigration(migrationId, outcome, completeProof) return outcome }, diff --git a/src/views/EmergencyAccessView.vue b/src/views/EmergencyAccessView.vue index 79c447e0a..15fbb3f77 100644 --- a/src/views/EmergencyAccessView.vue +++ b/src/views/EmergencyAccessView.vue @@ -90,7 +90,7 @@ + @click="promptRevoke(c.id)"> {{ t('keepiq', 'Revoke') }} @@ -145,13 +145,54 @@ }}

+ + + +
+ + {{ + t( + 'keepiq', + 'This deletes the recovery envelope for this contact. They will no longer be able to break glass unless you re-establish them.', + ) + }} + + + + {{ revokeError }} + +
+ + diff --git a/src/store/modules/encryptionSuite.js b/src/store/modules/encryptionSuite.js index 954cfe34b..0ab17f5b3 100644 --- a/src/store/modules/encryptionSuite.js +++ b/src/store/modules/encryptionSuite.js @@ -1068,7 +1068,7 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { * banner rather than pretending it succeeded. * * @return {Promise} The server's terminal result. - * @spec openspec/specs/encryption-suites/spec.md#requirement-a-migration-can-be-aborted-before-any-record-moves + * @spec openspec/changes/harden-vault-key-material-guards/specs/encryption-suites/spec.md#requirement-a-migration-can-be-aborted-before-any-record-moves */ async abortMigration() { await this.fetchMigrationStatus() diff --git a/src/views/EmergencyAccessView.vue b/src/views/EmergencyAccessView.vue index 15fbb3f77..cc55ff727 100644 --- a/src/views/EmergencyAccessView.vue +++ b/src/views/EmergencyAccessView.vue @@ -147,56 +147,28 @@ - + -
- - {{ - t( - 'keepiq', - 'This deletes the recovery envelope for this contact. They will no longer be able to break glass unless you re-establish them.', - ) - }} - - - - {{ revokeError }} - -
- - + v-model:password="revokePassword" + :revoking="revoking" + :error="revokeError" + @close="cancelRevoke" + @confirm="confirmRevoke" /> + + diff --git a/src/store/modules/encryptionSuite.js b/src/store/modules/encryptionSuite.js index 4c0f86921..99f229055 100644 --- a/src/store/modules/encryptionSuite.js +++ b/src/store/modules/encryptionSuite.js @@ -1214,6 +1214,87 @@ export const useEncryptionSuiteStore = defineStore('encryptionSuite', { } }, + /** + * Administrator force-revoke of any suite by id (admin settings surface). + * + * The administrator counterpart to the owner path's `revokeSuite()`. The + * vault is zero-knowledge (ADR-003), so an administrator holds no vault key + * to sign the revoke challenge; authorisation is the admin guard plus + * Nextcloud sudo. The endpoint carries `#[PasswordConfirmationRequired]`, so + * the password-confirmation (sudo) flow MUST complete BEFORE the request — + * the middleware rejects a request whose sudo has not been re-confirmed. + * + * `@nextcloud/password-confirmation` is imported lazily (like the offline + * store below) so its `@nextcloud/vue` dialog dependency stays off the + * store's static load path; `confirmPassword()` resolves immediately when + * sudo is not currently required and otherwise prompts, resolving only once + * the administrator has re-confirmed and rejecting if they cancel. + * + * The offline cache is deliberately NOT evicted here: it holds the acting + * administrator's OWN vault, not the (cross-owner) target suite's secrets, + * so evicting it on an unrelated admin action would be wrong. The owner + * `revokeSuite()` evicts because there the revoked suite IS the caller's own. + * + * @param {object} params The parameters. + * @param {string} params.id The suite id to force-revoke. + * @param {string} params.reason The required, free-form revocation reason. + * @param {boolean} params.markCompromised Treat the suite's secrets as compromised (default false). + * @return {Promise<{suite: object, emergencyContactsDestroyed: number, warning: string|null}>} + * The revoked suite, the count of destroyed usable emergency contacts, and + * (only when `markCompromised` was false) the rotation-may-be-warranted warning. + * @spec openspec/changes/admin-suite-revocation/specs/encryption-suites/spec.md#requirement-administrator-force-revocation + */ + async forceRevokeSuite({ id, reason, markCompromised = false }) { + if (!id) { + throw new Error('No suite id to revoke') + } + if (!reason) { + throw new Error('A reason is required to force-revoke a suite') + } + + // Complete Nextcloud sudo BEFORE issuing the request — the endpoint's + // #[PasswordConfirmationRequired] middleware rejects it otherwise. + const { confirmPassword } = + await import('@nextcloud/password-confirmation') + await confirmPassword() + + const response = await axios.post( + generateUrl(`/apps/keepiq/api/v1/suites/${id}/force-revoke`), + { reason, markCompromised }, + ) + + return { + suite: response.data, + emergencyContactsDestroyed: + response.data.emergencyContactsDestroyed ?? 0, + warning: response.data.warning ?? null, + } + }, + + /** + * Reinstate a revoked suite by id from the admin settings surface. + * + * Wired to the existing admin-only `reinstate()` endpoint, which carries no + * `#[PasswordConfirmationRequired]` — the `AuthorizedAdminSetting` guard is + * the authorization, so no sudo flow is needed. Frontend-only; the endpoint + * and `reinstateSuite()` service are unchanged. + * + * @param {string} id The suite id to reinstate. + * @return {Promise} The reinstated suite JSON. + * @spec openspec/changes/admin-suite-revocation/specs/encryption-suites/spec.md#requirement-administrator-force-revocation + */ + async reinstateSuiteAdmin(id) { + if (!id) { + throw new Error('No suite id to reinstate') + } + + const response = await axios.post( + generateUrl(`/apps/keepiq/api/v1/suites/${id}/reinstate`), + ) + + return response.data + }, + /** * Check migration status. * diff --git a/src/views/settings/Settings.vue b/src/views/settings/Settings.vue index d1d214d07..630c1dc2c 100644 --- a/src/views/settings/Settings.vue +++ b/src/views/settings/Settings.vue @@ -26,12 +26,14 @@ +