dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.11k stars 9.91k forks source link

ASP.NET Core OIDC/JWT Handlers should not have a direct dependency on the AAD JWT Library #36175

Open leastprivilege opened 2 years ago

leastprivilege commented 2 years ago

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

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/1687#issuecomment-912905994

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1574

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1516

https://twitter.com/ycrumeyrolle/status/1431544530357075968

davidfowl commented 2 years ago

@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.

Tratcher commented 2 years ago

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.

davidfowl commented 2 years ago

Which is why I think a concrete API proposal would be a great starting point

brockallen commented 2 years ago

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 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.

kevinchalet commented 2 years ago

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 Validate* options in TokenValidationParameters are not even discussed with the community.

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 🤷🏻

davidfowl commented 2 years ago

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.

veikkoeeva commented 2 years ago

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 .NET should be the owner and not ASP.NET. Noting also that web safe Base64 encoder in .NET could be nice.

davidfowl commented 2 years ago

Maybe relevant to think if .NET should be the owner and not ASP.NET. Noting also that web safe Base64 encoder in .NET could be nice.

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?

blowdart commented 2 years ago

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 -

  1. We can view a jwt as made up of three things, the header, the payload and the signature, all of which are part of a single string. Whilst in general most folks care about the claims in the payload and nothing more, it would make sense an optiolnal results object to contain all three sections of the token, so we'd end up with JwtValidationResult class which might be returned from a Parse method, which also includes a boolean of IsValid()
  2. Users may want to know why a parsed JWT is invalid, so, like X509Validation, a collection of JwtValidationErrors could be useful.
  3. ASP.NET takes claims and turns them into a principal which is treated like a User. But JWT can be used to encapsulate more than just a user. one question is would it be up to ASP.NET to transform the validation results into a Principal from the claims collection, or does that belong in the Validator/Parser?
  4. Do we want to go further, and allow creation of Jwts from the base classes? That becomes more complicated, because if we allow creation, do we support producing the right type of metadata endpoint contents for key publication etc,.? Personally, I'd say no to this, we'd want to go as simple as possible here, and creation of new tokens is beyond simple.
  5. Finally, do we create our own lightweight parser/validation in runtime to sit alongside the rest of the security classes? If so that has risks around getting it right and patching, and down-level support (would we package it as a nuget package for older .NET versions - by which I mean 6.0, rather than, say, Framework). We'd want to consider where its boundaries lie, given that the associated standards around jwt keep growing and branching. I'm not sure we'd want to play catchup.

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.

veikkoeeva commented 2 years ago

@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 new byte[] in somewhere in the guts of a library that has sensitive bytes there may make people, say, in payments, wonder if it's worth coming up mitigations or do something else (as that code shows, I'm mitigating it with prototyping a sensitive boundary wrapped, so that could consider TPMs, AES encryptinh memory, whatever). Maybe performance related issues too there.

I concur about that encoding. Goes to show there are many issues within many issues.

blowdart commented 2 years ago

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.

kevinchalet commented 2 years ago

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.

veikkoeeva commented 2 years ago

@blowdart Maybe I don't care about SecureString that much. I think I have seen those discussion somewhere in GH. :)

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.

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 Microsoft.IdentityModel.Tokens is how easy is it to plug in Azure HSM (maybe it's all working nicely these days and I used missed it). Maybe a good abstraction could use them too.

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 mlocked pages need to be used (on a separate server maybe, but can be a desktop too, as part of some regulated system). We can agree it may not be a splendid idea. Or that using a custom memory pools are too easy to use wrongly. Or maybe using the TPM RNG for some operations (or maybe just something like here Having separate public and secret RNGs for different purposes (IVs vs. session keys etc) sounds like an excellent defense in depth mitigation.). But it would be nice too if things like these could be more easily useable.

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.

Tratcher commented 2 years ago

Some points on logistics:

Thoughts?

kevinchalet commented 2 years ago

The OpenIdConnect and JwtBearer packages are not part of the shared framework due to the various risks of the external dependency.

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.

blowdart commented 2 years ago

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.

kevinchalet commented 2 years ago

If that's your end goal then here is not the place to discuss how other teams should govern themselves.

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?

Given that identity model is intimately linked with AAD, it's not something .NET should own either.

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.

And what is up for discussion is not an abstraction over identity model, but something much simpler in terms of abstraction, jwt parsing,

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.

blowdart commented 2 years ago

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.

kevinchalet commented 2 years ago

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.

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.

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.

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 EncryptingCredentials/SigningCredentials/SecurityKey types.

blowdart commented 2 years ago

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,

    services.AddContosoJwtParser(options =>
        options => options.SigningCredentials = ....)

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.

kevinchalet commented 2 years ago

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.

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.

If we go back to JSON like approach,

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 OpenIdConnectProtocolValidator. If it was something up to the "JWT implementation" to do, how would that be better than just creating your own ASP.NET Core authentication handler?

(since you mentioned MVC, it's interesting to note that while MVC uses an object property, the read/write JSON HttpContext extensions introduced more recently are tied to System.Text.Json and don't use object).

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... 🤷🏻

brentschmaltz commented 2 years ago

@kevinchalet thanks for including the IdentityModel team on this thread. There seems to be an overstatement that the IM is simply servicing AAD and as a secondary function providing 3p support. It is actually the reverse. The project (post WCF) was first shipped with Katana and then was a part of the .NET release cycle. After a couple of releases, we thought that we could move faster by decoupling IM from .NET release. There are times when AAD specific features need to be added to IM, but we endeavor to make them off by default as we have internal wrappers that set the functionality needed. We take feedback, we are dropping the obsolete PR on TokenValidationParameters.

That said it appears there is some disconnect as the comments on this thread make that apparent. The IM team is committed to supporting 3p's and are staffed to do so.

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. That is confusing.

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). I would love to see some proposals for what the community wants.

