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

Default 'sub' claim mapping does not resolve to ClaimsPrincipal.Identity.Name #415

Closed
bgever opened this Issue May 26, 2016 · 19 comments

Comments

Projects
None yet
7 participants
@bgever
Copy link
Contributor

bgever commented May 26, 2016

To process a JWT, the API consumer is going to use most likely the JwtSecurityTokenHandler.ValidateToken method, to get a ClaimsPrincipal.

RFC7519 states for the registered 'sub' claim (emphasis mine):

The "sub" (subject) claim identifies the principal that is the
subject of the JWT
. The claims in a JWT are normally statements
about the subject. The subject value MUST either be scoped to be
locally unique in the context of the issuer or be globally unique.
The processing of this claim is generally application specific. The
"sub" value is a case-sensitive string containing a StringOrURI
value. Use of this claim is OPTIONAL.

The ClaimsPrincipal object returned from the ValidateToken method allows to identify the subject via its default interface with the Identity.Name property only. There are no other default properties that expose a principal identity unless you'd query the claims.

Because the default mapping of the claims controlled with ClaimTypeMapping links sub to http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier, it does not match with the claim http://schemas.xmlsoap.org/ws/2005/05/identity/claims/name used to query the Name property.

Therefore, when a JWT has the sub claim only, there is no easy API to access its value.

The default mapping should be changed to http://schemas.xmlsoap.org/ws/2005/05/identity/claims/name, so that ClaimsPrincipal.Identity.Name resolves to the sub claim.

@brentschmaltz brentschmaltz added this to the RTM milestone May 26, 2016

@bgever

This comment has been minimized.

Copy link
Contributor Author

bgever commented May 27, 2016

Two workarounds for the current versions (tested with 5.0 RC2):

  1. Clear the mapping table, and set the NameClaimType on the TokenValidationParameters:
var params = new TokenValidationParameters { NameClaimType = "sub" };
var jwtHandler = new JwtSecurityTokenHandler();
jwtHandler.InboundClaimTypeMap.Clear();
SecurityToken token;
ClaimsPrincipal principal = jwtHandler.ValidateToken(serializedJwt, params, out token);
string sub = principal.Identity.Name;
  1. Override the default mapping table to use the Name claim type rather than NameIdentifier:
var params = new TokenValidationParameters();
var jwtHandler = new JwtSecurityTokenHandler();
jwtHandler.InboundClaimTypeMap[JwtRegisteredClaimNames.Sub] = ClaimTypes.Name;
SecurityToken token;
ClaimsPrincipal principal = jwtHandler.ValidateToken(serializedJwt, params, out token);
string sub = principal.Identity.Name;

The first one is more "true" to the original JWT claims, as none of the claims will expand to "long" versions.
Second one is the quickest operation, since only one claim type mapping needs to be overridden, and the ClaimsPrincipal will use default claim type to look up the name.

Obviously, I still prefer to see the default behavior changed. 😄

@leastprivilege

This comment has been minimized.

Copy link
Contributor

leastprivilege commented May 28, 2016

sub is not the name of a user. It's the unique user id.

@bgever

This comment has been minimized.

Copy link
Contributor Author

bgever commented May 30, 2016

What's the framework's recommended way of retrieving the sub (i.e. unique user id) from the ClaimsPrincipal object?

string sub = principal.FindFirst(JwtRegisteredClaimNames.Sub)?.Value;

This would not work, because by default the claims are converted to their long values. That's a framework feature, but not something I would expect when I'm just parsing a JWT.

So the following would work, but this behavior isn't clear to someone calling the API:

string sub = principal.FindFirst(ClaimTypes.NameIdentifier)?.Value;

The workarounds I've posted before work for me, or I could even query the claim as here, but I wonder how other consumers of the API could discover this implementation specific behavior?

@leastprivilege

This comment has been minimized.

Copy link
Contributor

leastprivilege commented May 30, 2016

I personally think it is evil that the JWT handler converts the standard claim types to the Microsoft favoured ones.

you can turn that globally off by setting the static DefaultInboundClaimTypeMap property on the JWT handler.

After that - yes do a FindFirst on sub.

@bgever

This comment has been minimized.

Copy link
Contributor Author

bgever commented May 30, 2016

The JwtSecurityTokenHandler should allow to be constructed without a mapping table (instead of requiring to override it), what could be done with a flag in the constructor.

