Add checks to protect the internal claims used by MIW. Ref: issue #2968 #3131
Conversation
Introduce InternalClaimDetectedException in Microsoft.Identity.Web and Microsoft.Identity.Web.OWIN namespaces. This exception is thrown when internal ID Token claims (UniqueTenantIdentifier, UniqueObjectIdentifier) are detected in the user's ID Token. Updated AppBuilderExtension.cs and MicrosoftIdentityWebAppAuthenticationBuilder.cs to check for these claims and throw the exception if found. The new exception class includes a property to store the invalid claim
|
@bgavrilMS / @jennyf19 / @jmprieur please review. Thank you! |
|
@microsoft-github-policy-service agree |
src/Microsoft.Identity.Web.OWIN/InternalClaimDetectedException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web.OWIN/Microsoft.Identity.Web.OWIN.xml
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web.OWIN/InternalClaimDetectedException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web.OWIN/InternalClaimDetectedException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilder.cs
Outdated
Show resolved
Hide resolved
|
LGTM, I only have minor comments and resolving @msbw2 's comments |
src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilder.cs
Show resolved
Hide resolved
Update the description based on PR review Co-authored-by: Bogdan Gavril <[email protected]>
|
@bgavrilMS @jmprieur @jennyf19 @pmaytak to be able to go forward, we just need your input on claims casing match (type+value/value only) and the changes are done. Thank you! |
I am fine with case insensitive. |
ClaimsType should never be treated as case insensitive |
|
I think claim names should be case sensitive at least as per JWT spec. IdentityModel also treats them as case sensstive. |
|
Read the issue, thought about this more. When we search for a claim to validate or use, then we should be case sensitive. This way we get the exact claim we're looking for. However, if our intent is to throw an exception if the user specified any variation of utid or uid, then we should use case insensitive approach to broaden the search. This way we ensure that the token only ever has utid and uid variations of claim names. |
| /// <param name="claimType">The claim type</param> | ||
| /// <param name="claimValue">The claim value</param> | ||
| /// <exception cref="AuthenticationException">The <see cref="ClaimsIdentity"/> contains internal claims that are used internal use by this library</exception> | ||
| private static void RejectInternalClaims(ClaimsIdentity identity, string claimType, string claimValue) |
There was a problem hiding this comment.
@DOMZE I am not clear on the intent, can you please clarify? Why are we comparing the claim value? My understanding is that we should just call FindFirst and if it returns something, then throw an exception because that claim is user-provided.
There was a problem hiding this comment.
It was suggested by @bgavrilMS that if an internal claim is found in the ID Token and the value of the claim in the ID Token matches what's in the client_info, then we shouldn't throw an exception as they match.
But happy to revisit if the team is not aligned.
Add checks to protect the users to not use internal claims used by the library
Summary of the changes (Less than 80 chars)
Description
Introduce InternalClaimDetectedException in Microsoft.Identity.Web and Microsoft.Identity.Web.OWIN namespaces. This exception is thrown when internal ID Token claims (UniqueTenantIdentifier, UniqueObjectIdentifier) are detected in the user's ID Token. Updated AppBuilderExtension.cs and MicrosoftIdentityWebAppAuthenticationBuilder.cs to check for these claims and throw the exception if found. The new exception class includes a property to store the invalid claim
Fixes #2968 (in this specific format)