Open drusellers opened 2 years ago
hi! sorry for the delay in the response! my main question would be if you're encoding or decoding? just to make sure, but I guess decoding, otherwise it doesn't make too much sense. Here's what's supported today:
and then
and finally
What means the decoder automatically will create an algorithm based on the alg
in the header.
An issue is though that the DI extensions project doesn't have a great API so doesn't support well registering the JWT classes for encoding and decoding in the same time. I tried to overcome it in #378.
What's the version of the library you're using?
Wait! Your question isn't about choosing the algorithm but the key based on the kid
in the header.
What if you provide a custom factory:
public sealed class KeyedAlgorithmFactory : JwtAlgorithmFactory
{
private readonly Func<string, X509Certificate2> _certSelector;
public KeyedAlgorithmFactory(Func<string, X509Certificate2> certSelector)
{
_certSelector = certSelector;
}
public override IJwtAlgorithm Create(JwtDecoderContext context)
{
var kid = context?.Header?.KeyId;
var cert = _certSelector(kid);
return new RS256Algorithm(cert); // or somehow else to choose the right algo
}
}
or something more generic:
public sealed class KeyedRSAlgorithmFactory : RSAlgorithmFactory // note the change in the base class
{
private readonly Func<string, X509Certificate2> _certSelector;
public KeyedRSAlgorithmFactory(Func<string, X509Certificate2> certSelector)
{
_certSelector = certSelector;
}
public override IJwtAlgorithm Create(JwtDecoderContext context)
{
var kid = context?.Header?.KeyId;
var cert = _certSelector(kid);
var algorithm = context?.Header?.Algorithm;
return base.CreateAlgorithm(algorithm, cert); // method doesn't exist today, the base class needs refactoring
}
}
If any of these works - then we can ship it with the library. Help to come up with a better name though.
@drusellers ping. Did you try any of the above? Does it work, does it not?
@abatishchev sorry I switched tasks, but reading the code this looks like exactly what I need. I'll implement it later today and put some thoughts towards naming - config - etc.
Ok, so that was great. It's looking like I'll be working with something like
namespace Authentication.Jwt;
using System.Security.Cryptography.X509Certificates;
using JWT;
using JWT.Algorithms;
public class KeyedRSAlgorithmFactory : IAlgorithmFactory
{
readonly Func<string, X509Certificate2?> _certSelector;
public KeyedRSAlgorithmFactory(Func<string, X509Certificate2?> certSelector)
{
_certSelector = certSelector;
}
public IJwtAlgorithm Create(JwtDecoderContext context)
{
var key = context?.Header?.KeyId;
if (key == null)
throw new InvalidOperationException("No KEY provided in the 'kid' header");
var cert = _certSelector(key);
if (cert == null)
throw new InvalidOperationException($"No certificate found for key '{key}'");
var algorithm = context?.Header?.Algorithm;
if (algorithm == null)
throw new InvalidOperationException("No algorithm was found in the 'alg' header");
var algorithmName = (JwtAlgorithmName)Enum.Parse(typeof(JwtAlgorithmName), algorithm);
return algorithmName switch
{
JwtAlgorithmName.RS256 => new RS256Algorithm(cert),
JwtAlgorithmName.RS384 => new RS384Algorithm(cert),
JwtAlgorithmName.RS512 => new RS512Algorithm(cert),
JwtAlgorithmName.RS1024 => new RS1024Algorithm(cert),
JwtAlgorithmName.RS2048 => new RS2048Algorithm(cert),
JwtAlgorithmName.RS4096 => new RS4096Algorithm(cert),
_ => throw new NotSupportedException($"{algorithm} not supported by {nameof(KeyedRSAlgorithmFactory)}")
};
}
}
Then I'm planning to build a registration that looks like this:
services.AddSingleton<ICertificateStore, CertificateStore>();
services.AddSingleton<IAlgorithmFactory>(provider =>
{
var store = provider.GetRequiredService<ICertificateStore>();
return new KeyedRSAlgorithmFactory(store.GetCertificate);
});
Looks promising! Few suggestions:
x ?? throw new Exception(...);
to shorten the checksclass CertificateStoreRSAlgorithmFactory : KeyedRSAlgorithmFactory
{
public CertificateStoreRSAlgorithmFactory(ICertificateStore store) : base(store.GeCertificate)
{}
}
??
I'll add those in.I'll post back here once the code has some wear and tear on it, then I'll look to submit a PR back. :)
then I'll look to submit a PR back
Awesome, thank you!
Ok, running into the null context issue again - for context it's this line of code - https://github.com/jwt-dotnet/jwt/blob/main/src/JWT/JwtEncoder.cs#L49
var store = new FileSystemCertificateStore();
var factory = new KeyedRSAlgorithmFactory(store.GetCertificate);
IJwtEncoder encoder = new JwtEncoder(factory, new JsonNetSerializer(), new JwtBase64UrlEncoder());
var extraHeaders = new Dictionary<string, object>
{
{ "kid", "test-cert" }
};
var payload = new Dictionary<string, object>
{
{ "uid", "bob" }
};
// key is only used for symmetric encryption
encoder.Encode(extraHeaders, payload, Array.Empty<byte>());
When I call Encode on the JwtEncoder it then does
// my factory needs a proper context
var algorithm = _algFactory.Create(null); // throws since its null
Sorry, just got back from a vacation.
Yes, the problem is that the algorithm factory that is used for decoding can't be used for encoding. Decoding has context (e.g. from the underlying HTTP request), encoding does not. Instead the latter should be driven by the configuration, simply put - the algorithm needs to be hard-coded.
Sorry for the troubles you're facing with the library. And appreciate your input and feedback, as you've uncovered a previously unused code path (hence purely designed).
In other words, I don't know how to make DI container friendly two factories (classes) that implement the same interface. An only (and rough) idea that comes to my mind is to have two new interfaces that would implement the current one. So each can be registered with a different implementation.
@abatishchev absolutely no worries.
Yes, the problem is that the algorithm factory that is used for decoding can't be used for encoding. Decoding has context (e.g. from the underlying HTTP request), encoding does not. Instead the latter should be driven by the configuration, simply put - the algorithm needs to be hard-coded.
Oh, this just kinda sank in for me. Ok, now I'm tracking as to the why. :D
Sorry for the troubles you're facing with the library. And appreciate your input and feedback, as you've uncovered a previously unused code path (hence purely designed).
Thank you for your patience in explaining things, and for even providing the library. :D
In other words, I don't know how to make DI container friendly two factories (classes) that implement the same interface. An only (and rough) idea that comes to my mind is to have two new interfaces that would implement the current one. So each can be registered with a different implementation.
Now that I better understand the constraint, I'll see what my caffeine addled brain comes up with, and let you know. Otherwise, it might be that I use the Builder Model for the encoding. I'll at least share with you the solution that I came up with.
hey @drusellers, how this one going?
Working on it today. Just getting all of my cert generation buttoned up and automated - and then I'll be testing my setup. I should have an update later today. :)
Ok, now that I have everything working, and I think a bit better understanding here is what I ended up doing.
So the decoding process works great, and and all of my work is really about working through the encoding side.
for decoding I have a KeyedRSAlgorithmFactory
that looks up the key based on the kid
in the headers. This is working well so far. I want to add some logging and just bake it a bit more and then I'm happy to share the code but in the end its pretty much what you recommended.
for encoding I have a ConfiguredRSAlgorithmFactory
that leans on the configuration to select the key and algorithm to use for signing things. Rather than use the IJwtEncoder
interface, I introduced my own JwtGenerator
that brings together the ConfiguredRSAlgorithmFactory
, the JwtConfiguration
class (which has the Algo and Key Id).
What is the purpose of the byte[]
key in the interfaces? It's for symmetric encryption right? How do we make this work for asymmetric encryption now that there is a compiler warning encouraging people to select one.
What if this had an JwtEncoderContext
similar to a JwtDecoderContext
. The encoder context could store the extra headers, the key data, and other items important to encoding - like ALGO selection etc.
Looking at the encodingFactory.Create you take a JwtDecoderContext
but I need to generate the Algo for use in Encoding as well. Maybe the AlgorithmFactory has two methods - Create(JwtDecodingContext cxt)
and Create(JwtEncodingContext cxt)
. (just the idea not the actual method names).
Sorry for a slow response, working on two things in parallel (my primary job is cool but high-demanding) requires context switching and it's hard to me.
What is the purpose of the byte[] key in the interfaces? It's for symmetric encryption right?
Primarily yes. But also it works as public key for asymmetric algorithms but that part is a magic to me. Or maybe not, need to look into the code. But if you wake me up in the middle of the night, I won't be able to explain :)
How do we make this work for asymmetric encryption now that there is a compiler warning encouraging people to select one.
I think we should drop it from the interface. The current version is 10.0.0-betaX, i made some breaking changes recently so it'd be perfect timing to make more drastic changes.
I've declared HMAC algorithms obsolete but I don't want to remove them from the library altogether, as there might be (legacy) users still.
What if this had an JwtEncoderContext similar to a JwtDecoderContext. The encoder context could store the extra headers, the key data, and other items important to encoding - like Algo selection etc.
Yes!
Looking at the
EncodingFactory.Create()
you take aJwtDecoderContext
but I need to generate the Algo for use in Encoding as well. Maybe the AlgorithmFactory has two methods -Create(JwtDecodingContext cxt)
andCreate(JwtEncodingContext cxt)
.
Yes! We might keep old Create()
, as a workaround and for back-compatibility, maybe temporally until next version and/or while the API design is being polished.
@abatishchev no worries - you have been very responsive and I'm very much appreciate your time. :)
I would be game to mark some the methods supporting symmetric encryption
as Obsolete
to further encourage the use of newer methods that better support asymmetry
. I'm also happy to help contribute a section on key generation and how I'm working through that and supporting it. I def don't know all of the best practices around this, so I'm looking forward to learning from others. :D
Ok, I am building up my ASP.Net API Application to use the RS1024 algorithm.
This is working, I can generate and consume the JWTs.
A year goes by, and now I want to rotate those keys. Reading through JWT it looks like I can use the
kid
header to give the keys id's which can be used for looking up the right cert. However I'm not sure how I would ever get access to the JWT header (or thekid
) in order to select the right X509. I'm probably missing something pretty obvious.The DelegateFactory and others do take a
Func<IJwtAlgorithm>
but thekid
isn't passed down.I see
WithSecret
is used with symmetric algo's - this makes me think I might be missing something about asymmetric algo's. 🤔A
WithKeys
would be helpful and could be used in theAddJwt
call too.I'm sure I'm missing something obvious, so any help would be appreciated. Also, if I've asked in the wrong forum, please let me know and I can move this conversation there.