@leastprivilege

This comment has been minimized.

Copy link
Contributor

leastprivilege commented May 30, 2016

If you construct the handler yourself you can clear the instance InboundClaimTypeMap property.

Yea - not a fan at all - but thats the way it is - legacy.

@bgever

This comment has been minimized.

Copy link
Contributor Author

bgever commented May 30, 2016

Yes, that's what I do in my first workaround above. 😄

@bgever

This comment has been minimized.

Copy link
Contributor Author

bgever commented May 31, 2016

I guess the way to go, is to add a constructor overload to the JwtSecurityTokenHandler, where the value of the InboundClaimTypeMap can be specified with an argument. When it's null, then the field will be set to an empty map. The constructor can then specify why the map may be needed (i.e. identity processing with "full" claim names), so that the consumer of the API knows what is happening under the hood.

So it would look something like this:

var params = new TokenValidationParameters();
var jwtHandler = new JwtSecurityTokenHandler(null); //TODO
SecurityToken token;
ClaimsPrincipal principal = jwtHandler.ValidateToken(serializedJwt, params, out token);
string sub = principal.FindFirst(JwtRegisteredClaimNames.Sub)?.Value;

More than happy to make a PR if that's appreciated.

@brentschmaltz

This comment has been minimized.

Copy link
Member

brentschmaltz commented Jun 2, 2016

@bgever @leastprivilege it seems you are both suggesting that mapping should be OFF by default.
Correct?
The legacy was the result of normalizing between SAML and JWT tokens so an authorization provider would not need to see the difference.

@brockallen

This comment has been minimized.

Copy link

brockallen commented Jun 2, 2016

IIRC we asked for the mapping to be off by default a long time ago. People using the JWT library are all targeting OIDC where those claims types are specified and it's burdensome to keep seeing XML namespaces for the legacy WS-* claim types.

So in short we do see the difference today in Katana because we have to explicitly disable the mapping in every API and every client app. It'd be nice to not have to explain why claims in your app don't match claims that we see in the JWT.

@polita polita modified the milestones: Backlog, RTM Jun 2, 2016

@leastprivilege

This comment has been minimized.

Copy link
Contributor

leastprivilege commented Jun 3, 2016

There will be always some sort of mapping going on - but the JWT handler is the wrong place to do that IMO. That's application logic.

So yes - I still opt for turning it off (now's the last chance)

@bgever

This comment has been minimized.

Copy link
Contributor Author

bgever commented Jun 3, 2016

Agreed, the mapping should be off, it is unexpected application logic as @leastprivilege said.

I guess the reason why it is happening in the JWT handler, is that the ClaimsPrincipal which is returned from ValidateToken, is created with a mapping. This logic is defined in TokenValidationParameters.CreateClaimsIdentity. It is possible to override the behavior, but the mapping table suits the default claim types for name and role. Relating to why I opened this issue in the first place.

@leastprivilege

This comment has been minimized.

@leastprivilege

This comment has been minimized.

@bgever

This comment has been minimized.

Copy link
Contributor Author

bgever commented Jul 22, 2016

@brentschmaltz I think there's now sufficient evidence that this is confusing behavior and should be fixed. Note my earlier suggestion of adding the mapping table as an argument to the constructor, also accepting a null value to initiate without mappings.

@brentschmaltz brentschmaltz removed the Fix 5.x label Feb 7, 2017

@brentschmaltz brentschmaltz modified the milestones: 5.x Release, Backlog Feb 7, 2017

@brentschmaltz brentschmaltz modified the milestones: 5.2.0, 5.x Release Aug 24, 2017

@brentschmaltz brentschmaltz removed their assignment Nov 10, 2017

@mafurman

This comment has been minimized.

Copy link
Member

mafurman commented Dec 14, 2017

@bgever This is legacy code, and making changes to the default mapping would likely break a lot of existing apps that depend on the JwtSecurityTokenHandler.

When we go async with the JsonWebTokenHandler, there will not be any mapping happening by default.

@mafurman mafurman closed this Dec 14, 2017

@bgever

This comment has been minimized.

Copy link
Contributor Author

bgever commented Dec 15, 2017

@mafurman Could you elaborate "When we go async with the JsonWebTokenHandler" a bit more please?

@khellang

This comment has been minimized.

Copy link

khellang commented May 9, 2018

This continues to bite people in 2018... aspnet/Mvc#7760

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