kevinchalet commented 2 years ago

We take feedback, we are dropping the obsolete PR on TokenValidationParameters.

Do you mean the Validate* switches deprecation? If so, that's great to hear! Such changes should always come with an impact study to ensure they are justified and affect 3rd party projects reasonably (which was not the case, AFAICT).

There are times when AAD specific features need to be added to IM, but we endeavor to make them off by default as we have internal wrappers that set the functionality needed.

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.

The IM team is committed to supporting 3p's and are staffed to do so.

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)

brentschmaltz commented 2 years ago

@kevinchalet thanks for the feedback. Yes, the Validate* deprecation PR will not go through as a result of feedback from you, brock and leastpriviledge and others.

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. This time we worked with @Tratcher to make sure we had the control in place.,

You are correct that we have much better funding now.

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.

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. It also plugs into the asp.net as the authentication layer. It solves a number of interesting problems, we are factoring out the 1p's specific features and will be introducing the model this year.

What is the best way to move forward so that we get the right product for everyone?

leastprivilege commented 2 years ago

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

davidfowl commented 2 years ago

The issue/API needs a champion and it needs to be filed in the right place. Otherwise this will just be a discussion issue.

leastprivilege commented 2 years ago

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.

blowdart commented 2 years ago

@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.

brentschmaltz commented 2 years ago

(I mean the ridiculousness of claim mapping defaults is another good example). All efforts should go into their new JsonWebTokenHandler -

@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

JWT and ODIC. The only place that uses them right now is ASP.NET,

There is wider use of IM then just ASP.NET

it was a mistake from day 1 that the ASP.NET Core team did not invest in their own OIDC client plumbing.

The OIDC middleware for asp.net was developed with the asp.net team.

I just wish Wilson would stop changing their APIs and keep the package more stable and reliable

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.

leastprivilege commented 2 years ago

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

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.

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

brentschmaltz commented 2 years ago

@leastprivilege in general i agree with you, we need this conversation. In general, the IM team wants our work to be tied to standards. Our early work was influenced by Ws and we have had an interesting transition moving away from Ws designs while still maintaining some support for SAML tokens, dsig and WsFed.

You should start with writing JSON/JWT round-tripping tests, if they would exist, some of the problems could have been caught before release.

We have developed a system using a home grown IdentityComparer that helps us ensure the round-trip is as expected see:
Tests

What i was encouraging is that users can add some important tests for their scenarios that we add to our release process.

switch to JsonWebToken/JsonWebTokenHandler

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.

remove all AAD specific features from the core library, e.g. remove all telemetry

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

PureKrome commented 2 years ago

While we can't remove this, we can make sure that it is opt-in.

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.

brentschmaltz commented 2 years ago

@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. By reviewing the telemetry and mapping to the application id, one can inform application owners. It's not always simple to know what version of libraries applications are using.

leastprivilege commented 2 years ago

By reviewing the telemetry and mapping to the application id, one can inform application owners. It's not always simple to know what version of libraries applications are using.

Sure - but this is not part of the RFC/spec - this is an AAD-specific product feature. Make it an extensibility point.

brentschmaltz commented 2 years ago

@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? For example, the change to JsonClaimSet using System.Json.Text is a big change, we want to make sure the dev community has opportunity to help us get it right. First step is for us to publish a design with intent, i suppose.

leastprivilege commented 2 years ago

I always push back against any work that is not per spec (unless we all agree, lax on receive for example)

That this is even needed is part of the problem. What is "lax on receive"?

There are other voices that have influence, currently i do have some influence and will try and hold the line.

If you cannot get "management buy-in" that the standards parts and AAD must be separate. We are back to square 1.

We have developed a system using a home grown IdentityComparer

That seems to test at the claims level - thats good - but first of all you need to test at the JSON text level.

How can we get folks involved in big issues? First step is for us to publish a design with intent, i suppose.

Yes. Open an issue. Write a proposal. Document the change and API usage. Then we can discuss. Feel free to tag me.

brentschmaltz commented 2 years ago

@leastprivilege (been on vacation :-) "lax on receive" can be simply we accept "BEArer", "beaRer" for the protocol name. Or more complicated, if a SAML id is not a UUID but still matches the reference id, we will use it.

"management but-in" management is bought into respecting standards. We cannot remove features, such as what we have in telemetry. When we need special feature such as when we added this for some internal needs, https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/commit/be1c621755848b38ca3222f52d01465fef9b3189 we did it in such a way that is is opt-in. I also what to note, internal 1ps will have to write some unnatural code to make it work.

"proposal" - yes that is needed and will help us create a better product.

leastprivilege commented 2 years ago

Any progress on this?

brentschmaltz commented 2 years ago

@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 Anyone else that wants to be tagged, please add a comment here.

leastprivilege commented 2 years ago

Are we making any progress on the criticism in this thread?

"lax on receive" can be simply we accept "BEArer", "beaRer" for the protocol name.

Are you serious?

Tbh - maybe there should be an issue in the JWT handler repo to continue the discussion.

brentschmaltz commented 2 years ago

@leastprivilege

"lax on receive" ... Yeah, serious about this, i don't think it's an interesting conversation.

We have made progress on using System.Text.Json and we are investing in separating AAD requirements into its own layer.

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

leastprivilege commented 1 year ago

Just stumbled over this - over one year later.

Maybe I am wrong - but it seems the situation has not improved in any way...