-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
ASP.NET Core OIDC/JWT Handlers should not have a direct dependency on the AAD JWT Library #36175
Comments
@leastprivilege would you like to write up an API proposal for these APIs? That will help kick start the process and also help us better understand the scope of the work. |
The current auth handlers are very thin wrappers around the IdentityModel libraries and have little additional functionality. What benefit do you get from abstracting the IdentityModel layer vs providing alternative auth handlers? I expect you'd want different types in the options which isn't something that abstracts well. |
Which is why I think a concrete API proposal would be a great starting point |
I think the concern is that the current JWT library that's mainly for AAD has a poor track record of quality, consistency, and breaking changes, and it would be nice to be able to not use it. |
As @leastprivilege and @brockallen said, the main problem is that IdentityModel is exclusively driven by AAD's needs, with close to zero consideration for the third-party projects that depend on it (e.g IdentityServer or OpenIddict). Sensitive things like AAD-specific telemetry being added to the metadata retrieval feature or the deprecation of the That said, I'm not sure this situation is specific to the IdentityModel project: lately, even the ASP.NET team hasn't really been interested in gathering feedback from community members/projects prior to introducing new changes. The situation even got worse with the adoption of the "monorepo" approach, that makes monitoring topics you're interested in - and thus sending spontaneous feedback - almost impossible 🤷🏻 |
Thanks for that feedback @kevinchalet, if this issue turns into more than just an API proposal then I'm inclined to move it to the discussions milestone. |
I concur. I tried the JWT tokens quickly as a prototype at https://github.com/lumoin/Verifiable and removing soon all the last traces. Mostly also my concerns are recorded here or in the links. Maybe relevant to think if |
I was thinking the same thing. It's possible this should just be an API proposal on the dotnet/runtime repo. I'm not sure it needs to be an abstraction then, maybe it should just be a core part of the libraries that implement the spec and use System.Text.Json as the implementation. @leastprivilege does that sound like a good idea? If so, can you close this issue and create an issue on dotnet/runtime with the API proposal template? |
I've been mulling this since the proposed changes from AAD last week, thinking about it in terms of how we abstracted json away and provided a reference implementation. At its bare minimum what is needed is just JwtValidator/Parser with a single method Validate/Parse, which returns the information needed. But I don't think that simple an implementation is flexible enough -
Configuration of the actual parser should be opaque to any parsing functions, so a parser can decide on all those contentious things, like claims mapping, metadata retrieval, refreshing of cached keys etc. Keeping the actual mechanics opaque would allow for alternative classes to start supporting things the community has wanted for quite a while like encrypted JWTs without consumers having to change code, but should we look at some base events, or callbacks much like Json.Net did or does that fall to a consumer like asp.net to expose? As for @veikkoeeva's comment on encoding, that's separate, I'd suggestion that becomes an issue in the runtime repo all its own and linked to the existing encoding classes. Anyway, these are random thoughts, dropping from my head over the last week, and gathered on an early Sunday as I drink the first coffee of the day, so they're by no means anything well designed, or complete, but thoughts for further discussion. |
@blowdart Let I do the same on my evening coffee... I could add one more and that is flexibility of choosing one's library to sign, validate and encrypt. The AAD one doesn't make it easy while also not providing much options either (one could say the hierarcy feels fairly rigid). It would be nice to have have a clear way to choose one's preferred library due to concerns such as:
I have currently a shim (test showing here, also will be properly named and not just tuples at some point) for NSec, BouncyCastle and at some point for .NET and TPM ones too. And looking how to best do some of this JWT stuff. Maybe I'd like to make a note, partially related to hardware that having a I concur about that encoding. Goes to show there are many issues within many issues. |
Oh dear lord let's not reinvent secure string 😂 Jwt creation is interesting. I'm not sure it's a key scenario for a lot of folks, that tends to be for token servers rather than a mainline scenario for the majority of users. I'd worry that would overcomplicate a parsing abstraction. |
|
@blowdart Maybe I don't care about
I think I understand. Let I persist a little while still. As this is a hopefully bit more free-wheeling discussion, maybe this is a good place make a note that if someone somewhere is thinking something, a strict class hierarchy that fixes internally everything to .NET platform offering may exclude people building on top abstractions that perhaps could have a bit more breathing room. This is a separate issue from parsing, token creation, validation and those for sure. But this fixing was one of the problems I experienced when I thought that maybe I could reuse the AAD abstractions and plug something else there. Maybe one example still concerning even users of Specifically concerning the library I'm building: It seem that AAD owns the MS DID/VC libraries too, while not having a .NET library for that (hence I'm working on one, likely need in the future :)). When someone in MS is considering to write one, maybe it'd be nice if it'd be useable in higher trust/regulated scenarios. Some more fringe but useable considerations... Besides, say, using HSMs, someone somewhere may mandate that I think I don't have much more to tell here (well, a plead of hands ands minds to https://github.com/microsoft/TSS.MSR :D) and I don't want to steal the thread. Maybe sufficient to note this is a pain point to other than the OP and the usual well known .NET names in the space. The perspective may be a bit different. There are people who have in passing mentioned building something like https://keylime.dev/ or things related to card payments handling, but are thinking if the .NET environment is too MS centric and fixed to what MS provides and so it's just too much of an effort to work around issues. Besides the already mentioned one, I would like to build maybe something into Orleans grains with the idea of semi-autonomous AI agents but using good abstractions (esp. if regulated, and that doesn't include cybersecurity and digital markets ones coming down too, we probably need to add docs for those later!). Of course this is anedoctal. |
Some points on logistics:
Thoughts? |
This says a lot about IdentityModel's governance when even the ASP.NET team considers IM too risky to have packages that depend on it in the shared framework. Building an abstraction on top of IM would certainly help advanced users opt out of IM, but it wouldn't solve the root causes we mentioned earlier, as IM would very likely stay the default implementation. At this point, IM's governance is still a central point. A few concrete proposals that could help improve things:
It's definitely something that should be discussed with @jmprieur and @brentschmaltz. |
Introducing an "advisory council" similar to what was suggested for DI (excellent idea, BTW), where members of the ASP.NET team and external people could help the IM team make the right choices on sensitive design decisions. If that's your end goal then here is not the place to discuss how other teams should govern themselves. Given that identity model is intimately linked with AAD, it's not something .NET should own either. We tried that with the WS* stuff before, and it did not work well. And what is up for discussion is not an abstraction over identity model, but something much simpler in terms of abstraction, jwt parsing, As for solving what you see as the root problem, let's note that it is advanced users that started this discussion and want the opt-out. I would posit that the vast majority of users don't care, they just want working ODIC and JWT, and how they get it doesn't matter. |
It's more about discussing how such a critical library could stop being exclusively driven by AAD's needs with no consideration for the impact on external projects. If it's not here, what's the right place to discuss that?
That's the problem: JWT and OIDC are open standards and it's regrettable their implementation in .NET is tied to Azure. IdentityModel - that was originally part of the .NET Framework BTW, in case you don't remember - fills a critical role in .NET and it's not clear to me why the JWT/JWS/JWE implementation couldn't be owned by .NET.
It's so much more than just "parsing", it's the whole validation stuff you'll need to abstract and make extensible enough to be as usable as the current implementation. |
You're conflating two issues. AAD having their own JWT library that's driven by AAD's needs (external and internal), and the desire to swap it with something else that isn't driven by AAD's needs. We don't need to abstract validation away if parsing includes validation, it can be an opaque process, controlled by configuration of the implementation class, where its config is set at startup, or though the options patterns. Configuration and validation parameters don't need to be exposed to the end call site, after all your criticism of the Identity Model doesn't call for that. That feels quite leaky. If you wanted to add validation external to parsing you might end up with TryParse, TryValidate and that's it, the internal mechanisms don't matter to the caller, only the failures that result from failed validation or parsing. |
I'm not. It's just that the two are closely related: if IdentityModel didn't have this governance issue, I doubt anyone would complain. It's because of that that @leastprivilege suggested we should be able to switch to a different - vendor-neutral - implementation. There's clearly a cause and effect relationship here.
Unless you design your new stack to only support OIDC configuration discovery and don't allow tweaking the validation logic (what's the issuer that is considered valid? do we check the not before/expiration dates? what audiences are allowed, etc.), you'll need to make basic things configurable via an options-like class, potentially with delegates if more control is needed. Even essential things like registering signing or encryption keys will require designing new types if you don't want to expose IdentityModel's |
Even essential things like registering signing or encryption keys will require designing new types If rely on DI to pass it around in ASP.NET (yes, I realise that you may end up wanting it in everything from WinForms to Blazer), you don't need the types, it's still just config and/or delegates that can be unique to the actual implementation rather than abstracted away. If we go back to JSON like approach,
but is that not just sugar really? Just something prettier than adding an IJwtParser into DI for everything to resolve? The System.Text.Json.JsonSerializerOptions is specific to the built in Json classes, just like the SerializerSettings in json.net are specific to json.net - the actual options.SerializerSettings is, in MVC just an object, but it sort of looks like it isn't during config. And going back to what most people use JWTs for, it's parsing/validation still, that's the main use. Very few people run off manually to refresh keys or credentials, that's set at startup, and that's still specific to the actual parser. If the parser wants to run off and grab things from /wellknown, great, internal implementation detail, if it wants to read all the keys from Hashicorp Vault, again, internal implemtation detail. Parsing/Validation from a caller perspective should not need to care. |
Using DI would be problematic if you wanted to support multiple authentication handlers with different "parsers"/"validators" (MS' DI container doesn't natively support named services). You'd probably want services-as-options here.
At this point, if everything is implementation-specific instead of being abstracted and absolutely nothing can be configured without being coupled to a specific "JWT validator" implementation, what's the difference with just rolling your own authentication handler? Genuine question. BTW, you say "JWT parser/validator", but it's more than that: the OIDC handler also uses IdentityModel to validate critical parts of the OIDC flow via its (since you mentioned MVC, it's interesting to note that while MVC uses an Eventually, I find very sad the fact we're debating about potential designs when the original issue could have been solved by being just a little bit more reasonable... 🤷🏻 |
@kevinchalet thanks for including the IdentityModel team on this thread. That said it appears there is some disconnect as the comments on this thread make that apparent. We are missing some key abstractions and have somewhat of an uncoordinated options setting structure where for example, JwtBearerOptions has some validation settings that need to be set by reaching down to IM and some on the options. We are working on currently working on abstractions that allow for plugging in handling of different protocols and tokens (auth is not just JwtBearer or JwtTokens). |
Do you mean the
IMHO, AAD-specific features should never be implemented in IdentityModel (that must remain as vendor-neutral as possible): they should be implemented higher in the "Microsoft Identity Web" stack using the extensibility hooks offered by IdentityModel. As for making these AAD-specific features opt-in, sadly I don't think it's true:
These changes - made in IdentityModel's core - were exclusively designed for AAD (no other implementation supports the custom parameters/headers you use), they are not opt-in and they were not announced/discussed prior to being implemented. If @brockallen, @leastprivilege or I didn't keep an eye on the IdentityModel repo, we wouldn't even notice them: I'm afraid, it's just unacceptable.
I already said it elsewhere: the situation largely improved since 2015: back then, the IM team was clearly underfunded and didn't have the HR needed to maintain such a critical stack and I'm glad to see it's no longer true. I'm also very happy you merged the PRs I sent last year and took my feedback into consideration, it's clearly very positive. Still, the main problem remains and every time an AAD-specific thing lands in IdentityModel, the confidence 3rd-party developers have in IM is impacted... to the point where major projects would like to get rid of it. This needs to change. (BTW, this thread focuses on IM, but the "push AAD stuff everywhere" movement is certainly not specific to IM: even the SQL client has a hard dependency on MSAL, which is unacceptable too: https://twitter.com/kevin_chalet/status/1434565739898449927) |
@kevinchalet thanks for the feedback. I agree with you that our design strategy should be to make only the necessary changes to enable AAD required features possible and leave them off. The 2016 fix with telemetry was a case we don't want to repeat. You are correct that we have much better funding now.
We have developed an internal layer on top of IM that has multiple features for AAD and plan on keeping AAD specific features for 1p's in that area. What is the best way to move forward so that we get the right product for everyone? |
Unfortunately this thread went into too many different directions - let me try to consolidate... My initial reason for opening this issue was
Tbh - the biggest problems we had upgrading our code over the last ASP.NET Core versions was always Wilson. Again and again. But once I thought about it a bit more - the issue is much more complicated. In a nutshell, it was a mistake from day 1 that the ASP.NET Core team did not invest in their own OIDC client plumbing. There is no way to make all this pluggable in ASP.NET Core (at least in a feasible time frame) - OIDC discovery document parsing, OIDC protocol message creation and parsing, JWT parsing etc... it's all outsourced (and tightly coupled) So yes - it crossed my mind if JWT should be part of the BCL. I think it should. It should be a compact and concise implementation of the spec - no opinions. After all JWT is "just" a signature/encryption wrapper for JSON objects. But there is a lot of other complexities as well - JWS, JWE, JWK etc. These things don't exist isolated and must be somehow interoperable with the other OIDC plumbing. This is a multi-year effort and we in particular have "advanced" use-cases which will not be the 80/20 target of the next versions of .NET. Now looking at other JWT implementations - I am bit scared when I see the "hand rolled" crypto. I was very tempted to create my own JWT handler on top of the Wilson crypto primitives. But I do not have the time for that. So to conclude - I just wish Wilson would stop changing their APIs and keep the package more stable and reliable - and at the same time remove all that AAD specific stuff like e.g. telemetry (I mean the ridiculousness of claim mapping defaults is another good example). All efforts should go into their new JsonWebTokenHandler - and ASP.NET Core should switch to their new plumbing (again - this is not possible today). At the same time the BCL needs a JWT implementation going forward - one that is going through the high standards of security testing that I would expect from Microsoft. If that base library is done right - opinionated semantics could be added at the app level. ...and no - I don't think I will do an initial API proposal - but I am happy to be involved (in some capacity) in this effort ;) @davidfowl |
The issue/API needs a champion and it needs to be filed in the right place. Otherwise this will just be a discussion issue. |
ok - then feel free to close it. |
@davidfowl The right place is a discussion all to itself, given there are two things in one issue here, JWT and ODIC. The only place that uses them right now is ASP.NET, but, in much the same way we have a JSON parser, an actual implementation of a JWT is probably best outside of ASP.NET because no-one wants to bring in the weight of that into, say, a Winforms project. OIDC on the other hand may well be best off inside ASP.NET, until you think about mobile apps, but they way they handle logins is often through an embedded browser. I can certainly champion the issue, but it needs careful design, and that's a heavy weight for the community to do, both in terms of time, and money. A good start for asp.net may be moving the middleware to use the new abstractions that frankly I didn't even know existed, but that still leaves the abstractions in assemblies that move at a different pace to the framework. It could be worth investigation moving the abstrations out into asp.net and pivoting the middleware to use them as a starting point. If we have the abstractions nestled into asp.net they'll move at the same pace, and breaking changes would be reduced and come under a more semanticly versioned and governed space. |
@leastprivilege it's time to put the claim mapping to bed, beating that dead horse doesn't help. That was a different time and place, we acknowledge that decision caused issues and have workarounds. The new JsonWebTokenHandler did not follow that pattern
There is wider use of IM then just ASP.NET
The OIDC middleware for asp.net was developed with the asp.net team.
We do have strong api back-compat, but there are runtime concerns. A while back i suggested that we are open to the idea of users adding scenario tests that if broken would block the release until the scenario owner signs off. We didn't go very far with that, this may be a beneficial investment. I see frustration focusing on out of scope features such as telemetry and such items was internalizing Newtonsoft, which caught everyone by surprise. We need better communication and an IM release model that ensures external scenarios are not broken. |
I think sometimes discussions are better than inventing new APIs. Especially if they bring all the parties to the table. It sounds like everybody here on this thread wants to do the right thing. While JWT handling is very important, implementing it is a multi-year effort. The Wilson team has already 10 years of investment in that space. At the same time .NET and ASP.NET in particular needs a good and modern JWT/OIDC implementation that is not riddled by internal Microsoft management challenges. So let me summarize what I think is necessary going forward: The Wilson team
The ASP.NET team will
We did exactly this in IdentityServer because of the quality issues. We have several tests that just make sure the JSON still looks like what we expect...so yes, this is a good idea. You should start with writing JSON/JWT round-tripping tests, if they would exist, some of the problems could have been caught before release. Does this sound like a foundation for a new discussion? @brentschmaltz @blowdart @Tratcher @davidfowl |
@leastprivilege in general i agree with you, we need this conversation.
We have developed a system using a home grown IdentityComparer that helps us ensure the round-trip is as expected see: What i was encouraging is that users can add some important tests for their scenarios that we add to our release process.
This needs to be the long term goal. The IM team has identified some blockers that need to be implemented first. Some implementations leaked during our early development. Our performance runs are showing up to a 50% improvement.
The only item i am aware of is the telemetry. While we can't remove this, we can make sure that it is opt-in. We have internal layers that can turn it on for our 1p hosts. Asp.net can ensure it is off by default. Brent |
Can't or Don't want to ? I think people would really appreciate the clarification/confirmation. EDIT: Also, I'm not trolling - It's an honest question. |
@PureKrome We would like to if possible. I haven't studied the call graph yet to see if it is possible. It's actually very valuable when a an important improvement arises to know the exact version of a library being used. |
Sure - but this is not part of the RFC/spec - this is an AAD-specific product feature. Make it an extensibility point. |
@leastprivilege I always push back against any work that is not per spec (unless we all agree, lax on receive for example). There are other voices that have influence, currently i do have some influence and will try and hold the line. If there is a need for some odd behavior, we will need to mark the 'issue' and 'pr' in such a way that interested parties can chime in so there are no surprises. Currently i do not see any items on the docket, but we all have work to do and smoothing this out will benefit us all. How can we get folks involved in big issues? |
That this is even needed is part of the problem. What is "lax on receive"?
If you cannot get "management buy-in" that the standards parts and AAD must be separate. We are back to square 1.
That seems to test at the claims level - thats good - but first of all you need to test at the JSON text level.
Yes. Open an issue. Write a proposal. Document the change and API usage. Then we can discuss. Feel free to tag me. |
@leastprivilege (been on vacation :-) "management but-in" management is bought into respecting standards. "proposal" - yes that is needed and will help us create a better product. |
Any progress on this? |
@leastprivilege i believe we are on the same page. Current management want us to be spec compliant and put any AAD specific features as opt-in or in separate internal libraries. We are completing a proposal along with a proof-of-concept for improving the JsonWebToken so that it depends on System.Text.Json and working on providing a solution for asp.net developers to move off of JwtSecurityToken. I will tag @leastprivilege , @kevinchalet |
Are we making any progress on the criticism in this thread?
Are you serious? Tbh - maybe there should be an issue in the JWT handler repo to continue the discussion. |
We have made progress on using System.Text.Json and we are investing in separating AAD requirements into its own layer. |
Thanks for contacting us. We're moving this issue to the |
Just stumbled over this - over one year later. Maybe I am wrong - but it seems the situation has not improved in any way... |
Is your feature request related to a problem? Please describe.
Right now, ASP.NET Core has a direct dependency on the AAD JWT library (token handler, token validation parameters etc). This library is primarily driven by its sponsor - the AAD team.
There are more JWT options in .NET - being able to plug in a different JWT implementation would be beneficial for the .NET ecosystem.
Describe the solution you'd like
ASP.NET Core should own its main JWT validation abstractions and rather ship with an by-default integration with the preferred JWT library. If that is the "in-house" one - fine. But it should be possible to plug in different implementations - similar to the DI system.
Additional context
AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#1687 (comment)
AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#1574
AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#1516
https://twitter.com/ycrumeyrolle/status/1431544530357075968
The text was updated successfully, but these errors were encountered: