Repository navigation
fix: allow multiple algorthms to be configured for compatibility store - #143
Open
jhbritton-RSK wants to merge 4 commits into
Open
jhbritton-RSK wants to merge 4 commits into
jhbritton-RSK wants to merge 4 commits into
Conversation
…lity to specify multiple supported algorithm support
…ompatibility store
JoStevensRSK
requested changes
Oct 29, 2026
patchandthat
requested changes
Oct 29, 2026
|
|
||
| You will be able to configure read-only access to the ``Keys`` table in Open.IdentityServer. This will take the latest | ||
| signing key that exists and use it for signing, and all signing keys in the table will be used for validation. We have added the extension method ``IIdentityServerBuilder.AddCompatibilityKeyStores`` which will register these key stores in the DI container for you. | ||
| signing key that exists for each configured algorithm and use it for signing, and all non-expired signing keys in the |
Collaborator
There was a problem hiding this comment.
I think this all needs rewording - it's not consistent with the behaviour given the introduction of propagation time.
|
|
||
| If you need automatic key rotation, see our KeyRotation component on `identityserver.com products page <https://www.identityserver.com/products>`_. | ||
| .. note:: | ||
| By default, the default propagation time will be respected if possible, and only RS256 keys will be loaded for signing, |
Collaborator
There was a problem hiding this comment.
This ought to be broken into two sentences?
| /// <summary> | ||
| /// Specifies signing algorthm of signing key to retrieve, defaults to <c>RS256</c> | ||
| /// </summary> | ||
| public string Algorithm = SecurityAlgorithms.RsaSha256; |
Collaborator
There was a problem hiding this comment.
This is a public field, not a property?
Also should the algorithm(s) not be pulled from the configured options?
Collaborator
Author
There was a problem hiding this comment.
I register a signing credential store for each supported algorithm, this works well with the way the existing KeyMaterialService works and requires no changes to this service. But this means that the algorithm filter in the store needs to be settable on init property.
This branch has not been deployed
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.
Description
Added setting for supported algorithms to the compatibility key store. Previously, if more than one signing algorithm was used in existing solutions, the latest key, regardless of the algorithm, would be used. No, by default, the latest RS256 key is used, and you can configure a list of supported algorithms in priority order.
Settings also added to configure key propagation time to prevent publishing of keys that have not yet had time to propagate. If no older keys exist, though, the oldest unpublished key will still be used as the fallback.
Edit: Discovered and fixed bug causing excepsion to be thrown when loading 'ES512' keys.
Type of change
Does this PR introduce a breaking change?
n/a
Testing
Added tests to signing key store to check for edge cases that new configuration setting should affect.
LLM Usage
n/a
Other context
n/a