feat: added options to configure read-only key store, and key mapping bug fixes#10
Open
jhbritton-RSK wants to merge 5 commits into
Open
feat: added options to configure read-only key store, and key mapping bug fixes#10jhbritton-RSK wants to merge 5 commits into
jhbritton-RSK wants to merge 5 commits into
Conversation
jhbritton-RSK
changed the title
There was a problem hiding this comment.
Pull request overview
This PR extends the IdentityServer compatibility signing/validation key stores to support time-based filtering (max key lifetime) via TimeProvider, adds configurable options, and updates unit/integration tests accordingly.
Changes:
- Introduces
CompatibilityKeyStoreOptionsand a sharedIdentityServerSigningKeyStorebase to centralize key retrieval and max-lifetime filtering. - Updates signing/validation key stores and DI registration (
AddCompatibilityKeyStores) to useTimeProviderand optional signing-store registration. - Adds/updates tests and test helpers for deterministic time and key expiration behavior; adds
DataProtectionConstantsand converter tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Open.IdentityServer/test/Open.IdentityServer.UnitTests/Stores/Compatibility/IdentityServerValidationKeysStoreTests.cs | Updates validation key store unit tests to use FakeTimeProvider and adds max-lifetime filtering test. |
| src/Open.IdentityServer/test/Open.IdentityServer.UnitTests/Stores/Compatibility/IdentityServerSigningCredentialStoreTests.cs | Updates signing credential store unit tests to use FakeTimeProvider and adds max-lifetime filtering test. |
| src/Open.IdentityServer/test/Open.IdentityServer.UnitTests/Stores/Compatibility/FakeKeyData.cs | Refactors test key/cert helpers (parameter initializers, RSA import, PFX export). |
| src/Open.IdentityServer/test/Open.IdentityServer.UnitTests/DataProtection/DataProtectedIdentityServerKeyMaterialConverterTests.cs | Adds unit tests covering key material conversion (RSA/EC/X509, protected/unprotected). |
| src/Open.IdentityServer/test/Open.IdentityServer.IntegrationTests/Open.IdentityServer.IntegrationTests.csproj | Adds Microsoft.Extensions.TimeProvider.Testing package reference for integration tests. |
| src/Open.IdentityServer/test/Open.IdentityServer.IntegrationTests/Endpoints/Discovery/JwkEndpointTests.cs | Injects a FakeTimeProvider into the pipeline for deterministic integration testing. |
| src/Open.IdentityServer/test/Open.IdentityServer.IntegrationTests/Endpoints/Discovery/FakeIdentityServerKeyStore.cs | Adds a fixed FakeNow timestamp and an “expired” key for max-lifetime filtering coverage. |
| src/Open.IdentityServer/src/Stores/Compatibility/IdentityServerValidationKeysStore.cs | Switches to shared base key retrieval/filtering logic. |
| src/Open.IdentityServer/src/Stores/Compatibility/IdentityServerSigningCredentialStore.cs | Switches to shared base key retrieval/filtering logic and returns newest valid signing credential. |
| src/Open.IdentityServer/src/Stores/Compatibility/IdentityServerKeyStore.cs | Adds new shared base class implementing key filtering and ordering. |
| src/Open.IdentityServer/src/DataProtectionConstants.cs | Adds a constant for the data protector purpose used by the converter. |
| src/Open.IdentityServer/src/DataProtection/DataProtectedIdentityServerKeyMaterialConverter.cs | Uses the new constant for protector purpose and improves X509 conversions (Created/KeyId). |
| src/Open.IdentityServer/src/Configuration/DependencyInjection/Options/CompatibilityKeyStoreOptions.cs | Adds options for key max lifetime and whether to register the signing credential store. |
| src/Open.IdentityServer/src/Configuration/DependencyInjection/BuilderExtensions/Crypto.cs | Extends AddCompatibilityKeyStores to accept options/configure callback and conditionally register signing store. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
patchandthat
requested changes
Jun 8, 2026
| return identityServerKeyStore.GetKeys() | ||
| .Where(x => x.Use == "signing") | ||
| .Select(dataProtectedIdentityServerKeyMaterialConverter.Convert) | ||
| .Where(x => x.Created.Add(options.MaxLifetime) > timeProvider.GetUtcNow()) |
Collaborator
There was a problem hiding this comment.
This line does need to change to .Where(x => x.Created.Add(options.MaxLifetime) > timeProvider.GetUtcNow().UtcDateTime)
The rationale in the copilot review that it won't compile is obviously false. There's an implicit conversion to turn DateTime up to DateTimeOffset.
The issue is that EF will load DateTimes with DateTimeKind.Unspecified, and the implicit conversion will then treat it as a local time not UTC.
| @@ -1,3 +1,4 @@ | |||
| using System; | |||
Collaborator
There was a problem hiding this comment.
File is missing copyright header
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.
No description provided.