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

Stop transforming inbound/outbound claim types by default #550

Closed
leastprivilege opened this Issue Dec 15, 2016 · 15 comments

Comments

Projects
None yet
3 participants
@leastprivilege
Copy link
Contributor

leastprivilege commented Dec 15, 2016

I gave this feedback a couple of times - the JwtHandler is not the place for (automatic) claims transformation. This is either application logic or explicit opt-in behavior.

I want the exact claim types in the principal that got issued from my STS - and not MS proprietary ones.

@brentschmaltz

This comment has been minimized.

Copy link
Member

brentschmaltz commented Dec 15, 2016

@leastprivilege I agree with you in principal.
If we turn it off by default, we need to consider how ClaimsIdentity.Name and ClaimsPrincipal.IsInRole are affected. These apis use ClaimsIdentity.NameClaimType and ClaimsIdentity.RoleClaimType with are set to the mapped types.

A possible mitigation is:
clearing static JwtSecurityTokenHandler.DefaultInboundClaimTypeMap in global scope.

@leastprivilege

This comment has been minimized.

Copy link
Contributor Author

leastprivilege commented Dec 16, 2016

Why is this your concern? Anybody can set the RoleClaimType and NameClaimType when validating a token to match whatever they are expecting.

Both are legacy and shouldn't be used anymore anyways.

wrt statics - it's 2016.

@brentschmaltz

This comment has been minimized.

Copy link
Member

brentschmaltz commented Dec 21, 2016

My concern is back-compat. ClaimsIdentity.Name, which is tied to NameClaimType, could be null. This has caused null-refs in code that expects a non-null value.

These values are not static, but instance variables. It almost 2017 :-).

@leastprivilege

This comment has been minimized.

Copy link
Contributor Author

leastprivilege commented Dec 21, 2016

Again - why is this your concern?

There might be JWTs without a "name" claim (or something that maps to it) at all. Actually they are pretty common.

The same issue would exist.

@brockallen

This comment has been minimized.

Copy link

brockallen commented Dec 22, 2016

My concern is back-compat

With that? This is a new(er) technology, and no where in the JOSE specs do you see WS-* claim types recommended. Stop treating the developers like they are incompetent.

If you must make these decisions for developers, then put a layer elsewhere (at the app layer) to give you SOAP-flavored claim types, but don't mandate this on the rest of people by default. It's 2017 -- separation of concerns is recommended.

At some point you (and the developers consuming your stuff) need to evolve. This awful mindset of wanting to babysit your developers prohibits that. By forcing this, you're shoehorning people into a legacy mode of claim types. Then you have more backwards compat problems down the road.

@brentschmaltz

This comment has been minimized.

Copy link
Member

brentschmaltz commented Dec 29, 2016

The back-compat scenario where an AuthorizationDecisionPoint (internal or external) functions differently when S.IM.Tokens.Jwt generates different claims by default, is the concern. A comprehensive set of tests may catch such issues. It is doubtful that all issues would be shown through testing.

@brockallen

This comment has been minimized.

Copy link

brockallen commented Dec 29, 2016

I think the issue here is the difference in expectation of a JWT library. From my point of view this library should just do JWT semantics and nothing more. From your perspective you think it should do a lot more including interpreting claim types and doing other things related to authorization (as you just said). The disconnect is the source of the complaints.

@brentschmaltz

This comment has been minimized.

Copy link
Member

brentschmaltz commented Jan 3, 2017

@brockallen I don't think the library should do more than JWT semantics. The tension is related to a choice we made 5 years ago when WsFed and OAuth were running side-by-side. We thought normalizing the claim types would help users write a single Authorization Policy independent of the token type OR protocol. That is where the claim transformation of JWT claim types originated. Now, we have existing users that have written Authorization Policies based on the claims returned from ValidateToken. If we change those claim types by default, the authorization result could change. These leads me to suggesting a simple way to set the default OFF as a compromise.

@brockallen

This comment has been minimized.

Copy link

brockallen commented Jan 3, 2017

That's why this should be a brand new library for a new age, rather than carrying forward legacy design and choices.

@brentschmaltz brentschmaltz modified the milestones: 5.1.2, 5.1.1 point release with bug fixes Jan 4, 2017

@brentschmaltz

This comment has been minimized.

Copy link
Member

brentschmaltz commented Jan 6, 2017

@brockallen I think you are suggesting for Microsoft.IdentityModel.Tokens.Jwt that works in the new world and let System.IdentityModel.Tokens.Jwt function as it does for legacy.

@brentschmaltz

This comment has been minimized.

Copy link
Member

brentschmaltz commented Feb 16, 2017

@leastprivilege @brockallen I propose in the next minor release we add an easy off switch.
Then in the 2.0 refresh we remove the mapping code and leave it to higher levels or a pluggable delegate.

@brockallen

This comment has been minimized.

Copy link

brockallen commented Feb 16, 2017

Then in the 2.0

2.0, as in the same time-frame as 2.0 of .NET Core and ASP.NET Core?

@brentschmaltz

This comment has been minimized.

Copy link
Member

brentschmaltz commented Feb 18, 2017

@brentschmaltz

This comment has been minimized.

Copy link
Member

brentschmaltz commented May 3, 2017

Design note: JwtSecurityTokenHandler.CreateClaimsIdentity claims should use Claim.Clone() when creating the ClaimsIdentity with claims. This will work for all 'non-mapped' claims, as for those a new claim will need to be created with a different 'type'. This is important since JwtPayload.Claims is virtual and users could be creating custom claims.

@brentschmaltz

This comment has been minimized.

Copy link
Member

brentschmaltz commented Jun 14, 2018

Our new Microsoft.IdentityModel.Tokens will not transform and will work directly with JSON.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment