feat(security): add a database-backed Data Protection key store (depends on #1372 and #1374) - #1376
Open
maxiar wants to merge 1 commit into
Open
Conversation
DEPENDS ON fullstackhero#1372 AND fullstackhero#1374 — see the PR description. It does not build against main on its own, and that is expected. The framework only persists Data Protection keys to Redis. That breaks down two ways. The DbMigrator is normally run standalone, outside the AppHost wiring that injects a Redis connection string, so anything its seed encrypts can become permanently undecryptable by the API — "CryptographicException: key {guid} not found in the key ring" — no matter how consistently the application name is pinned. And Redis configured as a cache can evict a key, taking every session and pending reset token with it. DataProtection:Store now selects Redis (default, unchanged) or Database, backed by DataProtectionKeysDbContext in Persistence with migrations for both providers. It follows the framework's existing shape for a framework-owned context — AddHeroDbContext plus an IDbInitializer, exactly like EventingDbContext — so the table is created by the API host, the DbMigrator and the integration-test harness alike. Registering the context WITHOUT its initializer is the failure this shape prevents: the table then exists only where someone migrated it by hand, and every flow that protects a payload fails elsewhere with "Invalid object name 'DataProtectionKeys'". That is how this was found, in a downstream project that wired it by hand and lost 123 of 747 integration tests to it, identically on both providers. The DbMigrator additionally creates the table BEFORE it starts its host. Data Protection resolves its key ring eagerly during StartAsync, well before the migrator's own Step 0/1/2 flow, so the initializer alone is too late there: against an empty database that logged six query failures with stack traces and still exited 0 — a key created while the table is missing cannot be persisted. Silent, not loud. The integration suite now runs against the database store. It has always run without Redis, so the key ring was ephemeral and neither store was exercised; pointing it at the database means all 747 tests also prove the key table is migrated and writable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
This PR depends on #1372 and #1374 and will not build until both are merged. That is expected, not an oversight.
MigratorLockFactory/IMigratorLocksrc/Host/FSH.Starter.Migrations.MSSQL— the project does not exist onmainyet, so those files are inert until it doesDataProtectionApplicationNameVerified: with those two applied locally, this branch builds clean and the full suite passes on both providers. Against
mainalone the only compile error isDataProtectionApplicationName(from #1372); theMigratorLockFactoryone appears after that is resolved.Happy to rebase, reorder or fold this differently — flagging the dependency up front so it is not discovered in review.
Why
The framework only persists Data Protection keys to Redis. That breaks down two ways:
CryptographicException: key {guid} not found in the key ring— no matter how consistently the application name is pinned.What
DataProtection:StoreselectsRedis(default, unchanged) orDatabase, backed byDataProtectionKeysDbContextin Persistence with migrations for both providers.It follows the framework's existing shape for a framework-owned context —
AddHeroDbContextplus anIDbInitializer, exactly likeEventingDbContext— so the table is created by the API host, the DbMigrator and the integration-test harness alike. No new "Migrations.Common"-style project is needed; the context lives in Persistence, which both migrations projects already reference.Registering the context without its initializer is the failure this shape prevents: the table then exists only where someone migrated it by hand, and every flow that protects a payload fails elsewhere with
Invalid object name 'DataProtectionKeys'. That is not hypothetical — it is how this was found, in a downstream project that wired it by hand and lost 123 of 747 integration tests to it, identically on both providers.The part that is easy to miss
The DbMigrator creates the table before it starts its host, via
DataProtectionSchema.EnsureAsync.Data Protection resolves its key ring eagerly during
StartAsync, well before the migrator's own Step 0/1/2 flow, so theIDbInitializeralone is too late there. Against an empty database that produced six logged query failures with stack traces — and the run still exited 0. That is the dangerous outcome: a key created while the table is missing cannot be persisted, so anything encrypted in that window is undecryptable afterwards. Silent, not loud.It waits for the database itself, because the migrator's own readiness wait is also after
StartAsync.Verification
The integration suite now runs against the database store. It has always run without Redis, so the key ring was ephemeral and neither store was exercised; pointing it at the database means all 747 tests also prove the key table is migrated and writable.
MigrationDriftTestspicks the new context up automatically, since it discovers contexts from the snapshots present in each provider's migrations assembly.One overlap worth naming: the hunk in
BuildingBlocks/Caching/Extensions.csalso carries the application-name change from #1372, because both touch the same block. If #1372 lands first, that part of the hunk collapses to nothing.🤖 Generated with Claude Code