-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[dotnet user-jwts] Move signing key details to top-level property #42674
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/Security/Authentication/JwtBearer/src/JwtBearerConfigureOptions.cs
Outdated
Show resolved
Hide resolved
3f4d910
to
efc571b
Compare
efc571b
to
a08011e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add test coverage for the value and length properties
.Where(key => key["Issuer"] == issuer) | ||
.Select(key => key["Value"]) | ||
.OfType<string>(); | ||
foreach (var signingKey in signingKeys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are multiple keys per issuer allowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible for a single issuer/authorization server to produce a set of signing keys that the application can use with no preference between them (ref). Most of what I've seen about this touches on them being used for OIDC scenarios, but it helps to support it in some way with JWT since we want the local dev scenario to map nicely to the pod scenario.
|
||
secrets ??= new JsonObject(); | ||
var signkingKeysPropertyName = GetSigningKeyPropertyName(scheme); | ||
var shortId = Guid.NewGuid().ToString("N").Substring(0, 8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the id for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little bit of a premature optimization. One of the things we discussed was making the key generation more robust in the future (supporting setting different key lengths, support different key types, crpyto algos, etc.). I included an ID field so it'll be easier to unique identifying signing keys in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New format:
{
"Authentication": {
"Schemes": {
"Bearer": {
"SigningKeys": [
{
"Id": "FI9W0",
"Issuer": "dotnet-user-jwts",
"Value": "somesigningkeyhere",
"KeyLength": 32
}
]
}
}
}
}
if (secrets.ContainsKey(signkingKeysPropertyName)) | ||
{ | ||
var signingKeys = secrets[signkingKeysPropertyName].AsArray(); | ||
if (reset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so if multiple keys for the same issuer is allowed, does reset need to delete all keys from the same issuer now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, to start, I'd only added support for multiple keys for the same issuer in the runtime, but not in the user-jwts tool. My rationale for this was that we would keep the surface area of the tool simpler than what the runtime could support. Users would still be able to support the multiple-keys-per-issuer if they set it up in config themselves, but the tool wouldn't support it.
I realize the incongruency is kinda weird though so maybe we should stick to one-key-per-issuer to keep things simple throughout the board (I want to avoid modifying too much of the existing handling in an RC1 PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize the incongruency is kinda weird though
Yeah, it's strange 😌, unless we have a scenario for multiple keys in the Runtime I don't feel like we should add it yet until both areas are updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've definitely seen examples of multiple keys from the same issuer being configured so leaving it in the runtime might be useful for now with the prospect of adding support to the tool later on.
This PR changes the schema for storing signing keys for the user-jwts tool in user secrets to support more modifications in the future. Currently, the signing key is stored in a schema that looks like this:
Where the key is stored relative to the issuer and the scheme name. The new format is as follows:
The
Issuer
is not a top-level unique identifier (only the schemes are) so it doesn't make sense to nest the key under this. The top-levelSigningKey
property gives us a place to add more options in the future (key algo and key modifications for key length).