feat(audit-trail): tenant-key API (closes scholiq deps #2)

[rubenvdlinde]

Summary

• Adds TenantKeyService with getCurrentTenantKey(string $tenantId): string and rotateTenantKey(string $tenantId): array
• Keys are 256-bit CSPRNG values encrypted at rest via OCP\Security\ICrypto; never exposed through any REST endpoint
• Bootstrap on first call per tenant; subsequent reads return same plaintext key until rotation is performed
• Rotation retires the previous row (status retired, retained for re-verification) and inserts a fresh active row
• Migration Version1Date20260511100000 creates openregister_tenant_keys table with tenant_id, encrypted_key, status, created_at columns
• Service wired into DI via registerSettingsServices() in Application::register

Behaviour contract

Scenario	Result
First call for new tenant	Generates + stores + returns fresh 64-char hex key
Repeated call for same tenant	Returns identical key (no new row inserted)
rotateTenantKey	Returns {old, new, rotated_at}; old row → retired; new row → active
rotateTenantKey on tenant with no prior key	old = "", new key bootstrapped
getCurrentTenantKey after rotation	Returns new key

Test plan

• 11 PHPUnit unit tests covering all scenarios above (run in Docker, all green)
• PHPCS clean on lib/ files in Docker container (same env as CI)
• PHPMD clean
• CI quality pipeline (triggered by PR open)

DO NOT admin-merge — OR is production-tier; awaiting review.

Addresses: openregister#1470 §2

[github-actions]

Quality Report — ConductionNL/openregister @ 4fa475c

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		❌
stylelint		✅
composer			✅	✅ 153/153
npm			✅	✅ 598/598
PHPUnit					❌
Newman					⏭️
Playwright					⏭️

---

Quality workflow — 2026-05-11 19:29 UTC

Download the full PDF report from the workflow artifacts.

[WilcoLouwerse]

[BLOCKER] No caller-identity check on getCurrentTenantKey() — any DI consumer can read any tenant's key

getCurrentTenantKey(string $tenantId) is a public method with no authorisation guard. Any class that obtains the service via DI can pass an arbitrary $tenantId and receive the plaintext HMAC key for that tenant. There is no check that the calling context (user session, service account, or admin) is actually allowed to act on behalf of $tenantId. This is a tenant-boundary breach: Tenant A's code can request Tenant B's signing key. The method must validate $tenantId against the authenticated session (e.g. via IUserSession + organisation membership) or be restricted to an explicit policy object.

[WilcoLouwerse]

[BLOCKER] No caller-identity check on getCurrentTenantKey() — any DI consumer can read any tenant's key

getCurrentTenantKey(string $tenantId) is a public method with no authorisation guard. Any class that obtains the service via DI can pass an arbitrary $tenantId and receive the plaintext HMAC key for that tenant. There is no check that the calling context (user session, service account, or admin) is actually allowed to act on behalf of $tenantId. This is a tenant-boundary breach: Tenant A's code can request Tenant B's signing key. The method must validate $tenantId against the authenticated session (e.g. via IUserSession + organisation membership) or be restricted to an explicit policy object.

[WilcoLouwerse]

[BLOCKER] rotateTenantKey() is public and unguarded — any DI consumer can silently rotate another tenant's key

Rotating a tenant's HMAC key invalidates every audit-trail signature produced under the previous key. The method carries no admin or ownership check, so a compromised non-admin plugin service could trigger rotation for every tenant in sequence, breaking audit verification. It must require either admin privilege (AuthorizedAdminSetting) or an explicit organisation-ownership assertion before execution.

[WilcoLouwerse]

[BLOCKER] No UNIQUE index on (tenant_id, status='active') — duplicate active keys per tenant possible under concurrency

The migration adds only a non-unique composite index idx_or_tkeys_tenant_status. Under concurrent first-call bootstrap two requests for the same fresh tenant hitting fetchActiveRow simultaneously will both find null and both insert. fetchActiveRow uses ORDER BY id DESC LIMIT 1, so future calls return the highest-ID row, but the lower-ID row is orphaned. Add $table->addUniqueIndex(['tenant_id', 'status'], 'uniq_or_tkeys_tenant_active') — though a partial unique index (only for status = 'active') is the correct relational design.

[WilcoLouwerse]

[BLOCKER] rotateTenantKey() is public and unguarded — any DI consumer can silently rotate another tenant's key

Rotating a tenant's HMAC key invalidates every audit-trail signature produced under the previous key. The method carries no admin or ownership check, so a compromised non-admin plugin service could trigger rotation for every tenant in sequence, breaking audit verification. It must require either admin privilege (AuthorizedAdminSetting) or an explicit organisation-ownership assertion before execution.

[WilcoLouwerse]

[BLOCKER] rotateTenantKey() returns the old plaintext key — unnecessary key exposure

The return array contains 'old' => $oldKey (plaintext 64-char hex HMAC key). No legitimate audit-trail use case requires the caller to hold the old key after rotation; they only need to know rotation succeeded and the rotated_at timestamp. Remove old from the return type or replace with a boolean had_prior_key.

[WilcoLouwerse]

[BLOCKER] SPDX license header missing from new lib/ files (Gate 1)

lib/Service/TenantKeyService.php and lib/Migration/Version1Date20260511100000.php use block-comment copyright headers but do not carry the machine-readable SPDX identifier line required by the hydra-gate-spdx check (// SPDX-FileCopyrightText: … + // SPDX-License-Identifier: AGPL-3.0-or-later or EUPL-1.2 per project standard). Add SPDX headers.

[WilcoLouwerse]

[BLOCKER] Gate 7 — decrypt() exception path collapses to silent null at call-sites that wrap in try/catch

ICrypto::decrypt() itself throws \Exception when ciphertext is tampered or the instance secret changes (e.g. after a NC secret rotation). Any caller that wraps getCurrentTenantKey() in catch (\Throwable) { return null; } (the pattern flagged by Gate 7) will silently treat a corrupted key store as 'no key' and may fall through to operating without tenant scope. Document that callers MUST NOT swallow exceptions, and re-throw a typed TenantKeyException to make this distinguishable from a 'no key' null path.

[WilcoLouwerse]

[BLOCKER] Migration has no down() / rollback path

Version1Date20260511100000 extends SimpleMigrationStep and only implements changeSchema(). There is no down logic. The migration system expects a down() pair for production migrations; its absence will block clean downgrades/uninstalls.

[WilcoLouwerse]

[CONCERN] getCurrentTenantKey() auto-bootstraps a key with no audit event or admin notification

On the very first call for a tenant the service silently creates a cryptographic key. No event dispatch, no admin notification. A misconfigured service passing an attacker-controlled $tenantId will create and persist a key for a non-existent tenant, consuming DB rows. Bootstrap should require an explicit admin-initiated provisioning step.

[WilcoLouwerse]

[CONCERN] No rate-limit or replay protection on rotateTenantKey()

Key rotation is irreversible. There is no cool-down, no idempotency token, no check that a rotation was not performed within the last N hours. A tight loop calling rotateTenantKey() repeatedly will produce an ever-growing table of retired rows.

[WilcoLouwerse]

[BLOCKER] No UNIQUE index on (tenant_id, status='active') — duplicate active keys per tenant possible under concurrency

The migration adds only a non-unique composite index idx_or_tkeys_tenant_status. Under concurrent first-call bootstrap two requests for the same fresh tenant hitting fetchActiveRow simultaneously will both find null and both insert. fetchActiveRow uses ORDER BY id DESC LIMIT 1, so future calls return the highest-ID row, but the lower-ID row is orphaned. Add $table->addUniqueIndex(['tenant_id', 'status'], 'uniq_or_tkeys_tenant_active') — though a partial unique index (only for status = 'active') is the correct relational design.

[WilcoLouwerse]

[CONCERN] No cross-tenant isolation test

testDifferentTenantsReceiveValidKeys() asserts both keys are 64-char hex strings but does NOT assert they are different (assertNotSame). There is no test for 'wrong tenant rejected' — a caller requesting a key for a $tenantId they do not own.

[WilcoLouwerse]

[CONCERN] Key material is hex-encoded but docblock claims 'raw binary'

generateKey() returns bin2hex(random_bytes(32)) — a 64-character lowercase hex string. The docblock says 'Raw binary key material (32 bytes / 256 bit)' but the actual value is hex-encoded text. Either return random_bytes(32) directly and store base64-encoded, or update the docblock.

[WilcoLouwerse]

Review

🔴 Blockers (7)

• No caller-identity check on getCurrentTenantKey() — any DI consumer can read any tenant's key — lib/Service/TenantKeyService.php:270
• rotateTenantKey() is public and unguarded — any DI consumer can silently rotate another tenant's key — lib/Service/TenantKeyService.php:300
• No UNIQUE index on (tenant_id, status='active') — duplicate active keys per tenant possible under concurrency — lib/Migration/Version1Date20260511100000.php:154
• rotateTenantKey() returns the old plaintext key — unnecessary key exposure — lib/Service/TenantKeyService.php:314
• SPDX license header missing from new lib/ files (Gate 1) — lib/Service/TenantKeyService.php:9
• Gate 7 — decrypt() exception path collapses to silent null at call-sites that wrap in try/catch — lib/Service/TenantKeyService.php:453
• Migration has no down() / rollback path — lib/Migration/Version1Date20260511100000.php:99

🟡 Concerns (4)

• getCurrentTenantKey() auto-bootstraps a key with no audit event or admin notification — lib/Service/TenantKeyService.php:278
• No rate-limit or replay protection on rotateTenantKey() — lib/Service/TenantKeyService.php:300
• No cross-tenant isolation test — tests/Unit/Service/TenantKeyServiceTest.php:640
• Key material is hex-encoded but docblock claims 'raw binary' — lib/Service/TenantKeyService.php:394

🟢 Minor (1)

• DateTime instead of DateTimeImmutable for rotatedAt (lib/Service/TenantKeyService.php:311)
  Both rotateTenantKey() and bootstrapKey() use new DateTime() where new DateTimeImmutable() is the safe default. Replace with new \DateTimeImmutable() throughout.

Reviewed by WilcoLouwerse via automated batch review.

[WilcoLouwerse]

[BLOCKER] rotateTenantKey() returns the old plaintext key — unnecessary key exposure

The return array contains 'old' => $oldKey (plaintext 64-char hex HMAC key). No legitimate audit-trail use case requires the caller to hold the old key after rotation; they only need to know rotation succeeded and the rotated_at timestamp. Remove old from the return type or replace with a boolean had_prior_key.

[WilcoLouwerse]

[BLOCKER] SPDX license header missing from new lib/ files (Gate 1)

lib/Service/TenantKeyService.php and lib/Migration/Version1Date20260511100000.php use block-comment copyright headers but do not carry the machine-readable SPDX identifier line required by the hydra-gate-spdx check (// SPDX-FileCopyrightText: … + // SPDX-License-Identifier: AGPL-3.0-or-later or EUPL-1.2 per project standard). Add SPDX headers.

[WilcoLouwerse]

[BLOCKER] Gate 7 — decrypt() exception path collapses to silent null at call-sites that wrap in try/catch

ICrypto::decrypt() itself throws \Exception when ciphertext is tampered or the instance secret changes (e.g. after a NC secret rotation). Any caller that wraps getCurrentTenantKey() in catch (\Throwable) { return null; } (the pattern flagged by Gate 7) will silently treat a corrupted key store as 'no key' and may fall through to operating without tenant scope. Document that callers MUST NOT swallow exceptions, and re-throw a typed TenantKeyException to make this distinguishable from a 'no key' null path.

[WilcoLouwerse]

[BLOCKER] Migration has no down() / rollback path

Version1Date20260511100000 extends SimpleMigrationStep and only implements changeSchema(). There is no down logic. The migration system expects a down() pair for production migrations; its absence will block clean downgrades/uninstalls.

[WilcoLouwerse]

[CONCERN] getCurrentTenantKey() auto-bootstraps a key with no audit event or admin notification

On the very first call for a tenant the service silently creates a cryptographic key. No event dispatch, no admin notification. A misconfigured service passing an attacker-controlled $tenantId will create and persist a key for a non-existent tenant, consuming DB rows. Bootstrap should require an explicit admin-initiated provisioning step.

[WilcoLouwerse]

[CONCERN] No rate-limit or replay protection on rotateTenantKey()

Key rotation is irreversible. There is no cool-down, no idempotency token, no check that a rotation was not performed within the last N hours. A tight loop calling rotateTenantKey() repeatedly will produce an ever-growing table of retired rows.

[WilcoLouwerse]

[CONCERN] No cross-tenant isolation test

testDifferentTenantsReceiveValidKeys() asserts both keys are 64-char hex strings but does NOT assert they are different (assertNotSame). There is no test for 'wrong tenant rejected' — a caller requesting a key for a $tenantId they do not own.

[WilcoLouwerse]

[CONCERN] Key material is hex-encoded but docblock claims 'raw binary'

generateKey() returns bin2hex(random_bytes(32)) — a 64-character lowercase hex string. The docblock says 'Raw binary key material (32 bytes / 256 bit)' but the actual value is hex-encoded text. Either return random_bytes(32) directly and store base64-encoded, or update the docblock.

[WilcoLouwerse]

Review

🔴 Blockers (7)

• No caller-identity check on getCurrentTenantKey() — any DI consumer can read any tenant's key — lib/Service/TenantKeyService.php:270
• rotateTenantKey() is public and unguarded — any DI consumer can silently rotate another tenant's key — lib/Service/TenantKeyService.php:300
• No UNIQUE index on (tenant_id, status='active') — duplicate active keys per tenant possible under concurrency — lib/Migration/Version1Date20260511100000.php:154
• rotateTenantKey() returns the old plaintext key — unnecessary key exposure — lib/Service/TenantKeyService.php:314
• SPDX license header missing from new lib/ files (Gate 1) — lib/Service/TenantKeyService.php:9
• Gate 7 — decrypt() exception path collapses to silent null at call-sites that wrap in try/catch — lib/Service/TenantKeyService.php:453
• Migration has no down() / rollback path — lib/Migration/Version1Date20260511100000.php:99

🟡 Concerns (4)

• getCurrentTenantKey() auto-bootstraps a key with no audit event or admin notification — lib/Service/TenantKeyService.php:278
• No rate-limit or replay protection on rotateTenantKey() — lib/Service/TenantKeyService.php:300
• No cross-tenant isolation test — tests/Unit/Service/TenantKeyServiceTest.php:640
• Key material is hex-encoded but docblock claims 'raw binary' — lib/Service/TenantKeyService.php:394

🟢 Minor (1)

• DateTime instead of DateTimeImmutable for rotatedAt (lib/Service/TenantKeyService.php:311)
  Both rotateTenantKey() and bootstrapKey() use new DateTime() where new DateTimeImmutable() is the safe default. Replace with new \DateTimeImmutable() throughout.

Reviewed by WilcoLouwerse via automated batch review.