[API Proposal]: ClaimsIdentity to perform case-sensitive comparison

[pmaytak]

Changes to Approved API

We previously approved this but missed a few constructor overloads. These are the suggested changes to the approved API.

namespace System.Security.Claims
{
    public partial class ClaimsIdentity
    {
        public ClaimsIdentity(
            IIdentity? identity = null,
            IEnumerable<Claim>? claims = null,
            string? authenticationType = null,
            string? nameType = null,
            string? roleType = null,
            StringComparison stringComparison = StringComparison.OrdinalIgnoreCase);

+       public ClaimsIdentity(BinaryReader reader, StringComparison stringComparison);
+       public ClaimsIdentity(ClaimsIdentity other, StringComparison stringComparison);
    }
}

---

Original Proposal

Background and motivation

Certain means of representing and handling of claims, like JWT, treat claim names (i.e. types) in a case-sensitive manner. Current ClaimsIdentity class in .NET compares claim names in a case-insensitive way. The issue could arise, if some application creates a claims identity and multiple claims with names that differ in casing. When retrieving this claim, the user may end up getting either of those claims and inconsistent with their expectations. For example, if there are two claims, "ID, 1" and "Id, 2"; a request to get "Id" claim will return the first one, not the second one.

The goals of these proposed changes is to make sure the users do the right and secure thing; to ensure secure behavior by making case sensitivity an intentional choice when creating a ClaimsIdentity instance; to avoid breaking existing functionality by not changing the default behavior, while still offering a secure alternative.

An earlier issue #83128 was created with a similar proposal.

API Proposal

Updates ClaimsIdentity class.

• Adds StringComparison parameter to all relevant constructors.
  • This StringComparison value is defaulted to StringComparison.OrdinalIgnoreCase.
  • This StringComparison value is used in FindAll, FindFirst, HasClaim methods.
• Adds overloads for FindAll, FindFirst, HasClaim methods which accepts an additional StringComparison parameter.
• Adds Roslyn analyzer rule to suggest to use one of the above case-sensitive overloads.

Some examples of the above proposals:

// Use for claim name comparison only (not claim value)
private readonly StringComparison _stringComparison = StringComparison.OrdinalIgnoreCase;

/// <summary>
/// Initializes an instance of <see cref="ClaimsIdentity"/>.
/// </summary>
/// <param name="stringComparison">The <see cref="StringComparison"/> to use when comparing claim names.</param>
public ClaimsIdentity(StringComparison stringComparison)
    : this((IIdentity?)null, (IEnumerable<Claim>?)null, (string?)null, (string?)null, (string?)null)
{
    _stringComparison = stringComparison;
}

public virtual Claim? FindFirst(string type)
{
    ArgumentNullException.ThrowIfNull(type);

    foreach (Claim claim in Claims)
    {
        if (claim != null)
        {
            if (string.Equals(claim.Type, type, _stringComparison))
            {
                return claim;
            }
        }
    }

    return null;
}

public virtual IEnumerable<Claim> FindAll(string type)
{
    ArgumentNullException.ThrowIfNull(type);
    return Core(type);

    IEnumerable<Claim> Core(string type)
    {
        foreach (Claim claim in Claims)
        {
            if (claim != null)
            {
                if (string.Equals(claim.Type, type, _stringComparison))
                {
                    yield return claim;
                }
            }
        }
    }
}

public virtual bool HasClaim(string type, string value)
{
    ArgumentNullException.ThrowIfNull(type);
    ArgumentNullException.ThrowIfNull(value);

    foreach (Claim claim in Claims)
    {
        if (claim != null
                && string.Equals(claim.Type, type, _stringComparison)
                && string.Equals(claim.Value, value, StringComparison.Ordinal))
        {
            return true;
        }
    }

    return false;
}

API Usage

var caseSensitiveClaimsIdentity = new ClaimsIdentity(StringComparison.Ordinal);
var claim = caseSensitiveClaimsIdentity.FindFirst("claimName"); // Case-sensitive search

var caseInsensitiveClaimsIdentity = new ClaimsIdentity();
var claim = caseInsensitiveClaimsIdentity .FindFirst("claimName", StringComparison.Ordinal); // Case-sensitive search

Alternative Designs

• Make the current ClaimsIdentity class case-sensitive by default.
  • This is the most secure way (in light of certain case-sensitive protocols); however, it is a breaking change and is impractical due to case-insensitive classes derived from the current ClaimsIdentity and with a ClaimsIdentity being a more general purpose type in concept
• Let users use the existing FindAll, FindFirst, HasClaim methods with predicate overloads which can be customized to do a case-sensitive match.
  • This assumes the users are aware of the case-sensitivity issue and they know these overloads exist. The library should guide the users towards the best practices and optimal solutions, if possible, and not leave it to the user to figure out.
  • An analyzer suggestion can be added to use predicate overloads; however, it would require the users to add that bit of repetitive code everywhere.

Risks

• These changes increase the ClaimsIdentity API surface area, which could make the class more complex to use.
• It could add confusion if these case-sensitivity flags affect the add and remove operations (which currently operate by Claim object reference). Although, the same difference exists between find and add and remove operations today.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

[jeffhandley]

@bartonjs Please invite @pmaytak and @keegan-caruso to the API Review session for when this bubbles to the top of the queue and make sure it has my attention for me to attend as well. Thank you in advance.

[KalleOlaviNiemitalo]

I worry about how this change would affect libraries that run on both .NET Core and .NET Framework. New overloads for FindAll etc. could be polyfilled on .NET Framework but it would not be possible to construct a case-sensitive ClaimsIdentity on .NET Framework. So a library that constructs identities would have to decide whether to have different behaviour on different target frameworks or abstain from using the new constructors.

[bartonjs]

Video

• Instead of constructor overload explosions, we added a default-argument constructor to enable the new parameter.
• The new ctor parameter should not be exposed as a property (until there's a compelling reason) to avoid confusion where a derived type exists solely to add ordinal semantics but the property defaults to OrdinalIgnoreCase.
• The ctors should throw for the culture-sensitive options, at least until they are well understood in context.

namespace System.Security.Claims
{
    public partial class ClaimsIdentity
    {
        public ClaimsIdentity(
            IIdentity? identity = null,
            IEnumerable<Claim>? claims = null,
            string? authenticationType = null,
            string? nameType = null,
            string? roleType = null,
            StringComparison stringComparison = StringComparison.OrdinalIgnoreCase);
    }
}

[vcsjones]

I took a quick look at implementing this

(until there's a compelling reason)

The compelling reason (to me) is that a derived type has no way to respect the StringComparison that was passed in to the constructor. Two scenarios

1. A derived type has its own find methods helper, FindFirstByFavoriteCheeseFlavor, and they have no way to follow the string semantics that were passed in to the constructor.
2. A derived type overrides FindFirst or FindAll, which are virtual and the default implementations of these will use the string comparison. An overridden member will not be able to follow the same string comparison semantics because it is not exposed.